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]>
gdt-dev · Sep 3, 2024 · 20bf151 · 20bf151
1 parent ecc124b
commit 20bf151
Show file tree

Hide file tree

Showing 8 changed files with 295 additions and 114 deletions.
diff --git 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)
 
 <div style="float: left">

diff --git 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
@@ -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
@@ -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
@@ -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)