Skip to content

Commit a266d16

Browse files
akurinnoyclaude
akurinnoy
and
claude
authored
Backup registry auth secret must not be owned by any workspace (#1631)
* fix: do not set ownerReference on backup registry auth secret The backup registry auth secret (devworkspace-backup-registry-auth) is a namespace singleton shared by all workspaces. Setting a controller ownerReference to a single workspace caused Kubernetes garbage collection to delete the secret when that workspace was deleted, breaking backup/restore for all remaining workspaces in the namespace. Remove the SetControllerReference call so the secret persists independently of any workspace lifecycle. The secret is cleaned up naturally when the namespace is deleted. Assisted-by: Claude Code Signed-off-by: Oleksii Kurinnyi <okurynny@redhat.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com> * fix: restore path falls back to operator namespace for auth secret When the backup registry auth secret is missing from the workspace namespace (e.g. after GC on upgrade), the restore path now resolves the operator namespace via infrastructure.GetNamespace() and copies the secret from there, matching the backup path behavior. Previously the restore path returned nil when the secret was missing, causing restore init containers to fail on private registries. Assisted-by: Claude Code Signed-off-by: Oleksii Kurinnyi <okurynny@redhat.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com> * fix: return error when operator namespace cannot be resolved Return an error instead of silently returning nil when infrastructure.GetNamespace() fails on the restore path. This makes auth failures visible immediately rather than causing a confusing image pull error later. Also properly save and restore the WATCH_NAMESPACE env var in tests. Assisted-by: Claude Code Signed-off-by: Oleksii Kurinnyi <okurynny@redhat.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com> * fixup! fix: do not set ownerReference on backup registry auth secret Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com> * refactor: move ADR from docs/ to adr/ directory Signed-off-by: Oleksii Kurinnyi <okurynny@redhat.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com> * chore: move ADR to separate PR The ADR framework is introduced in a separate PR to keep the bugfix focused. Signed-off-by: Oleksii Kurinnyi <okurynny@redhat.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com> * fix: check AuthSecret before resolving operator namespace Move the AuthSecret check before the infrastructure.GetNamespace() call so anonymous registries (no AuthSecret configured) don't fail with a namespace resolution error. Also explicitly unset WATCH_NAMESPACE in the restore-path test suite to avoid environment-dependent flakiness. Assisted-by: Claude Code Signed-off-by: Oleksii Kurinnyi <okurynny@redhat.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com> --------- Signed-off-by: Oleksii Kurinnyi <okurynny@redhat.com> Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 113b402 commit a266d16

2 files changed

Lines changed: 145 additions & 23 deletions

File tree

‎pkg/secrets/backup.go‎

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -22,13 +22,13 @@ import (
2222
dw "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2"
2323
controllerv1alpha1 "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1"
2424
"github.com/devfile/devworkspace-operator/pkg/constants"
25+
"github.com/devfile/devworkspace-operator/pkg/infrastructure"
2526
"github.com/go-logr/logr"
2627
corev1 "k8s.io/api/core/v1"
2728
k8sErrors "k8s.io/apimachinery/pkg/api/errors"
2829
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2930
"k8s.io/apimachinery/pkg/runtime"
3031
"sigs.k8s.io/controller-runtime/pkg/client"
31-
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
3232
)
3333

3434
// GetRegistryAuthSecret retrieves the registry authentication secret for accessing backup images
@@ -60,11 +60,6 @@ func HandleRegistryAuthSecret(ctx context.Context, c client.Client, workspace *d
6060
if client.IgnoreNotFound(err) != nil {
6161
return nil, err
6262
}
63-
// If we don't provide an operator namespace, don't attempt to look there.
64-
if operatorConfigNamespace == "" {
65-
return nil, nil
66-
}
67-
6863
// Check if AuthSecret is configured in operator config
6964
authSecretName := dwOperatorConfig.Workspace.BackupCronJob.Registry.AuthSecret
7065
if len(authSecretName) == 0 {
@@ -76,6 +71,14 @@ func HandleRegistryAuthSecret(ctx context.Context, c client.Client, workspace *d
7671
return nil, nil
7772
}
7873

74+
if operatorConfigNamespace == "" {
75+
resolvedNS, nsErr := infrastructure.GetNamespace()
76+
if nsErr != nil {
77+
return nil, fmt.Errorf("cannot resolve operator namespace to copy registry auth secret: %w", nsErr)
78+
}
79+
operatorConfigNamespace = resolvedNS
80+
}
81+
7982
log.Info("Registry auth secret not found in workspace namespace, checking operator namespace",
8083
"secretName", authSecretName,
8184
"operatorNamespace", operatorConfigNamespace)
@@ -111,10 +114,6 @@ func CopySecret(ctx context.Context, c client.Client, workspace *dw.DevWorkspace
111114
Type: sourceSecret.Type,
112115
}
113116

114-
if err := controllerutil.SetControllerReference(workspace, desiredSecret, scheme); err != nil {
115-
return nil, err
116-
}
117-
118117
err = c.Create(ctx, desiredSecret)
119118
if err != nil {
120119
if k8sErrors.IsAlreadyExists(err) {

‎pkg/secrets/backup_test.go‎

Lines changed: 136 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ package secrets_test
1818
import (
1919
"context"
2020
"errors"
21+
"os"
2122
"testing"
2223

2324
. "github.com/onsi/ginkgo/v2"
@@ -34,6 +35,7 @@ import (
3435
"sigs.k8s.io/controller-runtime/pkg/log/zap"
3536

3637
"github.com/devfile/devworkspace-operator/pkg/constants"
38+
"github.com/devfile/devworkspace-operator/pkg/infrastructure"
3739
"github.com/devfile/devworkspace-operator/pkg/secrets"
3840
)
3941

@@ -91,14 +93,26 @@ var _ = Describe("HandleRegistryAuthSecret (restore path: operatorConfigNamespac
9193
const workspaceNS = "user-namespace"
9294

9395
var (
94-
ctx context.Context
95-
scheme *runtime.Scheme
96-
log = zap.New(zap.UseDevMode(true)).WithName("SecretsTest")
96+
ctx context.Context
97+
scheme *runtime.Scheme
98+
log = zap.New(zap.UseDevMode(true)).WithName("SecretsTest")
99+
origWatchNS string
100+
hadWatchNS bool
97101
)
98102

99103
BeforeEach(func() {
100104
ctx = context.Background()
101105
scheme = buildScheme()
106+
origWatchNS, hadWatchNS = os.LookupEnv(infrastructure.WatchNamespaceEnvVar)
107+
os.Unsetenv(infrastructure.WatchNamespaceEnvVar)
108+
})
109+
110+
AfterEach(func() {
111+
if hadWatchNS {
112+
os.Setenv(infrastructure.WatchNamespaceEnvVar, origWatchNS)
113+
} else {
114+
os.Unsetenv(infrastructure.WatchNamespaceEnvVar)
115+
}
102116
})
103117

104118
It("returns the predefined secret when it exists in the workspace namespace", func() {
@@ -115,14 +129,15 @@ var _ = Describe("HandleRegistryAuthSecret (restore path: operatorConfigNamespac
115129
Expect(result.Name).To(Equal(constants.DevWorkspaceBackupAuthSecretName))
116130
})
117131

118-
It("returns nil when the predefined secret does not exist in the workspace namespace", func() {
119-
By("using a fake client with no secrets")
132+
It("returns error when secret is missing and operator namespace cannot be resolved", func() {
133+
By("using a fake client with no secrets and no WATCH_NAMESPACE set")
120134
fakeClient := fake.NewClientBuilder().WithScheme(scheme).Build()
121135
workspace := makeWorkspace(workspaceNS)
122136
config := makeConfig("quay-backup-auth")
123137

124138
result, err := secrets.HandleRegistryAuthSecret(ctx, fakeClient, workspace, config, "", scheme, log)
125-
Expect(err).NotTo(HaveOccurred())
139+
Expect(err).To(HaveOccurred())
140+
Expect(err.Error()).To(ContainSubstring("cannot resolve operator namespace"))
126141
Expect(result).To(BeNil())
127142
})
128143

@@ -188,7 +203,7 @@ var _ = Describe("HandleRegistryAuthSecret (backup path: operatorConfigNamespace
188203
Expect(result).To(BeNil())
189204
})
190205

191-
It("copies secret from operator namespace when AuthSecret is configured and secret not found in workspace namespace", func() {
206+
It("copies secret from operator namespace without ownerReferences", func() {
192207
By("creating a secret in the operator namespace")
193208
operatorSecret := makeSecret(constants.DevWorkspaceBackupAuthSecretName, operatorNS)
194209
operatorSecret.Data = map[string][]byte{"auth": []byte("operator-credentials")}
@@ -216,12 +231,8 @@ var _ = Describe("HandleRegistryAuthSecret (backup path: operatorConfigNamespace
216231
By("verifying the copied secret has the watch-secret label")
217232
Expect(copiedSecret.Labels).To(HaveKeyWithValue(constants.DevWorkspaceWatchSecretLabel, "true"))
218233

219-
By("verifying the copied secret has an owner reference to the workspace")
220-
Expect(copiedSecret.OwnerReferences).To(HaveLen(1))
221-
Expect(copiedSecret.OwnerReferences[0].Name).To(Equal(workspace.Name))
222-
Expect(copiedSecret.OwnerReferences[0].Kind).To(Equal("DevWorkspace"))
223-
Expect(copiedSecret.OwnerReferences[0].Controller).NotTo(BeNil())
224-
Expect(*copiedSecret.OwnerReferences[0].Controller).To(BeTrue())
234+
By("verifying the copied secret has no ownerReferences")
235+
Expect(copiedSecret.OwnerReferences).To(BeEmpty())
225236
})
226237

227238
It("NEVER overwrites user-provided secret even if operator has different credentials", func() {
@@ -266,6 +277,59 @@ var _ = Describe("HandleRegistryAuthSecret (backup path: operatorConfigNamespace
266277
})
267278
})
268279

280+
var _ = Describe("HandleRegistryAuthSecret (restore path: fallback to operator namespace)", func() {
281+
const (
282+
workspaceNS = "user-namespace"
283+
operatorNS = "operator-namespace"
284+
)
285+
286+
var (
287+
ctx context.Context
288+
scheme *runtime.Scheme
289+
log = zap.New(zap.UseDevMode(true)).WithName("SecretsTest")
290+
origWatchNS string
291+
hadWatchNS bool
292+
)
293+
294+
BeforeEach(func() {
295+
ctx = context.Background()
296+
scheme = buildScheme()
297+
origWatchNS, hadWatchNS = os.LookupEnv(infrastructure.WatchNamespaceEnvVar)
298+
os.Setenv(infrastructure.WatchNamespaceEnvVar, operatorNS)
299+
})
300+
301+
AfterEach(func() {
302+
if hadWatchNS {
303+
os.Setenv(infrastructure.WatchNamespaceEnvVar, origWatchNS)
304+
} else {
305+
os.Unsetenv(infrastructure.WatchNamespaceEnvVar)
306+
}
307+
})
308+
309+
It("copies the secret from operator namespace when missing in workspace namespace", func() {
310+
By("creating the auth secret only in the operator namespace")
311+
operatorSecret := makeSecret("quay-backup-auth", operatorNS)
312+
313+
fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(operatorSecret).Build()
314+
workspace := makeWorkspace(workspaceNS)
315+
config := makeConfig("quay-backup-auth")
316+
317+
result, err := secrets.HandleRegistryAuthSecret(ctx, fakeClient, workspace, config, "", scheme, log)
318+
Expect(err).NotTo(HaveOccurred())
319+
Expect(result).NotTo(BeNil())
320+
Expect(result.Name).To(Equal(constants.DevWorkspaceBackupAuthSecretName))
321+
Expect(result.Namespace).To(Equal(workspaceNS))
322+
323+
By("verifying the secret was copied to the workspace namespace")
324+
copied := &corev1.Secret{}
325+
err = fakeClient.Get(ctx, client.ObjectKey{
326+
Name: constants.DevWorkspaceBackupAuthSecretName,
327+
Namespace: workspaceNS,
328+
}, copied)
329+
Expect(err).NotTo(HaveOccurred())
330+
})
331+
})
332+
269333
// errorOnNameClient is a thin client wrapper that injects an error for a specific secret name.
270334
type errorOnNameClient struct {
271335
client.Client
@@ -285,3 +349,62 @@ func (e *errorOnNameClient) Get(ctx context.Context, key client.ObjectKey, obj c
285349

286350
// Ensure errorOnNameClient satisfies client.Client at compile time.
287351
var _ client.Client = &errorOnNameClient{}
352+
353+
var _ = Describe("CopySecret", func() {
354+
const (
355+
workspaceNS = "user-namespace"
356+
operatorNS = "operator-namespace"
357+
)
358+
359+
var (
360+
ctx context.Context
361+
scheme *runtime.Scheme
362+
log = zap.New(zap.UseDevMode(true)).WithName("SecretsTest")
363+
)
364+
365+
BeforeEach(func() {
366+
ctx = context.Background()
367+
scheme = buildScheme()
368+
})
369+
370+
It("creates the secret without ownerReferences", func() {
371+
By("copying a source secret into the workspace namespace")
372+
sourceSecret := makeSecret("quay-push-secret", operatorNS)
373+
workspace := makeWorkspace(workspaceNS)
374+
375+
fakeClient := fake.NewClientBuilder().WithScheme(scheme).Build()
376+
377+
result, err := secrets.CopySecret(ctx, fakeClient, workspace, sourceSecret, scheme, log)
378+
Expect(err).NotTo(HaveOccurred())
379+
Expect(result).NotTo(BeNil())
380+
Expect(result.Name).To(Equal(constants.DevWorkspaceBackupAuthSecretName))
381+
Expect(result.Namespace).To(Equal(workspaceNS))
382+
383+
By("verifying the created secret has no ownerReferences")
384+
created := &corev1.Secret{}
385+
err = fakeClient.Get(ctx, client.ObjectKey{
386+
Name: constants.DevWorkspaceBackupAuthSecretName,
387+
Namespace: workspaceNS,
388+
}, created)
389+
Expect(err).NotTo(HaveOccurred())
390+
Expect(created.OwnerReferences).To(BeEmpty())
391+
})
392+
393+
It("preserves the secret data and type from the source", func() {
394+
sourceSecret := &corev1.Secret{
395+
ObjectMeta: metav1.ObjectMeta{
396+
Name: "quay-push-secret",
397+
Namespace: operatorNS,
398+
},
399+
Data: map[string][]byte{".dockerconfigjson": []byte(`{"auths":{}}`)},
400+
Type: corev1.SecretTypeDockerConfigJson,
401+
}
402+
workspace := makeWorkspace(workspaceNS)
403+
fakeClient := fake.NewClientBuilder().WithScheme(scheme).Build()
404+
405+
result, err := secrets.CopySecret(ctx, fakeClient, workspace, sourceSecret, scheme, log)
406+
Expect(err).NotTo(HaveOccurred())
407+
Expect(result.Data).To(HaveKey(".dockerconfigjson"))
408+
Expect(result.Type).To(Equal(corev1.SecretTypeDockerConfigJson))
409+
})
410+
})

0 commit comments

Comments
 (0)