From b574ad6068c7554a872a53debefc5e384c51b3c4 Mon Sep 17 00:00:00 2001 From: Rohan Kumar Date: Mon, 4 May 2026 14:48:55 +0530 Subject: [PATCH 1/3] feat (backup) : Add copyOperatorAuthSecret configuration flag 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 Signed-off-by: Rohan Kumar --- .../devworkspaceoperatorconfig_types.go | 19 +++++++++++++++--- .../v1alpha1/zz_generated.deepcopy.go | 7 ++++++- ...evfile.io_devworkspaceoperatorconfigs.yaml | 20 ++++++++++++++++--- deploy/deployment/kubernetes/combined.yaml | 20 ++++++++++++++++--- ...r.devfile.io.CustomResourceDefinition.yaml | 20 ++++++++++++++++--- deploy/deployment/openshift/combined.yaml | 20 ++++++++++++++++--- ...r.devfile.io.CustomResourceDefinition.yaml | 20 ++++++++++++++++--- ...evfile.io_devworkspaceoperatorconfigs.yaml | 20 ++++++++++++++++--- pkg/config/sync.go | 3 +++ 9 files changed, 127 insertions(+), 22 deletions(-) diff --git a/apis/controller/v1alpha1/devworkspaceoperatorconfig_types.go b/apis/controller/v1alpha1/devworkspaceoperatorconfig_types.go index 60a2df5d4..94f019175 100644 --- a/apis/controller/v1alpha1/devworkspaceoperatorconfig_types.go +++ b/apis/controller/v1alpha1/devworkspaceoperatorconfig_types.go @@ -80,13 +80,26 @@ type RegistryConfig struct { // AuthSecret is the name of a Kubernetes secret of // type kubernetes.io/dockerconfigjson. // The secret is expected to be in the same namespace the workspace is running in. - // If secret is not found in the workspace namespace, the operator will look for the secret - // in the namespace where the operator is running in. - // as the DevWorkspaceOperatorCongfig. + // If secret is not found in the workspace namespace and copyOperatorAuthSecret is true, + // the operator will copy the secret from the operator namespace to the workspace namespace. // The secret must contain "controller.devfile.io/watch-secret=true" label so that it can be // recognized by the operator. // +kubebuilder:validation:Optional AuthSecret string `json:"authSecret,omitempty"` + // 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. + // + // If true, this enables the fallback mechanism where the operator will copy the secret from the operator namespace. + // If false, the operator will not copy the secret to workspace namespaces. + // + // Note: Regardless of this setting, if a secret already exists in the workspace namespace, + // it will never be overwritten. + // + // Defaults to false. + // +kubebuilder:validation:Optional + // +kubebuilder:default=false + CopyOperatorAuthSecret *bool `json:"copyOperatorAuthSecret,omitempty"` } type OrasConfig struct { diff --git a/apis/controller/v1alpha1/zz_generated.deepcopy.go b/apis/controller/v1alpha1/zz_generated.deepcopy.go index f795f3d43..223c2b8ff 100644 --- a/apis/controller/v1alpha1/zz_generated.deepcopy.go +++ b/apis/controller/v1alpha1/zz_generated.deepcopy.go @@ -58,7 +58,7 @@ func (in *BackupCronJobConfig) DeepCopyInto(out *BackupCronJobConfig) { if in.Registry != nil { in, out := &in.Registry, &out.Registry *out = new(RegistryConfig) - **out = **in + (*in).DeepCopyInto(*out) } if in.OrasConfig != nil { in, out := &in.OrasConfig, &out.OrasConfig @@ -669,6 +669,11 @@ func (in *Proxy) DeepCopy() *Proxy { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *RegistryConfig) DeepCopyInto(out *RegistryConfig) { *out = *in + if in.CopyOperatorAuthSecret != nil { + in, out := &in.CopyOperatorAuthSecret, &out.CopyOperatorAuthSecret + *out = new(bool) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new RegistryConfig. diff --git a/deploy/bundle/manifests/controller.devfile.io_devworkspaceoperatorconfigs.yaml b/deploy/bundle/manifests/controller.devfile.io_devworkspaceoperatorconfigs.yaml index 79a1844f0..74ce069b0 100644 --- a/deploy/bundle/manifests/controller.devfile.io_devworkspaceoperatorconfigs.yaml +++ b/deploy/bundle/manifests/controller.devfile.io_devworkspaceoperatorconfigs.yaml @@ -214,12 +214,26 @@ spec: AuthSecret is the name of a Kubernetes secret of type kubernetes.io/dockerconfigjson. The secret is expected to be in the same namespace the workspace is running in. - If secret is not found in the workspace namespace, the operator will look for the secret - in the namespace where the operator is running in. - as the DevWorkspaceOperatorCongfig. + If secret is not found in the workspace namespace and copyOperatorAuthSecret is true, + the operator will copy the secret from the operator namespace to the workspace namespace. The secret must contain "controller.devfile.io/watch-secret=true" label so that it can be recognized by the operator. type: string + 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. + + If true, this enables the fallback mechanism where the operator will copy the secret from the operator namespace. + If false, the operator will not copy the secret to workspace namespaces. + + Note: Regardless of this setting, if a secret already exists in the workspace namespace, + it will never be overwritten. + + Defaults to false. + type: boolean path: description: |- A registry where backup images are stored. Images are stored diff --git a/deploy/deployment/kubernetes/combined.yaml b/deploy/deployment/kubernetes/combined.yaml index e19401db5..f66de5401 100644 --- a/deploy/deployment/kubernetes/combined.yaml +++ b/deploy/deployment/kubernetes/combined.yaml @@ -218,12 +218,26 @@ spec: AuthSecret is the name of a Kubernetes secret of type kubernetes.io/dockerconfigjson. The secret is expected to be in the same namespace the workspace is running in. - If secret is not found in the workspace namespace, the operator will look for the secret - in the namespace where the operator is running in. - as the DevWorkspaceOperatorCongfig. + If secret is not found in the workspace namespace and copyOperatorAuthSecret is true, + the operator will copy the secret from the operator namespace to the workspace namespace. The secret must contain "controller.devfile.io/watch-secret=true" label so that it can be recognized by the operator. type: string + 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. + + If true, this enables the fallback mechanism where the operator will copy the secret from the operator namespace. + If false, the operator will not copy the secret to workspace namespaces. + + Note: Regardless of this setting, if a secret already exists in the workspace namespace, + it will never be overwritten. + + Defaults to false. + type: boolean path: description: |- A registry where backup images are stored. Images are stored diff --git a/deploy/deployment/kubernetes/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yaml b/deploy/deployment/kubernetes/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yaml index 3319d3bdf..91e7e6dc6 100644 --- a/deploy/deployment/kubernetes/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yaml +++ b/deploy/deployment/kubernetes/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yaml @@ -218,12 +218,26 @@ spec: AuthSecret is the name of a Kubernetes secret of type kubernetes.io/dockerconfigjson. The secret is expected to be in the same namespace the workspace is running in. - If secret is not found in the workspace namespace, the operator will look for the secret - in the namespace where the operator is running in. - as the DevWorkspaceOperatorCongfig. + If secret is not found in the workspace namespace and copyOperatorAuthSecret is true, + the operator will copy the secret from the operator namespace to the workspace namespace. The secret must contain "controller.devfile.io/watch-secret=true" label so that it can be recognized by the operator. type: string + 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. + + If true, this enables the fallback mechanism where the operator will copy the secret from the operator namespace. + If false, the operator will not copy the secret to workspace namespaces. + + Note: Regardless of this setting, if a secret already exists in the workspace namespace, + it will never be overwritten. + + Defaults to false. + type: boolean path: description: |- A registry where backup images are stored. Images are stored diff --git a/deploy/deployment/openshift/combined.yaml b/deploy/deployment/openshift/combined.yaml index 448227d7f..a14a3d203 100644 --- a/deploy/deployment/openshift/combined.yaml +++ b/deploy/deployment/openshift/combined.yaml @@ -218,12 +218,26 @@ spec: AuthSecret is the name of a Kubernetes secret of type kubernetes.io/dockerconfigjson. The secret is expected to be in the same namespace the workspace is running in. - If secret is not found in the workspace namespace, the operator will look for the secret - in the namespace where the operator is running in. - as the DevWorkspaceOperatorCongfig. + If secret is not found in the workspace namespace and copyOperatorAuthSecret is true, + the operator will copy the secret from the operator namespace to the workspace namespace. The secret must contain "controller.devfile.io/watch-secret=true" label so that it can be recognized by the operator. type: string + 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. + + If true, this enables the fallback mechanism where the operator will copy the secret from the operator namespace. + If false, the operator will not copy the secret to workspace namespaces. + + Note: Regardless of this setting, if a secret already exists in the workspace namespace, + it will never be overwritten. + + Defaults to false. + type: boolean path: description: |- A registry where backup images are stored. Images are stored diff --git a/deploy/deployment/openshift/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yaml b/deploy/deployment/openshift/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yaml index 3319d3bdf..91e7e6dc6 100644 --- a/deploy/deployment/openshift/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yaml +++ b/deploy/deployment/openshift/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yaml @@ -218,12 +218,26 @@ spec: AuthSecret is the name of a Kubernetes secret of type kubernetes.io/dockerconfigjson. The secret is expected to be in the same namespace the workspace is running in. - If secret is not found in the workspace namespace, the operator will look for the secret - in the namespace where the operator is running in. - as the DevWorkspaceOperatorCongfig. + If secret is not found in the workspace namespace and copyOperatorAuthSecret is true, + the operator will copy the secret from the operator namespace to the workspace namespace. The secret must contain "controller.devfile.io/watch-secret=true" label so that it can be recognized by the operator. type: string + 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. + + If true, this enables the fallback mechanism where the operator will copy the secret from the operator namespace. + If false, the operator will not copy the secret to workspace namespaces. + + Note: Regardless of this setting, if a secret already exists in the workspace namespace, + it will never be overwritten. + + Defaults to false. + type: boolean path: description: |- A registry where backup images are stored. Images are stored diff --git a/deploy/templates/crd/bases/controller.devfile.io_devworkspaceoperatorconfigs.yaml b/deploy/templates/crd/bases/controller.devfile.io_devworkspaceoperatorconfigs.yaml index 78b1a9275..c52f5a961 100644 --- a/deploy/templates/crd/bases/controller.devfile.io_devworkspaceoperatorconfigs.yaml +++ b/deploy/templates/crd/bases/controller.devfile.io_devworkspaceoperatorconfigs.yaml @@ -216,12 +216,26 @@ spec: AuthSecret is the name of a Kubernetes secret of type kubernetes.io/dockerconfigjson. The secret is expected to be in the same namespace the workspace is running in. - If secret is not found in the workspace namespace, the operator will look for the secret - in the namespace where the operator is running in. - as the DevWorkspaceOperatorCongfig. + If secret is not found in the workspace namespace and copyOperatorAuthSecret is true, + the operator will copy the secret from the operator namespace to the workspace namespace. The secret must contain "controller.devfile.io/watch-secret=true" label so that it can be recognized by the operator. type: string + 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. + + If true, this enables the fallback mechanism where the operator will copy the secret from the operator namespace. + If false, the operator will not copy the secret to workspace namespaces. + + Note: Regardless of this setting, if a secret already exists in the workspace namespace, + it will never be overwritten. + + Defaults to false. + type: boolean path: description: |- A registry where backup images are stored. Images are stored diff --git a/pkg/config/sync.go b/pkg/config/sync.go index bf474e986..84ca5f6dc 100644 --- a/pkg/config/sync.go +++ b/pkg/config/sync.go @@ -477,6 +477,9 @@ func mergeConfig(from, to *controller.OperatorConfiguration) { if from.Workspace.BackupCronJob.Registry.AuthSecret != "" { to.Workspace.BackupCronJob.Registry.AuthSecret = from.Workspace.BackupCronJob.Registry.AuthSecret } + if from.Workspace.BackupCronJob.Registry.CopyOperatorAuthSecret != nil { + to.Workspace.BackupCronJob.Registry.CopyOperatorAuthSecret = from.Workspace.BackupCronJob.Registry.CopyOperatorAuthSecret + } } if from.Workspace.BackupCronJob.OrasConfig != nil { if to.Workspace.BackupCronJob.OrasConfig == nil { From 43f48628144d37808d6e1f2ea7f2c7239fbedc9a Mon Sep 17 00:00:00 2001 From: Rohan Kumar Date: Tue, 5 May 2026 22:16:18 +0530 Subject: [PATCH 2/3] feat (backup) : Implement copyOperatorAuthSecret flag with tests 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 Signed-off-by: Rohan Kumar --- .../backupcronjob_controller_test.go | 48 +-- pkg/secrets/backup.go | 93 ++++-- pkg/secrets/backup_test.go | 280 ++++++++++++++++++ 3 files changed, 377 insertions(+), 44 deletions(-) diff --git a/controllers/backupcronjob/backupcronjob_controller_test.go b/controllers/backupcronjob/backupcronjob_controller_test.go index 134bdb17e..a2b0a8323 100644 --- a/controllers/backupcronjob/backupcronjob_controller_test.go +++ b/controllers/backupcronjob/backupcronjob_controller_test.go @@ -121,7 +121,8 @@ var _ = Describe("BackupCronJobReconciler", func() { Enable: pointer.Bool(true), Schedule: "* * * * *", Registry: &controllerv1alpha1.RegistryConfig{ - Path: "fake-registry", + Path: "fake-registry", + CopyOperatorAuthSecret: pointer.Bool(true), }, }, }, @@ -148,7 +149,8 @@ var _ = Describe("BackupCronJobReconciler", func() { Enable: &enabled, Schedule: schedule, Registry: &controllerv1alpha1.RegistryConfig{ - Path: "fake-registry", + Path: "fake-registry", + CopyOperatorAuthSecret: pointer.Bool(true), }, }, }, @@ -173,7 +175,8 @@ var _ = Describe("BackupCronJobReconciler", func() { Enable: &enabled, Schedule: schedule, Registry: &controllerv1alpha1.RegistryConfig{ - Path: "fake-registry", + Path: "fake-registry", + CopyOperatorAuthSecret: pointer.Bool(true), }, }, }, @@ -206,7 +209,8 @@ var _ = Describe("BackupCronJobReconciler", func() { Enable: &enabled, Schedule: schedule1, Registry: &controllerv1alpha1.RegistryConfig{ - Path: "fake-registry", + Path: "fake-registry", + CopyOperatorAuthSecret: pointer.Bool(true), }, }, }, @@ -242,7 +246,8 @@ var _ = Describe("BackupCronJobReconciler", func() { Enable: &enabled, Schedule: schedule1, Registry: &controllerv1alpha1.RegistryConfig{ - Path: "fake-registry", + Path: "fake-registry", + CopyOperatorAuthSecret: pointer.Bool(true), }, }, }, @@ -268,7 +273,8 @@ var _ = Describe("BackupCronJobReconciler", func() { Enable: &enabled, Schedule: schedule, Registry: &controllerv1alpha1.RegistryConfig{ - Path: "fake-registry", + Path: "fake-registry", + CopyOperatorAuthSecret: pointer.Bool(true), }, }, }, @@ -303,8 +309,9 @@ var _ = Describe("BackupCronJobReconciler", func() { Enable: &enabled, Schedule: schedule, Registry: &controllerv1alpha1.RegistryConfig{ - Path: "fake-registry", - AuthSecret: "non-existent", + Path: "fake-registry", + AuthSecret: "non-existent", + CopyOperatorAuthSecret: pointer.Bool(true), }, }, }, @@ -335,7 +342,8 @@ var _ = Describe("BackupCronJobReconciler", func() { Enable: &enabled, Schedule: schedule, Registry: &controllerv1alpha1.RegistryConfig{ - Path: "fake-registry", + Path: "fake-registry", + CopyOperatorAuthSecret: pointer.Bool(true), }, OrasConfig: &controllerv1alpha1.OrasConfig{ ExtraArgs: "--extra-arg1", @@ -392,7 +400,8 @@ var _ = Describe("BackupCronJobReconciler", func() { Schedule: schedule, BackoffLimit: &backoffLimit, Registry: &controllerv1alpha1.RegistryConfig{ - Path: "fake-registry", + Path: "fake-registry", + CopyOperatorAuthSecret: pointer.Bool(true), }, }, }, @@ -428,7 +437,8 @@ var _ = Describe("BackupCronJobReconciler", func() { Enable: &enabled, Schedule: schedule, Registry: &controllerv1alpha1.RegistryConfig{ - Path: "fake-registry", + Path: "fake-registry", + CopyOperatorAuthSecret: pointer.Bool(true), }, }, }, @@ -469,7 +479,8 @@ var _ = Describe("BackupCronJobReconciler", func() { Enable: &enabled, Schedule: schedule, Registry: &controllerv1alpha1.RegistryConfig{ - Path: "fake-registry", + Path: "fake-registry", + CopyOperatorAuthSecret: pointer.Bool(true), }, }, }, @@ -500,8 +511,9 @@ var _ = Describe("BackupCronJobReconciler", func() { Enable: &enabled, Schedule: schedule, Registry: &controllerv1alpha1.RegistryConfig{ - Path: "my-registry:5000", - AuthSecret: "my-secret", + Path: "my-registry:5000", + AuthSecret: "my-secret", + CopyOperatorAuthSecret: pointer.Bool(true), }, }, }, @@ -536,8 +548,9 @@ var _ = Describe("BackupCronJobReconciler", func() { Enable: &enabled, Schedule: schedule, Registry: &controllerv1alpha1.RegistryConfig{ - Path: "my-registry:5000", - AuthSecret: "my-secret", + Path: "my-registry:5000", + AuthSecret: "my-secret", + CopyOperatorAuthSecret: pointer.Bool(true), }, }, }, @@ -572,7 +585,8 @@ var _ = Describe("BackupCronJobReconciler", func() { Enable: &enabled, Schedule: schedule, Registry: &controllerv1alpha1.RegistryConfig{ - Path: "fake-registry", + Path: "fake-registry", + CopyOperatorAuthSecret: pointer.Bool(true), }, OrasConfig: &controllerv1alpha1.OrasConfig{ ExtraArgs: "--extra-arg1", diff --git a/pkg/secrets/backup.go b/pkg/secrets/backup.go index 1498a6a3e..d710ddcb7 100644 --- a/pkg/secrets/backup.go +++ b/pkg/secrets/backup.go @@ -24,12 +24,12 @@ 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" + "k8s.io/utils/pointer" "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 @@ -54,8 +54,14 @@ func HandleRegistryAuthSecret(ctx context.Context, c client.Client, workspace *d return nil, nil } + // Extract flag value (default: false, users must provide their own secret) + copyOperatorAuthSecret := pointer.BoolDeref( + dwOperatorConfig.Workspace.BackupCronJob.Registry.CopyOperatorAuthSecret, + false, + ) + // On the restore path (operatorConfigNamespace == ""), look for the predefined name - // that CopySecret always uses. On the backup path, look for the configured name + // that EnsureRegistryAuthSecret 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 == "" { @@ -88,13 +94,46 @@ func HandleRegistryAuthSecret(ctx context.Context, c client.Client, workspace *d log.Error(err, "Failed to get registry auth secret for backup job", "secretName", secretName) return nil, err } - log.Info("Successfully retrieved registry auth secret for backup job", "secretName", secretName) - return CopySecret(ctx, c, workspace, registryAuthSecret, scheme, log) + log.Info("Successfully retrieved registry auth secret from operator namespace", "secretName", secretName) + return EnsureRegistryAuthSecret(ctx, c, workspace, registryAuthSecret, scheme, log, copyOperatorAuthSecret) } -// CopySecret copies the given secret from the operator namespace to the workspace namespace. -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 +// EnsureRegistryAuthSecret copies the given secret from the operator namespace to the workspace namespace. +// If copyOperatorAuthSecret is false and the secret doesn't exist, it returns an error. +// If a secret already exists in the workspace namespace, it is never overwritten. +func EnsureRegistryAuthSecret(ctx context.Context, c client.Client, workspace *dw.DevWorkspace, sourceSecret *corev1.Secret, scheme *runtime.Scheme, log logr.Logger, copyOperatorAuthSecret bool) (namespaceSecret *corev1.Secret, err error) { + // Check if secret already exists (user-provided) + existingSecret := &corev1.Secret{} + err = c.Get(ctx, client.ObjectKey{ + Name: constants.DevWorkspaceBackupAuthSecretName, + Namespace: workspace.Namespace, + }, existingSecret) + + if err == nil { + // User provided their own secret - always respect it, never overwrite + log.Info("Using existing registry auth secret from workspace namespace", + "secretName", constants.DevWorkspaceBackupAuthSecretName) + return existingSecret, nil + } + + if client.IgnoreNotFound(err) != nil { + return nil, err + } + + // Secret doesn't exist - check if we should copy + if !copyOperatorAuthSecret { + return nil, fmt.Errorf( + "registry auth secret %q not found in workspace namespace %q. "+ + "copyOperatorAuthSecret is set to false, the secret will not be copied."+ + "Please create a secret of type kubernetes.io/dockerconfigjson with the name %q "+ + "in the workspace namespace", + constants.DevWorkspaceBackupAuthSecretName, + workspace.Namespace, + constants.DevWorkspaceBackupAuthSecretName, + ) + } + + // copyOperatorAuthSecret is true - create it from operator's secret desiredSecret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: constants.DevWorkspaceBackupAuthSecretName, @@ -111,27 +150,27 @@ 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) + // Use direct Create instead of SyncObjectWithCluster to avoid overwrites + 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("Created registry auth secret from operator credentials", + "secretName", constants.DevWorkspaceBackupAuthSecretName, + "namespace", workspace.Namespace) + return desiredSecret, nil } diff --git a/pkg/secrets/backup_test.go b/pkg/secrets/backup_test.go index e89133b18..5c29db10b 100644 --- a/pkg/secrets/backup_test.go +++ b/pkg/secrets/backup_test.go @@ -75,6 +75,21 @@ func makeConfig(authSecretName string) *controllerv1alpha1.OperatorConfiguration } } +// makeConfigWithCopyFlag returns an OperatorConfiguration with copyOperatorAuthSecret flag set. +func makeConfigWithCopyFlag(authSecretName string, copyFlag *bool) *controllerv1alpha1.OperatorConfiguration { + return &controllerv1alpha1.OperatorConfiguration{ + Workspace: &controllerv1alpha1.WorkspaceConfig{ + BackupCronJob: &controllerv1alpha1.BackupCronJobConfig{ + Registry: &controllerv1alpha1.RegistryConfig{ + Path: "example.registry.io/org", + AuthSecret: authSecretName, + CopyOperatorAuthSecret: copyFlag, + }, + }, + }, + } +} + // makeSecret returns a corev1.Secret with the given name and namespace. func makeSecret(name, namespace string) *corev1.Secret { return &corev1.Secret{ @@ -162,3 +177,268 @@ func (e *errorOnNameClient) Get(ctx context.Context, key client.ObjectKey, obj c // Ensure errorOnNameClient satisfies client.Client at compile time. var _ client.Client = &errorOnNameClient{} + +var _ = Describe("HandleRegistryAuthSecret (backup path: operatorConfigNamespace set)", func() { + const ( + workspaceNS = "user-namespace" + operatorNS = "devworkspace-controller" + secretName = "registry-auth-secret" + ) + + var ( + ctx context.Context + scheme *runtime.Scheme + log = zap.New(zap.UseDevMode(true)).WithName("SecretsTest") + ) + + BeforeEach(func() { + ctx = context.Background() + scheme = buildScheme() + }) + + Context("when copyOperatorAuthSecret is nil (default: false)", func() { + It("returns error when secret not found in workspace namespace", func() { + By("creating operator secret but no workspace secret") + operatorSecret := makeSecret(secretName, operatorNS) + fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(operatorSecret).Build() + + workspace := makeWorkspace(workspaceNS) + config := makeConfig(secretName) // copyOperatorAuthSecret is nil (default) + + result, err := secrets.HandleRegistryAuthSecret(ctx, fakeClient, workspace, config, operatorNS, scheme, log) + Expect(err).To(HaveOccurred()) + Expect(result).To(BeNil()) + + By("verifying error message is clear and actionable") + Expect(err.Error()).To(ContainSubstring("copyOperatorAuthSecret is set to false")) + Expect(err.Error()).To(ContainSubstring("not found in workspace namespace")) + Expect(err.Error()).To(ContainSubstring("Please manually create")) + Expect(err.Error()).To(ContainSubstring("kubernetes.io/dockerconfigjson")) + }) + + It("returns existing workspace secret without overwriting", func() { + By("creating both operator secret and user-provided workspace secret with different data") + operatorSecret := makeSecret(secretName, operatorNS) + operatorSecret.Data = map[string][]byte{"auth": []byte("operator-creds")} + + userSecret := makeSecret(constants.DevWorkspaceBackupAuthSecretName, workspaceNS) + userSecret.Data = map[string][]byte{"auth": []byte("user-creds")} + + fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(operatorSecret, userSecret).Build() + + workspace := makeWorkspace(workspaceNS) + config := makeConfig(secretName) + + 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)) + + By("verifying user's secret data is unchanged") + Expect(result.Data["auth"]).To(Equal([]byte("user-creds"))) + + By("verifying the secret in the cluster still has user's data") + fetchedSecret := &corev1.Secret{} + err = fakeClient.Get(ctx, client.ObjectKey{ + Name: constants.DevWorkspaceBackupAuthSecretName, + Namespace: workspaceNS, + }, fetchedSecret) + Expect(err).NotTo(HaveOccurred()) + Expect(fetchedSecret.Data["auth"]).To(Equal([]byte("user-creds"))) + }) + }) + + Context("when copyOperatorAuthSecret is true", func() { + It("creates secret from operator namespace when not in workspace namespace", func() { + By("creating operator secret but no workspace secret") + operatorSecret := makeSecret(secretName, operatorNS) + fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(operatorSecret).Build() + + workspace := makeWorkspace(workspaceNS) + trueVal := true + config := makeConfigWithCopyFlag(secretName, &trueVal) + + 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)) + + By("verifying the secret was created in workspace namespace") + createdSecret := &corev1.Secret{} + err = fakeClient.Get(ctx, client.ObjectKey{ + Name: constants.DevWorkspaceBackupAuthSecretName, + Namespace: workspaceNS, + }, createdSecret) + Expect(err).NotTo(HaveOccurred()) + Expect(createdSecret.Data).To(Equal(operatorSecret.Data)) + }) + + It("returns existing workspace secret without overwriting", func() { + By("creating both operator secret and user-provided workspace secret with different data") + operatorSecret := makeSecret(secretName, operatorNS) + operatorSecret.Data = map[string][]byte{"auth": []byte("operator-creds")} + + userSecret := makeSecret(constants.DevWorkspaceBackupAuthSecretName, workspaceNS) + userSecret.Data = map[string][]byte{"auth": []byte("user-creds")} + + fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(operatorSecret, userSecret).Build() + + workspace := makeWorkspace(workspaceNS) + trueVal := true + config := makeConfigWithCopyFlag(secretName, &trueVal) + + result, err := secrets.HandleRegistryAuthSecret(ctx, fakeClient, workspace, config, operatorNS, scheme, log) + Expect(err).NotTo(HaveOccurred()) + Expect(result).NotTo(BeNil()) + + By("verifying user's secret data is unchanged") + Expect(result.Data["auth"]).To(Equal([]byte("user-creds"))) + }) + }) + + Context("when copyOperatorAuthSecret is false", func() { + It("returns error when secret not found in workspace namespace", func() { + By("creating operator secret but no workspace secret") + operatorSecret := makeSecret(secretName, operatorNS) + fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(operatorSecret).Build() + + workspace := makeWorkspace(workspaceNS) + falseVal := false + config := makeConfigWithCopyFlag(secretName, &falseVal) + + result, err := secrets.HandleRegistryAuthSecret(ctx, fakeClient, workspace, config, operatorNS, scheme, log) + Expect(err).To(HaveOccurred()) + Expect(result).To(BeNil()) + + By("verifying error message is clear and actionable") + Expect(err.Error()).To(ContainSubstring("copyOperatorAuthSecret is set to false")) + Expect(err.Error()).To(ContainSubstring("not found in workspace namespace")) + Expect(err.Error()).To(ContainSubstring("Please manually create")) + Expect(err.Error()).To(ContainSubstring("kubernetes.io/dockerconfigjson")) + }) + + It("returns existing workspace secret when present", func() { + By("creating both operator secret and user-provided workspace secret") + operatorSecret := makeSecret(secretName, operatorNS) + operatorSecret.Data = map[string][]byte{"auth": []byte("operator-creds")} + + userSecret := makeSecret(constants.DevWorkspaceBackupAuthSecretName, workspaceNS) + userSecret.Data = map[string][]byte{"auth": []byte("user-creds")} + + fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(operatorSecret, userSecret).Build() + + workspace := makeWorkspace(workspaceNS) + falseVal := false + config := makeConfigWithCopyFlag(secretName, &falseVal) + + result, err := secrets.HandleRegistryAuthSecret(ctx, fakeClient, workspace, config, operatorNS, scheme, log) + Expect(err).NotTo(HaveOccurred()) + Expect(result).NotTo(BeNil()) + + By("verifying user's secret is returned") + Expect(result.Data["auth"]).To(Equal([]byte("user-creds"))) + }) + + It("does not create secret even if operator secret exists", func() { + By("creating operator secret but no workspace secret") + operatorSecret := makeSecret(secretName, operatorNS) + fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(operatorSecret).Build() + + workspace := makeWorkspace(workspaceNS) + falseVal := false + config := makeConfigWithCopyFlag(secretName, &falseVal) + + _, err := secrets.HandleRegistryAuthSecret(ctx, fakeClient, workspace, config, operatorNS, scheme, log) + Expect(err).To(HaveOccurred()) + + By("verifying no secret was created in workspace namespace") + createdSecret := &corev1.Secret{} + err = fakeClient.Get(ctx, client.ObjectKey{ + Name: constants.DevWorkspaceBackupAuthSecretName, + Namespace: workspaceNS, + }, createdSecret) + Expect(k8sErrors.IsNotFound(err)).To(BeTrue()) + }) + }) + + Context("secret creation edge cases", func() { + It("sets proper labels on created secret", func() { + By("creating operator secret") + operatorSecret := makeSecret(secretName, operatorNS) + fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(operatorSecret).Build() + + workspace := makeWorkspace(workspaceNS) + trueVal := true + config := makeConfigWithCopyFlag(secretName, &trueVal) + + _, err := secrets.HandleRegistryAuthSecret(ctx, fakeClient, workspace, config, operatorNS, scheme, log) + Expect(err).NotTo(HaveOccurred()) + + By("verifying the created secret has the watch label") + createdSecret := &corev1.Secret{} + err = fakeClient.Get(ctx, client.ObjectKey{ + Name: constants.DevWorkspaceBackupAuthSecretName, + Namespace: workspaceNS, + }, createdSecret) + Expect(err).NotTo(HaveOccurred()) + Expect(createdSecret.Labels).To(HaveKey(constants.DevWorkspaceWatchSecretLabel)) + Expect(createdSecret.Labels[constants.DevWorkspaceWatchSecretLabel]).To(Equal("true")) + }) + + It("sets controller reference on created secret", func() { + By("creating operator secret") + operatorSecret := makeSecret(secretName, operatorNS) + fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(operatorSecret).Build() + + workspace := makeWorkspace(workspaceNS) + trueVal := true + config := makeConfigWithCopyFlag(secretName, &trueVal) + + _, err := secrets.HandleRegistryAuthSecret(ctx, fakeClient, workspace, config, operatorNS, scheme, log) + Expect(err).NotTo(HaveOccurred()) + + By("verifying the created secret has owner reference to workspace") + createdSecret := &corev1.Secret{} + err = fakeClient.Get(ctx, client.ObjectKey{ + Name: constants.DevWorkspaceBackupAuthSecretName, + Namespace: workspaceNS, + }, createdSecret) + Expect(err).NotTo(HaveOccurred()) + Expect(createdSecret.OwnerReferences).To(HaveLen(1)) + Expect(createdSecret.OwnerReferences[0].Name).To(Equal(workspace.Name)) + Expect(createdSecret.OwnerReferences[0].Kind).To(Equal("DevWorkspace")) + }) + }) + + Context("when secret exists in workspace namespace with configured name", func() { + It("returns the existing secret without copying from operator namespace", func() { + By("creating a secret in workspace namespace with the configured name") + workspaceSecret := makeSecret(secretName, workspaceNS) + workspaceSecret.Data = map[string][]byte{"auth": []byte("workspace-specific-creds")} + + operatorSecret := makeSecret(secretName, operatorNS) + operatorSecret.Data = map[string][]byte{"auth": []byte("operator-creds")} + + fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(workspaceSecret, operatorSecret).Build() + + workspace := makeWorkspace(workspaceNS) + config := makeConfig(secretName) + + result, err := secrets.HandleRegistryAuthSecret(ctx, fakeClient, workspace, config, operatorNS, scheme, log) + Expect(err).NotTo(HaveOccurred()) + Expect(result).NotTo(BeNil()) + Expect(result.Name).To(Equal(secretName)) + Expect(result.Namespace).To(Equal(workspaceNS)) + Expect(result.Data["auth"]).To(Equal([]byte("workspace-specific-creds"))) + + By("verifying no secret with predefined name was created") + predefinedSecret := &corev1.Secret{} + err = fakeClient.Get(ctx, client.ObjectKey{ + Name: constants.DevWorkspaceBackupAuthSecretName, + Namespace: workspaceNS, + }, predefinedSecret) + Expect(k8sErrors.IsNotFound(err)).To(BeTrue()) + }) + }) +}) From 385a1b3cd32437c451fd2caf0f764e1e4ca31ac9 Mon Sep 17 00:00:00 2001 From: Rohan Kumar Date: Thu, 7 May 2026 10:31:15 +0530 Subject: [PATCH 3/3] test: Update error message expectation in backup_test.go Changed test expectation from "Please manually create" to "Please create" to match the updated error message in the source code. Co-Authored-By: Claude Sonnet 4.5 Signed-off-by: Rohan Kumar --- pkg/secrets/backup_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/secrets/backup_test.go b/pkg/secrets/backup_test.go index 5c29db10b..41a53116e 100644 --- a/pkg/secrets/backup_test.go +++ b/pkg/secrets/backup_test.go @@ -212,7 +212,7 @@ var _ = Describe("HandleRegistryAuthSecret (backup path: operatorConfigNamespace By("verifying error message is clear and actionable") Expect(err.Error()).To(ContainSubstring("copyOperatorAuthSecret is set to false")) Expect(err.Error()).To(ContainSubstring("not found in workspace namespace")) - Expect(err.Error()).To(ContainSubstring("Please manually create")) + Expect(err.Error()).To(ContainSubstring("Please create")) Expect(err.Error()).To(ContainSubstring("kubernetes.io/dockerconfigjson")) }) @@ -314,7 +314,7 @@ var _ = Describe("HandleRegistryAuthSecret (backup path: operatorConfigNamespace By("verifying error message is clear and actionable") Expect(err.Error()).To(ContainSubstring("copyOperatorAuthSecret is set to false")) Expect(err.Error()).To(ContainSubstring("not found in workspace namespace")) - Expect(err.Error()).To(ContainSubstring("Please manually create")) + Expect(err.Error()).To(ContainSubstring("Please create")) Expect(err.Error()).To(ContainSubstring("kubernetes.io/dockerconfigjson")) })