feat (backup) : add option to conditionally copy registry auth secret from operator namespace to workspace namespace for backup/restore#1618
Open
rohanKanojia wants to merge 2 commits intodevfile:mainfrom
Conversation
|
Skipping CI for Draft Pull Request. |
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThis PR adds a new ChangesRegistry Auth Secret Copy Control
Sequence DiagramsequenceDiagram
participant BC as Backup Controller
participant HSH as HandleRegistryAuthSecret
participant ESH as EnsureRegistryAuthSecret
participant K8s as Kubernetes API
participant WS as Workspace Namespace
BC->>HSH: HandleRegistryAuthSecret(config, workspace, operator-secret)
HSH->>HSH: Determine copyOperatorAuthSecret flag<br/>(default: false)
HSH->>ESH: EnsureRegistryAuthSecret(copyFlag, source-secret)
ESH->>K8s: Get workspace secret<br/>by name
alt Workspace secret exists
K8s-->>ESH: Existing secret
ESH-->>HSH: Return existing secret
else Workspace secret missing
alt copyOperatorAuthSecret = false
ESH-->>HSH: Error: manual secret creation required
else copyOperatorAuthSecret = true
ESH->>K8s: Create secret in workspace<br/>from operator secret
K8s->>WS: Write secret with labels<br/>& owner reference
WS-->>K8s: Success (or AlreadyExists)
K8s-->>ESH: Created/fetched secret
ESH-->>HSH: Return workspace secret
end
end
HSH-->>BC: Auth secret result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 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 |
rohanKanojia
force-pushed
the
pr/secret-fallback-option
branch
from
56c6857 to
f6f485c
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1618 +/- ##
==========================================
+ Coverage 35.48% 36.94% +1.46%
==========================================
Files 168 168
Lines 14484 14726 +242
==========================================
+ Hits 5139 5441 +302
+ Misses 9006 8932 -74
- Partials 339 353 +14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rohanKanojia
force-pushed
the
pr/secret-fallback-option
branch
3 times, most recently
from
802c09c to
87f720c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
controllers/backupcronjob/backupcronjob_controller_test.go (1)
312-314: ⚡ Quick winAdd one unset/false-path test for
CopyOperatorAuthSecret.All touched
executeBackupSyncfixtures now forceCopyOperatorAuthSecret=true, so this suite no longer validates the new default/disabled branch. Please add at least one case for unset/false (especially missing workspace secret) to guard the new behavior.Also applies to: 345-346, 403-404, 440-441, 482-483, 514-516, 551-553, 588-589
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controllers/backupcronjob/backupcronjob_controller_test.go` around lines 312 - 314, Tests in backupcronjob_controller_test.go currently set CopyOperatorAuthSecret = pointer.Bool(true) in all executeBackupSync fixtures, so the default/disabled branch (unset/false) and the missing workspace secret path are not covered; add at least one test case for executeBackupSync where CopyOperatorAuthSecret is either nil (unset) or pointer.Bool(false) and the workspace secret is absent, then assert the controller follows the disabled behavior (e.g., does not attempt to copy operator auth and returns the expected status/condition). Update the relevant table/fixtures used by TestExecuteBackupSync (or similarly named test harness) to include this false/unset case and verify expected outcomes for missing workspace secret to guard the new branch.deploy/bundle/manifests/controller.devfile.io_devworkspaceoperatorconfigs.yaml (1)
222-240: ⚡ Quick winSet an explicit CRD default for
copyOperatorAuthSecret.Line 239 says the field defaults to false, but the schema currently has no
default: false. Adding it makes the API contract enforceable and avoids nil/merge ambiguity when the field is omitted.Suggested schema tweak
copyOperatorAuthSecret: + default: false description: |- CopyOperatorAuthSecret controls whether the operator should copy the authentication secret from the operator namespace to the workspace namespace when it's not found in the workspace namespace.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/bundle/manifests/controller.devfile.io_devworkspaceoperatorconfigs.yaml` around lines 222 - 240, The CRD schema for the boolean field copyOperatorAuthSecret currently documents "Defaults to false" but lacks an explicit default; update the OpenAPI schema for the field copyOperatorAuthSecret (the property under the spec schema) to include default: false so the CRD enforces the default value when the field is omitted, ensuring API server/merging behavior matches the documented contract.pkg/secrets/backup.go (2)
101-176: 💤 Low valueLogic and race handling look correct.
EnsureRegistryAuthSecretcorrectly: (1) never overwrites a user-provided secret, (2) returns a clear, actionable error whencopyOperatorAuthSecretis false, (3) handles the Get→Create race viaIsAlreadyExistsre-fetch, and (4) sets the watch label and controller reference on the created secret.Two minor observations (no action required, just flagging for awareness):
- The relevant code snippet from
pkg/provision/sync/sync.goshowsSyncObjectWithClusterprovides diff/update behavior. Bypassing it here is the right call since the explicit goal is to never overwrite — just make sure no future caller expects this function to reconcile drift in the workspace secret (e.g., if the operator's source secret rotates, the workspace copy will not be updated).- The error message at lines 125–134 references
DevWorkspaceBackupAuthSecretName(the predefined name) for both the "not found" name and the "please create" name. That's consistent with the rest of the function but means the user-facing message will not mention the configuredsecretNamefromRegistry.AuthSecret, which can be confusing if the admin configured a custom name. Consider including both names in the error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/secrets/backup.go` around lines 101 - 176, Update the error returned when copyOperatorAuthSecret is false in EnsureRegistryAuthSecret to include both the workspace secret name (constants.DevWorkspaceBackupAuthSecretName) and the configured/operator secret name (use sourceSecret.Name) so users see which configured auth secret name the operator expects versus the name missing in the workspace; change the fmt.Errorf in the !copyOperatorAuthSecret branch inside EnsureRegistryAuthSecret to format and include sourceSecret.Name alongside constants.DevWorkspaceBackupAuthSecretName.
30-30: ⚡ Quick winReplace deprecated
k8s.io/utils/pointerwithk8s.io/utils/ptr.
golangci-lint(SA1019) reports thatk8s.io/utils/pointeris deprecated. Useptr.Derefinstead ofpointer.BoolDeref.♻️ Proposed fix
- "k8s.io/utils/pointer" + "k8s.io/utils/ptr"- copyOperatorAuthSecret := pointer.BoolDeref( - dwOperatorConfig.Workspace.BackupCronJob.Registry.CopyOperatorAuthSecret, - false, - ) + copyOperatorAuthSecret := ptr.Deref( + dwOperatorConfig.Workspace.BackupCronJob.Registry.CopyOperatorAuthSecret, + false, + )k8s.io/utils/ptr Deref function signature🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/secrets/backup.go` at line 30, Replace the deprecated import and calls to pointer.*: change the import "k8s.io/utils/pointer" to "k8s.io/utils/ptr" and update call sites using pointer.BoolDeref (and any other pointer.XDeref) to use ptr.Deref instead, e.g. replace pointer.BoolDeref(someBoolPtr, false) with ptr.Deref(someBoolPtr, false) in the functions/methods in pkg/secrets/backup.go.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@deploy/deployment/kubernetes/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yaml`:
- Around line 226-244: The CRD documents that copyOperatorAuthSecret defaults to
false but the schema lacks an enforced default; add a JSON schema default by
setting default: false for the copyOperatorAuthSecret property in the CRD (the
YAML under copyOperatorAuthSecret) so the API server will apply the default. If
the CRD is generated from Go types, add the kubebuilder default marker (e.g., //
+kubebuilder:default=false) to the Go field named CopyOperatorAuthSecret (or
copyOperatorAuthSecret) in the API type, regenerate the CRD manifests, and
verify the generated CRD includes default: false under the
copyOperatorAuthSecret schema.
In `@deploy/deployment/openshift/combined.yaml`:
- Around line 226-244: The CRD OpenAPI schema for the copyOperatorAuthSecret
boolean lacks an explicit default; add "default: false" to the property
definition in the combined.yaml CRD schema so the OpenAPIv3 spec matches the
documented and Go behavior (pointer.BoolDeref(..., false)); also update any CRD
description if needed to note this change is a breaking behavioral change for
users who relied on true.
In
`@deploy/deployment/openshift/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yaml`:
- Around line 223-224: The sentence about the
"controller.devfile.io/watch-secret=true" label is ambiguous: update the text
that references authSecret to state this label requirement only applies to
operator-managed (copied) secrets when copyOperatorAuthSecret is enabled (i.e.,
the operator-created/copy-source path), and clarify that user-provided/manual
authSecret values do not need that label; reference the fields authSecret and
copyOperatorAuthSecret and explicitly mention "operator-managed/copy-source
secrets" and "user-provided/manual secrets" so readers know which secrets must
have controller.devfile.io/watch-secret=true.
In
`@deploy/templates/crd/bases/controller.devfile.io_devworkspaceoperatorconfigs.yaml`:
- Around line 224-242: The CRD schema for the boolean property
copyOperatorAuthSecret declares "Defaults to false" in the description but lacks
an OpenAPI default; update the CRD schema for the copyOperatorAuthSecret
property by adding default: false alongside its type: boolean (i.e., under the
copyOperatorAuthSecret property in the CRD's schema) so the generated API docs
and schema reflect the described default behavior.
In `@pkg/secrets/backup.go`:
- Around line 57-61: The change introduced a breaking default (currently using
pointer.BoolDeref(..., false)) for
dwOperatorConfig.Workspace.BackupCronJob.Registry.CopyOperatorAuthSecret which
stops auto-copying the operator namespace secret; restore backward-compatible
behavior by changing the default to true in the copyOperatorAuthSecret
initialization (i.e., use pointer.BoolDeref(..., true)) so existing clusters
continue to auto-copy, and if you intentionally want false instead, update
docs/changelog and tests that rely on auto-copy (e.g.,
controllers/backupcronjob/backupcronjob_controller_test.go) to avoid upgrade
surprises.
---
Nitpick comments:
In `@controllers/backupcronjob/backupcronjob_controller_test.go`:
- Around line 312-314: Tests in backupcronjob_controller_test.go currently set
CopyOperatorAuthSecret = pointer.Bool(true) in all executeBackupSync fixtures,
so the default/disabled branch (unset/false) and the missing workspace secret
path are not covered; add at least one test case for executeBackupSync where
CopyOperatorAuthSecret is either nil (unset) or pointer.Bool(false) and the
workspace secret is absent, then assert the controller follows the disabled
behavior (e.g., does not attempt to copy operator auth and returns the expected
status/condition). Update the relevant table/fixtures used by
TestExecuteBackupSync (or similarly named test harness) to include this
false/unset case and verify expected outcomes for missing workspace secret to
guard the new branch.
In
`@deploy/bundle/manifests/controller.devfile.io_devworkspaceoperatorconfigs.yaml`:
- Around line 222-240: The CRD schema for the boolean field
copyOperatorAuthSecret currently documents "Defaults to false" but lacks an
explicit default; update the OpenAPI schema for the field copyOperatorAuthSecret
(the property under the spec schema) to include default: false so the CRD
enforces the default value when the field is omitted, ensuring API
server/merging behavior matches the documented contract.
In `@pkg/secrets/backup.go`:
- Around line 101-176: Update the error returned when copyOperatorAuthSecret is
false in EnsureRegistryAuthSecret to include both the workspace secret name
(constants.DevWorkspaceBackupAuthSecretName) and the configured/operator secret
name (use sourceSecret.Name) so users see which configured auth secret name the
operator expects versus the name missing in the workspace; change the fmt.Errorf
in the !copyOperatorAuthSecret branch inside EnsureRegistryAuthSecret to format
and include sourceSecret.Name alongside
constants.DevWorkspaceBackupAuthSecretName.
- Line 30: Replace the deprecated import and calls to pointer.*: change the
import "k8s.io/utils/pointer" to "k8s.io/utils/ptr" and update call sites using
pointer.BoolDeref (and any other pointer.XDeref) to use ptr.Deref instead, e.g.
replace pointer.BoolDeref(someBoolPtr, false) with ptr.Deref(someBoolPtr, false)
in the functions/methods in pkg/secrets/backup.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: 58f3b582-37ca-446d-9ace-6bf37dec79a5
📒 Files selected for processing (12)
apis/controller/v1alpha1/devworkspaceoperatorconfig_types.goapis/controller/v1alpha1/zz_generated.deepcopy.gocontrollers/backupcronjob/backupcronjob_controller_test.godeploy/bundle/manifests/controller.devfile.io_devworkspaceoperatorconfigs.yamldeploy/deployment/kubernetes/combined.yamldeploy/deployment/kubernetes/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yamldeploy/deployment/openshift/combined.yamldeploy/deployment/openshift/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yamldeploy/templates/crd/bases/controller.devfile.io_devworkspaceoperatorconfigs.yamlpkg/config/sync.gopkg/secrets/backup.gopkg/secrets/backup_test.go
Comment on lines
223
to
224
| The secret must contain "controller.devfile.io/watch-secret=true" label so that it can be | ||
| recognized by the operator. |
There was a problem hiding this comment.
Clarify label requirement scope for user-provided secrets
Line 223 currently reads as if all authSecret values must have controller.devfile.io/watch-secret=true. That conflicts with the manual-secret path when copyOperatorAuthSecret is disabled. Please scope this sentence to operator-managed/copy-source secrets only.
✏️ Proposed wording tweak
- The secret must contain "controller.devfile.io/watch-secret=true" label so that it can be
- recognized by the operator.
+ For operator-managed secret copying, the source secret in the operator namespace must contain
+ "controller.devfile.io/watch-secret=true" so the operator can recognize and manage it.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| The secret must contain "controller.devfile.io/watch-secret=true" label so that it can be | |
| recognized by the operator. | |
| For operator-managed secret copying, the source secret in the operator namespace must contain | |
| "controller.devfile.io/watch-secret=true" so the operator can recognize and manage it. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@deploy/deployment/openshift/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yaml`
around lines 223 - 224, The sentence about the
"controller.devfile.io/watch-secret=true" label is ambiguous: update the text
that references authSecret to state this label requirement only applies to
operator-managed (copied) secrets when copyOperatorAuthSecret is enabled (i.e.,
the operator-created/copy-source path), and clarify that user-provided/manual
authSecret values do not need that label; reference the fields authSecret and
copyOperatorAuthSecret and explicitly mention "operator-managed/copy-source
secrets" and "user-provided/manual secrets" so readers know which secrets must
have controller.devfile.io/watch-secret=true.
Add optional copyOperatorAuthSecret field to RegistryConfig to control whether operator copies registry auth secrets to workspace namespaces. Defaults to true for backward compatibility. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Rohan Kumar <rohaan@redhat.com>
Refactor CopySecret to respect copyOperatorAuthSecret flag and never overwrite existing user secrets. When flag is false, return error requiring users to provide their own credentials. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Rohan Kumar <rohaan@redhat.com>
rohanKanojia
force-pushed
the
pr/secret-fallback-option
branch
from
87f720c to
0f8ddda
Compare
|
@rohanKanojia: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
dkwon17
reviewed
May 5, 2026
Comment on lines
+93
to
+95
| // If true: The operator will copy the secret from the operator namespace | ||
| // if it's not found in the workspace namespace. This provides automatic configuration | ||
| // but exposes operator-level credentials to workspace users. |
Collaborator
There was a problem hiding this comment.
I think it's a bit verbose,
Can we update thee If true... section to:
If true, this enables the fallback mechanism where the operator will copy the secret from the operator namespace.
dkwon17
reviewed
May 5, 2026
| // | ||
| // If false (default): The operator will not copy the secret. Users must manually create a secret | ||
| // with the configured name in their workspace namespace. This is more secure as it allows | ||
| // users to provide scoped credentials with minimal privileges. |
Collaborator
There was a problem hiding this comment.
I think it's a bit verbose,
Can we update the If false... section to:
If false, the operator will not copy the secret to workspace namespaces.
dkwon17
reviewed
May 5, 2026
| // users to provide scoped credentials with minimal privileges. | ||
| // | ||
| // Note: Regardless of this setting, if a secret already exists in the workspace namespace, | ||
| // it will never be overwritten. User-provided secrets are always respected. |
Collaborator
There was a problem hiding this comment.
Suggested change
| // it will never be overwritten. User-provided secrets are always respected. | |
| // it will never be overwritten. |
dkwon17
reviewed
May 5, 2026
| // Extract flag value (default: false, users must provide their own secret) | ||
| copyOperatorAuthSecret := pointer.BoolDeref( | ||
| dwOperatorConfig.Workspace.BackupCronJob.Registry.CopyOperatorAuthSecret, | ||
| false, |
Collaborator
There was a problem hiding this comment.
Could we instead define the default field here?
dkwon17
reviewed
May 5, 2026
| return nil, fmt.Errorf( | ||
| "registry auth secret %q not found in workspace namespace %q. "+ | ||
| "copyOperatorAuthSecret is set to false, so the operator will not copy the secret. "+ | ||
| "Please manually create a secret of type kubernetes.io/dockerconfigjson with the name %q "+ |
Collaborator
There was a problem hiding this comment.
Suggested change
| "Please manually create a secret of type kubernetes.io/dockerconfigjson with the name %q "+ | |
| "Please create a secret of type kubernetes.io/dockerconfigjson with the name %q "+ |
dkwon17
reviewed
May 5, 2026
| if !copyOperatorAuthSecret { | ||
| return nil, fmt.Errorf( | ||
| "registry auth secret %q not found in workspace namespace %q. "+ | ||
| "copyOperatorAuthSecret is set to false, so the operator will not copy the secret. "+ |
Collaborator
There was a problem hiding this comment.
Suggested change
| "copyOperatorAuthSecret is set to false, so the operator will not copy the secret. "+ | |
| "copyOperatorAuthSecret is set to false, the secret will not be copied. "+ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Adds a new
copyOperatorAuthSecretconfiguration flag to control whether the DevWorkspace Operator automatically copies registry authentication secrets from the operator namespace to workspace namespaces for backup/restore operations.Breaking Change: The default value for
copyOperatorAuthSecretisfalse(was implicitlytruebefore).copyOperatorAuthSecretfield toRegistryConfigAPI (defaults tofalse)CopySecret()to respect the flag and never overwrite user-provided secretscopyOperatorAuthSecret: false (default), the operator will not copy secrets and returns a clear error message instructing users to create their own scoped credentialscopyOperatorAuthSecret: true, maintains existing behavior - automatically copies operator's secret to workspace namespace if not foundWhat issues does this PR fix or reference?
https://redhat.atlassian.net/browse/CRW-10758
Is it tested? How?
Scenario 1: Default Behavior `copyOperatorAuthSecret` not set (defaults to `false`)
Scenario 2: Explicit Secret Copying with copyOperatorAuthSecret: true
Scenario 3 : User Provides Secret (copyOperatorAuthSecret: false)
Scenario 4: copyOperatorAuthSecret is false and no manual secret exists.
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
Release Notes
New Features
copyOperatorAuthSecretconfiguration option to control whether the operator automatically copies authentication secrets from the operator namespace to workspace namespaces when workspace-level secrets are unavailable.Bug Fixes
Documentation