Fix openapi model generation for apply when x-int-or-string is set #97172

Jefftree · 2020-12-09T23:04:05Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

This is a cleanup of #90656.

#85127 and #90902 point out that we cannot build models for openapiv3 types if x-int-or-string is set. Moving schema.Unfold() above StripValueValidations lets us strip the anyOf tags that schema.Unfold() generates and fixes the invalid schema issue.

Which issue(s) this PR fixes:

Fixes #85127

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

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

And also, the current code only understands V2, so we should use that exclusively, or we're guaranteed to have errors in the future.

Jefftree · 2020-12-10T00:50:07Z

/test pull-kubernetes-integration

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

apelisse · 2020-12-10T20:33:35Z

staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder/builder.go

@@ -103,14 +103,14 @@ func BuildSwagger(crd *apiextensionsv1.CustomResourceDefinition, version string,
 				if opts.AllowNonStructural || len(structuralschema.ValidateStructural(nil, ss)) == 0 {
 					schema = ss

+					schema = schema.Unfold()


Maybe add a comment that this will add anyOf that needs to be stripped by the line below.

Why was this moved? In order to also skip the value validations coming from unfolding?

Yes. Unfolding adds the anyOf tag if x-int-or-string is set. The tag isn't compatible with the v2 schema so we need to strip them via schema.StripValueValidations()

fedebongio · 2020-12-10T21:06:27Z

/assign @apelisse
/triage accepted

apelisse · 2020-12-12T03:46:17Z

@sttts Would you mind taking a look at this? This has a change in behavior that I think we discussed and agreed would be better (don't use the schema when it's not structural). That should finally fix that bug you've been asking about! 😅
@jennybuckley Would you mind also looking at this since you wrote the initial code here?

apelisse · 2020-12-12T03:47:45Z

The addition of the test is a great step in the right direction, and gives us the confidence that we'll be able to possibly solve these bugs faster in the future, or even prevent them from happening, thanks @Jefftree! LGTM for me.

sttts · 2021-01-07T11:08:59Z

Sgtm.

Leaving lgtm to @apelisse.

apelisse · 2021-01-07T17:17:00Z

/lgtm

sttts · 2021-01-12T21:01:18Z

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

+	}
+
+	return staticSpec, nil
+}


Am not a fan of tests in staging repos that use files outside. This makes splitting repos hard some day.

+1, in addition to the fact that we don't need most of the types defined in this file (and json unmarshalling deep structures is slooooow in race mode)

opened #98694 with a local / trimmed swagger.json to use for the test

sttts · 2021-01-12T21:02:17Z

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

+	for _, test := range tests {
+		crd.Spec.Versions[0].Schema = &test
+		if _, err := buildOpenAPIModelsForApply(staticSpec, &crd); err != nil {
+			t.Fatalf("failed to convert to apply model: %v", err)


and print the index, or give the tests names

apelisse · 2021-01-12T21:02:41Z

I don't disagree. We could copy the file but it's not super useful now, and it's not really hard to do the day we're trying to split.

sttts · 2021-01-12T21:06:36Z

/approve

k8s-ci-robot · 2021-01-12T21:07:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Jefftree, 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~~ [sttts]
~~test/integration/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

liggitt · 2021-02-02T13:39:08Z

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

+		},
+	}
+
+	staticSpec, err := getOpenAPISpecFromFile()


this line takes ~12 seconds to run in unit tests run with -race detection

liggitt · 2021-02-02T13:40:01Z

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

+	}
+
+	return staticSpec, nil
+}


+1, in addition to the fact that we don't need most of the types defined in this file (and json unmarshalling deep structures is slooooow in race mode)

liggitt · 2021-02-02T13:40:24Z

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

+
+	for _, test := range tests {
+		crd.Spec.Versions[0].Schema = &test
+		if _, err := buildOpenAPIModelsForApply(staticSpec, &crd); err != nil {


this line takes ~6 seconds to run in unit tests with -race detection on

liggitt · 2021-02-02T13:53:52Z

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

+	}
+
+	return staticSpec, nil
+}


opened #98694 with a local / trimmed swagger.json to use for the test

... to bypass issue kubernetes#85127 until we possibly rebase this branch to get the real fix. Signed-off-by: David Festal <dfestal@redhat.com>

apelisse added 2 commits December 8, 2020 11:17

Only create SSA models for CRDs with structural schema

aebb47e

