apiextensions: complete default-under-metadata validation and storage pruning #78829

sttts · 2019-06-09T21:14:48Z

Implement the validation part of kubernetes/enhancements#1166. ~~Storage default pruning will be a follow-up.~~ Both the validation and storage part is included.

Possible follow-ups (not in this PR):

make error paths in under-metadata default values absolute

Verify that CRD default values in OpenAPI specs are pruned, with the exceptions of values under `metadata`.

fedebongio · 2019-06-10T21:10:22Z

/assign @roycaihw
/cc @jennybuckley

roycaihw · 2019-08-07T17:34:50Z

/cc @liggitt

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

roycaihw · 2019-08-22T20:59:26Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/convert.go

@@ -80,7 +80,7 @@ func NewStructural(s *apiextensions.JSONSchemaProps) (*Structural, error) {
 		ss.Items = item
 	}

-	if len(s.Properties) > 0 {
+	if s.Properties != nil {


can you provide an empty, but defined properties field

you cannot if you use client-go to wire a CRD to a JSON or proto (the empty field gets omitted in both cases); you can if you write a JSON and send it to apiserver directly

nit: this relaxes validation slightly, the validation error message might be inaccurate now

@liggitt are we fine with that?

we could add a formal validation that properties for non-preserve-unknown-fields objects are never empty, making JSON and proto to behave the same.

@liggitt are we fine with that?

not as written.

we have to be really careful here... this could allow a user to submit the following data that would pass validation, then fail validation after round-tripping through etcd:

openAPIV3Schema: type: object properties: template: type: object x-kubernetes-embedded-resource: true properties: {}

in 1.15, even if properties: {} was submitted to the apiserver (via a manual JSON request), the len(s.Properties) > 0 check here meant that the structural schema had a nil Properties field, and validation failed with ...properties: Required value: must not be empty if x-kubernetes-embedded-resource is true without x-kubernetes-preserve-unknown-fields error

with this PR, the CRD is allowed to be created, and updates fail with that message (since round-tripping through protobuf in etcd truncated the properties field to nil).

we could add a formal validation that properties for non-preserve-unknown-fields objects are never empty, making JSON and proto to behave the same.

that seems best. we know no existing schemas or existing 1.15 API clients successfully specified empty properties to get around that nil check validation, because of the len() > 0 check here.

I would:

leave the len() > 0 check here (preserving the difference between nil and 0-length doesn't seem useful if we know we can't round-trip it in through etcd)

switch the if s.XEmbeddedResource && !s.XPreserveUnknownFields && s.Properties == nil { check to if s.XEmbeddedResource && !s.XPreserveUnknownFields && len(s.Properties) == 0 { to make the check match what we can round-trip

If someone really wants an embedded resource with no other properties without preserving unknown fields, they can explicitly define apiVersion / kind properties

including tests.

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

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/surroundingobject.go

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/prune.go

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/surroundingobject.go

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

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

liggitt · 2019-08-23T14:43:09Z

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

+
+	for _, tst := range tests {
+		t.Run(strings.Join(tst.path, "."), func(t *testing.T) {
+			obj, found, err := unstructured.NestedMap(foo.Object, tst.path...)


this tests that defaults of unknown fields didn't survive defaulting->serialization->etcd->decoding->pruning, but it's not clear that the pruning happened before data got persisted to etcd (which is what we really care about)

can we use the storage.NewEtcdObjectReader (from the conversion webhook tests) to pull data out of etcd for verification?

yep, I noticed that as well. Let me check about etcd object reader.

done. And verified that without the fixed default pruning algorithm it breaks 👍

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

liggitt · 2019-08-23T15:37:21Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/prune.go

+		if err != nil {
+			return false, fmt.Errorf("failed to prune default value: %v", err)
+		}
+		pruning.Prune(obj, p.rootSchema, true)


just making sure I understand, this prunes both metadata and non-metadata fields? (though non-metadata defaults should be no-ops since we check there are no pruned fields in those at validation time?)

no, it didn't. Fixed.

liggitt · 2019-08-23T17:29:50Z

(also fix the golint verify errors :-/)

liggitt · 2019-08-23T19:38:36Z

/approve

squash review commits, fixup lint, then lgtm

liggitt · 2019-08-23T20:12:13Z

/lgtm
/approve
/retest

k8s-ci-robot · 2019-08-23T20:12:25Z

[APPROVALNOTIFIER] This PR is APPROVED

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

~~staging/src/k8s.io/apiextensions-apiserver/OWNERS~~ [liggitt,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

…ties

liggitt · 2019-08-23T20:23:09Z

/lgtm

sttts · 2019-08-23T20:50:05Z

Again no bazel test logs.

/retest

roycaihw · 2019-09-17T21:23:32Z

/area custom-resources

k8s-ci-robot requested review from deads2k and pwittrock June 9, 2019 21:16

sttts force-pushed the sttts-crd-embedded-resource-metadata-defaulting branch from 90b489c to 1e4f2f9 Compare June 10, 2019 09:32

sttts added this to the v1.16 milestone Jun 10, 2019

sttts force-pushed the sttts-crd-embedded-resource-metadata-defaulting branch from 1e4f2f9 to 67d25f5 Compare June 10, 2019 17:02

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 10, 2019

k8s-ci-robot assigned roycaihw Jun 10, 2019

k8s-ci-robot requested a review from jennybuckley June 10, 2019 21:10

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 30, 2019

k8s-ci-robot requested a review from liggitt August 7, 2019 17:34

sttts force-pushed the sttts-crd-embedded-resource-metadata-defaulting branch from 67d25f5 to 4f8261b Compare August 17, 2019 13:10

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 17, 2019

sttts changed the title ~~WIP: apiextensions: complete default-under-metadata validation~~ apiextensions: complete default-under-metadata validation Aug 17, 2019

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 17, 2019

liggitt reviewed Aug 22, 2019

View reviewed changes

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/validation.go Outdated Show resolved Hide resolved

liggitt reviewed Aug 22, 2019

View reviewed changes

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/validation.go Show resolved Hide resolved

liggitt reviewed Aug 22, 2019

View reviewed changes

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/validation.go Outdated Show resolved Hide resolved

roycaihw reviewed Aug 23, 2019

View reviewed changes

liggitt reviewed Aug 23, 2019

View reviewed changes

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go Outdated Show resolved Hide resolved

liggitt reviewed Aug 23, 2019

View reviewed changes

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go Outdated Show resolved Hide resolved

liggitt reviewed Aug 23, 2019

View reviewed changes

staging/src/k8s.io/apiextensions-apiserver/test/integration/pruning_test.go Show resolved Hide resolved

liggitt reviewed Aug 23, 2019

View reviewed changes

sttts force-pushed the sttts-crd-embedded-resource-metadata-defaulting branch from 5b49db8 to 1bfd7e9 Compare August 23, 2019 17:42

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 23, 2019

sttts force-pushed the sttts-crd-embedded-resource-metadata-defaulting branch from 1bfd7e9 to 55e2255 Compare August 23, 2019 20:07

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

sttts added 5 commits August 23, 2019 22:20

apiextension: fix structural additionalProperties in ToOpenAPI()

b916408

apiextension: fix go-openapi validation error path at the root

f140ceb

apiextension: reject x-kubernetes-embedded-resource with empty proper…

90fed07

…ties

apiextension: complete validation of meta data defaults

135902b

apiextension: prune default values in storage

4fd200c

sttts force-pushed the sttts-crd-embedded-resource-metadata-defaulting branch from 55e2255 to 4fd200c Compare August 23, 2019 20:21

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 23, 2019

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

k8s-ci-robot merged commit f9afe46 into kubernetes:master Aug 24, 2019

k8s-ci-robot added the area/custom-resources label Sep 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

apiextensions: complete default-under-metadata validation and storage pruning #78829

apiextensions: complete default-under-metadata validation and storage pruning #78829

sttts commented Jun 9, 2019 •

edited

fedebongio commented Jun 10, 2019

roycaihw commented Aug 7, 2019

roycaihw Aug 22, 2019

sttts Aug 23, 2019

sttts Aug 23, 2019

liggitt Aug 23, 2019

liggitt Aug 23, 2019 •

edited

sttts Aug 23, 2019

sttts Aug 23, 2019

liggitt Aug 23, 2019

sttts Aug 23, 2019

sttts Aug 23, 2019

liggitt Aug 23, 2019 •

edited

sttts Aug 23, 2019

liggitt commented Aug 23, 2019

liggitt commented Aug 23, 2019

liggitt commented Aug 23, 2019

k8s-ci-robot commented Aug 23, 2019

liggitt commented Aug 23, 2019

sttts commented Aug 23, 2019

roycaihw commented Sep 17, 2019

apiextensions: complete default-under-metadata validation and storage pruning #78829

apiextensions: complete default-under-metadata validation and storage pruning #78829

Conversation

sttts commented Jun 9, 2019 • edited

fedebongio commented Jun 10, 2019

roycaihw commented Aug 7, 2019

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

liggitt Aug 23, 2019 • edited

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

liggitt Aug 23, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Aug 23, 2019

liggitt commented Aug 23, 2019

liggitt commented Aug 23, 2019

k8s-ci-robot commented Aug 23, 2019

liggitt commented Aug 23, 2019

sttts commented Aug 23, 2019

roycaihw commented Sep 17, 2019

sttts commented Jun 9, 2019 •

edited

liggitt Aug 23, 2019 •

edited

liggitt Aug 23, 2019 •

edited