apiextensions: validate list-type map+set uniqueness in CRs #84920

sttts · 2019-11-07T13:26:11Z

/kind api-change
/kind bug

Fixed missing validation of uniqueness of list items in lists with `x-kubernetes-list-type: map` or x-kubernetes-list-type: set` in CustomResources.

jennybuckley · 2019-11-07T21:18:38Z

/assign

fejta-bot · 2019-11-07T22:07:44Z

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.

staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/strategy.go

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

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/listtype/validation_test.go

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/listtype/validation.go

jtslear · 2020-02-05T12:34:34Z

Hello @liggitt, @sttts
Bug Triage team here for the 1.18 release. This is a friendly reminder that code freeze is scheduled for 5 March. Is this PR appears to be blocking #84724, is this still intended for milestone 1.18?

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/listtype/validation.go

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/listtype/validation_test.go

sttts · 2020-02-19T08:00:58Z

@apelisse addressed your comments.

liggitt · 2020-02-24T20:46:05Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/listtype/validation.go

+		return nil, nil
+	}
+	if len(obj) == 2 {
+		if reflect.DeepEqual(obj[0], obj[1]) {


are there cases where this would return false for complex array or map objects, but json-marshalling and comparing would be identical?

json.Marshal is the inverse of json.Unmarshal. If both values are the result of json.Unmarshal then this will not happen.

What do you have in mind? Our patch logic?

more issues like json-iterator/go#323, where json-iterator normalizes on marshal as opposed to encoding/json normalizing on both unmarshal/marshal

ic. Suggestion? Marshal again?

that is definitely simpler to reason about; that's what I would do to start

apelisse · 2020-02-24T22:24:28Z

I have a couple of additional comments.

I'm really not convinced that we want listMapKeys fields to be anything but required. Having a "nil" item validate because both keys are missing and transforming this into a valid case really feels wrong. As far as I know, we have one such case in Kubernetes, that's the protocol in the list of ports of the podspec. We might find a way to fix that one specifically, but I'd rather not generalize this problem. Alternatively, we could allow these to be if they have a default value, and in this case consider the default as the key value. IOW, "required or has a default" (that would also work for the protocol problem).

Second, I was discussion ratcheting with Daniel, and forcing the created objects to pass this validation only works if we assume that no controller/system is currently creating invalid objects. This change would break such a controller?

sttts · 2020-02-25T08:42:12Z

I'm really not convinced that we want listMapKeys fields to be anything but required. Having a "nil" item validate because both keys are missing and transforming this into a valid case really feels wrong. As far as I know, we have one such case in Kubernetes, that's the protocol in the list of ports of the podspec. We might find a way to fix that one specifically, but I'd rather not generalize this problem. Alternatively, we could allow these to be if they have a default value, and in this case consider the default as the key value. IOW, "required or has a default" (that would also work for the protocol problem).

All this is done via ratcheting validation, i.e. we enforce required or a default. We forbid nullable: #88076. Please review.

sttts · 2020-02-25T08:43:39Z

Second, I was discussion ratcheting with Daniel, and forcing the created objects to pass this validation only works if we assume that no controller/system is currently creating invalid objects. This change would break such a controller?

This is what we always do in those cases, ratcheting validation. It is the very reason why this issue is marked urgent for months. The more we wait (1.18 is around the corner) the more controllers will exist. I bet the number is zero today.

liggitt · 2020-02-25T13:27:57Z

Anyone defining a CRD that uses list-type: map/set must be able to rely on the documented semantics (we rely on them ourselves in server-side-apply). It is very important we not allow data that violates those semantics, and do so as soon as possible.

liggitt · 2020-02-25T13:37:18Z

Alternatively, we could allow these to be if they have a default value, and in this case consider the default as the key value. IOW, "required or has a default" (that would also work for the protocol problem).

That's what we settled on. Disallowing nullable and making the key attributes either be required or defaulted gives this set validation reasonable inputs to work with, and allows evolution or recovery from underspecified key sets (e.g. an underspecified item schema for a port realizes in the future it needs a protocol field that should be added to the item key specification, and needs to provide a default for existing persisted data that lacked that field).

liggitt · 2020-02-25T14:39:54Z

/lgtm
/approve

k8s-ci-robot · 2020-02-25T14:41:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, 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~~ [liggitt,sttts]
~~vendor/OWNERS~~ [liggitt,sttts]

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 · 2020-02-25T17:40:50Z

New changes are detected. LGTM label has been removed.

fejta-bot · 2020-02-25T23:49:57Z

/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.

fejta-bot · 2020-02-26T02:38:00Z

/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.

sttts assigned liggitt and apelisse Nov 7, 2019

k8s-ci-robot requested review from deads2k and smarterclayton November 7, 2019 13:27

k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Nov 7, 2019

sttts force-pushed the sttts-cr-list-type-set-map-validation branch from 590c837 to 5cc374e Compare November 7, 2019 13:29

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 7, 2019

sttts force-pushed the sttts-cr-list-type-set-map-validation branch from 5cc374e to 7be2000 Compare November 7, 2019 17:22

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 7, 2019

sttts force-pushed the sttts-cr-list-type-set-map-validation branch from 7be2000 to 26b38de Compare November 7, 2019 20:39

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 7, 2019

sttts changed the title ~~WIP: apiextensions: validate list-type map+set uniqueness in CRs~~ apiextensions: validate list-type map+set uniqueness in CRs Nov 7, 2019

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 Nov 7, 2019

k8s-ci-robot assigned jennybuckley Nov 7, 2019

liggitt reviewed Nov 13, 2019

View reviewed changes

liggitt reviewed Jan 23, 2020

View reviewed changes

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/listtype/validation.go Show resolved Hide resolved

eloyekunle mentioned this pull request Feb 12, 2020

Extended CRD Validation #88076

Merged

apelisse reviewed Feb 19, 2020

View reviewed changes

sttts force-pushed the sttts-cr-list-type-set-map-validation branch from 0965aee to 5c82b80 Compare February 19, 2020 08:02

liggitt reviewed Feb 24, 2020

View reviewed changes

sttts force-pushed the sttts-cr-list-type-set-map-validation branch 4 times, most recently from 66f5e8c to 99b4ec1 Compare February 25, 2020 14:31

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

apiextions: add list-type: map|set CR validation

ea45da7

sttts force-pushed the sttts-cr-list-type-set-map-validation branch from 99b4ec1 to ea45da7 Compare February 25, 2020 17:35

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 25, 2020

sttts added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 25, 2020

k8s-ci-robot merged commit ddf53b8 into kubernetes:master Feb 26, 2020

donbowman mentioned this pull request Apr 6, 2020

kubernetes 1.18, kubectl apply --validate=yes can't install CRD prometheus-operator/prometheus-operator#3122

Closed

amisevsk mentioned this pull request Apr 28, 2020

[workspace-controller] CRDs are invalid on kubernetes >=1.18 eclipse-che/che#16791

Open

liggitt mentioned this pull request Jul 6, 2020

HNC: Review API for sub-project graduation kubernetes-retired/multi-tenancy#748

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

apiextensions: validate list-type map+set uniqueness in CRs #84920

apiextensions: validate list-type map+set uniqueness in CRs #84920

sttts commented Nov 7, 2019 •

edited

Loading

jennybuckley commented Nov 7, 2019

fejta-bot commented Nov 7, 2019

jtslear commented Feb 5, 2020

sttts commented Feb 19, 2020

liggitt Feb 24, 2020 •

edited

Loading

sttts Feb 24, 2020

liggitt Feb 24, 2020

sttts Feb 25, 2020

liggitt Feb 25, 2020

apelisse commented Feb 24, 2020

sttts commented Feb 25, 2020

sttts commented Feb 25, 2020

liggitt commented Feb 25, 2020

liggitt commented Feb 25, 2020

liggitt commented Feb 25, 2020

k8s-ci-robot commented Feb 25, 2020

k8s-ci-robot commented Feb 25, 2020

fejta-bot commented Feb 25, 2020

fejta-bot commented Feb 26, 2020

apiextensions: validate list-type map+set uniqueness in CRs #84920

apiextensions: validate list-type map+set uniqueness in CRs #84920

Conversation

sttts commented Nov 7, 2019 • edited Loading

jennybuckley commented Nov 7, 2019

fejta-bot commented Nov 7, 2019

jtslear commented Feb 5, 2020

sttts commented Feb 19, 2020

liggitt Feb 24, 2020 • edited Loading

Choose a reason for hiding this comment

sttts Feb 24, 2020

Choose a reason for hiding this comment

liggitt Feb 24, 2020

Choose a reason for hiding this comment

sttts Feb 25, 2020

Choose a reason for hiding this comment

liggitt Feb 25, 2020

Choose a reason for hiding this comment

apelisse commented Feb 24, 2020

sttts commented Feb 25, 2020

sttts commented Feb 25, 2020

liggitt commented Feb 25, 2020

liggitt commented Feb 25, 2020

liggitt commented Feb 25, 2020

k8s-ci-robot commented Feb 25, 2020

k8s-ci-robot commented Feb 25, 2020

fejta-bot commented Feb 25, 2020

fejta-bot commented Feb 26, 2020

sttts commented Nov 7, 2019 •

edited

Loading

liggitt Feb 24, 2020 •

edited

Loading