KEP-1027: API Unions for 1.25 #3377

kevindelgado · 2022-06-09T21:22:56Z

One-line PR description: Update KEP for API Unions.

Issue link: API Unions #1027

Other comments:

apelisse

I think that's all my comments

apelisse · 2022-06-09T22:17:28Z

keps/sig-api-machinery/1027-api-unions/README.md

@@ -87,12 +125,14 @@ become possible.
 ### Goals

 The goal is to enable a union or "oneof" semantics in Kubernetes types, both for
-in-tree types and for CRDs.
+in-tree types and for CRDs. Validation of these unions should be centralized


It's also doing normalization, not just validation.

apelisse · 2022-06-09T22:18:18Z

keps/sig-api-machinery/1027-api-unions/README.md

+
+#### Story 1
+
+As a CRD owner, I should be able to author my CRD such that the openapi schema


I'd split into these two stories:

CRD Authors don't have to write the validation

Type users can commit their intent more easily, especially when a new field they don't know has been introduced.

keps/sig-api-machinery/1027-api-unions/README.md

apelisse · 2022-06-09T22:19:22Z

keps/sig-api-machinery/1027-api-unions/README.md

-
-Multiple unions can exist per structure, but unions can't span across multiple
-go structures (all the fields that are part of a union has to be together in the
+  discriminator for the union. This field MUST be a string. Can be optional or


Optional discriminator is confusing, I'd always make it required, though you can allow empty discriminator to mean unset.

done. We need a way to for the author to indicate that empty discriminator is allowed vs not allowed. I've proposed a new unionAllowEmpty tag. Let me know what you think

apelisse · 2022-06-09T22:19:35Z

keps/sig-api-machinery/1027-api-unions/README.md

@@ -195,6 +292,7 @@ Conversion between OpenAPI v2 and OpenAPI v3 will preserve these fields.

 ### Discriminator

+// TODO: ask Antoine
 For backward compatibility reasons, discriminators should be added to existing


Most of these paragraphs aren't accurate anymore.

apelisse · 2022-06-09T22:20:20Z

keps/sig-api-machinery/1027-api-unions/README.md

+We propose supporting both "at most one" and "exactly one" semantics. 
+
+To distinguish between the two, API authors should tag the discriminator as
+"optional" or "required" based on the desired behavior. We the discriminator is


As mentioned above, required while allowing empty string (and no omitempty)

This whole section is confusing. I think it makes more sense just left in the tags section above

apelisse · 2022-06-09T22:21:07Z

keps/sig-api-machinery/1027-api-unions/README.md

+This means two things:
+1. when the server receives a request with a discriminator set to a
+given field, it should clear out any other fields that are set.
+2. when the server receives a request with a discriminator set to a given field,


Really not convinced about this, this specific feature requires doing the update while looking at the old object. Also, dropping fields is part of more general problem with structure deserializing, why would we fix this just for unions?

Yea, I think I agree, and this was my original thoughts when discussing with Daniel. Leaving it in for now though to let @lavalamp take a look and see if I accurately described his views on it.

So I think the current consensus is we want to error loudly here. Is this right @deads2k?

All of our update code gets access to the old object (including webhooks!), I'm not sure why that has become a consideration, I don't think it is.

If there's no risk of being wrong, I think it's logical to repair the requests. If there is some risk, I think it's probably best to fail loudly.

Sounds good, will leave it as is, but leave the question in the "open questions section for discussion"

Removed this based on sig meeting discussion and decision to error loudly. Testing matrix reflects the desired behavior

apelisse · 2022-06-09T22:21:28Z

keps/sig-api-machinery/1027-api-unions/README.md


+For union types without a discriminator (only existing unions), normalization is a little more


Drop these two paragraphs for now, I think we should focus on discriminated unions for now.

lavalamp · 2022-06-13T23:40:12Z

keps/sig-api-machinery/1027-api-unions/README.md

+
+* Validation - ensuring only one member field is set (or at most one if
+  desired).
+* Normalization - ensuring the API server can modify the fields of the oneOf to


Is this the goal? Or are we merely trying to detect and fail requests sent by clients who have misunderstood?

There's no way for unstructured clients to get the object right without clearing the fields with version skew though.

and the question is, do we force users to update such clients by failing their requests, or do we want to try and repair the requests.

Right, in the former case, it means that adding a branch to a union is a backwards incompatible change, which I think is a little unfortunate (how do we even update Kubernetes if we start failing clients? We basically can't, so adding a branch to a union will be forbidden in practice).

added a line about this to the "open questions" section.

Is this the goal? Or are we merely trying to detect and fail requests sent by clients who have misunderstood?

I'm also unsure that this is a goal versus detection and good failure messages.

There's no way for unstructured clients to get the object right without clearing the fields with version skew though.

An unstructured client has access to all the fields in the union, right? Is it a question of knowledge?

I think an explanation that include if and how server-side-apply should handle unions may help sway me one way or the other. I'll keep reading below to see how that is addressed.

I narrowed this description down to just talking about normalizing for clients unaware of fields due to version skew.

keps/sig-api-machinery/1027-api-unions/README.md

lavalamp · 2022-06-13T23:50:03Z

keps/sig-api-machinery/1027-api-unions/README.md

+
+#### Story 1
+
+As a CRD owner, I can use simple semantics (such as go tags), to express the


openapi tags?

Or go markers, go tags means something else.

lavalamp · 2022-06-13T23:51:39Z

keps/sig-api-machinery/1027-api-unions/README.md

+We present a [guide doc](https://docs.google.com/document/d/1Wruosjo0ELLl1yxauzpsUjgH2fK9KdgXDmOdJ5sG7Kg/edit?resourcekey=0-8Pwzx6EvsFR7VQoXzCTY4Q) on how to interpret the test matrix, but the major
+conclusions are as follows (along with the test case number from the test matrix):
+
+* (Case #22 and #27) If an unstructured client is unaware of field on the union, but wants to clear


define unstructured first?

a little wordy, but done.

lavalamp · 2022-06-13T23:51:59Z

keps/sig-api-machinery/1027-api-unions/README.md

+  the union entirely (assuming the union is optional), it will have no way of doing
+  so without a discriminator. With a discriminator, the client can express its
+  intention by setting the discriminator to the empty value and the server can
+  respects it intentions and clear any fields the client is unaware of.


Suggested change

respects it intentions and clear any fields the client is unaware of.

respect its intentions and clear any fields the client is unaware of.

lavalamp · 2022-06-13T23:52:41Z

keps/sig-api-machinery/1027-api-unions/README.md

+  just wants to echo back the union it received in a get request (such as when
+  updating other parts of the object), a client without a discriminator will
+  silently drop the currently set field, while a client with the discriminator
+  will not change the discriminator value, indicating to the client that no


Suggested change

will not change the discriminator value, indicating to the client that no

will not change the discriminator value, indicating to the server that no

lavalamp · 2022-06-13T23:54:10Z

keps/sig-api-machinery/1027-api-unions/README.md

+- `// +unionDiscriminator` before a field means that this field is the
+  discriminator for the union. This field MUST be a string. This field MUST be
+  required.
+- `// +unionAllowEmpty` before a discriminator field means that by setting the


The wording here is confusing, I'd go with optional/required as the metaphor personally.

Also, I don't think that's needed, we'll always want validation on the discriminator, just allow your discriminator to be empty?

Ya I think I like that Antoine.

It does go against the "we don't want to have to write validation on a per resource" principal, but this is different than union validation so it seems okay? Thoughts @lavalamp?

Changed to unionOptional for now pending deletion.

Yes maybe marking the discriminator as optional ("" is never valid, but you can omit it?) may have the same effect as treating the union as optional?

lavalamp · 2022-06-13T23:55:02Z

keps/sig-api-machinery/1027-api-unions/README.md

+  discriminator, exactly one of the union members must be set in order to be
+  valid.
+- `// +unionMember=<discriminatorName>` before a field means that this
+  field is a member of a union. The `<discriminatorName>` is optional if there


it's also not optional if you have backwards compatibility concerns, I'd consider requiring it personally

Note: I misunderstood what "discriminatorName" meant here. You need to add a way to state what you set the discriminator to in order to select this field.

I was thinking we would use the "json" name of the field for what you need to set the discriminator. If so, I can just document it better here.

I'm not sure I understand the value of letting users customize this name, but I've added the ability to do so and optionally fall back to the json representation of the field name otherwise.

johnbelamaric · 2022-06-23T17:05:35Z

keps/prod-readiness/sig-api-machinery/1027.yaml

@@ -0,0 +1,3 @@
+kep-number: 1027
+alpha:
+        approver: "@deads2k"


this should be me

keps/sig-api-machinery/1027-api-unions/README.md

kevindelgado

Updated the openapi given our discussions offline

keps/sig-api-machinery/1027-api-unions/README.md

kevindelgado · 2022-06-23T20:31:32Z

All feedback addressed. PTAL @liggitt @apelisse

liggitt · 2022-06-23T20:26:03Z

keps/sig-api-machinery/1027-api-unions/README.md

+  // MemberField may be nil in the case of empty union members where a valid
+  // discriminator value has no corresponding member field.


I'm not sure null map values are expressible with all API requests (I think they get interpreted as "drop this key" in some patch types), so this may need to be an empty value (like {}) rather than null, but I'm ok figuring that out during implementation/API review

keps/sig-api-machinery/1027-api-unions/README.md

liggitt · 2022-06-23T20:33:36Z

resolved my comments which were addressed. this lgtm

kevindelgado · 2022-06-23T20:52:19Z

PTAL @lavalamp

lavalamp · 2022-06-23T21:13:56Z

/lgtm
/approve

(I am not rereading this due to too many other things going on but it was 90% there at the sig meeting and if Jordan is happy I'm going to assume we got all the open questions answered -- as always we reserve the right to fix bugs later ;) )

johnbelamaric · 2022-06-23T21:59:12Z

/approve

k8s-ci-robot · 2022-06-23T21:59:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johnbelamaric, kevindelgado, lavalamp

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:

~~keps/prod-readiness/OWNERS~~ [johnbelamaric]
~~keps/sig-api-machinery/OWNERS~~ [johnbelamaric,lavalamp]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

kevindelgado added 2 commits June 9, 2022 18:00

update kep.yaml

68a3592

update kep readme to match current template

cd96e98

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 9, 2022

k8s-ci-robot requested a review from apelisse June 9, 2022 21:23

k8s-ci-robot added the kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory label Jun 9, 2022

k8s-ci-robot requested a review from fedebongio June 9, 2022 21:23

k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Jun 9, 2022

Update unions KEP for v1.25

5100aa8

kevindelgado force-pushed the api-unions-try-2 branch from 04f9721 to 5100aa8 Compare June 9, 2022 21:26

apelisse reviewed Jun 9, 2022

View reviewed changes

kevindelgado added 2 commits June 10, 2022 08:34

address apelisse feedback

d882c04

Add test case summary and open questions

e31cd19

kevindelgado force-pushed the api-unions-try-2 branch from 1121b5f to e31cd19 Compare June 13, 2022 15:33

kevindelgado changed the title ~~WIP: API Unions for 1.25~~ API Unions for 1.25 Jun 13, 2022

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 13, 2022

kevindelgado requested a review from apelisse June 13, 2022 21:42

kevindelgado force-pushed the api-unions-try-2 branch from 58d88d8 to 0ac9cba Compare June 13, 2022 21:48

update kep

85c40c0

kevindelgado force-pushed the api-unions-try-2 branch from 0ac9cba to 85c40c0 Compare June 13, 2022 21:51

lavalamp reviewed Jun 13, 2022

View reviewed changes

keps/sig-api-machinery/1027-api-unions/README.md Show resolved Hide resolved

lavalamp reviewed Jun 13, 2022

View reviewed changes

keps/sig-api-machinery/1027-api-unions/README.md Show resolved Hide resolved

lavalamp reviewed Jun 13, 2022

View reviewed changes

kevindelgado requested review from liggitt and apelisse June 22, 2022 23:16

sketch open api

a67ef11

kevindelgado force-pushed the api-unions-try-2 branch from 8f40344 to a67ef11 Compare June 22, 2022 23:25

kevindelgado added 2 commits June 23, 2022 01:26

remove empty members from open api definition

182167d

Add open api deserialization struct

9bcae5a

johnbelamaric reviewed Jun 23, 2022

View reviewed changes

liggitt reviewed Jun 23, 2022

View reviewed changes

keps/sig-api-machinery/1027-api-unions/README.md Outdated Show resolved Hide resolved

liggitt reviewed Jun 23, 2022

View reviewed changes

keps/sig-api-machinery/1027-api-unions/README.md Outdated Show resolved Hide resolved

liggitt reviewed Jun 23, 2022

View reviewed changes

keps/sig-api-machinery/1027-api-unions/README.md Outdated Show resolved Hide resolved

liggitt reviewed Jun 23, 2022

View reviewed changes

keps/sig-api-machinery/1027-api-unions/README.md Outdated Show resolved Hide resolved

OpenAPI extension feedback

5e85c60

kevindelgado commented Jun 23, 2022

View reviewed changes

keps/sig-api-machinery/1027-api-unions/README.md Show resolved Hide resolved

remove multiple unions per object possibility

7da001c

liggitt reviewed Jun 23, 2022

View reviewed changes

kevindelgado requested review from liggitt and johnbelamaric June 23, 2022 20:31

k8s-ci-robot assigned lavalamp Jun 23, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 23, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 23, 2022

k8s-ci-robot merged commit f062c19 into kubernetes:master Jun 23, 2022

k8s-ci-robot added this to the v1.25 milestone Jun 23, 2022


		#### Story 1

		As a CRD owner, I should be able to author my CRD such that the openapi schema


		For union types without a discriminator (only existing unions), normalization is a little more


		#### Story 1

		As a CRD owner, I can use simple semantics (such as go tags), to express the

	respects it intentions and clear any fields the client is unaware of.
	respect its intentions and clear any fields the client is unaware of.

	will not change the discriminator value, indicating to the client that no
	will not change the discriminator value, indicating to the server that no

		// MemberField may be nil in the case of empty union members where a valid
		// discriminator value has no corresponding member field.

KEP-1027: API Unions for 1.25 #3377

KEP-1027: API Unions for 1.25 #3377

Conversation

kevindelgado commented Jun 9, 2022 • edited

apelisse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevindelgado left a comment

Choose a reason for hiding this comment

kevindelgado commented Jun 23, 2022

liggitt Jun 23, 2022 • edited

Choose a reason for hiding this comment

liggitt commented Jun 23, 2022

kevindelgado commented Jun 23, 2022

lavalamp commented Jun 23, 2022

johnbelamaric commented Jun 23, 2022

k8s-ci-robot commented Jun 23, 2022

kevindelgado commented Jun 9, 2022 •

edited

liggitt Jun 23, 2022 •

edited