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

Adding GEP 922: Gateway API Versioning Plan #923

Merged
merged 1 commit into from Nov 15, 2021

Conversation

robscott
Copy link
Member

What type of PR is this?
/kind gep

What this PR does / why we need it:
This proposes a set of versioning guidelines for future Gateway API releases. This is a follow up to the related doc that has been discussed in gateway-api and sig-apimachinery meetings.

Which issue(s) this PR fixes:
Related to #922, will not be fixed until this GEP moves to "Implemented"

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot k8s-ci-robot added kind/gep PRs related to Gateway Enhancement Proposal(GEP) cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 29, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: robscott

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 Oct 29, 2021
Copy link
Contributor

@youngnick youngnick left a comment

Choose a reason for hiding this comment

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

This is looking pretty good overall to me, but there's a couple of things that could be clearer.

site-src/geps/gep-922.md Show resolved Hide resolved
site-src/geps/gep-922.md Show resolved Hide resolved
@robscott robscott changed the title Adding GEP 922: Adding Gateway API Versioning Adding GEP 922: Gateway API Versioning Plan Nov 2, 2021
site-src/geps/gep-922.md Outdated Show resolved Hide resolved
site-src/geps/gep-922.md Outdated Show resolved Hide resolved
site-src/geps/gep-922.md Outdated Show resolved Hide resolved
site-src/geps/gep-922.md Show resolved Hide resolved
site-src/geps/gep-922.md Outdated Show resolved Hide resolved
site-src/geps/gep-922.md Outdated Show resolved Hide resolved
site-src/geps/gep-922.md Show resolved Hide resolved
site-src/geps/gep-922.md Outdated Show resolved Hide resolved
```
gateway.networking.k8s.io/bundle-version: v0.4.0
gateway.networking.k8s.io/channel: stable|experimental
```
Copy link
Contributor

Choose a reason for hiding this comment

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

So, at the end of the day, a cluster can either have a stable or an experimental CRD. It is exclusive because you can only have one CRD at a time for a GK.

Why have the distinction in that case?
What if we just ship the experimental fields as part of the stable channel?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we do that, then the only way for YAML consumers (and let's be real, most people will use YAML, not read the full spec) to know what fields are experimental is for us to prefix them somehow - that is, we'd have to call all the experimental fields experimentalFieldName or something.

Other things I can think of that only supplying on CRD spec would imply:

  • Implementations would need a switch of some sort to turn on experimental field processing - effectively, we're moving the feature gate to the implementing controller, and trusting them to figure it out.
  • We'd also need to supply a method for implementations to determine what the experimental fields are, so that they can tell people they're using an experimental field but haven't enabled it.

Having two separate CRDs solves all of those problems - the implementation can switch experimental field processing on or off based on the annotation on the CRD object itself (this also means you can't enable experimental field processing without having modify CRDs access, which I think is fine).

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree with @youngnick's points above. I think it's important to have users explicitly opt in to experimental functionality, and I think separate CRDs are likely the best mechanism we have for this. Although I'd really like something better like kubectl support/gating for alpha fields, I don't think that's going to be practical anytime soon.

site-src/geps/gep-922.md Outdated Show resolved Hide resolved
site-src/geps/gep-922.md Outdated Show resolved Hide resolved
* Making required fields optional
* Removal of experimental fields
* Removal of experimental resources
* Graduation of fields or resources from experimental to stable track
Copy link
Contributor

Choose a reason for hiding this comment

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

I totally see the need for doing this within the same API version but I also think it is our responsibility to help out implementers here. Is there any way a controller can check the version bundle version in the cluster?
Like an annotation on the CRD (I'm not sure if those annotations are readable programmatically from a controller or not).

Since Gateways are very sensitive to security, most operators will prefer a hard failure compared to an implementation having buggy behavior (or a vulnerable behavior). We don't need to instruct what controller authors should do when the bungle version is higher than the controller is programmed against.

Can we make this part of this GEP?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point - we do have annotations proposed further down that would enable this. Annotations should be able to be read programmatically, as long as the controller requests read access on CRDs which seems reasonable and likely necessary.

site-src/geps/gep-922.md Show resolved Hide resolved
site-src/geps/gep-922.md Outdated Show resolved Hide resolved
Note that each new bundle version, no matter how small, may include updated
CRDs, webhook, or both.

### Implementers:
Copy link
Contributor

Choose a reason for hiding this comment

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

What if an implementation doesn't want to work in a cluster unless the installed version matches a support matrix that the it is programmed against? Is that invalid behavior?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that's valid, but likely unnecessarily constrained. If all the guidelines here are followed, newer additions to the API would simply not be supported by an older implementation of the API. A warning or set of warnings is likely preferable to stopping altogether.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay. Can we explicitly mention that controllers can rely on the annotation to decide their behavior?

site-src/geps/gep-922.md Outdated Show resolved Hide resolved
Copy link
Contributor

@youngnick youngnick left a comment

Choose a reason for hiding this comment

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

This lgtm with a few non-blocking nits.

the following:

* API Types/CRDs
* Validating Webhook
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we mention other validation code here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we should definitely add that to our bundle versions if/when we have it. I have a note in #926 to come back to this KEP and update it if/when we add that logic.

responsibilities to ensure we're providing a consistent experience.

### API Authors:
* MUST provide conversion between stable API versions, starting with v1alpha2.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* MUST provide conversion between stable API versions, starting with v1alpha2.
* MUST provide conversion between API versions, starting with v1alpha2.

Feels weird to call an alpha API "stable".

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, the terminology here is confusing. I've updated this to "excluding experimental fields", which I think is a clearer representation of what I was aiming for here.

crashing
* MUST NOT rely on webhook or CRD validation as a security mechanism. If field
values need to be escaped to secure an implementation, both webhook and CRD
validation can be bypassed and cannot be relied on. Instead, implementations
Copy link
Contributor

Choose a reason for hiding this comment

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

This wording is fine for now, but if we do implement the validation module, feels like it should be a MUST to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, I added a note to #926 so we remember to come back to this GEP and update it.

@youngnick
Copy link
Contributor

/lgtm
/hold for multiple lgtm though.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 10, 2021
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 10, 2021
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 11, 2021
@hbagdi
Copy link
Contributor

hbagdi commented Nov 11, 2021

Here is to hoping we have thought this through.
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 11, 2021
@robscott
Copy link
Member Author

I think there's broad enough consensus on this to merge it in, we can revisit through smaller tweaks if needed.

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 15, 2021
@k8s-ci-robot k8s-ci-robot merged commit 01ecd5a into kubernetes-sigs:master Nov 15, 2021
@robscott robscott deleted the gep-922 branch January 8, 2022 01:05
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/gep PRs related to Gateway Enhancement Proposal(GEP) lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants