Fixing bug with preferred versions

In the original implementation of the definition handler, the resource
list was checked for preferred version, and this overrode the preferred
version of the overall group. However, this logic was inaccurate and
did not use the group as the source of truth on the preferred version
like it should have.

pkg/schema/definitions/handler.go

@@ -1,7 +1,6 @@
 package definitions
 
 import (
-	"errors"
 	"fmt"
 	"net/http"
 	"sync"
@@ -10,7 +9,7 @@ import (
 	"github.com/rancher/apiserver/pkg/types"
 	"github.com/rancher/steve/pkg/schema/converter"
 	"github.com/rancher/wrangler/v2/pkg/schemas/validation"
-	"k8s.io/apimachinery/pkg/runtime/schema"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/discovery"
 	"k8s.io/kube-openapi/pkg/util/proto"
 )
@@ -43,8 +42,6 @@ type SchemaDefinitionHandler struct {
 // Refresh writeLocks and updates the cache with new schemaDefinitions. Will result in a call to kubernetes to retrieve
 // the openAPI schemas.
 func (s *SchemaDefinitionHandler) Refresh() error {
-	s.Lock()
-	defer s.Unlock()
 	openapi, err := s.client.OpenAPISchema()
 	if err != nil {
 		return fmt.Errorf("unable to fetch openapi definition: %w", err)
@@ -53,16 +50,15 @@ func (s *SchemaDefinitionHandler) Refresh() error {
 	if err != nil {
 		return fmt.Errorf("unable to parse openapi definition into models: %w", err)
 	}
-	s.models = &models
-	nameIndex, err := s.indexSchemaNames(models)
-	// indexSchemaNames may successfully refresh some definitions, but still return an error
-	// in these cases, store what we could find, but still return up an error
-	if nameIndex != nil {
-		s.schemaToModel = nameIndex
-	}
+	groups, err := s.client.ServerGroups()
 	if err != nil {
-		return fmt.Errorf("unable to index schema name to model name: %w", err)
+		return fmt.Errorf("unable to retrieve groups: %w", err)
 	}
+	s.Lock()
+	defer s.Unlock()
+	nameIndex := s.indexSchemaNames(models, groups)
+	s.schemaToModel = nameIndex
+	s.models = &models
 	return nil
 }
 
@@ -113,34 +109,12 @@ func (s *SchemaDefinitionHandler) byIDHandler(request *types.APIRequest) (types.
 }
 
 // indexSchemaNames returns a map of schemaID to the modelName for a given schema. Will use the preferred version of a
-// resource if possible. May return a map and an error if it was able to index some schemas but not others.
-func (s *SchemaDefinitionHandler) indexSchemaNames(models proto.Models) (map[string]string, error) {
-	_, resourceLists, err := s.client.ServerGroupsAndResources()
-	// this may occasionally fail to discover certain groups, but we still can refresh the others in those cases
-	if _, ok := err.(*discovery.ErrGroupDiscoveryFailed); err != nil && !ok {
-		return nil, fmt.Errorf("unable to retrieve groups and resources: %w", err)
-	}
-	preferredResourceVersions := map[schema.GroupKind]string{}
-	for _, resourceList := range resourceLists {
-		if resourceList == nil {
-			continue
-		}
-		groupVersion, gvErr := schema.ParseGroupVersion(resourceList.GroupVersion)
-		// we may fail to parse the GV of one group, but can still parse out the others
-		if gvErr != nil {
-			err = errors.Join(err, fmt.Errorf("unable to parse group version %s: %w", resourceList.GroupVersion, gvErr))
-			continue
-		}
-		for _, resource := range resourceList.APIResources {
-			gk := schema.GroupKind{
-				Group: groupVersion.Group,
-				Kind:  resource.Kind,
-			}
-			// per the resource docs, if the resource.Version is empty, the preferred version for
-			// this resource is the version of the APIResourceList it is in
-			if resource.Version == "" || resource.Version == groupVersion.Version {
-				preferredResourceVersions[gk] = groupVersion.Version
-			}
+// resource if possible. Can return an error if unable to find groups.
+func (s *SchemaDefinitionHandler) indexSchemaNames(models proto.Models, groups *metav1.APIGroupList) map[string]string {
+	preferredResourceVersions := map[string]string{}
+	if groups != nil {
+		for _, group := range groups.Groups {
+			preferredResourceVersions[group.Name] = group.PreferredVersion.Version
 		}
 	}
 	schemaToModel := map[string]string{}
@@ -156,17 +130,13 @@ func (s *SchemaDefinitionHandler) indexSchemaNames(models proto.Models) (map[str
 			// we can safely continue
 			continue
 		}
-		gk := schema.GroupKind{
-			Group: gvk.Group,
-			Kind:  gvk.Kind,
-		}
-		prefVersion, ok := preferredResourceVersions[gk]
+		prefVersion := preferredResourceVersions[gvk.Group]
 		// if we don't have a known preferred version for this group or we are the preferred version
 		// add this as the model name for the schema
-		if !ok || prefVersion == gvk.Version {
+		if prefVersion == "" || prefVersion == gvk.Version {
 			schemaID := converter.GVKToSchemaID(*gvk)
 			schemaToModel[schemaID] = modelName
 		}
 	}
-	return schemaToModel, err
+	return schemaToModel
 }

pkg/schema/definitions/handler_test.go

@@ -10,7 +10,6 @@ import (
 	wschemas "github.com/rancher/wrangler/v2/pkg/schemas"
 	"github.com/stretchr/testify/require"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/version"
 	"k8s.io/client-go/discovery"
 	"k8s.io/client-go/openapi"
@@ -24,17 +23,19 @@ func TestRefresh(t *testing.T) {
 	defaultModels, err := proto.NewOpenAPIData(defaultDocument)
 	require.NoError(t, err)
 	defaultSchemaToModel := map[string]string{
-		"management.cattle.io.globalrole": "io.cattle.management.v2.GlobalRole",
+		"management.cattle.io.globalrole": "io.cattle.management.v1.GlobalRole",
+		"noversion.cattle.io.resource":    "io.cattle.noversion.v2.Resource",
+		"missinggroup.cattle.io.resource": "io.cattle.missinggroup.v2.Resource",
 	}
 	tests := []struct {
-		name                     string
-		openapiError             error
-		serverGroupsResourcesErr error
-		useBadOpenApiDoc         bool
-		unparseableGV            bool
-		wantModels               *proto.Models
-		wantSchemaToModel        map[string]string
-		wantError                bool
+		name              string
+		openapiError      error
+		serverGroupsErr   error
+		useBadOpenApiDoc  bool
+		nilGroups         bool
+		wantModels        *proto.Models
+		wantSchemaToModel map[string]string
+		wantError         bool
 	}{
 		{
 			name:              "success",
@@ -52,31 +53,19 @@ func TestRefresh(t *testing.T) {
 			wantError:        true,
 		},
 		{
-			name:                     "error - unable to retrieve groups and resources",
-			serverGroupsResourcesErr: fmt.Errorf("server not available"),
-			wantModels:               &defaultModels,
-			wantError:                true,
+			name:            "error - unable to retrieve groups and resources",
+			serverGroupsErr: fmt.Errorf("server not available"),
+			wantError:       true,
 		},
 		{
-			name: "error - unable to retrieve all groups and resources",
-			serverGroupsResourcesErr: &discovery.ErrGroupDiscoveryFailed{
-				Groups: map[schema.GroupVersion]error{
-					{
-						Group:   "other.cattle.io",
-						Version: "v1",
-					}: fmt.Errorf("some group error"),
-				},
+			name:       "no groups or error from server",
+			nilGroups:  true,
+			wantModels: &defaultModels,
+			wantSchemaToModel: map[string]string{
+				"management.cattle.io.globalrole": "io.cattle.management.v2.GlobalRole",
+				"noversion.cattle.io.resource":    "io.cattle.noversion.v2.Resource",
+				"missinggroup.cattle.io.resource": "io.cattle.missinggroup.v2.Resource",
 			},
-			wantModels:        &defaultModels,
-			wantSchemaToModel: defaultSchemaToModel,
-			wantError:         true,
-		},
-		{
-			name:              "error - unparesable gv",
-			unparseableGV:     true,
-			wantModels:        &defaultModels,
-			wantSchemaToModel: defaultSchemaToModel,
-			wantError:         true,
 		},
 	}
 	for _, test := range tests {
@@ -85,17 +74,15 @@ func TestRefresh(t *testing.T) {
 			t.Parallel()
 			client, err := buildDefaultDiscovery()
 			client.DocumentErr = test.openapiError
-			client.GroupResourcesErr = test.serverGroupsResourcesErr
+			client.GroupsErr = test.serverGroupsErr
 			if test.useBadOpenApiDoc {
 				schema := client.Document.Definitions.AdditionalProperties[0]
 				schema.Value.Type = &openapi_v2.TypeItem{
 					Value: []string{"multiple", "entries"},
 				}
 			}
-			if test.unparseableGV {
-				client.Resources = append(client.Resources, &metav1.APIResourceList{
-					GroupVersion: "not/parse/able",
-				})
+			if test.nilGroups {
+				client.Groups = nil
 			}
 			require.Nil(t, err)
 			handler := SchemaDefinitionHandler{
@@ -293,66 +280,64 @@ func buildDefaultDiscovery() (*fakeDiscovery, error) {
 	if err != nil {
 		return nil, fmt.Errorf("unable to parse openapi document %w", err)
 	}
-	groups := []*metav1.APIGroup{
+	groups := []metav1.APIGroup{
 		{
 			Name: "management.cattle.io",
 			PreferredVersion: metav1.GroupVersionForDiscovery{
-				Version: "v2",
+				GroupVersion: "management.cattle.io/v2",
+				Version:      "v1",
 			},
-		},
-	}
-	resources := []*metav1.APIResourceList{
-		{
-			GroupVersion: schema.GroupVersion{
-				Group:   "management.cattle.io",
-				Version: "v2",
-			}.String(),
-			APIResources: []metav1.APIResource{
+			Versions: []metav1.GroupVersionForDiscovery{
 				{
-					Group:   "management.cattle.io",
-					Kind:    "GlobalRole",
-					Version: "v2",
+					GroupVersion: "management.cattle.io/v1",
+					Version:      "v1",
+				},
+				{
+					GroupVersion: "management.cattle.io/v2",
+					Version:      "v2",
 				},
 			},
 		},
 		{
-			GroupVersion: schema.GroupVersion{
-				Group:   "management.cattle.io",
-				Version: "v1",
-			}.String(),
-			APIResources: []metav1.APIResource{
+			Name: "noversion.cattle.io",
+			Versions: []metav1.GroupVersionForDiscovery{
 				{
-					Group:   "management.cattle.io",
-					Kind:    "GlobalRole",
-					Version: "v2",
+					GroupVersion: "noversion.cattle.io/v1",
+					Version:      "v1",
+				},
+				{
+					GroupVersion: "noversion.cattle.io/v2",
+					Version:      "v2",
 				},
 			},
 		},
-		nil,
 	}
 	return &fakeDiscovery{
-		Groups:    groups,
-		Resources: resources,
-		Document:  document,
+		Groups: &metav1.APIGroupList{
+			Groups: groups,
+		},
+		Document: document,
 	}, nil
 }
 
 type fakeDiscovery struct {
-	Groups            []*metav1.APIGroup
-	Resources         []*metav1.APIResourceList
-	Document          *openapi_v2.Document
-	GroupResourcesErr error
-	DocumentErr       error
+	Groups      *metav1.APIGroupList
+	Document    *openapi_v2.Document
+	GroupsErr   error
+	DocumentErr error
 }
 
-// ServerGroupsAndResources is the only method we actually need for the test - just returns what is on the struct
-func (f *fakeDiscovery) ServerGroupsAndResources() ([]*metav1.APIGroup, []*metav1.APIResourceList, error) {
-	return f.Groups, f.Resources, f.GroupResourcesErr
+// ServerGroups is the only method that needs to be mocked
+func (f *fakeDiscovery) ServerGroups() (*metav1.APIGroupList, error) {
+	return f.Groups, f.GroupsErr
 }
 
 // The rest of these methods are just here to conform to discovery.DiscoveryInterface
-func (f *fakeDiscovery) RESTClient() restclient.Interface            { return nil }
-func (f *fakeDiscovery) ServerGroups() (*metav1.APIGroupList, error) { return nil, nil }
+func (f *fakeDiscovery) ServerGroupsAndResources() ([]*metav1.APIGroup, []*metav1.APIResourceList, error) {
+	return nil, nil, nil
+}
+
+func (f *fakeDiscovery) RESTClient() restclient.Interface { return nil }
 func (f *fakeDiscovery) ServerResourcesForGroupVersion(groupVersion string) (*metav1.APIResourceList, error) {
 	return nil, nil
 }

pkg/schema/definitions/openapi_test.go

@@ -82,6 +82,128 @@ definitions:
     - group: "management.cattle.io"
       version: "v2"
       kind: "GlobalRole"
+  io.cattle.noversion.v2.Resource:
+    description: "A No Version V2 resource is for a group with no preferred version"
+    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"
+          newField:
+            description: "A new field not present in v1"
+            type: "string"
+    x-kubernetes-group-version-kind:
+    - group: "noversion.cattle.io"
+      version: "v2"
+      kind: "Resource"
+  io.cattle.noversion.v1.Resource:
+    description: "A No Version V1 resource is for a group with no preferred version"
+    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: "noversion.cattle.io"
+      version: "v1"
+      kind: "Resource"
+  io.cattle.missinggroup.v2.Resource:
+    description: "A Missing Group V2 resource is for a group not listed by server groups"
+    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"
+          newField:
+            description: "A new field not present in v1"
+            type: "string"
+    x-kubernetes-group-version-kind:
+    - group: "missinggroup.cattle.io"
+      version: "v2"
+      kind: "Resource"
+  io.cattle.missinggroup.v1.Resource:
+    description: "A Missing Group V1 resource is for a group not listed by server groups"
+    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: "missinggroup.cattle.io"
+      version: "v1"
+      kind: "Resource"
   io.cattle.management.NotAKind:
     type: "string"
     description: "Some string which isn't a kind"