Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
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
16 changes: 12 additions & 4 deletions pkg/secrets/backup.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,19 +54,27 @@ func HandleRegistryAuthSecret(ctx context.Context, c client.Client, workspace *d
return nil, nil
}

// First check the workspace namespace for the secret
// On the restore path (operatorConfigNamespace == ""), look for the predefined name
// that CopySecret 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 == "" {
lookupName = constants.DevWorkspaceBackupAuthSecretName
}

registryAuthSecret := &corev1.Secret{}
err := c.Get(ctx, client.ObjectKey{
Name: secretName,
Name: lookupName,
Namespace: workspace.Namespace}, registryAuthSecret)
if err == nil {
log.Info("Successfully retrieved registry auth secret for backup from workspace namespace", "secretName", secretName)
log.Info("Successfully retrieved registry auth secret for backup from workspace namespace",
"secretName", lookupName)
return registryAuthSecret, nil
}
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.
if operatorConfigNamespace == "" {
return nil, nil
}
Expand Down
164 changes: 164 additions & 0 deletions pkg/secrets/backup_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,164 @@
//
// 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"
"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 predefined secret when it exists in the workspace namespace", func() {
By("creating the predefined DevWorkspaceBackupAuthSecretName secret in the workspace namespace")
predefinedSecret := makeSecret(constants.DevWorkspaceBackupAuthSecretName, workspaceNS)

fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(predefinedSecret).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).NotTo(BeNil())
Expect(result.Name).To(Equal(constants.DevWorkspaceBackupAuthSecretName))
})

It("returns nil when the predefined secret does not exist in the workspace namespace", func() {
By("using a fake client with no secrets")
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("propagates a non-NotFound error from the workspace namespace lookup", func() {
By("wrapping a fake client so that the predefined name lookup returns a server 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{}
Loading