Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use CRD validation field in server-side apply #77354

Merged
merged 7 commits into from Aug 31, 2019

Conversation

@jennybuckley
Copy link
Contributor

commented May 2, 2019

What type of PR is this?
/kind feature
/sig api-machinery
/wg apply
/cc @apelisse @kwiesmueller

Which issue(s) this PR fixes:
Related to #73723

Does this PR introduce a user-facing change?:

Server-side apply will now use the openapi provided in the CRD validation field to help figure out how to correctly merge objects and update ownership.
@apelisse

This comment has been minimized.

Copy link
Member

commented May 2, 2019

/assign @apelisse

@jennybuckley jennybuckley force-pushed the jennybuckley:crd-apply branch from 19d7253 to badb38d May 3, 2019
@jennybuckley

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

/test pull-kubernetes-e2e-gce-alpha-features

@sttts sttts referenced this pull request Jul 5, 2019
2 of 2 tasks complete
@jennybuckley jennybuckley force-pushed the jennybuckley:crd-apply branch from badb38d to bb6ea02 Aug 8, 2019
@jennybuckley

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

Just rebasing this to see what happens

@jennybuckley jennybuckley force-pushed the jennybuckley:crd-apply branch from bb6ea02 to b3425e1 Aug 12, 2019
@jennybuckley jennybuckley force-pushed the jennybuckley:crd-apply branch 5 times, most recently from 6d911fc to 4121b8d Aug 12, 2019
@jennybuckley

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2019

/retest

1 similar comment
@jennybuckley

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2019

/retest

}

if schema.Items != nil && schema.Items.Schema == nil {
allErrs = append(allErrs, field.Invalid(fldPath.Child("items"), schema.Items, "must only have a single schema if x-kubernetes-list-type is map"))

This comment has been minimized.

Copy link
@sttts

sttts Aug 30, 2019

Contributor

we don't support multiple schemas. They are invalidated elsewhere.

This comment has been minimized.

Copy link
@lavalamp

lavalamp Aug 30, 2019

Member

Can we fix in a followup?

This comment has been minimized.

Copy link
@sttts

sttts Aug 30, 2019

Contributor

we can

for _, k := range schema.XListMapKeys {
if s, ok := schema.Items.Schema.Properties[k]; ok {
if s.Type == "array" || s.Type == "object" {
allErrs = append(allErrs, field.Invalid(fldPath.Child("items").Child("properties").Child(k).Child("type"), schema.Items.Schema.Type, "must be a scalar type if parent array's x-kubernetes-list-type is map"))

This comment has been minimized.

Copy link
@sttts

sttts Aug 30, 2019

Contributor

Key(k)

This comment has been minimized.

Copy link
@lavalamp

lavalamp Aug 30, 2019

Member

In a followup?

This comment has been minimized.

Copy link
@sttts

sttts Aug 30, 2019

Contributor

sure

This comment has been minimized.

Copy link
@tedyu

tedyu Aug 31, 2019

Contributor

See #82210

keys := map[string]struct{}{}
for _, k := range schema.XListMapKeys {
if s, ok := schema.Items.Schema.Properties[k]; ok {
if s.Type == "array" || s.Type == "object" {

This comment has been minimized.

Copy link
@sttts

sttts Aug 30, 2019

Contributor

nullable scalars are ok?

This comment has been minimized.

Copy link
@lavalamp

This comment has been minimized.

Copy link
@jennybuckley

jennybuckley Aug 30, 2019

Author Contributor

Technically anything is okay from smd's perspective, but to keep in line with kubernetes types we did this

// These lists are like maps in that their elements have a non-index key
// used to identify them. Order is preserved upon merge. The map tag
// must only be used on a list with elements of type object.
// Defaults to atomic for arrays.

This comment has been minimized.

Copy link
@lavalamp

lavalamp Aug 30, 2019

Member

In a followup: clarify that this is the behavior, not a literal api default.

@lavalamp

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

As best I can tell, it looks like Jordan's last set of comments have been addressed. I think everything else can be fixed in followups (double checked w/ @sttts on slack). It would be best if we didn't force CRD users to have the default behavior for the entire release.

/approve
/lgtm

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@jennybuckley

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2019

/hold cancel
I think everything is addressed now

@jennybuckley

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2019

/retest

@lavalamp

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

/milestone v1.16

@k8s-ci-robot k8s-ci-robot added this to the v1.16 milestone Aug 30, 2019
@apelisse

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

@kacole2 Looks like the PR was missing the milestone, do we need to file anything to get this merged?

@liggitt

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

As best I can tell, it looks like Jordan's last set of comments have been addressed.

yes, lgtm

@fejta-bot

This comment has been minimized.

Copy link

commented Aug 31, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit ab162cd into kubernetes:master Aug 31, 2019
24 checks passed
24 checks passed
cla/linuxfoundation jennybuckley authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-conformance-kind-ipv6 Job succeeded.
Details
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
s, err := builder.BuildSwagger(crd, v.Name, builder.Options{V2: false, StripDefaults: true, StripValueValidation: true})
if err != nil {
utilruntime.HandleError(err)
return nil, fmt.Errorf("the server could not properly serve the CR schema")

This comment has been minimized.

Copy link
@tedyu

tedyu Sep 2, 2019

Contributor

I think using errors.Wrapf is better, preserving err.

This comment has been minimized.

Copy link
@lavalamp

lavalamp Sep 3, 2019

Member

This repeats a pattern from earlier in the function, see commented blocks above. It seems that errors are deliberately suppressed here. e.g., they're not errors the user will gain benefit from seeing. @sttts, is there a reason for that?

@liggitt

This comment has been minimized.

Copy link
Member

commented Sep 6, 2019

the way this builds openapi specs using the kube-openapi parser has caused a regression in CRD serving. see #82436 for details

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.