OCPBUGS-63524: Sync master-user-data secret #397
Conversation
WalkthroughThe Secret sync controller now validates and reconciles individual user-data secrets (worker and master) by name, syncing each from the source namespace into the managed namespace. Watch predicates were refactored to a namespace-aware higher-order mapping and ownership checks accept multiple valid secret names. Changes
Sequence DiagramsequenceDiagram
participant Watcher as Watch Predicates
participant Reconciler as Secret Sync Reconciler
participant SourceNS as Source Namespace (openshift-machine-api)
participant TargetNS as Managed Namespace
Watcher->>Reconciler: enqueue reconcileRequest(secretName, managedNamespace)
Reconciler->>Reconciler: isValidUserDataSecretName(secretName)
alt Invalid name
Reconciler-->>Watcher: skip
else Valid (worker/master)
Reconciler->>SourceNS: Get Secret(secretName)
alt Source found
SourceNS-->>Reconciler: sourceSecret
Reconciler->>TargetNS: Get Secret(secretName)
alt Target exists
TargetNS-->>Reconciler: targetSecret
Reconciler->>Reconciler: areSecretsEqual?
alt differ
Reconciler->>TargetNS: Update targetSecret (syncSecretData)
TargetNS-->>Reconciler: Success
else equal
Reconciler-->>Reconciler: No-op
end
else Target missing
Reconciler->>TargetNS: Create targetSecret (syncSecretData)
TargetNS-->>Reconciler: Created
end
else Source missing/error
Reconciler-->>Reconciler: set degraded / return error
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/controllers/secretsync/secret_sync_controller.go (2)
128-132: Equality check bugs: pointer identity and StringData cause false diffs
- Immutable is a *bool; comparing pointers by identity is wrong. Compare dereferenced values safely.
- StringData is write-only; reading a Secret never returns it, so including it here can force perpetual updates.
Fix:
-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) && - source.Type == target.Type -} +func (r *UserDataSecretController) areSecretsEqual(source, target *corev1.Secret) bool { + // Compare Immutable by value, tolerating nils. + immutableEqual := false + switch { + case source.Immutable == nil && target.Immutable == nil: + immutableEqual = true + case source.Immutable != nil && target.Immutable != nil: + immutableEqual = *source.Immutable == *target.Immutable + } + return immutableEqual && + reflect.DeepEqual(source.Data[mapiUserDataKey], target.Data[capiUserDataKey]) && + source.Type == target.Type +}
140-147: Avoid StringData writes and use the capiUserDataKey constantStringData is write-only; writing it every reconcile can cause unnecessary updates. Also prefer the constant for the “value” key for consistency.
- target.SetName(secretName) + target.SetName(secretName) target.SetNamespace(r.ManagedNamespace) target.Data = map[string][]byte{ - "value": userData, + capiUserDataKey: userData, "format": []byte("ignition"), } - target.StringData = source.StringData target.Immutable = source.Immutable target.Type = source.Type
🧹 Nitpick comments (3)
pkg/controllers/secretsync/secret_sync_controller_test.go (1)
48-49: Generalize error message (applies to worker and master)Minor polish for accuracy across both contexts.
- errMissingFormatKey = errors.New("could not find a format key in the worker data secret") + errMissingFormatKey = errors.New("could not find a 'format' key in the user data secret")pkg/controllers/secretsync/secret_sync_controller.go (1)
83-88: Status churn: consider updating CO conditions only on state transitionsNot a blocker, but reducing unconditional writes can cut noise and API traffic.
pkg/controllers/secretsync/watch_predicates.go (1)
46-55: Change parameter type toclient.Objectfor controller-runtime v0.20.4 consistencyYour controller-runtime version (v0.20.4) supports and expects
client.Objectin predicate callbacks. TheisOwnedUserDataSecretfunction at line 44 currently usesruntime.Object, which works but is inconsistent with modern versions. Change the parameter type fromruntime.Objecttoclient.Object:isOwnedUserDataSecret := func(obj client.Object) bool {The rest of the logic (namespace + name allowlist) is correct. Both
runtimeandclientare already imported, so no additional imports are needed.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
docs/controllers/secretsync.md(1 hunks)pkg/controllers/secretsync/secret_sync_controller.go(5 hunks)pkg/controllers/secretsync/secret_sync_controller_test.go(3 hunks)pkg/controllers/secretsync/watch_predicates.go(1 hunks)
🔇 Additional comments (2)
pkg/controllers/secretsync/watch_predicates.go (1)
29-40: LGTM: per-namespace source→target mapperClean, minimal mapping; pairs well with the source-namespace predicate and per-secret reconcile.
pkg/controllers/secretsync/secret_sync_controller.go (1)
189-193: Allowlist helper is fineSimple and explicit; OK as is.
RadekManak
force-pushed
the
master-user-data
branch
from
978d879 to
8d70517
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/controllers/secretsync/secret_sync_controller_test.go (1)
47-49: Genericize error textMessage hardcodes “worker” but used for both secrets. Use generic wording.
-var ( - errMissingFormatKey = errors.New("could not find a format key in the worker data secret") -) +var ( + errMissingFormatKey = errors.New("could not find the 'format' key in the user data secret") +)pkg/controllers/secretsync/secret_sync_controller.go (1)
137-169: Immutable Secret update will fail; add delete‑and‑recreate fallbackIf the target Secret is immutable and data changes, Update returns Invalid. Handle by deleting and recreating.
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(secretName) target.SetNamespace(r.ManagedNamespace) target.Data = map[string][]byte{ - capiUserDataKey: userData, - "format": []byte("ignition"), + capiUserDataKey: userData, + formatKey: []byte(formatIgnition), } target.Immutable = source.Immutable target.Type = source.Type - // check if the target secret exists, create if not - err := r.Get(ctx, client.ObjectKeyFromObject(target), &corev1.Secret{}) - if err != nil && apierrors.IsNotFound(err) { - if err := r.Create(ctx, target); err != nil { - return fmt.Errorf("failed to create target secret: %w", err) - } - return nil - } else if err != nil { - return fmt.Errorf("failed to get target secret: %w", err) - } - - if err := r.Update(ctx, target); err != nil { - return fmt.Errorf("failed to update target secret: %w", err) - } + // Upsert with immutable fallback + current := &corev1.Secret{} + if err := r.Get(ctx, client.ObjectKeyFromObject(target), current); err != nil { + if apierrors.IsNotFound(err) { + if err := r.Create(ctx, target); err != nil { + return fmt.Errorf("failed to create target secret: %w", err) + } + return nil + } + return fmt.Errorf("failed to get target secret: %w", err) + } + // Preserve ResourceVersion for update + target.SetResourceVersion(current.GetResourceVersion()) + if ptr.Deref(current.Immutable, false) { + // Replace immutable secret + if err := r.Delete(ctx, current); err != nil { + return fmt.Errorf("failed to delete immutable target secret: %w", err) + } + target.SetResourceVersion("") + if err := r.Create(ctx, target); err != nil { + return fmt.Errorf("failed to recreate immutable target secret: %w", err) + } + return nil + } + if err := r.Update(ctx, target); err != nil { + return fmt.Errorf("failed to update target secret: %w", err) + } return nil }
🧹 Nitpick comments (6)
pkg/controllers/secretsync/secret_sync_controller_test.go (2)
179-184: Don’t delete all Secrets cluster‑wide in testsListing all Secrets risks removing unrelated objects. Scope cleanup to our namespaces or specific names.
- allSecrets := &corev1.SecretList{} - Expect(cl.List(ctx, allSecrets)).To(Succeed()) - for _, cm := range allSecrets.Items { - Expect(test.CleanupAndWait(ctx, cl, cm.DeepCopy())).To(Succeed()) - } + // Cleanup only what we own in managed namespace + { + list := &corev1.SecretList{} + Expect(cl.List(ctx, list, client.InNamespace(controllers.DefaultManagedNamespace))).To(Succeed()) + for _, s := range list.Items { + if s.Name == workerUserDataSecretName || s.Name == masterUserDataSecretName { + Expect(test.CleanupAndWait(ctx, cl, s.DeepCopy())).To(Succeed()) + } + } + } + // Cleanup only what we created in source namespace + { + list := &corev1.SecretList{} + Expect(cl.List(ctx, list, client.InNamespace(SecretSourceNamespace))).To(Succeed()) + for _, s := range list.Items { + if s.Name == workerUserDataSecretName || s.Name == masterUserDataSecretName { + Expect(test.CleanupAndWait(ctx, cl, s.DeepCopy())).To(Succeed()) + } + } + }
259-268: Strengthen “no update” assertionUse Consistently to ensure ResourceVersion remains stable for a period, not just between two reads.
- Expect(cl.Get(ctx, syncedSecretKey, syncedUserDataSecret)).Should(Succeed()) - Expect(initialSecretresourceVersion).Should(BeEquivalentTo(syncedUserDataSecret.ResourceVersion)) + Consistently(func(g Gomega) { + g.Expect(cl.Get(ctx, syncedSecretKey, syncedUserDataSecret)).To(Succeed()) + g.Expect(syncedUserDataSecret.ResourceVersion).To(Equal(initialSecretresourceVersion)) +}, time.Second*2, time.Millisecond*100).Should(Succeed())pkg/controllers/secretsync/watch_predicates.go (1)
28-40: Guard against misuse if predicates changeMapper assumes predicates filter names. Add a local name check as a safety net to avoid spurious reconciles if the mapper is reused elsewhere.
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 + // Extra safety: only map valid user-data secrets + if !isValidUserDataSecretName(secret.GetName()) { + 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}, }} }pkg/controllers/secretsync/secret_sync_controller.go (3)
18-23: Use bytes.Equal and drop reflectSwitching equality for []byte to bytes.Equal avoids reflect overhead and clarifies intent.
-import ( - "context" - "errors" - "fmt" - "reflect" +import ( + "bytes" + "context" + "errors" + "fmt"
50-53: Avoid magic strings for “format”Promote keys/values to constants used in code and tests.
const ( mapiUserDataKey = "userData" capiUserDataKey = "value" controllerName = "SecretSyncController" + formatKey = "format" + formatIgnition = "ignition" )
96-113: Minor: avoid double GET on target SecretYou GET the target in reconcileUserDataSecret and again in syncSecretData. Consider passing a targetExists flag or the fetched object to avoid the second GET.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
docs/controllers/secretsync.md(1 hunks)pkg/controllers/secretsync/secret_sync_controller.go(5 hunks)pkg/controllers/secretsync/secret_sync_controller_test.go(3 hunks)pkg/controllers/secretsync/watch_predicates.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/controllers/secretsync.md
🔇 Additional comments (4)
pkg/controllers/secretsync/secret_sync_controller_test.go (1)
110-113: Closure-capture fix looks goodIterating by index and rebinding tc avoids the Ginkgo closure trap.
pkg/controllers/secretsync/watch_predicates.go (1)
42-61: Predicate logic is tight and namespace‑awareFilters by type, namespace, and allowed names. Good.
pkg/controllers/secretsync/secret_sync_controller.go (2)
171-189: Controller wiring looks correctPrimary resource: managed‑ns Secrets; watch source‑ns Secrets via mapper. Good.
191-195: Name allowlist helper is simple and effectiveKeeps reconciliation bounded to known secrets.
RadekManak
force-pushed
the
master-user-data
branch
from
8d70517 to
6e680b3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/controllers/secretsync/secret_sync_controller.go (1)
137-169: Create/Update race: Update may lack resourceVersionIf the target didn’t exist earlier but appears between calls, Update() is invoked on ‘target’ without a resourceVersion, causing 409/invalid object errors. Fetch existing, copy its resourceVersion, then update. Also use the new format constants.
Apply this diff:
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(secretName) target.SetNamespace(r.ManagedNamespace) target.Data = map[string][]byte{ - capiUserDataKey: userData, - "format": []byte("ignition"), + capiUserDataKey: userData, + formatKey: []byte(formatIgnition), } target.Immutable = source.Immutable target.Type = source.Type // check if the target secret exists, create if not - err := r.Get(ctx, client.ObjectKeyFromObject(target), &corev1.Secret{}) - if err != nil && apierrors.IsNotFound(err) { - if err := r.Create(ctx, target); err != nil { - return fmt.Errorf("failed to create target secret: %w", err) - } - return nil - } else if err != nil { - return fmt.Errorf("failed to get target secret: %w", err) - } - - if err := r.Update(ctx, target); err != nil { - return fmt.Errorf("failed to update target secret: %w", err) - } + existing := &corev1.Secret{} + if err := r.Get(ctx, client.ObjectKeyFromObject(target), existing); err != nil { + if apierrors.IsNotFound(err) { + if err := r.Create(ctx, target); err != nil { + return fmt.Errorf("failed to create target secret: %w", err) + } + return nil + } + return fmt.Errorf("failed to get target secret: %w", err) + } + // Preserve optimistic lock + target.SetResourceVersion(existing.GetResourceVersion()) + if err := r.Update(ctx, target); err != nil { + return fmt.Errorf("failed to update target secret: %w", err) + } return nil }
♻️ Duplicate comments (1)
pkg/controllers/secretsync/secret_sync_controller.go (1)
129-135: Equality misses ‘format’ and uses reflect.DeepEqual for bytesThis can leave drift undetected when only format changes; prefer bytes.Equal for byte slices. Mirrors an earlier review.
Apply this diff:
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 + if ptr.Deref(source.Immutable, false) != ptr.Deref(target.Immutable, false) { + return false + } + if !bytes.Equal(source.Data[mapiUserDataKey], target.Data[capiUserDataKey]) { + return false + } + // Ensure the managed secret keeps the expected format value. + if !bytes.Equal([]byte(formatIgnition), target.Data[formatKey]) { + return false + } + return source.Type == target.Type }
🧹 Nitpick comments (8)
pkg/controllers/secretsync/watch_predicates.go (2)
28-39: Add a defensive name guard in the mapperPrevents accidental enqueues if predicates change or are bypassed.
Apply this diff:
return func(ctx context.Context, obj client.Object) []reconcile.Request { secret, ok := obj.(*corev1.Secret) if !ok { return nil } + // Extra guard: only enqueue known user-data secret names + if !isValidUserDataSecretName(secret.GetName()) { + 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}, }} }
42-62: Nil‑safe predicateAdd a nil guard to avoid a possible panic on malformed events.
isOwnedUserDataSecret := func(obj client.Object) bool { + if obj == nil { + return false + } secret, ok := obj.(*corev1.Secret) if !ok { return false }pkg/controllers/secretsync/secret_sync_controller.go (3)
18-37: Switch from reflect to bytes; import updateUse bytes.Equal for []byte comparisons; drop reflect.
-import ( - "context" - "errors" - "fmt" - "reflect" +import ( + "context" + "errors" + "fmt" + "bytes"
39-53: Introduce constants for format keysAvoid string duplication and clarify intent.
const ( workerUserDataSecretName = "worker-user-data" masterUserDataSecretName = "master-user-data" + formatKey = "format" + formatIgnition = "ignition" // SecretSourceNamespace is the source namespace to copy the user data secret from. SecretSourceNamespace = "openshift-machine-api"
171-189: Optional: narrow primary watch with name filterYou already predicate by namespace; the predicate also filters names, so this is fine. If you see noisy reconciles later, consider an additional name predicate for the primary watch too.
pkg/controllers/secretsync/secret_sync_controller_test.go (3)
65-88: Add an equality test for format driftCatches regressions where only the format changes.
It("should return 'false' if Secrets content are not equal", func() { targetUserDataSecret.Immutable = ptr.To(true) Expect(reconciler.areSecretsEqual(sourceUserDataSecret, targetUserDataSecret)).Should(BeFalse()) targetUserDataSecret.Data = map[string][]byte{} Expect(reconciler.areSecretsEqual(sourceUserDataSecret, targetUserDataSecret)).Should(BeFalse()) }) + + It("should return 'false' if format is missing or wrong", func() { + // missing format + targetUserDataSecret.Data = map[string][]byte{ + capiUserDataKey: sourceUserDataSecret.Data[mapiUserDataKey], + } + Expect(reconciler.areSecretsEqual(sourceUserDataSecret, targetUserDataSecret)).Should(BeFalse()) + + // wrong format + targetUserDataSecret.Data = map[string][]byte{ + capiUserDataKey: sourceUserDataSecret.Data[mapiUserDataKey], + formatKey: []byte("cloud-config"), + } + Expect(reconciler.areSecretsEqual(sourceUserDataSecret, targetUserDataSecret)).Should(BeFalse()) + })
193-231: Add a controller test for format‑only driftEnsures reconcile corrects format even when userData is unchanged.
It("secret should be synced up after first reconcile", func() { @@ }) + It("secret should be re-synced if only the format changes", func() { + // Wait for initial sync + syncedUserDataSecret := &corev1.Secret{} + Eventually(func() error { + return cl.Get(ctx, syncedSecretKey, syncedUserDataSecret) + }, timeout).Should(Succeed()) + + // Change only the format on the managed secret + syncedUserDataSecret.Data[formatKey] = []byte("cloud-config") + Expect(cl.Update(ctx, syncedUserDataSecret)).To(Succeed()) + + // Expect controller to restore ignition + Eventually(func() (string, error) { + if err := cl.Get(ctx, syncedSecretKey, syncedUserDataSecret); err != nil { + return "", err + } + return string(syncedUserDataSecret.Data[formatKey]), nil + }, timeout).Should(Equal("ignition")) + })
47-49: Nit: generic error messageMinor wording tweak: the error mentions “worker” but is used for master too. Consider “user data secret”.
-var ( - errMissingFormatKey = errors.New("could not find a format key in the worker data secret") -) +var ( + errMissingFormatKey = errors.New("could not find a format key in the user data secret") +)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
docs/controllers/secretsync.md(1 hunks)pkg/controllers/secretsync/secret_sync_controller.go(5 hunks)pkg/controllers/secretsync/secret_sync_controller_test.go(3 hunks)pkg/controllers/secretsync/watch_predicates.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/controllers/secretsync.md
🔇 Additional comments (1)
pkg/controllers/secretsync/secret_sync_controller.go (1)
191-195: Valid names helper looks goodCovers the two supported secrets explicitly.
RadekManak
changed the title
openshift-ci-robot
added
jira/valid-reference
|
@RadekManak: This pull request references Jira Issue OCPBUGS-63524, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@RadekManak: This pull request references Jira Issue OCPBUGS-63524, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@RadekManak: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Updates secret sync controller to sync
master-user-datasecret.Summary by CodeRabbit
New Features
Improvements
Documentation
Tests