Directly convert CRD structuralSchema to smdSchema #87872

jennybuckley · 2020-02-06T02:13:24Z

What type of PR is this?
/kind feature
/sig api-machinery
/wg apply
/priority important-longterm
/cc @apelisse @jpbetz @sttts

What this PR does / why we need it:
Currently we convert the validation field in CRDs into v2 openapi, then into 'proto.Models', then into an smd schema. This causes issues because CRDs are allowed to put v3 openapi in their validation fields, some of which an smd schema could express, but isn't compatible with 'proto.Models', so that information is lost.

Does this PR introduce a user-facing change?:

NONE

apelisse · 2020-02-07T00:27:24Z

the verify failure seem legitimate :-)

k8s-ci-robot · 2020-02-07T23:54:59Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jennybuckley
To complete the pull request process, please assign apelisse, sttts
You can assign the PR to them by writing /assign @apelisse @sttts in a comment when ready.

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

Needs approval from an approver in each of these files:

staging/src/k8s.io/apiextensions-apiserver/OWNERS
~~staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/OWNERS~~ [jennybuckley]
test/integration/apiserver/OWNERS
vendor/OWNERS

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

jennybuckley · 2020-02-11T00:18:31Z

This is mostly done but I still need to figure out a plan for testing it

jennybuckley · 2020-02-14T18:58:00Z

rebased

jennybuckley · 2020-02-18T17:50:45Z

/retest

apelisse

I've mostly started looking at the wiring. Haven't looked at the actual conversion. Let's talk about it in person though.

apelisse · 2020-02-27T18:15:45Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/structuredmerge.go

