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

Adds x-kubernetes-map-type annotation as a CRD API extension #84113

Merged
merged 4 commits into from Oct 28, 2019

Conversation

@enxebre
Copy link
Member

enxebre commented Oct 19, 2019

What type of PR is this?

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

/kind api-change

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

What this PR does / why we need it:
Adds support for the x-kubernetes-map-type annotation by introducing XMapType to the CRD extensions

// x-kubernetes-map-type annotates an object to further describe its topology.
// This extension must only be used when type is object and may have 2 possible values:
//
// 1) `granular`:
//      These maps are actual maps (key-value pairs) and each fields are independent
//      from each other (they can each be manipulated by separate actors). This is
//      the default behaviour for all maps.
// 2) `atomic`: the list is treated as a single entity, like a scalar.
//      Atomic maps will be entirely replaced when updated.
// +optional

For more information on release notes see: https://git.k8s.io/community/contributors/guide/release-notes.md
-->

Introduce x-kubernetes-map-type annotation as a CRD API extension. Enables this particular validation for server-side apply.

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

https://github.com/kubernetes/enhancements/blob/master/keps/sig-api-machinery/0006-apply.md
https://github.com/kubernetes/enhancements/blob/master/keps/sig-api-machinery/20190425-structural-openapi.md
#73723

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Oct 19, 2019

Hi @enxebre. 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.

@k8s-ci-robot k8s-ci-robot requested review from davidopp and derekwaynecarr Oct 19, 2019
@enxebre enxebre changed the title Adds `x-kubernetes-map-type` annotation as a CRD API extension Adds x-kubernetes-map-type annotation as a CRD API extension Oct 19, 2019
@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Oct 19, 2019

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.

@enxebre

This comment has been minimized.

Copy link
Member Author

enxebre commented Oct 21, 2019

@@ -117,6 +117,19 @@ type JSONSchemaProps struct {
// Defaults to atomic for arrays.
// +optional
XListType *string `json:"x-kubernetes-list-type,omitempty" protobuf:"bytes,42,opt,name=xKubernetesListType"`

// x-kubernetes-map-type annotates a map to further describe its topology.
// This extension must only be used on maps and may have 2 possible values:

This comment has been minimized.

Copy link
@sttts

sttts Oct 21, 2019

Contributor

would refer to the type instead of "map":

This extension must only be used when type is object and may have 2 possible values

This comment has been minimized.

Copy link
@apelisse

apelisse Oct 21, 2019

Member

Right, on top of that, I would like to mention that we also want to support this extension on "go structures", which are also "object" types in openapi.

// the default for all maps.
// 2) `atomic`: the list is treated as a single entity, like a scalar.
// Atomic maps will be entirely replaced when updated.
// Defaults to map for maps.

This comment has been minimized.

Copy link
@sttts

sttts Oct 21, 2019

Contributor

This is also false for list-type: we don't default these. We have implicit default semantics, not default values. Would drop this line.

// 1) `map`:
// These maps are actual maps (key-value pairs) and each fields are independent
// from each other (they can each be manipulated by separate actors). This is
// the default for all maps.

This comment has been minimized.

Copy link
@sttts

sttts Oct 21, 2019

Contributor

This is the default behaviour for all maps.

},
},
wantError: false,
},

This comment has been minimized.

Copy link
@sttts

sttts Oct 21, 2019

Contributor

XMapType: strPtr("somethingElse") is missing as another test.

@sttts

This comment has been minimized.

Copy link
Contributor

sttts commented Oct 21, 2019

/ok-to-test

@sttts

This comment has been minimized.

Copy link
Contributor

sttts commented Oct 21, 2019

/assign @liggitt

For API review.

@sttts

This comment has been minimized.

Copy link
Contributor

sttts commented Oct 21, 2019

/assign @apelisse

@sttts sttts added this to the v1.17 milestone Oct 21, 2019
@enxebre enxebre force-pushed the enxebre:x-kubernetes-map-type branch from d4c03e4 to 6f27c5e Oct 21, 2019
@enxebre enxebre force-pushed the enxebre:x-kubernetes-map-type branch from ad32f6a to 29b1493 Oct 23, 2019
@enxebre

This comment has been minimized.

Copy link
Member Author

enxebre commented Oct 23, 2019

/test pull-kubernetes-e2e-gce

@enxebre

This comment has been minimized.

Copy link
Member Author

enxebre commented Oct 23, 2019

/test pull-kubernetes-kubemark-e2e-gce-big

@enxebre enxebre force-pushed the enxebre:x-kubernetes-map-type branch from 29b1493 to 2bee977 Oct 24, 2019
@enxebre

This comment has been minimized.

Copy link
Member Author

enxebre commented Oct 24, 2019

/test pull-kubernetes-integration

input: apiextensions.CustomResourceValidation{
OpenAPIV3Schema: &apiextensions.JSONSchemaProps{
Type: "array",
XMapType: strPtr("map"),

This comment has been minimized.

Copy link
@apelisse

apelisse Oct 24, 2019

Member

granular.

input: apiextensions.CustomResourceValidation{
OpenAPIV3Schema: &apiextensions.JSONSchemaProps{
Type: "object",
XMapType: strPtr("map"),

This comment has been minimized.

Copy link
@apelisse

apelisse Oct 24, 2019

Member

granular?

name: "unset type with map type extension set",
input: apiextensions.CustomResourceValidation{
OpenAPIV3Schema: &apiextensions.JSONSchemaProps{
XMapType: strPtr("map"),

This comment has been minimized.

Copy link
@apelisse

apelisse Oct 24, 2019

Member

granular :-)

}
}

if schema.XMapType != nil && *schema.XMapType != "atomic" && *schema.XMapType != "map" {

This comment has been minimized.

Copy link
@apelisse

apelisse Oct 24, 2019

Member

granular!

@apelisse

This comment has been minimized.

Copy link
Member

apelisse commented Oct 24, 2019

Small but important comment and then lgtm for me. Thank you!

@enxebre enxebre force-pushed the enxebre:x-kubernetes-map-type branch from 2bee977 to da61794 Oct 25, 2019
@enxebre

This comment has been minimized.

Copy link
Member Author

enxebre commented Oct 25, 2019

thanks @apelisse important indeed... I really want to move those into reusable constants. Updated.

@enxebre enxebre force-pushed the enxebre:x-kubernetes-map-type branch from da61794 to b598b3e Oct 25, 2019
@apelisse

This comment has been minimized.

Copy link
Member

apelisse commented Oct 25, 2019

Thanks @enxebre!

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Oct 25, 2019
@markyjackson-taulia

This comment has been minimized.

Copy link

markyjackson-taulia commented Oct 26, 2019

Bug triage for 1.17 here with a gentle reminder that code freeze for this release is on November 18. Is this issue still intended for 1.17?

@lavalamp

This comment has been minimized.

Copy link
Member

lavalamp commented Oct 28, 2019

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Oct 28, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enxebre, lavalamp

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

@k8s-ci-robot k8s-ci-robot merged commit e1dc959 into kubernetes:master Oct 28, 2019
15 checks passed
15 checks passed
cla/linuxfoundation enxebre authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
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-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-kind Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
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
tide In merge pool.
Details
@jennybuckley

This comment has been minimized.

Copy link
Contributor

jennybuckley commented Jan 8, 2020

I think we might need a follow up to this in kube-openapi, because it looks like the extension isn't wired all the way through: https://github.com/kubernetes/kube-openapi/blob/master/pkg/schemaconv/smd.go#L387-L388

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.