fixup! Initial KEP for unions

keps/sig-api-machinery/20190325-unions.md

@@ -14,6 +14,7 @@ status: provisional
 see-also:
   - "/keps/sig-api-machinery/0006-apply.md"
 replaces:
+  - https://docs.google.com/document/d/1lrV-P25ZTWukixE9ZWyvchfFR0NE2eCHlObiCUgNQGQ
 superseded-by:
 ---
 
@@ -26,10 +27,10 @@ have a keyword to implement “oneof” or "union". They allow APIs to have a be
 semantic, typically as a way to say “only one of the given fields can be
 set”. We currently have multiple occurrences of this semantic in kubernetes core
 types, at least:
+- VolumeSource is a structure that holds the definition of all the possible
+  volume types, only one of them must be set, it doesn't have a discriminator.
 - DeploymentStrategy is a structure that has a discrminator
   "DeploymentStrategyType" which decides if "RollingUpate" should be set
-- VolumeSource is a structure that holds the definition of all the possible
-  volume types, only one of them must be set
 
 The problem with the lack of solution is that:
 - The API is implicit, and people don't know how to use it
@@ -57,6 +58,9 @@ And then, for other people:
 - https://github.com/helm/charts/pull/11546
 - https://github.com/kubernetes/kubernetes/pull/35343
 
+Server-side [apply](http://features.k8s.io/555) is what enables this proposal to
+become possible.
+
 ### Goals
 
 The goal is to enable a union or "oneof" semantic in Kubernetes types, both for
@@ -72,21 +76,24 @@ to document what we're doing.
 In order to support unions in a backward compatible way in kubernetes, we're
 proposing the following changes.
 
+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.
+
 ### Go tags
 
 We're proposing a new type of tags for go types (in-tree types, and also
 kubebuilder types):
 
 - `// +oneof` before a field means that it is part of the union. Multiple fields
-  can have this prefix. Alternatively, the form `// +oneof=<name>` allows to
-  provide the discriminated name for that specific field. These fields MUST BE
-  optional, omitempty and pointers.
+  can have this prefix. These fields MUST BE optional, omitempty and pointers.
 - `// +discriminator` 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.
 
 Note that this means that only one "union" type can be defined per structure,
-which is a deliberate decision.
+which is a deliberate decision. This also includes go embedding which, once
+flattened, would define multiple unions in the same structure. Also note that we
+have no occurrences of these existing in core-types (AFAIK).
 
 ### OpenAPI
 
@@ -97,7 +104,9 @@ properly if possible (see CRD GA effort).
 Since OpenAPI v2 has no support for oneOf, a couple of kubernetes specific
 extensions will be added to OpenAPI:
 - `x-kubernetes-oneof-discriminator: true` is set for the discriminator field,
-- `x-kubernetes-oneof: "<discriminatedName>` is set for discriminated fields,
+- `x-kubernetes-oneof: "<discriminatedName>` is set for discriminated
+  fields. `<discriminatedName>` is the name that should be used as the value of
+  the discriminator to indicate that this specific field should be kept.
 
 As for Go types, we only accept one union per structure, which means there can
 be only one discriminator, and all other `oneof` fields are assumed to belong to
@@ -107,22 +116,36 @@ the same union.
 
 For backward compatibility reasons, discriminators can be added to existing
 union structures as an optional string. This has a nice property that it's going
-to allow conflict detection when a union field is flipped.
+to allow conflict detection when the selected union field is changed.
 
 The value of the discriminator is going to be set automatically by the apiserver
-when a new field is changed in the union, and will be interpreted as an
-intention to clear all the other fields when its value is changed. See section
+when a new field is changed in the union. It will be set to the value of the
+`x-kubernetes-oneof` extension for that specific field.
+
+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.
 
-### Normalization process
+If the object had exactly one of the union field set, and that field is removed,
+the discriminator will be cleared.
 
-The normalization process (which runs before validating that the union property
-is verified), works as follows:
+### Normalizing on updates
+
+A "normalization process" will run automatically for all creations and
+modifications (either with put 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).
+
+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, set the discriminator (if exists) to that
-    value. Otherwise
+    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
@@ -148,54 +171,15 @@ 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.
 
-## Drawbacks
+## Future Work
 
 Since the proposal only normalizes the object after the patch has been applied,
 it is hard to fail if the patch is invalid. There are scenarios where the patch
-is invalid but it results in an unpredictable object:
-
-Object has a discriminator named `discriminator` for a union of fields `a` and `b`.
-
-Object currently looks like:
-
-```
-discriminator: a
-a: 1
-```
-
-A clearly wrong patch is sent:
-
-```
-discriminator: b
-a: 1
-```
-
-The merge object looks like this:
-
-```
-discriminator: b
-a: 1
-```
-
-Since the discriminator has changed, `a` is cleared by normalization, which is (or
-isn't) what the client intended to do.
-
-Also, given the same patch, if the object was:
-
-```
-discriminator: b
-b: 1
-```
-
-the merged object would look like:
-```
-discriminator: b
-a: 1
-b: 1
-```
-
-Since the discriminator hasn't changed, but `a` has been added, the discriminator
-will flip to `a`, and `b` will be dropped.
+is invalid but it results in an unpredictable object. For example, if a patch
+sends both a discriminator and a field that is not the discriminated field, it
+will either clear the value sent if the discriminator changes, or it will change
+the value of the sent discriminator.
 
 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 semantic.
+validate "Apply" objects to make sure that they do not define such broken
+semantic.