Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion pkg/secrets/backup.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,30 @@ func HandleRegistryAuthSecret(ctx context.Context, c client.Client, workspace *d
if client.IgnoreNotFound(err) != nil {
return nil, err
}
// If we don't provide an operator namespace, don't attempt to look there
// If we don't provide an operator namespace, don't attempt to look there.
// However, CopySecret always writes the secret under DevWorkspaceBackupAuthSecretName,
// regardless of the configured name. If the configured name differs, attempt a fallback
// lookup under the canonical name so that the restore path can find it.
if operatorConfigNamespace == "" {
if secretName == constants.DevWorkspaceBackupAuthSecretName {
// The configured name IS the canonical name; it was already looked up and not
// found above, so there is nothing else to try.
return nil, nil
}
fallbackSecret := &corev1.Secret{}
fallbackErr := c.Get(ctx, client.ObjectKey{
Name: constants.DevWorkspaceBackupAuthSecretName,
Namespace: workspace.Namespace,
}, fallbackSecret)
if fallbackErr == nil {
log.Info("Registry auth secret not found under configured name in workspace namespace; using canonical backup auth secret as fallback",
"configuredName", secretName,
"fallbackName", constants.DevWorkspaceBackupAuthSecretName)
Comment thread
akurinnoy marked this conversation as resolved.
Outdated
return fallbackSecret, nil
}
if client.IgnoreNotFound(fallbackErr) != nil {
return nil, fallbackErr
}
return nil, nil
}
log.Info("Registry auth secret not found in workspace namespace, checking operator namespace", "secretName", secretName)
Expand Down
201 changes: 201 additions & 0 deletions pkg/secrets/backup_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,201 @@
//
// Copyright (c) 2019-2026 Red Hat, Inc.
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
//

package secrets_test

import (
"context"
"errors"
"testing"

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"

dwv2 "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2"
controllerv1alpha1 "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1"
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/apimachinery/pkg/runtime/schema"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/client/fake"
"sigs.k8s.io/controller-runtime/pkg/log/zap"

"github.com/devfile/devworkspace-operator/pkg/constants"
"github.com/devfile/devworkspace-operator/pkg/secrets"
Comment on lines +26 to +37
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Move all project-local imports into the third import block.

github.com/devfile/devworkspace-operator/apis/controller/v1alpha1 is project-local but is currently grouped with third-party/Kubernetes imports.

As per coding guidelines "Organize imports into three groups separated by blank lines: (1) standard library, (2) third-party + Kubernetes, (3) project-local."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/secrets/backup_test.go` around lines 26 - 37, The imports in
backup_test.go are mis-grouped: move project-local imports (specifically
github.com/devfile/devworkspace-operator/apis/controller/v1alpha1 imported as
controllerv1alpha1 and the project-local packages
github.com/devfile/devworkspace-operator/pkg/constants and
github.com/devfile/devworkspace-operator/pkg/secrets) into the third import
block (project-local group) separated by a blank line from the standard library
and third-party/Kubernetes groups so the file follows the three-group import
guideline.

)

func TestSecrets(t *testing.T) {
RegisterFailHandler(Fail)
RunSpecs(t, "Secrets Suite")
}

// buildScheme returns a minimal runtime.Scheme for tests: core v1 and the DWO API types.
func buildScheme() *runtime.Scheme {
scheme := runtime.NewScheme()
Expect(corev1.AddToScheme(scheme)).To(Succeed())
Expect(dwv2.AddToScheme(scheme)).To(Succeed())
Expect(controllerv1alpha1.AddToScheme(scheme)).To(Succeed())
return scheme
}

// makeWorkspace returns a minimal DevWorkspace in the given namespace.
func makeWorkspace(namespace string) *dwv2.DevWorkspace {
return &dwv2.DevWorkspace{
ObjectMeta: metav1.ObjectMeta{
Name: "test-workspace",
Namespace: namespace,
},
}
}

// makeConfig returns an OperatorConfiguration with BackupCronJob configured to use the given auth secret name.
func makeConfig(authSecretName string) *controllerv1alpha1.OperatorConfiguration {
return &controllerv1alpha1.OperatorConfiguration{
Workspace: &controllerv1alpha1.WorkspaceConfig{
BackupCronJob: &controllerv1alpha1.BackupCronJobConfig{
Registry: &controllerv1alpha1.RegistryConfig{
Path: "example.registry.io/org",
AuthSecret: authSecretName,
},
},
},
}
}

// makeSecret returns a corev1.Secret with the given name and namespace.
func makeSecret(name, namespace string) *corev1.Secret {
return &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: name,
Namespace: namespace,
},
Data: map[string][]byte{"auth": []byte("dXNlcjpwYXNz")},
Type: corev1.SecretTypeDockerConfigJson,
}
Comment on lines +79 to +87
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify dockerconfigjson fixtures and key usage across Go tests.
# Expected: SecretTypeDockerConfigJson fixtures should use the ".dockerconfigjson" key.
rg -nP --type=go -C3 'SecretTypeDockerConfigJson|map\[string\]\[\]byte\{'

Repository: devfile/devworkspace-operator

Length of output: 6697


Change the secret fixture to use .dockerconfigjson key.

For corev1.SecretTypeDockerConfigJson secrets, the fixture should use ".dockerconfigjson" instead of "auth" to match Kubernetes API server expectations. The correct pattern is used in other tests (e.g., controllers/workspace/devworkspace_controller_test.go).

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/secrets/backup_test.go` around lines 79 - 87, The secret fixture function
makeSecret currently populates Data with the "auth" key but should use the
Kubernetes-standard ".dockerconfigjson" key for
corev1.SecretTypeDockerConfigJson; update makeSecret to set Data to
map[string][]byte{".dockerconfigjson": []byte("dXNlcjpwYXNz")} so the fixture
matches API expectations and other tests (e.g.,
controllers/workspace/devworkspace_controller_test.go).

}

var _ = Describe("HandleRegistryAuthSecret (restore path: operatorConfigNamespace='')", func() {
const workspaceNS = "user-namespace"

var (
ctx context.Context
scheme *runtime.Scheme
log = zap.New(zap.UseDevMode(true)).WithName("SecretsTest")
)

BeforeEach(func() {
ctx = context.Background()
scheme = buildScheme()
})

It("returns the secret directly when the configured name is present in the workspace namespace", func() {
By("creating a workspace-namespace secret whose name matches the configured auth secret name")
configuredName := "quay-backup-auth"
secret := makeSecret(configuredName, workspaceNS)

fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(secret).Build()
workspace := makeWorkspace(workspaceNS)
config := makeConfig(configuredName)

result, err := secrets.HandleRegistryAuthSecret(ctx, fakeClient, workspace, config, "", scheme, log)
Expect(err).NotTo(HaveOccurred())
Expect(result).NotTo(BeNil())
Expect(result.Name).To(Equal(configuredName))
})

It("returns the canonical backup auth secret as fallback when configured name is absent (the bug fix case)", func() {
By("creating only the canonical DevWorkspaceBackupAuthSecretName secret in the workspace namespace")
canonicalSecret := makeSecret(constants.DevWorkspaceBackupAuthSecretName, workspaceNS)

fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(canonicalSecret).Build()
workspace := makeWorkspace(workspaceNS)
// Configured name is something like "quay-backup-auth" — different from the canonical name.
config := makeConfig("quay-backup-auth")

result, err := secrets.HandleRegistryAuthSecret(ctx, fakeClient, workspace, config, "", scheme, log)
Expect(err).NotTo(HaveOccurred())
Expect(result).NotTo(BeNil(), "should fall back to the canonical secret copied by CopySecret")
Expect(result.Name).To(Equal(constants.DevWorkspaceBackupAuthSecretName))
})

It("returns nil, nil when neither the configured name nor the canonical name exists in the workspace namespace", func() {
By("using a fake client with no secrets at all")
fakeClient := fake.NewClientBuilder().WithScheme(scheme).Build()
workspace := makeWorkspace(workspaceNS)
config := makeConfig("quay-backup-auth")

result, err := secrets.HandleRegistryAuthSecret(ctx, fakeClient, workspace, config, "", scheme, log)
Expect(err).NotTo(HaveOccurred())
Expect(result).To(BeNil())
})

It("returns the secret on the first lookup when the configured name IS the canonical name (no duplicate query)", func() {
By("creating the canonical secret in the workspace namespace and configuring the same name")
canonicalSecret := makeSecret(constants.DevWorkspaceBackupAuthSecretName, workspaceNS)

fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(canonicalSecret).Build()
workspace := makeWorkspace(workspaceNS)
// Configured name equals the canonical constant — must be found on the first Get.
config := makeConfig(constants.DevWorkspaceBackupAuthSecretName)

result, err := secrets.HandleRegistryAuthSecret(ctx, fakeClient, workspace, config, "", scheme, log)
Expect(err).NotTo(HaveOccurred())
Expect(result).NotTo(BeNil())
Expect(result.Name).To(Equal(constants.DevWorkspaceBackupAuthSecretName))
})

It("propagates a real (non-NotFound) error from the fallback lookup", func() {
By("wrapping a fake client so that the fallback lookup returns a server error")
// The configured name differs from the canonical name, so the code will attempt the fallback
// lookup. We simulate that lookup returning a non-NotFound error.
errClient := &errorOnNameClient{
Client: fake.NewClientBuilder().WithScheme(scheme).Build(),
failName: constants.DevWorkspaceBackupAuthSecretName,
failErr: k8sErrors.NewInternalError(errors.New("simulated etcd timeout")),
}
workspace := makeWorkspace(workspaceNS)
config := makeConfig("quay-backup-auth")

result, err := secrets.HandleRegistryAuthSecret(ctx, errClient, workspace, config, "", scheme, log)
Expect(err).To(HaveOccurred())
Expect(result).To(BeNil())
Expect(err.Error()).To(ContainSubstring("simulated etcd timeout"))
})
Comment on lines +104 to +143
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add restore-path coverage for configured-name-first behavior.

These cases only validate predefined-name lookup. To match the fallback requirement, add cases where:

  1. configured secret exists (should be used), and
  2. configured name equals predefined name (fallback path is skipped).
    Without these, a direct-predefined lookup regression can pass tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/secrets/backup_test.go` around lines 104 - 143, Add two unit tests to
cover the "configured-name-first" behavior in HandleRegistryAuthSecret: (1)
create a secret named as the configured value (e.g., config :=
makeConfig("my-configured-secret")) in the workspace and assert
HandleRegistryAuthSecret returns that secret (not the predefined one), and (2)
set the configured name equal to constants.DevWorkspaceBackupAuthSecretName and
ensure the function still returns the predefined secret via the direct lookup
path (i.e., the fallback lookup must be skipped). Use the existing test patterns
(fake.NewClientBuilder(), makeWorkspace, makeSecret, ctx, scheme, log) and
assert errors/results the same way as the other cases so these new tests catch
regressions in the configured-name-first logic.

})

// errorOnNameClient is a thin client wrapper that injects an error for a specific secret name.
type errorOnNameClient struct {
client.Client
failName string
failErr error
}

func (e *errorOnNameClient) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error {
if secret, ok := obj.(*corev1.Secret); ok {
_ = secret
if key.Name == e.failName {
return e.failErr
}
}
return e.Client.Get(ctx, key, obj, opts...)
}

// Ensure errorOnNameClient satisfies client.Client at compile time.
var _ client.Client = &errorOnNameClient{}

// secretGR is the GroupResource for secrets, used when constructing test errors.
var secretGR = schema.GroupResource{Group: "", Resource: "secrets"} //nolint:unused
Comment thread
akurinnoy marked this conversation as resolved.
Outdated
Loading