Adding changes from code review

pkg/schema/definitions/fixtures_test.go

@@ -4,10 +4,8 @@ import (
 	"bytes"
 	"fmt"
 
-	openapi_v2 "github.com/google/gnostic-models/openapiv2"
 	"github.com/rancher/wrangler/v3/pkg/yaml"
 	apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
-	"k8s.io/kube-openapi/pkg/util/proto"
 )
 
 var (
@@ -65,6 +63,26 @@ spec:
                     nullable: true
     served: true
     storage: true
+---
+apiVersion: apiextensions.k8s.io/v1
+kind: CustomResourceDefinition
+metadata:
+  name: schemaless.management.cattle.io
+spec:
+  conversion:
+    strategy: None
+  group: management.cattle.io
+  names:
+    kind: Schemaless
+    listKind: SchemalessList
+    plural: schemalese
+    singular: schemaless
+  scope: Cluster
+  preserveUnkownFields: true
+  versions:
+  - name: v2
+    served: true
+    storage: true
 `
 )
 
@@ -348,6 +366,35 @@ definitions:
     - group: "management.cattle.io"
       version: "v2"
       kind: "Nullable"
+  io.cattle.management.v2.Schemaless:
+    description: "this kind has no schema"
+    type: "object"
+    properties:
+      apiVersion:
+        description: "The APIVersion of this resource"
+        type: "string"
+      kind:
+        description: "The kind"
+        type: "string"
+      metadata:
+        description: "The metadata"
+        $ref: "#/definitions/io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta"
+      spec:
+        description: "The spec for the resource"
+        type: "object"
+        required:
+        - "name"
+        properties:
+          name:
+            description: "The name of the resource"
+            type: "string"
+          notRequired:
+            description: "Some field that isn't required"
+            type: "boolean"
+    x-kubernetes-group-version-kind:
+    - group: "management.cattle.io"
+      version: "v2"
+      kind: "Schemaless"
   io.cattle.management.NotAKind:
     type: "string"
     description: "Some string which isn't a kind"
@@ -400,114 +447,3 @@ definitions:
       kind: "ConfigMap"
       version: "v1"
 `
-
-var (
-	rawSchemalessCRDs = `
-apiVersion: apiextensions.k8s.io/v1
-kind: CustomResourceDefinition
-metadata:
-  name: schemaless.management.cattle.io
-spec:
-  conversion:
-    strategy: None
-  group: management.cattle.io
-  names:
-    kind: Schemaless
-    listKind: SchemalessList
-    plural: schemalese
-    singular: schemaless
-  scope: Cluster
-  versions:
-  - name: v2
-    schema:
-      openAPIV3Schema:
-        type: object
-        properties:
-          spec:
-            type: object
-            properties:
-              rkeConfig:
-                type: object
-                nullable: true
-        x-kubernetes-preserve-unknown-fields: true
-    served: true
-    storage: true
-`
-
-	rawSchemalessModels = `
-swagger: "2.0"
-info:
-  title: "Test openapi spec"
-  version: "v1.0.0"
-paths:
-  /apis/management.cattle.io/v3/globalroles:
-    get:
-      description: "get a global role"
-      responses:
-        200:
-          description: "OK"
-definitions:
-  io.cattle.management.v2.schemaless:
-    description: "this kind has no schema"
-    type: "object"
-    properties:
-      apiVersion:
-        description: "The APIVersion of this resource"
-        type: "string"
-      kind:
-        description: "The kind"
-        type: "string"
-      metadata:
-        description: "The metadata"
-        $ref: "#/definitions/io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta"
-      spec:
-        description: "The spec for the resource"
-        type: "object"
-        required:
-        - "name"
-        properties:
-          name:
-            description: "The name of the resource"
-            type: "string"
-          notRequired:
-            description: "Some field that isn't required"
-            type: "boolean"
-    x-kubernetes-group-version-kind:
-    - group: "management.cattle.io"
-      version: "v2"
-      kind: "Schemaless"
-  io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta:
-    description: "Object Metadata"
-    properties:
-      annotations:
-        description: "annotations of the resource"
-        type: "object"
-        additionalProperties:
-          type: "string"
-      name:
-        description: "name of the resource"
-        type: "string"
-`
-)
-
-func getSchemalessCRDs() ([]*apiextv1.CustomResourceDefinition, error) {
-	crds, err := yaml.UnmarshalWithJSONDecoder[*apiextv1.CustomResourceDefinition](bytes.NewBuffer([]byte(rawSchemalessCRDs)))
-	if err != nil {
-		return nil, fmt.Errorf("unmarshaling raw CRDs: %w", err)
-	}
-	for _, crd := range crds {
-		for _, crdVersion := range crd.Spec.Versions {
-			crdVersion.Schema = nil
-		}
-	}
-	return crds, err
-}
-
-func getSchemalessModels() (proto.Models, error) {
-	doc, err := openapi_v2.ParseDocument([]byte(rawSchemalessModels))
-	if err != nil {
-		return nil, fmt.Errorf("unmarshaling raw models: %w", err)
-	}
-	models, err := proto.NewOpenAPIData(doc)
-	return models, err
-}

pkg/schema/definitions/handler.go

@@ -225,8 +225,6 @@ func listGVKModels(models proto.Models, groups *metav1.APIGroupList, crdCache wa
 			}
 			if version.Schema != nil {
 				gvkToCRD[gvk] = version.Schema.OpenAPIV3Schema
-			} else {
-				gvkToCRD[gvk] = nil
 			}
 		}
 	}

pkg/schema/definitions/handler_test.go

