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

controllers/backupcronjob/backupcronjob_controller_test.go

@@ -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",
@@ -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{}
@@ -392,7 +397,8 @@ var _ = Describe("BackupCronJobReconciler", func() {
 							Schedule:     schedule,
 							BackoffLimit: &backoffLimit,
 							Registry: &controllerv1alpha1.RegistryConfig{
-								Path: "fake-registry",
+								Path:       "fake-registry",
+								AuthSecret: "backup-auth",
 							},
 						},
 					},
@@ -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{}
@@ -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())

docs/dwo-configuration.md

@@ -125,7 +125,7 @@ Backup CronJob configuration fields:
 The value provided for registry.path is only the first segment of the final location. The full registry path is assembled dynamically, incorporating the name of the workspace and the :latest tag, following this pattern:
 `<registry.path>/<devworkspace-name>:latest`
 
-- **`registry.authSecret`**: (Optional) The name of the Kubernetes Secret containing credentials to access the OCI registry. If not provided, it is assumed that the registry is public or uses integrated OpenShift registry.
+- **`registry.authSecret`**: (Optional) The name of the secret in the **operator namespace** to copy to workspace namespaces. The secret is always copied as `devworkspace-backup-registry-auth` in the workspace namespace. If not provided, backup/restore jobs proceed without authentication.
 - **`oras.extraArgs`**: (Optional) Additional arguments to pass to the `oras` CLI tool during push and pull operations.
 
 

main.go

@@ -192,7 +192,7 @@ func main() {
 	if err = (&backupCronJobController.BackupCronJobReconciler{
 		Client:           mgr.GetClient(),
 		NonCachingClient: nonCachingClient,
-		Log:              ctrl.Log.WithName("controllers").WithName("BackupCronJob").V(1),
+		Log:              ctrl.Log.WithName("controllers").WithName("BackupCronJob"),
 		Scheme:           mgr.GetScheme(),
 	}).SetupWithManager(mgr); err != nil {
 		setupLog.Error(err, "unable to create controller", "controller", "BackupCronJob")

pkg/secrets/backup.go

@@ -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
@@ -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 {
@@ -78,23 +64,41 @@ 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 {
+		log.Info("Registry auth secret not found in workspace namespace and not configured in operator config. "+
+			"Proceeding without authentication. Ensure your registry allows anonymous access or configure authentication if needed.",
+			"secretName", constants.DevWorkspaceBackupAuthSecretName,
+			"namespace", workspace.Namespace,
+			"registry", dwOperatorConfig.Workspace.BackupCronJob.Registry.Path)
+		return nil, nil
+	}
+
+	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
 	}
-	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
 	desiredSecret := &corev1.Secret{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      constants.DevWorkspaceBackupAuthSecretName,
@@ -111,27 +115,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,
+			}, sourceSecret); err != nil {
+				return nil, err
+			}
+			log.Info("Registry auth secret was created concurrently, using existing secret",
+				"secretName", constants.DevWorkspaceBackupAuthSecretName)
+			return sourceSecret, 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
 }

pkg/secrets/backup_test.go

@@ -143,6 +143,129 @@ 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 nil when AuthSecret is not configured and secret not found in workspace namespace (anonymous registry access)", 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).NotTo(HaveOccurred())
+		Expect(result).To(BeNil())
+	})
+
+	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")))
+
+		By("verifying the copied secret has the watch-secret label")
+		Expect(copiedSecret.Labels).To(HaveKeyWithValue(constants.DevWorkspaceWatchSecretLabel, "true"))
+
+		By("verifying the copied secret has an owner reference to the workspace")
+		Expect(copiedSecret.OwnerReferences).To(HaveLen(1))
+		Expect(copiedSecret.OwnerReferences[0].Name).To(Equal(workspace.Name))
+		Expect(copiedSecret.OwnerReferences[0].Kind).To(Equal("DevWorkspace"))
+		Expect(copiedSecret.OwnerReferences[0].Controller).NotTo(BeNil())
+		Expect(*copiedSecret.OwnerReferences[0].Controller).To(BeTrue())
+	})
+
+	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