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

Gateway API v0.5.0 API Review #1086

Closed

Conversation

robscott
Copy link
Member

@robscott robscott commented Apr 4, 2022

What type of PR is this?
/kind api-change

What this PR does / why we need it:
This PR represents the diff between v0.4.x and a proposed v0.5.0. This was accomplished by copying the apis/v1alpha2 from master to a branch based on the release-0.4 branch.

Current status

We believe GatewayClass, Gateway, and HTTPRoute have met the criteria we defined earlier for graduation to beta (details). The following work is still in progress:

Changes from v0.4

The proposed API is largely identical to what we released in v1alpha2, with the following exceptions:

  1. We want to graduate GatewayClass, Gateway, and HTTPRoute to v1beta1 (More Details)
  2. We have introduced a new "release channels" concept. The new "experimental" release channel allows us to add new features to Gateway API in a similar way to how we use alpha feature gates in upstream to guard new fields initially. (More Details)

New experimental features

There are 3 new experimental features included in this change:

  • Path redirects and rewrites GEP 726
  • Address matching on TCPRoute and UDPRoute GEP 735
  • Port matching in ParentRef GEP 957

Future port changes

As a follow up to the previous review, there has been a lot of discussion around Ports, and potential for port ranges, all ports, and auto assignment:

At this point, we believe that all of the above can be added in the future with backwards compatible changes/additions. Note: this is not saying we should or will make these additions, just that they will still be possible after graduating to beta.

Goals of this review

Since we're an official Kubernetes API, we need formal approval for this change from SIG-Network API Reviewers, assigned below. Of course feedback from others is also welcome here. Our goal is to complete this process by mid to late April.

Note: The aim is not to merge this PR, just to provide a way to review what has changed between v0.4 and v0.5.

/assign @danwinship @khenidak @thockin

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 4, 2022
@k8s-ci-robot k8s-ci-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Apr 4, 2022
@k8s-ci-robot k8s-ci-robot added 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 Apr 4, 2022
@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 requested review from hbagdi and thockin April 4, 2022 21:34
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 4, 2022
apis/v1alpha2/httproute_types.go Outdated Show resolved Hide resolved
apis/v1alpha2/shared_types.go Outdated Show resolved Hide resolved
apis/v1alpha2/shared_types.go Outdated Show resolved Hide resolved
apis/v1alpha2/shared_types.go Outdated Show resolved Hide resolved
//
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=253
Value string `json:"value"`
Copy link

Choose a reason for hiding this comment

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

IPv4, IPv6, hostname are unambiguously parseable (with some effort) - is this justified?

If we want to keep this, should it follow the same 1-field-per enum value pattern?

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 it's at least possible that we'd add additional types in the future that may not be as easy to differentiate between. Even with named address, it's possible that in some systems, a name is just a number, or a name is a valid hostname, or something else. I could get on board with the 1-field-per enum pattern here though.

@danwinship
Copy link

The proposed API is largely identical to what we released in v1alpha2, with the following exceptions:

  1. We want to graduate GatewayClass, Gateway, and HTTPRoute to v1beta1 (More Details)

I see no references to "v1beta1" here... I had assumed when I read this that you'd be duplicating the "graduating" parts of apis/v1alpha2/ into a new apis/v1beta1/, so that people using only beta APIs could do

apiVersion: gateway.networking.k8s.io/v1beta1
kind: Gateway
...

@youngnick
Copy link
Contributor

@danwinship I think that's the plan for actually cutting the release, but I think that @robscott felt this review would be easier if you didn't need to review files that were just all added lines (since a v1beta1 would be net-new and not produce a useful diff.)

The plan is absolutely to take the three resources mentioned and copy them into a v1beta1 version however.

@robscott
Copy link
Member Author

@danwinship Good point, I can make a separate PR with /apis/v1beta1, I was just trying to make it as easy as possible to see what was actually changing. The new v1beta1 types will be identical to the v1alpha2 types shown here, so I'd hoped just seeing a diff covering the current state with what you'd currently reviewed would be more manageable. Let me know if adding /apis/v1beta1 at this point would be helpful.

@youngnick
Copy link
Contributor

Looks like this got closed in error?

@danwinship
Copy link

/unassign
(I skimmed over it, but I haven't used this API at all, or thought about it since the last review, so I don't know the context of most of these changes, etc. None of it seems obviously wrong...)

@candita
Copy link
Contributor

candita commented May 11, 2022

@robscott looks good to me, as someone new to the review. I just had a few nits and minor comments.

@robscott robscott force-pushed the v0.5.0-api-review branch from 73a77fa to aa2c2c3 Compare May 12, 2022 20:20
@robscott robscott force-pushed the v0.5.0-api-review branch from 00d0f36 to 1fa24e4 Compare June 9, 2022 18:54
Copy link

@thockin thockin left a comment

Choose a reason for hiding this comment

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

Discussed in real-time - couple small followups Rob noted. LGTM. Thanks.

@youngnick
Copy link
Contributor

So, I think that once we resolve #1200, #1201, and #1202, we can close this out as complete?

@thockin
Copy link

thockin commented Jun 10, 2022 via email

@evankanderson
Copy link
Contributor

It looks like the three mentioned issues were closed out.

Should this stop being a draft PR?

@youngnick
Copy link
Contributor

Yes, I think we may just close this out - this is really a placeholder to give the API reviewers an easier time. Unless there's something else @robscott wants to do here?

@robscott
Copy link
Member Author

Yep, I think this was good to go. All the changes reviewed here were released as part of v0.5.0-rc1.

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. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API 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.

10 participants