From 277921820aa53cbf878c6d8c44c156407dfc0944 Mon Sep 17 00:00:00 2001 From: Rohan Kumar Date: Mon, 4 May 2026 14:48:55 +0530 Subject: [PATCH 1/2] 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 | 24 +++++++++++++++--- .../v1alpha1/zz_generated.deepcopy.go | 7 +++++- ...evfile.io_devworkspaceoperatorconfigs.yaml | 25 ++++++++++++++++--- deploy/deployment/kubernetes/combined.yaml | 25 ++++++++++++++++--- ...r.devfile.io.CustomResourceDefinition.yaml | 25 ++++++++++++++++--- deploy/deployment/openshift/combined.yaml | 25 ++++++++++++++++--- ...r.devfile.io.CustomResourceDefinition.yaml | 25 ++++++++++++++++--- ...evfile.io_devworkspaceoperatorconfigs.yaml | 25 ++++++++++++++++--- pkg/config/sync.go | 3 +++ 9 files changed, 162 insertions(+), 22 deletions(-) diff --git a/apis/controller/v1alpha1/devworkspaceoperatorconfig_types.go b/apis/controller/v1alpha1/devworkspaceoperatorconfig_types.go index 60a2df5d4..8283440ff 100644 --- a/apis/controller/v1alpha1/devworkspaceoperatorconfig_types.go +++ b/apis/controller/v1alpha1/devworkspaceoperatorconfig_types.go @@ -80,13 +80,31 @@ 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: The operator will copy the secret from the operator namespace + // if it's not found in the workspace namespace. This provides automatic configuration + // but exposes operator-level credentials to workspace users. + // + // If false (default): The operator will not copy the secret. Users must manually create a secret + // with the configured name in their workspace namespace. This is more secure as it allows + // users to provide scoped credentials with minimal privileges. + // + // Note: Regardless of this setting, if a secret already exists in the workspace namespace, + // it will never be overwritten. User-provided secrets are always respected. + // + // 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..7af1aff11 100644 --- a/deploy/bundle/manifests/controller.devfile.io_devworkspaceoperatorconfigs.yaml +++ b/deploy/bundle/manifests/controller.devfile.io_devworkspaceoperatorconfigs.yaml @@ -214,12 +214,31 @@ 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: The operator will copy the secret from the operator namespace + if it's not found in the workspace namespace. This provides automatic configuration + but exposes operator-level credentials to workspace users. + + If false (default): The operator will not copy the secret. Users must manually create a secret + with the configured name in their workspace namespace. This is more secure as it allows + users to provide scoped credentials with minimal privileges. + + Note: Regardless of this setting, if a secret already exists in the workspace namespace, + it will never be overwritten. User-provided secrets are always respected. + + 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..86fccc025 100644 --- a/deploy/deployment/kubernetes/combined.yaml +++ b/deploy/deployment/kubernetes/combined.yaml @@ -218,12 +218,31 @@ 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: The operator will copy the secret from the operator namespace + if it's not found in the workspace namespace. This provides automatic configuration + but exposes operator-level credentials to workspace users. + + If false (default): The operator will not copy the secret. Users must manually create a secret + with the configured name in their workspace namespace. This is more secure as it allows + users to provide scoped credentials with minimal privileges. + + Note: Regardless of this setting, if a secret already exists in the workspace namespace, + it will never be overwritten. User-provided secrets are always respected. + + 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..c3973e2b4 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,31 @@ 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: The operator will copy the secret from the operator namespace + if it's not found in the workspace namespace. This provides automatic configuration + but exposes operator-level credentials to workspace users. + + If false (default): The operator will not copy the secret. Users must manually create a secret + with the configured name in their workspace namespace. This is more secure as it allows + users to provide scoped credentials with minimal privileges. + + Note: Regardless of this setting, if a secret already exists in the workspace namespace, + it will never be overwritten. User-provided secrets are always respected. + + 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..0e0437d93 100644 --- a/deploy/deployment/openshift/combined.yaml +++ b/deploy/deployment/openshift/combined.yaml @@ -218,12 +218,31 @@ 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: The operator will copy the secret from the operator namespace + if it's not found in the workspace namespace. This provides automatic configuration + but exposes operator-level credentials to workspace users. + + If false (default): The operator will not copy the secret. Users must manually create a secret + with the configured name in their workspace namespace. This is more secure as it allows + users to provide scoped credentials with minimal privileges. + + Note: Regardless of this setting, if a secret already exists in the workspace namespace, + it will never be overwritten. User-provided secrets are always respected. + + 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..c3973e2b4 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,31 @@ 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: The operator will copy the secret from the operator namespace + if it's not found in the workspace namespace. This provides automatic configuration + but exposes operator-level credentials to workspace users. + + If false (default): The operator will not copy the secret. Users must manually create a secret + with the configured name in their workspace namespace. This is more secure as it allows + users to provide scoped credentials with minimal privileges. + + Note: Regardless of this setting, if a secret already exists in the workspace namespace, + it will never be overwritten. User-provided secrets are always respected. + + 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..b6fa3f3b4 100644 --- a/deploy/templates/crd/bases/controller.devfile.io_devworkspaceoperatorconfigs.yaml +++ b/deploy/templates/crd/bases/controller.devfile.io_devworkspaceoperatorconfigs.yaml @@ -216,12 +216,31 @@ 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: The operator will copy the secret from the operator namespace + if it's not found in the workspace namespace. This provides automatic configuration + but exposes operator-level credentials to workspace users. + + If false (default): The operator will not copy the secret. Users must manually create a secret + with the configured name in their workspace namespace. This is more secure as it allows + users to provide scoped credentials with minimal privileges. + + Note: Regardless of this setting, if a secret already exists in the workspace namespace, + it will never be overwritten. User-provided secrets are always respected. + + 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 0f8dddae20386d95625a2c90e4c97c62019d783b Mon Sep 17 00:00:00 2001 From: Rohan Kumar Date: Tue, 5 May 2026 22:16:18 +0530 Subject: [PATCH 2/2] 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..e597faca8 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, so the operator will not copy the secret. "+ + "Please manually 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()) + }) + }) +})