Skip to content

refactor (backup) : conditionally copy registry auth secret from operator namespace to workspace namespace for backup/restore #1618

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
rohanKanojia wants to merge 1 commit into
base: main
Choose a base branch
from
+190 −45
Draft

refactor (backup) : conditionally copy registry auth secret from operator namespace to workspace namespace for backup/restore #1618

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions controllers/backupcronjob/backupcronjob_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,8 @@ var _ = Describe("BackupCronJobReconciler", func() {
Enable: &enabled,
Schedule: schedule,
Registry: &controllerv1alpha1.RegistryConfig{
Path: "fake-registry",
Path: "fake-registry",
AuthSecret: "backup-auth",
},
OrasConfig: &controllerv1alpha1.OrasConfig{
ExtraArgs: "--extra-arg1",
Expand All @@ -353,6 +354,10 @@ var _ = Describe("BackupCronJobReconciler", func() {
pvc := &corev1.PersistentVolumeClaim{ObjectMeta: metav1.ObjectMeta{Name: "claim-devworkspace", Namespace: dw.Namespace}}
Expect(fakeClient.Create(ctx, pvc)).To(Succeed())

// Create auth secret in operator namespace so it can be copied
authSecret := createAuthSecret("backup-auth", nameNamespace.Namespace, map[string][]byte{})
Expect(fakeClient.Create(ctx, authSecret)).To(Succeed())

Expect(reconciler.executeBackupSync(ctx, dwoc, log)).To(Succeed())

jobList := &batchv1.JobList{}
Expand Down Expand Up @@ -392,7 +397,8 @@ var _ = Describe("BackupCronJobReconciler", func() {
Schedule: schedule,
BackoffLimit: &backoffLimit,
Registry: &controllerv1alpha1.RegistryConfig{
Path: "fake-registry",
Path: "fake-registry",
AuthSecret: "backup-auth",
},
},
},
Expand All @@ -407,6 +413,10 @@ var _ = Describe("BackupCronJobReconciler", func() {
pvc := &corev1.PersistentVolumeClaim{ObjectMeta: metav1.ObjectMeta{Name: "claim-devworkspace", Namespace: dw.Namespace}}
Expect(fakeClient.Create(ctx, pvc)).To(Succeed())

// Create auth secret in operator namespace so it can be copied
authSecret := createAuthSecret("backup-auth", nameNamespace.Namespace, map[string][]byte{})
Expect(fakeClient.Create(ctx, authSecret)).To(Succeed())

Expect(reconciler.executeBackupSync(ctx, dwoc, log)).To(Succeed())

jobList := &batchv1.JobList{}
Expand Down Expand Up @@ -552,7 +562,8 @@ var _ = Describe("BackupCronJobReconciler", func() {
pvc := &corev1.PersistentVolumeClaim{ObjectMeta: metav1.ObjectMeta{Name: "claim-devworkspace", Namespace: dw.Namespace}}
Expect(fakeClient.Create(ctx, pvc)).To(Succeed())

authSecret := createAuthSecret("my-secret", "ns-a", map[string][]byte{})
// User-provided secret in workspace namespace with canonical name
authSecret := createAuthSecret("devworkspace-backup-registry-auth", "ns-a", map[string][]byte{})
Expect(fakeClient.Create(ctx, authSecret)).To(Succeed())

Expect(reconciler.executeBackupSync(ctx, dwoc, log)).To(Succeed())
Expand Down
103 changes: 61 additions & 42 deletions pkg/secrets/backup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,11 @@ import (
"github.com/devfile/devworkspace-operator/pkg/constants"
"github.com/go-logr/logr"
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/controller/controllerutil"

"github.com/devfile/devworkspace-operator/pkg/provision/sync"
)

// GetRegistryAuthSecret retrieves the registry authentication secret for accessing backup images
Expand All @@ -48,27 +47,14 @@ func HandleRegistryAuthSecret(ctx context.Context, c client.Client, workspace *d
dwOperatorConfig.Workspace.BackupCronJob.Registry == nil {
return nil, fmt.Errorf("backup/restore configuration not properly set in DevWorkspaceOperatorConfig")
}
secretName := dwOperatorConfig.Workspace.BackupCronJob.Registry.AuthSecret
if secretName == "" {
// No auth secret configured - anonymous access to registry
return nil, nil
}

// On the restore path (operatorConfigNamespace == ""), look for the predefined name
// that CopySecret always uses. On the backup path, look for the configured name
// because the secret may exist directly in the workspace namespace under that name.
lookupName := secretName
if operatorConfigNamespace == "" {
lookupName = constants.DevWorkspaceBackupAuthSecretName
}

registryAuthSecret := &corev1.Secret{}
err := c.Get(ctx, client.ObjectKey{
Name: lookupName,
Name: constants.DevWorkspaceBackupAuthSecretName,
Namespace: workspace.Namespace}, registryAuthSecret)
if err == nil {
log.Info("Successfully retrieved registry auth secret for backup from workspace namespace",
"secretName", lookupName)
"secretName", constants.DevWorkspaceBackupAuthSecretName)
return registryAuthSecret, nil
}
if client.IgnoreNotFound(err) != nil {
Expand All @@ -78,23 +64,58 @@ func HandleRegistryAuthSecret(ctx context.Context, c client.Client, workspace *d
if operatorConfigNamespace == "" {
return nil, nil
}
log.Info("Registry auth secret not found in workspace namespace, checking operator namespace", "secretName", secretName)

// If the secret is not found in the workspace namespace, check the operator namespace as fallback
// Check if AuthSecret is configured in operator config
authSecretName := dwOperatorConfig.Workspace.BackupCronJob.Registry.AuthSecret
if len(authSecretName) == 0 {
return nil, fmt.Errorf(
"registry auth secret %q not found in workspace namespace %q and AuthSecret is not configured in DevWorkspaceOperatorConfig. "+
"Either create the secret in the workspace namespace or configure Registry.AuthSecret in the operator config to enable copying from the operator namespace",
constants.DevWorkspaceBackupAuthSecretName,
workspace.Namespace,
)
}

log.Info("Registry auth secret not found in workspace namespace, checking operator namespace",
"secretName", authSecretName,
"operatorNamespace", operatorConfigNamespace)

// Look for the configured secret name in operator namespace
err = c.Get(ctx, client.ObjectKey{
Name: secretName,
Name: authSecretName,
Namespace: operatorConfigNamespace}, registryAuthSecret)
if err != nil {
log.Error(err, "Failed to get registry auth secret for backup job", "secretName", secretName)
log.Error(err, "Failed to get registry auth secret for backup job",
"secretName", authSecretName,
"namespace", operatorConfigNamespace)
return nil, err
}
Comment on lines +67 to 92
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Admin-configured Registry.AuthSecret name is silently ignored.

The gate at line 69 only checks whether Registry.AuthSecret is non-empty, but the actual operator-namespace lookup at line 84 uses the hard-coded constants.DevWorkspaceBackupAuthSecretName. Net effect: an admin who configures AuthSecret: "my-org-pull-secret" will hit a confusing NotFound for devworkspace-backup-registry-auth in the operator namespace, with no hint that the configured name was disregarded. This matches the concern raised on the PR that the configured AuthSecret name is effectively ignored by the lookup logic.

There are also two contradictory readings of the same field in this code path:

  • The error string at line 71-72 tells admins to "configure Registry.AuthSecret in the operator config to enable copying", treating it as a boolean.
  • The CRD/type defines AuthSecret as a name (string), so admins reasonably expect it to be the name used for the lookup.

Pick one model and stick to it:

  1. Treat AuthSecret as a name (preferred, matches the field's existing semantics): use it for the operator-namespace Get, and only fall through if it is set.
  2. Use a dedicated boolean (copyOperatorAuthSecret mentioned in the PR description) for the toggle, and keep AuthSecret as the source name in the operator namespace.

Either way, the configured value should not be silently dropped.

🛠️ Sketch for option 1 (use AuthSecret as the operator-namespace name) 
-	// Check if AuthSecret is configured in operator config
-	if len(dwOperatorConfig.Workspace.BackupCronJob.Registry.AuthSecret) == 0 {
+	operatorAuthSecretName := dwOperatorConfig.Workspace.BackupCronJob.Registry.AuthSecret
+	if operatorAuthSecretName == "" {
 		return nil, fmt.Errorf(
 			"registry auth secret %q not found in workspace namespace %q and AuthSecret is not configured in DevWorkspaceOperatorConfig. "+
 				"Either create the secret in the workspace namespace or configure Registry.AuthSecret in the operator config to enable copying from the operator namespace",
 			constants.DevWorkspaceBackupAuthSecretName,
 			workspace.Namespace,
 		)
 	}

 	log.Info("Registry auth secret not found in workspace namespace, checking operator namespace",
-		"secretName", constants.DevWorkspaceBackupAuthSecretName,
+		"secretName", operatorAuthSecretName,
 		"operatorNamespace", operatorConfigNamespace)

 	// If the secret is not found in the workspace namespace, check the operator namespace as fallback
 	err = c.Get(ctx, client.ObjectKey{
-		Name:      constants.DevWorkspaceBackupAuthSecretName,
+		Name:      operatorAuthSecretName,
 		Namespace: operatorConfigNamespace}, registryAuthSecret)

The corresponding tests in backup_test.go (e.g. lines 193-217, 248-258) would need to seed/reference operatorAuthSecretName rather than constants.DevWorkspaceBackupAuthSecretName in the operator 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 `@pkg/secrets/backup.go` around lines 67 - 89, The code currently ignores the
admin-configured name in
dwOperatorConfig.Workspace.BackupCronJob.Registry.AuthSecret and always looks up
constants.DevWorkspaceBackupAuthSecretName in the operator namespace; change the
operator-namespace lookup (the c.Get call that populates registryAuthSecret) to
use the configured name
(dwOperatorConfig.Workspace.BackupCronJob.Registry.AuthSecret) instead of the
constant, and update the preceding error/log message to reflect that the
configured name will be used for the operator-namespace lookup; also update
related tests (backup_test.go) to seed the operator namespace with the
configured secret name rather than constants.DevWorkspaceBackupAuthSecretName.

log.Info("Successfully retrieved registry auth secret for backup job", "secretName", secretName)
log.Info("Successfully retrieved registry auth secret from operator namespace",
"secretName", authSecretName)
return CopySecret(ctx, c, workspace, registryAuthSecret, scheme, log)
}

// CopySecret copies the given secret from the operator namespace to the workspace namespace.
// It NEVER overwrites an existing secret: if a secret already exists in the workspace namespace,
// it returns the existing secret without modification.
func CopySecret(ctx context.Context, c client.Client, workspace *dw.DevWorkspace, sourceSecret *corev1.Secret, scheme *runtime.Scheme, log logr.Logger) (namespaceSecret *corev1.Secret, err error) {
// Construct the desired secret state
// First check if secret already exists in workspace namespace
existingSecret := &corev1.Secret{}
err = c.Get(ctx, client.ObjectKey{
Name: constants.DevWorkspaceBackupAuthSecretName,
Namespace: workspace.Namespace,
}, existingSecret)

if err == nil {
log.Info("Registry auth secret already exists in workspace namespace, using existing secret",
"secretName", constants.DevWorkspaceBackupAuthSecretName)
return existingSecret, nil
}

if client.IgnoreNotFound(err) != nil {
return nil, err
}

desiredSecret := &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: constants.DevWorkspaceBackupAuthSecretName,
Expand All @@ -111,27 +132,25 @@ func CopySecret(ctx context.Context, c client.Client, workspace *dw.DevWorkspace
return nil, err
}

// Use the sync mechanism
clusterAPI := sync.ClusterAPI{
Client: c,
Scheme: scheme,
Logger: log,
Ctx: ctx,
}

syncedObj, err := sync.SyncObjectWithCluster(desiredSecret, clusterAPI)
err = c.Create(ctx, desiredSecret)
if err != nil {
if _, ok := err.(*sync.NotInSyncError); !ok {
return nil, err
if k8sErrors.IsAlreadyExists(err) {
// Race condition - secret was created between Get and Create
// Fetch and return it (respect what's there)
if err := c.Get(ctx, client.ObjectKey{
Name: constants.DevWorkspaceBackupAuthSecretName,
Namespace: workspace.Namespace,
}, existingSecret); err != nil {
return nil, err
}
log.Info("Registry auth secret was created concurrently, using existing secret",
"secretName", constants.DevWorkspaceBackupAuthSecretName)
return existingSecret, nil
}
// NotInSyncError means the sync operation was successful but triggered a change
log.Info("Successfully synced secret", "name", desiredSecret.Name, "namespace", workspace.Namespace)
}

// If syncedObj is nil (due to NotInSyncError), return the desired object
if syncedObj == nil {
return desiredSecret, nil
return nil, err
}

return syncedObj.(*corev1.Secret), nil
log.Info("Successfully copied registry auth secret to workspace namespace",
"name", desiredSecret.Name, "namespace", workspace.Namespace)
return desiredSecret, nil
}
115 changes: 115 additions & 0 deletions pkg/secrets/backup_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,121 @@ var _ = Describe("HandleRegistryAuthSecret (restore path: operatorConfigNamespac
})
})

var _ = Describe("HandleRegistryAuthSecret (backup path: operatorConfigNamespace set)", func() {
const (
workspaceNS = "user-namespace"
operatorNS = "devworkspace-controller"
)

var (
ctx context.Context
scheme *runtime.Scheme
log = zap.New(zap.UseDevMode(true)).WithName("SecretsTest")
)

BeforeEach(func() {
ctx = context.Background()
scheme = buildScheme()
})

It("returns the secret from workspace namespace if it exists", func() {
By("creating a secret in the workspace namespace")
workspaceSecret := makeSecret(constants.DevWorkspaceBackupAuthSecretName, workspaceNS)
workspaceSecret.Data = map[string][]byte{"auth": []byte("user-credentials")}

fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(workspaceSecret).Build()
workspace := makeWorkspace(workspaceNS)
config := makeConfig(constants.DevWorkspaceBackupAuthSecretName)

result, err := secrets.HandleRegistryAuthSecret(ctx, fakeClient, workspace, config, operatorNS, scheme, log)
Expect(err).NotTo(HaveOccurred())
Expect(result).NotTo(BeNil())
Expect(result.Name).To(Equal(constants.DevWorkspaceBackupAuthSecretName))
Expect(result.Namespace).To(Equal(workspaceNS))
Expect(result.Data["auth"]).To(Equal([]byte("user-credentials")))
})

It("returns error when AuthSecret is not configured and secret not found in workspace namespace", func() {
By("using a config with empty AuthSecret")
fakeClient := fake.NewClientBuilder().WithScheme(scheme).Build()
workspace := makeWorkspace(workspaceNS)
config := makeConfig("") // Empty AuthSecret

result, err := secrets.HandleRegistryAuthSecret(ctx, fakeClient, workspace, config, operatorNS, scheme, log)
Expect(err).To(HaveOccurred())
Expect(result).To(BeNil())
Expect(err.Error()).To(ContainSubstring("AuthSecret is not configured"))
Expect(err.Error()).To(ContainSubstring(workspaceNS))
})

It("copies secret from operator namespace when AuthSecret is configured and secret not found in workspace namespace", func() {
By("creating a secret in the operator namespace")
operatorSecret := makeSecret(constants.DevWorkspaceBackupAuthSecretName, operatorNS)
operatorSecret.Data = map[string][]byte{"auth": []byte("operator-credentials")}

fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(operatorSecret).Build()
workspace := makeWorkspace(workspaceNS)
config := makeConfig(constants.DevWorkspaceBackupAuthSecretName)

result, err := secrets.HandleRegistryAuthSecret(ctx, fakeClient, workspace, config, operatorNS, scheme, log)
Expect(err).NotTo(HaveOccurred())
Expect(result).NotTo(BeNil())
Expect(result.Name).To(Equal(constants.DevWorkspaceBackupAuthSecretName))
Expect(result.Namespace).To(Equal(workspaceNS))
Expect(result.Data["auth"]).To(Equal([]byte("operator-credentials")))

By("verifying the secret was created in the workspace namespace")
copiedSecret := &corev1.Secret{}
err = fakeClient.Get(ctx, client.ObjectKey{
Name: constants.DevWorkspaceBackupAuthSecretName,
Namespace: workspaceNS,
}, copiedSecret)
Expect(err).NotTo(HaveOccurred())
Expect(copiedSecret.Data["auth"]).To(Equal([]byte("operator-credentials")))
})

It("NEVER overwrites user-provided secret even if operator has different credentials", func() {
By("creating different secrets in both namespaces")
userSecret := makeSecret(constants.DevWorkspaceBackupAuthSecretName, workspaceNS)
userSecret.Data = map[string][]byte{"auth": []byte("user-scoped-credentials")}

operatorSecret := makeSecret(constants.DevWorkspaceBackupAuthSecretName, operatorNS)
operatorSecret.Data = map[string][]byte{"auth": []byte("operator-wide-credentials")}

fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(userSecret, operatorSecret).Build()
workspace := makeWorkspace(workspaceNS)
config := makeConfig(constants.DevWorkspaceBackupAuthSecretName)

result, err := secrets.HandleRegistryAuthSecret(ctx, fakeClient, workspace, config, operatorNS, scheme, log)
Expect(err).NotTo(HaveOccurred())
Expect(result).NotTo(BeNil())

By("verifying the user's secret was NOT overwritten")
Expect(result.Data["auth"]).To(Equal([]byte("user-scoped-credentials")), "User's secret should be preserved")

By("verifying the secret in workspace namespace still has user's credentials")
workspaceSecret := &corev1.Secret{}
err = fakeClient.Get(ctx, client.ObjectKey{
Name: constants.DevWorkspaceBackupAuthSecretName,
Namespace: workspaceNS,
}, workspaceSecret)
Expect(err).NotTo(HaveOccurred())
Expect(workspaceSecret.Data["auth"]).To(Equal([]byte("user-scoped-credentials")), "User's secret must never be overwritten")
})

It("returns error when AuthSecret is configured but secret not found in operator namespace", func() {
By("using a config with AuthSecret but no secret in operator namespace")
fakeClient := fake.NewClientBuilder().WithScheme(scheme).Build()
workspace := makeWorkspace(workspaceNS)
config := makeConfig(constants.DevWorkspaceBackupAuthSecretName)

result, err := secrets.HandleRegistryAuthSecret(ctx, fakeClient, workspace, config, operatorNS, scheme, log)
Expect(err).To(HaveOccurred())
Expect(result).To(BeNil())
Expect(k8sErrors.IsNotFound(err)).To(BeTrue(), "Should return a NotFound error when secret doesn't exist in operator namespace")
})
})

// errorOnNameClient is a thin client wrapper that injects an error for a specific secret name.
type errorOnNameClient struct {
client.Client
Expand Down
Loading