Show error in status if preserve unknown fields is true for nonstructural schemas #93078

vareti · 2020-07-14T21:26:30Z

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:

/kind api-change

/kind bug

/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:

CRDs with preserveUnknownFields: true won't be published via OpenAPI and kubectl explain will not work. This is intentional. But, nothing in the CRD status suggests that preserveUnknownFields: true is the problem. This PR adds an error message when that is the case for non-structural schema.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Status of v1beta1 CRDs without "preserveUnknownFields:false" will show violation "spec.preserveUnknownFields: Invalid value: true: must be false"

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2020-07-14T21:26:38Z

Hi @vareti. 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 /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.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

vareti · 2020-07-14T21:27:16Z

/assign @sttts

sttts · 2020-07-15T07:31:34Z

...apiextensions-apiserver/pkg/controller/nonstructuralschema/nonstructuralschema_controller.go

+	if in.Spec.PreserveUnknownFields {
+		allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "preserveUnknownFields"),
+			in.Spec.PreserveUnknownFields,
+			fmt.Sprint("preserveUnknownFields must not be true")))


I believe the error string can be short,i.e. "must not be true". Would the condition still look good?

Agreed. Field name is redundant in error message. Condition still looks good even after removing the field.

fejta-bot · 2020-07-19T14:32:13Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

sttts · 2020-09-21T20:32:17Z

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

