Skip to content

Commit 87f720c

Browse files
rohanKanojiaclaude
andcommitted
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 <noreply@anthropic.com> Signed-off-by: Rohan Kumar <rohaan@redhat.com>
1 parent add5c8b commit 87f720c

3 files changed

Lines changed: 377 additions & 44 deletions

File tree

controllers/backupcronjob/backupcronjob_controller_test.go

Lines changed: 31 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,8 @@ var _ = Describe("BackupCronJobReconciler", func() {
121121
Enable: pointer.Bool(true),
122122
Schedule: "* * * * *",
123123
Registry: &controllerv1alpha1.RegistryConfig{
124-
Path: "fake-registry",
124+
Path: "fake-registry",
125+
CopyOperatorAuthSecret: pointer.Bool(true),
125126
},
126127
},
127128
},
@@ -148,7 +149,8 @@ var _ = Describe("BackupCronJobReconciler", func() {
148149
Enable: &enabled,
149150
Schedule: schedule,
150151
Registry: &controllerv1alpha1.RegistryConfig{
151-
Path: "fake-registry",
152+
Path: "fake-registry",
153+
CopyOperatorAuthSecret: pointer.Bool(true),
152154
},
153155
},
154156
},
@@ -173,7 +175,8 @@ var _ = Describe("BackupCronJobReconciler", func() {
173175
Enable: &enabled,
174176
Schedule: schedule,
175177
Registry: &controllerv1alpha1.RegistryConfig{
176-
Path: "fake-registry",
178+
Path: "fake-registry",
179+
CopyOperatorAuthSecret: pointer.Bool(true),
177180
},
178181
},
179182
},
@@ -206,7 +209,8 @@ var _ = Describe("BackupCronJobReconciler", func() {
206209
Enable: &enabled,
207210
Schedule: schedule1,
208211
Registry: &controllerv1alpha1.RegistryConfig{
209-
Path: "fake-registry",
212+
Path: "fake-registry",
213+
CopyOperatorAuthSecret: pointer.Bool(true),
210214
},
211215
},
212216
},
@@ -242,7 +246,8 @@ var _ = Describe("BackupCronJobReconciler", func() {
242246
Enable: &enabled,
243247
Schedule: schedule1,
244248
Registry: &controllerv1alpha1.RegistryConfig{
245-
Path: "fake-registry",
249+
Path: "fake-registry",
250+
CopyOperatorAuthSecret: pointer.Bool(true),
246251
},
247252
},
248253
},
@@ -268,7 +273,8 @@ var _ = Describe("BackupCronJobReconciler", func() {
268273
Enable: &enabled,
269274
Schedule: schedule,
270275
Registry: &controllerv1alpha1.RegistryConfig{
271-
Path: "fake-registry",
276+
Path: "fake-registry",
277+
CopyOperatorAuthSecret: pointer.Bool(true),
272278
},
273279
},
274280
},
@@ -303,8 +309,9 @@ var _ = Describe("BackupCronJobReconciler", func() {
303309
Enable: &enabled,
304310
Schedule: schedule,
305311
Registry: &controllerv1alpha1.RegistryConfig{
306-
Path: "fake-registry",
307-
AuthSecret: "non-existent",
312+
Path: "fake-registry",
313+
AuthSecret: "non-existent",
314+
CopyOperatorAuthSecret: pointer.Bool(true),
308315
},
309316
},
310317
},
@@ -335,7 +342,8 @@ var _ = Describe("BackupCronJobReconciler", func() {
335342
Enable: &enabled,
336343
Schedule: schedule,
337344
Registry: &controllerv1alpha1.RegistryConfig{
338-
Path: "fake-registry",
345+
Path: "fake-registry",
346+
CopyOperatorAuthSecret: pointer.Bool(true),
339347
},
340348
OrasConfig: &controllerv1alpha1.OrasConfig{
341349
ExtraArgs: "--extra-arg1",
@@ -392,7 +400,8 @@ var _ = Describe("BackupCronJobReconciler", func() {
392400
Schedule: schedule,
393401
BackoffLimit: &backoffLimit,
394402
Registry: &controllerv1alpha1.RegistryConfig{
395-
Path: "fake-registry",
403+
Path: "fake-registry",
404+
CopyOperatorAuthSecret: pointer.Bool(true),
396405
},
397406
},
398407
},
@@ -428,7 +437,8 @@ var _ = Describe("BackupCronJobReconciler", func() {
428437
Enable: &enabled,
429438
Schedule: schedule,
430439
Registry: &controllerv1alpha1.RegistryConfig{
431-
Path: "fake-registry",
440+
Path: "fake-registry",
441+
CopyOperatorAuthSecret: pointer.Bool(true),
432442
},
433443
},
434444
},
@@ -469,7 +479,8 @@ var _ = Describe("BackupCronJobReconciler", func() {
469479
Enable: &enabled,
470480
Schedule: schedule,
471481
Registry: &controllerv1alpha1.RegistryConfig{
472-
Path: "fake-registry",
482+
Path: "fake-registry",
483+
CopyOperatorAuthSecret: pointer.Bool(true),
473484
},
474485
},
475486
},
@@ -500,8 +511,9 @@ var _ = Describe("BackupCronJobReconciler", func() {
500511
Enable: &enabled,
501512
Schedule: schedule,
502513
Registry: &controllerv1alpha1.RegistryConfig{
503-
Path: "my-registry:5000",
504-
AuthSecret: "my-secret",
514+
Path: "my-registry:5000",
515+
AuthSecret: "my-secret",
516+
CopyOperatorAuthSecret: pointer.Bool(true),
505517
},
506518
},
507519
},
@@ -536,8 +548,9 @@ var _ = Describe("BackupCronJobReconciler", func() {
536548
Enable: &enabled,
537549
Schedule: schedule,
538550
Registry: &controllerv1alpha1.RegistryConfig{
539-
Path: "my-registry:5000",
540-
AuthSecret: "my-secret",
551+
Path: "my-registry:5000",
552+
AuthSecret: "my-secret",
553+
CopyOperatorAuthSecret: pointer.Bool(true),
541554
},
542555
},
543556
},
@@ -572,7 +585,8 @@ var _ = Describe("BackupCronJobReconciler", func() {
572585
Enable: &enabled,
573586
Schedule: schedule,
574587
Registry: &controllerv1alpha1.RegistryConfig{
575-
Path: "fake-registry",
588+
Path: "fake-registry",
589+
CopyOperatorAuthSecret: pointer.Bool(true),
576590
},
577591
OrasConfig: &controllerv1alpha1.OrasConfig{
578592
ExtraArgs: "--extra-arg1",

pkg/secrets/backup.go

Lines changed: 66 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -24,12 +24,12 @@ import (
2424
"github.com/devfile/devworkspace-operator/pkg/constants"
2525
"github.com/go-logr/logr"
2626
corev1 "k8s.io/api/core/v1"
27+
k8sErrors "k8s.io/apimachinery/pkg/api/errors"
2728
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2829
"k8s.io/apimachinery/pkg/runtime"
30+
"k8s.io/utils/pointer"
2931
"sigs.k8s.io/controller-runtime/pkg/client"
3032
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
31-
32-
"github.com/devfile/devworkspace-operator/pkg/provision/sync"
3333
)
3434

3535
// GetRegistryAuthSecret retrieves the registry authentication secret for accessing backup images
@@ -54,8 +54,14 @@ func HandleRegistryAuthSecret(ctx context.Context, c client.Client, workspace *d
5454
return nil, nil
5555
}
5656

57+
// Extract flag value (default: false, users must provide their own secret)
58+
copyOperatorAuthSecret := pointer.BoolDeref(
59+
dwOperatorConfig.Workspace.BackupCronJob.Registry.CopyOperatorAuthSecret,
60+
false,
61+
)
62+
5763
// On the restore path (operatorConfigNamespace == ""), look for the predefined name
58-
// that CopySecret always uses. On the backup path, look for the configured name
64+
// that EnsureRegistryAuthSecret always uses. On the backup path, look for the configured name
5965
// because the secret may exist directly in the workspace namespace under that name.
6066
lookupName := secretName
6167
if operatorConfigNamespace == "" {
@@ -88,13 +94,46 @@ func HandleRegistryAuthSecret(ctx context.Context, c client.Client, workspace *d
8894
log.Error(err, "Failed to get registry auth secret for backup job", "secretName", secretName)
8995
return nil, err
9096
}
91-
log.Info("Successfully retrieved registry auth secret for backup job", "secretName", secretName)
92-
return CopySecret(ctx, c, workspace, registryAuthSecret, scheme, log)
97+
log.Info("Successfully retrieved registry auth secret from operator namespace", "secretName", secretName)
98+
return EnsureRegistryAuthSecret(ctx, c, workspace, registryAuthSecret, scheme, log, copyOperatorAuthSecret)
9399
}
94100

95-
// CopySecret copies the given secret from the operator namespace to the workspace namespace.
96-
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) {
97-
// Construct the desired secret state
101+
// EnsureRegistryAuthSecret copies the given secret from the operator namespace to the workspace namespace.
102+
// If copyOperatorAuthSecret is false and the secret doesn't exist, it returns an error.
103+
// If a secret already exists in the workspace namespace, it is never overwritten.
104+
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) {
105+
// Check if secret already exists (user-provided)
106+
existingSecret := &corev1.Secret{}
107+
err = c.Get(ctx, client.ObjectKey{
108+
Name: constants.DevWorkspaceBackupAuthSecretName,
109+
Namespace: workspace.Namespace,
110+
}, existingSecret)
111+
112+
if err == nil {
113+
// User provided their own secret - always respect it, never overwrite
114+
log.Info("Using existing registry auth secret from workspace namespace",
115+
"secretName", constants.DevWorkspaceBackupAuthSecretName)
116+
return existingSecret, nil
117+
}
118+
119+
if client.IgnoreNotFound(err) != nil {
120+
return nil, err
121+
}
122+
123+
// Secret doesn't exist - check if we should copy
124+
if !copyOperatorAuthSecret {
125+
return nil, fmt.Errorf(
126+
"registry auth secret %q not found in workspace namespace %q. "+
127+
"copyOperatorAuthSecret is set to false, so the operator will not copy the secret. "+
128+
"Please manually create a secret of type kubernetes.io/dockerconfigjson with the name %q "+
129+
"in the workspace namespace",
130+
constants.DevWorkspaceBackupAuthSecretName,
131+
workspace.Namespace,
132+
constants.DevWorkspaceBackupAuthSecretName,
133+
)
134+
}
135+
136+
// copyOperatorAuthSecret is true - create it from operator's secret
98137
desiredSecret := &corev1.Secret{
99138
ObjectMeta: metav1.ObjectMeta{
100139
Name: constants.DevWorkspaceBackupAuthSecretName,
@@ -111,27 +150,27 @@ func CopySecret(ctx context.Context, c client.Client, workspace *dw.DevWorkspace
111150
return nil, err
112151
}
113152

114-
// Use the sync mechanism
115-
clusterAPI := sync.ClusterAPI{
116-
Client: c,
117-
Scheme: scheme,
118-
Logger: log,
119-
Ctx: ctx,
120-
}
121-
122-
syncedObj, err := sync.SyncObjectWithCluster(desiredSecret, clusterAPI)
153+
// Use direct Create instead of SyncObjectWithCluster to avoid overwrites
154+
err = c.Create(ctx, desiredSecret)
123155
if err != nil {
124-
if _, ok := err.(*sync.NotInSyncError); !ok {
125-
return nil, err
156+
if k8sErrors.IsAlreadyExists(err) {
157+
// Race condition - secret was created between Get and Create
158+
// Fetch and return it (respect what's there)
159+
if err := c.Get(ctx, client.ObjectKey{
160+
Name: constants.DevWorkspaceBackupAuthSecretName,
161+
Namespace: workspace.Namespace,
162+
}, existingSecret); err != nil {
163+
return nil, err
164+
}
165+
log.Info("Registry auth secret was created concurrently, using existing secret",
166+
"secretName", constants.DevWorkspaceBackupAuthSecretName)
167+
return existingSecret, nil
126168
}
127-
// NotInSyncError means the sync operation was successful but triggered a change
128-
log.Info("Successfully synced secret", "name", desiredSecret.Name, "namespace", workspace.Namespace)
129-
}
130-
131-
// If syncedObj is nil (due to NotInSyncError), return the desired object
132-
if syncedObj == nil {
133-
return desiredSecret, nil
169+
return nil, err
134170
}
135171

136-
return syncedObj.(*corev1.Secret), nil
172+
log.Info("Created registry auth secret from operator credentials",
173+
"secretName", constants.DevWorkspaceBackupAuthSecretName,
174+
"namespace", workspace.Namespace)
175+
return desiredSecret, nil
137176
}

0 commit comments

Comments
 (0)