From 12c1ef4ff2781482e996d88521090db3e00c5af7 Mon Sep 17 00:00:00 2001 From: samay43 Date: Wed, 5 Aug 2026 14:03:42 +0530 Subject: [PATCH 1/9] validate backup name format before contacting the API server Signed-off-by: samay43 --- pkg/cmd/cli/backup/create.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/pkg/cmd/cli/backup/create.go b/pkg/cmd/cli/backup/create.go index 5e18f468f..f42a4dc70 100644 --- a/pkg/cmd/cli/backup/create.go +++ b/pkg/cmd/cli/backup/create.go @@ -26,6 +26,7 @@ import ( "github.com/spf13/pflag" kubeerrs "k8s.io/apimachinery/pkg/util/errors" "k8s.io/client-go/tools/cache" + "k8s.io/apimachinery/pkg/util/validation" kbclient "sigs.k8s.io/controller-runtime/pkg/client" velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" @@ -186,10 +187,12 @@ func (o *CreateOptions) Validate(c *cobra.Command, args []string, f client.Facto return err } - // Ensure that unless FromSchedule is set, args contains a backup name - if o.FromSchedule == "" && len(args) != 1 { - return fmt.Errorf("a backup name is required, unless you are creating based on a schedule") - } + // Ensure the backup name is a valid Kubernetes resource name + if o.FromSchedule == "" { + if errs := validation.IsDNS1123Subdomain(o.Name); len(errs) > 0 { + return fmt.Errorf("invalid backup name %q: %s", o.Name, strings.Join(errs, "; ")) + } +} errs := collections.ValidateNamespaceIncludesExcludes(o.IncludeNamespaces, o.ExcludeNamespaces) if len(errs) > 0 { From be6b4d38f2e828045801096ad74224744cb26e52 Mon Sep 17 00:00:00 2001 From: samay43 Date: Wed, 5 Aug 2026 20:24:35 +0530 Subject: [PATCH 2/9] gofmt pkg/cmd/cli/backup/create.go Signed-off-by: samay43 --- pkg/cmd/cli/backup/create.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/cmd/cli/backup/create.go b/pkg/cmd/cli/backup/create.go index f42a4dc70..fd46c50fa 100644 --- a/pkg/cmd/cli/backup/create.go +++ b/pkg/cmd/cli/backup/create.go @@ -25,8 +25,8 @@ import ( "github.com/spf13/cobra" "github.com/spf13/pflag" kubeerrs "k8s.io/apimachinery/pkg/util/errors" - "k8s.io/client-go/tools/cache" "k8s.io/apimachinery/pkg/util/validation" + "k8s.io/client-go/tools/cache" kbclient "sigs.k8s.io/controller-runtime/pkg/client" velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" @@ -189,10 +189,10 @@ func (o *CreateOptions) Validate(c *cobra.Command, args []string, f client.Facto // Ensure the backup name is a valid Kubernetes resource name if o.FromSchedule == "" { - if errs := validation.IsDNS1123Subdomain(o.Name); len(errs) > 0 { - return fmt.Errorf("invalid backup name %q: %s", o.Name, strings.Join(errs, "; ")) - } -} + if errs := validation.IsDNS1123Subdomain(o.Name); len(errs) > 0 { + return fmt.Errorf("invalid backup name %q: %s", o.Name, strings.Join(errs, "; ")) + } + } errs := collections.ValidateNamespaceIncludesExcludes(o.IncludeNamespaces, o.ExcludeNamespaces) if len(errs) > 0 { From e97d01405b49c936971bbdc764c0798a1cac1bdc Mon Sep 17 00:00:00 2001 From: samay43 Date: Thu, 6 Aug 2026 10:52:03 +0530 Subject: [PATCH 3/9] add changelog entry Signed-off-by: samay43 --- changelogs/unreleased/10167-samay43 | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelogs/unreleased/10167-samay43 diff --git a/changelogs/unreleased/10167-samay43 b/changelogs/unreleased/10167-samay43 new file mode 100644 index 000000000..0955fcc99 --- /dev/null +++ b/changelogs/unreleased/10167-samay43 @@ -0,0 +1 @@ +Validate backup name format before contacting the API server From 164d9343aad08b9a4091ab042487af2d608c1d50 Mon Sep 17 00:00:00 2001 From: samay43 Date: Wed, 12 Aug 2026 09:51:10 +0530 Subject: [PATCH 4/9] move backup name validation to Args to run before cluster contact, and validate regardless of --from-schedule Signed-off-by: samay43 --- pkg/cmd/cli/backup/create.go | 12 +++++++++++- pkg/cmd/cli/backup/create_test.go | 2 +- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/cli/backup/create.go b/pkg/cmd/cli/backup/create.go index 7046f83e2..74cfe861d 100644 --- a/pkg/cmd/cli/backup/create.go +++ b/pkg/cmd/cli/backup/create.go @@ -46,7 +46,17 @@ func NewCreateCommand(f client.Factory, use string) *cobra.Command { c := &cobra.Command{ Use: use + " NAME", Short: "Create a backup", - Args: cobra.MaximumNArgs(1), + Args: func(c *cobra.Command, args []string) error { + if err := cobra.MaximumNArgs(1)(c, args); err != nil { + return err + } + if len(args) == 1 { + if errs := validation.IsDNS1123Subdomain(args[0]); len(errs) > 0 { + return fmt.Errorf("invalid backup name %q: %s", args[0], strings.Join(errs, "; ")) + } + } + return nil + }, Run: func(c *cobra.Command, args []string) { cmd.CheckError(o.Complete(args, f)) cmd.CheckError(o.Validate(c, args, f)) diff --git a/pkg/cmd/cli/backup/create_test.go b/pkg/cmd/cli/backup/create_test.go index 718ab0e96..4512ccd6c 100644 --- a/pkg/cmd/cli/backup/create_test.go +++ b/pkg/cmd/cli/backup/create_test.go @@ -233,7 +233,7 @@ func TestCreateOptions_OrderedResources(t *testing.T) { } func TestCreateCommand(t *testing.T) { - name := "nameToBeCreated" + name := "name-to-be-created" args := []string{name} t.Run("create a backup create command with full options except fromSchedule and wait, then run by create option", func(t *testing.T) { From 52f7c24084fef0062c10f76a251826cfdc0d7167 Mon Sep 17 00:00:00 2001 From: samay43 Date: Wed, 12 Aug 2026 16:39:47 +0530 Subject: [PATCH 5/9] address review feedback: validate backup name and require name unless from-schedule Signed-off-by: samay43 --- pkg/cmd/cli/backup/create.go | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/pkg/cmd/cli/backup/create.go b/pkg/cmd/cli/backup/create.go index 74cfe861d..b5821a9b1 100644 --- a/pkg/cmd/cli/backup/create.go +++ b/pkg/cmd/cli/backup/create.go @@ -46,10 +46,14 @@ func NewCreateCommand(f client.Factory, use string) *cobra.Command { c := &cobra.Command{ Use: use + " NAME", Short: "Create a backup", - Args: func(c *cobra.Command, args []string) error { + Args: func(c *cobra.Command, args []string) error { if err := cobra.MaximumNArgs(1)(c, args); err != nil { return err } + fromSchedule, _ := c.Flags().GetString("from-schedule") + if fromSchedule == "" && len(args) == 0 { + return fmt.Errorf("a backup name is required, unless you are creating based on a schedule") + } if len(args) == 1 { if errs := validation.IsDNS1123Subdomain(args[0]); len(errs) > 0 { return fmt.Errorf("invalid backup name %q: %s", args[0], strings.Join(errs, "; ")) @@ -202,13 +206,16 @@ func (o *CreateOptions) Validate(c *cobra.Command, args []string, f client.Facto return err } - // Ensure the backup name is a valid Kubernetes resource name - if o.FromSchedule == "" { + // Ensure that unless FromSchedule is set, a backup name is required + if o.FromSchedule == "" && o.Name == "" { + return fmt.Errorf("a backup name is required, unless you are creating based on a schedule") + } + // Validate the backup name format whenever a name is provided + if o.Name != "" { if errs := validation.IsDNS1123Subdomain(o.Name); len(errs) > 0 { return fmt.Errorf("invalid backup name %q: %s", o.Name, strings.Join(errs, "; ")) } } - errs := collections.ValidateNamespaceIncludesExcludes(o.IncludeNamespaces, o.ExcludeNamespaces) if len(errs) > 0 { return kubeerrs.NewAggregate(errs) From c27a661d1d269eaea2f90251c22aba10fff8b959 Mon Sep 17 00:00:00 2001 From: samay43 Date: Thu, 13 Aug 2026 09:44:47 +0530 Subject: [PATCH 6/9] gofmt: fix indentation in create.go Signed-off-by: samay43 --- pkg/cmd/cli/backup/create.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/cli/backup/create.go b/pkg/cmd/cli/backup/create.go index b5821a9b1..3ef773b47 100644 --- a/pkg/cmd/cli/backup/create.go +++ b/pkg/cmd/cli/backup/create.go @@ -46,7 +46,7 @@ func NewCreateCommand(f client.Factory, use string) *cobra.Command { c := &cobra.Command{ Use: use + " NAME", Short: "Create a backup", - Args: func(c *cobra.Command, args []string) error { + Args: func(c *cobra.Command, args []string) error { if err := cobra.MaximumNArgs(1)(c, args); err != nil { return err } From fa3f5737c896c2836d7b922bca744309eed7e25e Mon Sep 17 00:00:00 2001 From: samay43 Date: Thu, 13 Aug 2026 10:57:54 +0530 Subject: [PATCH 7/9] add test coverage for Args validation and Validate method Signed-off-by: samay43 --- pkg/cmd/cli/backup/create_test.go | 110 ++++++++++++++++++++++++++++++ 1 file changed, 110 insertions(+) diff --git a/pkg/cmd/cli/backup/create_test.go b/pkg/cmd/cli/backup/create_test.go index fe321f9c6..e955719eb 100644 --- a/pkg/cmd/cli/backup/create_test.go +++ b/pkg/cmd/cli/backup/create_test.go @@ -449,3 +449,113 @@ func TestCreateCommand(t *testing.T) { assert.NoError(t, e) }) } +func TestCreateCommand_Args(t *testing.T) { + testCases := []struct { + name string + args []string + fromSchedule string + expectError bool + }{ + { + name: "should error when no name and no from-schedule", + args: []string{}, + expectError: true, + }, + { + name: "should pass when a valid name is provided", + args: []string{"my-backup"}, + expectError: false, + }, + { + name: "should error when the name is not a valid DNS1123 subdomain", + args: []string{"Invalid_Name!"}, + expectError: true, + }, + { + name: "should pass with no name when from-schedule is set", + args: []string{}, + fromSchedule: "daily-backup", + expectError: false, + }, + { + name: "should error when more than one arg is given", + args: []string{"name1", "name2"}, + expectError: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + f := &factorymocks.Factory{} + cmd := NewCreateCommand(f, "") + if tc.fromSchedule != "" { + err := cmd.Flags().Set("from-schedule", tc.fromSchedule) + assert.NoError(t, err) + } + + err := cmd.Args(cmd, tc.args) + + if tc.expectError { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } +} + +func TestCreateOptions_Validate(t *testing.T) { + testCases := []struct { + name string + optName string + fromSchedule string + args []string + expectError bool + }{ + { + name: "should error when no name and no from-schedule", + optName: "", + args: []string{}, + expectError: true, + }, + { + name: "should pass with a valid name and no from-schedule", + optName: "my-backup", + args: []string{"my-backup"}, + expectError: false, + }, + { + name: "should error when name is invalid, regardless of from-schedule", + optName: "Invalid_Name!", + fromSchedule: "daily-backup", + args: []string{"Invalid_Name!"}, + expectError: true, + }, + { + name: "should pass when from-schedule is set and no name given", + optName: "", + fromSchedule: "daily-backup", + args: []string{}, + expectError: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + f := &factorymocks.Factory{} + cmd := NewCreateCommand(f, "") + + o := NewCreateOptions() + o.Name = tc.optName + o.FromSchedule = tc.fromSchedule + + err := o.Validate(cmd, tc.args, f) + + if tc.expectError { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } +} From e2c3b4b2699836b7df83787ba2293c2053729eb2 Mon Sep 17 00:00:00 2001 From: samay43 Date: Thu, 13 Aug 2026 11:08:21 +0530 Subject: [PATCH 8/9] fix lint: use require.NoError for error assertion Signed-off-by: samay43 --- pkg/cmd/cli/backup/create_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/cli/backup/create_test.go b/pkg/cmd/cli/backup/create_test.go index e955719eb..5f9b9aac0 100644 --- a/pkg/cmd/cli/backup/create_test.go +++ b/pkg/cmd/cli/backup/create_test.go @@ -490,7 +490,7 @@ func TestCreateCommand_Args(t *testing.T) { cmd := NewCreateCommand(f, "") if tc.fromSchedule != "" { err := cmd.Flags().Set("from-schedule", tc.fromSchedule) - assert.NoError(t, err) + require.NoError(t, err) } err := cmd.Args(cmd, tc.args) From efe1279d49b945b437f1ed598a4eaef798c8816d Mon Sep 17 00:00:00 2001 From: samay43 Date: Fri, 14 Aug 2026 10:37:57 +0530 Subject: [PATCH 9/9] validate schedule-derived backup names don't exceed length limit Signed-off-by: samay43 --- pkg/cmd/cli/backup/create.go | 11 +++++++++++ pkg/cmd/cli/backup/create_test.go | 14 ++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/pkg/cmd/cli/backup/create.go b/pkg/cmd/cli/backup/create.go index 7fe6b0e64..a8d988d72 100644 --- a/pkg/cmd/cli/backup/create.go +++ b/pkg/cmd/cli/backup/create.go @@ -216,6 +216,17 @@ func (o *CreateOptions) Validate(c *cobra.Command, args []string, f client.Facto return fmt.Errorf("invalid backup name %q: %s", o.Name, strings.Join(errs, "; ")) } } + // When a backup name will be generated from the schedule (i.e. FromSchedule + // is set and no explicit name was given), ensure the schedule name leaves + // enough room for the generated timestamp suffix ("-" + 14-digit timestamp, + // 15 characters total) within the DNS1123 subdomain length limit. + if o.FromSchedule != "" && o.Name == "" { + const timestampSuffixLen = 15 // "-" + "20060102150405" + maxScheduleNameLen := validation.DNS1123SubdomainMaxLength - timestampSuffixLen + if len(o.FromSchedule) > maxScheduleNameLen { + return fmt.Errorf("schedule name %q is too long: must be %d characters or fewer to leave room for the generated timestamp suffix", o.FromSchedule, maxScheduleNameLen) + } + } errs := collections.ValidateNamespaceIncludesExcludes(o.IncludeNamespaces, o.ExcludeNamespaces) if len(errs) > 0 { return kubeerrs.NewAggregate(errs) diff --git a/pkg/cmd/cli/backup/create_test.go b/pkg/cmd/cli/backup/create_test.go index 5f9b9aac0..528e76943 100644 --- a/pkg/cmd/cli/backup/create_test.go +++ b/pkg/cmd/cli/backup/create_test.go @@ -538,6 +538,20 @@ func TestCreateOptions_Validate(t *testing.T) { args: []string{}, expectError: false, }, + { + name: "should pass when schedule name leaves room for timestamp suffix", + optName: "", + fromSchedule: strings.Repeat("a", 238), // exactly at the 238-char limit + args: []string{}, + expectError: false, + }, + { + name: "should error when schedule name is too long to leave room for timestamp suffix", + optName: "", + fromSchedule: strings.Repeat("a", 239), // one over the 238-char limit + args: []string{}, + expectError: true, + }, } for _, tc := range testCases {