From 20bf151732beea7acef32ced731effc66e525fa3 Mon Sep 17 00:00:00 2001 From: Jay Pipes Date: Tue, 3 Sep 2024 13:33:39 -0400 Subject: [PATCH] fix inability to reference same-named Kinds There were a couple problems that were the root cause of this issue. First, I was only handling the difference between namespaced and non-namespaced CRDs in the GET action. For CREATE, DELETE, and APPLY, I was not properly differentiating between cluster-scoped and namespace-scoped CRDs. After fixing that, there was a problem with how the `gvrFromGVK` method on the connection struct was discovering an APIResource from the GroupVersionKind. I was only passing in the Kind instead of the GroupVersion information as well, which resulted in the discovery cache in the kube client failing to match the correct CRD when there were two different CRDs with the same Kind but different GroupVersions. Closes Issue #21 Signed-off-by: Jay Pipes --- README.md | 2 +- action.go | 131 ++++++++++++++++++++++------------ connect.go | 52 +++++++++++--- errors.go | 8 +-- eval_test.go | 17 +++++ identifier.go | 103 +++++++++++++------------- spec.go | 2 +- testdata/same-named-kind.yaml | 94 ++++++++++++++++++++++++ 8 files changed, 295 insertions(+), 114 deletions(-) create mode 100644 testdata/same-named-kind.yaml diff --git a/README.md b/README.md index 6ca43d5..554a472 100644 --- a/README.md +++ b/README.md @@ -2,7 +2,7 @@ [![Go Reference](https://pkg.go.dev/badge/github.com/gdt-dev/kube.svg)](https://pkg.go.dev/github.com/gdt-dev/kube) [![Go Report Card](https://goreportcard.com/badge/github.com/gdt-dev/kube)](https://goreportcard.com/report/github.com/gdt-dev/kube) -[![Build Status](https://github.com/gdt-dev/kube/actions/workflows/test.yml/badge.svg?branch=main)](https://github.com/gdt-dev/kube/actions) +[![Build Status](https://github.com/gdt-dev/kube/actions/workflows/gate-tests.yml/badge.svg?branch=main)](https://github.com/gdt-dev/kube/actions) [![Contributor Covenant](https://img.shields.io/badge/Contributor%20Covenant-2.1-4baaaa.svg)](CODE_OF_CONDUCT.md)
diff --git a/action.go b/action.go index adb49f5..10b9282 100644 --- a/action.go +++ b/action.go @@ -121,14 +121,11 @@ func (a *Action) get( ns string, out *interface{}, ) error { - kind, name := a.Get.KindName() - gvk := schema.GroupVersionKind{ - Kind: kind, - } - res, err := c.gvrFromGVK(gvk) + res, err := c.gvrFromArg(a.Get.Arg) if err != nil { return err } + name := a.Get.Name if name == "" { list, err := a.doList(ctx, c, res, ns) if err == nil { @@ -154,7 +151,7 @@ func (a *Action) doList( resName := res.Resource labelSelString := "" opts := metav1.ListOptions{} - withlabels := a.Get.Labels() + withlabels := a.Get.Labels if withlabels != nil { // We already validated the label selector during parse-time labelsStr := labels.Set(withlabels).String() @@ -250,21 +247,30 @@ func (a *Action) create( } for _, obj := range objs { gvk := obj.GetObjectKind().GroupVersionKind() - ons := obj.GetNamespace() - if ons == "" { - ons = ns - } res, err := c.gvrFromGVK(gvk) if err != nil { return err } resName := res.Resource - debug.Println(ctx, "kube.create: %s (ns: %s)", resName, ons) - obj, err := c.client.Resource(res).Namespace(ons).Create( - ctx, - obj, - metav1.CreateOptions{}, - ) + if c.resourceNamespaced(res) { + ons := obj.GetNamespace() + if ons == "" { + ons = ns + } + debug.Println(ctx, "kube.create: %s (ns: %s)", resName, ons) + obj, err = c.client.Resource(res).Namespace(ons).Create( + ctx, + obj, + metav1.CreateOptions{}, + ) + } else { + debug.Println(ctx, "kube.create: %s (non-namespaced resource)", resName) + obj, err = c.client.Resource(res).Create( + ctx, + obj, + metav1.CreateOptions{}, + ) + } if err != nil { return err } @@ -314,28 +320,44 @@ func (a *Action) apply( } for _, obj := range objs { gvk := obj.GetObjectKind().GroupVersionKind() - ons := obj.GetNamespace() - if ons == "" { - ons = ns - } res, err := c.gvrFromGVK(gvk) if err != nil { return err } resName := res.Resource - debug.Println(ctx, "kube.apply: %s (ns: %s)", resName, ons) - obj, err := c.client.Resource(res).Namespace(ns).Apply( - ctx, - // NOTE(jaypipes): Not sure why a separate name argument is - // necessary considering `obj` is of type - // `*unstructured.Unstructured` and therefore has the `GetName()` - // method... - obj.GetName(), - obj, - // TODO(jaypipes): Not sure if this hard-coded options struct is - // always going to work. Maybe add ability to control it? - metav1.ApplyOptions{FieldManager: fieldManagerName, Force: true}, - ) + if c.resourceNamespaced(res) { + ons := obj.GetNamespace() + if ons == "" { + ons = ns + } + debug.Println(ctx, "kube.apply: %s (ns: %s)", resName, ons) + obj, err = c.client.Resource(res).Namespace(ns).Apply( + ctx, + // NOTE(jaypipes): Not sure why a separate name argument is + // necessary considering `obj` is of type + // `*unstructured.Unstructured` and therefore has the `GetName()` + // method... + obj.GetName(), + obj, + // TODO(jaypipes): Not sure if this hard-coded options struct is + // always going to work. Maybe add ability to control it? + metav1.ApplyOptions{FieldManager: fieldManagerName, Force: true}, + ) + } else { + debug.Println(ctx, "kube.apply: %s (non-namespaced resource)", resName) + obj, err = c.client.Resource(res).Apply( + ctx, + // NOTE(jaypipes): Not sure why a separate name argument is + // necessary considering `obj` is of type + // `*unstructured.Unstructured` and therefore has the `GetName()` + // method... + obj.GetName(), + obj, + // TODO(jaypipes): Not sure if this hard-coded options struct is + // always going to work. Maybe add ability to control it? + metav1.ApplyOptions{FieldManager: fieldManagerName, Force: true}, + ) + } if err != nil { return err } @@ -385,14 +407,11 @@ func (a *Action) delete( return nil } - kind, name := a.Delete.KindName() - gvk := schema.GroupVersionKind{ - Kind: kind, - } - res, err := c.gvrFromGVK(gvk) + res, err := c.gvrFromArg(a.Delete.Arg) if err != nil { return err } + name := a.Delete.Name if name == "" { return a.doDeleteCollection(ctx, c, res, ns) } @@ -408,11 +427,22 @@ func (a *Action) doDelete( name string, ) error { resName := res.Resource + if c.resourceNamespaced(res) { + debug.Println( + ctx, "kube.delete: %s/%s (ns: %s)", + resName, name, ns, + ) + return c.client.Resource(res).Namespace(ns).Delete( + ctx, + name, + metav1.DeleteOptions{}, + ) + } debug.Println( - ctx, "kube.delete: %s/%s (ns: %s)", - resName, name, ns, + ctx, "kube.delete: %s/%s (non-namespaced resource)", + resName, name, ) - return c.client.Resource(res).Namespace(ns).Delete( + return c.client.Resource(res).Delete( ctx, name, metav1.DeleteOptions{}, @@ -428,7 +458,7 @@ func (a *Action) doDeleteCollection( ns string, ) error { opts := metav1.ListOptions{} - withlabels := a.Delete.Labels() + withlabels := a.Delete.Labels labelSelString := "" if withlabels != nil { // We already validated the label selector during parse-time @@ -437,11 +467,22 @@ func (a *Action) doDeleteCollection( opts.LabelSelector = labelsStr } resName := res.Resource + if c.resourceNamespaced(res) { + debug.Println( + ctx, "kube.delete: %s%s (ns: %s)", + resName, labelSelString, ns, + ) + return c.client.Resource(res).Namespace(ns).DeleteCollection( + ctx, + metav1.DeleteOptions{}, + opts, + ) + } debug.Println( - ctx, "kube.delete: %s%s (ns: %s)", - resName, labelSelString, ns, + ctx, "kube.delete: %s%s (non-namespaced resource)", + resName, labelSelString, ) - return c.client.Resource(res).Namespace(ns).DeleteCollection( + return c.client.Resource(res).DeleteCollection( ctx, metav1.DeleteOptions{}, opts, diff --git a/connect.go b/connect.go index 2bdd8b6..ed57e72 100644 --- a/connect.go +++ b/connect.go @@ -97,9 +97,27 @@ type connection struct { client dynamic.Interface } -// mappingFor returns a RESTMapper for a given resource type or kind -func (c *connection) mappingFor(typeOrKind string) (*meta.RESTMapping, error) { - fullySpecifiedGVR, groupResource := schema.ParseResourceArg(typeOrKind) +// mappingForGVK returns a RESTMapper for a given GroupVersionKind +func (c *connection) mappingForGVK(gvk schema.GroupVersionKind) (*meta.RESTMapping, error) { + mapping, err := c.mapper.RESTMapping(gvk.GroupKind(), gvk.Version) + if err != nil { + // if we error out here, it is because we could not match a resource or a kind + // for the given argument. To maintain consistency with previous behavior, + // announce that a resource type could not be found. + // if the error is _not_ a *meta.NoKindMatchError, then we had trouble doing discovery, + // so we should return the original error since it may help a user diagnose what is actually wrong + if meta.IsNoMatchError(err) { + return nil, fmt.Errorf("the server doesn't have a resource type %q", gvk) + } + return nil, err + } + + return mapping, nil +} + +// mappingForArg returns a RESTMapper for a given GroupVersionKind +func (c *connection) mappingForArg(arg string) (*meta.RESTMapping, error) { + fullySpecifiedGVR, groupResource := schema.ParseResourceArg(arg) gvk := schema.GroupVersionKind{} if fullySpecifiedGVR != nil { @@ -112,7 +130,7 @@ func (c *connection) mappingFor(typeOrKind string) (*meta.RESTMapping, error) { return c.mapper.RESTMapping(gvk.GroupKind(), gvk.Version) } - fullySpecifiedGVK, groupKind := schema.ParseKindArg(typeOrKind) + fullySpecifiedGVK, groupKind := schema.ParseKindArg(arg) if fullySpecifiedGVK == nil { gvk := groupKind.WithVersion("") fullySpecifiedGVK = &gvk @@ -140,18 +158,34 @@ func (c *connection) mappingFor(typeOrKind string) (*meta.RESTMapping, error) { return mapping, nil } +// gvrFromArg returns a GroupVersionResource from a resource or kind arg +// string, using the discovery client to look up the resource name (the plural +// of the kind). The returned GroupVersionResource will have the proper Group +// and Version filled in (as opposed to an APIResource which has empty Group +// and Version strings because it "inherits" its APIResourceList's GroupVersion +// ... ugh.) +func (c *connection) gvrFromArg( + arg string, +) (schema.GroupVersionResource, error) { + empty := schema.GroupVersionResource{} + r, err := c.mappingForArg(arg) + if err != nil { + return empty, ResourceUnknown(arg) + } + + return r.Resource, nil +} + // gvrFromGVK returns a GroupVersionResource from a GroupVersionKind, using the // discovery client to look up the resource name (the plural of the kind). The // returned GroupVersionResource will have the proper Group and Version filled // in (as opposed to an APIResource which has empty Group and Version strings // because it "inherits" its APIResourceList's GroupVersion ... ugh.) -func (c *connection) gvrFromGVK( - gvk schema.GroupVersionKind, -) (schema.GroupVersionResource, error) { +func (c *connection) gvrFromGVK(gvk schema.GroupVersionKind) (schema.GroupVersionResource, error) { empty := schema.GroupVersionResource{} - r, err := c.mappingFor(gvk.Kind) + r, err := c.mappingForGVK(gvk) if err != nil { - return empty, ResourceUnknown(gvk) + return empty, ResourceUnknown(gvk.String()) } return r.Resource, nil diff --git a/errors.go b/errors.go index f5c3c9a..a162551 100644 --- a/errors.go +++ b/errors.go @@ -9,7 +9,6 @@ import ( "github.com/gdt-dev/gdt/api" "gopkg.in/yaml.v3" - "k8s.io/apimachinery/pkg/runtime/schema" ) var ( @@ -181,9 +180,10 @@ func InvalidWithLabels(err error, node *yaml.Node) error { ) } -// ResourceUnknown returns ErrRuntimeResourceUnknown for a given kind -func ResourceUnknown(gvk schema.GroupVersionKind) error { - return fmt.Errorf("%w: %s", ErrResourceUnknown, gvk) +// ResourceUnknown returns ErrRuntimeResourceUnknown for a given resource or +// kind arg string +func ResourceUnknown(arg string) error { + return fmt.Errorf("%w: %s", ErrResourceUnknown, arg) } // ExpectedNotFound returns ErrExpectedNotFound for a given status code or diff --git a/eval_test.go b/eval_test.go index 3e95c65..6ba9175 100644 --- a/eval_test.go +++ b/eval_test.go @@ -67,6 +67,23 @@ func TestCreateUnknownResource(t *testing.T) { require.Nil(err) } +func TestSameNamedKind(t *testing.T) { + testutil.SkipIfNoKind(t) + require := require.New(t) + + fp := filepath.Join("testdata", "same-named-kind.yaml") + + s, err := gdt.From(fp) + require.Nil(err) + require.NotNil(s) + + ctx := gdtcontext.New() + ctx = gdtcontext.RegisterFixture(ctx, "kind", kindfix.New()) + + err = s.Run(ctx, t) + require.Nil(err) +} + func TestDeleteResourceNotFound(t *testing.T) { testutil.SkipIfNoKind(t) require := require.New(t) diff --git a/identifier.go b/identifier.go index 47184e7..65578b3 100644 --- a/identifier.go +++ b/identifier.go @@ -19,6 +19,8 @@ type resourceIdentifierWithSelector struct { // Type is the resource type to select. This should *not* be a type/name // combination. Type string `yaml:"type"` + // Name is the optional name of the resource to get + Name string `yaml:"name,omitempty"` // Labels is a map, keyed by metadata Label, of Label values to select a // resource by Labels map[string]string `yaml:"labels,omitempty"` @@ -28,27 +30,17 @@ type resourceIdentifierWithSelector struct { // either a string or a struct containing a selector with things like a label // key/value map. type ResourceIdentifier struct { - kind string `yaml:"-"` - name string `yaml:"-"` - labels map[string]string `yaml:"-"` + Arg string `yaml:"-"` + Name string `yaml:"-"` + Labels map[string]string `yaml:"-"` } // Title returns the resource identifier's kind and name, if present func (r *ResourceIdentifier) Title() string { - if r.name == "" { - return r.kind + if r.Name == "" { + return r.Arg } - return r.kind + "/" + r.name -} - -// KindName returns the resource identifier's kind and name -func (r *ResourceIdentifier) KindName() (string, string) { - return r.kind, r.name -} - -// Labels returns the resource identifier's labels map, if present -func (r *ResourceIdentifier) Labels() map[string]string { - return r.labels + return r.Arg + "/" + r.Name } // UnmarshalYAML is a custom unmarshaler that understands that the value of the @@ -67,7 +59,7 @@ func (r *ResourceIdentifier) UnmarshalYAML(node *yaml.Node) error { if strings.Count(s, "/") > 1 { return InvalidResourceSpecifier(s, node) } - r.kind, r.name = splitKindName(s) + r.Arg, r.Name = splitArgName(s) return nil } // Otherwise the resource identifier should be specified broken out as a @@ -80,21 +72,21 @@ func (r *ResourceIdentifier) UnmarshalYAML(node *yaml.Node) error { if err != nil { return InvalidWithLabels(err, node) } - r.kind = ri.Type - r.name = "" - r.labels = ri.Labels + r.Arg = ri.Type + r.Name = ri.Name + r.Labels = ri.Labels return nil } func NewResourceIdentifier( - kind string, + arg string, name string, labels map[string]string, ) *ResourceIdentifier { return &ResourceIdentifier{ - kind: kind, - name: name, - labels: labels, + Arg: arg, + Name: name, + Labels: labels, } } @@ -103,9 +95,9 @@ func NewResourceIdentifier( // like a label key/value map. type ResourceIdentifierOrFile struct { fp string `yaml:"-"` - kind string `yaml:"-"` - name string `yaml:"-"` - labels map[string]string `yaml:"-"` + Arg string `yaml:"-"` + Name string `yaml:"-"` + Labels map[string]string `yaml:"-"` } // FilePath returns the resource identifier's file path, if present @@ -119,20 +111,10 @@ func (r *ResourceIdentifierOrFile) Title() string { if r.fp != "" { return filepath.Base(r.fp) } - if r.name == "" { - return r.kind + if r.Name == "" { + return r.Arg } - return r.kind + "/" + r.name -} - -// KindName returns the resource identifier's kind and name -func (r *ResourceIdentifierOrFile) KindName() (string, string) { - return r.kind, r.name -} - -// Labels returns the resource identifier's labels map, if present -func (r *ResourceIdentifierOrFile) Labels() map[string]string { - return r.labels + return r.Arg + "/" + r.Name } // UnmarshalYAML is a custom unmarshaler that understands that the value of the @@ -158,7 +140,7 @@ func (r *ResourceIdentifierOrFile) UnmarshalYAML(node *yaml.Node) error { if strings.Count(s, "/") > 1 { return InvalidResourceSpecifierOrFilepath(s, node) } - r.kind, r.name = splitKindName(s) + r.Arg, r.Name = splitArgName(s) return nil } // Otherwise the resource identifier should be specified broken out as a @@ -171,30 +153,43 @@ func (r *ResourceIdentifierOrFile) UnmarshalYAML(node *yaml.Node) error { if err != nil { return InvalidWithLabels(err, node) } - r.kind = ri.Type - r.name = "" - r.labels = ri.Labels + r.Arg = ri.Type + r.Name = ri.Name + r.Labels = ri.Labels return nil } func NewResourceIdentifierOrFile( fp string, - kind string, + arg string, name string, labels map[string]string, ) *ResourceIdentifierOrFile { return &ResourceIdentifierOrFile{ fp: fp, - kind: kind, - name: name, - labels: labels, + Arg: arg, + Name: name, + Labels: labels, } } -// splitKindName returns the Kind for a supplied `Get` or `Delete` command -// where the user can specify either a resource kind or alias, e.g. "pods" or -// "po", or the resource kind followed by a forward slash and a resource name. -func splitKindName(subject string) (string, string) { - kind, name, _ := strings.Cut(subject, "/") - return kind, name +// splitArgName returns the resource or kind arg string for a supplied `Get` or +// `Delete` command where the user can specify either a resource kind or alias, +// e.g. "pods" or "po", or the resource kind followed by a forward slash and a +// resource name. +// +// Valid resource/kind arg plus name strings: +// +// * "pods" +// * "pod" +// * "pods/name" +// * "pod/name" +// * "deployments.apps/name" +// * "deployments.v1.apps/name" +// * "Deployment/name" +// * "Deployment.apps/name" +// * "Deployment.v1.apps/name" +func splitArgName(subject string) (string, string) { + arg, name, _ := strings.Cut(subject, "/") + return arg, name } diff --git a/spec.go b/spec.go index 75e3214..cbefb2d 100644 --- a/spec.go +++ b/spec.go @@ -138,7 +138,7 @@ func probablyFilePath(subject string) bool { if strings.ContainsAny(subject, " :\n\r\t") { return false } - return strings.ContainsRune(subject, '.') + return strings.HasSuffix(subject, ".yaml") || strings.HasSuffix(subject, ".yml") } func (s *Spec) SetBase(b api.Spec) { diff --git a/testdata/same-named-kind.yaml b/testdata/same-named-kind.yaml new file mode 100644 index 0000000..f592e5b --- /dev/null +++ b/testdata/same-named-kind.yaml @@ -0,0 +1,94 @@ +name: same-named-kind +description: test multiple Kinds with different APIVersions +fixtures: + - kind +tests: + - kube: + create: | + apiVersion: apiextensions.k8s.io/v1 + kind: CustomResourceDefinition + metadata: + name: foos.example.com + spec: + group: example.com + versions: + - name: v1 + served: true + storage: true + schema: + openAPIV3Schema: + type: object + properties: + spec: + type: object + properties: + a: + type: string + b: + type: integer + scope: Namespaced + names: + kind: Foo + plural: foos + singular: foo + - kube: + create: | + apiVersion: apiextensions.k8s.io/v1 + kind: CustomResourceDefinition + metadata: + name: foos.anotherexample.com + spec: + group: anotherexample.com + versions: + - name: v1 + served: true + storage: true + schema: + openAPIV3Schema: + type: object + properties: + spec: + type: object + properties: + a: + type: string + b: + type: integer + scope: Namespaced + names: + kind: Foo + plural: foos + singular: foo + - kube.get: foos.example.com/nonexisting + assert: + notfound: true + - kube.get: foos.anotherexample.com/nonexisting + assert: + notfound: true + # Kube assumes the first Kind-matching resource type, so this should return a + # not found, not an unknown resource + - kube.get: foos/nonexisting + assert: + notfound: true + - kube: + create: | + apiVersion: anotherexample.com/v1 + kind: Foo + metadata: + name: fooexample + spec: + a: example + b: 42 + - kube.get: foos.anotherexample.com/fooexample + assert: + matches: + spec: + b: 42 + - kube.get: foos.v1.anotherexample.com/fooexample + assert: + matches: + spec: + b: 42 + - kube.delete: foos.anotherexample.com/fooexample + - kube.delete: crds/foos.example.com + - kube.delete: crds/foos.anotherexample.com