Fix schema definitions #215
Conversation
5468cbf
to
daa89b4
Compare
| } | ||
| } | ||
|
|
||
| func Test_byID(t *testing.T) { | ||
| defaultDocument, err := openapi_v2.ParseDocument([]byte(openapi_raw)) | ||
| func TestRefresh(t *testing.T) { |
There was a problem hiding this comment.
The previous test code was using private fields of the handler to change the behavior. This made it difficult for me when I was working on this since initially I changed the implementation quite a bit. (eg: the models/schemaToModel fields were no longer in the handler).
Those fields are now back in the handler so I could revert the change here, but I felt this way also worked fine. Let me know what you think.
There was a problem hiding this comment.
A couple of things to keep in mind:
- It's ok to refactor tests if the old way was making it hard to test
- It's best to avoid refactoring tests if you are also changing the production code. In this case, since you changed the prod code (not too much on this specific function), I'd personally say you should avoid refactoring the test.
- You can't drop coverage if you refactor the tests. When checking the coverage there's a few error cases of the refresh function you no longer seem to cover.
There was a problem hiding this comment.
Agreed, once I have a design I'm okay with code wise I'll revisit this. I'll probably be reverting the changes to the test.
daa89b4
to
e4fcb11
Compare
There was a problem hiding this comment.
Overall comments:
- I'd like to see if we can share as much logic as possible between the v2 approach and the CRDs. I think this will be easier to maintain if we can do that.
- Overall, there's some tests that would be good to add.
- Some minor code organization things to improve.
- There's a changed return code - that will be a problem. I think this will need to be changed back.
pkg/schema/definitions/schema.go
Outdated
|
|
||
| // mergeSchemaDefinitions the given schemaDefinitions in order, overriding values | ||
| // when they are set. | ||
| func mergeSchemaDefinitions(schemas ...schemaDefinition) schemaDefinition { |
There was a problem hiding this comment.
Nit: I don't think that this function belongs in this file. It's not used anywhere in here. It seems like something that should go in the handler, since the handler is the only place that needs to do this.
There are a few other options here (like having this be a method on the schemaDefinition struct, or having a separate types.go file) but I think the current place isn't optimal in any other place.
There was a problem hiding this comment.
Moved it to be a method on the schemaDefinition struct. It also no longer accepts many schemaDefinition since that's not really needed.
pkg/schema/definitions/schema.go
Outdated
| return merged | ||
| } | ||
|
|
||
| merged.DefinitionType = schemas[0].DefinitionType |
There was a problem hiding this comment.
Correct me if I am wrong, but it seems like we could makemerged simply be schemas[0] rather than selecting the definition type, right?
There was a problem hiding this comment.
Yeah, the new .Merge method just does an inplace merge
pkg/schema/definitions/converter.go
Outdated
| } | ||
|
|
||
| func convertJSONSchemaPropsToDefinition(props apiextv1.JSONSchemaProps, path proto.Path, definitions map[string]definition) { | ||
| if props.Type != "array" && props.Type != "object" { |
There was a problem hiding this comment.
Here and elsewhere we seem to only cover the following types:
- array
- object
- string
- boolean
- integer
- number
How do we know that these are the only types?
There was a problem hiding this comment.
These are the only types defined by the OpenAPI V3 spec. I have added a comment that describes the available types and where they come from.
// This supports all OpenAPI V3 types: boolean, number, integer, string, object and array
// as defined here: https://swagger.io/specification/v3/| } | ||
| } | ||
|
|
||
| func Test_byID(t *testing.T) { | ||
| defaultDocument, err := openapi_v2.ParseDocument([]byte(openapi_raw)) | ||
| func TestRefresh(t *testing.T) { |
There was a problem hiding this comment.
A couple of things to keep in mind:
- It's ok to refactor tests if the old way was making it hard to test
- It's best to avoid refactoring tests if you are also changing the production code. In this case, since you changed the prod code (not too much on this specific function), I'd personally say you should avoid refactoring the test.
- You can't drop coverage if you refactor the tests. When checking the coverage there's a few error cases of the refresh function you no longer seem to cover.
fb97cb9
to
7b8e6e8
Compare
00e3fca
to
85632ec
Compare
|
Made some changes. The refresh does most of the job now since we have to do OpenAPI requests and merge. The byHandler handler simply returns the latest computed merged result. What's missing:
|
b03b163
to
ca9ad29
Compare
pkg/schema/definitions/schema.go
Outdated
| // TODO: What do we mean by required? OpenAPI has both concept: | ||
| // - required: The field must be present, whatever its value is (eg: null would be a valid value) | ||
| // - nullable: The field can be null |
There was a problem hiding this comment.
Please remove the TODO.
pkg/schema/definitions/schema.go
Outdated
| // mergeSchemaDefinitions the given schemaDefinitions in order, overriding values | ||
| // when they are set. |
There was a problem hiding this comment.
Nit:
| // mergeSchemaDefinitions the given schemaDefinitions in order, overriding values | |
| // when they are set. | |
| // Merge merges the provided schema into s. All conflicting values (i.e. that are in both schema and s) | |
| // are replaced with the values from s. |
| func TestSchemaDefinitionMerge(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| schemas [2]schemaDefinition |
There was a problem hiding this comment.
Nit: Since we only ever can merge two schemas, and the direction of merge is important (since it determines which values are overridden), I think separating these out into two params makes sense.
| }, | ||
| }, | ||
| { | ||
| name: "empty definition type", |
There was a problem hiding this comment.
Nit: I think we also want a case where there is a definition type, but it's not the same between the two schemas.
pkg/schema/definitions/handler.go
Outdated
| // models that has a GVK. The schemaDefinition is the result of merging | ||
| // OpenAPI V2 information with the CRD information (if any). | ||
| gvkModels map[string]gvkModel | ||
| models proto.Models |
There was a problem hiding this comment.
Looks like the comment here was removed by accident, can you re-add it?
pkg/schema/definitions/converter.go
Outdated
| field.SubType = getPrimitiveType(item.Type) | ||
| } | ||
| } | ||
| case "object": |
There was a problem hiding this comment.
From the swagger docs it might be possible for us to identify some of these objects as maps.
| name: "not refreshed", | ||
| schemaName: "management.cattle.io.globalrole", | ||
| wantError: true, | ||
| wantErrorCode: intPtr(503), | ||
| }, |
There was a problem hiding this comment.
This tests seems to have been removed. I think the case is still valid.
| require.Equal(t, test.wantModels, handler.models) | ||
| require.Equal(t, test.wantSchemaToModel, handler.schemaToModel) | ||
| require.Equal(t, test.wantGVKModels, handler.gvkModels) | ||
| }) | ||
|
|
||
| } | ||
| } | ||
|
|
||
| func Test_byID(t *testing.T) { |
There was a problem hiding this comment.
Some other tests that would be good to add:
- Error when trying to build definition for model
- Error when trying to construct opnapiV2 definition
- Error when trying to merge definitions
| apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" | ||
| ) | ||
|
|
||
| func TestCRDToDefinition(t *testing.T) { |
There was a problem hiding this comment.
Some other cases to cover:
- Array of type arrray/object
- Required field
- No items schema in the array
- For an array props.Items.JSONSchemas has some values.
118285e
to
93ac195
Compare
93ac195
to
0de5f6f
Compare
Issue rancher/rancher#45157
Background
The schema definition endpoint in steve (
/v1/schemaDefinitions/<schema ID>) provide a schema definition for a Kubernetes resource. The UI can then use this definition for whatever it wants. For example, the endpoint/v1/schemaDefinitions/management.cattle.io.projectswill provide a schema definition for the resourceProjectsin themanagement.cattle.iogroup.Here's an example schema definition for that object (trimmed because it's a big output):
{ "id": "management.cattle.io.projects", "type": "schemaDefinition", "links": { "self": "https://schema-def.cattle.tomlebreux.com/v1/schemaDefinitions/management.cattle.io.projects" }, "definitionType": "io.cattle.management.v3.Project", "definitions": { "io.cattle.management.v3.Project": { "resourceFields": { "apiVersion": { "type": "string", "description": "APIVersion defines the versioned schema of this representation of an object. Servers should convert recognized schemas to the latest internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources" }, "kind": { "type": "string", "description": "Kind is a string value representing the REST resource this object represents. Servers may infer this from the endpoint the client submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds" }, "metadata": { "type": "io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta", "description": "Standard object's metadata. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#metadata" }, "spec": { "type": "io.cattle.management.v3.Project.spec", "description": "Spec is the specification of the desired configuration for the project." }, "status": { "type": "io.cattle.management.v3.Project.status", "description": "Status is the most recently observed status of the project." } }, "type": "io.cattle.management.v3.Project", "description": "Project is a group of namespaces. Projects are used to create a multi-tenant environment within a Kubernetes cluster by managing namespace operations, such as role assignments or quotas, as a group." }, "io.cattle.management.v3.Project.spec": { "resourceFields": { "clusterName": { "type": "string", "description": "ClusterName is the name of the cluster the project belongs to. Immutable.", "required": true } }, "type": "io.cattle.management.v3.Project.spec", "description": "Spec is the specification of the desired configuration for the project." }, "io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta": { "name": { "type": "string", "description": "Name must be unique within a namespace. Is required when creating resources, although some resources may allow a client to request the generation of an appropriate name automatically. Name is primarily intended for creation idempotence and configuration definition. Cannot be updated. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names#names" } }, "type": "io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta", "description": "ObjectMeta is metadata that all persisted resources must have, which includes all objects users must create." } } }It's essentially flattening the Kubernetes resource structure so that each nested objects becomes a top-level object in the schema.
We obtain this information by polling the OpenAPI V2 endpoint exposed by the Kubernetes API server. You can have a look at it yourself by doing the following command (warning: very long output):
The problems
Unrepresentable fields in OpenAPI V2
CustomResourceDefinition objects (CRDs) are defined by an OpenAPI V3 schema. There are major differences between V2 and V3, such that some fields representable by V3 are not representable by V2. For example, a field will
nullable: truein V3 is not representable in V2.Here's an example for the
Clusterobject inprovisioning.cattle.io/v1group.OpenAPI V2
OpenAPI V3
CRD that is NOT a
proto.KindWe're parsing the OpenAPI V2 schemas of some objects. The parsed document is represented by the interface proto.Schema, with the top-level object generally being a *proto.Kind.
For some odd reason, the UserAttribute object is seen as a *proto.Map instead. The previous code assumed we would get a Kind, so the UserAttribute object returns an empty object.
In addition, it seems like because this is treated as a
proto.Map, the API server does not automatically inject the usualmetadata,apiVersionandkindfields. We can see the difference again between OpenAPI V2 and OpenAPI V3:CRD
The
kind,apiVersionandmetadatafields are not set here.OpenAPI V2
The
kind,apiVersionandmetadatafields are not set here.OpenAPI V3
The
kind,apiVersionandmetadatafields are set here (automagically added by the API server).The fix
It's pretty clear that OpenAPI V2 lacks information, so we need to add another source. We could use OpenAPI V3, but we opted not too because it wasn't clear what minimal version of Kubernetes we were tackling for the release (OpenAPI V3 is GA only in k8s 1.27). So for now, we're going to use the CRD definition instead (which is technically OpenAPI V3, but at least it's GA already).
The high-level view of the changes are the following:
byHandlerfunction now does the following:This fixes the rkeConfig case.
apiVersion,kindandmetadatafields in the OpenAPI V2 definition builder. To do this, we take those fields from a known object (ConfigMap) and just inject them as fields. To avoid those fields being overriden by the CRD (which ends up having less information in that case... 😕 ...), we delete those fields in the CRD definition builder. (Yes, really, lol)This fixes the case of missing
apiVersion,kindandmetadatafields, specifically for the UserAttribute object.*proto.Mapwith a GVK extension as a*proto.Kind. So now we can reuse the visitor code to correctly handle the UserAttribute object.