address comments

contributors/design-proposals/add-metadata-indicating-to-replace-union-in-patch.md

@@ -7,13 +7,92 @@ This approach could be used to fix the issues with `kubectl apply` simply by hav
 
 ### APIs
 
-Add a go struct tag/openAPI value telling clients to replace the entire struct when doing a *PATCH*. For the *inline* fields, we have 2 options:
+Add a go struct tag/openAPI value telling clients to replace the entire struct when doing a *PATCH*. For the *inlined* fields, we have 2 options:
 
-1) Move the tag to the parent struct's. E.g. add a tag to `Volume` instead of `VolumeSource`. This will limit the flexibility to add more fields in the future.
+1) Move the tag to the parent struct's. e.g. add a tag to `Volume` instead of `VolumeSource`. 
+This will limit the flexibility to add more fields in the parent struct that already has an inlined union. 
+And we need to examine all the impacted APIs to check if it is safe to replace the entire parent struct. e.g. if some controller is updating some fields.
+
+Examples of how to add go struct tag for option 1:
+```go
+type PodSpec struct {
+	...
+	// patchStrategyForListEntry:"replace" is an additional struct tag indicating we replace the entire entry in the list, if they have the same merge key. 
+	Volumes []Volume `json:"volumes,omitempty" patchStrategy:"merge" patchMergeKey:"name" patchStrategyForListEntry:"replace" protobuf:"bytes,1,rep,name=volumes"`
+	...
+}
+type Volume struct {
+	Name string `json:"name" protobuf:"bytes,1,opt,name=name"`
+	VolumeSource `json:",inline" protobuf:"bytes,2,opt,name=volumeSource"`
+}
+
+type DeploymentSpec struct {
+	...
+	Strategy DeploymentStrategy `json:"strategy,omitempty" protobuf:"bytes,4,opt,name=strategy" patchStrategy:"replace"`
+	...
+}
+```
+
+*Backwards / Forward Compatibility Behavior using option 1*
+
+We reuse `patchStrategy:"replace"` if possible. Otherwise, we introduce a new struct tag, `patchStrategyForListEntry:"replace"`.
+
+If we are reusing `patchStrategy:"replace"`, the new patch has no new fields. So an old API server still understands the new patch and is able to apply the patch.
+
+The new patch for non-inlined union will looks like:
+```yaml
+union-key:
+  $patch: replace
+  key1: v1
+  key2: v2
+```
+
+The new patch for inlined union will looks like:
+```yaml
+parent-key:
+  $patch: replace
+  inlined-union-key1: v1
+  inlined-union-key2: v2
+  non-union-key1: v3
+  non-union-key2: v4
+```
+
+We cannot reuse `patchStrategy:"replace"` for a list of unions, because `patchStrategy:"replace"` means replacing the entire list.
+
+E.g. `Volume` is the parent of `VolumeSource`. And `Volumes`, which is a list of `Volume`, has the metadata `patchStrategy:"merge"`.
+We cannot use `patchStrategy:"replace"` in this case.
+
+The struct tags are listed as below:
+```go
+type PodSpec struct {
+	...
+	Volumes []Volume `json:"volumes,omitempty" patchStrategy:"merge" patchMergeKey:"name" protobuf:"bytes,1,rep,name=volumes"`
+	...
+}
+```
+In this case, we introduce struct tag`patchStrategyForListEntry:"replace"`.
+
+The new patch for list of inlined union will looks like:
+```yaml
+podSpec:
+  volumes:
+  - name: vol1
+    $listEntryStrategy: replace
+    emptyDir:
+      ...
+  - name: vol2
+    $listEntryStrategy: replace
+    hostPath:
+      ...
+```
+
+Impacted APIs are listed in the [Appendix](#appendix).
 
 2) Add some logic when lookup the metadata in go struct to find the tags for inline fields.
+The limitation of this approach is that the metadata associated with the inlined APIs will not be reflected in the OpenAPI schema, 
+because OpenAPI schemas flatten inlined structs.
 
-Examples of how to add go struct tag respect option 2:
+Examples of how to add go struct tag for option 2:
 ```go
 type Volume struct {
 	Name string `json:"name" protobuf:"bytes,1,opt,name=name"`
@@ -31,22 +110,41 @@ type DeploymentSpec struct {
 
 No required change.
 
-## Strategic Merge Patch
+### Open API
 
-Add logic to support replacing the entire union fields when there is a `replace` directive. The logic will be similar to the logic of merge strategy for lists. 
-Add lookup logic to support lookup metadata of inline fields.
+We would need to add extensions to the openapi spec we publish. This is something we already need to do for the `patchStrategy` and `mergeKey` struct tags.
 
-The change in Strategic Merge Patch package will affect the behavior of how kubectl creating a patch and how API server apply a patch.
+### Strategic Merge Patch
 
-### Open API
+The logic of `$patch: replace` already exists.
 
-We would need to add extensions to the openapi spec we publish. This is something we already need to do for the `patchStrategy` and `mergeKey` struct tags.
+We need to add additional logic to support:
+- Generate the correct patch for a list of maps according to the struct tag. 
+- Replace the entire union when `$listEntryStrategy: replace` appears in the patch.
+
+### Docs
+
+Update `API-conventions-md` to include: 
+```
+we should avoid adding new inlined unions in the future.
+```
 
 # Alternatives Considered
 
+The proposals below are not mutually exclusive with the proposal above, and could be added at some point in the future.
+
 # 1. Add Union Support to the API Server
 
-The limitation is that it will be impossible or very hard to make it work for inline union types.
+The limitation is that it will be very hard to make it work for inline union types. E.g.
+```yaml
+parent-key:
+  $patch: replace
+  inlined-union-key1: v1
+  inlined-union-key2: v2
+  non-union-key1: v3
+  non-union-key2: v4
+```
+If we want to use an approach similar to option 1 in section [APIs](#apis) of the proposal above, using `union:"oneof"` is not sufficient. Because the validator doesn't have enough metadata to tell which fields belongs to the inlined union.
 
 ## Proposed Changes
 
@@ -264,17 +362,54 @@ Apply the new config by `kubectl apply -f`. The operation should succeed now, be
 
 ## List of Impacted APIs
 In `pkg/api/v1/types.go`:
-- [`VolumeSource`](https://github.com/kubernetes/kubernetes/blob/master/pkg/api/v1/types.go#L235)
-- [`PersistentVolumeSource`](https://github.com/kubernetes/kubernetes/blob/master/pkg/api/v1/types.go#L345)
-- [`Handler`](https://github.com/kubernetes/kubernetes/blob/master/pkg/api/v1/types.go#L1485)
-- [`ContainerState`](https://github.com/kubernetes/kubernetes/blob/master/pkg/api/v1/types.go#L1576)
-- [`PodSignature`](https://github.com/kubernetes/kubernetes/blob/master/pkg/api/v1/types.go#L2973): It has only one field, but the comment says "Exactly one field should be set". Maybe we will add more in the future?
-
+- [`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/master/pkg/apis/authorization/types.go#L108)Exactly one of ResourceAttributes and NonResourceAttributes must be set. But other non-union fields also exist in this API.
-- [`SelfSubjectAccessReviewSpec`](https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/authorization/types.go#L130)
+- [`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/master/pkg/apis/extensions/types.go#L249)
-- [`NetworkPolicyPeer`](https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/extensions/v1beta1/types.go#L1188)
-- [`IngressRuleValue`](https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/extensions/v1beta1/types.go#L733): It says "exactly one of the following must be set". But it has only one field.
+- [`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"`
+```