From a5391e13e7e60834b45c39da8e3f2d62d9b2a4a6 Mon Sep 17 00:00:00 2001 From: Adam Zhang Date: Tue, 24 Mar 2026 16:05:20 +0800 Subject: [PATCH] [main] fix configmap lookup in non-default namespaces (#9638) * fix configmap lookup in non-default namespaces o.Namespace is empty when Validate runs (Complete hasn't been called yet), causing VerifyJSONConfigs to query the default namespace instead of the intended one. Replace o.Namespace with f.Namespace() in all three ConfigMap validation calls so the factory's already-resolved namespace is used. Signed-off-by: Adam Zhang * switch the call order of validate/complete switch the call order of validate/complete which accomplish the same effect. Signed-off-by: Adam Zhang --------- Signed-off-by: Adam Zhang --- changelogs/unreleased/9638-adam-jian-zhang | 1 + pkg/cmd/cli/install/install.go | 2 +- pkg/cmd/cli/install/install_test.go | 172 +++++++++++++++++++++ 3 files changed, 174 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/9638-adam-jian-zhang diff --git a/changelogs/unreleased/9638-adam-jian-zhang b/changelogs/unreleased/9638-adam-jian-zhang new file mode 100644 index 000000000..3fd99e5e7 --- /dev/null +++ b/changelogs/unreleased/9638-adam-jian-zhang @@ -0,0 +1 @@ +Fix issue #9636, fix configmap lookup in non-default namespaces diff --git a/pkg/cmd/cli/install/install.go b/pkg/cmd/cli/install/install.go index 83b84ce2f..8115e1353 100644 --- a/pkg/cmd/cli/install/install.go +++ b/pkg/cmd/cli/install/install.go @@ -381,8 +381,8 @@ This is useful as a starting point for more customized installations. # velero install --provider azure --plugins velero/velero-plugin-for-microsoft-azure:v1.0.0 --bucket $BLOB_CONTAINER --secret-file ./credentials-velero --backup-location-config resourceGroup=$AZURE_BACKUP_RESOURCE_GROUP,storageAccount=$AZURE_STORAGE_ACCOUNT_ID[,subscriptionId=$AZURE_BACKUP_SUBSCRIPTION_ID] --snapshot-location-config apiTimeout=[,resourceGroup=$AZURE_BACKUP_RESOURCE_GROUP,subscriptionId=$AZURE_BACKUP_SUBSCRIPTION_ID]`, Run: func(c *cobra.Command, args []string) { - cmd.CheckError(o.Validate(c, args, f)) cmd.CheckError(o.Complete(args, f)) + cmd.CheckError(o.Validate(c, args, f)) cmd.CheckError(o.Run(c, f)) }, } diff --git a/pkg/cmd/cli/install/install_test.go b/pkg/cmd/cli/install/install_test.go index c5d147646..1e0e7a4cf 100644 --- a/pkg/cmd/cli/install/install_test.go +++ b/pkg/cmd/cli/install/install_test.go @@ -17,11 +17,18 @@ limitations under the License. package install import ( + "context" "testing" + "github.com/spf13/cobra" "github.com/spf13/pflag" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + corev1api "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + factorymocks "github.com/vmware-tanzu/velero/pkg/client/mocks" + velerotest "github.com/vmware-tanzu/velero/pkg/test" ) func TestPriorityClassNameFlag(t *testing.T) { @@ -91,3 +98,168 @@ func TestPriorityClassNameFlag(t *testing.T) { }) } } + +// makeValidateCmd returns a minimal *cobra.Command that satisfies output.ValidateFlags. +func makeValidateCmd() *cobra.Command { + c := &cobra.Command{} + // output.ValidateFlags only inspects the "output" flag; add it so validation passes. + c.Flags().StringP("output", "o", "", "output format") + return c +} + +// configMapInNamespace builds a ConfigMap with a single JSON data entry in the given namespace. +func configMapInNamespace(namespace, name, jsonValue string) *corev1api.ConfigMap { + return &corev1api.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: namespace, + Name: name, + }, + Data: map[string]string{ + "config": jsonValue, + }, + } +} + +// TestValidateConfigMapsUseFactoryNamespace verifies that Validate resolves the target +// namespace correctly for all three ConfigMap flags. +// +// The fix (Option B) calls Complete before Validate in NewCommand so that o.Namespace is +// populated from f.Namespace() before VerifyJSONConfigs runs. Tests mirror that order by +// calling Complete before Validate. +func TestValidateConfigMapsUseFactoryNamespace(t *testing.T) { + const targetNS = "tenant-b" + const defaultNS = "default" + + // Shared options that satisfy every other validation gate: + // - NoDefaultBackupLocation=true + UseVolumeSnapshots=false skips provider/bucket/plugins checks + // - NoSecret=true satisfies the secret-file check + baseOptions := func() *Options { + o := NewInstallOptions() + o.NoDefaultBackupLocation = true + o.UseVolumeSnapshots = false + o.NoSecret = true + return o + } + + tests := []struct { + name string + setupOpts func(o *Options, cmName string) + cmJSON string + wantErrMsg string // substring expected in error; empty means success + }{ + { + name: "NodeAgentConfigMap found in factory namespace", + setupOpts: func(o *Options, cmName string) { + o.NodeAgentConfigMap = cmName + }, + cmJSON: `{}`, + }, + { + name: "NodeAgentConfigMap not found when only in default namespace", + setupOpts: func(o *Options, cmName string) { + o.NodeAgentConfigMap = cmName + }, + cmJSON: `{}`, + wantErrMsg: "--node-agent-configmap specified ConfigMap", + }, + { + name: "RepoMaintenanceJobConfigMap found in factory namespace", + setupOpts: func(o *Options, cmName string) { + o.RepoMaintenanceJobConfigMap = cmName + }, + cmJSON: `{}`, + }, + { + name: "RepoMaintenanceJobConfigMap not found when only in default namespace", + setupOpts: func(o *Options, cmName string) { + o.RepoMaintenanceJobConfigMap = cmName + }, + cmJSON: `{}`, + wantErrMsg: "--repo-maintenance-job-configmap specified ConfigMap", + }, + { + name: "BackupRepoConfigMap found in factory namespace", + setupOpts: func(o *Options, cmName string) { + o.BackupRepoConfigMap = cmName + }, + cmJSON: `{}`, + }, + { + name: "BackupRepoConfigMap not found when only in default namespace", + setupOpts: func(o *Options, cmName string) { + o.BackupRepoConfigMap = cmName + }, + cmJSON: `{}`, + wantErrMsg: "--backup-repository-configmap specified ConfigMap", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + const cmName = "my-config" + + // Decide where to place the ConfigMap: + // "not found" cases put it in "default", so the factory namespace lookup misses it. + cmNamespace := targetNS + if tc.wantErrMsg != "" { + cmNamespace = defaultNS + } + + cm := configMapInNamespace(cmNamespace, cmName, tc.cmJSON) + kbClient := velerotest.NewFakeControllerRuntimeClient(t, cm) + + f := &factorymocks.Factory{} + f.On("Namespace").Return(targetNS) + f.On("KubebuilderClient").Return(kbClient, nil) + + o := baseOptions() + tc.setupOpts(o, cmName) + + // Mirror the NewCommand call order: Complete populates o.Namespace before Validate runs. + require.NoError(t, o.Complete([]string{}, f)) + + c := makeValidateCmd() + c.SetContext(context.Background()) + + err := o.Validate(c, []string{}, f) + + if tc.wantErrMsg == "" { + require.NoError(t, err) + } else { + require.Error(t, err) + assert.Contains(t, err.Error(), tc.wantErrMsg) + } + }) + } +} + +// TestNewCommandRunClosureOrder covers the Run closure in NewCommand (the lines that were +// reordered by the fix: Complete → Validate → Run). +// +// The closure uses CheckError which calls os.Exit on any error, so the only safe path is one +// where all three steps return nil. DryRun=true causes o.Run to return after PrintWithFormat +// (which is a no-op when no --output flag is set) without touching any cluster clients. +func TestNewCommandRunClosureOrder(t *testing.T) { + const targetNS = "tenant-b" + const cmName = "my-config" + + cm := configMapInNamespace(targetNS, cmName, `{}`) + kbClient := velerotest.NewFakeControllerRuntimeClient(t, cm) + + f := &factorymocks.Factory{} + f.On("Namespace").Return(targetNS) + f.On("KubebuilderClient").Return(kbClient, nil) + + c := NewCommand(f) + c.SetArgs([]string{ + "--no-default-backup-location", + "--use-volume-snapshots=false", + "--no-secret", + "--dry-run", + "--node-agent-configmap", cmName, + }) + + // Execute drives the full Run closure: Complete populates o.Namespace, Validate + // looks up the ConfigMap in targetNS (succeeds), Run returns early via DryRun. + require.NoError(t, c.Execute()) +}