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 00000000000..bec26f968cf --- /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"` +```