apiextensions: validation for customresources by nikhita · Pull Request #47263 · kubernetes/kubernetes

nikhita · 2017-06-09T19:19:27Z

Proposal: kubernetes/community#708
Additional discussion: #49879, #50625

Release note:

Add validation for CustomResources via JSON Schema.

/cc @sttts @deads2k

k8s-ci-robot · 2017-06-09T19:19:35Z

Hi @nikhita. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

luxas · 2017-06-11T14:49:02Z

@nikhita Please use two commits for this PR. One for your code and one for automatically generated code

@k8s-bot ok to test

😄

nikhita · 2017-07-02T22:44:50Z

This will not pass tests until kubernetes/gengo#61 is merged.

sttts · 2017-07-03T15:33:58Z

cmd/libs/go2idl/conversion-gen/generators/conversion.go

 		return true
 	case in.Kind == out.Kind:
+		// if the type exists already, return early to avoid recursion
+		if existingTypes[in] {


also here: alreadyVisitedTypes

deads2k · 2017-08-29T13:08:13Z

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

+}
+
+func (s JSON) MarshalJSON() ([]byte, error) {
+	if len(s.Raw) > 0 {


note that you're compressing empty into "null". That's reasonable given the rest of the API, it just catches my eye.

deads2k · 2017-08-29T13:12:03Z

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

+func Convert_v1beta1_JSON_To_apiextensions_JSON(in *JSON, out *apiextensions.JSON, s conversion.Scope) error {
+	if in != nil {
+		var i interface{}
+		if err := json.Unmarshal(in.Raw, &i); err != nil {


This makes sense. The end result is a protobuf shell with a delicious json nougat-y center, right?

sttts · 2017-08-29T13:20:39Z

As the JSON protobuf change is squashed already, here are the relevant lines of code:

the internal JSON type (just an alias for interface{}): https://github.com/kubernetes/kubernetes/pull/47263/files#diff-48b8b5215523ba995d8b41c2dfdf68afR192
the external type (like an runtime.Unknown, but without encoding and kind logic): https://github.com/kubernetes/kubernetes/pull/47263/files#diff-3029a18192feb70af52de676209da9b6R194,
an (optional) JSON field: https://github.com/kubernetes/kubernetes/pull/47263/files#diff-48b8b5215523ba995d8b41c2dfdf68afR159,
the json marshal/unmarshal: https://github.com/kubernetes/kubernetes/pull/47263/files#diff-3fe7a62396a7708e034adc5c0bbff3fdR121
the conversion: https://github.com/kubernetes/kubernetes/pull/47263/files#diff-a48a0d878266c677cd0c5c9987b327b6R40

@smarterclayton ^^^

deads2k · 2017-08-29T13:26:36Z

As the JSON protobuf change is squashed already, here are the relevant lines of code:

the internal JSON type (just an alias for interface{}): https://github.com/kubernetes/kubernetes/pull/47263/files#diff-48b8b5215523ba995d8b41c2dfdf68afR192
the external type: https://github.com/kubernetes/kubernetes/pull/47263/files#diff-3029a18192feb70af52de676209da9b6R194,
an (optional) JSON field: https://github.com/kubernetes/kubernetes/pull/47263/files#diff-48b8b5215523ba995d8b41c2dfdf68afR159,
the json marshal/unmarshal: https://github.com/kubernetes/kubernetes/pull/47263/files#diff-3fe7a62396a7708e034adc5c0bbff3fdR121
the conversion: https://github.com/kubernetes/kubernetes/pull/47263/files#diff-a48a0d878266c677cd0c5c9987b327b6R40

This looks fairly straightforward. protobuf wrapping a limited json field with simple deserialization and a little validation. This may actually have been what the service catalog was looking for a while back.

smarterclayton · 2017-08-29T15:29:31Z

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

+}
+
+// JSONSchemaProps is a JSON-Schema following Specification Draft 4 (http://json-schema.org/).
+type JSONSchemaProps struct {


Maybe move this into its own types.go file - types_jsonschema.go

smarterclayton · 2017-08-29T15:30:12Z

Looks like I expected. This has high level approval from me.

Remove protobuf generation because of the interface type Add custom fuzzer funcs Add custom marshalling Add custom conversion functions move jsonschema types to separate file

update generated proto

* convert our types to openAPI types * update strategy to include crd * use strategy to validate customresource * add helper funcs * Fix conversion of empty ref field * add validation for forbidden fields * add defaulting for schema field * Validate CRD Schema

Update test schema Add polling for TestCRValidationOnCRDUpdate Add tests for forbidden fields Enable featureGate for CustomResourceValidation

update feature gates for generic apiserver Add apiextensions-apiserver features to golint_failures Ignore alpha feature if gate is disabled

sttts · 2017-08-29T17:10:22Z

/lgtm

smarterclayton · 2017-08-29T17:32:14Z

/approve

k8s-github-robot · 2017-08-29T17:32:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, nikhita, smarterclayton, sttts

Associated issue: 49747

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

Details

Needs approval from an approver in each of these OWNERS Files:

~~hack/OWNERS~~ [deads2k,smarterclayton,sttts]
~~pkg/OWNERS~~ [smarterclayton]
~~staging/src/k8s.io/api/core/OWNERS~~ [smarterclayton]
~~staging/src/k8s.io/apiextensions-apiserver/OWNERS~~ [deads2k,smarterclayton,sttts]
~~staging/src/k8s.io/apiserver/OWNERS~~ [deads2k,smarterclayton,sttts]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-ci-robot · 2017-08-30T00:37:20Z

@nikhita: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-gce-bazel	`6ba1523`	link	`/test pull-kubernetes-e2e-gce-bazel`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-github-robot · 2017-08-30T01:37:10Z

Automatic merge from submit-queue

k8s-ci-robot requested review from deads2k and sttts June 9, 2017 19:19

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 9, 2017

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 9, 2017

k8s-github-robot assigned sttts and deads2k Jun 9, 2017

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels Jun 9, 2017

nikhita mentioned this pull request Jun 10, 2017

validation: add types nikhita/gsoc-meta-k8s#18

Closed

3 tasks

nikhita force-pushed the crd-01-validation-types branch from 1cdc672 to 6ef70fe Compare June 11, 2017 20:13

nikhita mentioned this pull request Jun 11, 2017

Fix defaulter-gen for recursive types kubernetes/gengo#61

Merged

nikhita force-pushed the crd-01-validation-types branch from 5044557 to 5e1c126 Compare June 12, 2017 17:58

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 12, 2017

nikhita force-pushed the crd-01-validation-types branch from 5e1c126 to 3bf86ac Compare June 12, 2017 20:23

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 12, 2017

nikhita force-pushed the crd-01-validation-types branch from 3bf86ac to c70f918 Compare June 22, 2017 23:52

nikhita changed the title ~~[WIP] apiextensions: Add types for validation~~ [WIP] apiextensions: validation for customresources Jun 22, 2017

nikhita force-pushed the crd-01-validation-types branch 2 times, most recently from 4ce3290 to 8192977 Compare July 2, 2017 22:37

nikhita force-pushed the crd-01-validation-types branch from 8192977 to 604f48b Compare July 2, 2017 23:08

sttts reviewed Jul 3, 2017

View reviewed changes

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 29, 2017

deads2k reviewed Aug 29, 2017

View reviewed changes

smarterclayton reviewed Aug 29, 2017

View reviewed changes

nikhita added 6 commits August 29, 2017 21:35

Add types for validation of CustomResources

6133d84

Remove protobuf generation because of the interface type Add custom fuzzer funcs Add custom marshalling Add custom conversion functions move jsonschema types to separate file

Add generated code

64948df

update generated proto

Validate CustomResource

fd09c3d

* convert our types to openAPI types * update strategy to include crd * use strategy to validate customresource * add helper funcs * Fix conversion of empty ref field * add validation for forbidden fields * add defaulting for schema field * Validate CRD Schema

Add integration tests

b1e16bf

Update test schema Add polling for TestCRValidationOnCRDUpdate Add tests for forbidden fields Enable featureGate for CustomResourceValidation

Update godeps

ccb8554

Add feature gate for CustomResourceValidation

6ba1523

update feature gates for generic apiserver Add apiextensions-apiserver features to golint_failures Ignore alpha feature if gate is disabled

nikhita force-pushed the crd-01-validation-types branch from 73203bd to 6ba1523 Compare August 29, 2017 16:05

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 29, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 29, 2017

k8s-github-robot merged commit 4457e43 into kubernetes:master Aug 30, 2017

m1093782566 mentioned this pull request Aug 30, 2017

Implement IPVS-based in-cluster service load balancing #46580

Merged

tamalsaha mentioned this pull request Aug 31, 2017

Improve CRD support voyagermesh/voyager#440

Closed

4 tasks

mbohlool mentioned this pull request Aug 31, 2017

PR Dashboard: Incoming PR still included in the list after merge kubernetes/test-infra#4298

Closed

nikhita mentioned this pull request Sep 4, 2017

Add docs for CustomResource validation kubernetes/website#5290

Merged

kokhang mentioned this pull request Sep 18, 2017

On edit of replica pool manifest, operator should resize pool accordingly rook/rook#989

Closed

colemickens mentioned this pull request Oct 12, 2017

Getting CRD Validation to Beta #53829

Closed

munnerz mentioned this pull request Oct 27, 2017

Support OpenAPI v3 #51163

Closed

bbeaudreault mentioned this pull request Apr 20, 2020

JSONSchemaProps does not support properties of type boolean with default value fabric8io/kubernetes-client#2144

Closed

Conversation

nikhita commented Jun 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Jun 9, 2017

Uh oh!

luxas commented Jun 11, 2017

Uh oh!

nikhita commented Jul 2, 2017

Uh oh!

sttts Jul 3, 2017

Choose a reason for hiding this comment

Uh oh!

deads2k Aug 29, 2017

Choose a reason for hiding this comment

Uh oh!

deads2k Aug 29, 2017

Choose a reason for hiding this comment

Uh oh!

sttts Aug 29, 2017

Choose a reason for hiding this comment

Uh oh!

nikhita Aug 29, 2017

Choose a reason for hiding this comment

Uh oh!

sttts commented Aug 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deads2k commented Aug 29, 2017

Uh oh!

smarterclayton Aug 29, 2017

Choose a reason for hiding this comment

Uh oh!

smarterclayton commented Aug 29, 2017

Uh oh!

sttts commented Aug 29, 2017

Uh oh!

smarterclayton commented Aug 29, 2017

Uh oh!

k8s-github-robot commented Aug 29, 2017

Uh oh!

k8s-ci-robot commented Aug 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-github-robot commented Aug 30, 2017 • edited by rmmh Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

nikhita commented Jun 9, 2017 •

edited

Loading

sttts commented Aug 29, 2017 •

edited

Loading

k8s-ci-robot commented Aug 30, 2017 •

edited

Loading

k8s-github-robot commented Aug 30, 2017 •

edited by rmmh

Loading