fix: fall back to canonical backup auth secret name on restore#1614
fix: fall back to canonical backup auth secret name on restore#1614akurinnoy wants to merge 1 commit intodevfile:mainfrom
Conversation
When CopySecret writes an auth secret into the workspace namespace, it
always uses DevWorkspaceBackupAuthSecretName ("devworkspace-backup-registry-auth")
regardless of what name the admin configured (e.g. "quay-backup-auth").
GetNamespaceRegistryAuthSecret (the restore path) calls HandleRegistryAuthSecret
with operatorConfigNamespace="" and looked only for the configured name, found
nothing, and returned nil — leaving the workspace-restore init container without
credentials and causing CrashLoopBackOff on private registries.
Fix: when operatorConfigNamespace is empty and the configured name is not found,
attempt a second Get using the canonical DevWorkspaceBackupAuthSecretName. Skip
the extra lookup when the configured name already equals the canonical name to
avoid a redundant API call. Propagate any non-NotFound error from the fallback.
Assisted-by: Claude Sonnet 4.6 (1M context)
Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>
akurinnoy
requested review from
dkwon17,
ibuziuk and
rohanKanojia
as code owners
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: akurinnoy The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/secrets/backup_test.go (1)
146-159: “No duplicate query” is not actually asserted.The test name claims query deduplication, but current assertions only check returned secret correctness. Consider either renaming the test or instrumenting
Getcall counting to assert the single-lookup behavior explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/secrets/backup_test.go` around lines 146 - 159, The test claims "no duplicate query" but never asserts Get call count; update the test to assert a single Get by replacing or wrapping the fakeClient with a small counting decorator that implements client.Client and increments a counter inside its Get method, then call secrets.HandleRegistryAuthSecret and Expect(counter).To(Equal(1)); reference the used symbols: fakeClient (or the decorated client), secrets.HandleRegistryAuthSecret, and constants.DevWorkspaceBackupAuthSecretName so the instrumentation targets the same fake client used in the test. Ensure all other client methods delegate to the underlying fake.NewClientBuilder().WithScheme(...).WithObjects(...).Build() instance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/secrets/backup_test.go`:
- Around line 146-159: The test claims "no duplicate query" but never asserts
Get call count; update the test to assert a single Get by replacing or wrapping
the fakeClient with a small counting decorator that implements client.Client and
increments a counter inside its Get method, then call
secrets.HandleRegistryAuthSecret and Expect(counter).To(Equal(1)); reference the
used symbols: fakeClient (or the decorated client),
secrets.HandleRegistryAuthSecret, and constants.DevWorkspaceBackupAuthSecretName
so the instrumentation targets the same fake client used in the test. Ensure all
other client methods delegate to the underlying
fake.NewClientBuilder().WithScheme(...).WithObjects(...).Build() instance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e89b05c2-703d-43b0-80d8-9c0b7f09c772
📒 Files selected for processing (2)
pkg/secrets/backup.gopkg/secrets/backup_test.go
| // First check the workspace namespace for the secret | ||
| registryAuthSecret := &corev1.Secret{} | ||
| err := c.Get(ctx, client.ObjectKey{ | ||
| Name: secretName, |
There was a problem hiding this comment.
Since workspace namespace's secret name is always constants.DevWorkspaceBackupAuthSecretName, should we do the following? :
| Name: constants.DevWorkspaceBackupAuthSecretName , |
2 participants
What does this PR do?
When a workspace is backed up to a private registry,
CopySecretcopies the registry auth secret into the workspace namespace using a hardcoded name (devworkspace-backup-registry-auth), regardless of the name configured in theDWOC. During restore,GetNamespaceRegistryAuthSecretlooks for the configured name, doesn't find it, and returnsnil- so theworkspace-restoreinit container has no credentials and crashes withCrashLoopBackOff.This PR adds a fallback lookup in
HandleRegistryAuthSecret: when called from the restore path (operatorConfigNamespace == ""), if the configured secret name is not found in the workspace namespace, it alsochecks for the canonical
devworkspace-backup-registry-authname thatCopySecretalways uses.Note: This is a surgical workaround. See #1615 for a root-cause fix that changes the secret naming in
CopySecretinstead.What issues does this PR fix or reference?
CRW-10591
Is it tested? How?
New unit tests added.
PR Checklist
/test v8-devworkspace-operator-e2e, v8-che-happy-pathto trigger)v8-devworkspace-operator-e2e: DevWorkspace e2e testv8-che-happy-path: Happy path for verification integration with CheSummary by CodeRabbit
Bug Fixes
Tests