Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KEP-1027: API Unions for 1.25 #3377

Merged
merged 22 commits into from
Jun 23, 2022

Conversation

kevindelgado
Copy link
Contributor

@kevindelgado kevindelgado commented Jun 9, 2022

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

@k8s-ci-robot 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 k8s-ci-robot added the kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory label Jun 9, 2022
@k8s-ci-robot k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Jun 9, 2022
Copy link
Member

@apelisse apelisse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's all my comments

@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's also doing normalization, not just validation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


#### Story 1

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd split into these two stories:

  1. CRD Authors don't have to write the validation
  2. Type users can commit their intent more easily, especially when a new field they don't know has been introduced.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

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

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of these paragraphs aren't accurate anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@kevindelgado kevindelgado changed the title WIP: API Unions for 1.25 API Unions for 1.25 Jun 13, 2022
@k8s-ci-robot 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

* 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


#### Story 1

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

openapi tags?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or go markers, go tags means something else.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

define unstructured first?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a little wordy, but done.

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

- `// +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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be me

Copy link
Contributor Author

@kevindelgado kevindelgado left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the openapi given our discussions offline

@kevindelgado
Copy link
Contributor Author

All feedback addressed. PTAL @liggitt @apelisse

Comment on lines +491 to +492
// MemberField may be nil in the case of empty union members where a valid
// discriminator value has no corresponding member field.
Copy link
Member

@liggitt liggitt Jun 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@liggitt
Copy link
Member

liggitt commented Jun 23, 2022

resolved my comments which were addressed. this lgtm

@kevindelgado
Copy link
Contributor Author

PTAL @lavalamp

@lavalamp
Copy link
Member

/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 ;) )

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

/approve

@k8s-ci-robot
Copy link
Contributor

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

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

@k8s-ci-robot 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 k8s-ci-robot merged commit f062c19 into kubernetes:master Jun 23, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.25 milestone Jun 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants