support multiple keys as merge key

kubernetes · Mar 23, 2017 · c650c37 · c650c37
1 parent 945936c
commit c650c37
Showing 1 changed file with 336 additions and 0 deletions.
diff --git a/contributors/design-proposals/key-set-as-merge-key.md b/contributors/design-proposals/key-set-as-merge-key.md
@@ -0,0 +1,336 @@
+# Key Set as Merge Key in Strategic Merge Patch
+
+## Abstract
+
+Support a key set as the merge key in Strategic Merge Patch.
+
+## Motivation
+
+*Background*: In Strategic Merge Patch, Merge Key is the key to distinguish the entries in the list of non-primitive types.
+It must always be present and unique to perform the merge on the list of non-primitive types,
+and will be preserved.
+
+The issue in practice is that sometimes we cannot find a key that always be present and unique.
+[#39188](https://github.com/kubernetes/kubernetes/issues/39188) is a typical example. We need to
+support a key set as Merge Key. A key set must contain at least 1 key.
+
+## Proposed Change
+
+### API Change
+
+We will update the struct tags for the API resources whose Merge Key is not guaranteed to be always
+present and unique. The keys will be seperated by "|", i.e. `patchMergeKey:"<key1>|<key2>|<key3>"`.
+
+E.g. `Ports` in `ServiceSpec`.
+```go
+type ServiceSpec struct {
+  // Change patchMergeKey from "port" to "name|port"
+  Ports []ServicePort `json:"ports,omitempty" patchStrategy:"merge" patchMergeKey:"name|port" protobuf:"bytes,1,rep,name=ports"`
+  ...
+}
+```
+
+All the impacted APIs are listed in section [Impacted APIs](#impacted-apis)
+
+### Open API
+
+Update Open API schema to have a string that contains multiple keys for `patchMergeKey`.
+The change should be trivial.
+
+### Strategic Merge Patch pkg
+
+Entries are considered as the same if and only if all the keys in the key set are identical.
+We allow keys to be missing in the key set as long as it is not empty.
+
+Take `Ports` as an example:
+
+Suppose we are using `name` and `port` as merge key as mentioned in section [API Change](#api-change)
+
+We have live config:
+```yaml
+spec:
+  type: NodePort
+  ports:
+    - protocol: UDP
+      port: 30420
+      nodePort: 30420
+```
+
+We want to add another port, and the user's local config file becomes:
+```yaml
+spec:
+  type: NodePort
+  ports:
+    - protocol: UDP
+      port: 30420
+      name: udpport
+      nodePort: 30420
+    - protocol: TCP
+      port: 30420
+      name: tcpport
+      nodePort: 30420
+```
+
+The patch we send should be:
+```yaml
+spec:
+  type: NodePort
+  ports:
+    # the entry with key set {port: 30420} is considered missing in local config file
+    - port: 30420
+      $patch: delete
+    # the entry with key set {name: udpport, port: 30420} is considered as a new entry
+    - protocol: UDP
+      port: 30420
+      name: udpport
+      nodePort: 30420
+    # the entry with key set {name: tcpport, port: 30420} is a new entry
+    - protocol: TCP
+      port: 30420
+      name: tcpport
+      nodePort: 30420
+```
+
+### Docs
+
+Document what the developer should consider when adding an API with `mergeKey`.
+
+## Version Skew
+
+Some changes are not backward compatible, e.g. moving the `mergeKey` and `patchStrategy` from field level to the list level.
+
+The plan to not break backward compatibility:
+
+In 1.7, add the logic support for key set, but keep the APIs' tags unchanged.
+Meanwhile, `kubectl` will move to using OpenAPI spec instead of compiled in types hopefully.
+In 1.8, we introduce the changes to APIs' tags. The changes will be backward compatible.
+
+## Impacted APIs
+
+Impacted APIs are grouped into two groups:
+- Broken APIs
+- Need auditing APIs
+
+### Broken APIs
+(1a) `ContainerPort`: Change merge key from `containerPort` to `name|containerPort`.
+
+Usage of [ContainerPort](https://github.com/kubernetes/kubernetes/blob/db9fcb06295b3db49be8efa5c4584114af0696bc/pkg/api/v1/types.go#L1637)
+and its [definition](https://github.com/kubernetes/kubernetes/blob/db9fcb06295b3db49be8efa5c4584114af0696bc/pkg/api/v1/types.go#L1286).
+```go
+type Container struct {
+Ports []ContainerPort `json:"ports,omitempty" patchStrategy:"merge" patchMergeKey:"containerPort" protobuf:"bytes,6,rep,name=ports"`
+...
+}
+```
+```go
+type ContainerPort struct {
+	// +optional
+	Name string `json:"name,omitempty" protobuf:"bytes,1,opt,name=name"`
+	// +optional
+	HostPort int32 `json:"hostPort,omitempty" protobuf:"varint,2,opt,name=hostPort"`
+	ContainerPort int32 `json:"containerPort" protobuf:"varint,3,opt,name=containerPort"`
+	// +optional
+	Protocol Protocol `json:"protocol,omitempty" protobuf:"bytes,4,opt,name=protocol,casttype=Protocol"`
+	// +optional
+	HostIP string `json:"hostIP,omitempty" protobuf:"bytes,5,opt,name=hostIP"`
+}
+```
+
+(1b) `ServicePort`: Similar to `ContainerPort`. Change merge key from `port` to `name|port`.
+
+Usage of [ServicePort](https://github.com/kubernetes/kubernetes/blob/db9fcb06295b3db49be8efa5c4584114af0696bc/pkg/api/v1/types.go#L2777)
+and its [definition](https://github.com/kubernetes/kubernetes/blob/db9fcb06295b3db49be8efa5c4584114af0696bc/pkg/api/v1/types.go#L2867).
+```go
+type ServiceSpec struct {
+	Ports []ServicePort `json:"ports,omitempty" patchStrategy:"merge" patchMergeKey:"port" protobuf:"bytes,1,rep,name=ports"`
+  ...
+}
+```
+```go
+type ServicePort struct {
+	// +optional
+	Name string `json:"name,omitempty" protobuf:"bytes,1,opt,name=name"`
+	// +optional
+	Protocol Protocol `json:"protocol,omitempty" protobuf:"bytes,2,opt,name=protocol,casttype=Protocol"`
+	Port int32 `json:"port" protobuf:"varint,3,opt,name=port"`
+	// +optional
+	TargetPort intstr.IntOrString `json:"targetPort,omitempty" protobuf:"bytes,4,opt,name=targetPort"`
+	// +optional
+	NodePort int32 `json:"nodePort,omitempty" protobuf:"varint,5,opt,name=nodePort"`
+}
+```
+
+(2a) `NodeSelectorRequirement`: The merge key and patch strategy is not used correctly, they should be in the list
+level (`[]NodeSelectorRequirement`) instead of field level. We should move them out.
+
+Usage of [MatchExpressions](https://github.com/kubernetes/kubernetes/blob/db9fcb06295b3db49be8efa5c4584114af0696bc/pkg/api/v1/types.go#L1971)
+and its [definition](https://github.com/kubernetes/kubernetes/blob/db9fcb06295b3db49be8efa5c4584114af0696bc/pkg/api/v1/types.go#L1976).
+
+```go
+type NodeSelectorTerm struct {
+	MatchExpressions []NodeSelectorRequirement `json:"matchExpressions" protobuf:"bytes,1,rep,name=matchExpressions"`
+  ...
+}
+```
+```go
+type NodeSelectorRequirement struct {
+	Key string `json:"key" patchStrategy:"merge" patchMergeKey:"key" protobuf:"bytes,1,opt,name=key"`
+	Operator NodeSelectorOperator `json:"operator" protobuf:"bytes,2,opt,name=operator,casttype=NodeSelectorOperator"`
+	// +optional
+	Values []string `json:"values,omitempty" protobuf:"bytes,3,rep,name=values"`
+}
+```
+
+(2b) `Taint`: Similar to `NodeSelectorRequirement`. Tags should be in the list level.
+
+Usage of [Taints](https://github.com/kubernetes/kubernetes/blob/db9fcb06295b3db49be8efa5c4584114af0696bc/pkg/api/v1/types.go#L3114)
+and its [definition](https://github.com/kubernetes/kubernetes/blob/db9fcb06295b3db49be8efa5c4584114af0696bc/pkg/api/v1/types.go#L2160).
+```go
+type NodeSpec struct {
+  Taints []Taint `json:"taints,omitempty" protobuf:"bytes,5,opt,name=taints"`
+  ...
+}
+```
+```go
+type Taint struct {
+	Key string `json:"key" patchStrategy:"merge" patchMergeKey:"key" protobuf:"bytes,1,opt,name=key"`
+	// +optional
+	Value string `json:"value,omitempty" protobuf:"bytes,2,opt,name=value"`
+	Effect TaintEffect `json:"effect" protobuf:"bytes,3,opt,name=effect,casttype=TaintEffect"`
+	// +optional
+	TimeAdded metav1.Time `json:"timeAdded,omitempty" protobuf:"bytes,4,opt,name=timeAdded"`
+}
+```
+
+(2c) `Toleration`: Similar to `NodeSelectorRequirement`. Tags should be in the list level.
+
+Usage of [Tolerations](https://github.com/kubernetes/kubernetes/blob/db9fcb06295b3db49be8efa5c4584114af0696bc/pkg/api/v1/types.go#L2375)
+and its [definition](https://github.com/kubernetes/kubernetes/blob/db9fcb06295b3db49be8efa5c4584114af0696bc/pkg/api/v1/types.go#L2200)
+```go
+type PodSpec struct {
+  Tolerations []Toleration `json:"tolerations,omitempty" protobuf:"bytes,22,opt,name=tolerations"`
+  ...
+}
+```
+```go
+type Toleration struct {
+	// +optional
+	Key string `json:"key,omitempty" patchStrategy:"merge" patchMergeKey:"key" protobuf:"bytes,1,opt,name=key"`
+	// +optional
+	Operator TolerationOperator `json:"operator,omitempty" protobuf:"bytes,2,opt,name=operator,casttype=TolerationOperator"`
+	// +optional
+	Value string `json:"value,omitempty" protobuf:"bytes,3,opt,name=value"`
+	// +optional
+	Effect TaintEffect `json:"effect,omitempty" protobuf:"bytes,4,opt,name=effect,casttype=TaintEffect"`
+	// +optional
+	TolerationSeconds *int64 `json:"tolerationSeconds,omitempty" protobuf:"varint,5,opt,name=tolerationSeconds"`
+}
+```
+
+### Need auditing APIs
+
+The API owner should audit these APIs to make sure the Merge Key are guaranteed to be the unique identifier.
+
+1) [OwnerReferences](https://github.com/kubernetes/kubernetes/blob/db9fcb06295b3db49be8efa5c4584114af0696bc/pkg/api/v1/types.go#L200)
+```go
+type OwnerReference struct {
+  OwnerReferences []metav1.OwnerReference `json:"ownerReferences,omitempty" patchStrategy:"merge" patchMergeKey:"uid" protobuf:"bytes,13,rep,name=ownerReferences"`
+...
+}
+```
+
+2) [Env](https://github.com/kubernetes/kubernetes/blob/db9fcb06295b3db49be8efa5c4584114af0696bc/pkg/api/v1/types.go#L1649)
+```go
+type Container struct {
+  Env []EnvVar `json:"env,omitempty" patchStrategy:"merge" patchMergeKey:"name" protobuf:"bytes,7,rep,name=env"`
+  ...
+}
+``` 
+
+3) [Volumes](https://github.com/kubernetes/kubernetes/blob/db9fcb06295b3db49be8efa5c4584114af0696bc/pkg/api/v1/types.go#L2261),
+[InitContainers](https://github.com/kubernetes/kubernetes/blob/db9fcb06295b3db49be8efa5c4584114af0696bc/pkg/api/v1/types.go#L2275),
+[Containers](https://github.com/kubernetes/kubernetes/blob/db9fcb06295b3db49be8efa5c4584114af0696bc/pkg/api/v1/types.go#L2281)
+and [ImagePullSecrets](https://github.com/kubernetes/kubernetes/blob/db9fcb06295b3db49be8efa5c4584114af0696bc/pkg/api/v1/types.go#L2357).
+```go
+type PodSpec struct {
+  Volumes []Volume `json:"volumes,omitempty" patchStrategy:"merge" patchMergeKey:"name" protobuf:"bytes,1,rep,name=volumes"`
+  InitContainers []Container `json:"initContainers,omitempty" patchStrategy:"merge" patchMergeKey:"name" protobuf:"bytes,20,rep,name=initContainers"`
+  Containers []Container `json:"containers" patchStrategy:"merge" patchMergeKey:"name" protobuf:"bytes,2,rep,name=containers"`
+  ImagePullSecrets []LocalObjectReference `json:"imagePullSecrets,omitempty" patchStrategy:"merge" patchMergeKey:"name" protobuf:"bytes,15,rep,name=imagePullSecrets"`
+  ...
+}
+```
+
+4) [Secrets](https://github.com/kubernetes/kubernetes/blob/db9fcb06295b3db49be8efa5c4584114af0696bc/pkg/api/v1/types.go#L2962)
+```go
+type ServiceAccount struct {
+  Secrets []ObjectReference `json:"secrets,omitempty" patchStrategy:"merge" patchMergeKey:"name" protobuf:"bytes,2,rep,name=secrets"`
+  ...
+}
+```
+
+5) [Addresses](https://github.com/kubernetes/kubernetes/blob/db9fcb06295b3db49be8efa5c4584114af0696bc/pkg/api/v1/types.go#L3187)
+```go
+type NodeStatus struct {
+  Addresses []NodeAddress `json:"addresses,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,5,rep,name=addresses"`???
+  ...
+}
+```
+
+6) All `Conditions` using the same pattern.
+In `v1/`:
+```go
+type PodStatus struct {
+  Conditions []PodCondition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,2,rep,name=conditions"`
+  ...
+}
+
+type ReplicationControllerStatus struct {
+  Conditions []ReplicationControllerCondition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,6,rep,name=conditions"`
+  ...
+}
+
+type NodeStatus struct {
+  Conditions []NodeCondition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,4,rep,name=conditions"`???
+  ...
+}
+
+type ComponentStatus struct {
+  Conditions []ComponentCondition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,2,rep,name=conditions"`
+  ...
+}
+```
+
+In `extensions/`:
+```go
+type DeploymentStatus struct {
+  Conditions []DeploymentCondition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,6,rep,name=conditions"`
+  ...
+}
+
+type ReplicaSetStatus struct {
+  Conditions []ReplicaSetCondition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,6,rep,name=conditions"`
+  ...
+}
+
+type DeploymentStatus struct {
+  Conditions []DeploymentCondition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,6,rep,name=conditions"`
+  ...
+}
+```
+
+In `apps/`:
+```go
+type DeploymentStatus struct {
+  Conditions []DeploymentCondition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,6,rep,name=conditions"`
+  ...
+}
+```
+
+In `batch/`:
+```go
+type JobStatus struct {
+  Conditions []JobCondition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,1,rep,name=conditions"`
+  ...
+}
+```