CRD supports multi-version Schema, Subresources and AdditionalPrintColumns #70211

roycaihw · 2018-10-25T01:25:55Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
Part of the CRD conversion webhook design: customresource-conversion-webhook.md#top-level-fields-to-per-version-fields
Main CRD conversion webhook PR: #67006
This PR is a combination of two previous separate PRs (#67553, #67521) following the updated KEP design: kubernetes/community#2737

Special notes for your reviewer:

Please review the integration test change (in a standalone commit) with whitespace change ignored.
This PR currently does ~~redundant~~ pre-creation/update-dropping-field and failing-validation for disabled feature-gated fields, please refer to the related topic in sig-api-machinery meeting (Oct 24) for more information.
Two subresources integration tests (with TODO comments) will be extended to test per-version subresources after Fix custom resource handler in-memory version #70087 fixes the unstructured conversion bug. Updated

Does this PR introduce a user-facing change?:

CRD supports multi-version Schema, Subresources and AdditionalPrintColumns (NOTE that CRDs created prior to 1.13 populated the top-level additionalPrinterColumns field by default. To apply an update that changes to per-version additionalPrinterColumns, the top-level additionalPrinterColumns field must be explicitly set to null).

/sig api-machinery
/area custom-resources
/cc @sttts @mbohlool @liggitt

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go

staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresourcedefinition/strategy.go

roycaihw · 2018-10-25T01:39:55Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/types.go

 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )

+// CustomResourceColumns masks the slice so protobuf can preserve empty slice
+// +protobuf.nullable=true


@liggitt Do we need to test the round-tripping?

same as edd5736#r228339108, we can drop this and just use []CustomResourceColumnDefinition

liggitt · 2018-10-25T14:08:11Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/types.go

@@ -20,6 +20,9 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )

+// CustomResourceColumns masks the slice so protobuf can preserve empty slice
+type CustomResourceColumns []CustomResourceColumnDefinition


remind me why we care about distinguishing nil and [] here if we didn't care for the top-level field?

The concern was brought up in #67521 (comment). My understanding is that we need to make protobuf preserve [], to allow overriding with an empty list. I didn't change the top-level field because it would be a breaking change.

rereading that thread, it looks like the feedback there was to avoid making the distinction between nil and [] (to not do this nullable typedef). I think the len(...) == 0 checks for emptiness are sufficient. Were there specific places where we needed to be able to tell nil from [] for some reason?

Thanks for the explanation :) No the apiserver doesn't need to distinguish nil and [] (and defaulting happens for both cases).

great, then let's drop this and just go with []CustomResourceColumnDefinition

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/types.go

liggitt · 2018-10-25T14:14:57Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go

