From b7618463e6e8d0027c563c04879b53b6110c2f97 Mon Sep 17 00:00:00 2001 From: Michael Bolot Date: Wed, 28 Feb 2024 14:39:05 -0600 Subject: [PATCH] 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 | 64 ++++-------- pkg/schema/definitions/handler_test.go | 131 +++++++++++-------------- pkg/schema/definitions/openapi_test.go | 122 +++++++++++++++++++++++ 3 files changed, 197 insertions(+), 120 deletions(-) diff --git a/pkg/schema/definitions/handler.go b/pkg/schema/definitions/handler.go index 155950d9..c54da3f9 100644 --- a/pkg/schema/definitions/handler.go +++ b/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 } diff --git a/pkg/schema/definitions/handler_test.go b/pkg/schema/definitions/handler_test.go index 1ad676a4..f7a96b7d 100644 --- a/pkg/schema/definitions/handler_test.go +++ b/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 } diff --git a/pkg/schema/definitions/openapi_test.go b/pkg/schema/definitions/openapi_test.go index a0dad44a..1ab4fac3 100644 --- a/pkg/schema/definitions/openapi_test.go +++ b/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"