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 4 commits 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
📝 WalkthroughWalkthroughThe workspace-secret lookup in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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
|
@akurinnoy : Do we need this now that #1615 seems to be ready? |
|
@rohanKanojia I'm leaning towards this PR, as opposed to #1615, with one reason being, the secret name is predictable to the user. Another reason, is, for #1615 if the admin repeatedly changes the secret name overtime, multiple secrets can get copied to user namespaces overtime (although, they will still get cleaned up if the owner DevWorkspaces is deleted). Although, this is a very unlikely an edge case. |
|
@dkwon17 : I don't have any objection with proceeding with this PR. |
fixup\! fix: fall back to canonical backup auth secret name on restore Address review feedback: - Use canonical DevWorkspaceBackupAuthSecretName for workspace NS lookup directly instead of falling back (dkwon17) - Remove unused secretGR variable and schema import (rohanKanojia) Assisted-by: Claude Opus 4.6 (1M context) Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: akurinnoy, rohanKanojia 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 |
…restore fixup\! fixup\! fix: fall back to canonical backup auth secret name on restore Use canonical name only on restore path, keep configured name on backup path Assisted-by: Claude Opus 4.6 (1M context) Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>
|
New changes are detected. LGTM label has been removed. |
|
@akurinnoy thank you, a small suggestion, throughout this PR instead of using the term "canonical name", could we change it to "predefined name"? |
…ame on restore fixup\! fixup\! fixup\! fix: fall back to canonical backup auth secret name on restore Replace "canonical" with "predefined" throughout Assisted-by: Claude Opus 4.6 (1M context) Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/secrets/backup_test.go`:
- Around line 104-143: Add two unit tests to cover the "configured-name-first"
behavior in HandleRegistryAuthSecret: (1) create a secret named as the
configured value (e.g., config := makeConfig("my-configured-secret")) in the
workspace and assert HandleRegistryAuthSecret returns that secret (not the
predefined one), and (2) set the configured name equal to
constants.DevWorkspaceBackupAuthSecretName and ensure the function still returns
the predefined secret via the direct lookup path (i.e., the fallback lookup must
be skipped). Use the existing test patterns (fake.NewClientBuilder(),
makeWorkspace, makeSecret, ctx, scheme, log) and assert errors/results the same
way as the other cases so these new tests catch regressions in the
configured-name-first logic.
- Around line 26-37: The imports in backup_test.go are mis-grouped: move
project-local imports (specifically
github.com/devfile/devworkspace-operator/apis/controller/v1alpha1 imported as
controllerv1alpha1 and the project-local packages
github.com/devfile/devworkspace-operator/pkg/constants and
github.com/devfile/devworkspace-operator/pkg/secrets) into the third import
block (project-local group) separated by a blank line from the standard library
and third-party/Kubernetes groups so the file follows the three-group import
guideline.
- Around line 79-87: The secret fixture function makeSecret currently populates
Data with the "auth" key but should use the Kubernetes-standard
".dockerconfigjson" key for corev1.SecretTypeDockerConfigJson; update makeSecret
to set Data to map[string][]byte{".dockerconfigjson": []byte("dXNlcjpwYXNz")} so
the fixture matches API expectations and other tests (e.g.,
controllers/workspace/devworkspace_controller_test.go).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 10199d67-4e57-4239-bada-ac6641ed2180
📒 Files selected for processing (2)
pkg/secrets/backup.gopkg/secrets/backup_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/secrets/backup.go
| dwv2 "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" | ||
| controllerv1alpha1 "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" | ||
| corev1 "k8s.io/api/core/v1" | ||
| k8sErrors "k8s.io/apimachinery/pkg/api/errors" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/apimachinery/pkg/runtime" | ||
| "sigs.k8s.io/controller-runtime/pkg/client" | ||
| "sigs.k8s.io/controller-runtime/pkg/client/fake" | ||
| "sigs.k8s.io/controller-runtime/pkg/log/zap" | ||
|
|
||
| "github.com/devfile/devworkspace-operator/pkg/constants" | ||
| "github.com/devfile/devworkspace-operator/pkg/secrets" |
There was a problem hiding this comment.
Move all project-local imports into the third import block.
github.com/devfile/devworkspace-operator/apis/controller/v1alpha1 is project-local but is currently grouped with third-party/Kubernetes imports.
As per coding guidelines "Organize imports into three groups separated by blank lines: (1) standard library, (2) third-party + Kubernetes, (3) project-local."
🤖 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 26 - 37, The imports in
backup_test.go are mis-grouped: move project-local imports (specifically
github.com/devfile/devworkspace-operator/apis/controller/v1alpha1 imported as
controllerv1alpha1 and the project-local packages
github.com/devfile/devworkspace-operator/pkg/constants and
github.com/devfile/devworkspace-operator/pkg/secrets) into the third import
block (project-local group) separated by a blank line from the standard library
and third-party/Kubernetes groups so the file follows the three-group import
guideline.
| func makeSecret(name, namespace string) *corev1.Secret { | ||
| return &corev1.Secret{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: name, | ||
| Namespace: namespace, | ||
| }, | ||
| Data: map[string][]byte{"auth": []byte("dXNlcjpwYXNz")}, | ||
| Type: corev1.SecretTypeDockerConfigJson, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify dockerconfigjson fixtures and key usage across Go tests.
# Expected: SecretTypeDockerConfigJson fixtures should use the ".dockerconfigjson" key.
rg -nP --type=go -C3 'SecretTypeDockerConfigJson|map\[string\]\[\]byte\{'Repository: devfile/devworkspace-operator
Length of output: 6697
Change the secret fixture to use .dockerconfigjson key.
For corev1.SecretTypeDockerConfigJson secrets, the fixture should use ".dockerconfigjson" instead of "auth" to match Kubernetes API server expectations. The correct pattern is used in other tests (e.g., controllers/workspace/devworkspace_controller_test.go).
🤖 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 79 - 87, The secret fixture function
makeSecret currently populates Data with the "auth" key but should use the
Kubernetes-standard ".dockerconfigjson" key for
corev1.SecretTypeDockerConfigJson; update makeSecret to set Data to
map[string][]byte{".dockerconfigjson": []byte("dXNlcjpwYXNz")} so the fixture
matches API expectations and other tests (e.g.,
controllers/workspace/devworkspace_controller_test.go).
| It("returns the predefined secret when it exists in the workspace namespace", func() { | ||
| By("creating the predefined DevWorkspaceBackupAuthSecretName secret in the workspace namespace") | ||
| predefinedSecret := makeSecret(constants.DevWorkspaceBackupAuthSecretName, workspaceNS) | ||
|
|
||
| fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(predefinedSecret).Build() | ||
| workspace := makeWorkspace(workspaceNS) | ||
| config := makeConfig("quay-backup-auth") | ||
|
|
||
| result, err := secrets.HandleRegistryAuthSecret(ctx, fakeClient, workspace, config, "", scheme, log) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(result).NotTo(BeNil()) | ||
| Expect(result.Name).To(Equal(constants.DevWorkspaceBackupAuthSecretName)) | ||
| }) | ||
|
|
||
| It("returns nil when the predefined secret does not exist in the workspace namespace", func() { | ||
| By("using a fake client with no secrets") | ||
| fakeClient := fake.NewClientBuilder().WithScheme(scheme).Build() | ||
| workspace := makeWorkspace(workspaceNS) | ||
| config := makeConfig("quay-backup-auth") | ||
|
|
||
| result, err := secrets.HandleRegistryAuthSecret(ctx, fakeClient, workspace, config, "", scheme, log) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(result).To(BeNil()) | ||
| }) | ||
|
|
||
| It("propagates a non-NotFound error from the workspace namespace lookup", func() { | ||
| By("wrapping a fake client so that the predefined name lookup returns a server error") | ||
| errClient := &errorOnNameClient{ | ||
| Client: fake.NewClientBuilder().WithScheme(scheme).Build(), | ||
| failName: constants.DevWorkspaceBackupAuthSecretName, | ||
| failErr: k8sErrors.NewInternalError(errors.New("simulated etcd timeout")), | ||
| } | ||
| workspace := makeWorkspace(workspaceNS) | ||
| config := makeConfig("quay-backup-auth") | ||
|
|
||
| result, err := secrets.HandleRegistryAuthSecret(ctx, errClient, workspace, config, "", scheme, log) | ||
| Expect(err).To(HaveOccurred()) | ||
| Expect(result).To(BeNil()) | ||
| Expect(err.Error()).To(ContainSubstring("simulated etcd timeout")) | ||
| }) |
There was a problem hiding this comment.
Add restore-path coverage for configured-name-first behavior.
These cases only validate predefined-name lookup. To match the fallback requirement, add cases where:
- configured secret exists (should be used), and
- configured name equals predefined name (fallback path is skipped).
Without these, a direct-predefined lookup regression can pass tests.
🤖 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 104 - 143, Add two unit tests to
cover the "configured-name-first" behavior in HandleRegistryAuthSecret: (1)
create a secret named as the configured value (e.g., config :=
makeConfig("my-configured-secret")) in the workspace and assert
HandleRegistryAuthSecret returns that secret (not the predefined one), and (2)
set the configured name equal to constants.DevWorkspaceBackupAuthSecretName and
ensure the function still returns the predefined secret via the direct lookup
path (i.e., the fallback lookup must be skipped). Use the existing test patterns
(fake.NewClientBuilder(), makeWorkspace, makeSecret, ctx, scheme, log) and
assert errors/results the same way as the other cases so these new tests catch
regressions in the configured-name-first logic.
3 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