+// and uses a default deduced type converter if none is provided.
+func NewCRDStructuredMergeManager(typeConverter TypeConverter, objectConverter runtime.ObjectConvertor, objectDefaulter runtime.ObjectDefaulter, gv schema.GroupVersion, hub schema.GroupVersion) (_ Manager, err error) {
+	if typeConverter == nil {
+		typeConverter = internal.DeducedTypeConverter{}


I realize that this is how we used to do it, but wouldn't it be better to have this somewhere else actually? It feels a little odd here now.

apelisse · 2020-02-27T18:29:25Z

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

-		utilruntime.HandleError(fmt.Errorf("error building openapi models for %s: %v", crd.Name, err))
-		openAPIModels = nil
+		utilruntime.HandleError(fmt.Errorf("failed to building typeConverter for apply: %s: %v", crd.Name, err))
+		return nil, fmt.Errorf("the server could not properly serve the CR schema") // validation should avoid this


Even understanding the context, I'm not sure I understand what this error means?

This is the error we return in many other cases in this file, and then we log a more specific reason in the apiserver logs.

Not that we couldn't also return the specific error to the client

That seems like something people can take an action on? "It failed because of XYZ", at least they can figure out what to do, no? Agreed that if it's a bug that requires an update/bug-fix then it's not as useful.

If the validation of the crd's structural schema is done correctly, then this shouldn't ever be hit, so if it does get hit, then this would be due to a bug that requires an update/bug-fix

This isn't meant to catch structural schema errors, it's meant to catch errors where, the structural schema is valid and it still fails to convert

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

Lines 1257 to 1263 in bb677ae

s := &structuralschema.Structural{}

if structuralSchema, ok := structuralSchemas[v]; ok {

if len(structuralschema.ValidateStructural(nil, structuralSchema)) == 0 {

s = structuralSchema

}

}

err = builder.AddStructural(v, s)

rather than block all get/put/post/delete requests to the custom resource on this, I'd rather see us attempt this same operation in the CRD controller and add an error condition to the CRD if it fails, to surface the error to the CRD author (similar to what we do for structural errors)

I'm not against doing that for safety, but we don't expect this error to be able to be possibly triggered by users, since we ensure that only valid structural schemas are passed into the builder.AddStructural, which should only return an error if an invalid structural schema was passed in.

If a user sends the apiserver a CRD with a schema that fails structural validation, then we won't even try to pass it into the builder.AddStructural function (and we pass in an empty structural schema instead).

the surface area is large enough and the conversion complex enough that I think we should gain confidence with a non-blocking condition-adding approach before making it blocking

apelisse · 2020-02-27T18:33:35Z

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

 // and merges it with the models defined in the static OpenAPI spec.
-// Returns nil models if the ServerSideApply feature is disabled, or the static spec is nil, or an error is encountered.
-func buildOpenAPIModelsForApply(staticOpenAPISpec *spec.Swagger, crd *apiextensionsv1.CustomResourceDefinition) (proto.Models, error) {
+// Returns nil if the ServerSideApply feature is disabled, or the static spec is nil, or an error is encountered.


This is not something you just changed, but I think it's a little odd to read the code when you can return nil even if there is no error. That's not ideal. Especially since both these checks (feature enabled, staticOpenAPISpec == nil) could be done before calling the method. Would you mind moving these out?

k8s-ci-robot · 2020-03-04T19:30:22Z

@jennybuckley: PR needs rebase.

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.

apelisse · 2020-05-09T04:01:01Z

Quick question: is this only going to change the schemas that are known as structural, or all of them?

fejta-bot · 2020-08-07T04:46:01Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

apelisse · 2020-08-07T16:32:32Z

/remove-lifecycle stale

fejta-bot · 2020-11-05T17:03:19Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

fejta-bot · 2020-12-05T17:48:59Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

fejta-bot · 2021-01-04T18:33:50Z

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

k8s-ci-robot · 2021-01-04T18:34:03Z

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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.

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 6, 2020

k8s-ci-robot requested review from apelisse, jpbetz and sttts February 6, 2020 02:13

jennybuckley force-pushed the crd-smd-direct branch 2 times, most recently from b57f3e8 to 0c8bbbc Compare February 6, 2020 21:27

k8s-ci-robot added the area/dependency Issues or PRs related to dependency changes label Feb 6, 2020

jennybuckley force-pushed the crd-smd-direct branch from 0c8bbbc to 4368ba6 Compare February 7, 2020 23:54

k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Feb 7, 2020

jennybuckley force-pushed the crd-smd-direct branch 6 times, most recently from 4023fe4 to 6caad85 Compare February 10, 2020 23:34

jennybuckley changed the title ~~[WIP] Directly convert CRD structuralSchema to smdSchema~~ Directly convert CRD structuralSchema to smdSchema Feb 11, 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 Feb 11, 2020

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

Directly convert CRD structuralSchema to smdSchema

bb677ae

jennybuckley force-pushed the crd-smd-direct branch from 6caad85 to bb677ae Compare February 14, 2020 18:57

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 14, 2020

apelisse reviewed Feb 27, 2020

View reviewed changes

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

apelisse mentioned this pull request Mar 23, 2020

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

Closed

k8s-ci-robot added wg/api-expression Categorizes an issue or PR as relevant to WG API Expression. and removed wg/apply labels Apr 17, 2020

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 7, 2020

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 7, 2020

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 5, 2020

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Dec 5, 2020

k8s-ci-robot closed this Jan 4, 2021

apelisse mentioned this pull request Nov 14, 2022

testing fixture should support types.ApplyPatchType kubernetes/client-go#992

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Directly convert CRD structuralSchema to smdSchema #87872

Directly convert CRD structuralSchema to smdSchema #87872

jennybuckley commented Feb 6, 2020

apelisse commented Feb 7, 2020

k8s-ci-robot commented Feb 7, 2020

jennybuckley commented Feb 11, 2020

jennybuckley commented Feb 14, 2020

jennybuckley commented Feb 18, 2020

apelisse left a comment

apelisse Feb 27, 2020

apelisse Feb 27, 2020

jennybuckley Mar 3, 2020

apelisse Mar 4, 2020

jennybuckley Mar 4, 2020

jennybuckley Mar 4, 2020

jennybuckley Mar 4, 2020

liggitt Mar 4, 2020 •

edited

jennybuckley Mar 4, 2020

jennybuckley Mar 4, 2020

liggitt Mar 5, 2020

apelisse Feb 27, 2020

k8s-ci-robot commented Mar 4, 2020

apelisse commented May 9, 2020

fejta-bot commented Aug 7, 2020

apelisse commented Aug 7, 2020

fejta-bot commented Nov 5, 2020

fejta-bot commented Dec 5, 2020

fejta-bot commented Jan 4, 2021

k8s-ci-robot commented Jan 4, 2021

	s := &structuralschema.Structural{}
	if structuralSchema, ok := structuralSchemas[v]; ok {
	if len(structuralschema.ValidateStructural(nil, structuralSchema)) == 0 {
	s = structuralSchema
	}
	}
	err = builder.AddStructural(v, s)

Directly convert CRD structuralSchema to smdSchema #87872

Directly convert CRD structuralSchema to smdSchema #87872

Conversation

jennybuckley commented Feb 6, 2020

apelisse commented Feb 7, 2020

k8s-ci-robot commented Feb 7, 2020

jennybuckley commented Feb 11, 2020

jennybuckley commented Feb 14, 2020

jennybuckley commented Feb 18, 2020

apelisse left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Mar 4, 2020 • 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

k8s-ci-robot commented Mar 4, 2020

apelisse commented May 9, 2020

fejta-bot commented Aug 7, 2020

apelisse commented Aug 7, 2020

fejta-bot commented Nov 5, 2020

fejta-bot commented Dec 5, 2020

fejta-bot commented Jan 4, 2021

k8s-ci-robot commented Jan 4, 2021

liggitt Mar 4, 2020 •

edited