diff --git a/controllers/alias.go b/controllers/alias.go index ce629e37756b..657b1c03477d 100644 --- a/controllers/alias.go +++ b/controllers/alias.go @@ -303,19 +303,23 @@ func (r *MachinePoolReconciler) SetupWithManager(ctx context.Context, mgr ctrl.M // ExtensionConfigReconciler reconciles an ExtensionConfig object. type ExtensionConfigReconciler struct { - Client client.Client - APIReader client.Reader - RuntimeClient runtimeclient.Client + Client client.Client + APIReader client.Reader + RuntimeClient runtimeclient.Client + PartialSecretCache cache.Cache + ReadOnly bool // WatchFilterValue is the label value used to filter events prior to reconciliation. WatchFilterValue string } -func (r *ExtensionConfigReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options, partialSecretCache cache.Cache) error { +func (r *ExtensionConfigReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options) error { return (&extensionconfigcontroller.Reconciler{ - Client: r.Client, - APIReader: r.APIReader, - RuntimeClient: r.RuntimeClient, - WatchFilterValue: r.WatchFilterValue, - }).SetupWithManager(ctx, mgr, options, partialSecretCache) + Client: r.Client, + APIReader: r.APIReader, + RuntimeClient: r.RuntimeClient, + PartialSecretCache: r.PartialSecretCache, + ReadOnly: r.ReadOnly, + WatchFilterValue: r.WatchFilterValue, + }).SetupWithManager(ctx, mgr, options) } diff --git a/controlplane/kubeadm/config/rbac/role.yaml b/controlplane/kubeadm/config/rbac/role.yaml index ba9d19f28edb..b861bb0f15db 100644 --- a/controlplane/kubeadm/config/rbac/role.yaml +++ b/controlplane/kubeadm/config/rbac/role.yaml @@ -11,6 +11,14 @@ rules: verbs: - create - patch +- apiGroups: + - "" + resources: + - namespaces + verbs: + - get + - list + - watch - apiGroups: - "" resources: @@ -90,3 +98,11 @@ rules: - patch - update - watch +- apiGroups: + - runtime.cluster.x-k8s.io + resources: + - extensionconfigs + verbs: + - get + - list + - watch diff --git a/controlplane/kubeadm/main.go b/controlplane/kubeadm/main.go index 2d6c992f2667..0576fa26ad8c 100644 --- a/controlplane/kubeadm/main.go +++ b/controlplane/kubeadm/main.go @@ -56,22 +56,30 @@ import ( controlplanev1beta1 "sigs.k8s.io/cluster-api/api/controlplane/kubeadm/v1beta1" controlplanev1 "sigs.k8s.io/cluster-api/api/controlplane/kubeadm/v1beta2" clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2" + runtimehooksv1 "sigs.k8s.io/cluster-api/api/runtime/hooks/v1alpha1" + runtimev1 "sigs.k8s.io/cluster-api/api/runtime/v1beta2" + "sigs.k8s.io/cluster-api/controllers" "sigs.k8s.io/cluster-api/controllers/clustercache" "sigs.k8s.io/cluster-api/controllers/crdmigrator" "sigs.k8s.io/cluster-api/controllers/remote" kubeadmcontrolplanecontrollers "sigs.k8s.io/cluster-api/controlplane/kubeadm/controllers" "sigs.k8s.io/cluster-api/controlplane/kubeadm/internal/etcd" kcpwebhooks "sigs.k8s.io/cluster-api/controlplane/kubeadm/webhooks" + runtimecatalog "sigs.k8s.io/cluster-api/exp/runtime/catalog" + runtimeclient "sigs.k8s.io/cluster-api/exp/runtime/client" "sigs.k8s.io/cluster-api/feature" controlplanev1alpha3 "sigs.k8s.io/cluster-api/internal/api/controlplane/kubeadm/v1alpha3" controlplanev1alpha4 "sigs.k8s.io/cluster-api/internal/api/controlplane/kubeadm/v1alpha4" "sigs.k8s.io/cluster-api/internal/contract" + internalruntimeclient "sigs.k8s.io/cluster-api/internal/runtime/client" + runtimeregistry "sigs.k8s.io/cluster-api/internal/runtime/registry" "sigs.k8s.io/cluster-api/util/apiwarnings" "sigs.k8s.io/cluster-api/util/flags" "sigs.k8s.io/cluster-api/version" ) var ( + catalog = runtimecatalog.New() scheme = runtime.NewScheme() setupLog = ctrl.Log.WithName("setup") controllerName = "cluster-api-kubeadm-control-plane-manager" @@ -94,6 +102,8 @@ var ( webhookCertDir string webhookCertName string webhookKeyName string + runtimeExtensionCertFile string + runtimeExtensionKeyFile string healthAddr string managerOptions = flags.ManagerOptions{} logOptions = logs.NewOptions() @@ -116,6 +126,10 @@ func init() { _ = controlplanev1.AddToScheme(scheme) _ = bootstrapv1.AddToScheme(scheme) _ = apiextensionsv1.AddToScheme(scheme) + _ = runtimev1.AddToScheme(scheme) + + // Register the RuntimeHook types into the catalog. + _ = runtimehooksv1.AddToCatalog(catalog) } // InitFlags initializes the flags. @@ -186,6 +200,12 @@ func InitFlags(fs *pflag.FlagSet) { fs.StringVar(&webhookKeyName, "webhook-key-name", "tls.key", "Webhook key name.") + fs.StringVar(&runtimeExtensionCertFile, "runtime-extension-client-cert-file", "", + "Path of the PEM-encoded client certificate to be used when calling runtime extensions.") + + fs.StringVar(&runtimeExtensionKeyFile, "runtime-extension-client-key-file", "", + "Path of the PEM-encoded client key to be used when calling runtime extensions.") + fs.StringVar(&healthAddr, "health-addr", ":9440", "The address the health endpoint binds to.") @@ -209,6 +229,9 @@ func InitFlags(fs *pflag.FlagSet) { // ADD CRD RBAC for CRD Migrator. // +kubebuilder:rbac:groups=apiextensions.k8s.io,resources=customresourcedefinitions,verbs=get;list;watch // +kubebuilder:rbac:groups=apiextensions.k8s.io,resources=customresourcedefinitions;customresourcedefinitions/status,verbs=update;patch,resourceNames=kubeadmcontrolplanes.controlplane.cluster.x-k8s.io;kubeadmcontrolplanetemplates.controlplane.cluster.x-k8s.io +// Add RBAC for ExtensionConfig controller and runtime client (intentionally does not include write permissions) +// +kubebuilder:rbac:groups=runtime.cluster.x-k8s.io,resources=extensionconfigs,verbs=get;list;watch +// +kubebuilder:rbac:groups="",resources=namespaces,verbs=get;list;watch func main() { InitFlags(pflag.CommandLine) @@ -437,6 +460,30 @@ func setupReconcilers(ctx context.Context, mgr ctrl.Manager) { setupLog.Error(err, "unable to create etcd logger") os.Exit(1) } + + var runtimeClient runtimeclient.Client + if feature.Gates.Enabled(feature.InPlaceUpdates) { + // This is the creation of the runtimeClient for the controllers, embedding a shared catalog and registry instance. + runtimeClient = internalruntimeclient.New(internalruntimeclient.Options{ + CertFile: runtimeExtensionCertFile, + KeyFile: runtimeExtensionKeyFile, + Catalog: catalog, + Registry: runtimeregistry.New(), + Client: mgr.GetClient(), + }) + + if err = (&controllers.ExtensionConfigReconciler{ + Client: mgr.GetClient(), + APIReader: mgr.GetAPIReader(), + RuntimeClient: runtimeClient, + ReadOnly: true, + WatchFilterValue: watchFilterValue, + }).SetupWithManager(ctx, mgr, concurrency(10)); err != nil { + setupLog.Error(err, "Unable to create controller", "controller", "ExtensionConfig") + os.Exit(1) + } + } + if err := (&kubeadmcontrolplanecontrollers.KubeadmControlPlaneReconciler{ Client: mgr.GetClient(), SecretCachingClient: secretCachingClient, @@ -446,6 +493,7 @@ func setupReconcilers(ctx context.Context, mgr ctrl.Manager) { EtcdCallTimeout: etcdCallTimeout, EtcdLogger: etcdLogger, RemoteConditionsGracePeriod: remoteConditionsGracePeriod, + //RuntimeClient: runtimeClient, // TODO(in-place): enable once we want to use it, also validate in SetupWithManager that RuntimeClient is set if feature gate is enabled. }).SetupWithManager(ctx, mgr, concurrency(kubeadmControlPlaneConcurrency)); err != nil { setupLog.Error(err, "unable to create controller", "controller", "KubeadmControlPlane") os.Exit(1) diff --git a/internal/controllers/extensionconfig/extensionconfig_controller.go b/internal/controllers/extensionconfig/extensionconfig_controller.go index f5c79a6d6ac2..a8296f0e396f 100644 --- a/internal/controllers/extensionconfig/extensionconfig_controller.go +++ b/internal/controllers/extensionconfig/extensionconfig_controller.go @@ -17,6 +17,7 @@ limitations under the License. package extensionconfig import ( + "bytes" "context" "fmt" "strings" @@ -56,23 +57,39 @@ const ( // Reconciler reconciles an ExtensionConfig object. type Reconciler struct { - Client client.Client - APIReader client.Reader - RuntimeClient runtimeclient.Client + Client client.Client + APIReader client.Reader + RuntimeClient runtimeclient.Client + PartialSecretCache cache.Cache + + // ReadOnly configures if the ExtensionConfig controller should write ExtensionConfig objects or only read them + ReadOnly bool + // WatchFilterValue is the label value used to filter events prior to reconciliation. WatchFilterValue string } -func (r *Reconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options, partialSecretCache cache.Cache) error { +func (r *Reconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options) error { if r.Client == nil || r.APIReader == nil || r.RuntimeClient == nil { return errors.New("Client, APIReader and RuntimeClient must not be nil") } + if r.ReadOnly && r.PartialSecretCache != nil { + return errors.New("PartialSecretCache must not be set if ReadOnly is true") + } + if !r.ReadOnly && r.PartialSecretCache == nil { + return errors.New("PartialSecretCache must be set if ReadOnly is false") + } predicateLog := ctrl.LoggerFrom(ctx).WithValues("controller", "extensionconfig") - err := ctrl.NewControllerManagedBy(mgr). + b := ctrl.NewControllerManagedBy(mgr). For(&runtimev1.ExtensionConfig{}). - WatchesRawSource(source.Kind( - partialSecretCache, + WithOptions(options). + WithEventFilter(predicates.ResourceHasFilterLabel(mgr.GetScheme(), predicateLog, r.WatchFilterValue)) + + if !r.ReadOnly { + // The watch on Secrets is only needed when reconciling caBundle (readOnly mode doesn't do that). + b.WatchesRawSource(source.Kind( + r.PartialSecretCache, &metav1.PartialObjectMetadata{ TypeMeta: metav1.TypeMeta{ Kind: "Secret", @@ -83,11 +100,10 @@ func (r *Reconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, opt r.secretToExtensionConfig, ), predicates.TypedResourceIsChanged[*metav1.PartialObjectMetadata](mgr.GetScheme(), predicateLog), - )). - WithOptions(options). - WithEventFilter(predicates.ResourceHasFilterLabel(mgr.GetScheme(), predicateLog, r.WatchFilterValue)). - Complete(r) - if err != nil { + )) + } + + if err := b.Complete(r); err != nil { return errors.Wrap(err, "failed setting up with a controller manager") } @@ -97,10 +113,11 @@ func (r *Reconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, opt // warmupRunnable will attempt to sync the RuntimeSDK registry with existing ExtensionConfig objects to ensure extensions // are discovered before controllers begin reconciling. - err = mgr.Add(&warmupRunnable{ + err := mgr.Add(&warmupRunnable{ Client: r.Client, APIReader: r.APIReader, RuntimeClient: r.RuntimeClient, + ReadOnly: r.ReadOnly, }) if err != nil { return errors.Wrap(err, "failed adding warmupRunnable to controller manager") @@ -109,7 +126,6 @@ func (r *Reconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, opt } func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { - var errs []error log := ctrl.LoggerFrom(ctx) // Requeue events when the registry is not ready. @@ -132,43 +148,47 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu return ctrl.Result{}, err } - // Copy to avoid modifying the original extensionConfig. - original := extensionConfig.DeepCopy() - - if isPaused, requeue, err := paused.EnsurePausedCondition(ctx, r.Client, nil, extensionConfig); err != nil || isPaused || requeue { - return ctrl.Result{}, err - } - // Handle deletion reconciliation loop. + // Note: This only unregisters the ExtensionConfig which is intentionally done even if the ExtensionConfig is paused. if !extensionConfig.DeletionTimestamp.IsZero() { return r.reconcileDelete(ctx, extensionConfig) } - // Inject CABundle from secret if annotation is set. Otherwise https calls may fail. - if err := reconcileCABundle(ctx, r.Client, extensionConfig); err != nil { - return ctrl.Result{}, err - } + // In readOnly mode only validate instead of reconciling CA bundle and running discovery. + if r.ReadOnly { + if conditions.IsTrue(extensionConfig, clusterv1.PausedCondition) { + return ctrl.Result{}, nil + } - // discoverExtensionConfig will return a discovered ExtensionConfig with the appropriate conditions. - discoveredExtensionConfig, err := discoverExtensionConfig(ctx, r.RuntimeClient, extensionConfig) - if err != nil { - errs = append(errs, err) - } + if err := validateExtensionConfig(extensionConfig); err != nil { + return ctrl.Result{}, errors.Wrapf(err, "failed to validate ExtensionConfig") + } - // Always patch the ExtensionConfig as it may contain updates in conditions or clientConfig.caBundle. - if err = patchExtensionConfig(ctx, r.Client, original, discoveredExtensionConfig); err != nil { - errs = append(errs, err) - } + // Register the ExtensionConfig if it is valid. + log.V(4).Info("Registering ExtensionConfig information into registry") + if err = r.RuntimeClient.Register(extensionConfig); err != nil { + return ctrl.Result{}, errors.Wrapf(err, "failed to register ExtensionConfig %s/%s", extensionConfig.Namespace, extensionConfig.Name) + } + } else { + // Preserve original, EnsurePausedCondition might bump observedGeneration of the Paused condition without requeuing. + original := extensionConfig.DeepCopy() - if len(errs) != 0 { - return ctrl.Result{}, kerrors.NewAggregate(errs) - } + if isPaused, requeue, err := paused.EnsurePausedCondition(ctx, r.Client, nil, extensionConfig); err != nil || isPaused || requeue { + return ctrl.Result{}, err + } - // Register the ExtensionConfig if it was found and patched without error. - log.V(4).Info("Registering ExtensionConfig information into registry") - if err = r.RuntimeClient.Register(discoveredExtensionConfig); err != nil { - return ctrl.Result{}, errors.Wrapf(err, "failed to register ExtensionConfig %s/%s", extensionConfig.Namespace, extensionConfig.Name) + extensionConfig, err := reconcileExtensionConfig(ctx, r.Client, r.RuntimeClient, original, extensionConfig) + if err != nil { + return ctrl.Result{}, errors.Wrapf(err, "failed to reconcile ExtensionConfig") + } + + // Register the ExtensionConfig if it was found and patched without error. + log.V(4).Info("Registering ExtensionConfig information into registry") + if err = r.RuntimeClient.Register(extensionConfig); err != nil { + return ctrl.Result{}, errors.Wrapf(err, "failed to register ExtensionConfig %s/%s", extensionConfig.Namespace, extensionConfig.Name) + } } + return ctrl.Result{}, nil } @@ -292,3 +312,61 @@ func splitNamespacedName(nameStr string) types.NamespacedName { } return types.NamespacedName{Namespace: nameStr[:splitPoint], Name: nameStr[splitPoint+1:]} } + +func validateExtensionConfig(extensionConfig *runtimev1.ExtensionConfig) error { + // Verify caBundle (a more complete validation would be too much effort here) + if len(extensionConfig.Spec.ClientConfig.CABundle) == 0 { + return errors.Errorf("caBundle is not set on ExtensionConfig %s", klog.KObj(extensionConfig)) + } + + // Verify discovery. + discoveredCondition := conditions.Get(extensionConfig, runtimev1.ExtensionConfigDiscoveredCondition) + switch { + case discoveredCondition == nil: + return errors.Errorf("%s condition not yet set on ExtensionConfig %s", runtimev1.ExtensionConfigDiscoveredCondition, klog.KObj(extensionConfig)) + case discoveredCondition.Status != metav1.ConditionTrue: + return errors.Errorf("%s condition on ExtensionConfig %s must have status: True (instead it has: %s)", runtimev1.ExtensionConfigDiscoveredCondition, klog.KObj(extensionConfig), discoveredCondition.Status) + case discoveredCondition.ObservedGeneration != extensionConfig.Generation: + return errors.Errorf("%s condition on ExtensionConfig %s must have observedGeneration: %d (instead it has: %d)", runtimev1.ExtensionConfigDiscoveredCondition, klog.KObj(extensionConfig), extensionConfig.Generation, discoveredCondition.ObservedGeneration) + } + + return nil +} + +func reconcileExtensionConfig(ctx context.Context, c client.Client, runtimeClient runtimeclient.Client, original, extensionConfig *runtimev1.ExtensionConfig) (*runtimev1.ExtensionConfig, error) { + // Inject CABundle from secret if annotation is set. Otherwise https calls may fail. + if err := reconcileCABundle(ctx, c, extensionConfig); err != nil { + return nil, err + } + if !bytes.Equal(original.Spec.ClientConfig.CABundle, extensionConfig.Spec.ClientConfig.CABundle) { + // Note: This is intentionally not using the patch helper as the patch helper does not propagate metadata.generation back. + // We want to have the current generation here because otherwise the condition set below would have an outdated observedGeneration. + if err := c.Patch(ctx, extensionConfig, client.MergeFrom(original)); err != nil { + return nil, errors.Wrapf(err, "failed to patch ExtensionConfig %s", klog.KObj(extensionConfig)) + } + // Update original so that patchExtensionConfig below does not try to patch caBundle again. + // Note: This means that we might lose observedGeneration bumps on the Paused condition, but: + // * in this code path the generation changed again anyway (so the bump would have been outdated) + // * the next reconcile that doesn't change caBundle will bump the observedGeneration of the Paused + // condition in patchExtensionConfig + original = extensionConfig.DeepCopy() + } + + var errs []error + // discoverExtensionConfig will return a discovered ExtensionConfig with the appropriate conditions. + extensionConfig, err := discoverExtensionConfig(ctx, runtimeClient, extensionConfig) + if err != nil { + errs = append(errs, err) + } + + // Note: Intentionally always patching ExtensionConfig even if discoverExtensionConfig failed. + if err := patchExtensionConfig(ctx, c, original, extensionConfig); err != nil { + errs = append(errs, err) + } + + if len(errs) > 0 { + return nil, kerrors.NewAggregate(errs) + } + + return extensionConfig, nil +} diff --git a/internal/controllers/extensionconfig/extensionconfig_controller_test.go b/internal/controllers/extensionconfig/extensionconfig_controller_test.go index 742922772e46..ab01a4c844b4 100644 --- a/internal/controllers/extensionconfig/extensionconfig_controller_test.go +++ b/internal/controllers/extensionconfig/extensionconfig_controller_test.go @@ -64,6 +64,11 @@ func TestExtensionReconciler_Reconcile(t *testing.T) { Catalog: cat, Registry: registry, }) + registryReadOnly := runtimeregistry.New() + runtimeClientReadOnly := internalruntimeclient.New(internalruntimeclient.Options{ + Catalog: cat, + Registry: registryReadOnly, + }) g.Expect(runtimehooksv1.AddToCatalog(cat)).To(Succeed()) @@ -72,6 +77,12 @@ func TestExtensionReconciler_Reconcile(t *testing.T) { APIReader: env.GetAPIReader(), RuntimeClient: runtimeClient, } + rReadOnly := &Reconciler{ + Client: env.GetAPIReader().(client.Client), + APIReader: env.GetAPIReader(), + RuntimeClient: runtimeClientReadOnly, + ReadOnly: true, + } caCertSecret := fakeCASecret(ns.Name, "ext1-webhook", testcerts.CACert) server, err := fakeSecureExtensionServer(discoveryHandler("first", "second", "third")) @@ -85,11 +96,7 @@ func TestExtensionReconciler_Reconcile(t *testing.T) { defer func() { g.Expect(env.CleanupAndWait(ctx, caCertSecret)).To(Succeed()) }() - // Create the ExtensionConfig. - g.Expect(env.CreateAndWait(ctx, extensionConfig)).To(Succeed()) - defer func() { - g.Expect(env.CleanupAndWait(ctx, extensionConfig)).To(Succeed()) - }() + t.Run("fail reconcile if registry has not been warmed up", func(*testing.T) { // Attempt to reconcile. This will be an error as the registry has not been warmed up at this point. res, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: util.ObjectKey(extensionConfig)}) @@ -99,13 +106,30 @@ func TestExtensionReconciler_Reconcile(t *testing.T) { }) t.Run("successful reconcile and discovery on ExtensionConfig create", func(*testing.T) { - // Warm up the registry before trying reconciliation again. + // Warm up the registry before trying reconciliation again (these are no-ops as ExtensionConfig doesn't exist yet). warmup := &warmupRunnable{ Client: env.GetAPIReader().(client.Client), APIReader: env.GetAPIReader(), RuntimeClient: runtimeClient, } g.Expect(warmup.Start(ctx)).To(Succeed()) + warmupReadOnly := &warmupRunnable{ + ReadOnly: true, + warmupTimeout: 3 * time.Second, // Use a short timeout so the test doesn't take too long + Client: env.GetAPIReader().(client.Client), + APIReader: env.GetAPIReader(), + RuntimeClient: internalruntimeclient.New(internalruntimeclient.Options{ + Catalog: cat, + Registry: registryReadOnly, + }), + } + g.Expect(warmupReadOnly.Start(ctx)).To(Succeed()) + + // Create the ExtensionConfig. + g.Expect(env.CreateAndWait(ctx, extensionConfig)).To(Succeed()) + t.Cleanup(func() { + g.Expect(env.CleanupAndWait(ctx, extensionConfig)).To(Succeed()) + }) // Reconcile the extension and assert discovery has succeeded (the first reconcile adds the Paused condition). _, err = r.Reconcile(ctx, ctrl.Request{NamespacedName: util.ObjectKey(extensionConfig)}) @@ -120,9 +144,40 @@ func TestExtensionReconciler_Reconcile(t *testing.T) { g.Expect(pausedCondition.ObservedGeneration).To(Equal(conf.Generation)) }).WithTimeout(10 * time.Second).WithPolling(100 * time.Millisecond).Should(Succeed()) + // Initially Reconcile on the read-only Reconciler should fail if ExtensionConfig has not been reconciled yet. + _, err = rReadOnly.Reconcile(ctx, ctrl.Request{NamespacedName: util.ObjectKey(extensionConfig)}) + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(Equal("failed to validate ExtensionConfig: caBundle is not set on ExtensionConfig ext1")) + + // Reconcile should succeed on the regular Reconciler. _, err = r.Reconcile(ctx, ctrl.Request{NamespacedName: util.ObjectKey(extensionConfig)}) g.Expect(err).ToNot(HaveOccurred()) + // Regular registry should have the handlers. + _, err = registry.Get("first.ext1") + g.Expect(err).ToNot(HaveOccurred()) + _, err = registry.Get("second.ext1") + g.Expect(err).ToNot(HaveOccurred()) + _, err = registry.Get("third.ext1") + g.Expect(err).ToNot(HaveOccurred()) + + // Read-only registry should not have the handlers yet + _, err = registryReadOnly.Get("first.ext1") + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(Equal("failed to get extension handler \"first.ext1\" from registry: handler with name \"first.ext1\" has not been registered")) + + // Reconcile should succeed on the read-only Reconciler now as well. + _, err = rReadOnly.Reconcile(ctx, ctrl.Request{NamespacedName: util.ObjectKey(extensionConfig)}) + g.Expect(err).ToNot(HaveOccurred()) + + // Read-only registry should have the handlers now as well. + _, err = registryReadOnly.Get("first.ext1") + g.Expect(err).ToNot(HaveOccurred()) + _, err = registryReadOnly.Get("second.ext1") + g.Expect(err).ToNot(HaveOccurred()) + _, err = registryReadOnly.Get("third.ext1") + g.Expect(err).ToNot(HaveOccurred()) + config := &runtimev1.ExtensionConfig{} g.Expect(env.GetAPIReader().Get(ctx, util.ObjectKey(extensionConfig), config)).To(Succeed()) @@ -143,13 +198,6 @@ func TestExtensionReconciler_Reconcile(t *testing.T) { g.Expect(v1beta2Conditions[0].Type).To(Equal(runtimev1.ExtensionConfigDiscoveredCondition)) g.Expect(v1beta2Conditions[0].Status).To(Equal(metav1.ConditionTrue)) g.Expect(v1beta2Conditions[0].Reason).To(Equal(runtimev1.ExtensionConfigDiscoveredReason)) - - _, err = registry.Get("first.ext1") - g.Expect(err).ToNot(HaveOccurred()) - _, err = registry.Get("second.ext1") - g.Expect(err).ToNot(HaveOccurred()) - _, err = registry.Get("third.ext1") - g.Expect(err).ToNot(HaveOccurred()) }) t.Run("Successful reconcile and discovery on Extension update", func(*testing.T) { @@ -183,6 +231,20 @@ func TestExtensionReconciler_Reconcile(t *testing.T) { _, err = r.Reconcile(ctx, ctrl.Request{NamespacedName: util.ObjectKey(extensionConfig)}) g.Expect(err).ToNot(HaveOccurred()) + _, err = rReadOnly.Reconcile(ctx, ctrl.Request{NamespacedName: util.ObjectKey(extensionConfig)}) + g.Expect(err).ToNot(HaveOccurred()) + + for _, registry := range []runtimeregistry.ExtensionRegistry{registry, registryReadOnly} { + _, err := registry.Get("first.ext1") + g.Expect(err).ToNot(HaveOccurred()) + _, err = registry.Get("third.ext1") + g.Expect(err).ToNot(HaveOccurred()) + + // Second should not be found in the registry, as it has been removed from the ExtensionServer. + _, err = registry.Get("second.ext1") + g.Expect(err).To(HaveOccurred()) + } + var config runtimev1.ExtensionConfig g.Expect(env.GetAPIReader().Get(ctx, util.ObjectKey(extensionConfig), &config)).To(Succeed()) @@ -201,24 +263,21 @@ func TestExtensionReconciler_Reconcile(t *testing.T) { g.Expect(v1beta2Conditions[0].Type).To(Equal(runtimev1.ExtensionConfigDiscoveredCondition)) g.Expect(v1beta2Conditions[0].Status).To(Equal(metav1.ConditionTrue)) g.Expect(v1beta2Conditions[0].Reason).To(Equal(runtimev1.ExtensionConfigDiscoveredReason)) - - _, err = registry.Get("first.ext1") - g.Expect(err).ToNot(HaveOccurred()) - _, err = registry.Get("third.ext1") - g.Expect(err).ToNot(HaveOccurred()) - - // Second should not be found in the registry: - _, err = registry.Get("second.ext1") - g.Expect(err).To(HaveOccurred()) }) t.Run("Successful reconcile and deregister on ExtensionConfig delete", func(*testing.T) { g.Expect(env.CleanupAndWait(ctx, extensionConfig)).To(Succeed()) _, err = r.Reconcile(ctx, ctrl.Request{NamespacedName: util.ObjectKey(extensionConfig)}) - g.Expect(env.Get(ctx, util.ObjectKey(extensionConfig), extensionConfig)).To(Not(Succeed())) - _, err = registry.Get("first.ext1") - g.Expect(err).To(HaveOccurred()) - _, err = registry.Get("third.ext1") - g.Expect(err).To(HaveOccurred()) + g.Expect(err).ToNot(HaveOccurred()) + _, err = rReadOnly.Reconcile(ctx, ctrl.Request{NamespacedName: util.ObjectKey(extensionConfig)}) + g.Expect(err).ToNot(HaveOccurred()) + + for _, registry := range []runtimeregistry.ExtensionRegistry{registry, registryReadOnly} { + g.Expect(env.Get(ctx, util.ObjectKey(extensionConfig), extensionConfig)).To(Not(Succeed())) + _, err = registry.Get("first.ext1") + g.Expect(err).To(HaveOccurred()) + _, err = registry.Get("third.ext1") + g.Expect(err).To(HaveOccurred()) + } }) } @@ -374,6 +433,73 @@ func Test_reconcileCABundle(t *testing.T) { } } +func Test_validateExtensionConfig(t *testing.T) { + tests := []struct { + name string + config *runtimev1.ExtensionConfig + wantErr bool + wantErrMessage string + }{ + { + name: "caBundle not set", + config: extensionConfig(nil), + wantErr: true, + wantErrMessage: "caBundle is not set on ExtensionConfig default/extensionconfig", + }, + { + name: "condition missing", + config: extensionConfig([]byte("caBundle")), + wantErr: true, + wantErrMessage: "Discovered condition not yet set on ExtensionConfig default/extensionconfig", + }, + { + name: "condition false", + config: extensionConfig([]byte("caBundle"), metav1.Condition{ + Type: runtimev1.ExtensionConfigDiscoveredCondition, + Status: metav1.ConditionFalse, + Reason: runtimev1.ExtensionConfigNotDiscoveredReason, + ObservedGeneration: 1, // ExtensionConfig has generation 1. + }), + wantErr: true, + wantErrMessage: "Discovered condition on ExtensionConfig default/extensionconfig must have status: True (instead it has: False)", + }, + { + name: "condition observedGeneration outdated", + config: extensionConfig([]byte("caBundle"), metav1.Condition{ + Type: runtimev1.ExtensionConfigDiscoveredCondition, + Status: metav1.ConditionTrue, + Reason: runtimev1.ExtensionConfigDiscoveredReason, + ObservedGeneration: 0, // ExtensionConfig has generation 1. + }), + wantErr: true, + wantErrMessage: "Discovered condition on ExtensionConfig default/extensionconfig must have observedGeneration: 1 (instead it has: 0)", + }, + { + name: "All good", + config: extensionConfig([]byte("caBundle"), metav1.Condition{ + Type: runtimev1.ExtensionConfigDiscoveredCondition, + Status: metav1.ConditionTrue, + Reason: runtimev1.ExtensionConfigDiscoveredReason, + ObservedGeneration: 1, // ExtensionConfig has generation 1. + }), + wantErr: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + err := validateExtensionConfig(tt.config) + if tt.wantErr { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(Equal(tt.wantErrMessage)) + } else { + g.Expect(err).ToNot(HaveOccurred()) + } + }) + } +} + func discoveryHandler(handlerList ...string) func(http.ResponseWriter, *http.Request) { handlers := []runtimehooksv1.ExtensionHandler{} for _, name := range handlerList { @@ -471,3 +597,21 @@ func fakeCAInjectionRuntimeExtensionConfig(namespace, name, annotationString, ca } return ext } + +func extensionConfig(caBundle []byte, conditions ...metav1.Condition) *runtimev1.ExtensionConfig { + return &runtimev1.ExtensionConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "extensionconfig", + Namespace: "default", + Generation: 1, + }, + Spec: runtimev1.ExtensionConfigSpec{ + ClientConfig: runtimev1.ClientConfig{ + CABundle: caBundle, + }, + }, + Status: runtimev1.ExtensionConfigStatus{ + Conditions: conditions, + }, + } +} diff --git a/internal/controllers/extensionconfig/warmup.go b/internal/controllers/extensionconfig/warmup.go index a6d5c7fb82b9..13a79087af50 100644 --- a/internal/controllers/extensionconfig/warmup.go +++ b/internal/controllers/extensionconfig/warmup.go @@ -44,6 +44,7 @@ type warmupRunnable struct { Client client.Client APIReader client.Reader RuntimeClient runtimeclient.Client + ReadOnly bool warmupTimeout time.Duration warmupInterval time.Duration } @@ -72,16 +73,16 @@ func (r *warmupRunnable) Start(ctx context.Context) error { ctx, cancel := context.WithTimeoutCause(ctx, r.warmupTimeout, errors.New("warmup timeout expired")) defer cancel() + var warmupErr error err := wait.PollUntilContextTimeout(ctx, r.warmupInterval, r.warmupTimeout, true, func(ctx context.Context) (done bool, err error) { - if err = warmupRegistry(ctx, r.Client, r.APIReader, r.RuntimeClient); err != nil { - log.Error(err, "ExtensionConfig registry warmup failed") - return false, nil + if warmupErr = r.warmupRegistry(ctx); warmupErr != nil { + log.Error(warmupErr, "ExtensionConfig registry warmup failed") + return false, nil //nolint:nilerr // Intentionally not returning the error here } return true, nil }) - if err != nil { - return errors.Wrapf(err, "ExtensionConfig registry warmup timed out after %s", r.warmupTimeout.String()) + return errors.Wrapf(warmupErr, "ExtensionConfig registry warmup timed out after %s", r.warmupTimeout.String()) } return nil @@ -89,48 +90,44 @@ func (r *warmupRunnable) Start(ctx context.Context) error { // warmupRegistry attempts to discover all existing ExtensionConfigs and patch their status with discovered Handlers. // It warms up the registry by passing it the up-to-date list of ExtensionConfigs. -func warmupRegistry(ctx context.Context, client client.Client, reader client.Reader, runtimeClient runtimeclient.Client) error { +func (r *warmupRunnable) warmupRegistry(ctx context.Context) error { log := ctrl.LoggerFrom(ctx) - var errs []error - extensionConfigList := runtimev1.ExtensionConfigList{} - if err := reader.List(ctx, &extensionConfigList); err != nil { + if err := r.APIReader.List(ctx, &extensionConfigList); err != nil { return errors.Wrapf(err, "failed to list ExtensionConfigs") } + var errs []error for i := range extensionConfigList.Items { extensionConfig := &extensionConfigList.Items[i] - original := extensionConfig.DeepCopy() log := log.WithValues("ExtensionConfig", klog.KObj(extensionConfig)) ctx := ctrl.LoggerInto(ctx, log) - // Inject CABundle from secret if annotation is set. Otherwise https calls may fail. - if err := reconcileCABundle(ctx, client, extensionConfig); err != nil { - errs = append(errs, err) - // Note: we continue here because if reconcileCABundle doesn't work discovery will fail as well. - continue - } - - extensionConfig, err := discoverExtensionConfig(ctx, runtimeClient, extensionConfig) - if err != nil { - errs = append(errs, err) - } - - // Always patch the ExtensionConfig as it may contain updates in conditions or clientConfig.caBundle. - if err = patchExtensionConfig(ctx, client, original, extensionConfig); err != nil { - errs = append(errs, err) + // In readOnly mode only validate instead of reconciling CA bundle and running discovery. + if r.ReadOnly { + if err := validateExtensionConfig(extensionConfig); err != nil { + errs = append(errs, errors.Wrapf(err, "failed to validate ExtensionConfig")) + } + } else { + // extensionConfig is equal to original here, but we have to deepcopy so that if extensionConfig is changed original is not changed. + original := extensionConfig.DeepCopy() + extensionConfig, err := reconcileExtensionConfig(ctx, r.Client, r.RuntimeClient, original, extensionConfig) + if err != nil { + errs = append(errs, errors.Wrapf(err, "failed to reconcile ExtensionConfig")) + continue + } + extensionConfigList.Items[i] = *extensionConfig } - extensionConfigList.Items[i] = *extensionConfig } - // If there was some error in discovery or patching return before committing to the Registry. - if len(errs) != 0 { + // If there was an error in discovery or patching return before committing to the registry. + if len(errs) > 0 { return kerrors.NewAggregate(errs) } - if err := runtimeClient.WarmUp(&extensionConfigList); err != nil { + if err := r.RuntimeClient.WarmUp(&extensionConfigList); err != nil { return err } diff --git a/internal/controllers/extensionconfig/warmup_test.go b/internal/controllers/extensionconfig/warmup_test.go index b6676f384325..de1ad6a61097 100644 --- a/internal/controllers/extensionconfig/warmup_test.go +++ b/internal/controllers/extensionconfig/warmup_test.go @@ -17,6 +17,7 @@ limitations under the License. package extensionconfig import ( + "context" "fmt" "testing" "time" @@ -24,8 +25,10 @@ import ( . "github.com/onsi/gomega" "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apiserver/pkg/admission/plugin/webhook/testcerts" utilfeature "k8s.io/component-base/featuregate/testing" + "sigs.k8s.io/controller-runtime/pkg/client" runtimehooksv1 "sigs.k8s.io/cluster-api/api/runtime/hooks/v1alpha1" runtimev1 "sigs.k8s.io/cluster-api/api/runtime/v1beta2" @@ -37,39 +40,42 @@ import ( ) func Test_warmupRunnable_Start(t *testing.T) { - g := NewWithT(t) utilfeature.SetFeatureGateDuringTest(t, feature.Gates, feature.ClusterTopology, true) utilfeature.SetFeatureGateDuringTest(t, feature.Gates, feature.RuntimeSDK, true) t.Run("succeed to warm up registry on Start", func(t *testing.T) { + g := NewWithT(t) + ns, err := env.CreateNamespace(ctx, "test-runtime-extension") g.Expect(err).ToNot(HaveOccurred()) caCertSecret := fakeCASecret(ns.Name, "ext1-webhook", testcerts.CACert) // Create the secret which contains the fake ca certificate. g.Expect(env.CreateAndWait(ctx, caCertSecret)).To(Succeed()) - defer func() { + t.Cleanup(func() { g.Expect(env.CleanupAndWait(ctx, caCertSecret)).To(Succeed()) - }() + }) cat := runtimecatalog.New() g.Expect(fakev1alpha1.AddToCatalog(cat)).To(Succeed()) + g.Expect(runtimehooksv1.AddToCatalog(cat)).To(Succeed()) registry := runtimeregistry.New() - g.Expect(runtimehooksv1.AddToCatalog(cat)).To(Succeed()) for _, name := range []string{"ext1", "ext2", "ext3"} { server, err := fakeSecureExtensionServer(discoveryHandler("first", "second", "third")) g.Expect(err).ToNot(HaveOccurred()) - defer server.Close() + t.Cleanup(func() { + server.Close() + }) extensionConfig := fakeExtensionConfigForURL(ns.Name, name, server.URL) extensionConfig.Annotations[runtimev1.InjectCAFromSecretAnnotation] = caCertSecret.GetNamespace() + "/" + caCertSecret.GetName() // Create the ExtensionConfig. g.Expect(env.CreateAndWait(ctx, extensionConfig)).To(Succeed()) - defer func(namespace, name, url string) { - g.Expect(env.CleanupAndWait(ctx, fakeExtensionConfigForURL(namespace, name, url))).To(Succeed()) - }(ns.Name, name, server.URL) + t.Cleanup(func() { + g.Expect(env.CleanupAndWait(ctx, fakeExtensionConfigForURL(ns.Name, name, server.URL))).To(Succeed()) + }) } r := &warmupRunnable{ @@ -81,28 +87,88 @@ func Test_warmupRunnable_Start(t *testing.T) { }), } - if err := r.Start(ctx); err != nil { - t.Error(err) + g.Expect(r.Start(ctx)).To(Succeed()) + + validateExtensionConfigsAndRegistry(ctx, g, env.GetAPIReader(), registry) + }) + + t.Run("succeed to warm up registry on Start (with read-only warmup runnable)", func(t *testing.T) { + g := NewWithT(t) + + ns, err := env.CreateNamespace(ctx, "test-runtime-extension") + g.Expect(err).ToNot(HaveOccurred()) + + caCertSecret := fakeCASecret(ns.Name, "ext1-webhook", testcerts.CACert) + // Create the secret which contains the fake ca certificate. + g.Expect(env.CreateAndWait(ctx, caCertSecret)).To(Succeed()) + t.Cleanup(func() { + g.Expect(env.CleanupAndWait(ctx, caCertSecret)).To(Succeed()) + }) + + cat := runtimecatalog.New() + g.Expect(fakev1alpha1.AddToCatalog(cat)).To(Succeed()) + g.Expect(runtimehooksv1.AddToCatalog(cat)).To(Succeed()) + + registry := runtimeregistry.New() + registryReadOnly := runtimeregistry.New() + + for _, name := range []string{"ext1", "ext2", "ext3"} { + server, err := fakeSecureExtensionServer(discoveryHandler("first", "second", "third")) + g.Expect(err).ToNot(HaveOccurred()) + t.Cleanup(func() { + server.Close() + }) + extensionConfig := fakeExtensionConfigForURL(ns.Name, name, server.URL) + extensionConfig.Annotations[runtimev1.InjectCAFromSecretAnnotation] = caCertSecret.GetNamespace() + "/" + caCertSecret.GetName() + + // Create the ExtensionConfig. + g.Expect(env.CreateAndWait(ctx, extensionConfig)).To(Succeed()) + t.Cleanup(func() { + g.Expect(env.CleanupAndWait(ctx, fakeExtensionConfigForURL(ns.Name, name, server.URL))).To(Succeed()) + }) } - list := &runtimev1.ExtensionConfigList{} - g.Expect(env.GetAPIReader().List(ctx, list)).To(Succeed()) - g.Expect(list.Items).To(HaveLen(3)) - for i, config := range list.Items { - // Expect three handlers for each extension and expect the name to be the handler name plus the extension name. - handlers := config.Status.Handlers - g.Expect(handlers).To(HaveLen(3)) - g.Expect(handlers[0].Name).To(Equal(fmt.Sprintf("first.ext%d", i+1))) - g.Expect(handlers[1].Name).To(Equal(fmt.Sprintf("second.ext%d", i+1))) - g.Expect(handlers[2].Name).To(Equal(fmt.Sprintf("third.ext%d", i+1))) - conditions := config.GetV1Beta1Conditions() - g.Expect(conditions).To(HaveLen(1)) - g.Expect(conditions[0].Status).To(Equal(corev1.ConditionTrue)) - g.Expect(conditions[0].Type).To(Equal(runtimev1.RuntimeExtensionDiscoveredV1Beta1Condition)) + r := &warmupRunnable{ + Client: env.GetClient(), + APIReader: env.GetAPIReader(), + RuntimeClient: internalruntimeclient.New(internalruntimeclient.Options{ + Catalog: cat, + Registry: registry, + }), + } + + rReadOnly := &warmupRunnable{ + ReadOnly: true, + warmupTimeout: 3 * time.Second, // Use a short timeout so the test doesn't take too long + Client: env.GetClient(), + APIReader: env.GetAPIReader(), + RuntimeClient: internalruntimeclient.New(internalruntimeclient.Options{ + Catalog: cat, + Registry: registryReadOnly, + }), } + + // Initially warmup should fail if ExtensionConfigs have not been reconciled yet. + err = rReadOnly.Start(ctx) + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(Equal("ExtensionConfig registry warmup timed out after 3s: [" + + "failed to validate ExtensionConfig: caBundle is not set on ExtensionConfig ext1, " + + "failed to validate ExtensionConfig: caBundle is not set on ExtensionConfig ext2, " + + "failed to validate ExtensionConfig: caBundle is not set on ExtensionConfig ext3]")) + + // This will reconcile ExtensionConfigs. + g.Expect(r.Start(ctx)).To(Succeed()) + + // Now warmup should work. + g.Expect(rReadOnly.Start(ctx)).To(Succeed()) + + validateExtensionConfigsAndRegistry(ctx, g, env.GetAPIReader(), registry) + validateExtensionConfigsAndRegistry(ctx, g, env.GetAPIReader(), registryReadOnly) }) t.Run("fail to warm up registry on Start with broken extension", func(t *testing.T) { + g := NewWithT(t) + // This test should time out and throw a failure. ns, err := env.CreateNamespace(ctx, "test-runtime-extension") g.Expect(err).ToNot(HaveOccurred()) @@ -181,3 +247,31 @@ func Test_warmupRunnable_Start(t *testing.T) { } }) } + +func validateExtensionConfigsAndRegistry(ctx context.Context, g Gomega, c client.Reader, registry runtimeregistry.ExtensionRegistry) { + extensionRegistrationList, err := registry.List(runtimecatalog.GroupHook{Group: fakev1alpha1.GroupVersion.Group, Hook: "FakeHook"}) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(extensionRegistrationList).To(HaveLen(9)) + g.Expect(extensionRegistrationList).To(ContainElements( + HaveField("Name", "first.ext1"), HaveField("Name", "second.ext1"), HaveField("Name", "third.ext1"), + HaveField("Name", "first.ext2"), HaveField("Name", "second.ext2"), HaveField("Name", "third.ext2"), + HaveField("Name", "first.ext3"), HaveField("Name", "second.ext3"), HaveField("Name", "third.ext3"), + )) + + list := &runtimev1.ExtensionConfigList{} + g.Expect(c.List(ctx, list)).To(Succeed()) + g.Expect(list.Items).To(HaveLen(3)) + for i, config := range list.Items { + // Expect three handlers for each extension and expect the name to be the handler name plus the extension name. + handlers := config.Status.Handlers + g.Expect(handlers).To(HaveLen(3)) + g.Expect(handlers[0].Name).To(Equal(fmt.Sprintf("first.ext%d", i+1))) + g.Expect(handlers[1].Name).To(Equal(fmt.Sprintf("second.ext%d", i+1))) + g.Expect(handlers[2].Name).To(Equal(fmt.Sprintf("third.ext%d", i+1))) + + conditions := config.GetConditions() + g.Expect(conditions).To(HaveLen(1)) + g.Expect(conditions[0].Status).To(Equal(metav1.ConditionTrue)) + g.Expect(conditions[0].Type).To(Equal(runtimev1.ExtensionConfigDiscoveredCondition)) + } +} diff --git a/main.go b/main.go index 99a67fa50fe3..f8056b6efc17 100644 --- a/main.go +++ b/main.go @@ -620,11 +620,12 @@ func setupReconcilers(ctx context.Context, mgr ctrl.Manager, watchNamespaces map if feature.Gates.Enabled(feature.RuntimeSDK) { if err = (&controllers.ExtensionConfigReconciler{ - Client: mgr.GetClient(), - APIReader: mgr.GetAPIReader(), - RuntimeClient: runtimeClient, - WatchFilterValue: watchFilterValue, - }).SetupWithManager(ctx, mgr, concurrency(extensionConfigConcurrency), partialSecretCache); err != nil { + Client: mgr.GetClient(), + APIReader: mgr.GetAPIReader(), + RuntimeClient: runtimeClient, + PartialSecretCache: partialSecretCache, + WatchFilterValue: watchFilterValue, + }).SetupWithManager(ctx, mgr, concurrency(extensionConfigConcurrency)); err != nil { setupLog.Error(err, "Unable to create controller", "controller", "ExtensionConfig") os.Exit(1) }