Validate namespace in velero backup create command (#4057)

* Add namespace validation in the client

Signed-off-by: F. Gold <fgold@vmware.com>

* Add namespace validation in the backup controller

Signed-off-by: F. Gold <fgold@vmware.com>

* Add changelog for PR 4057

Signed-off-by: F. Gold <fgold@vmware.com>

* Update Copyright notice

Signed-off-by: F. Gold <fgold@vmware.com>

* Update include_excludes_test.go to follow Go standards and be easier to read

Signed-off-by: F. Gold <fgold@vmware.com>

* Add unit tests for namespace validation functions

Signed-off-by: F. Gold <fgold@vmware.com>

* Make changes per review comments

- use one set of namespace validation logic instead of writing two
- remove duplicate namespace validation functions and tests
- add namespace validation tests in includes_excludes_test.go

Signed-off-by: F. Gold <fgold@vmware.com>

* Return all ns validation err msgs as error list

Signed-off-by: F. Gold <fgold@vmware.com>

* Make error message more clear

Signed-off-by: F. Gold <fgold@vmware.com>

changelogs/unreleased/4057-codegold79

@@ -0,0 +1 @@
+Validate namespace in Velero backup create command

pkg/cmd/cli/backup/create.go

@@ -1,5 +1,5 @@
 /*
-Copyright 2020 the Velero contributors.
+Copyright The Velero Contributors.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -27,6 +27,7 @@ import (
 	"github.com/spf13/cobra"
 	"github.com/spf13/pflag"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	kubeerrs "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/client-go/tools/cache"
 
 	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
@@ -37,6 +38,7 @@ import (
 	"github.com/vmware-tanzu/velero/pkg/cmd/util/output"
 	veleroclient "github.com/vmware-tanzu/velero/pkg/generated/clientset/versioned"
 	v1 "github.com/vmware-tanzu/velero/pkg/generated/informers/externalversions/velero/v1"
+	"github.com/vmware-tanzu/velero/pkg/util/collections"
 )
 
 const DefaultBackupTTL time.Duration = 30 * 24 * time.Hour
@@ -162,6 +164,11 @@ func (o *CreateOptions) Validate(c *cobra.Command, args []string, f client.Facto
 		return fmt.Errorf("A backup name is required, unless you are creating based on a schedule.")
 	}
 
+	errs := collections.ValidateNamespaceIncludesExcludes(o.IncludeNamespaces, o.ExcludeNamespaces)
+	if len(errs) > 0 {
+		return kubeerrs.NewAggregate(errs)
+	}
+
 	if o.StorageLocation != "" {
 		location := &velerov1api.BackupStorageLocation{}
 		if err := client.Get(context.Background(), kbclient.ObjectKey{

pkg/cmd/cli/backup/create_test.go

@@ -1,5 +1,5 @@
 /*
-Copyright 2020 the Velero contributors.
+Copyright The Velero Contributors.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.

pkg/controller/backup_controller.go

@@ -1,5 +1,5 @@
 /*
-Copyright the Velero contributors.
+Copyright The Velero Contributors.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -424,7 +424,7 @@ func (c *backupController) prepareBackupRequest(backup *velerov1api.Backup) *pkg
 	}
 
 	// validate the included/excluded namespaces
-	for _, err := range collections.ValidateIncludesExcludes(request.Spec.IncludedNamespaces, request.Spec.ExcludedNamespaces) {
+	for _, err := range collections.ValidateNamespaceIncludesExcludes(request.Spec.IncludedNamespaces, request.Spec.ExcludedNamespaces) {
 		request.Status.ValidationErrors = append(request.Status.ValidationErrors, fmt.Sprintf("Invalid included/excluded namespace lists: %v", err))
 	}
 

pkg/controller/backup_deletion_controller_test.go

@@ -1,5 +1,5 @@
 /*
-Copyright the Velero contributors.
+Copyright The Velero Contributors.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.

pkg/util/collections/includes_excludes.go

@@ -1,5 +1,5 @@
 /*
-Copyright 2020 the Velero contributors.
+Copyright The Velero Contributors.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -21,6 +21,7 @@ import (
 
 	"github.com/gobwas/glob"
 	"github.com/pkg/errors"
+	"k8s.io/apimachinery/pkg/api/validation"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/util/sets"
 
@@ -154,6 +155,48 @@ func ValidateIncludesExcludes(includesList, excludesList []string) []error {
 	return errs
 }
 
+// ValidateNamespaceIncludesExcludes checks provided lists of included and
+// excluded namespaces to ensure they are a valid set of IncludesExcludes data.
+func ValidateNamespaceIncludesExcludes(includesList, excludesList []string) []error {
+	errs := ValidateIncludesExcludes(includesList, excludesList)
+
+	includes := sets.NewString(includesList...)
+	excludes := sets.NewString(excludesList...)
+
+	for _, itm := range includes.List() {
+		// Although asterisks is not a valid Kubernetes namespace name, it is
+		// allowed here.
+		if itm != "*" {
+			if nsErrs := validateNamespaceName(itm); nsErrs != nil {
+				errs = append(errs, nsErrs...)
+			}
+		}
+	}
+
+	for _, itm := range excludes.List() {
+		// Asterisks in excludes list have been checked previously.
+		if itm != "*" {
+			if nsErrs := validateNamespaceName(itm); nsErrs != nil {
+				errs = append(errs, nsErrs...)
+			}
+		}
+	}
+
+	return errs
+}
+
+func validateNamespaceName(ns string) []error {
+	var errs []error
+
+	if errMsgs := validation.ValidateNamespaceName(ns, false); errMsgs != nil {
+		for _, msg := range errMsgs {
+			errs = append(errs, errors.Errorf("invalid namespace %q: %s", ns, msg))
+		}
+	}
+
+	return errs
+}
+
 // GenerateIncludesExcludes constructs an IncludesExcludes struct by taking the provided
 // include/exclude slices, applying the specified mapping function to each item in them,
 // and adding the output of the function to the new struct. If the mapping function returns

pkg/util/collections/includes_excludes_test.go

@@ -1,5 +1,5 @@
 /*
-Copyright 2017 the Velero contributors.
+Copyright The Velero Contributors.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -29,86 +29,87 @@ func TestShouldInclude(t *testing.T) {
 		name     string
 		includes []string
 		excludes []string
-		check    string
-		should   bool
+		item     string
+		want     bool
 	}{
 		{
-			name:   "empty - include everything",
-			check:  "foo",
-			should: true,
+			name: "empty string should include every item",
+			item: "foo",
+			want: true,
 		},
 		{
-			name:     "include *",
+			name:     "include * should include every item",
 			includes: []string{"*"},
-			check:    "foo",
-			should:   true,
+			item:     "foo",
+			want:     true,
 		},
 		{
-			name:     "include specific - found",
+			name:     "item in includes list should include item",
 			includes: []string{"foo", "bar", "baz"},
-			check:    "foo",
-			should:   true,
+			item:     "foo",
+			want:     true,
 		},
 		{
-			name:     "include specific - not found",
+			name:     "item not in includes list should not include item",
 			includes: []string{"foo", "baz"},
-			check:    "bar",
-			should:   false,
+			item:     "bar",
+			want:     false,
 		},
 		{
-			name:     "include *, exclude foo",
+			name:     "include *, excluded item should not include item",
 			includes: []string{"*"},
 			excludes: []string{"foo"},
-			check:    "foo",
-			should:   false,
+			item:     "foo",
+			want:     false,
 		},
 		{
-			name:     "include *, exclude foo, check bar",
+			name:     "include *, exclude foo, bar should be included",
 			includes: []string{"*"},
 			excludes: []string{"foo"},
-			check:    "bar",
-			should:   true,
+			item:     "bar",
+			want:     true,
 		},
 		{
-			name:     "both include and exclude foo",
+			name:     "an item both included and excluded should not be included",
 			includes: []string{"foo"},
 			excludes: []string{"foo"},
-			check:    "foo",
-			should:   false,
+			item:     "foo",
+			want:     false,
 		},
 		{
-			name:     "wildcard include",
+			name:     "wildcard should include item",
 			includes: []string{"*.bar"},
-			check:    "foo.bar",
-			should:   true,
+			item:     "foo.bar",
+			want:     true,
 		},
 		{
-			name:     "wildcard include fail",
+			name:     "wildcard mismatch should not include item",
 			includes: []string{"*.bar"},
-			check:    "bar.foo",
-			should:   false,
+			item:     "bar.foo",
+			want:     false,
 		},
 		{
-			name:     "wildcard exclude",
+			name:     "wildcard exclude should not include item",
 			includes: []string{"*"},
 			excludes: []string{"*.bar"},
-			check:    "foo.bar",
-			should:   false,
+			item:     "foo.bar",
+			want:     false,
 		},
 		{
-			name:     "wildcard exclude fail",
+			name:     "wildcard mismatch should include item",
 			includes: []string{"*"},
 			excludes: []string{"*.bar"},
-			check:    "bar.foo",
-			should:   true,
+			item:     "bar.foo",
+			want:     true,
 		},
 	}
 
-	for _, test := range tests {
-		t.Run(test.name, func(t *testing.T) {
-			i := NewIncludesExcludes().Includes(test.includes...).Excludes(test.excludes...)
-			if e, a := test.should, i.ShouldInclude(test.check); e != a {
-				t.Errorf("expected %t, got %t", e, a)
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			includesExcludes := NewIncludesExcludes().Includes(tc.includes...).Excludes(tc.excludes...)
+
+			if got := includesExcludes.ShouldInclude((tc.item)); got != tc.want {
+				t.Errorf("want %t, got %t", tc.want, got)
 			}
 		})
 	}
@@ -119,7 +120,7 @@ func TestValidateIncludesExcludes(t *testing.T) {
 		name     string
 		includes []string
 		excludes []string
-		expected []error
+		want     []error
 	}{
 		{
 			name:     "empty includes (everything) is allowed",
@@ -132,30 +133,30 @@ func TestValidateIncludesExcludes(t *testing.T) {
 		{
 			name:     "include everything not allowed with other includes",
 			includes: []string{"*", "foo"},
-			expected: []error{errors.New("includes list must either contain '*' only, or a non-empty list of items")},
+			want:     []error{errors.New("includes list must either contain '*' only, or a non-empty list of items")},
 		},
 		{
 			name:     "exclude everything not allowed",
 			includes: []string{"foo"},
 			excludes: []string{"*"},
-			expected: []error{errors.New("excludes list cannot contain '*'")},
+			want:     []error{errors.New("excludes list cannot contain '*'")},
 		},
 		{
 			name:     "excludes cannot contain items in includes",
 			includes: []string{"foo", "bar"},
 			excludes: []string{"bar"},
-			expected: []error{errors.New("excludes list cannot contain an item in the includes list: bar")},
+			want:     []error{errors.New("excludes list cannot contain an item in the includes list: bar")},
 		},
 	}
 
-	for _, test := range tests {
-		t.Run(test.name, func(t *testing.T) {
-			res := ValidateIncludesExcludes(test.includes, test.excludes)
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			errs := ValidateIncludesExcludes(tc.includes, tc.excludes)
 
-			require.Equal(t, len(test.expected), len(res))
+			require.Equal(t, len(tc.want), len(errs))
 
-			for i := 0; i < len(test.expected); i++ {
-				assert.Equal(t, test.expected[i].Error(), res[i].Error())
+			for i := 0; i < len(tc.want); i++ {
+				assert.Equal(t, tc.want[i].Error(), errs[i].Error())
 			}
 		})
 	}
@@ -163,33 +164,117 @@ func TestValidateIncludesExcludes(t *testing.T) {
 
 func TestIncludeExcludeString(t *testing.T) {
 	tests := []struct {
-		name             string
-		includes         []string
-		excludes         []string
-		expectedIncludes string
-		expectedExcludes string
+		name         string
+		includes     []string
+		excludes     []string
+		wantIncludes string
+		wantExcludes string
 	}{
 		{
-			name:             "unspecified includes/excludes should return '*'/'<none>'",
-			includes:         nil,
-			excludes:         nil,
-			expectedIncludes: "*",
-			expectedExcludes: "<none>",
+			name:         "unspecified includes/excludes should return '*'/'<none>'",
+			includes:     nil,
+			excludes:     nil,
+			wantIncludes: "*",
+			wantExcludes: "<none>",
 		},
 		{
-			name:             "specific resources should result in sorted joined string",
-			includes:         []string{"foo", "bar"},
-			excludes:         []string{"baz", "xyz"},
-			expectedIncludes: "bar, foo",
-			expectedExcludes: "baz, xyz",
+			name:         "specific resources should result in sorted joined string",
+			includes:     []string{"foo", "bar"},
+			excludes:     []string{"baz", "xyz"},
+			wantIncludes: "bar, foo",
+			wantExcludes: "baz, xyz",
 		},
 	}
 
-	for _, test := range tests {
-		t.Run(test.name, func(t *testing.T) {
-			ie := NewIncludesExcludes().Includes(test.includes...).Excludes(test.excludes...)
-			assert.Equal(t, test.expectedIncludes, ie.IncludesString())
-			assert.Equal(t, test.expectedExcludes, ie.ExcludesString())
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			includesExcludes := NewIncludesExcludes().Includes(tc.includes...).Excludes(tc.excludes...)
+			assert.Equal(t, tc.wantIncludes, includesExcludes.IncludesString())
+			assert.Equal(t, tc.wantExcludes, includesExcludes.ExcludesString())
+		})
+	}
+}
+
+func TestValidateNamespaceIncludesExcludes(t *testing.T) {
+	tests := []struct {
+		name     string
+		includes []string
+		excludes []string
+		wantErr  bool
+	}{
+		{
+			name:     "empty slice doesn't return error",
+			includes: []string{},
+			wantErr:  false,
+		},
+		{
+			name:     "empty string is invalid",
+			includes: []string{""},
+			wantErr:  true,
+		},
+		{
+			name:     "asterisk by itself is valid",
+			includes: []string{"*"},
+			wantErr:  false,
+		},
+		{
+			name:     "alphanumeric names with optional dash inside are valid",
+			includes: []string{"foobar", "bar-321", "foo123bar"},
+			excludes: []string{"123bar", "barfoo", "foo-321", "bar123foo"},
+			wantErr:  false,
+		},
+		{
+			name:     "not starting or ending with an alphanumeric character is invalid",
+			includes: []string{"-123foo"},
+			excludes: []string{"foo321-", "foo321-"},
+			wantErr:  true,
+		},
+		{
+			name:     "special characters in name is invalid",
+			includes: []string{"foo?", "foo.bar", "bar_321"},
+			excludes: []string{"$foo", "foo*bar", "bar=321"},
+			wantErr:  true,
+		},
+		{
+			name:     "empty includes (everything) is valid",
+			includes: []string{},
+			wantErr:  false,
+		},
+		{
+			name:     "include everything using asterisk is valid",
+			includes: []string{"*"},
+			wantErr:  false,
+		},
+		{
+			name:     "include everything not allowed with other includes",
+			includes: []string{"*", "foo"},
+			wantErr:  true,
+		},
+		{
+			name:     "exclude everything not allowed",
+			includes: []string{"foo"},
+			excludes: []string{"*"},
+			wantErr:  true,
+		},
+		{
+			name:     "excludes cannot contain items in includes",
+			includes: []string{"foo", "bar"},
+			excludes: []string{"bar"},
+			wantErr:  true,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			errs := ValidateNamespaceIncludesExcludes(tc.includes, tc.excludes)
+
+			if tc.wantErr && len(errs) == 0 {
+				t.Errorf("%s: wanted errors but got none", tc.name)
+			}
+
+			if !tc.wantErr && len(errs) != 0 {
+				t.Errorf("%s: wanted no errors but got: %v", tc.name, errs)
+			}
 		})
 	}
 }