Fix schema definitions #215

tomleb · 2024-05-16T00:01:49Z

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.projects will provide a schema definition for the resource Projects in the management.cattle.io group.

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):

# Get the whole thing
$ kubectl get --raw /openapi/v2

# Filter for a specific resource (in this case, ConfigMap)
$ kubectl get --raw /openapi/v2 | jq '.definitions."io.k8s.api.core.v1.ConfigMap"'

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: true in V3 is not representable in V2.

Here's an example for the Cluster object in provisioning.cattle.io/v1 group.

OpenAPI V2

$ kubectl get --raw /openapi/v2 | jq '.definitions."io.cattle.provisioning.v1.Cluster"'
{
  "type": "object",
  "properties": {
    ...
    "spec": {
      "type": "object",
      "properties": {
        ...
        "rkeConfig": {}         <---- Notice the empty object here, this is wrong!
      }
    }
  },
  ...
}

OpenAPI V3

$ kubectl get --raw /openapi/v3/apis/provisioning.cattle.io/v1 | jq '.components.schemas."io.cattle.provisioning.v1.Cluster"'
{
  "type": "object",
  "properties": {
    "spec": {
      "type": "object",
      "properties": {
        "rkeConfig": {
          "type": "object",
          "nullable": true,
          "properties": {
             (The fields are actually defined here)
          }
        }
      }
    },
  }
}

CRD that is NOT a `proto.Kind`

We'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 usual metadata, apiVersion and kind fields. We can see the difference again between OpenAPI V2 and OpenAPI V3:

CRD

The kind, apiVersion and metadata fields are not set here.

apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
  name: userattributes.management.cattle.io
spec:
  ...
  versions:
  - name: v3
    schema:
      openAPIV3Schema:
        type: object
        x-kubernetes-preserve-unknown-fields: true

OpenAPI V2

The kind, apiVersion and metadata fields are not set here.

$ kubectl get --raw /openapi/v2 | jq '.definitions."io.cattle.management.v3.UserAttribute"'
{
  "type": "object",                                                              <----- Notice no fields injected here
  "x-kubernetes-group-version-kind": [
    {
      "group": "management.cattle.io",
      "kind": "UserAttribute",
      "version": "v3"
    }
  ]
}

OpenAPI V3

The kind, apiVersion and metadata fields are set here (automagically added by the API server).

$ kubectl get --raw /openapi/v3/apis/management.cattle.io/v3 | jq '.components.schemas."io.cattle.management.v3.UserAttribute"'
{
  "type": "object",
  "properties": {
    "apiVersion": {
      "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",
      "type": "string"
    },
    "kind": {
      "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",
      "type": "string"
    },
    "metadata": {
      "description": "Standard object's metadata. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#metadata",
      "allOf": [
        {
          "$ref": "#/components/schemas/io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta"
        }
      ]
    }
  }
  ..
}

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:

byHandler function now does the following:

If the schema is from a BaseSchema (a schema that doens't exist in Kubernetes), then build the schema definition for it and return early. Otherwise:
Build a schema definition from the OpenAPI V2 info.
If the schema is also a CRD (remember, built-in resources like Pods, Deployments, etc aren't CRDs), build a schema definition from it
Merge both schema definitions from the OpenAPI V2 and CRD. CRD will ALWAYS override whatever is in OpenAPI V2. This makes sense because CRD is defined by OpenAPI V3, so has more information.

This fixes the rkeConfig case.

Inject the apiVersion, kind and metadata fields 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, kind and metadata fields, specifically for the UserAttribute object.

Treat a *proto.Map with a GVK extension as a *proto.Kind. So now we can reuse the visitor code to correctly handle the UserAttribute object.

pkg/schema/definitions/handler.go

tomleb · 2024-05-16T15:30:08Z

pkg/schema/definitions/handler_test.go

 	}
 }

