Add per version field section to custom resource webhook conversion KEP #2737

mbohlool · 2018-10-02T03:49:39Z

The defaulting section of customresource webhook conversion KEP was backward incompatible. This PR suggest a new approach. Different approaches as well as the problem description is detailed in this document.

@liggitt @lavalamp @deads2k @sttts @erictune @roycaihw

roycaihw · 2018-10-02T21:09:16Z

contributors/design-proposals/api-machinery/customresource-conversion-webhook.md

@@ -128,7 +128,25 @@ type CustomResourceConversionWebhook {
 ### Defaulting

 In case that there is no versions list, a single version with values defaulted to top level version will be created. That means a single version with a name set to spec.version.
-All newly added per version fields (schema, additionalPrinterColumns or subresources) will be defaulted to the coresponding top level field except for the first version in the list that will remain empty.
+None of the added per version fields (schema, additionalPrinterColumns or subresources) will be defaulted to the coresponding top level field except because that is a backward incompatible change.


roycaihw · 2018-10-02T21:24:57Z

contributors/design-proposals/api-machinery/customresource-conversion-webhook.md

+* *Validation*: Either top level X or per version X can be set not both.
+* *Mutation*: On update, if the top level field is set, clear all per version X.
+
+With this suggestion, there can be a single list in CRD-V1 (apiextensions.k8s.io/v1). If top level fields are set, they will be copied to all items in the list otherwise we use `spec.versions` list.


How are we going to distinguish round-tripping v1beta1->v1->v1beta1 between

v1beta1 CRD that sets top level X

v1beta1 CRD that sets same value to all per version X

Added a section in the linked doc: https://docs.google.com/document/d/1A6-q7l0M9L1HIqTzbB2Y-9YQ-ggL6HGO7CJSGCxJZF8/edit#bookmark=id.vm10z07du3o7

Let's discuss it there first and I will move it to this PR after.

roycaihw · 2018-10-09T00:16:05Z

contributors/design-proposals/api-machinery/customresource-conversion-webhook.md


 ### Support Level

 The feature will be alpha in the first implementation and will have a feature gate that is defaulted to false. The roll-back story with a feature gate is much more clear. if we have the features as alpha in kubernetes release Y (>X where the feature is missing) and we make it beta in kubernetes release Z, it is not safe to use the feature and downgrade from Y to X but the feature is alpha in Y which is fine. It is safe to downgrade from Z to Y (given that we enable the feature gate in Y) and that is desirable as the feature is beta in Z.
-
+On downgrading from a Z to Y, stored CRDs can have per version fields set. While the feature gate can be off on Y (alpha cluster), it is dangerous to disable per-version Schema Validation or Status subresources as it makes the status field mutable and validation on CRs will be disabled. Thus the feature gate in Y only protects adding per version fields not the actual behaviour. This way even if we downgrade from Z to Y, per version validations and subresources will be honored. This will not be the case for webhook conversion itself. The feature gate will also protect the implementation of webhook conversion and alpha cluster with disabled feature gate will return error for CRDs with webhook conversion (that are created with a future version of the cluster).


Maybe add how the feature gate is checked to this section or the section above?

I'd suggest that we document kubernetes/kubernetes#67521 (comment) somewhere as a recommended pattern for how to use feature gates, but I'm not sure if it applies to all features (probably not, e.g. for webhook conversion itself).

is this cover what you are asking? ... Thus the feature gate in Y only protects adding per version fields not the actual behaviour. ....

I was concerned if it's clear that we allows updating per version X when gate is off, if the CRD object already uses the per version X; also different from the other validation rules, I suppose this feature gate validation rule applies to the whole set instead of individual X’s?

E.g. when gate is off, for a CRD that already uses per version Schema, users can update the CRD to use per version Subresources from using top-level Subresources previously.

Maybe it's just me :) I'm willing to LGTM after the typo reported by CI is fixed.

Added more detail, does it look good now?

roycaihw · 2018-10-09T00:20:11Z

contributors/design-proposals/api-machinery/customresource-conversion-webhook.md

+
+Hence the solution is the two sets of fields cannot be set at the same time. These validation rules need to be applied:
+
+* Either top level X or per version X can be set not both. This rule applies to individual X’s not the whole set. E.g. top level Schema can be set while per version subresources are set.


E.g. top level Schema can be set while per version subresources are set.

+1

justaugustus · 2018-10-13T03:09:06Z

/kind kep

mbohlool · 2018-10-15T23:19:34Z

@deads2k @liggitt can you take another look please.

mbohlool · 2018-10-15T23:20:10Z

@roycaihw comment addressed. PTAL.

roycaihw · 2018-10-16T00:38:47Z

contributors/design-proposals/api-machinery/customresource-conversion-webhook.md


 ### Support Level

 The feature will be alpha in the first implementation and will have a feature gate that is defaulted to false. The roll-back story with a feature gate is much more clear. if we have the features as alpha in kubernetes release Y (>X where the feature is missing) and we make it beta in kubernetes release Z, it is not safe to use the feature and downgrade from Y to X but the feature is alpha in Y which is fine. It is safe to downgrade from Z to Y (given that we enable the feature gate in Y) and that is desirable as the feature is beta in Z.
+On downgrading from a Z to Y, stored CRDs can have per version fields set. While the feature gate can be off on Y (alpha cluster), it is dangerous to disable per-version Schema Validation or Status subresources as it makes the status field mutable and validation on CRs will be disabled. Thus the feature gate in Y only protects adding per version fields not the actual behaviour. Thus if the fearure gate is off in Y:
+
+* None of the per-version X cannot be set on create.


~~None of~~ or s/cannot/can/

roycaihw · 2018-10-16T00:58:09Z

contributors/design-proposals/api-machinery/customresource-conversion-webhook.md

+
+* None of the per-version X cannot be set on create.
+* None of the per-version X cannot be changed on update unless the update command remove them all.
+* Any update that does not change per-version X (if they exists) are allowed.


My understanding (of kubernetes/kubernetes#67521 (comment); please correct me if we are pursuing a different strategy) is that we allow updates that do change per-version X's if per-version X's exist.

Any update that does not change per-version X (if they exists) sounds like "patching top-level X (when per-version X exists)" to me, which would fail the mutual exclusivity validation.

I think these three rules can be simplified to this when the feature gate is disabled:

Per-version X cannot be set on CRD create (per-version fields are auto-cleared).

Per-version X can only be set/changed on CRD update unless the existing CRD object already has per-version X set.

"cannot be changed on update unless the update command remove them all" seems fiddly... can you describe a scenario that protects against? if it were my CRD, I'd rather be able to fix up one per-version field if needed.

I think when the feature gate is disabled, there should be something wrong with the feature and we should not less user guess what they can do to make it work. Any change to the feature may make it worse. But I don't feel strong about it and can go with your way too (except the auto-clear part).

on the auto-clear part, while we are doing that for other feature gates, I object to doing it here. End user of the cluster does not have access to feature gate and if admin disable a feature gate, Operators that installed before that act differently than operator installed after that and both installation goes successfully. This will just create confusion for people.

I agree to continue with auto-clear for now as we do it elsewhere.

roycaihw · 2018-10-17T19:41:29Z

Talked with @mbohlool offline, the purposed strategy for how to use feature gate is more strict compared with kubernetes/kubernetes#67521 (comment).

kubernetes/kubernetes#67521 (comment) will be discussed in sig-arch meeting. If we want to go with that strategy in future, it's safe (backward compatible) to relax the validation.

/lgtm

contributors/design-proposals/api-machinery/customresource-conversion-webhook.md

roycaihw · 2018-10-22T18:02:47Z

nit: please update the comments for top-level and per-version X's in API changes section.

mbohlool · 2018-10-22T21:18:15Z

@roycaihw @liggitt addressed your comments, PTAL.

roycaihw · 2018-10-22T22:42:57Z

/lgtm

liggitt

a few nits/clarifications. one substantive comment around behavior when the feature gate is disabled. lgtm otherwise.

liggitt · 2018-10-23T03:40:00Z

contributors/design-proposals/api-machinery/customresource-conversion-webhook.md


+* Either top level X or per version X can be set not both. This rule applies to individual X’s not the whole set. E.g. top level Schema can be set while per version subresources are set.


nits:
"...can be set, but not both."
lowercase "Schema"
s/per version/per-version/g

liggitt · 2018-10-23T03:41:37Z

contributors/design-proposals/api-machinery/customresource-conversion-webhook.md

+
+#### Alternative approaches considered
+
+First a defaulting approach is considered which per version fields would be defaulted to top level fields. but that will breaks backward incompatible change; Quoting from API [guidelines](https://github.com/kubernetes/community/blob/master/contributors/devel/api_changes.md#backward-compatibility-gotchas):


s/will breaks/breaks/

liggitt · 2018-10-23T03:44:15Z

contributors/design-proposals/api-machinery/customresource-conversion-webhook.md

+
+There are other solution considered that does not involved defaulting:
+
+* Field Discriminator: Use `Spec.Conversion.Strategy` as discriminator to decide which set of fields to use. This approach would work but the proposed solution is keeping the mutual excusivity in a broader sense and is preferred.


This approach would work but the proposed solution is keeping the mutual excusivity in a broader sense and is preferred.

I think the main benefit of the mutual exclusivity over the strategy-based discriminator is that it makes the API less surprising (accepting schema/subresource data and then not making use of it because the conversion strategy was set in a particular way is surprising).

well the point was validating here too. we won't accept anything that we don't use. we validate based on strategy field and use what we validated. e.g. if it is none we only let top level X to be set and we use top level X. if it is not None, then we only allow per version fields to be set and we use those.

liggitt · 2018-10-23T04:08:05Z

contributors/design-proposals/api-machinery/customresource-conversion-webhook.md


 ### Support Level

 The feature will be alpha in the first implementation and will have a feature gate that is defaulted to false. The roll-back story with a feature gate is much more clear. if we have the features as alpha in kubernetes release Y (>X where the feature is missing) and we make it beta in kubernetes release Z, it is not safe to use the feature and downgrade from Y to X but the feature is alpha in Y which is fine. It is safe to downgrade from Z to Y (given that we enable the feature gate in Y) and that is desirable as the feature is beta in Z.
+On downgrading from a Z to Y, stored CRDs can have per version fields set. While the feature gate can be off on Y (alpha cluster), it is dangerous to disable per-version Schema Validation or Status subresources as it makes the status field mutable and validation on CRs will be disabled. Thus the feature gate in Y only protects adding per version fields not the actual behaviour. Thus if the fearure gate is off in Y:


s/fearure/feature/

feature is a valid word :) cool.

liggitt · 2018-10-23T04:22:34Z

contributors/design-proposals/api-machinery/customresource-conversion-webhook.md

+
+* None of the per-version X cannot be set on create.
+* None of the per-version X cannot be changed on update unless the update command remove them all.
+* Any update that does not change per-version X (if they exists) are allowed.


I think these three rules can be simplified to this when the feature gate is disabled:

Per-version X cannot be set on CRD create (per-version fields are auto-cleared).

Per-version X can only be set/changed on CRD update unless the existing CRD object already has per-version X set.

"cannot be changed on update unless the update command remove them all" seems fiddly... can you describe a scenario that protects against? if it were my CRD, I'd rather be able to fix up one per-version field if needed.

mbohlool · 2018-10-23T20:47:01Z

@liggitt PTAL

liggitt · 2018-10-23T21:37:33Z

/lgtm

liggitt · 2018-10-23T23:09:10Z

/approve

lavalamp · 2018-10-24T00:00:55Z

/approve

k8s-ci-robot · 2018-10-24T00:01:01Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~contributors/design-proposals/api-machinery/OWNERS~~ [lavalamp]

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

erictune · 2018-10-24T04:16:13Z

🎉 thank you everyone who contributed to working out the design around defaulting!

…lting Add per version field section to custom resource webhook conversion KEP

k8s-ci-robot requested review from deads2k and lavalamp October 2, 2018 03:49

roycaihw reviewed Oct 2, 2018

View reviewed changes

mbohlool force-pushed the crd_conversion_defaulting branch from 567ac61 to 794d7b9 Compare October 8, 2018 19:58

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 8, 2018

mbohlool force-pushed the crd_conversion_defaulting branch from 794d7b9 to cafe4c1 Compare October 8, 2018 20:01

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 8, 2018

roycaihw reviewed Oct 9, 2018

View reviewed changes

k8s-ci-robot added the kind/kep label Oct 13, 2018

mbohlool force-pushed the crd_conversion_defaulting branch from cafe4c1 to 7ccdea7 Compare October 15, 2018 23:18

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 15, 2018

mbohlool force-pushed the crd_conversion_defaulting branch from 7ccdea7 to 44ca73f Compare October 15, 2018 23:23

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

roycaihw reviewed Oct 16, 2018

View reviewed changes

mbohlool force-pushed the crd_conversion_defaulting branch from 44ca73f to cc77ce7 Compare October 17, 2018 18:44

k8s-ci-robot assigned roycaihw Oct 17, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 17, 2018

liggitt reviewed Oct 19, 2018

View reviewed changes

contributors/design-proposals/api-machinery/customresource-conversion-webhook.md Show resolved Hide resolved

roycaihw mentioned this pull request Oct 19, 2018

CRD supports multi-version Schema kubernetes/kubernetes#67553

Closed

mbohlool force-pushed the crd_conversion_defaulting branch 2 times, most recently from 072001f to da16453 Compare October 22, 2018 21:17

mbohlool force-pushed the crd_conversion_defaulting branch from da16453 to 924ffb1 Compare October 22, 2018 21:20

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 22, 2018

mbohlool force-pushed the crd_conversion_defaulting branch from 924ffb1 to 0f9b999 Compare October 22, 2018 21:52

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 22, 2018

liggitt reviewed Oct 23, 2018

View reviewed changes

liggitt mentioned this pull request Oct 23, 2018

Webhook Conversion for Custom Resources kubernetes/enhancements#598

Closed

mbohlool force-pushed the crd_conversion_defaulting branch from 0f9b999 to 570a4fa Compare October 23, 2018 20:46

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 23, 2018

Add per version field section to custom resource webhook conversion KEP

25ae80b

mbohlool force-pushed the crd_conversion_defaulting branch from 570a4fa to 25ae80b Compare October 23, 2018 21:33

k8s-ci-robot assigned liggitt Oct 23, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 23, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 24, 2018

k8s-ci-robot merged commit 04ff155 into kubernetes:master Oct 24, 2018

roycaihw mentioned this pull request Oct 25, 2018

CRD supports multi-version Schema, Subresources and AdditionalPrintColumns kubernetes/kubernetes#70211

Merged

liggitt mentioned this pull request Nov 9, 2018

Improvements to CRD webhook conversion KEP #2484

Closed

MadhavJivrajani pushed a commit to MadhavJivrajani/community that referenced this pull request Nov 30, 2021

Merge pull request kubernetes#2737 from mbohlool/crd_conversion_defau…

3c6ff67

…lting Add per version field section to custom resource webhook conversion KEP

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add per version field section to custom resource webhook conversion KEP #2737

Add per version field section to custom resource webhook conversion KEP #2737

mbohlool commented Oct 2, 2018

roycaihw Oct 2, 2018

roycaihw Oct 2, 2018 •

edited

Loading

mbohlool Oct 4, 2018

roycaihw Oct 9, 2018

mbohlool Oct 15, 2018

roycaihw Oct 15, 2018

mbohlool Oct 15, 2018

roycaihw Oct 9, 2018

justaugustus commented Oct 13, 2018

mbohlool commented Oct 15, 2018

mbohlool commented Oct 15, 2018

roycaihw Oct 16, 2018

roycaihw Oct 16, 2018

liggitt Oct 23, 2018

mbohlool Oct 23, 2018

roycaihw commented Oct 17, 2018

roycaihw commented Oct 22, 2018

mbohlool commented Oct 22, 2018

roycaihw commented Oct 22, 2018

liggitt left a comment

liggitt Oct 23, 2018

liggitt Oct 23, 2018

liggitt Oct 23, 2018

mbohlool Oct 23, 2018

liggitt Oct 23, 2018

mbohlool Oct 23, 2018

liggitt Oct 23, 2018

mbohlool commented Oct 23, 2018

liggitt commented Oct 23, 2018

liggitt commented Oct 23, 2018

lavalamp commented Oct 24, 2018

k8s-ci-robot commented Oct 24, 2018

erictune commented Oct 24, 2018


		Hence the solution is the two sets of fields cannot be set at the same time. These validation rules need to be applied:

		* Either top level X or per version X can be set not both. This rule applies to individual X’s not the whole set. E.g. top level Schema can be set while per version subresources are set.


		#### Alternative approaches considered

		First a defaulting approach is considered which per version fields would be defaulted to top level fields. but that will breaks backward incompatible change; Quoting from API [guidelines](https://github.com/kubernetes/community/blob/master/contributors/devel/api_changes.md#backward-compatibility-gotchas):


		There are other solution considered that does not involved defaulting:

		* Field Discriminator: Use `Spec.Conversion.Strategy` as discriminator to decide which set of fields to use. This approach would work but the proposed solution is keeping the mutual excusivity in a broader sense and is preferred.

Add per version field section to custom resource webhook conversion KEP #2737

Add per version field section to custom resource webhook conversion KEP #2737

Conversation

mbohlool commented Oct 2, 2018

Choose a reason for hiding this comment

roycaihw Oct 2, 2018 • edited Loading

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

justaugustus commented Oct 13, 2018

mbohlool commented Oct 15, 2018

mbohlool commented Oct 15, 2018

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

roycaihw commented Oct 17, 2018

roycaihw commented Oct 22, 2018

mbohlool commented Oct 22, 2018

roycaihw commented Oct 22, 2018

liggitt 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

mbohlool commented Oct 23, 2018

liggitt commented Oct 23, 2018

liggitt commented Oct 23, 2018

lavalamp commented Oct 24, 2018

k8s-ci-robot commented Oct 24, 2018

erictune commented Oct 24, 2018

roycaihw Oct 2, 2018 •

edited

Loading