@@ -34,7 +34,7 @@ func TestRefresh(t *testing.T) {
 	require.NotNil(t, userAttributesV2)
 
 	nullableV2 := getJSONSchema(crds, "nullable.management.cattle.io", "v2")
-	require.NotNil(t, userAttributesV2)
+	require.NotNil(t, nullableV2)
 
 	tests := []struct {
 		name             string
@@ -85,6 +85,11 @@ func TestRefresh(t *testing.T) {
 					Schema:    defaultModels.LookupModel("io.cattle.management.v2.Nullable"),
 					CRD:       nullableV2,
 				},
+				"management.cattle.io.schemaless": {
+					ModelName: "io.cattle.management.v2.Schemaless",
+					Schema:    defaultModels.LookupModel("io.cattle.management.v2.Schemaless"),
+					CRD:       nil,
+				},
 			},
 		},
 		{
@@ -147,6 +152,11 @@ func TestRefresh(t *testing.T) {
 					Schema:    defaultModels.LookupModel("io.cattle.management.v2.Nullable"),
 					CRD:       nullableV2,
 				},
+				"management.cattle.io.schemaless": {
+					ModelName: "io.cattle.management.v2.Schemaless",
+					Schema:    defaultModels.LookupModel("io.cattle.management.v2.Schemaless"),
+					CRD:       nil,
+				},
 			},
 		},
 	}
@@ -196,59 +206,6 @@ func TestRefresh(t *testing.T) {
 	}
 }
 
-func TestRefreshSchemalessCRDs(t *testing.T) {
-	schemalessModels, err := getSchemalessModels()
-	require.NoError(t, err)
-
-	crds, err := getSchemalessCRDs()
-	require.NoError(t, err)
-
-	for _, crd := range crds {
-		for _, crdVersion := range crd.Spec.Versions {
-			crdVersion.Schema = nil
-		}
-	}
-
-	test := struct {
-		name          string
-		nilGroups     bool
-		wantModels    proto.Models
-		wantGVKModels map[string]gvkModel
-	}{
-		name:       "problem - missing schema",
-		wantModels: schemalessModels,
-		wantGVKModels: map[string]gvkModel{
-			"management.cattle.io.schemaless": {
-				ModelName: "io.cattle.management.v2.schemaless",
-			},
-		},
-	}
-	t.Run(test.name, func(t *testing.T) {
-		client, err := buildDefaultServerlessDiscovery()
-		require.Nil(t, err)
-		baseSchemas := types.EmptyAPISchemas()
-
-		ctrl := gomock.NewController(t)
-		defer ctrl.Finish()
-
-		crdCache := fake.NewMockNonNamespacedCacheInterface[*apiextv1.CustomResourceDefinition](ctrl)
-		crdCache.EXPECT().List(labels.Everything()).Return(crds, nil).AnyTimes()
-
-		handler := NewSchemaDefinitionHandler(baseSchemas, crdCache, client)
-		err = handler.Refresh()
-		require.NoError(t, err)
-
-		handler.lock.RLock()
-		defer handler.lock.RUnlock()
-		require.Equal(t, test.wantModels, handler.models)
-		// Just test the model names, because the schema and crd will be null in the input
-		require.Equal(t, len(test.wantGVKModels), len(handler.gvkModels))
-		for k, v := range test.wantGVKModels {
-			require.Equal(t, v.ModelName, handler.gvkModels[k].ModelName)
-		}
-	})
-}
-
 func Test_byID(t *testing.T) {
 	discoveryClient, err := buildDefaultDiscovery()
 	require.NoError(t, err)
@@ -788,65 +745,6 @@ func buildDefaultDiscovery() (*fakeDiscovery, error) {
 	}, nil
 }
 
-func buildDefaultServerlessDiscovery() (*fakeDiscovery, error) {
-	document, err := openapi_v2.ParseDocument([]byte(rawSchemalessModels))
-	if err != nil {
-		return nil, fmt.Errorf("unable to parse rawSchemalessModels: %w", err)
-	}
-	groups := []metav1.APIGroup{
-		// The core groups (eg: Pods, ConfigMaps, etc)
-		{
-			Name: "",
-			PreferredVersion: metav1.GroupVersionForDiscovery{
-				GroupVersion: "v1",
-				Version:      "v1",
-			},
-			Versions: []metav1.GroupVersionForDiscovery{
-				{
-					GroupVersion: "v1",
-					Version:      "v1",
-				},
-			},
-		},
-		{
-			Name: "management.cattle.io",
-			PreferredVersion: metav1.GroupVersionForDiscovery{
-				GroupVersion: "management.cattle.io/v2",
-				Version:      "v2",
-			},
-			Versions: []metav1.GroupVersionForDiscovery{
-				{
-					GroupVersion: "management.cattle.io/v1",
-					Version:      "v1",
-				},
-				{
-					GroupVersion: "management.cattle.io/v2",
-					Version:      "v2",
-				},
-			},
-		},
-		{
-			Name: "noversion.cattle.io",
-			Versions: []metav1.GroupVersionForDiscovery{
-				{
-					GroupVersion: "noversion.cattle.io/v1",
-					Version:      "v1",
-				},
-				{
-					GroupVersion: "noversion.cattle.io/v2",
-					Version:      "v2",
-				},
-			},
-		},
-	}
-	return &fakeDiscovery{
-		Groups: &metav1.APIGroupList{
-			Groups: groups,
-		},
-		Document: document,
-	}, nil
-}
-
 type fakeDiscovery struct {
 	Groups      *metav1.APIGroupList
 	Document    *openapi_v2.Document