From 2cb3c7f706dbf266820fbde2e1b23a320e5d3de7 Mon Sep 17 00:00:00 2001 From: Daniel <40526523+danielrodriguez@users.noreply.github.com> Date: Sun, 27 Mar 2022 23:41:21 -0300 Subject: [PATCH] Fix: kubectl create -f and kubectl delete -f are not glob friendly (#102265) * Fix: kubectl create -f and kubectl delete -f are not glob friendly * gofmt fix * removing unused receiver * adding extra test * log paths used * fixing bad merge * klog/v2 mod * rebase fixes * better error fmt * error fmt unit test * PR comments and tests * Add reference to globbing to help snippets --- .../artifacts/oddly-named-file[x].yaml | 12 +++ .../cli-runtime/pkg/resource/builder.go | 26 +++++- .../cli-runtime/pkg/resource/builder_test.go | 82 +++++++++++++++++++ .../src/k8s.io/kubectl/pkg/cmd/apply/apply.go | 3 + .../k8s.io/kubectl/pkg/cmd/delete/delete.go | 3 + 5 files changed, 124 insertions(+), 2 deletions(-) create mode 100644 staging/src/k8s.io/cli-runtime/artifacts/oddly-named-file[x].yaml diff --git a/staging/src/k8s.io/cli-runtime/artifacts/oddly-named-file[x].yaml b/staging/src/k8s.io/cli-runtime/artifacts/oddly-named-file[x].yaml new file mode 100644 index 00000000000..901276c2d71 --- /dev/null +++ b/staging/src/k8s.io/cli-runtime/artifacts/oddly-named-file[x].yaml @@ -0,0 +1,12 @@ +apiVersion: v1 +kind: Pod +metadata: + name: busybox + labels: + name: busybox +spec: + containers: + - name: busybox + image: busybox + ports: + - containerPort: 80 diff --git a/staging/src/k8s.io/cli-runtime/pkg/resource/builder.go b/staging/src/k8s.io/cli-runtime/pkg/resource/builder.go index 78040a87800..153030e9b7f 100644 --- a/staging/src/k8s.io/cli-runtime/pkg/resource/builder.go +++ b/staging/src/k8s.io/cli-runtime/pkg/resource/builder.go @@ -22,6 +22,7 @@ import ( "io" "net/url" "os" + "path/filepath" "strings" "sync" @@ -256,10 +257,15 @@ func (b *Builder) FilenameParam(enforceNamespace bool, filenameOptions *Filename } b.URL(defaultHttpGetAttempts, url) default: - if !recursive { + matches, err := expandIfFilePattern(s) + if err != nil { + b.errs = append(b.errs, fmt.Errorf("pattern %q is not valid: %v", s, err)) + continue + } + if !recursive && len(matches) == 1 { b.singleItemImplied = true } - b.Path(recursive, s) + b.Path(recursive, matches...) } } if filenameOptions.Kustomize != "" { @@ -1200,6 +1206,22 @@ func HasNames(args []string) (bool, error) { return hasCombinedTypes || len(args) > 1, nil } +// expandIfFilePattern returns all the filenames that match the input pattern +// or the filename if it is a specific filename and not a pattern. +// If the input is a pattern and it yields no result it will result in an error. +func expandIfFilePattern(pattern string) ([]string, error) { + if _, err := os.Stat(pattern); os.IsNotExist(err) { + matches, err := filepath.Glob(pattern) + if err == nil && len(matches) == 0 { + return nil, fmt.Errorf("no match") + } + if err == nil || err != filepath.ErrBadPattern { + return matches, err + } + } + return []string{pattern}, nil +} + type cachingCategoryExpanderFunc struct { delegate CategoryExpanderFunc diff --git a/staging/src/k8s.io/cli-runtime/pkg/resource/builder_test.go b/staging/src/k8s.io/cli-runtime/pkg/resource/builder_test.go index 5206e6cc22c..1f5eb6058ec 100644 --- a/staging/src/k8s.io/cli-runtime/pkg/resource/builder_test.go +++ b/staging/src/k8s.io/cli-runtime/pkg/resource/builder_test.go @@ -625,6 +625,88 @@ func TestDirectoryBuilder(t *testing.T) { } } +func TestFilePatternBuilderWhenPatternYieldsNoResult(t *testing.T) { + const pathPattern = "../../artifacts/_does_not_exist_/*.yaml" + b := newDefaultBuilder(). + FilenameParam(false, &FilenameOptions{Recursive: false, Filenames: []string{pathPattern}}). + NamespaceParam("test").DefaultNamespace() + + test := &testVisitor{} + singleItemImplied := false + + err := b.Do().IntoSingleItemImplied(&singleItemImplied).Visit(test.Handle) + if err == nil { + t.Fatalf("unexpected response: error is nil") + } + const expectedErrorMsg = "no match" + if !strings.Contains(err.Error(), expectedErrorMsg) { + t.Fatalf("expected %s but got %s", expectedErrorMsg, err.Error()) + } + if !strings.Contains(err.Error(), pathPattern) { + t.Fatalf("expected %s but got %s", pathPattern, err.Error()) + } +} + +func TestFilePatternBuilderWhenFileLiteralExists(t *testing.T) { + const pathPattern = "../../artifacts/oddly-named-file[x].yaml" + b := newDefaultBuilder(). + FilenameParam(false, &FilenameOptions{Recursive: false, Filenames: []string{pathPattern}}). + NamespaceParam("test").DefaultNamespace() + + test := &testVisitor{} + singleItemImplied := false + + err := b.Do().IntoSingleItemImplied(&singleItemImplied).Visit(test.Handle) + if err != nil || !singleItemImplied || len(test.Infos) != 1 { + t.Fatalf("unexpected result: %v %t %#v", err, singleItemImplied, test.Infos) + } + if !strings.Contains(test.Infos[0].Source, "oddly-named-file[x]") { + t.Errorf("unexpected file name: %#v", test.Infos[0].Source) + } +} + +func TestFilePatternBuilderWhenBadPatternUsesRawInput(t *testing.T) { + const pathPattern = "../../artifacts/[a-z*.yaml" // missing closing bracket, "[a-z]*.yaml" + b := newDefaultBuilder(). + FilenameParam(false, &FilenameOptions{Recursive: false, Filenames: []string{pathPattern}}). + NamespaceParam("test").DefaultNamespace() + + test := &testVisitor{} + singleItemImplied := false + + err := b.Do().IntoSingleItemImplied(&singleItemImplied).Visit(test.Handle) + if err == nil { + t.Fatalf("unexpected response: error is nil") + } + const expectedErrorMsg = "does not exist" + if !strings.Contains(err.Error(), expectedErrorMsg) { + t.Fatalf("expected %s but got %s", expectedErrorMsg, err.Error()) + } + if !strings.Contains(err.Error(), pathPattern) { + t.Fatalf("expected %s but got %s", pathPattern, err.Error()) + } +} + +func TestFilePatternBuilder(t *testing.T) { + b := newDefaultBuilder(). + FilenameParam(false, &FilenameOptions{Recursive: false, Filenames: []string{"../../artifacts/guestbook/redis-*.yaml"}}). + NamespaceParam("test").DefaultNamespace() + + test := &testVisitor{} + singleItemImplied := false + + err := b.Do().IntoSingleItemImplied(&singleItemImplied).Visit(test.Handle) + if err != nil || singleItemImplied || len(test.Infos) < 3 { + t.Fatalf("unexpected response: %v %t %#v", err, singleItemImplied, test.Infos) + } + + for _, info := range test.Infos { + if strings.Index(info.Name, "redis-") != 0 { + t.Errorf("unexpected response: %#v", info.Name) + } + } +} + func setupKustomizeDirectory() (string, error) { path, err := ioutil.TempDir("/tmp", "") if err != nil { diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/apply/apply.go b/staging/src/k8s.io/kubectl/pkg/cmd/apply/apply.go index 77cbcc294c4..2f13cf9698f 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/apply/apply.go +++ b/staging/src/k8s.io/kubectl/pkg/cmd/apply/apply.go @@ -151,6 +151,9 @@ var ( # Apply the JSON passed into stdin to a pod cat pod.json | kubectl apply -f - + # Apply the configuration from all files that end with '.json' - i.e. expand wildcard characters in file names + kubectl apply -f '*.json' + # Note: --prune is still in Alpha # Apply the configuration in manifest.yaml that matches label app=nginx and delete all other resources that are not in the file and match label app=nginx kubectl apply --prune -f manifest.yaml -l app=nginx diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/delete/delete.go b/staging/src/k8s.io/kubectl/pkg/cmd/delete/delete.go index 90ed11c8052..09023420114 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/delete/delete.go +++ b/staging/src/k8s.io/kubectl/pkg/cmd/delete/delete.go @@ -82,6 +82,9 @@ var ( # Delete resources from a directory containing kustomization.yaml - e.g. dir/kustomization.yaml kubectl delete -k dir + # Delete resources from all files that end with '.json' - i.e. expand wildcard characters in file names + kubectl apply -f '*.json' + # Delete a pod based on the type and name in the JSON passed into stdin cat pod.json | kubectl delete -f -