diff --git a/docs/controllers/secretsync.md b/docs/controllers/secretsync.md index 6aa1dd1ac..2fd404beb 100644 --- a/docs/controllers/secretsync.md +++ b/docs/controllers/secretsync.md @@ -2,7 +2,7 @@ ## Overview -[Secret sync controller](../../pkg/controllers/secretsync/secret_sync_controller.go) is responsible for syncing `worker-user-data` secret that is created by installer in `openshift-machine-api` namespace. The secret is used to store ignition configuration data for worker nodes. +[Secret sync controller](../../pkg/controllers/secretsync/secret_sync_controller.go) is responsible for syncing `worker-user-data` and `master-user-data` secrets created by the installer in the `openshift-machine-api` namespace. These secrets store ignition configuration data for worker and control plane nodes, respectively. ## Behavior diff --git a/pkg/controllers/secretsync/secret_sync_controller.go b/pkg/controllers/secretsync/secret_sync_controller.go index 164f36171..f1ea8b746 100644 --- a/pkg/controllers/secretsync/secret_sync_controller.go +++ b/pkg/controllers/secretsync/secret_sync_controller.go @@ -26,6 +26,7 @@ import ( corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/utils/ptr" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" @@ -36,7 +37,8 @@ import ( ) const ( - managedUserDataSecretName = "worker-user-data" + workerUserDataSecretName = "worker-user-data" + masterUserDataSecretName = "master-user-data" // SecretSourceNamespace is the source namespace to copy the user data secret from. SecretSourceNamespace = "openshift-machine-api" @@ -63,86 +65,87 @@ type UserDataSecretController struct { // Reconcile reconciles the user data secret. func (r *UserDataSecretController) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { log := ctrl.LoggerFrom(ctx).WithName(controllerName) - log.Info("reconciling worker user data secret") + secretName := req.Name + log.Info("reconciling user data secret", "secretName", secretName) - defaultSourceSecretObjectKey := client.ObjectKey{ - Name: managedUserDataSecretName, Namespace: SecretSourceNamespace, + if !isValidUserDataSecretName(secretName) { + log.Info("ignoring request for unknown secret", "secretName", secretName) + return ctrl.Result{}, nil } - sourceSecret := &corev1.Secret{} - - if err := r.Get(ctx, defaultSourceSecretObjectKey, sourceSecret); err != nil { - log.Error(err, "unable to get source secret for sync") + if err := r.reconcileUserDataSecret(ctx, log, secretName); err != nil { if err := r.setDegradedCondition(ctx, log); err != nil { return ctrl.Result{}, fmt.Errorf("failed to set conditions for secret sync controller: %w", err) } - return ctrl.Result{}, fmt.Errorf("failed to get source secret: %w", err) + return ctrl.Result{}, err } - targetSecret := &corev1.Secret{} - targetSecretKey := client.ObjectKey{ - Namespace: r.ManagedNamespace, - Name: managedUserDataSecretName, + if err := r.setAvailableCondition(ctx, log); err != nil { + return ctrl.Result{}, fmt.Errorf("failed to set conditions for user data secret controller: %w", err) } - // If the secret does not exist, it will be created later, so we can ignore a Not Found error - if err := r.Get(ctx, targetSecretKey, targetSecret); err != nil && !apierrors.IsNotFound(err) { - log.Error(err, "unable to get target secret for sync") + return ctrl.Result{}, nil +} - if err := r.setDegradedCondition(ctx, log); err != nil { - return ctrl.Result{}, fmt.Errorf("failed to set conditions for secret controller: %w", err) - } +// reconcileUserDataSecret performs the actual reconciliation for a specific user data secret. +func (r *UserDataSecretController) reconcileUserDataSecret(ctx context.Context, log logr.Logger, secretName string) error { + sourceSecret := &corev1.Secret{} + targetSecret := &corev1.Secret{} - return ctrl.Result{}, fmt.Errorf("failed to get target secret: %w", err) + sourceSecretObjectKey := client.ObjectKey{ + Name: secretName, Namespace: SecretSourceNamespace, } - - if r.areSecretsEqual(sourceSecret, targetSecret) { - log.Info("user data in source and target secrets is the same, no sync needed") - - if err := r.setAvailableCondition(ctx, log); err != nil { - return ctrl.Result{}, fmt.Errorf("failed to set conditions for user data secret controller: %w", err) - } - - return ctrl.Result{}, nil + if err := r.Get(ctx, sourceSecretObjectKey, sourceSecret); err != nil { + log.Error(err, "unable to get source secret for sync", "secretName", secretName) + return fmt.Errorf("failed to get source secret %s: %w", secretName, err) } - if err := r.syncSecretData(ctx, sourceSecret, targetSecret); err != nil { - log.Error(err, "unable to sync user data secret") - - if err := r.setDegradedCondition(ctx, log); err != nil { - return ctrl.Result{}, fmt.Errorf("failed to set conditions for user data secret controller: %w", err) - } + targetSecretObjectKey := client.ObjectKey{ + Namespace: r.ManagedNamespace, + Name: secretName, + } + // If the secret does not exist, it will be created later, so we can ignore a Not Found error + if err := r.Get(ctx, targetSecretObjectKey, targetSecret); err != nil && !apierrors.IsNotFound(err) { + log.Error(err, "unable to get target secret for sync", "secretName", secretName) + return fmt.Errorf("failed to get target secret %s: %w", secretName, err) + } - return ctrl.Result{}, err + if r.areSecretsEqual(sourceSecret, targetSecret) { + log.Info("user data in source and target secrets is the same, no sync needed", "secretName", secretName) + return nil } - if err := r.setAvailableCondition(ctx, log); err != nil { - return ctrl.Result{}, fmt.Errorf("failed to set conditions for user data secret controller: %w", err) + if err := r.syncSecretData(ctx, sourceSecret, targetSecret, secretName); err != nil { + log.Error(err, "unable to sync user data secret", "secretName", secretName) + return fmt.Errorf("failed to sync secret %s: %w", secretName, err) } - return ctrl.Result{}, nil + log.Info("user data secret synced successfully", "secretName", secretName) + + return nil } -func (r *UserDataSecretController) areSecretsEqual(source *corev1.Secret, target *corev1.Secret) bool { - return source.Immutable == target.Immutable && - reflect.DeepEqual(source.Data[mapiUserDataKey], target.Data[capiUserDataKey]) && reflect.DeepEqual(source.StringData, target.StringData) && +func (r *UserDataSecretController) areSecretsEqual(source, target *corev1.Secret) bool { + immutableEqual := ptr.Deref(source.Immutable, false) == ptr.Deref(target.Immutable, false) + + return immutableEqual && + reflect.DeepEqual(source.Data[mapiUserDataKey], target.Data[capiUserDataKey]) && source.Type == target.Type } -func (r *UserDataSecretController) syncSecretData(ctx context.Context, source *corev1.Secret, target *corev1.Secret) error { +func (r *UserDataSecretController) syncSecretData(ctx context.Context, source *corev1.Secret, target *corev1.Secret, secretName string) error { userData := source.Data[mapiUserDataKey] if userData == nil { return errSourceSecretMissingUserData } - target.SetName(managedUserDataSecretName) + target.SetName(secretName) target.SetNamespace(r.ManagedNamespace) target.Data = map[string][]byte{ - "value": userData, - "format": []byte("ignition"), + capiUserDataKey: userData, + "format": []byte("ignition"), } - target.StringData = source.StringData target.Immutable = source.Immutable target.Type = source.Type @@ -175,7 +178,7 @@ func (r *UserDataSecretController) SetupWithManager(mgr ctrl.Manager) error { ). Watches( &corev1.Secret{}, - handler.EnqueueRequestsFromMapFunc(toUserDataSecret), + handler.EnqueueRequestsFromMapFunc(toUserDataSecret(r.ManagedNamespace)), builder.WithPredicates(userDataSecretPredicate(SecretSourceNamespace)), ). Complete(r); err != nil { @@ -185,6 +188,11 @@ func (r *UserDataSecretController) SetupWithManager(mgr ctrl.Manager) error { return nil } +// isValidUserDataSecretName checks if the given secret name is a user data secret we should sync. +func isValidUserDataSecretName(secretName string) bool { + return secretName == workerUserDataSecretName || secretName == masterUserDataSecretName +} + func (r *UserDataSecretController) setAvailableCondition(ctx context.Context, log logr.Logger) error { co, err := r.GetOrCreateClusterOperator(ctx) if err != nil { diff --git a/pkg/controllers/secretsync/secret_sync_controller_test.go b/pkg/controllers/secretsync/secret_sync_controller_test.go index b224d1ef7..e7b21c33c 100644 --- a/pkg/controllers/secretsync/secret_sync_controller_test.go +++ b/pkg/controllers/secretsync/secret_sync_controller_test.go @@ -48,9 +48,16 @@ var ( errMissingFormatKey = errors.New("could not find a format key in the worker data secret") ) -func makeUserDataSecret() *corev1.Secret { +func makeWorkerUserDataSecret() *corev1.Secret { return &corev1.Secret{ObjectMeta: metav1.ObjectMeta{ - Name: managedUserDataSecretName, + Name: workerUserDataSecretName, + Namespace: SecretSourceNamespace, + }, Data: map[string][]byte{mapiUserDataKey: []byte(defaultSecretValue)}} +} + +func makeMasterUserDataSecret() *corev1.Secret { + return &corev1.Secret{ObjectMeta: metav1.ObjectMeta{ + Name: masterUserDataSecretName, Namespace: SecretSourceNamespace, }, Data: map[string][]byte{mapiUserDataKey: []byte(defaultSecretValue)}} } @@ -62,8 +69,8 @@ var _ = Describe("areSecretsEqual reconciler method", func() { var targetUserDataSecret *corev1.Secret BeforeEach(func() { - sourceUserDataSecret = makeUserDataSecret() - targetUserDataSecret = makeUserDataSecret() + sourceUserDataSecret = makeWorkerUserDataSecret() + targetUserDataSecret = makeWorkerUserDataSecret() targetUserDataSecret.Data[capiUserDataKey] = sourceUserDataSecret.Data[mapiUserDataKey] }) @@ -81,157 +88,184 @@ var _ = Describe("areSecretsEqual reconciler method", func() { }) var _ = Describe("User Data Secret controller", func() { - var rec *record.FakeRecorder - - var mgrCtxCancel context.CancelFunc - var mgrStopped chan struct{} - ctx := context.Background() - - var sourceSecret *corev1.Secret - - var reconciler *UserDataSecretController - - syncedSecretKey := client.ObjectKey{Namespace: controllers.DefaultManagedNamespace, Name: managedUserDataSecretName} - - BeforeEach(func() { - By("Setting up a manager and controller") - var err error - mgr, err := ctrl.NewManager(cfg, ctrl.Options{ - Controller: config.Controller{ - SkipNameValidation: ptr.To(true), - }, + type testConfig struct { + description string + secretFactory func() *corev1.Secret + secretName string + } + + configs := []testConfig{ + { + description: "Worker User Data Secret", + secretFactory: makeWorkerUserDataSecret, + secretName: workerUserDataSecretName, + }, + { + description: "Master User Data Secret", + secretFactory: makeMasterUserDataSecret, + secretName: masterUserDataSecretName, + }, + } + + // Run the same tests for both worker and master user data secrets + for i := range configs { + tc := configs[i] + + Describe(tc.description, func() { + var rec *record.FakeRecorder + + var mgrCtxCancel context.CancelFunc + var mgrStopped chan struct{} + ctx := context.Background() + + var sourceSecret *corev1.Secret + + var reconciler *UserDataSecretController + + syncedSecretKey := client.ObjectKey{Namespace: controllers.DefaultManagedNamespace, Name: tc.secretName} + + BeforeEach(func() { + By("Setting up a manager and controller") + var err error + mgr, err := ctrl.NewManager(cfg, ctrl.Options{ + Controller: config.Controller{ + SkipNameValidation: ptr.To(true), + }, + }) + Expect(err).ToNot(HaveOccurred(), "Manager should be able to be created") + + rec = record.NewFakeRecorder(32) + reconciler = &UserDataSecretController{ + ClusterOperatorStatusClient: operatorstatus.ClusterOperatorStatusClient{ + Client: cl, + Recorder: rec, + ManagedNamespace: controllers.DefaultManagedNamespace, + }, + + Scheme: scheme.Scheme, + } + Expect(reconciler.SetupWithManager(mgr)).To(Succeed()) + + By("Creating needed Secret") + sourceSecret = tc.secretFactory() + Expect(cl.Create(ctx, sourceSecret)).To(Succeed()) + + var mgrCtx context.Context + mgrCtx, mgrCtxCancel = context.WithCancel(ctx) + mgrStopped = make(chan struct{}) + + By("Starting the manager") + go func() { + defer GinkgoRecover() + defer close(mgrStopped) + + Expect(mgr.Start(mgrCtx)).To(Succeed()) + }() + }) + + AfterEach(func() { + By("Closing the manager") + mgrCtxCancel() + Eventually(mgrStopped, timeout).Should(BeClosed()) + + co := &configv1.ClusterOperator{ + ObjectMeta: metav1.ObjectMeta{ + Name: controllers.ClusterOperatorName, + }, + } + Expect(test.CleanupAndWait(ctx, cl, co)).To(Succeed()) + + By("Cleanup resources") + allSecrets := &corev1.SecretList{} + Expect(cl.List(ctx, allSecrets)).To(Succeed()) + for _, cm := range allSecrets.Items { + Expect(test.CleanupAndWait(ctx, cl, cm.DeepCopy())).To(Succeed()) + } + + sourceSecret = nil + + // Creating the cluster api operator + co = &configv1.ClusterOperator{} + co.SetName(controllers.ClusterOperatorName) + Expect(cl.Create(context.Background(), co.DeepCopy())).To(Succeed()) + }) + + It("secret should be synced up after first reconcile", func() { + Eventually(func() (bool, error) { + syncedUserDataSecret := &corev1.Secret{} + err := cl.Get(ctx, syncedSecretKey, syncedUserDataSecret) + if err != nil { + return false, err + } + + formatValue, ok := syncedUserDataSecret.Data["format"] + if !ok { + return false, errMissingFormatKey + } + Expect(string(formatValue)).Should(Equal("ignition")) + + return bytes.Equal(syncedUserDataSecret.Data[capiUserDataKey], []byte(defaultSecretValue)), nil + }, timeout).Should(BeTrue()) + }) + + It("secret should be synced up if managed user data secret changed", func() { + changedSourceSecret := sourceSecret.DeepCopy() + changedSourceSecret.Data = map[string][]byte{mapiUserDataKey: []byte("changed")} + Expect(cl.Update(ctx, changedSourceSecret)).To(Succeed()) + + Eventually(func() (bool, error) { + syncedUserDataSecret := &corev1.Secret{} + err := cl.Get(ctx, syncedSecretKey, syncedUserDataSecret) + if err != nil { + return false, err + } + + formatValue, ok := syncedUserDataSecret.Data["format"] + if !ok { + return false, errMissingFormatKey + } + Expect(string(formatValue)).Should(Equal("ignition")) + + return bytes.Equal(syncedUserDataSecret.Data[capiUserDataKey], []byte("changed")), nil + }, timeout).Should(BeTrue()) + }) + + It("secret should be synced up if owned user data secret is deleted or changed", func() { + syncedUserDataSecret := &corev1.Secret{} + Eventually(func() error { + return cl.Get(ctx, syncedSecretKey, syncedUserDataSecret) + }, timeout).Should(Succeed()) + + syncedUserDataSecret.Data = map[string][]byte{capiUserDataKey: []byte("baz")} + Expect(cl.Update(ctx, syncedUserDataSecret)).To(Succeed()) + Eventually(func() (bool, error) { + err := cl.Get(ctx, syncedSecretKey, syncedUserDataSecret) + if err != nil { + return false, err + } + + formatValue, ok := syncedUserDataSecret.Data["format"] + if !ok { + return false, errMissingFormatKey + } + Expect(string(formatValue)).Should(Equal("ignition")) + + return bytes.Equal(syncedUserDataSecret.Data[capiUserDataKey], []byte(defaultSecretValue)), nil + }, timeout).Should(BeTrue()) + + Expect(test.CleanupAndWait(ctx, cl, sourceSecret)).To(Succeed()) + }) + + It("secret should not be updated if source and target secret contents are identical", func() { + syncedUserDataSecret := &corev1.Secret{} + Eventually(func() error { + return cl.Get(ctx, syncedSecretKey, syncedUserDataSecret) + }, timeout).Should(Succeed()) + initialSecretresourceVersion := syncedUserDataSecret.ResourceVersion + + Expect(cl.Get(ctx, syncedSecretKey, syncedUserDataSecret)).Should(Succeed()) + Expect(initialSecretresourceVersion).Should(BeEquivalentTo(syncedUserDataSecret.ResourceVersion)) + }) }) - Expect(err).ToNot(HaveOccurred(), "Manager should be able to be created") - - reconciler = &UserDataSecretController{ - ClusterOperatorStatusClient: operatorstatus.ClusterOperatorStatusClient{ - Client: cl, - Recorder: rec, - ManagedNamespace: controllers.DefaultManagedNamespace, - }, - - Scheme: scheme.Scheme, - } - Expect(reconciler.SetupWithManager(mgr)).To(Succeed()) - - By("Creating needed Secret") - sourceSecret = makeUserDataSecret() - Expect(cl.Create(ctx, sourceSecret)).To(Succeed()) - - var mgrCtx context.Context - mgrCtx, mgrCtxCancel = context.WithCancel(ctx) - mgrStopped = make(chan struct{}) - - By("Starting the manager") - go func() { - defer GinkgoRecover() - defer close(mgrStopped) - - Expect(mgr.Start(mgrCtx)).To(Succeed()) - }() - }) - - AfterEach(func() { - By("Closing the manager") - mgrCtxCancel() - Eventually(mgrStopped, timeout).Should(BeClosed()) - - co := &configv1.ClusterOperator{ - ObjectMeta: metav1.ObjectMeta{ - Name: controllers.ClusterOperatorName, - }, - } - Expect(test.CleanupAndWait(ctx, cl, co)) - - By("Cleanup resources") - allSecrets := &corev1.SecretList{} - Expect(cl.List(ctx, allSecrets)).To(Succeed()) - for _, cm := range allSecrets.Items { - Expect(test.CleanupAndWait(ctx, cl, cm.DeepCopy())).To(Succeed()) - } - - sourceSecret = nil - - // Creating the cluster api operator - co = &configv1.ClusterOperator{} - co.SetName(controllers.ClusterOperatorName) - Expect(cl.Create(context.Background(), co.DeepCopy())).To(Succeed()) - }) - - It("secret should be synced up after first reconcile", func() { - Eventually(func() (bool, error) { - syncedUserDataSecret := &corev1.Secret{} - err := cl.Get(ctx, syncedSecretKey, syncedUserDataSecret) - if err != nil { - return false, err - } - - formatValue, ok := syncedUserDataSecret.Data["format"] - if !ok { - return false, errMissingFormatKey - } - Expect(string(formatValue)).Should(Equal("ignition")) - - return bytes.Equal(syncedUserDataSecret.Data[capiUserDataKey], []byte(defaultSecretValue)), nil - }, timeout).Should(BeTrue()) - }) - - It("secret should be synced up if managed user data secret changed", func() { - changedSourceSecret := sourceSecret.DeepCopy() - changedSourceSecret.Data = map[string][]byte{mapiUserDataKey: []byte("managed one changed")} - Expect(cl.Update(ctx, changedSourceSecret)).To(Succeed()) - - Eventually(func() (bool, error) { - syncedUserDataSecret := &corev1.Secret{} - err := cl.Get(ctx, syncedSecretKey, syncedUserDataSecret) - if err != nil { - return false, err - } - - formatValue, ok := syncedUserDataSecret.Data["format"] - if !ok { - return false, errMissingFormatKey - } - Expect(string(formatValue)).Should(Equal("ignition")) - - return bytes.Equal(syncedUserDataSecret.Data[capiUserDataKey], []byte("managed one changed")), nil - }, timeout).Should(BeTrue()) - }) - - It("secret should be synced up if owned user data secret is deleted or changed", func() { - syncedUserDataSecret := &corev1.Secret{} - Eventually(func() error { - return cl.Get(ctx, syncedSecretKey, syncedUserDataSecret) - }, timeout).Should(Succeed()) - - syncedUserDataSecret.Data = map[string][]byte{capiUserDataKey: []byte("baz")} - Expect(cl.Update(ctx, syncedUserDataSecret)).To(Succeed()) - Eventually(func() (bool, error) { - err := cl.Get(ctx, syncedSecretKey, syncedUserDataSecret) - if err != nil { - return false, err - } - - formatValue, ok := syncedUserDataSecret.Data["format"] - if !ok { - return false, errMissingFormatKey - } - Expect(string(formatValue)).Should(Equal("ignition")) - - return bytes.Equal(syncedUserDataSecret.Data[capiUserDataKey], []byte(defaultSecretValue)), nil - }, timeout).Should(BeTrue()) - - Expect(test.CleanupAndWait(ctx, cl, sourceSecret)).To(Succeed()) - }) - - It("secret not be updated if source and target secret contents are identical", func() { - syncedUserDataSecret := &corev1.Secret{} - Eventually(func() error { - return cl.Get(ctx, syncedSecretKey, syncedUserDataSecret) - }, timeout).Should(Succeed()) - initialSecretresourceVersion := syncedUserDataSecret.ResourceVersion - - Expect(cl.Get(ctx, syncedSecretKey, syncedUserDataSecret)).Should(Succeed()) - Expect(initialSecretresourceVersion).Should(BeEquivalentTo(syncedUserDataSecret.ResourceVersion)) - }) + } }) diff --git a/pkg/controllers/secretsync/watch_predicates.go b/pkg/controllers/secretsync/watch_predicates.go index 7bd001b41..2955f2cba 100644 --- a/pkg/controllers/secretsync/watch_predicates.go +++ b/pkg/controllers/secretsync/watch_predicates.go @@ -19,23 +19,38 @@ import ( "context" corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/predicate" "sigs.k8s.io/controller-runtime/pkg/reconcile" ) -func toUserDataSecret(ctx context.Context, obj client.Object) []reconcile.Request { - return []reconcile.Request{{ - NamespacedName: client.ObjectKey{Name: managedUserDataSecretName, Namespace: SecretSourceNamespace}, - }} +func toUserDataSecret(namespace string) func(context.Context, client.Object) []reconcile.Request { + return func(ctx context.Context, obj client.Object) []reconcile.Request { + secret, ok := obj.(*corev1.Secret) + if !ok { + return nil + } + + // Map the source secret to the target secret with the same name + return []reconcile.Request{{ + NamespacedName: client.ObjectKey{Name: secret.GetName(), Namespace: namespace}, + }} + } } func userDataSecretPredicate(targetNamespace string) predicate.Funcs { - isOwnedUserDataSecret := func(obj runtime.Object) bool { + isOwnedUserDataSecret := func(obj client.Object) bool { secret, ok := obj.(*corev1.Secret) - return ok && secret.GetNamespace() == targetNamespace && secret.GetName() == managedUserDataSecretName + if !ok { + return false + } + + if secret.GetNamespace() != targetNamespace { + return false + } + // Accept both worker-user-data and master-user-data secrets + return isValidUserDataSecretName(secret.GetName()) } return predicate.Funcs{