community/contributors/design-proposals/add-new-patchStrategy-to-clear-fields-not-present-in-patch.md

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.

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 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:

type ContainerStatus struct {
	...
	// Add patchStrategy:"replaceKeys"
	State ContainerState `json:"state,omitempty" protobuf:"bytes,2,opt,name=state" patchStrategy:"replaceKeys"``
	...
}

Live object:

state:
  running:
    startedAt: ...

Local file config:

state:
  terminated:
    exitCode: 0
    finishedAt: ...

Patch:

state:
  $patch: replaceKeys
  terminated:
    exitCode: 0
    finishedAt: ...

Result after merging

state:
  terminated:
    exitCode: 0
    finishedAt: ...

2. Non-inlined discriminated union:

Type definition:

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:

unionName:
  discriminatorName: foo
  fooField:
    fooSubfield: val1

Local file config:

unionName:
  discriminatorName: bar
  barField:
    barSubfield: val2

Patch:

unionName:
  $patch: replaceKeys
  discriminatorName: bar
  barField:
    barSubfield: val2

Result after merging

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:

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:

spec:
  volumes:
  - name: foo
    emptyDir:
      medium:
        ...

Local file config:

spec:
  volumes:
  - name: foo
    hostPath:
      path: ...

Patch:

spec:
  volumes:
  - name: foo
    $patch: replaceKeys
    hostPath:
      path: ...

Result after merging

spec:
  volumes:
  - name: foo
    hostPath:
      path: ...

Impacted APIs are listed in the 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.

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 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: It is inlined. Besides VolumeSource. its parent Volume has Name.
• PersistentVolumeSource: It is inlined. Besides PersistentVolumeSource, its parent PersistentVolumeSpec has the following fields:

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: It is inlined. Besides Handler, its parent struct Probe also has the following fields:

// +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: It is NOT inlined.
• PodSignature: 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: 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: It is NOT inlined.

In pkg/apis/extensions/v1beta1/types.go:

• DeploymentStrategy: It is NOT inlined.
• NetworkPolicyPeer: It is NOT inlined.
• IngressRuleValue: It says "exactly one of the following must be set". But it has only one field. It is inlined. Its parent IngressRule also has the following fields:

// +optional
Host string `json:"host,omitempty" protobuf:"bytes,1,opt,name=host"`