Update alpha field guidance #869

liggitt · 2017-08-01T17:13:46Z

Summarizing outcome of discussions with @kubernetes/sig-api-machinery-misc

ref:

liggitt · 2017-08-01T17:26:20Z

cc @deads2k @lavalamp @smarterclayton @bgrant0607 as those involved in the api-machinery meetings around this

@kubernetes/sig-api-machinery-pr-reviews @kubernetes/sig-api-machinery-proposals @kubernetes/api-reviewers

deads2k · 2017-08-01T18:05:33Z

contributors/devel/api_changes.md

+    }
+    ```
+
+3. In validation, ensure that the new field is rejected if the feature gate is disabled:


Doing this would make turning on the feature-gate a one way operation since future updates (even updates that attempt to delete the thing from GC) would fail.

gates are for alphas, and we have no strong guarantees for alpha anyway?

gates are for alphas, and we have no strong guarantees for alpha anyway?

If you had an alpha field on a Deployment, then you wanted to turn it off, after you turned it off, you would not be able to touch the deployment again and might not even be able to update status on the objects. In fact, because of the way validation and GC works, you may not even be able to delete the object after turning on the alpha field.

The only "fix" is to rewrite the objects and hope no one races you in. This becomes really problematic for alpha fields that have reflections in status which are controller maintained. In such a case, I don't see how you turn alpha back off with this validation requirement.

I'm not saying we can't do it, but the warnings around this need to be explicit. I doubt that its an obvious repercussion of the approach for many/most people.

There is a similar issue with enabling an alpha version... a cluster admin has to be aware that they might have to delete all alpha data before upgrading or they can get stuck with undecodeable content in etcd (if the alpha version was removed in the upgraded version, which is allowed).

I'll update the text to be explicit, but enabling an alpha version or an alpha feature puts more burdens on a cluster admin, and I'm ok with that.

Rejection when specifying an unknown field is incompatible with expected behavior.

Strong schema validation needs to be optional, and I don't see a reason to treat disabled alpha fields differently. Discussed further here:
kubernetes/kubernetes#5889

More consistent behavior would be to ensure that the contents were dropped when the field is disabled.

Rejection when specifying an unknown field is incompatible with expected behavior.

More consistent behavior would be to ensure that the contents were dropped when the field is disabled.

good point, will update, though I'll have to think about where we'd recommend the clearing be done, especially for fields in reused types (like PodSpec). There's not a great single place to clear the field based on a feature gate:

PrepareForCreate/PrepareForUpdate would have to be set in every resthandler that uses the type, and it'd be tough to ensure we didn't miss any

Validation functions shouldn't mutate

defaulting or conversion would affect components other than the API server

More consistent behavior would be to ensure that the contents were dropped when the field is disabled.

There are three types of alpha fields:

new additive fields an old server would ignore while still accepting the rest of the object.

new fields in a union type (e.g. local persistent volume source) that would cause an old server to reject the object (since it did not have a recognized field in the union type)

new values for an enumerated field an old server would reject.

I think only the first type should be dropped. The other two should fail validation.

Opened kubernetes/kubernetes#50924 for the existing feature-gated alpha fields we have to drop contents of disabled alpha fields on create/update

MHBauer · 2017-08-01T18:12:23Z

contributors/devel/api_changes.md

 }
 ```

 The latter requires that all objects in the same API group as `Frobber` to be
-replicated in the new version, `v6alpha2`. This also requires user to use a new
+replicated in the new version, `v7alpha1`. This also requires user to use a new


I think this particular example offers a strong amount of guidance.

before it seemed to suggest taking an api 'back to alpha', chronologically, like so:

v6alpha1 v6beta1 v6 v6alpha2

and now what makes more sense to me to suggest a main version bump, again chronologically

v6alpha1 v6beta1 v6 v7alpha1

agree, starting v6alpha2 after v6 exists wouldn't make sense.

It was recently pointed out to me that we don't really document this API versioning convention anywhere.

MHBauer · 2017-08-01T18:13:50Z

contributors/devel/api_changes.md