-func Test_byID(t *testing.T) {
-	defaultDocument, err := openapi_v2.ParseDocument([]byte(openapi_raw))
+func TestRefresh(t *testing.T) {


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.

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.

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.

pkg/schema/definitions/converter.go

MbolotSuse

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/converter/k8stonorman.go

pkg/schema/definitions/fixtures_test.go

pkg/schema/definitions/handler.go

MbolotSuse · 2024-05-20T14:10:09Z

pkg/schema/definitions/schema.go

+
+// mergeSchemaDefinitions the given schemaDefinitions in order, overriding values
+// when they are set.
+func mergeSchemaDefinitions(schemas ...schemaDefinition) schemaDefinition {


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.

Moved it to be a method on the schemaDefinition struct. It also no longer accepts many schemaDefinition since that's not really needed.

MbolotSuse · 2024-05-20T14:12:16Z

pkg/schema/definitions/schema.go

+		return merged
+	}
+
+	merged.DefinitionType = schemas[0].DefinitionType


Correct me if I am wrong, but it seems like we could makemerged simply be schemas[0] rather than selecting the definition type, right?

Yeah, the new .Merge method just does an inplace merge

pkg/schema/definitions/converter.go

MbolotSuse · 2024-05-20T19:17:24Z

pkg/schema/definitions/converter.go

+}
+
+func convertJSONSchemaPropsToDefinition(props apiextv1.JSONSchemaProps, path proto.Path, definitions map[string]definition) {
+	if props.Type != "array" && props.Type != "object" {


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?

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/

pkg/schema/definitions/converter.go

pkg/schema/definitions/fixtures_test.go

MbolotSuse · 2024-05-20T19:30:19Z

pkg/schema/definitions/handler_test.go

 	}
 }

-func Test_byID(t *testing.T) {
-	defaultDocument, err := openapi_v2.ParseDocument([]byte(openapi_raw))
+func TestRefresh(t *testing.T) {


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.

tomleb · 2024-05-28T16:00:03Z

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:

Tests for crdToDefinition
More test for CRD
Need to find more information on handling SchemaOrArray
Need to do something about weird union of merge
EDIT: And revert and add more tests for Refresh()

MbolotSuse

Overall, mostly nits and some test cases to add.

MbolotSuse · 2024-06-12T19:10:47Z

pkg/schema/definitions/schema.go

+	// 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


Please remove the TODO.

MbolotSuse · 2024-06-12T19:13:54Z

pkg/schema/definitions/schema.go

+// mergeSchemaDefinitions the given schemaDefinitions in order, overriding values
+// when they are set.


Nit:

Suggested change

// 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.

MbolotSuse · 2024-06-12T19:33:35Z

pkg/schema/definitions/schema_test.go

+func TestSchemaDefinitionMerge(t *testing.T) {
+	tests := []struct {
+		name     string
+		schemas  [2]schemaDefinition


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.

MbolotSuse · 2024-06-12T19:38:18Z

pkg/schema/definitions/schema_test.go

+			},
+		},
+		{
+			name: "empty definition type",


Nit: I think we also want a case where there is a definition type, but it's not the same between the two schemas.

MbolotSuse · 2024-06-12T19:40:23Z

pkg/schema/definitions/handler.go

+	// 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


Looks like the comment here was removed by accident, can you re-add it?

pkg/schema/definitions/converter.go

MbolotSuse · 2024-06-12T20:27:01Z

pkg/schema/definitions/converter.go

+				field.SubType = getPrimitiveType(item.Type)
+			}
+		}
+	case "object":


From the swagger docs it might be possible for us to identify some of these objects as maps.

MbolotSuse · 2024-06-12T20:30:13Z

pkg/schema/definitions/handler_test.go

-			name:          "not refreshed",
-			schemaName:    "management.cattle.io.globalrole",
-			wantError:     true,
-			wantErrorCode: intPtr(503),
-		},


This tests seems to have been removed. I think the case is still valid.

MbolotSuse · 2024-06-12T20:54:02Z

pkg/schema/definitions/handler_test.go

 			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) {


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

MbolotSuse · 2024-06-12T21:08:44Z

pkg/schema/definitions/converter_test.go

+	apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+)
+
+func TestCRDToDefinition(t *testing.T) {


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.

pkg/schema/definitions/converter.go

nflynt

This looks good! This is simultaneously a metric ton of tests, and not quite enough tests, but I'm not going to block the PR on that; The happy path is covered decently enough, and we can always add more tests as future work.

tomleb force-pushed the fix-schema-definitions branch 9 times, most recently from 5468cbf to daa89b4 Compare May 16, 2024 15:25

tomleb commented May 16, 2024

View reviewed changes

pkg/schema/definitions/handler.go Outdated Show resolved Hide resolved

tomleb commented May 16, 2024

View reviewed changes

tomleb force-pushed the fix-schema-definitions branch from daa89b4 to e4fcb11 Compare May 16, 2024 15:31

tomleb marked this pull request as ready for review May 16, 2024 15:37

tomleb requested a review from a team as a code owner May 16, 2024 15:37

tomleb commented May 17, 2024

View reviewed changes

pkg/schema/definitions/converter.go Outdated Show resolved Hide resolved

MbolotSuse self-requested a review May 20, 2024 13:36

MbolotSuse requested changes May 20, 2024

View reviewed changes

tomleb force-pushed the fix-schema-definitions branch 6 times, most recently from fb97cb9 to 7b8e6e8 Compare May 28, 2024 14:47

tomleb force-pushed the fix-schema-definitions branch 2 times, most recently from 00e3fca to 85632ec Compare May 28, 2024 15:50

tomleb force-pushed the fix-schema-definitions branch 2 times, most recently from b03b163 to ca9ad29 Compare June 12, 2024 16:26

tomleb requested a review from MbolotSuse June 12, 2024 16:41

MbolotSuse requested changes Jun 12, 2024

View reviewed changes

MbolotSuse requested a review from nflynt June 12, 2024 21:21

tomleb force-pushed the fix-schema-definitions branch 2 times, most recently from 118285e to 93ac195 Compare June 13, 2024 19:26

nflynt reviewed Jun 13, 2024

View reviewed changes

pkg/schema/definitions/converter.go Outdated Show resolved Hide resolved

Fix missing fields in schema definition endpoint

0de5f6f

tomleb force-pushed the fix-schema-definitions branch from 93ac195 to 0de5f6f Compare June 14, 2024 13:15

MbolotSuse approved these changes Jun 14, 2024

View reviewed changes

nflynt approved these changes Jun 17, 2024

View reviewed changes

tomleb merged commit 9ac9be9 into rancher:master Jun 17, 2024
1 check passed

tomleb deleted the fix-schema-definitions branch June 17, 2024 14:52

tomleb mentioned this pull request Jun 17, 2024

Backfill Tests for Steve rancher/rancher#45247

Open

MbolotSuse mentioned this pull request Jun 27, 2024

Bumping steve rancher/rancher#45928

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix schema definitions #215

Fix schema definitions #215

tomleb commented May 16, 2024 •

edited

Loading

tomleb May 16, 2024

MbolotSuse May 20, 2024

tomleb May 27, 2024

MbolotSuse left a comment

MbolotSuse May 20, 2024

tomleb May 28, 2024

MbolotSuse May 20, 2024

tomleb May 28, 2024

MbolotSuse May 20, 2024

tomleb May 27, 2024

MbolotSuse May 20, 2024

tomleb commented May 28, 2024 •

edited

Loading

MbolotSuse left a comment

MbolotSuse Jun 12, 2024

MbolotSuse Jun 12, 2024

MbolotSuse Jun 12, 2024

MbolotSuse Jun 12, 2024

MbolotSuse Jun 12, 2024

MbolotSuse Jun 12, 2024

MbolotSuse Jun 12, 2024

MbolotSuse Jun 12, 2024

MbolotSuse Jun 12, 2024

nflynt left a comment

		// mergeSchemaDefinitions the given schemaDefinitions in order, overriding values
		// when they are set.

Fix schema definitions #215

Fix schema definitions #215

Conversation

tomleb commented May 16, 2024 • edited Loading

Issue rancher/rancher#45157

Background

The problems

Unrepresentable fields in OpenAPI V2

CRD that is NOT a proto.Kind

The fix

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MbolotSuse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomleb commented May 28, 2024 • edited Loading

MbolotSuse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nflynt left a comment

Choose a reason for hiding this comment

tomleb commented May 16, 2024 •

edited

Loading

CRD that is NOT a `proto.Kind`

tomleb commented May 28, 2024 •

edited

Loading