Initial KEP for API union/oneOf #926
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
/kind api-change |
k8s-ci-robot
added
kind/api-change
|
/assign @liggitt |
|
Note that a lot of the logic is already written here: Steps missing:
|
|
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
| TO be a string, and CAN be optional. | ||
|
|
||
| Note that this means that only one "union" type can be defined per structure, | ||
| which is a deliberate decision. |
There was a problem hiding this comment.
Can I effectively get multiple by using embedding?
There was a problem hiding this comment.
Do we do a lot of embedding? It has nice properties for sharing "base type" but less nice properties for code that consumes them. I don't know if our OpenAPI schema generation flattens embedded types properly (it seems so from a quick glance). If we think it's a good pattern, we might do well to use it more. I guess we can't refit existing code with embedding (breaks protobuf encoding) but we could use it more in the future (and in future API versions)
|
|
||
| For backward compatibility reasons, discriminators can be added to existing | ||
| union structures as an optional string. This has a nice property that it's going | ||
| to allow conflict detection when a union field is flipped. |
There was a problem hiding this comment.
What does "flipped" mean exactly?
| union structures as an optional string. This has a nice property that it's going | ||
| to allow conflict detection when a union field is flipped. | ||
|
|
||
| The value of the discriminator is going to be set automatically by the apiserver |
There was a problem hiding this comment.
I think you intend the value of the discriminator to exactly match the names of the discriminated fields. Please say that explicitly?
Is it an option to choose none of the fields, and if so, how is that represented in the discriminator?
|
I thought we, the kubernetes maintainers, were unionizing. |
| optional, omitempty and pointers. | ||
| - `// +discriminator` before a field means that this field is the discriminator | ||
| for the union. Only one field per structure can have this prefix. This field HAS | ||
| TO be a string, and CAN be optional. |
There was a problem hiding this comment.
If optional it must have a documented default behavior including "not set".
| TO be a string, and CAN be optional. | ||
|
|
||
| Note that this means that only one "union" type can be defined per structure, | ||
| which is a deliberate decision. |
There was a problem hiding this comment.
Do we do a lot of embedding? It has nice properties for sharing "base type" but less nice properties for code that consumes them. I don't know if our OpenAPI schema generation flattens embedded types properly (it seems so from a quick glance). If we think it's a good pattern, we might do well to use it more. I guess we can't refit existing code with embedding (breaks protobuf encoding) but we could use it more in the future (and in future API versions)
|
|
||
| ### Discriminator | ||
|
|
||
| For backward compatibility reasons, discriminators can be added to existing |
There was a problem hiding this comment.
Should we take a stronger stance? Existing one-of sets might not have discriminators, but in order to add a field to a one-of block in a skew-compatible it's really the only way that the laggier half of the skew can know "this block was set but to something I don't understand", right?
Especially for "exactly one of these must be set" blocks.
| We're not planning to use this KEP to release the feature, but mostly as a way | ||
| to document what we're doing. | ||
|
|
||
| ## Proposal |
There was a problem hiding this comment.
We have examples in our code of both "at most one of" and "exactly one of" blocks. Does that affect this proposal? Should we write strong rules in one case than the other?
There was a problem hiding this comment.
Yeah, this is not really replacing the validation logic, just trying to help clear the fields so that there is at most one. If there is 0, I expect the validation will catch that. I'll think about it a little bit more, it'd be nice if we had a way to formalize that.
There was a problem hiding this comment.
OK, I thought about it, I like the parallel with protobuf: this gives a semantic for "at most one of" and clears other fields. It's up to validation to decide if exactly-one of is accepted (which can be implemented using open-api)
| intention to clear all the other fields when its value is changed. See section | ||
| below. | ||
|
|
||
| ### Normalization process |
|
|
||
| ### Normalization process | ||
|
|
||
| The normalization process (which runs before validating that the union property |
There was a problem hiding this comment.
Is this going to be handled automatically as part of the REST strategy or something? E.g. are we generating "normalize" functions that get called before validation? It would be amazing if this was magical and Just Worked without individual API authors having to do anything.
There was a problem hiding this comment.
I'll clarify that but basically: since we now have access to the openapi in all the handler path (CREATE/UPDATE/PATCH/"Apply") to track fields manager, we can also automatically process these normalization before validation.
|
I've updated some things, I'm not exactly happy with everything yet. |
apelisse
force-pushed
the
unions
branch
2 times, most recently
from
655da41
to
55c0a68
Compare
| Since OpenAPI v2 has no support for oneOf, a couple of kubernetes specific | ||
| extensions will be added to OpenAPI: | ||
| - `x-kubernetes-oneof-discriminator: true` is set for the discriminator field, | ||
| - `x-kubernetes-oneof: "<discriminatedName>` is set for discriminated |
There was a problem hiding this comment.
for easier interpretation of our v3 output, shouldn't be add these extensions there as well?
At best we come up with some oneOf pattern in OpenAPI which is pretty hard to detect. There will be more things to check than just the existence of the oneOf operator. E.g. oneOf of to patterns is no union.
There was a problem hiding this comment.
Another point: I don't want to see that we turn a concrete oneOf pattern into some API convention that we cannot change ever for compatibilty reasons. It is much easier to leave the pattern open (of course it must comply with OpenAPI semantics, but nobody should depend on it to stay the same), and at the same time have these more abstract extensions which will stay forever and are part of the API server API contract.
There was a problem hiding this comment.
Last but not least, we have to offer these extensions in the CRD validation schemata. Translating some oneOf pattern into those extensions for v2 is unnecessary complex. Also we couple the openapi generator to this pattern (compare previous comment), although both apiserver and generator have different life cycles.
|
/assign @sttts |
|
|
||
| As for Go types, we only accept one union per structure, which means there can | ||
| be only one discriminator, and all other `oneof` fields are assumed to belong to | ||
| the same union. |
There was a problem hiding this comment.
what does this paragraph mean here for OpenAPI?
There was a problem hiding this comment.
yeah, one union per openapi object (allof flattening included). I'll clarify the doc
| - `x-kubernetes-union-discriminator: <discriminator>` is set to the name of the | ||
| discriminator field, if present, | ||
| - `x-kubernetes-oneof-fields-discriminated: {"<fieldName>": | ||
| "<discriminatedName>"}` is a map of fields that belong to the union to their |
There was a problem hiding this comment.
if we restrict us to one union per object, let's simplify this tag to a simple list of field names.
Also I would suggest a simpler tag name: x-kubernetes-union-fields
There was a problem hiding this comment.
it's not a list on purpose, because we really want to be able to have a discriminated name that is not explicitely the name of the field. For example today, we have an implicit discriminator in DeploymentStrategyType whose value must be RolloutStrategy (matching the field name rolloutStrategy). For volumes, it's not as simple as capitalizing, e.g. nfs -> NFS. Keeping this mapping of field name to discriminated name is useful.
There was a problem hiding this comment.
How about instead of "discriminated", we call this "union member", where the discriminator field value is the name of a union member?
Then could we have x-kubernetes-union-member-fields: { "<fieldName>": "<unionMemberName>"} ? Or, if I'm following @sttts, maybe we could have { "<unionMemberName>": "<listOfFieldNames>"}
| We're proposing a new type of tags for go types (in-tree types, and also | ||
| kubebuilder types): | ||
|
|
||
| - `// +union` before a field means that it is part of the union. Multiple fields |
There was a problem hiding this comment.
I think we should put this on types:
// +union
type MetricSource struct {
// +discriminant
Type MetricSourceType
Object *ObjectMetricSource
Pods *PodsMetricSource
...
}We should be thinking of unions like enums in Rust, algebraic datatypes in Haskell, discriminated unions in typescript, etc: as a type-level construct with multiple variants and a discriminator, at both the IDL-ish (Go) level and at an API level. This is conceptually similar to what people are familiar with, and makes writing things out easier.
This should hold for most cases in Kubernetes. The few where it doesn't we can either a) special-case, b) fix before they go alpha (cough webhookconfig cough), or c) fix at a type level and leave the API the same by making an external struct and embedding.
There was a problem hiding this comment.
I also prefer a formulation like this one, I think. Embedding can potentially be used to transition existing places where union and non-union fields are mixed.
There was a problem hiding this comment.
What about something in the middle: +union-fields=Object,Pods on types.
There was a problem hiding this comment.
That seems more error-prone and tedious and hard to read? :-/
I'm going with both: you can either specify tags on members individually or entire structs. They'll generate the same OpenAPI, and we'll only document the struct way, which is the one we want people to use (and also is probably going to be the only one available through kubebuilder)
There was a problem hiding this comment.
'm going with both:
sounds good.
| ### Discriminator | ||
|
|
||
| For backward compatibility reasons, discriminators can be added to existing | ||
| union structures as an optional string. This has a nice property that it's going |
There was a problem hiding this comment.
If this value is defaulted, it does not have to be optional anymore.
There was a problem hiding this comment.
which remind me that client side validation does not see defaults yet and hence falls over a non-optional discriminator. I guess that's what you have in mind.
There was a problem hiding this comment.
Right, if we make it required, then client-side validation will force people to set it, which we don't want.
There was a problem hiding this comment.
On one hand, it should be OK to have clients be aware of defaulting rules (they are part of the API), so clients could make a copy, apply defaults, and validate that.
On the other hand, that fails in case of admission controllers. But if that is a requirement, then client-side validation can never work.
I know this is a little controversial, but I think it's OK to client-side default (in a temporary copy!!) to validate.
There was a problem hiding this comment.
we have dry-run, don't we?
Sure
There was a problem hiding this comment.
We could also just stop validating required fields client-side.
There was a problem hiding this comment.
I don't see why this is important anyway, you can have a look at the behavior here (which I think is a big improvement): https://github.com/kubernetes-sigs/structured-merge-diff/blob/master/merge/union_test.go
| Objects have to be validated AFTER the normalization process, which is going to | ||
| leave multiple fields of the union if it can't normalize. As discussed in | ||
| drawbacks below, it can also be useful to validate apply requests before | ||
| applying them. |
There was a problem hiding this comment.
would normalization be part of the defaulter?
There was a problem hiding this comment.
No, for server-side apply it happens BEFORE we default, for all other modifying end-points, it happens after defaulting. It's part of the "field management"
There was a problem hiding this comment.
ok, so the plumbing is mostly in place. good.
|
|
||
| #### "At most one" versus "exactly one" | ||
|
|
||
| The goal of this proposal is not to change the validation, but to help client |
There was a problem hiding this comment.
... help clients to clear ...
|
Lgtm. What I miss: will we add discriminators and tags to all existing unions in k/k? What is the roadmap for this? |
I'm currently working at this, we'll add discriminator as optional before the release. |
|
|
||
| - `// +union` before a field means that it is part of the union. Multiple fields | ||
| can have this prefix. These fields MUST BE optional, omitempty and pointers. | ||
| - `// +discriminator` before a field means that this field is the discriminator |
There was a problem hiding this comment.
I appreciate having "union" in the metadata name
| ### Discriminator | ||
|
|
||
| For backward compatibility reasons, discriminators can be added to existing | ||
| union structures as an optional string. This has a nice property that it's going |
There was a problem hiding this comment.
On one hand, it should be OK to have clients be aware of defaulting rules (they are part of the API), so clients could make a copy, apply defaults, and validate that.
On the other hand, that fails in case of admission controllers. But if that is a requirement, then client-side validation can never work.
I know this is a little controversial, but I think it's OK to client-side default (in a temporary copy!!) to validate.
|
Updated, PTAL |
| people to embed their unions directly in structures, and only exist because of | ||
| some existing core types (e.g. `Value` and `ValueFrom` in | ||
| [EnvVar](https://github.com/kubernetes/kubernetes/blob/3ebb8ddd8a21b/staging/src/k8s.io/api/core/v1/types.go#L1817-L1836)). | ||
| - `// +union-discriminator` before a field means that this field is the |
There was a problem hiding this comment.
nit: why the variance in naming? unionDeprecated and union-discriminator ?
k8s-ci-robot
added
the
lgtm
k8s-ci-robot
removed
the
lgtm
|
Updated reviewer's list and squashed. |
|
Nice to see our existing behavior documented. I've fielded questions about this in the past. /approve |
k8s-ci-robot
added
the
approved
|
reapplying @thockin 's lgtm /lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: apelisse, deads2k, thockin The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Not meant to be a feature, mostly to document what we're doing.