-metadata:
-  name: myfrobber
-  annotations:
-    frobbing.alpha.kubernetes.io/width: 2


Is this annotation process no longer advocated? @cmluciano !!

it is not advocated. see the links in this PR's description for the problems. the alternatives to the approach described in this PR are:

use annotations

exposes you to time-bomb data added while running an earlier version of the server

no way for an admin to control use

if the field graduates to beta/stable it requires breaking clusters using alpha fields or trying to migrate/surface the options in both an annotation and field at the same time (which doesn't work)

use fields with "alpha" in the name

if the field graduates to beta/stable it requires breaking clusters using alpha fields or trying to migrate/surface the options in two fields at the same time (which doesn't work)

use fields without "alpha" in the name and allow changing them in incompatible ways

breaks old servers and/or clients if the schema changes on the way to beta/stable

I think we need to mention the old approach (and that it is no longer recommended/allowed for new features), since it will still exist in a number of places for some time to come.

MHBauer

Is there a use of this feature gate process I can look at?

liggitt · 2017-08-01T18:19:29Z

Is there a use of this feature gate process I can look at?

it was used for the priority fields added to the pod spec in kubernetes/kubernetes@91f893e

seh · 2017-08-01T18:57:41Z

contributors/devel/api_changes.md

+  // param ...
+  Param  string `json:"param" protobuf:"bytes,2,opt,name=param"`
+  // width ...
+  Width  *int32 `json:"width,omitempty" protobuf:"varint,3,opt,name=width"`


Do you prescribe adding the new field to the end of the Go struct, with the intention of allowing existing "unkeyed" composite literals to remain valid when recompiled, or do you mandate use keyed elements so that existing code can't impose assumptions about field order?

Do you prescribe adding the new field to the end of the Go struct, with the intention of allowing existing "unkeyed" composite literals to remain valid when recompiled

No, unkeyed literal compatibility is impossible when adding new fields.

Adding new fields at the end just makes it easier to see the incrementing protobuf tag.

Ah, yes, a trip to the Go Playground confirmed what I had forgotten. I was thinking that that an unkeyed composite literal could get away with specifying a prefix of the fields, but then this is where that trick of using a field called "_" comes in, defending against callers using unkeyed literals.

thockin

If this is the new guidance ( I thought we had larger concerns with this approach when we discussed it last year) we need to spread the word FAR and WIDE.

Are we comfortable with the idea that an alpha feature can burn a field name basically forever? Do we need alpha fields to be named specially? Beta seems like a safer bet...

thockin · 2017-08-02T00:09:37Z

contributors/devel/api_changes.md

+2. Add the field to the API type:
+
+    * mark the field `omitempty` and optional
+    * ensure the field is entirely absent from the API when empty (might require making the field a pointer)


all optionals should be pointers or otherwise nil-able

Link to the documentation on optional fields:

https://github.com/kubernetes/community/blob/master/contributors/devel/api-conventions.md#optional-vs-required

thockin · 2017-08-02T00:12:11Z

contributors/devel/api_changes.md

+    }
+    ```
+
+3. In validation, ensure that the new field is rejected if the feature gate is disabled:


gates are for alphas, and we have no strong guarantees for alpha anyway?

liggitt · 2017-08-02T00:31:51Z

If this is the new guidance ( I thought we had larger concerns with this approach when we discussed it last year) we need to spread the word FAR and WIDE.

Agree. This is the start. The alternatives (annotations, field with alpha-name, field without alpha-name that allows changing the schema) all have major downsides.

To be clear, this PR is an attempt to summarize the outcomes of the linked discussions for general consumption. Please correct my understanding if I'm misrepresenting those in any way.

Are we comfortable with the idea that an alpha feature can burn a field name basically forever?

I don't think we have much of an option... if you name the field specially, there is no good path to beta/stable that doesn't summarily break everyone using the alpha field. If you don't name it specially, and just change the schema, you break decoding of new beta data by old clients and servers with inert alpha fields in their internal structs.

liggitt · 2017-08-02T00:32:42Z

gates are for alphas, and we have no strong guarantees for alpha anyway?

gates ensure the server doesn't accept alpha data without opt-in, since it can drive unwanted or unstable behavior

DirectXMan12 · 2017-08-04T20:27:23Z

contributors/devel/api_changes.md

+In future versions:
+
+* if the feature progresses to beta or stable status, the feature gate can simply be enabled by default.
+* if the schema of the alpha field must change in an incompatible way, a new field name must be used.


so, if we're working with a new alpha field Quuxer, and, while still in alpha, we want to change the value type from int to resource.Quantity, we have to abandon the name Quuxer forever? That seems to partially defeat the purpose of alpha...

to be clear, this is talking about changing an alpha field that has already been included in a release.

until we generate servers and clients with alpha fields completely absent from the go structs, changing the schema breaks old clients and servers:

an old client with the inert alpha field quxxer int in its go struct would fail to decode a server sending data including the beta field "quxxer":"1m", which breaks forward client compatibility

an old server with the inert alpha field quxxer int in its go struct would fail to decode a client sending data including the beta field "quxxer":"1m", which breaks backward client compatibility

This is admittedly a compromise, but the alternative discussed was to always prefix alpha fields with alpha, which would always incur breakage when transitioning out of alpha, as it is not possible to support multiple representations of the same attribute in the same API version.

https://github.com/kubernetes/community/blob/master/contributors/devel/api_changes.md#backward-compatibility-gotchas

bgrant0607 · 2017-08-09T19:44:18Z

Also ref kubernetes/kubernetes#4855

bgrant0607 · 2017-08-09T19:50:55Z

contributors/devel/api_changes.md

+
+The preferred approach adds an alpha field to the existing object, and ensures it is disabled by default.
+
+1. Add a feature gate to the API server to control enablement of the new field (and associated function):


A link to the file or at least a string to search for would be useful.

https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/util/feature/feature_gate.go

bgrant0607 · 2017-08-09T19:56:15Z

contributors/devel/api_changes.md

+      // param ...
+      Param  string `json:"param" protobuf:"bytes,2,opt,name=param"`
+      // width indicates how wide the object is.
+      // This field is alpha-level and should only be sent to servers that enable the Frobber2D feature.


@mbohlool Would it be possible for us to strip disabled fields out of the OpenAPI spec?

~~Do we want to remove them if the Frobber2D feature is disabled? ...~~ (updating comment, removing the old one)

Yes, we need to pass DefaultFeatureGate (probably converting it to simple map[string]bool) to the generator and add a comment tag here for alpha fields (e.g. // +openapi-gen: EnableOn(Forbber2D) or // +openapi-gen: AlphaField(Forbber2D)). Should I create an issue for it?

Removing fields from OpenAPI spec would have a nice effect on kubectl validation when it uses OpenAPI spec for validation.

bgrant0607 · 2017-08-09T20:03:18Z

Also relevant:

kubernetes/kubernetes#46073

kubernetes/kubernetes#23233 (comment)

bgrant0607 · 2017-08-09T21:18:42Z

Thanks, @liggitt. I think the only major issue is what to do when a value is specified for a disabled field. IMO, we need to drop the contents, silently by default, as with entirely unrecognized fields.

liggitt · 2017-08-18T21:04:53Z

Updated with links to other docs, updated guidance to clear disabled alpha fields before storing

bgrant0607 · 2017-08-23T04:39:17Z

Thanks.
/lgtm
/approve

k8s-github-robot · 2017-08-23T04:41:03Z

Automatic merge from submit-queue

Automatic merge from submit-queue (batch tested with PRs 51682, 51546, 51369, 50924, 51827) Clear values for disabled alpha fields Fixes kubernetes#51831 Before persisting new or updated resources, alpha fields that are disabled by feature gate must be removed from the incoming objects. This adds a helper for clearing these values for pod specs and calls it from the strategies of all in-tree resources containing pod specs. Addresses kubernetes/community#869

…rnetes#869)

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 1, 2017

liggitt added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Aug 1, 2017

liggitt mentioned this pull request Aug 1, 2017

Using annotations for new API features creates the potential for security lapses kubernetes/kubernetes#30819

Closed

liggitt mentioned this pull request Aug 1, 2017

Field-level versioning kubernetes/kubernetes#34508

Open

liggitt force-pushed the api-changes branch 4 times, most recently from 4bcfc7c to 71f4e76 Compare August 1, 2017 17:32

liggitt assigned deads2k, lavalamp, smarterclayton and bgrant0607 Aug 1, 2017

liggitt mentioned this pull request Aug 1, 2017

Proposal for growing persistent volumes #657

Merged

deads2k reviewed Aug 1, 2017

View reviewed changes

MHBauer reviewed Aug 1, 2017

View reviewed changes

seh reviewed Aug 1, 2017

View reviewed changes

cmluciano mentioned this pull request Aug 1, 2017

Support CIDR on ingress network-policy kubernetes/kubernetes#49978

Closed

thockin reviewed Aug 2, 2017

View reviewed changes

tallclair mentioned this pull request Aug 3, 2017

AllowPrivilegeEscalation aka no_new_privs kubernetes/enhancements#381

Closed

DirectXMan12 reviewed Aug 4, 2017

View reviewed changes

bgrant0607 reviewed Aug 9, 2017

View reviewed changes

bgrant0607 mentioned this pull request Aug 9, 2017

Redesign mount propagation #659

Merged

This was referenced Aug 15, 2017

Proposal: SubResources for CustomResources #913

Merged

Capture information about the user that created an instance or binding kubernetes-retired/service-catalog#939

Closed

k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 15, 2017

deads2k mentioned this pull request Aug 18, 2017

apiextensions: validation for customresources kubernetes/kubernetes#47263

Merged

13 tasks

liggitt mentioned this pull request Aug 18, 2017

Clear values for disabled alpha fields kubernetes/kubernetes#50924

Merged

Update alpha field guidance

a866717

liggitt force-pushed the api-changes branch from 71f4e76 to a866717 Compare August 18, 2017 21:00

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

k8s-github-robot merged commit 0150dd4 into kubernetes:master Aug 23, 2017

liggitt deleted the api-changes branch October 20, 2017 16:32

danehans pushed a commit to danehans/community that referenced this pull request Jul 18, 2023

Automator: update common-files@master in istio/community@master (kube…

a592c3a

…rnetes#869)


		The preferred approach adds an alpha field to the existing object, and ensures it is disabled by default.

		1. Add a feature gate to the API server to control enablement of the new field (and associated function):

Update alpha field guidance #869

Update alpha field guidance #869

Conversation

liggitt commented Aug 1, 2017 • edited Loading

liggitt commented Aug 1, 2017 • edited by bgrant0607 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

liggitt Aug 18, 2017 • edited Loading

Choose a reason for hiding this comment

liggitt Aug 18, 2017 • 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

liggitt Aug 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MHBauer left a comment

Choose a reason for hiding this comment

liggitt commented Aug 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin 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

liggitt commented Aug 2, 2017 • edited Loading

liggitt commented Aug 2, 2017

Choose a reason for hiding this comment

liggitt Aug 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgrant0607 commented Aug 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbohlool Aug 11, 2017 • edited Loading

Choose a reason for hiding this comment

bgrant0607 commented Aug 9, 2017

bgrant0607 commented Aug 9, 2017

liggitt commented Aug 18, 2017

bgrant0607 commented Aug 23, 2017

k8s-github-robot commented Aug 23, 2017

liggitt commented Aug 1, 2017 •

edited

Loading

liggitt commented Aug 1, 2017 •

edited by bgrant0607

Loading

liggitt Aug 18, 2017 •

edited

Loading

liggitt Aug 18, 2017 •

edited

Loading

liggitt Aug 1, 2017 •

edited

Loading

liggitt commented Aug 2, 2017 •

edited

Loading

liggitt Aug 4, 2017 •

edited

Loading

mbohlool Aug 11, 2017 •

edited

Loading