Merge pull request #278 from ymqytw/add_discriminators

Proposal: Add Tag Indicating To Replace Union Fields in Patch

1 files changed, 340 insertions, 0 deletions

diff --git a/contributors/design-proposals/add-new-patchStrategy-to-clear-fields-not-present-in-patch.md b/contributors/design-proposals/add-new-patchStrategy-to-clear-fields-not-present-in-patch.md
new file mode 100644
index 00000000..bec26f96
--- /dev/null
+++ b/contributors/design-proposals/add-new-patchStrategy-to-clear-fields-not-present-in-patch.md
@@ -0,0 +1,340 @@
+Add new patchStrategy to clear fields not present in the patch
+=============
+
+Add tags `patchStrategy:"replaceKeys"`. For a given type that has the tag, all keys/fields missing
+from the request will be cleared when patching the object.
+For a field presents in the request, it will be merged with the live config.
+
+The proposal of Full Union is in [kubernetes/community#388](https://github.com/kubernetes/community/pull/388).
+
+| Capability | Supported By This Proposal | Supported By Full Union |
+|---|---|---|---|
+| Auto clear missing fields on patch | X | X |
+| Merge union fields on patch | X | X |
+| Validate only 1 field set on type |  | X |
+| Validate discriminator field matches one-of field |  | X |
+| Support non-union patchKey | X | TBD |
+| Support arbitrary combinations of set fields | X |  |
+
+## Use cases
+
+- As a user patching a map, I want keys mutually exclusive with those that I am providing to automatically be cleared.
+
+- As a user running kubectl apply, when I update a field in my configuration file,
+I want mutually exclusive fields never specified in my configuration to be cleared.
+
+## Examples:
+
+- General Example: Keys in a Union are mutually exclusive. Clear unspecified union values in a Union that contains a discriminator.
+
+- Specific Example: When patching a Deployment .spec.strategy, clear .spec.strategy.rollingUpdate
+if it is not provided in the patch so that changing .spec.strategy.type will not fail.
+
+- General Example: Keys in a Union are mutually exclusive. Clear unspecified union values in a Union
+that does not contain a discriminator.
+
+- Specific Example: When patching a Pod .spec.volume, clear all volume fields except the one specified in the patch.
+
+## Proposed Changes
+
+### APIs
+
+**Scope**:
+
+| Union Type | Supported |
+|---|---|---|
+| non-inlined non-discriminated union | Yes |
+| non-inlined discriminated union | Yes |
+| inlined union with [patchMergeKey](https://github.com/kubernetes/community/blob/master/contributors/devel/api-conventions.md#strategic-merge-patch) only | Yes |
+| other inlined union | No |
+
+For the inlined union with patchMergeKey, we move the tag to the parent struct's instead of
+adding some logic to lookup the metadata in go struct of the inline union.
+Because the limitation of the latter is that the metadata associated with
+the inlined APIs will not be reflected in the OpenAPI schema.
+
+#### Tags
+
+old tags:
+
+1) `patchMergeKey`:
+It is the key to distinguish the entries in the list of non-primitive types. It must always be
+present to perform the merge on the list of non-primitive types, and will be preserved.
+
+2) `patchStrategy`:
+It indicates how to generate and merge a patch for lists. It could be `merge` or `replace`. It is optional for lists.
+
+new tags:
+
+`patchStrategy: replaceKeys`:
+
+We introduce a new value `replaceKeys` for `patchStrategy`.
+It indicates that all fields needing to be preserved must be present in the patch.
+And the fields that are present will be merged with live object. All the missing fields will be cleared when patching.
+
+#### Examples
+
+1) Non-inlined non-discriminated union:
+
+Type definition:
+```go
+type ContainerStatus struct {
+	...
+	// Add patchStrategy:"replaceKeys"
+	State ContainerState `json:"state,omitempty" protobuf:"bytes,2,opt,name=state" patchStrategy:"replaceKeys"``
+	...
+}
+```
+Live object:
+```yaml
+state:
+  running:
+    startedAt: ...
+```
+Local file config:
+```yaml
+state:
+  terminated:
+    exitCode: 0
+    finishedAt: ...
+```
+Patch:
+```yaml
+state:
+  $patch: replaceKeys
+  terminated:
+    exitCode: 0
+    finishedAt: ...
+```
+Result after merging
+```yaml
+state:
+  terminated:
+    exitCode: 0
+    finishedAt: ...
+```
+
+2) Non-inlined discriminated union:
+
+Type definition:
+```go
+type DeploymentSpec struct {
+	...
+	// Add patchStrategy:"replaceKeys"
+	Strategy DeploymentStrategy `json:"strategy,omitempty" protobuf:"bytes,4,opt,name=strategy" patchStrategy:"replaceKeys"`
+	...
+}
+```
+Since there are no fields associated with `recreate` in `DeploymentSpec`, I will use a generic example.
+
+Live object:
+```yaml
+unionName:
+  discriminatorName: foo
+  fooField:
+    fooSubfield: val1
+```
+Local file config:
+```yaml
+unionName:
+  discriminatorName: bar
+  barField:
+    barSubfield: val2
+```
+Patch:
+```yaml
+unionName:
+  $patch: replaceKeys
+  discriminatorName: bar
+  barField:
+    barSubfield: val2
+```
+Result after merging
+```yaml
+unionName:
+  discriminatorName: bar
+  barField:
+    barSubfield: val2
+```
+
+3) Inlined union with `patchMergeKey` only.
+This case is special, because `Volumes` already has a tag `patchStrategy:"merge"`.
+We change the tag to `patchStrategy:"merge|replaceKeys"`
+
+Type definition:
+```go
+type PodSpec struct {
+	...
+	// Add another value "replaceKeys" to patchStrategy
+	Volumes []Volume `json:"volumes,omitempty" patchStrategy:"merge|replaceKeys" patchMergeKey:"name" protobuf:"bytes,1,rep,name=volumes"`
+	...
+}
+```
+Live object:
+```yaml
+spec:
+  volumes:
+  - name: foo
+    emptyDir:
+      medium:
+        ...
+```
+Local file config:
+```yaml
+spec:
+  volumes:
+  - name: foo
+    hostPath:
+      path: ...
+```
+Patch:
+```yaml
+spec:
+  volumes:
+  - name: foo
+    $patch: replaceKeys
+    hostPath:
+      path: ...
+```
+Result after merging
+```yaml
+spec:
+  volumes:
+  - name: foo
+    hostPath:
+      path: ...
+```
+
+**Impacted APIs** are listed in the [Appendix](#appendix).
+
+### API server
+
+No required change.
+Auto clearing missing fields of a patch relies on package Strategic Merge Patch.
+We don't validate only 1 field is set in union in a generic way. We don't validate discriminator
+field matches one-of field. But we still rely on hardcoded per field based validation.
+
+### kubectl
+
+No required change.
+Changes about how to generate the patch rely on package Strategic Merge Patch.
+
+### Strategic Merge Patch
+**Background**
+Strategic Merge Patch is a package used by both client and server. A typical usage is that a client
+calls the function to calculate the patch and the API server calls another function to merge the patch.
+
+We need to make sure the client always sends a patch that includes all of the fields that it wants to keep.
+When merging, auto clear missing fields of a patch if the patch has a directive `$patch: replaceKeys`
+
+### Open API
+
+Update OpenAPI schema.
+
+## Version Skew
+
+The changes are all backward compatible.
+
+Old kubectl vs New server: All behave the same as before, since no new directive in the patch.
+
+New kubectl vs Old server: All behave the same as before, since new directive will not be recognized
+by the old server and it will be dropped in conversion; Unchanged fields will not affect the merged result.
+
+# Alternatives Considered
+
+The proposals below are not mutually exclusive with the proposal above, and maybe can be added at some point in the future.
+
+# 1. Add Discriminators in All Unions/OneOf APIs
+
+Original issue is described in kubernetes/kubernetes#35345
+
+## Analysis
+
+### Behavior
+
+If the discriminator were set, we'd require that the field corresponding to its value were set and the APIServer (registry) could automatically clear the other fields.
+
+If the discriminator were unset, behavior would be as before -- exactly one of the fields in the union/oneof would be required to be set and the operation would otherwise fail validation.
+
+We should set discriminators by default. This means we need to change it accordingly when the corresponding union/oneof fields were set and unset.
+
+## Proposed Changes
+
+### API
+Add a discriminator field in all unions/oneof APIs. The discriminator should be optional for backward compatibility. There is an example below, the field `Type` works as a discriminator.
+```go
+type PersistentVolumeSource struct {
+...
+	// Discriminator for PersistentVolumeSource, it can be "gcePersistentDisk", "awsElasticBlockStore" and etc.
+	// +optional
+	Type *string `json:"type,omitempty" protobuf:"bytes,24,opt,name=type"`
+}
+```
+
+### API Server
+
+We need to add defaulting logic described in the [Behavior](#behavior) section.
+
+### kubectl
+
+No change required on kubectl.
+
+## Summary
+
+Limitation: Server-side automatically clearing fields based on discriminator may be unsafe.
+
+# Appendix
+
+## List of Impacted APIs
+In `pkg/api/v1/types.go`:
+- [`VolumeSource`](https://github.com/kubernetes/kubernetes/blob/v1.5.2/pkg/api/v1/types.go#L235):
+It is inlined. Besides `VolumeSource`. its parent [Volume](https://github.com/kubernetes/kubernetes/blob/v1.5.2/pkg/api/v1/types.go#L222) has `Name`.
+- [`PersistentVolumeSource`](https://github.com/kubernetes/kubernetes/blob/v1.5.2/pkg/api/v1/types.go#L345):
+It is inlined. Besides `PersistentVolumeSource`, its parent [PersistentVolumeSpec](https://github.com/kubernetes/kubernetes/blob/v1.5.2/pkg/api/v1/types.go#L442) has the following fields:
+```go
+Capacity ResourceList `json:"capacity,omitempty" protobuf:"bytes,1,rep,name=capacity,casttype=ResourceList,castkey=ResourceName"`
+// +optional
+AccessModes []PersistentVolumeAccessMode `json:"accessModes,omitempty" protobuf:"bytes,3,rep,name=accessModes,casttype=PersistentVolumeAccessMode"`
+// +optional
+ClaimRef *ObjectReference `json:"claimRef,omitempty" protobuf:"bytes,4,opt,name=claimRef"`
+// +optional
+PersistentVolumeReclaimPolicy PersistentVolumeReclaimPolicy `json:"persistentVolumeReclaimPolicy,omitempty" protobuf:"bytes,5,opt,name=persistentVolumeReclaimPolicy,casttype=PersistentVolumeReclaimPolicy"`
+```
+- [`Handler`](https://github.com/kubernetes/kubernetes/blob/v1.5.2/pkg/api/v1/types.go#L1485):
+It is inlined. Besides `Handler`, its parent struct [`Probe`](https://github.com/kubernetes/kubernetes/blob/v1.5.2/pkg/api/v1/types.go#L1297) also has the following fields:
+```go
+// +optional
+InitialDelaySeconds int32 `json:"initialDelaySeconds,omitempty" protobuf:"varint,2,opt,name=initialDelaySeconds"`
+// +optional
+TimeoutSeconds int32 `json:"timeoutSeconds,omitempty" protobuf:"varint,3,opt,name=timeoutSeconds"`
+// +optional
+PeriodSeconds int32 `json:"periodSeconds,omitempty" protobuf:"varint,4,opt,name=periodSeconds"`
+// +optional
+SuccessThreshold int32 `json:"successThreshold,omitempty" protobuf:"varint,5,opt,name=successThreshold"`
+// +optional
+FailureThreshold int32 `json:"failureThreshold,omitempty" protobuf:"varint,6,opt,name=failureThreshold"`
+````
+- [`ContainerState`](https://github.com/kubernetes/kubernetes/blob/v1.5.2/pkg/api/v1/types.go#L1576):
+It is NOT inlined.
+- [`PodSignature`](https://github.com/kubernetes/kubernetes/blob/v1.5.2/pkg/api/v1/types.go#L2953):
+It has only one field, but the comment says "Exactly one field should be set". Maybe we will add more in the future? It is NOT inlined.
+In `pkg/authorization/types.go`:
+- [`SubjectAccessReviewSpec`](https://github.com/kubernetes/kubernetes/blob/v1.5.2/pkg/apis/authorization/types.go#L108):
+Comments says: `Exactly one of ResourceAttributes and NonResourceAttributes must be set.`
+But there are some other non-union fields in the struct.
+So this is similar to INLINED struct.
+- [`SelfSubjectAccessReviewSpec`](https://github.com/kubernetes/kubernetes/blob/v1.5.2/pkg/apis/authorization/types.go#L130):
+It is NOT inlined.
+
+In  `pkg/apis/extensions/v1beta1/types.go`:
+- [`DeploymentStrategy`](https://github.com/kubernetes/kubernetes/blob/v1.5.2/pkg/apis/extensions/types.go#L249):
+It is NOT inlined.
+- [`NetworkPolicyPeer`](https://github.com/kubernetes/kubernetes/blob/v1.5.2/pkg/apis/extensions/v1beta1/types.go#L1340):
+It is NOT inlined.
+- [`IngressRuleValue`](https://github.com/kubernetes/kubernetes/blob/v1.5.2/pkg/apis/extensions/v1beta1/types.go#L876):
+It says "exactly one of the following must be set". But it has only one field.
+It is inlined. Its parent [`IngressRule`](https://github.com/kubernetes/kubernetes/blob/v1.5.2/pkg/apis/extensions/v1beta1/types.go#L848) also has the following fields:
+```go
+// +optional
+Host string `json:"host,omitempty" protobuf:"bytes,1,opt,name=host"`
+```