diff --git a/changelogs/unreleased/4057-codegold79 b/changelogs/unreleased/4057-codegold79 new file mode 100644 index 000000000..321868bb5 --- /dev/null +++ b/changelogs/unreleased/4057-codegold79 @@ -0,0 +1 @@ +Validate namespace in Velero backup create command diff --git a/pkg/cmd/cli/backup/create.go b/pkg/cmd/cli/backup/create.go index b4e391314..9c1d4ffe1 100644 --- a/pkg/cmd/cli/backup/create.go +++ b/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{ diff --git a/pkg/cmd/cli/backup/create_test.go b/pkg/cmd/cli/backup/create_test.go index 09bffd81e..ec6236994 100644 --- a/pkg/cmd/cli/backup/create_test.go +++ b/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. diff --git a/pkg/controller/backup_controller.go b/pkg/controller/backup_controller.go index 8fdcb332e..034f56d43 100644 --- a/pkg/controller/backup_controller.go +++ b/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)) } diff --git a/pkg/controller/backup_deletion_controller_test.go b/pkg/controller/backup_deletion_controller_test.go index 388df8ae3..98109d7fe 100644 --- a/pkg/controller/backup_deletion_controller_test.go +++ b/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. diff --git a/pkg/util/collections/includes_excludes.go b/pkg/util/collections/includes_excludes.go index c113f0d95..898d1a260 100644 --- a/pkg/util/collections/includes_excludes.go +++ b/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 diff --git a/pkg/util/collections/includes_excludes_test.go b/pkg/util/collections/includes_excludes_test.go index 9f08a7012..a9841c25a 100644 --- a/pkg/util/collections/includes_excludes_test.go +++ b/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 '*'/''", - includes: nil, - excludes: nil, - expectedIncludes: "*", - expectedExcludes: "", + name: "unspecified includes/excludes should return '*'/''", + includes: nil, + excludes: nil, + wantIncludes: "*", + wantExcludes: "", }, { - 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) + } }) } }