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()) +}