🐛 Fix namespaced GVK check to use version (#2875)

* Fix namespaced GVK check to use version
A particular Kind may only be present in a specific version of a group. When
querying the RESTMapper we should include the version to ensure the
cached group is updated to pick up new versions as needed.

Signed-off-by: Griffin Davis <[email protected]>

* Add unit tests for IsGVKNamespaced

Include unit tests for varying combinations of new GVKs introduced at
runtime to validate cache updates.

Signed-off-by: Griffin Davis <[email protected]>

---------

Signed-off-by: Griffin Davis <[email protected]>

pkg/client/apiutil/apimachinery.go

@@ -72,7 +72,10 @@ func IsObjectNamespaced(obj runtime.Object, scheme *runtime.Scheme, restmapper m
 // IsGVKNamespaced returns true if the object having the provided
 // GVK is namespace scoped.
 func IsGVKNamespaced(gvk schema.GroupVersionKind, restmapper meta.RESTMapper) (bool, error) {
-	restmapping, err := restmapper.RESTMapping(schema.GroupKind{Group: gvk.Group, Kind: gvk.Kind})
+	// Fetch the RESTMapping using the complete GVK. If we exclude the Version, the Version set
+	// will be populated using the cached Group if available. This can lead to failures updating
+	// the cache with new Versions of CRDs registered at runtime.
+	restmapping, err := restmapper.RESTMapping(schema.GroupKind{Group: gvk.Group, Kind: gvk.Kind}, gvk.Version)
 	if err != nil {
 		return false, fmt.Errorf("failed to get restmapping: %w", err)
 	}

pkg/client/apiutil/apimachinery_test.go

@@ -0,0 +1,188 @@
+/*
+Copyright 2024 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package apiutil_test
+
+import (
+	"context"
+	"testing"
+
+	gmg "github.com/onsi/gomega"
+	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/client-go/discovery"
+	"k8s.io/client-go/kubernetes/scheme"
+	"k8s.io/client-go/rest"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
+)
+
+func TestApiMachinery(t *testing.T) {
+	restCfg, tearDownFn := setupEnvtest(t)
+	defer tearDownFn(t)
+
+	// Details of the GVK registered at initialization.
+	initialGvk := metav1.GroupVersionKind{
+		Group:   "crew.example.com",
+		Version: "v1",
+		Kind:    "Driver",
+	}
+
+	// A set of GVKs to register at runtime with varying properties.
+	runtimeGvks := []struct {
+		name   string
+		gvk    metav1.GroupVersionKind
+		plural string
+	}{
+		{
+			name: "new Kind and Version added to existing Group",
+			gvk: metav1.GroupVersionKind{
+				Group:   "crew.example.com",
+				Version: "v1alpha1",
+				Kind:    "Passenger",
+			},
+			plural: "passengers",
+		},
+		{
+			name: "new Kind added to existing Group and Version",
+			gvk: metav1.GroupVersionKind{
+				Group:   "crew.example.com",
+				Version: "v1",
+				Kind:    "Garage",
+			},
+			plural: "garages",
+		},
+		{
+			name: "new GVK",
+			gvk: metav1.GroupVersionKind{
+				Group:   "inventory.example.com",
+				Version: "v1",
+				Kind:    "Taxi",
+			},
+			plural: "taxis",
+		},
+	}
+
+	t.Run("IsGVKNamespaced should report scope for GVK registered at initialization", func(t *testing.T) {
+		g := gmg.NewWithT(t)
+
+		httpClient, err := rest.HTTPClientFor(restCfg)
+		g.Expect(err).NotTo(gmg.HaveOccurred())
+
+		lazyRestMapper, err := apiutil.NewDynamicRESTMapper(restCfg, httpClient)
+		g.Expect(err).NotTo(gmg.HaveOccurred())
+
+		s := scheme.Scheme
+		err = apiextensionsv1.AddToScheme(s)
+		g.Expect(err).NotTo(gmg.HaveOccurred())
+
+		// Query the scope of a GVK that was registered at initialization.
+		scope, err := apiutil.IsGVKNamespaced(
+			schema.GroupVersionKind(initialGvk),
+			lazyRestMapper,
+		)
+		g.Expect(err).NotTo(gmg.HaveOccurred())
+		g.Expect(scope).To(gmg.BeTrue())
+	})
+
+	for _, runtimeGvk := range runtimeGvks {
+		t.Run("IsGVKNamespaced should report scope for "+runtimeGvk.name, func(t *testing.T) {
+			g := gmg.NewWithT(t)
+			ctx := context.Background()
+
+			httpClient, err := rest.HTTPClientFor(restCfg)
+			g.Expect(err).NotTo(gmg.HaveOccurred())
+
+			lazyRestMapper, err := apiutil.NewDynamicRESTMapper(restCfg, httpClient)
+			g.Expect(err).NotTo(gmg.HaveOccurred())
+
+			s := scheme.Scheme
+			err = apiextensionsv1.AddToScheme(s)
+			g.Expect(err).NotTo(gmg.HaveOccurred())
+
+			c, err := client.New(restCfg, client.Options{Scheme: s})
+			g.Expect(err).NotTo(gmg.HaveOccurred())
+
+			// Run a valid query to initialize cache.
+			scope, err := apiutil.IsGVKNamespaced(
+				schema.GroupVersionKind(initialGvk),
+				lazyRestMapper,
+			)
+			g.Expect(err).NotTo(gmg.HaveOccurred())
+			g.Expect(scope).To(gmg.BeTrue())
+
+			// Register a new CRD at runtime.
+			crd := newCRD(ctx, g, c, runtimeGvk.gvk.Group, runtimeGvk.gvk.Kind, runtimeGvk.plural)
+			version := crd.Spec.Versions[0]
+			version.Name = runtimeGvk.gvk.Version
+			version.Storage = true
+			version.Served = true
+			crd.Spec.Versions = []apiextensionsv1.CustomResourceDefinitionVersion{version}
+			crd.Spec.Scope = apiextensionsv1.NamespaceScoped
+
+			g.Expect(c.Create(ctx, crd)).To(gmg.Succeed())
+			t.Cleanup(func() {
+				g.Expect(c.Delete(ctx, crd)).To(gmg.Succeed())
+			})
+
+			// Wait until the CRD is registered.
+			g.Eventually(func(g gmg.Gomega) {
+				isRegistered, err := isCrdRegistered(restCfg, runtimeGvk.gvk)
+				g.Expect(err).NotTo(gmg.HaveOccurred())
+				g.Expect(isRegistered).To(gmg.BeTrue())
+			}).Should(gmg.Succeed(), "GVK should be available")
+
+			// Query the scope of the GVK registered at runtime.
+			scope, err = apiutil.IsGVKNamespaced(
+				schema.GroupVersionKind(runtimeGvk.gvk),
+				lazyRestMapper,
+			)
+			g.Expect(err).NotTo(gmg.HaveOccurred())
+			g.Expect(scope).To(gmg.BeTrue())
+		})
+	}
+}
+
+// Check if a slice of APIResource contains a given Kind.
+func kindInAPIResources(resources *metav1.APIResourceList, kind string) bool {
+	for _, res := range resources.APIResources {
+		if res.Kind == kind {
+			return true
+		}
+	}
+	return false
+}
+
+// Check if a CRD has registered with the API server using a DiscoveryClient.
+func isCrdRegistered(cfg *rest.Config, gvk metav1.GroupVersionKind) (bool, error) {
+	discHTTP, err := rest.HTTPClientFor(cfg)
+	if err != nil {
+		return false, err
+	}
+
+	discClient, err := discovery.NewDiscoveryClientForConfigAndClient(cfg, discHTTP)
+	if err != nil {
+		return false, err
+	}
+
+	resources, err := discClient.ServerResourcesForGroupVersion(gvk.Group + "/" + gvk.Version)
+	if err != nil {
+		return false, err
+	}
+
+	return kindInAPIResources(resources, gvk.Kind), nil
+}