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.

Sign up for GitHub

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

Jump to bottom

implement GEP-820: remove extension points from route match types #829

Merged
merged 1 commit into from
Aug 31, 2021
Merged

implement GEP-820: remove extension points from route match types #829

merged 1 commit into from
Aug 31, 2021

Conversation

hbagdi
Copy link
Contributor

@hbagdi hbagdi commented Aug 25, 2021

What type of PR is this?

/kind api-change
What this PR does / why we need it:
Implements GEP #820 Drop extension points from Route matches
Which issue(s) this PR fixes:

Fixes #820

Does this PR introduce a user-facing change?:

Extension points within match blocks from all Routes have been removed

GEP isn't merged (#822).
/hold

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API 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 Aug 25, 2021
@k8s-ci-robot k8s-ci-robot requested review from bowei and robscott August 25, 2021 21:18
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 25, 2021
@robscott robscott mentioned this pull request Aug 25, 2021
Closed
@hbagdi hbagdi force-pushed the feat/drop-extension-points branch from 59bc62c to cf8564d Compare August 26, 2021 22:33
@hbagdi
Copy link
Contributor Author

hbagdi commented Aug 26, 2021

GEP merged.
/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 Aug 26, 2021
jpeach
jpeach approved these changes Aug 26, 2021
View reviewed changes
Copy link
Contributor

@jpeach jpeach left a comment

Choose a reason for hiding this comment

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

This looks good overall, as per the GEP. Preserving the match field but disallowing its use seems a bit strange, but I can see why you went there.

Maybe we should take an issue to remove the match field for the release?

/lgtm
/hold

@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 Aug 26, 2021
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 26, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hbagdi, jpeach

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

robscott
robscott reviewed Aug 27, 2021
View reviewed changes
apis/v1alpha2/tcproute_types.go Outdated
//
// +optional
// +kubebuilder:validation:MaxItems=8
// +kubebuilder:validation:MaxItems=0
Copy link
Member

Choose a reason for hiding this comment

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

I think it may be better to just remove the matches field entirely until we add content to them. I think that would actually make our L4 routes easier to understand.

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 conflicted. Erroring towards conservative side.
This may confuse users who try to create types by copying the art here. Let's take that risk.

@hbagdi hbagdi force-pushed the feat/drop-extension-points branch from cf8564d to 4b6943e Compare August 27, 2021 20:43
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 27, 2021
@hbagdi hbagdi force-pushed the feat/drop-extension-points branch from 4b6943e to e3450f7 Compare August 27, 2021 20:56
@youngnick
Copy link
Contributor

/lgtm

I think that this is a good change, and that removing the match functionality makes TCPRoute and UDPRoute easier to explain, at least initially.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 31, 2021
@hbagdi hbagdi requested review from robscott and jpeach August 31, 2021 18:31
@robscott
Copy link
Member

Thanks!

/lgtm

@robscott
Copy link
Member

Based on our discussion at the community meeting yesterday, I think we have sufficiently broad consensus to remove the hold on this.

/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 Aug 31, 2021
@k8s-ci-robot k8s-ci-robot merged commit 9e43c43 into kubernetes-sigs:master Aug 31, 2021
markmc added a commit to markmc/gateway-api that referenced this pull request Feb 28, 2022
@markmc
tcp guide: remove section on metadata based tcp matches
7d0763c 
In commit c7bb1d6 (kubernetes-sigs#808), we documented how the tcp route match
extension point can be used for metadata based tcp matching.

Soon after, in commit e3450f7 (kubernetes-sigs#829), this extension point was
removed, but the docs remained.

Remove this section to reflect the current state of the API.
@markmc markmc mentioned this pull request Feb 28, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bowei bowei Awaiting requested review from bowei

@robscott robscott Awaiting requested review from robscott

@jpeach jpeach Awaiting requested review from jpeach

Assignees

@jpeach jpeach

@robscott robscott

@youngnick youngnick

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. breaking-change cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

GEP: Drop extension points from Route matches
5 participants
@hbagdi @k8s-ci-robot @youngnick @robscott @jpeach