Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow more fields at root of CRD schema if status is enabled #65357

Merged
merged 1 commit into from Jul 3, 2018

Conversation

@nikhita
Copy link
Member

nikhita commented Jun 22, 2018

Fixes #65293

Currently, we allow only properties, required and description at the root of the CRD schema when the status subresource is enabled.

We can also include some other fields, even though sometimes they might not make sense (but they don't harm).

The main idea is that when validation schema for status is extracted as properties["status"], validation for status is not lost.

Release note:

More fields are allowed at the root of the CRD validation schema when the status subresource is enabled.
@nikhita

This comment has been minimized.

Copy link
Member Author

nikhita commented Jun 22, 2018

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

// only "object" type is valid at root of the schema since validation schema for status is extracted as properties["status"]
if fieldName == "Type" {
if schema.Type != "object" {
allErrs = append(allErrs, field.Invalid(fldPath.Child("openAPIV3Schema"), *schema, fmt.Sprintf(`only "object" is allowed as the type at the root of the schema if the status subresource is enabled, got "%v"`, schema.Type)))

This comment has been minimized.

@sttts

sttts Jun 22, 2018

Contributor

.Child("type"), no?

This comment has been minimized.

@nikhita

nikhita Jun 22, 2018

Author Member

needed .Child("openAPIV3Schema.type") to get the correct path. :)

fixed. 👍

@nikhita nikhita force-pushed the nikhita:crd-subresources-root-schema branch from 1a56d99 to b38e1a9 Jun 22, 2018

@@ -521,3 +533,14 @@ func validateSimpleJSONPath(s string, fldPath *field.Path) field.ErrorList {

return allErrs
}

var allowedFieldsAtRootSchema = []string{"Description", "Type", "Format", "Title", "Maximum", "ExclusiveMaximum", "Minimum", "ExclusiveMinimum", "MaxLength", "MinLength", "Pattern", "MaxItems", "MinItems", "UniqueItems", "MultipleOf", "Required", "Items", "Properties", "ExternalDocs", "Example"}

This comment has been minimized.

@sttts

sttts Jun 22, 2018

Contributor

none of these are harmful, most are useless. But for uniformity I would tend to allow all these.

This comment has been minimized.

@nikhita

nikhita Jun 22, 2018

Author Member

most are useless.

yeah, most of them don't make sense once we have restricted the type to object as well.

Allow more fields at root of CRD schema if status is enabled
Currently, we allow only properties, required and description at the
root of the CRD schema when the status subresource is enabled.

We can also include some other fields, even though sometimes they
might not make sense (but they don't harm).

The main idea is that when validation schema for status is extracted
as properties["status"], validation for status is not lost.
@sttts

This comment has been minimized.

Copy link
Contributor

sttts commented Jun 22, 2018

To be clear, we have three sets of now valid fields at the root:

  • type: this is redundant, but correct (with value object).
  • example, title, ...: these are about meta info, docs and such. They don't harm, and there might be tooling use-cases where these are helpful.
  • mininum, maximum, uniqueItems, ...: these are no-ops for type object. So they are kind of senseless. But JSON Schema allows to set them even though the type is object. So for uniformity we should allow these at the root as well. Higher level tools will be simpler as they don't have to special case the root of the schema.

I have no strong feelings about this, but tend to go forward with the PR.

/cc @kubernetes/sig-api-machinery-api-reviews

@mbohlool

This comment has been minimized.

Copy link
Member

mbohlool commented Jun 22, 2018

Why do we limit root fields when there is a status sub-resource?

@nikhita

This comment has been minimized.

Copy link
Member Author

nikhita commented Jun 24, 2018

Why do we limit root fields when there is a status sub-resource?

For validation of the status subresource, we only validate the "status part" of the object (as opposed to validating the whole object). To do this, we extract the schema for status validation using crd.Spec.Validation.OpenAPIV3Schema.Properties["status"].

// for the status subresource, validate only against the status schema
if crd.Spec.Validation != nil && crd.Spec.Validation.OpenAPIV3Schema != nil && crd.Spec.Validation.OpenAPIV3Schema.Properties != nil {
if statusSchema, ok := crd.Spec.Validation.OpenAPIV3Schema.Properties["status"]; ok {
openapiSchema := &spec.Schema{}
if err := apiservervalidation.ConvertJSONSchemaProps(&statusSchema, openapiSchema); err != nil {
return nil, err
}
statusValidator = validate.NewSchemaValidator(openapiSchema, nil, "", strfmt.Default)
}
}
}

Since we extract the schema for status validation using crd.Spec.Validation.OpenAPIV3Schema.Properties["status"], we need to make sure that none of the root fields affect validation for status.

Fields like Description, Title, etc at the root don't hurt, but fields like AnyOf, AllOf at the root affect the validation at Properties["status"]. So we need to make sure that AnyOf, AllOf (and similar fields) are not allowed at the root.

@marun

This comment has been minimized.

Copy link
Member

marun commented Jul 2, 2018

Looking forward to this landing! Marshalling of JSONSchemaProps (openapischema type) will always output Type: "object", so CRDs defined in code and then written to yaml (as kubebuilder does) will fail validation without this PR.

Can this be a cherrypick candidate for 1.11?

cc: @pmorie

@sttts

This comment has been minimized.

Copy link
Contributor

sttts commented Jul 3, 2018

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Jul 3, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jul 3, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nikhita, 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:

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

@nikhita

This comment has been minimized.

Copy link
Member Author

nikhita commented Jul 3, 2018

Can this be a cherrypick candidate for 1.11?

This is safe to be cherry-picked because it allows fields that don't really "do" anything. @sttts wdyt?

@sttts

This comment has been minimized.

Copy link
Contributor

sttts commented Jul 3, 2018

This is safe to be cherry-picked because it allows fields that don't really "do" anything. @sttts wdyt?

CRDs compatible with 1.11.1 won't be accepted in 1.11.0. Is that an issue?

@nikhita

This comment has been minimized.

Copy link
Member Author

nikhita commented Jul 3, 2018

CRDs compatible with 1.11.1 won't be accepted in 1.11.0. Is that an issue?

cc @foxish @calebamiles for cherry-pick question to 1.11

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jul 3, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jul 3, 2018

Automatic merge from submit-queue (batch tested with PRs 65357, 65568). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit da64942 into kubernetes:master Jul 3, 2018

17 checks passed

Submit Queue Requested and waiting for github e2e test to start running a second time.
Details
cla/linuxfoundation nikhita authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@nikhita nikhita deleted the nikhita:crd-subresources-root-schema branch Jul 5, 2018

@foxish

This comment has been minimized.

Copy link
Member

foxish commented Jul 10, 2018

Given this looks like a breaking change, it doesn't seem like it should be cherrypicked. Can we have some more details on what exactly will/won't work for a user going between versions?

@nikhita

This comment has been minimized.

Copy link
Member Author

nikhita commented Jul 10, 2018

Can we have some more details on what exactly will/won't work for a user going between versions?

When the status subresource is enabled, the current behaviour right now (for 1.11.0) is to allow only properties, description and required at the root of the CRD validation schema (https://kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definitions/#advanced-topics).

If the status subresource is not enabled, some other fields like title are also allowed.

So if someone enables the status subresource, their CRDs will now be invalid if it contained title within the root CRD validation schema. This PR allows more fields like title, etc so that such CRDs do not become invalid.

Note: the fields that this PR allows are "harmless" in the sense that they don't add any more functionality. They just make sure that if those fields in the CRD existed initially before enabling the status subresource, the CRD does not become invalid.

droot added a commit to droot/controller-tools that referenced this pull request Aug 30, 2018

removed type field from crd validation schema
k8s 1.11 rejects CRDs with type property at the root if status sub-resources enabled.
This change drops the type field for CRD at the root level.

kubernetes/kubernetes#65293

kubernetes/kubernetes#65357

droot added a commit to droot/controller-tools that referenced this pull request Aug 30, 2018

removed type field from crd validation schema
k8s 1.11 rejects CRDs with type property at the root if status sub-resources enabled.
This change drops the type field for CRD at the root level.

kubernetes/kubernetes#65293

kubernetes/kubernetes#65357
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.