+	}
+	value := versions[0].AdditionalPrinterColumns
+	for _, v := range versions {
+		if v.AdditionalPrinterColumns == nil || !reflect.DeepEqual(v.AdditionalPrinterColumns, value) {


should we check len(...)==0 instead of nil? should we use semantic equality rather than reflect.DeepEqual? (same question applies everywhere)

By semantic equality you mean for two column slices, we should consider them to be the same if they have same elements but in different order?

I mean to use apiequality.Semantic.DeepEqual(). It equates nil and empty lists, and does a few other semantic comparisons (like the absolute value of resource.Quantity fields, rather than their literal value, so 1M == 1000)

nikopen · 2018-10-25T22:19:18Z

/milestone v1.13

roycaihw · 2018-10-26T00:19:51Z

@liggitt Addressed comments and updated integration test (I had to rebase to pick up the conversion bug fix, but the first five commits didn't change). PTAL :)

liggitt · 2018-10-25T21:50:15Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/types.go

 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )

+// CustomResourceColumns masks the slice so protobuf can preserve empty slice
+// +protobuf.nullable=true


same as edd5736#r228339108, we can drop this and just use []CustomResourceColumnDefinition

liggitt · 2018-10-30T02:01:47Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/types.go

+	// Per-version schemas may not all be set to identical values (top-level validation schema should be used instead)
+	// This field is alpha-level and is only honored by servers that enable the CustomResourceWebhookConversion feature.
+	// +optional
+	Schema *JSONSchemaProps `json:"schema,omitempty" protobuf:"bytes,4,opt,name=schema"`


I just noticed this does not parallel the top-level field. what was the reason for specifying the type of schema (nested openAPIV3Schema field) in the top-level field, and why does that reason not apply for the per-version schemas?

cc @sttts

My understanding is that for the top-level field, we were planning on having a list of validation methods for CRs, so we defined a struct and made openAPIV3Schema nested.

Now it becomes clear that we are not adding new methods to the CustomResourceValidation, so we simplify the per-version schema to just JSONSchemaProps.

also cc @mbohlool @lavalamp on the related conversation: #67840

Now it becomes clear that we are not adding new methods to the CustomResourceValidation, so we simplify the per-version schema to just JSONSchemaProps.

also cc @mbohlool @lavalamp on the related conversation: #67840

#67840 actually proposes supporting a schema format other than openapi v3, which is exactly the use case for nesting openapi v3 as one of potentially multiple schema types. I think we should make the per-version schema structs in v1beta1 match the top-level structs, so that we can round-trip to whatever we end up with in v1, and have a path to transition to a different schema if that's where #67840 ends up. @lavalamp @sttts do you agree?

Yes. In the KEP we proposed dropping the nested struct and going with just openapi v3, then in #67840 we brought up the idea around replacing openapi v3. I'm happy to go with the nested struct if we think that will make it easier to support another schema format, before we merge this PR.

I am fine with keeping top level struct format with nested openapi v3. While last proposal on #67840 is to keep a more restricted version of openapi v3 but keeping the options open make more sense.

More related context: #64483

I'm going to add a commit with

Schema *CustomResourceValidation `json:"schema,omitempty" protobuf:"bytes,4,opt,name=schema"`

first, which may look like:

spec: versions: - name: v1 schema: openAPIV3Schema: ... // covers schema, defaults and static validation anotherSchemaLanguage: ... // to be added in future? prune: true

liggitt · 2018-10-30T02:47:40Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go

-		validator, _, err := apiservervalidation.NewSchemaValidator(crd.Spec.Validation)
+		validationSchema, err := getCRDSchemaForVersion(&crd.Spec, v.Name)
+		if err != nil {
+			return nil, err


will this return the full content of the CRD spec to the custom resource caller in an error? that seems very verbose and probably targeted at the wrong user... the custom resource API caller isn't necessarily the same as the CRD author. maybe call utilruntime.HandleError and return a generic error message about the subresources to the API caller?

Good point. Yes we shouldn't return the full CRD spec to a CR client when ServeHTTP. I think the same comment also applies to https://github.com/kubernetes/kubernetes/pull/70211/files/956879420560759244d2210acff668902ef20780#diff-2f720a7e61a4ddfcfe9a9e353be8784bR225.

May I ask what kind of generic error message do you have in mind? While the error can be triggered by spec with malformed subresources/schema/columns, the incoming HTTP request isn't necessarily asking for subresources-- it's called here in ServeHTTP:

kubernetes/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go

Line 207 in a49e4e1

crdInfo, err := r.getOrCreateServingInfoFor(crd)

So something like the server could not properly server the CRD subresources/schema/columns (corresponding to which field might be malformed)?

yeah, something like that would be fine

liggitt · 2018-10-30T02:48:26Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go

-
+		subresources, err := getCRDSubresourcesForVersion(&crd.Spec, v.Name)
+		if err != nil {
+			return nil, err


same comment about not returning the full CRD schema to the caller in an error here

liggitt · 2018-10-30T02:49:01Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go

-		table, err := tableconvertor.New(crd.Spec.AdditionalPrinterColumns)
+		columns, err := getCRDColumnsForVersion(&crd.Spec, v.Name)
+		if err != nil {
+			return nil, err


same error comment here

liggitt · 2018-10-30T02:53:58Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/helpers.go

+			continue
+		}
+		if crdSpec.Validation != nil && v.Schema != nil {
+			return nil, fmt.Errorf("malformed CustomResourceDefinitionSpec: top-level and per-version schemas must be mutual exclusive; spec: %v, version: %s", *crdSpec, version)


be sure to include the CRD name in the error so whoever sees it can resolve it. we probably don't need to dump the whole spec... just the CRD name, version name, and the fact that there was a top-level and per-version value specified is enough (same comment applies to the other errors as well)

staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresourcedefinition/strategy.go

lavalamp · 2018-10-31T00:03:33Z

I'm a big fan of leaving open the door for another schema format.

…

On Tue, Oct 30, 2018 at 4:18 PM Haowei Cai (Roy) ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/types.go <#70211 (comment)> : > @@ -66,6 +72,24 @@ type CustomResourceDefinitionVersion struct { // Storage flags the version as storage version. There must be exactly one // flagged as storage version. Storage bool `json:"storage" protobuf:"varint,3,opt,name=storage"` + // Schema describes the schema for CustomResource used in validation, pruning, and defaulting. + // Top-level and per-version schemas are mutually exclusive. + // Per-version schemas may not all be set to identical values (top-level validation schema should be used instead) + // This field is alpha-level and is only honored by servers that enable the CustomResourceWebhookConversion feature. + // +optional + Schema *JSONSchemaProps `json:"schema,omitempty" protobuf:"bytes,4,opt,name=schema"` More related context: #64483 <#64483> I'm going to add a commit with Schema *CustomResourceValidation `json:"schema,omitempty" protobuf:"bytes,4,opt,name=schema"` first, which may look like: spec: versions: - name: v1 schema: openAPIV3Schema: ... // covers schema, defaults and static validation anotherSchemaLanguage: ... // to be added in future? prune: true — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#70211 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAnglq-Vl38t8cITMyUEwnN_EDM-8FNiks5uqN4sgaJpZM4X5U_r> .

mbohlool · 2018-10-31T17:33:21Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/helpers.go

+		if v.Schema != nil {
+			return v.Schema, nil
+		}
+		return crd.Spec.Validation, nil


are we doing override here? we agreed not to do overrides. That means in getCRDSchemaForVersion we should call the other helper to see if there is any per-version Schema. If there is we should return v.Schema even if it is nil.

No, it's not doing override. I think the auto-clear strategy makes sure that both ways work the same. In the case that you described (v.Schema is nil while there exists other per-version schema), crd.Spec.Validation will also be nil, so getCRDSchemaForVersion returns nil in the end.

You are right. Still to anybody reading this code, it look like we are overriding. It would be a cleaner code if we apply the logic here too and it wouldn't hurt.

or you can check if top level field is not nil and return it at the start of the method with a comment explaining why if it is not nil, it means all per versions will be. I still prefer the first approach though :)

mbohlool · 2018-10-31T17:33:34Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/helpers.go

+		if v.Subresources != nil {
+			return v.Subresources, nil
+		}
+		return crd.Spec.Subresources, nil


mbohlool · 2018-10-31T17:33:45Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/helpers.go

+		if len(v.AdditionalPrinterColumns) > 0 {
+			return v.AdditionalPrinterColumns, nil
+		}
+		return crd.Spec.AdditionalPrinterColumns, nil


mbohlool · 2018-10-31T17:34:26Z

staging/src/k8s.io/apiextensions-apiserver/pkg/features/kube_features.go

@@ -40,6 +40,12 @@ const (
 	//
 	// CustomResourceSubresources defines the subresources for CustomResources
 	CustomResourceSubresources utilfeature.Feature = "CustomResourceSubresources"
+
+	// owner: @mbohlool


add yourself as owner here too please.

roycaihw · 2018-10-31T20:38:22Z

/test pull-kubernetes-e2e-kops-aws

mbohlool · 2018-10-31T20:54:48Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/helpers.go

+		if version != v.Name {
+			continue
+		}
+		if hasPerVersionSchema(crd.Spec.Versions) {


you can do this check earlier before for loop and return crd.Spec.Validation as early as possible then you will have less nested blocks.

agree that unnesting like this would improve readability. up to you if you want to unnest like this:

if !hasPerVersionSchema(crd.Spec.Versions) { return crd.Spec.Validation, nil } if crd.Spec.Validation != nil { return nil, fmt.Errorf("malformed CustomResourceDefinition %s version %s: top-level and per-version schemas must be mutual exclusive", crd.Name, version) } for _, v := range crd.Spec.Versions { if version == v.Name { return v.Schema, nil } } return nil, fmt.Errorf("version %s not found in CustomResourceDefinition: %v", version, crd.Name)

roycaihw · 2018-11-06T07:52:21Z

@liggitt Thanks for the suggestions. I amended latest change to the last commit. Please take a look

sttts · 2018-11-06T09:02:18Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/types.go

@@ -149,6 +158,24 @@ type CustomResourceDefinitionVersion struct {
 	// Storage flags the version as storage version. There must be exactly one flagged
 	// as storage version.
 	Storage bool
+	// Schema describes the schema for CustomResource used in validation, pruning, and defaulting.
+	// Top-level and per-version schemas are mutually exclusive.
+	// Per-version schemas may not all be set to identical values (top-level validation schema should be used instead)


What does "may not" mean here?

Updated to "must not"

sttts · 2018-11-06T09:02:55Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/types.go

+	// This field is alpha-level and is only honored by servers that enable the CustomResourceWebhookConversion feature.
+	// +optional
+	Schema *CustomResourceValidation
+	// Subresources describes the subresources for CustomResources


CustomResource

Updated.

This was copied from the top-level Subresources comment. I updated the top-level one as well. We have similar comments for other fields (e.g. Validation describes the validation methods for CustomResources). It might worth another PR to figure out the scope and do the clean up.

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go

staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresourcedefinition/strategy.go

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/helpers.go

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go

staging/src/k8s.io/apiextensions-apiserver/test/integration/helpers.go

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/types.go

roycaihw · 2018-11-06T23:16:46Z

/test pull-kubernetes-integration

roycaihw · 2018-11-06T23:49:06Z

@sttts Comments addressed. Please take a look :)

@liggitt I interpreted "squash in the comment commit" in #70211 (review) as squashing new changes in an "address comments:" commit, so that we don't have to review the functionality part from the beginning. Please let me know if you want me to squash everything together.

liggitt · 2018-11-07T03:15:56Z

lgtm

once @sttts acks, can squash the review commits in and we can tag for merge

sttts · 2018-11-07T09:55:38Z

Lgtm. Leaving #70211 (comment) to @liggitt.

/approve

schema, subresources and columns

CRD per-version schema/subresources/columns

whitespace changes)

roycaihw · 2018-11-07T17:40:39Z

Commits squashed. Please take a look

liggitt · 2018-11-07T17:43:16Z

/lgtm
/approve

k8s-ci-robot · 2018-11-07T17:43:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, roycaihw, sttts

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~api/OWNERS~~ [liggitt]
~~staging/src/k8s.io/apiextensions-apiserver/OWNERS~~ [sttts]
~~staging/src/k8s.io/apiextensions-apiserver/pkg/apis/OWNERS~~ [liggitt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot requested review from liggitt, mbohlool and sttts October 25, 2018 01:25

roycaihw commented Oct 25, 2018

View reviewed changes

liggitt reviewed Oct 25, 2018

View reviewed changes

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/types.go Show resolved Hide resolved

liggitt reviewed Oct 25, 2018

View reviewed changes

This was referenced Oct 25, 2018

CRD supports multi-version Schema #67553

Closed

CRD supports multi-version Subresources and AdditionalPrintColumns #67521

Closed

CRD webhook conversion #67006

Merged

k8s-ci-robot added this to the v1.13 milestone Oct 25, 2018

roycaihw force-pushed the crd-multi-version-x branch from 006683a to 9568794 Compare October 26, 2018 00:14

liggitt reviewed Oct 30, 2018

View reviewed changes

shubheksha mentioned this pull request Oct 30, 2018

Webhook Conversion for Custom Resources kubernetes/enhancements#598

Closed

roycaihw force-pushed the crd-multi-version-x branch from e3365d0 to 2f83326 Compare October 31, 2018 00:56

mbohlool reviewed Oct 31, 2018

View reviewed changes