And also, the current code only understands V2, so we should use that exclusively, or we're guaranteed to have errors in the future.

Start writing a unit-test for buildOpenAPIForApplyModels

216d820

k8s-ci-robot requested review from enisoc and jpbetz December 9, 2020 23:04

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

Jefftree force-pushed the opentest branch from bdb8834 to 37545d0 Compare December 9, 2020 23:06

apelisse reviewed Dec 10, 2020

View reviewed changes

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

apelisse reviewed Dec 10, 2020

View reviewed changes

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

apelisse reviewed Dec 10, 2020

View reviewed changes

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

apelisse reviewed Dec 10, 2020

View reviewed changes

k8s-ci-robot assigned apelisse Dec 10, 2020

Jefftree changed the title ~~[WIP] Use openapi V2 for building openapi models for apply~~ Fix openapi model generation for apply when x-int-or-string is set Dec 10, 2020

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 Dec 10, 2020

tests for buildOpenAPIModelsForApply

32a64cf

Jefftree force-pushed the opentest branch from f76e0b2 to 32a64cf Compare December 10, 2020 23:33

sttts added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Jan 7, 2021

k8s-ci-robot removed the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Jan 7, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 7, 2021

sttts reviewed Jan 12, 2021

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 12, 2021

k8s-ci-robot merged commit 70435a2 into kubernetes:master Jan 13, 2021

k8s-ci-robot added this to the v1.21 milestone Jan 13, 2021

Jefftree mentioned this pull request Jan 14, 2021

"error building openapi models" error logged serving CRD containing x-kubernetes-int-or-string #90902

Closed

apelisse mentioned this pull request Jan 14, 2021

[Docs] Improve documentation for CRD/Structured merge diff integration #89374

Closed

avalluri mentioned this pull request Jan 25, 2021

pmemcsideployments.pmem-csi.intel.com: ... has invalid property: anyOf intel/pmem-csi#849

Closed

liggitt mentioned this pull request Feb 2, 2021

Speed up CRD apply/openapi unit tests #98694

Merged

liggitt reviewed Feb 2, 2021

View reviewed changes

bsctl mentioned this pull request Feb 8, 2021

Tenant CRD schema generates some errors in kube-apiserver logs projectcapsule/capsule#199

Closed

cbandy mentioned this pull request Mar 3, 2021

x-kubernetes-int-or-string changes finalizers behavior #99730

Closed

binoue mentioned this pull request Mar 23, 2021

[Coil] Fix egresses.coil.cybozu.com invalid property error cybozu-go/neco#1481

Closed

Jefftree deleted the opentest branch December 7, 2021 17:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix openapi model generation for apply when x-int-or-string is set #97172

Fix openapi model generation for apply when x-int-or-string is set #97172

Jefftree commented Dec 9, 2020 •

edited

Jefftree commented Dec 10, 2020

apelisse Dec 10, 2020

sttts Jan 6, 2021

Jefftree Jan 6, 2021

sttts Jan 7, 2021

fedebongio commented Dec 10, 2020

apelisse commented Dec 12, 2020

apelisse commented Dec 12, 2020 •

edited

sttts commented Jan 7, 2021

apelisse commented Jan 7, 2021

sttts Jan 12, 2021

liggitt Feb 2, 2021

liggitt Feb 2, 2021

sttts Jan 12, 2021

sttts Jan 12, 2021 •

edited

apelisse commented Jan 12, 2021

sttts commented Jan 12, 2021

k8s-ci-robot commented Jan 12, 2021

liggitt Feb 2, 2021

liggitt Feb 2, 2021

liggitt Feb 2, 2021

liggitt Feb 2, 2021

Fix openapi model generation for apply when x-int-or-string is set #97172

Fix openapi model generation for apply when x-int-or-string is set #97172

Conversation

Jefftree commented Dec 9, 2020 • edited

Jefftree commented Dec 10, 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

fedebongio commented Dec 10, 2020

apelisse commented Dec 12, 2020

apelisse commented Dec 12, 2020 • edited

sttts commented Jan 7, 2021

apelisse commented Jan 7, 2021

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 Jan 12, 2021 • edited

Choose a reason for hiding this comment

apelisse commented Jan 12, 2021

sttts commented Jan 12, 2021

k8s-ci-robot commented Jan 12, 2021

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

Jefftree commented Dec 9, 2020 •

edited

apelisse commented Dec 12, 2020 •

edited

sttts Jan 12, 2021 •

edited