Proposal: Add Tag Indicating To Replace Union Fields in Patch #278

mengqiy · 2017-01-20T07:19:27Z

Move from issue #229.

@kubernetes/sig-api-machinery-misc @kubernetes/sig-cli-misc

liggitt · 2017-01-20T15:50:31Z

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

+
+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.
+
+2) Add some logic when lookup the metadata in go struct to find the tags for inline fields.


I'd like to avoid adding more paths that require access to the go struct, and cannot be expressed in the openapi schema

We will need to add the existing mergeStrategy for lists as extensions to the open-api schema. Can we just do the same for this at the same time?

openapi schemas flatten inlined structs

I believe that is ok for the current use cases, we can just put it in the parent right?

right... that's option 1 :)

Yup makes sense. I miss interpreted your original comment.

liggitt · 2017-01-20T15:52:01Z

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

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


I don't see how you can compatibly add more non-union fields to a type you have defined as a union type, so I'm not sure that loss of flexibility is a dealbreaker

can you summarize current usage of union types, along with any that inline peer to other fields (like this name+volumeSource example)? If a significant number fall into the name+union pattern, we could describe a "named union" that gives special significance to the "name" field, and could be understood via the openapi schema.

I think @ymqytw has a list of these. I know rollout strategy for Deployment is another.

I have a list in the appendix, but it doesn't have enough information you need. I will update it according to your comment.

Would be useful to include in this document all of the union types as of 1.5 and what the tag additions would be for each of them.

Can we maybe live with existing inlined unions and ban it for future use?

i.e., VolumeSource should not have been inlined, but should have been a nested object.

@thockin I think that is the proposal here. The compromise would be that each Volume would need to be entirely managed by apply + configuration file or not managed by it at all. e.g. A Volume could not have some fields managed by apply and come fields managed by another writer (e.g. some controller).

liggitt · 2017-01-20T15:55:52Z

cc @jwforres

pwittrock · 2017-01-23T03:09:43Z

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

+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.
+
+The change in Strategic Merge Patch package will affect the behavior of how kubectl creating a patch and how API server apply a patch.


Add a section about backwards / forward compatibility behavior. Are there any issues here? I think we might need to be careful for Volumes where the name may not be present in the old patches that expect merge, and will be lost. I wonder if we can solve this with a new patch strategy in some way.

pwittrock · 2017-01-23T03:12:18Z

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

+
+# 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.


Is this true? Can you explain this more?

pwittrock · 2017-01-23T03:15:34Z

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

+
+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.
+
+# Alternatives Considered


I'd mention that the proposals below are not mutually exclusive with the proposal above, and could be added at some point in the future.

pwittrock · 2017-01-23T03:21:02Z

@bgrant0607 Is this something you would want to review? This seems like a minimally invasive way to get support for unions in apply and I don't think it prevents us from implementing Unions in the future.

@lavalamp FYI, I already briefly talked with you about this, but in case you wanted the design doc as a follow up. The only required api changes would be adding the struct tags and some changes to the patch merge logic.

bgrant0607 · 2017-01-23T08:14:01Z

@pwittrock Yes, thanks, I'd like to take a look.

pwittrock · 2017-01-23T20:31:26Z

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

+
+## Strategic Merge Patch
+
+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. 


How would this be implemented? Does the server know to replace the entire object? Or does the client explicitly clear these fields?

mengqiy · 2017-01-24T21:21:24Z

Updated the proposal to address the comments.

thockin · 2017-01-24T23:08:21Z

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

+
+### APIs
+
+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:


If this is the proposal, should we encourage or discourage discriminator fields for oneof cases? We have both, and there will be more.

We should encourage discriminators:
kubernetes/kubernetes#35345

bgrant0607 · 2017-01-27T17:20:27Z

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

+
+*Backwards / Forward Compatibility Behavior using option 1*
+
+We reuse `patchStrategy:"replace"` if possible. Otherwise, we introduce a new struct tag, `patchStrategyForListEntry:"replace"`.


I think if we use replace, then automatically set fields within the replaced entry will be overwritten. For defaults, it doesn't matter, because the same defaults would be set again, but any other automatically set fields would be lost, even if updating the same entry of the union.

bgrant0607 · 2017-01-27T18:11:24Z

@ymqytw Sorry, this is pretty long. Could you please summarize the proposal?

mengqiy · 2017-01-27T18:21:19Z

@bgrant0607 Sorry about that. Will summarize it today.

liggitt · 2017-02-01T06:40:00Z

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

-	Name string `json:"name" protobuf:"bytes,1,opt,name=name"`
-	VolumeSource `json:",inline" protobuf:"bytes,2,opt,name=volumeSource"`
-}
+We reuse `patchStrategy:"replace"` if possible. This kind of patch will be backward compatible.


would the replace strategy prevent merging of descendants?

liggitt · 2017-02-01T06:44:29Z

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

-}
+We reuse `patchStrategy:"replace"` if possible. This kind of patch will be backward compatible.
+
+Otherwise, we introduce a new struct tag, `patchStrategyForListEntry:"replace"`, for a list of inlined unions. This kind of patch will not be backward compatible.


isn't the point of a union type to ensure only one of a set of fields is set at that level in the object? both replace tags seem to remove the possibility of merging further down in the tree if the specified member of the union type didn't change.

Yes, this is potential problem. Thanks for pointing this out.
I guess an solution is:

replace the whole struct if the union field changes.

merge, if the union field is still the old one.

WDYT?

that's what I would expect a union-type tag to accomplish...

@liggitt a union-type tag would be a super set of what is proposed here. This is an incremental step in that direction, but requires fewer changes to the API.

This proposal allows us to defer some of the following design questions:

Do we validate that only 1 field is set?

How do we validate and express discriminators? Do that tags define which field matches a discriminator value?

What about patchKey fields? Are these exempt from the union?

What about fields that are not pure unions, and 2 fields may be set together where neither is a discriminator, but are mutually exclusive with other fields that may have defaulted values.

mengqiy · 2017-02-03T22:04:42Z

Updated the proposal. PTAL. Thanks.

pwittrock · 2017-02-06T23:11:18Z

@bgrant0607 here is a quick summary:

This proposal is to define a tag where, if set, for a given type all keys/fields missing from the request will be cleared when patching the object. The goal is to defer making decisions around validating the correctness of unions, while enabling more correct patch behavior for union types.

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 1-of field		X
Support non-union patchKey	X	TBD
Support arbitrary combinations of set fields	X

This proposal supports any type where all of the desired fields must be set on every patch request. It is agnostic to details such as if the type is a union or something else. This makes things like supporting patchKeys simpler.

adohe-zz · 2017-02-20T13:02:18Z