diff --git a/keps/prod-readiness/sig-api-machinery/1027.yaml b/keps/prod-readiness/sig-api-machinery/1027.yaml new file mode 100644 index 00000000000..ef0e558d303 --- /dev/null +++ b/keps/prod-readiness/sig-api-machinery/1027.yaml @@ -0,0 +1,3 @@ +kep-number: 1027 +alpha: + approver: "@johnbelamaric" diff --git a/keps/sig-api-machinery/1027-api-unions/README.md b/keps/sig-api-machinery/1027-api-unions/README.md index 68d82ac53af..bc04280820f 100644 --- a/keps/sig-api-machinery/1027-api-unions/README.md +++ b/keps/sig-api-machinery/1027-api-unions/README.md @@ -1,4 +1,4 @@ -# Union types +# KEP-1027: Union types ## Table of Contents @@ -9,31 +9,71 @@ - [Goals](#goals) - [Non-Goals](#non-goals) - [Proposal](#proposal) - - [Go tags](#go-tags) + - [User Stories (Optional)](#user-stories-optional) + - [Story 1](#story-1) + - [Story 2](#story-2) + - [Notes/Constraints/Caveats (Optional)](#notesconstraintscaveats-optional) + - [Risks and Mitigations](#risks-and-mitigations) +- [Design Details](#design-details) + - [Discriminator Field](#discriminator-field) + - [Go Markers](#go-markers) + - [Discriminator Values](#discriminator-values) + - [Empty Union Members](#empty-union-members) + - [Examples](#examples) - [OpenAPI](#openapi) - - [Discriminator](#discriminator) - - [Normalizing on updates](#normalizing-on-updates) - - ["At most one" versus "exactly one"](#at-most-one-versus-exactly-one) - - [Clearing all the fields](#clearing-all-the-fields) - - [Backward compatibilities properties](#backward-compatibilities-properties) - - [Validation](#validation) + - [Normalization and Validation](#normalization-and-validation) + - [Normalization](#normalization) + - [Validation](#validation) + - [Ratcheting Validation](#ratcheting-validation) + - [Migrating existing unions](#migrating-existing-unions) - [Test Plan](#test-plan) + - [Prerequisite testing updates](#prerequisite-testing-updates) + - [Unit tests](#unit-tests) + - [Integration tests](#integration-tests) + - [e2e tests](#e2e-tests) - [Graduation Criteria](#graduation-criteria) - - [Beta -> GA Graduation](#beta---ga-graduation) -- [Future Work](#future-work) + - [Alpha Graduation](#alpha-graduation) + - [Upgrade / Downgrade Strategy](#upgrade--downgrade-strategy) + - [Version Skew Strategy](#version-skew-strategy) +- [Production Readiness Review Questionnaire](#production-readiness-review-questionnaire) + - [Feature Enablement and Rollback](#feature-enablement-and-rollback) + - [Rollout, Upgrade and Rollback Planning](#rollout-upgrade-and-rollback-planning) + - [Monitoring Requirements](#monitoring-requirements) + - [Dependencies](#dependencies) + - [Scalability](#scalability) + - [Troubleshooting](#troubleshooting) - [Implementation History](#implementation-history) +- [Drawbacks](#drawbacks) +- [Alternatives](#alternatives) ## Release Signoff Checklist -- [x] kubernetes/enhancements issue in release milestone, which links to KEP (this should be a link to the KEP location in kubernetes/enhancements, not the initial KEP PR) -- [x] KEP approvers have set the KEP status to `implementable` -- [x] Design details are appropriately documented -- [x] Test plan is in place, giving consideration to SIG Architecture and SIG Testing input -- [x] Graduation criteria is in place -- [x] "Implementation History" section is up-to-date for milestone +Items marked with (R) are required *prior to targeting to a milestone / release*. + +- [ ] (R) Enhancement issue in release milestone, which links to KEP dir in [kubernetes/enhancements] (not the initial KEP PR) +- [ ] (R) KEP approvers have approved the KEP status as `implementable` +- [ ] (R) Design details are appropriately documented +- [ ] (R) Test plan is in place, giving consideration to SIG Architecture and SIG Testing input (including test refactors) + - [ ] e2e Tests for all Beta API Operations (endpoints) + - [ ] (R) Ensure GA e2e tests for meet requirements for [Conformance Tests](https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/conformance-tests.md) + - [ ] (R) Minimum Two Week Window for GA e2e tests to prove flake free +- [ ] (R) Graduation criteria is in place + - [ ] (R) [all GA Endpoints](https://github.com/kubernetes/community/pull/1806) must be hit by [Conformance Tests](https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/conformance-tests.md) +- [ ] (R) Production readiness review completed +- [ ] (R) Production readiness review approved +- [ ] "Implementation History" section is up-to-date for milestone - [ ] User-facing documentation has been created in [kubernetes/website], for publication to [kubernetes.io] -- [ ] Supporting documentation e.g., additional design documents, links to mailing list discussions/SIG meetings, relevant PRs/issues, release notes +- [ ] Supporting documentation—e.g., additional design documents, links to mailing list discussions/SIG meetings, relevant PRs/issues, release notes + + + +[kubernetes.io]: https://kubernetes.io/ +[kubernetes/enhancements]: https://git.k8s.io/enhancements +[kubernetes/kubernetes]: https://git.k8s.io/kubernetes +[kubernetes/website]: https://git.k8s.io/website ## Summary @@ -86,92 +126,254 @@ become possible. ### Goals -The goal is to enable a union or "oneof" semantics in Kubernetes types, both for -in-tree types and for CRDs. +Provide a simple mechanism for API authors to label fields of a resource as +members of a oneOf, in order to receive standardized validation and +normalization, rather than having to author it themeselves per +resource as currently done as a workaround in various validation +functions (e.g. `pkg/apis//validation/validation.go`). + +* Validation - ensuring only one member field is set (or at most one if + desired). +* Normalization - ensuring the API server can understand the intent of clients + that are unable to update/modify fields the clients are unaware of due to + version skew. ### Non-Goals -We're not planning to use this KEP to release the feature, but mostly as a way -to document what we're doing. +Migrating all existing unions away from their bespoke validation +logic (e.g validation functions), is an explicit non-goal and will be pursued in +a separate KEP or later release. ## Proposal -In order to support unions in a backward compatible way in kubernetes, we're -proposing the following changes. +### User Stories (Optional) + + + + +#### Story 1 + +As a CRD owner, I can use simple semantics (such as openapi tags/go markers), to express the +desired validation of a oneOf (at most one or exactly one field may be set), and +the API server will perform this validation automatically. + +#### Story 2 + +As a client, I can read, modify, and update the union fields of an object, even +if I am not aware of all of the possible fields, and the server will properly +interpret my intent. + +### Notes/Constraints/Caveats (Optional) + + -Note that this proposes unions to be "at most one of". Whether exactly one is -supported or not should be implemented by the validation logic. +### Risks and Mitigations +- We need to ensure we do not break existing union types. This can be done by + not forcing existing unions to conform to the newly proposed union semantics. + Integration testing with older types should give us the confidence to be sure + we have done so. +- There is a lot of risk for errors when there exists skew between clients and + server. In the section on normalization, we discuss mitigating these risks. -### Go tags + + +## Design Details + +### Discriminator Field + +We propose that all new unions maintain a "discriminator". This is a field that +points to which of the other "union member" fields is to be respected as the +truly desired union field in the case that there are any conflicts. + +In order to demonstrate the need for the discriminator, we developed an +extensive [test matrix](https://docs.google.com/spreadsheets/d/1dIOOqgrgvMI9b2412eVuRSydEaOxhYOoqk6bUjZOY9c/edit?resourcekey=0-wlOfJTC_EX-qpU680STHMA#gid=3601413) that looks at various configurations of the performing +REST operations on a union where the client or server is unaware of a newly +added field to the union (due to version skew). + +We present a [guide doc](https://docs.google.com/document/d/1Wruosjo0ELLl1yxauzpsUjgH2fK9KdgXDmOdJ5sG7Kg/edit?resourcekey=0-8Pwzx6EvsFR7VQoXzCTY4Q) on how to interpret the test matrix, but the major +conclusions are as follows (along with the test case number from the test matrix): + +* (Case #22 and #27) If an unstructured client (i.e. a client that represents data as raw json maps with no knowledge of the schema) + is unaware of field on the union, but wants to clear + the union entirely (assuming the union is optional), it will have no way of doing + so without a discriminator. With a discriminator, the client can express its + intention by setting the discriminator to the empty value and the server can + respect its intentions and clear any fields the client is unaware of. +* (Case #12 and #16) If a structured client is unaware of a field in the union that is set and it + just wants to echo back the union it received in a get request (such as when + updating other parts of the object), a client without a discriminator will + silently drop the currently set field, while a client with the discriminator + will not change the discriminator value, indicating to the server that no + changes are desired in the union. +* (Case #34 and #39) If a client sets a union field that the server is not aware of, the server + will silently drop it and attempt to clear the object of the union field. With + a discriminator, the server will see the unrecognized discriminator value and + can fail loudly. +* (Case #23 and #28) When a client goes to set a field it knows of, but a separate field it doesn't + know about is currently set, the server can simply know to always respect the + discriminator. Without a discriminator, the server will have to do convoluted + logic to detect that the previously set field has not been modified and that + only one of the other union fields has been. + +### Go Markers We're proposing a new type of tags for go types (in-tree types, and also kubebuilder types): -- `// +union` before a structure means that the structure is a union. All the - fields must be optional (beside the discriminator) and will be included as - members of the union. That structure CAN be embedded in another structure. -- `// +unionDeprecated` before a field means that it is part of the - union. Multiple fields can have this prefix. These fields MUST BE optional, - omitempty and pointers. The field is named deprecated because we don't want - people to embed their unions directly in structures, and only exist because of - some existing core types (e.g. `Value` and `ValueFrom` in - [EnvVar](https://github.com/kubernetes/kubernetes/blob/3ebb8ddd8a21b/staging/src/k8s.io/api/core/v1/types.go#L1817-L1836)). + - `// +unionDiscriminator` before a field means that this field is the - discriminator for the union. Only one field per structure can have this - prefix. This field HAS TO be a string, and CAN be optional. + discriminator for the union. This field MUST be an enum defined as a string (see section on + discriminator values). This field MUST be required if there is no default + option, omitempty if the default option is the empty string, or optional and + omitempty if a default value is specified with the `// +default` marker. +- `// +unionMember[=][,optional] before a field means that this + field is a member of a union. The `` is the name of the field that will be set as the discriminator value. + It MUST correspond to one of the valid enum values of the discriminator's enum + type. It defaults to the go (i.e `CamelCase`) representation of the field name if not specified. + `` should only be set if authors want to customize how the fields + are represented in the discriminator field. `` should match the + serialized JSON name of the field case-insensitively. The comma separated + optional value determines whether or not the member field must be set when the + discriminator selects it. Meaning, when `optional` is present on a field, the + discriminator can select the field even if the field is not set. If optional + is not present in the `unionMember` tag, then the object will fail validation + if the discriminator selects the field but it is nil. A field can be marked as + optional without specifying memberName via `// +unionMember,optional`. + +#### Discriminator Values + +Here we present a description of how discriminators and their valid values +should be defined. + +As described above, the discriminator field must be a string and required. +Because, there are only a few specific values that the discriminator can be, we +propose that all discriminators should be defined as an enum, and should be +tagged so via the enum go marker `// +enum`. + +If no option is a valid option for a union, such an option must be defined as a +member of the discriminator values enum. By convention, this "no member" +discriminator should be the empty string, but there is nothing stopping API +authors from defining their own "no option" discriminator value. + +##### Empty Union Members + +In some cases there are more discriminator values than there are member fields +defined in the struct when that specific member requires no configuration. An +example is the `DeploymentStrategy` where it has one member field `rollingUpdate`, +but two valid discriminator values `RollingUpdate` and `Recreate`. By using an +enum as the discriminator value we are able to define values beyond the member +fields in order to accommodate this pattern. + +#### Examples + +Below is an example of how to define a union based on the above design -Multiple unions can exist per structure, but unions can't span across multiple -go structures (all the fields that are part of a union has to be together in the +``` +// +enum +type UnionType string + +const ( + FieldA UnionType = "FieldA" + FieldB UnionType = "FieldB" + FieldC UnionType = "FieldC" + FieldD UnionType = "FieldD" + FieldNone UnionType = "" +) + +type Union struct { + // +unionDiscriminator + // +required + UnionType UnionType + + // +unionMember + // +optional + FieldA int + // +unionMember + // +optional + FieldB int +} +``` + + +Note unions can't span across multiple go structures (all the fields that are part of a union has to be together in the same structure), examples of what is allowed: ``` // This will have one embedded union. type TopLevelUnion struct { - Name string `json:"name"` + Name string `json:"name"` - Union `json:",inline"` + Union `json:",inline"` } +// +enum +type UnionType string + +const ( + FieldA UnionType = "FieldA" + FieldB UnionType = "FieldB" + FieldC UnionType = "FieldC" + FieldD UnionType = "FieldD" + FieldNone UnionType = "" +) + // This will generate one union, with two fields and a discriminator. -// +union type Union struct { - // +unionDiscriminator - // +optional - UnionType string `json:"unionType"` - - // +optional - FieldA int `json:"fieldA"` - // +optional - FieldB int `json:"fieldB"` + // +unionDiscriminator + // +required + UnionType UnionType `json:"unionType"` + + // +unionMember + // +optional + FieldA int `json:"fieldA"` + // +unionMember,optional + // +optional + FieldB int `json:"fieldB"` } -// This also generates one union, with two fields and on discriminator. -type Union2 struct { - // +unionDiscriminator - Type string `json:"type"` - // +unionDeprecated - // +optional - Alpha int `json:"alpha"` - // +unionDeprecated - // +optional - Beta int `json:"beta"` -} +// +enum +type Union2Type string -// This has 3 embedded unions: -// One for the fields that are directly embedded, one for Union, and one for Union2. -type InlinedUnion struct { - Name string `json:"name"` +const ( + Alpha Union2Type = "ALPHA" + Beta = "BETA" +) - // +unionDeprecated - // +optional - Field1 *int `json:"field1,omitempty"` - // +unionDeprecated - // +optional - Field2 *int `json:"field2,omitempty"` - - Union `json:",inline"` - Union2 `json:",inline"` +// This generates a union where the unionMember markers demonstrate how to +customize the names used for each field in the discriminator. +type Union2 struct { + // +unionDiscriminator + // +required + Type2 Union2Type `json:"type"` + // +unionMember=ALPHA + // +optional + Alpha int `json:"alpha"` + // +unionMember=BETA,optional + // +optional + Beta int `json:"beta"` } + ``` ### OpenAPI @@ -183,133 +385,773 @@ for validation, but is "on-top" of this proposal. A new extension is created in the openapi to describe the behavior: `x-kubernetes-unions`. -This is a list of unions that are part of this structure/object. Here is what -each list item is made of: -- `discriminator: ` is set to the name of the discriminator - field, if present, -- `fields-to-discriminateBy: {"": ""}` is a map of - fields that belong to the union to their discriminated names. The - discriminatedValue will typically be set to the name of the Go variable. +This is a list of unions that are part of this structure/object. Each item in +the list represents a discriminator for the union, and for each member field, +the discriminator value of that field and whether or not that field is optional. Conversion between OpenAPI v2 and OpenAPI v3 will preserve these fields. -### Discriminator +The following is an example of what the generated OpenAPI definition will look +like for a given go type. + +``` +const ( + FieldA Union1Type = "FieldA" + FieldB Union1Type = "FieldB" + FieldC Union1Type = "FieldC" + FieldD Union1Type = "FieldD" + FieldNone Union1Type = "" +) + +// This will generate one union, with two fields and a discriminator. +type Union struct { + // +unionDiscriminator + // +required + Union1 Union1Type `json:"union1"` + + // +unionMember + // +unionDiscriminatedBy=Union1 + // +optional + FieldA int `json:"fieldA"` + // +unionMember,optional + // +unionDiscriminatedBy=Union1 + // +optional + FieldB int `json:"fieldB"` + +} +``` + +The OpenAPI x-kubernetes-unions extension will then be attached to the discriminator's property and +deserialized into go structs as follows: +``` +// XKubernetesUnions is the top level extension +type XKubernetesUnions struct { + // FieldMembers are the mapping of all valid discriminator values in a + // union to the corresponding member field. + // Discriminator value is the value to which the discriminator is set to + // in order to indicate that a given member field is the currently set member + // of the union. + // MemberField may be nil in the case of empty union members where a valid + // discriminator value has no corresponding member field. + FieldMembers map[DiscriminatorValue]*MemberField `json:"fieldMembers"` +} + +// DiscriminatorValue is the value that the discriminator is set to +// in order to indicate the selection of a union member. +type DiscriminatorValue string + +// MemberField +type MemberField struct { + // Name is the name of the field corresponding to the member. + // It will be the json representation of the field marked with the + // `// +unionMember` marker in the go type. + Name string `json:"name"` + // Optional determines whether the discriminator _may_ select this member + // even when the member field is empty. Optional defaults to false. + Optional bool `json:"optional"` +} +``` + +### Normalization and Validation + +#### Normalization + +Normalization refers to the process by which the API server attempts to understand +and correct clients which may provide the server with conflicting or incomplete +information about a union in update or patch requests. + +Issues primarily arise here because of version skew between a client and a server, +such as when a client is unaware of new fields added to a union and thus doesn't +know how to clear these new fields when trying to set a different field. + +For unions that follow this design, normalization is simple: the server should always respect the +discriminator. + +This means that when the server receives an update request with a discriminator set to a +given field, and multiple member fields are set it should clear all fields +except the one pointed to by the discriminator _if and only if_ the +discriminator has been modified. Having multiple fields set, and a discriminator +not modified is invalid and caught later by the validation step (see below). -For backward compatibility reasons, discriminators should be added to existing -union structures as an optional string. This has a nice property that it's going -to allow conflict detection when the selected union field is changed. +For both custom resources and built-in types, we expect union normalization to be +called by the request handlers shortly after mutating admission occurs. -We also do strongly recommend new APIs to be written with a discriminator, and -tools (kube-builder) should probably enforce the presence of a discriminator in -CRDs. +#### Validation -The value of the discriminator is going to be set automatically by the apiserver -when a new field is changed in the union. It will be set to the value of the -`fields-to-discriminateBy` for that specific field. +Objects must be validated AFTER the normalization process. -When the value of the discriminator is explicitly changed by the client, it -will be interpreted as an intention to clear all the other fields. See section -below. +Some validation situations specific to unions are: +1. When multiple union fields are set and the discriminator has not been modified we should + error loudly that the client must change the discriminator if it changes any + union member fields. +2. When the server receives a request with a discriminator set to a given + field, but that given field is empty and not marked as optional, the server should fail with a clear + error message. Note this does not apply to discriminator values that do not + correspond to any field (as in the "empty union members case"). -### Normalizing on updates +For both custom resources and built-in types, validation will occur as part of +the request validation, before validating admission occurs. -A "normalization process" will run automatically for all creations and -modifications (either with update or patch). It will happen automatically in order -to clear fields and update discriminators. This process will run for both -core-types and CRDs. It will take place before validation. The sent object -doesn't have to be valid, but fields will be cleared in order to make it valid. -This process will also happen before fields tracking (server-side apply), so -changes in discriminator, even if implicit, will be owned by the client making -the update (and may result in conflicts). +For custom resources, union validation will be done at the same point as the +existing structural schema validation that occurs in the custom resource handler. +This ensures that any generic validation changes made to all custom resources (such +as the ratcheting validation discussed below), behaves appropriately with union +validation. -This process works as follows: -- If there is a discriminator, and its value has changed, clear all fields but - the one specified by the discriminator, -- If there is no discriminator, or if its value hasn't changed, - - if there is exactly one field, set the discriminator when there is one - to that value. Otherwise, - - compare the fields set before and after. If there is exactly one field - added, set the discriminator (if present) to that value, and remove all - other fields. if more than one field has been added, leave the process so - that validation will fail. +#### Ratcheting Validation -#### "At most one" versus "exactly one" +When updating CRDs to support union validation, it is possible that existing CRs +become invalid. -The goal of this proposal is not to change the validation, but to help clients -to clear other fields in the union. Validation should be implemented for in-tree -types as it is today, or through "oneOf" properties in CRDs. +The naive solution is to require existing CRs to be updated to a valid state +before they can be updated again. -In other word, this is proposing to implement "at most one", and the exactly one -should be provided through another layer of validation (separated). +This creates many potential landmines, and so ratcheting validation is proposed +as an alternative. Ratcheting validation means that objects will ignore stricter +validation rules if and only if the existing object also fails the stricter +validation for the same reason. -#### Clearing all the fields +Ratcheting validation for custom resources is a [separate +effort](https://github.com/kubernetes/kubernetes/issues/94060) proposed +outside of this unions effort. For the initial alpha graduation of unions, we +do not propose supporting ratcheting validation. We will require all invalid CRs +to be made valid before they can be updated (the naive solution). -Since the system is trying to do the right thing, it can be hard to "clear -everything". In that case, each API could decide to have their own "Nothing" -value in the discriminator, which will automatically trigger a clearing of all -fields beside "Nothing". +In order to potentially support ratcheting validation in the future, we will +ensure that all callers of union validation retain access to both old and new +objects, so that future ratcheting validation can be implemented within the +union validation library. -### Backward compatibilities properties +#### Migrating existing unions -This normalization process has a few nice properties, especially for dumb -clients, when it comes to backward compatibility: -- A dumb client that doesn't know which fields belong to the union can just set - a new field and get all the others cleared automatically -- A dumb client that doesn't know about the discriminator is going to change a - field, leave the discriminator as it is, and should still expect the fields to - be cleared accordingly -- A dumb client that knows about the discriminator can change the discriminator - without knowing which fields to clear, they will get cleared automatically +As mentioned, one of the goals is to migrate at least one existing union to +using the new marker based union validation and normalization. While open +questions remain around the priority and urgency of migrating existing unions, +nonetheless we should be able to come to a consensus on which types to migrate +first. +For discriminated unions, a couple relatively straightforward discriminated types are +`MetricSpec` and `MetricStatus`. These have clearly defined discriminator values +that map one-to-one to a member field, which make them good candidates for +initial migration. -### Validation +For non-discriminated unions, there are a few relatively straightforward types +that make good candidates for initial migration, such as `ContainerState` + +Until migrated, union types without a discriminator (i.e. only existing unions that have not been migrated +to the current desgin), cannot be tagged with the go markers described above and +thus will not be treated as "unions" in the sense of this currently proposed +normalization and validation logic. + +These legacy unions must continue to perform normalization and +validation manually, per resource in the validation functions. -Objects have to be validated AFTER the normalization process, which is going to -leave multiple fields of the union if it can't normalize. As discussed in -drawbacks below, it can also be useful to validate apply requests before -applying them. ### Test Plan -There are mostly 3 aspects to this plan: -- [x] Core functionality, isolated from all other components: https://github.com/kubernetes-sigs/structured-merge-diff/blob/master/typed/union_test.go -- [x] Functionality as part of server-side apply: How human and robot interactions work: https://github.com/kubernetes-sigs/structured-merge-diff/blob/master/typed/union_test.go -- [x] Integration in kubernetes: https://github.com/kubernetes/kubernetes/pull/77370/files#diff-4ac5831d494b1b52c7c7be81e552a458 +[x] I/we understand the owners of the involved components may require updates to +existing tests to make this code solid enough prior to committing the changes necessary +to implement this enhancement. + +##### Prerequisite testing updates + + + +##### Unit tests + + + + +Core functionality will be extensively unit tested in the SMD typed package +(union_test.go). + +Parts of the kubernetes endpoints handlers package that are modified to call +into the SMD code will also be unit tested as appropriate. + + + + + +##### Integration tests + +We will have extensive integration testing of the union code in the +`test/integration/apiserver` package. + +We will be testing along the dimensions of: +* Which fields of the union get modified (none, existing fields, newly updated + fields) +* Type of union (discriminated vs non-discriminated) +* Whether the client is aware of all the fields +* Whether the server is aware of all fields +* Whether the union is optional or required + +A fully documented test matrix exists in a [google +spreadsheet](https://docs.google.com/spreadsheets/d/1dIOOqgrgvMI9b2412eVuRSydEaOxhYOoqk6bUjZOY9c/edit?resourcekey=0-wlOfJTC_EX-qpU680STHMA#gid=3601413) along with a +[guide +doc](https://docs.google.com/document/d/1Wruosjo0ELLl1yxauzpsUjgH2fK9KdgXDmOdJ5sG7Kg/edit?resourcekey=0-8Pwzx6EvsFR7VQoXzCTY4Q) on how to read and understand the test matrix. + +As part of implementing the test matrix we will be able to prove the viability +of upgrading existing unions by writing tests to mimic using the standardized +union semantics on existing unions (even if actually upgrading these unions is +outside the scope of alpha graduation) + + + +##### e2e tests + + + We are considering adding kubectl e2e tests to mimic kubectl users performing + various operations on objects with union fields. ### Graduation Criteria -Since this project is a sub-project of Server-side apply, it will be introduced -directly as Beta, and will graduate to GA in a later release, according to the -criteria below. +#### Alpha Graduation -#### Beta -> GA Graduation +- CRDs can be created with union fields and are properly validated when + created/updated. +- Prove the viability of upgrading existing unions to the new semantics by + mimicking existing unions in e2e tests. +- Existing unions that don't have discriminators do not break when upgraded. -- CRD support has been proven successful -- Core-types all implement the semantics properly -- Stable and bug-free for two releases +### Upgrade / Downgrade Strategy -## Future Work + -Validating patches is not a problem that we want to tackle now, but we can -validate "Apply" objects to make sure that they do not define such broken -semantics. +Turning the flag on for alpha just enables different runtime codepaths (i.e. +performing the unified union validation and normalization) -## Implementation History +Any schema markers (added by CRD authors or propagated from tags on built-in +types) will appear in the schema, but not do anything if the flag is off. + +### Version Skew Strategy + + + +See test matrix and commentary about discriminators. It clearly documents how +the server will use the discriminator to understand the client's intention even +if the client is not aware of all union fields because of version skew. + +Skew with alpha flag on/off shouldn't make much of a difference. +* Objects created with the union semantics, but applied to a cluster with the + alpha flag off will simply not perform union validation and normalization. +* Objects created without union semantics will simply not trigger union + validation and normalization (regardless of whether the server has the alpha + enabled or disabled). -Here are the major milestones for this KEP: -- Initial discussion happened a year before the creation of this kep: - https://docs.google.com/document/d/1lrV-P25ZTWukixE9ZWyvchfFR0NE2eCHlObiCUgNQGQ/edit#heading=h.w5eqnf1f76x5 -- Points made in the initial document have been improved and put into this kep, - which has approved by sig-api-machinery tech-leads -- KEP has been implemented: - - logic mostly lives in sigs.k8s.io/structured-merge-diff - - conversion between schema and openapi definition are in k8s.io/kube-openapi - - core types have been modified in k8s.io/kubernetes -- Feature is ready to be released in Beta in kubernetes 1.15 +## Production Readiness Review Questionnaire + + + +### Feature Enablement and Rollback + + + +###### How can this feature be enabled / disabled in a live cluster? + + + +- [x] Feature gate (also fill in values in `kep.yaml`) + - Feature gate name: APIUnions + - Components depending on the feature gate: kube-apiserver + +Request handlers in the api server will call into union validation and +normalization function from the structured-merge-diff repo when feature is +enabled. + + +###### Does enabling the feature change any default behavior? + +Enabling the feature could cause existing CRs to fail validation if the +correspond CRD has union fields and the existing CRs have invalid unions that +were unvalidated when initially created in a cluster that had the unions feature +disabled. + +These CRs will need to be corrected in order to pass validation (or the feature +disabled). + + + +###### Can the feature be disabled once it has been enabled (i.e. can we roll back the enablement)? + + + +Yes, requests will simply skip union validation and normalization. + +###### What happens if we reenable the feature if it was previously rolled back? + +Custom resources that were skipping union validation when when the feature was +rolled back may have allowed invalid data to persist. + +For alpha, we require that all modifying requests (update/patch) fail unless the +data passes union validation. Retrieving newly invalid CRs should still always +succeed. + +In the future, we may require looser "ratcheting validation" which would allow +modifications to ignore union validation if the existing object fails the union +validation for the same reason as the new object (see section on "Ratcheting +Validation" above). This is not a priority for alpha. + +###### Are there any tests for feature enablement/disablement? + + + +We will have integration tests demonstrating how CRs with persisted invalid data +will need to be corrected when the feature is re-enabled (and requires more +strict union validation). + +### Rollout, Upgrade and Rollback Planning + + + +###### How can a rollout or rollback fail? Can it impact already running workloads? + +N/A + + + +###### What specific metrics should inform a rollback? + +* `apiserver_request_total` could be watched to see if the number of create and + update requests that are failing increase substantially. + + + +###### Were upgrade and rollback tested? Was the upgrade->downgrade->upgrade path tested? + +N/A + + +###### Is the rollout accompanied by any deprecations and/or removals of features, APIs, fields of API types, flags, etc.? + +N/A + + +### Monitoring Requirements + + + +###### How can an operator determine if the feature is in use by workloads? + +For builtins in alpha, it won't be possible to break clients since turning on vs +off will validate the same thing via different code paths. + +For CRDs, you can see if they have the new union markers. If the CRD has no +other validation mechanism, turning off the flag may result in CRs accepting +invalid input. + + + +###### How can someone using this feature know that it is working for their instance? + +1. Create a new CRD with a union field (and no other validation mechanism) +2. Apply the CRD +3. Create a CR with an invalid union (multiple fields set, no discriminator + set), see if the CR is rejected via union validation + +When we write the e2e test, a standard union CRD and test CR will be obtainable +for users to test on their instance. + + +###### What are the reasonable SLOs (Service Level Objectives) for the enhancement? + +N/A + + + +###### What are the SLIs (Service Level Indicators) an operator can use to determine the health of the service? + +N/A + + +###### Are there any missing metrics that would be useful to have to improve observability of this feature? + +N/A + + +### Dependencies + + + + +###### Does this feature depend on any specific services running in the cluster? + +N/A + + +### Scalability + +N/A + + +###### Will enabling / using this feature result in any new API calls? + +No + + + +###### Will enabling / using this feature result in introducing new API types? + +No + + + +###### Will enabling / using this feature result in any new calls to the cloud provider? + +No + + +###### Will enabling / using this feature result in increasing size or count of the existing API objects? + +No + + +###### Will enabling / using this feature result in increasing time taken by any operations covered by existing SLIs/SLOs? + +In GA (maybe beta), we might expect resource reduction/reliability improvement, +since this removes a need for webhooks. + + + +###### Will enabling / using this feature result in non-negligible increase of resource usage (CPU, RAM, disk, IO, ...) in any components? + +New validation and normalization logic should be negligible given that the +functions will be in the same SMD path currently used by SSA code. + +We will have benchmarking to validate this assumption. + + + +### Troubleshooting + + + +###### How does this feature react if the API server and/or etcd is unavailable? + +N/A objects are not reachable + +###### What are other known failure modes? + + + +###### What steps should be taken if SLOs are not being met to determine the problem? + +## Implementation History +- Unions implemented, but disabled in SMD. + +## Drawbacks + + +An issue that one might have with requiring a discriminator is that it might +seem redundant to have to set a field _and_ set another field indicating to the +server to use the set field. The reasons for doing so are discussed above in the +normalization section. + +One other drawback is that our approach does not standardize all existing unions +into a single format. We don't see a way to do so without drastically changing +existing APIs and breaking backwards compatibility + +## Alternatives + +###### Non-Discrminated + +The primary alternative discussed is to not have a discriminator for new union +types. As discussed in the normalization section, requiring a discriminator +allows the server to better understand the intentions of clients that do not +have knowledge of all the fields in a union if newer versions of the server add +new fields to the union. + +###### "None" Discriminator Values + +A number of strategies were discussed around how to represent the "none" value +of the discriminator (see "Discriminator Values" section above). + +* One alternative was to mandate the "none" value always be the empty string. + The advantage to this is its simplicity and not creating a situation where + different API authors define there "none" value differently, so that anyone + could immediately know that a discriminator set to "" (empty string), is not + selecting any of the member fields. Also, it would allow us to not have to + define the set of enum values for each discriminator (as we could just use the + name of the member field). The disadvantage is that by not defining the set of + enum values, we make it impossible to support the "empty union members" case. +* Another alternative was to make the discriminator a pointer to a string and + its value nil. The disadvantage here is that this requires more complicated + union validation logic (first do a nil check, then check the value) and makes + it harder to determine client intent on patches where the discriminator is not + set. +* A third alternative is to require all unions be defined in their own separate + struct. This was rejected because there are many existing unions that define + random fields that are not members in the union within the same struct as + fields that do make up the union and we hope to be able to migrate at least + some of the existing unions to the new semantics. + + diff --git a/keps/sig-api-machinery/1027-api-unions/kep.yaml b/keps/sig-api-machinery/1027-api-unions/kep.yaml index 6f74bebec83..af083e055cc 100644 --- a/keps/sig-api-machinery/1027-api-unions/kep.yaml +++ b/keps/sig-api-machinery/1027-api-unions/kep.yaml @@ -2,8 +2,12 @@ title: Union types kep-number: 1027 authors: - "@apelisse" + - "@kevindelgado" owning-sig: sig-api-machinery participating-sigs: +status: implementable +creation-date: 2019-03-25 +last-updated: 2022-06-13 reviewers: - "@sttts" - "@lavalamp" @@ -11,15 +15,18 @@ reviewers: - "@DirectXMan12" approvers: - "@lavalamp" -editor: TBD -creation-date: 2019-03-25 -last-updated: 2019-03-25 -status: implementable + - "@deads2k" + see-also: - "/keps/sig-api-machinery/0006-apply.md" replaces: - "https://docs.google.com/document/d/1lrV-P25ZTWukixE9ZWyvchfFR0NE2eCHlObiCUgNQGQ" -superseded-by: -latest-milestone: "1.15" stage: "alpha" +latest-milestone: "1.25" + +feature-gates: + - name: APIUnions + components: + - kube-apiserver +disable-supported: true