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 <[email protected]>

Author: jaypipes

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)
 
 <div style="float: left">

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,

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

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

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)