diff --git a/controllers/backupcronjob/backupcronjob_controller_test.go b/controllers/backupcronjob/backupcronjob_controller_test.go index 134bdb17e..d8c7858ed 100644 --- a/controllers/backupcronjob/backupcronjob_controller_test.go +++ b/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()) diff --git a/docs/dwo-configuration.md b/docs/dwo-configuration.md index 8e31ce5e9..c7c5f8611 100644 --- a/docs/dwo-configuration.md +++ b/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: `/: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. diff --git a/main.go b/main.go index d6759513c..cb2a9f757 100644 --- a/main.go +++ b/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") diff --git a/pkg/secrets/backup.go b/pkg/secrets/backup.go index 1498a6a3e..2d4a149d1 100644 --- a/pkg/secrets/backup.go +++ b/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 } diff --git a/pkg/secrets/backup_test.go b/pkg/secrets/backup_test.go index e89133b18..546480d0c 100644 --- a/pkg/secrets/backup_test.go +++ b/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