Skip to content

Commit cec7215

Browse files
akurinnoydkwon17
authored andcommitted
fix: fall back to canonical backup auth secret name on restore (#1614)
* fix: fall back to canonical backup auth secret name on restore When CopySecret writes an auth secret into the workspace namespace, it always uses DevWorkspaceBackupAuthSecretName ("devworkspace-backup-registry-auth") regardless of what name the admin configured (e.g. "quay-backup-auth"). GetNamespaceRegistryAuthSecret (the restore path) calls HandleRegistryAuthSecret with operatorConfigNamespace="" and looked only for the configured name, found nothing, and returned nil — leaving the workspace-restore init container without credentials and causing CrashLoopBackOff on private registries. Fix: when operatorConfigNamespace is empty and the configured name is not found, attempt a second Get using the canonical DevWorkspaceBackupAuthSecretName. Skip the extra lookup when the configured name already equals the canonical name to avoid a redundant API call. Propagate any non-NotFound error from the fallback. Assisted-by: Claude Sonnet 4.6 (1M context) Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com> * fixup! fix: fall back to canonical backup auth secret name on restore fixup\! fix: fall back to canonical backup auth secret name on restore Address review feedback: - Use canonical DevWorkspaceBackupAuthSecretName for workspace NS lookup directly instead of falling back (dkwon17) - Remove unused secretGR variable and schema import (rohanKanojia) Assisted-by: Claude Opus 4.6 (1M context) Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com> * fixup! fixup! fix: fall back to canonical backup auth secret name on restore fixup\! fixup\! fix: fall back to canonical backup auth secret name on restore Use canonical name only on restore path, keep configured name on backup path Assisted-by: Claude Opus 4.6 (1M context) Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com> * fixup! fixup! fixup! fix: fall back to canonical backup auth secret name on restore fixup\! fixup\! fixup\! fix: fall back to canonical backup auth secret name on restore Replace "canonical" with "predefined" throughout Assisted-by: Claude Opus 4.6 (1M context) Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com> --------- Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>
1 parent 54d1084 commit cec7215

File tree

2 files changed

+176
-4
lines changed

2 files changed

+176
-4
lines changed

pkg/secrets/backup.go

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -54,19 +54,27 @@ func HandleRegistryAuthSecret(ctx context.Context, c client.Client, workspace *d
5454
return nil, nil
5555
}
5656

57-
// First check the workspace namespace for the secret
57+
// On the restore path (operatorConfigNamespace == ""), look for the predefined name
58+
// that CopySecret always uses. On the backup path, look for the configured name
59+
// because the secret may exist directly in the workspace namespace under that name.
60+
lookupName := secretName
61+
if operatorConfigNamespace == "" {
62+
lookupName = constants.DevWorkspaceBackupAuthSecretName
63+
}
64+
5865
registryAuthSecret := &corev1.Secret{}
5966
err := c.Get(ctx, client.ObjectKey{
60-
Name: secretName,
67+
Name: lookupName,
6168
Namespace: workspace.Namespace}, registryAuthSecret)
6269
if err == nil {
63-
log.Info("Successfully retrieved registry auth secret for backup from workspace namespace", "secretName", secretName)
70+
log.Info("Successfully retrieved registry auth secret for backup from workspace namespace",
71+
"secretName", lookupName)
6472
return registryAuthSecret, nil
6573
}
6674
if client.IgnoreNotFound(err) != nil {
6775
return nil, err
6876
}
69-
// If we don't provide an operator namespace, don't attempt to look there
77+
// If we don't provide an operator namespace, don't attempt to look there.
7078
if operatorConfigNamespace == "" {
7179
return nil, nil
7280
}

pkg/secrets/backup_test.go

Lines changed: 164 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,164 @@
1+
//
2+
// Copyright (c) 2019-2026 Red Hat, Inc.
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
//
15+
16+
package secrets_test
17+
18+
import (
19+
"context"
20+
"errors"
21+
"testing"
22+
23+
. "github.com/onsi/ginkgo/v2"
24+
. "github.com/onsi/gomega"
25+
26+
dwv2 "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2"
27+
controllerv1alpha1 "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1"
28+
corev1 "k8s.io/api/core/v1"
29+
k8sErrors "k8s.io/apimachinery/pkg/api/errors"
30+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
31+
"k8s.io/apimachinery/pkg/runtime"
32+
"sigs.k8s.io/controller-runtime/pkg/client"
33+
"sigs.k8s.io/controller-runtime/pkg/client/fake"
34+
"sigs.k8s.io/controller-runtime/pkg/log/zap"
35+
36+
"github.com/devfile/devworkspace-operator/pkg/constants"
37+
"github.com/devfile/devworkspace-operator/pkg/secrets"
38+
)
39+
40+
func TestSecrets(t *testing.T) {
41+
RegisterFailHandler(Fail)
42+
RunSpecs(t, "Secrets Suite")
43+
}
44+
45+
// buildScheme returns a minimal runtime.Scheme for tests: core v1 and the DWO API types.
46+
func buildScheme() *runtime.Scheme {
47+
scheme := runtime.NewScheme()
48+
Expect(corev1.AddToScheme(scheme)).To(Succeed())
49+
Expect(dwv2.AddToScheme(scheme)).To(Succeed())
50+
Expect(controllerv1alpha1.AddToScheme(scheme)).To(Succeed())
51+
return scheme
52+
}
53+
54+
// makeWorkspace returns a minimal DevWorkspace in the given namespace.
55+
func makeWorkspace(namespace string) *dwv2.DevWorkspace {
56+
return &dwv2.DevWorkspace{
57+
ObjectMeta: metav1.ObjectMeta{
58+
Name: "test-workspace",
59+
Namespace: namespace,
60+
},
61+
}
62+
}
63+
64+
// makeConfig returns an OperatorConfiguration with BackupCronJob configured to use the given auth secret name.
65+
func makeConfig(authSecretName string) *controllerv1alpha1.OperatorConfiguration {
66+
return &controllerv1alpha1.OperatorConfiguration{
67+
Workspace: &controllerv1alpha1.WorkspaceConfig{
68+
BackupCronJob: &controllerv1alpha1.BackupCronJobConfig{
69+
Registry: &controllerv1alpha1.RegistryConfig{
70+
Path: "example.registry.io/org",
71+
AuthSecret: authSecretName,
72+
},
73+
},
74+
},
75+
}
76+
}
77+
78+
// makeSecret returns a corev1.Secret with the given name and namespace.
79+
func makeSecret(name, namespace string) *corev1.Secret {
80+
return &corev1.Secret{
81+
ObjectMeta: metav1.ObjectMeta{
82+
Name: name,
83+
Namespace: namespace,
84+
},
85+
Data: map[string][]byte{"auth": []byte("dXNlcjpwYXNz")},
86+
Type: corev1.SecretTypeDockerConfigJson,
87+
}
88+
}
89+
90+
var _ = Describe("HandleRegistryAuthSecret (restore path: operatorConfigNamespace='')", func() {
91+
const workspaceNS = "user-namespace"
92+
93+
var (
94+
ctx context.Context
95+
scheme *runtime.Scheme
96+
log = zap.New(zap.UseDevMode(true)).WithName("SecretsTest")
97+
)
98+
99+
BeforeEach(func() {
100+
ctx = context.Background()
101+
scheme = buildScheme()
102+
})
103+
104+
It("returns the predefined secret when it exists in the workspace namespace", func() {
105+
By("creating the predefined DevWorkspaceBackupAuthSecretName secret in the workspace namespace")
106+
predefinedSecret := makeSecret(constants.DevWorkspaceBackupAuthSecretName, workspaceNS)
107+
108+
fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(predefinedSecret).Build()
109+
workspace := makeWorkspace(workspaceNS)
110+
config := makeConfig("quay-backup-auth")
111+
112+
result, err := secrets.HandleRegistryAuthSecret(ctx, fakeClient, workspace, config, "", scheme, log)
113+
Expect(err).NotTo(HaveOccurred())
114+
Expect(result).NotTo(BeNil())
115+
Expect(result.Name).To(Equal(constants.DevWorkspaceBackupAuthSecretName))
116+
})
117+
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")
120+
fakeClient := fake.NewClientBuilder().WithScheme(scheme).Build()
121+
workspace := makeWorkspace(workspaceNS)
122+
config := makeConfig("quay-backup-auth")
123+
124+
result, err := secrets.HandleRegistryAuthSecret(ctx, fakeClient, workspace, config, "", scheme, log)
125+
Expect(err).NotTo(HaveOccurred())
126+
Expect(result).To(BeNil())
127+
})
128+
129+
It("propagates a non-NotFound error from the workspace namespace lookup", func() {
130+
By("wrapping a fake client so that the predefined name lookup returns a server error")
131+
errClient := &errorOnNameClient{
132+
Client: fake.NewClientBuilder().WithScheme(scheme).Build(),
133+
failName: constants.DevWorkspaceBackupAuthSecretName,
134+
failErr: k8sErrors.NewInternalError(errors.New("simulated etcd timeout")),
135+
}
136+
workspace := makeWorkspace(workspaceNS)
137+
config := makeConfig("quay-backup-auth")
138+
139+
result, err := secrets.HandleRegistryAuthSecret(ctx, errClient, workspace, config, "", scheme, log)
140+
Expect(err).To(HaveOccurred())
141+
Expect(result).To(BeNil())
142+
Expect(err.Error()).To(ContainSubstring("simulated etcd timeout"))
143+
})
144+
})
145+
146+
// errorOnNameClient is a thin client wrapper that injects an error for a specific secret name.
147+
type errorOnNameClient struct {
148+
client.Client
149+
failName string
150+
failErr error
151+
}
152+
153+
func (e *errorOnNameClient) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error {
154+
if secret, ok := obj.(*corev1.Secret); ok {
155+
_ = secret
156+
if key.Name == e.failName {
157+
return e.failErr
158+
}
159+
}
160+
return e.Client.Get(ctx, key, obj, opts...)
161+
}
162+
163+
// Ensure errorOnNameClient satisfies client.Client at compile time.
164+
var _ client.Client = &errorOnNameClient{}

0 commit comments

Comments
 (0)