Merge pull request #10167 from samay43/fix/backup-name-validation

validate backup name format before contacting the API server

pkg/cmd/cli/backup/create.go

@@ -25,6 +25,7 @@ import (
 	"github.com/spf13/cobra"
 	"github.com/spf13/pflag"
 	kubeerrs "k8s.io/apimachinery/pkg/util/errors"
+	"k8s.io/apimachinery/pkg/util/validation"
 	"k8s.io/client-go/tools/cache"
 	kbclient "sigs.k8s.io/controller-runtime/pkg/client"
 
@@ -45,7 +46,21 @@ 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
+			}
+			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, "; "))
+				}
+			}
+			return nil
+		},
 		Run: func(c *cobra.Command, args []string) {
 			cmd.CheckError(o.Complete(args, f))
 			cmd.CheckError(o.Validate(c, args, f))
@@ -191,11 +206,27 @@ 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 {
+	// 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, "; "))
+		}
+	}
+	// 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)

pkg/cmd/cli/backup/create_test.go

@@ -245,7 +245,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) {
@@ -457,3 +457,127 @@ 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)
+				require.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,
+		},
+		{
+			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 {
+		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)
+			}
+		})
+	}
+}