+		{
+			desc: "empty",
+			expectedViolations: []string{
+				"spec.preserveUnknownFields: Invalid value: true: must not be true",


can you make preserveUnknownFields a test input and add some test with and without?

I added preserveUnknownFields as test input

sttts · 2020-09-21T20:33:57Z

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

+		if v := "spec.versions[0].schema.openAPIV3Schema.properties[a].type: Required value: must not be empty for specified object fields"; strings.Contains(cond.Message, v) {
+			return false, nil
+		}
+		return true, nil


would add another step where you set preserveUnknownFields to false. Then this last violation should also go.

If I set preserveUnknownFields to false, I get a validation error that says the all fields in schema will be pruned which I think is a valid error. Instead, I tried fixing both the validation errors with a patch. Now there will be no violations

sttts · 2020-09-21T20:34:34Z

...tensions-apiserver/pkg/controller/nonstructuralschema/nonstructuralschema_controller_test.go

+				Reason: "Violations",
+				Message: field.Invalid(field.NewPath("spec", "preserveUnknownFields"),
+					true,
+					fmt.Sprint("must not be true")).Error(),


must be false is better IMO.

Yes, sounds good. I updated all the references to must be false

sttts · 2020-09-21T20:44:43Z

/ok-to-test

liggitt · 2020-09-21T20:53:00Z

aren't there several reasons we'll elide the schema?

spec.preserveUnknownFields (Skip publishing OpenAPI for nonstructural schemas #82653)
x-kubernetes-preserve-unknown-fields (Fix publishing x-kubernetes-preserve-unknown-fields working with kubectl #79636)
array type missing items schema (apiextensions: prune array type without items in published OpenAPI #94888)
invalid openapi types (DO NOT publish openapi specs containing bad types #79587)

should we treat those coherently (either by adding/removing a condition for each of those, or a single condition to cover "reasons openapi isn't being published")?

sttts · 2020-09-21T21:02:01Z

@liggitt don't mix non-structural and having constructs that kubectl does not understand or handle well.

This here is about the former. We have a NonStructural condition exactly for that, but forget the global spec.preserveUnknownFields to show as violation.

…ural schemas

liggitt · 2020-09-21T21:48:15Z

This here is about the former. We have a NonStructural condition exactly for that, but forget the global spec.preserveUnknownFields to show as violation.

ah, ok

sttts · 2020-09-23T09:07:19Z

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

@@ -934,7 +946,9 @@ properties:
 metadata:
   type: object
 `,
-			expectedViolations: []string{},
+			expectedViolations: []string{
+				"spec.preserveUnknownFields: Invalid value: true: must be false",


would instead set the value to false in all test fixtures by default, instead of changing all the expectations below.

sttts · 2020-09-23T09:08:32Z

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

+		for _, violation := range expectedViolations {
+			if !strings.Contains(cond.Message, violation) {
+				t.Fatalf("expected violation %q, but got: %v", violation, cond.Message)
+			}


am still a little unsure and surprised about this big fallout here in the test. Would expect something very local. Isn't there a way to rewrite this test by adding steps only without rewriting what we have?

I agree the change is big and the same can be achieved by also adding the steps. But the intent was to just refactor the code to avoid duplication. This part of code does not change any of the previous logic and only adds some checking for preserveUnknownFields

made the changes to update the steps locally. please take a look.

sttts · 2020-09-28T18:45:12Z

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

 			continue
 		}
 		if err != nil {
 			t.Fatalf("unexpected update error: %v", err)
 		}
+		break


sttts · 2020-09-28T18:48:03Z

/lgtm
/approve

k8s-ci-robot · 2020-09-28T18:48:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sttts, vareti

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~~ [sttts]

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

…ings See kubernetes/kubernetes#93078

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 Jul 14, 2020

k8s-ci-robot requested review from enisoc and roycaihw July 14, 2020 21:26

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 14, 2020

k8s-ci-robot assigned sttts Jul 14, 2020

sttts reviewed Jul 15, 2020

View reviewed changes

vareti force-pushed the show-error-in-status-if-preserve-unknown-field-is-true-for-nonstructural-schemas branch 2 times, most recently from 73da3be to e8d38ba Compare July 19, 2020 13:43

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 19, 2020

vareti force-pushed the show-error-in-status-if-preserve-unknown-field-is-true-for-nonstructural-schemas branch from e8d38ba to d782656 Compare July 19, 2020 13:51

vareti force-pushed the show-error-in-status-if-preserve-unknown-field-is-true-for-nonstructural-schemas branch from d782656 to 4680ed7 Compare September 3, 2020 13:15

vareti force-pushed the show-error-in-status-if-preserve-unknown-field-is-true-for-nonstructural-schemas branch from 4680ed7 to 6a6f5b7 Compare September 21, 2020 20:23

sttts reviewed Sep 21, 2020

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 21, 2020

Show error in status if preserve unknown fields is true for nonstruct…

e643286

…ural schemas

vareti force-pushed the show-error-in-status-if-preserve-unknown-field-is-true-for-nonstructural-schemas branch 2 times, most recently from f81dbb1 to 762bdd6 Compare September 22, 2020 18:36

sttts reviewed Sep 23, 2020

View reviewed changes

vareti force-pushed the show-error-in-status-if-preserve-unknown-field-is-true-for-nonstructural-schemas branch from 762bdd6 to 6e39fc6 Compare September 23, 2020 19:27

vareti added 3 commits September 28, 2020 13:28

fix error message

4c995d8

fix integration tests

d141efc

make update

68c23df

vareti force-pushed the show-error-in-status-if-preserve-unknown-field-is-true-for-nonstructural-schemas branch from 6e39fc6 to 68c23df Compare September 28, 2020 18:29

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 28, 2020

sttts reviewed Sep 28, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 28, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 28, 2020

k8s-ci-robot merged commit 6e3ef0b into kubernetes:master Sep 28, 2020

rfranzke mentioned this pull request Dec 2, 2020

☂️-Issue for "Support for Kubernetes v1.20” gardener/gardener#3260

Closed

rfranzke added a commit to gardener/gardener that referenced this pull request Dec 3, 2020

Update CRD OpenAPI validations and set "preserve-unknown-fields" sett…

06170ee

…ings See kubernetes/kubernetes#93078

rfranzke added a commit to gardener/gardener that referenced this pull request Dec 3, 2020

Update CRD OpenAPI validations and set "preserve-unknown-fields" sett…

5f07504

…ings See kubernetes/kubernetes#93078

rfranzke added a commit to gardener/gardener that referenced this pull request Dec 12, 2020

Update CRD OpenAPI validations and set "preserve-unknown-fields" sett…

75f503a

…ings See kubernetes/kubernetes#93078

rfranzke added a commit to gardener/gardener that referenced this pull request Dec 14, 2020

Update CRD OpenAPI validations and set "preserve-unknown-fields" sett…

4cad502

…ings See kubernetes/kubernetes#93078

rfranzke added a commit to gardener/gardener that referenced this pull request Dec 15, 2020

Update CRD OpenAPI validations and set "preserve-unknown-fields" sett…

c8a20da

…ings See kubernetes/kubernetes#93078

rfranzke added a commit to gardener/gardener that referenced this pull request Dec 16, 2020

Update CRD OpenAPI validations and set "preserve-unknown-fields" sett…

71cb86b

…ings See kubernetes/kubernetes#93078

ezeeyahoo pushed a commit to ezeeyahoo/gardener that referenced this pull request Feb 17, 2021

Update CRD OpenAPI validations and set "preserve-unknown-fields" sett…

517f5b9

…ings See kubernetes/kubernetes#93078

ezeeyahoo pushed a commit to ezeeyahoo/gardener that referenced this pull request Feb 20, 2021

Update CRD OpenAPI validations and set "preserve-unknown-fields" sett…

3d83d89

…ings See kubernetes/kubernetes#93078

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show error in status if preserve unknown fields is true for nonstructural schemas #93078

Show error in status if preserve unknown fields is true for nonstructural schemas #93078

vareti commented Jul 14, 2020 •

edited

Loading

k8s-ci-robot commented Jul 14, 2020

vareti commented Jul 14, 2020

sttts Jul 15, 2020

vareti Jul 15, 2020

fejta-bot commented Jul 19, 2020

sttts Sep 21, 2020

vareti Sep 22, 2020

sttts Sep 21, 2020

vareti Sep 22, 2020

sttts Sep 21, 2020

vareti Sep 22, 2020

sttts commented Sep 21, 2020

liggitt commented Sep 21, 2020

sttts commented Sep 21, 2020

liggitt commented Sep 21, 2020

sttts Sep 23, 2020

vareti Sep 23, 2020

sttts Sep 23, 2020

vareti Sep 23, 2020

vareti Sep 23, 2020 •

edited

Loading

sttts Sep 28, 2020

sttts commented Sep 28, 2020

k8s-ci-robot commented Sep 28, 2020

Show error in status if preserve unknown fields is true for nonstructural schemas #93078

Show error in status if preserve unknown fields is true for nonstructural schemas #93078

Conversation

vareti commented Jul 14, 2020 • edited Loading

k8s-ci-robot commented Jul 14, 2020

vareti commented Jul 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fejta-bot commented Jul 19, 2020

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

sttts commented Sep 21, 2020

liggitt commented Sep 21, 2020

sttts commented Sep 21, 2020

liggitt commented Sep 21, 2020

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

vareti Sep 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Sep 28, 2020

k8s-ci-robot commented Sep 28, 2020

vareti commented Jul 14, 2020 •

edited

Loading

vareti Sep 23, 2020 •

edited

Loading