Skip to content

Move HTTPRouteRule and GRPCRouteRule 'name' fields to Standard #3826

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
Merged
merged 9 commits into from
Jul 3, 2025
Merged

Move HTTPRouteRule and GRPCRouteRule 'name' fields to Standard #3826

merged 9 commits into from
Jul 3, 2025

Conversation

guicassolato
Copy link
Contributor

@guicassolato guicassolato commented Jun 2, 2025 •
edited
Loading

What type of PR is this?

/kind gep
/area conformance-test

What this PR does / why we need it:

Moves HTTPRouteRule and GRPCRouteRule name fields (GEP-995) to Standard.

Which issue(s) this PR fixes:

Fixes #995

Does this PR introduce a user-facing change?:

Promote the `name` field of the HTTPRouteRule and GRPCRouteRule types to Standard.

@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 release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/gep PRs related to Gateway Enhancement Proposal(GEP) area/conformance-test Issues or PRs related to Conformance tests. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 2, 2025
@k8s-ci-robot k8s-ci-robot requested review from kflynn and mlavacca June 2, 2025 16:00
@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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 2, 2025
@kflynn
Copy link
Contributor

kflynn commented Jun 3, 2025

@guicassolato, should this be moved out of draft?

@guicassolato guicassolato marked this pull request as ready for review June 3, 2025 14:57
@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 3, 2025
@guicassolato
Copy link
Contributor Author

@guicassolato, should this be moved out of draft?

Done, @kflynn.

I wanted to get an idea if, at a glance, it looks good, but apparently ppl are already approving it. So I guess I got my answer 🙂

Thanks!

@LiorLieberman
Copy link
Member

/cc

LiorLieberman
LiorLieberman reviewed Jun 3, 2025
View reviewed changes
Copy link
Member

@LiorLieberman LiorLieberman left a comment

Choose a reason for hiding this comment

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

looks good to me, thanks @guicassolato !

Can you also add two conformance tests in conformance/tests/mesh ? it should be very similar tests with just a few changes. you can take an example from the tests thats already in that folder.

@guicassolato
Copy link
Contributor Author

Can you also add two conformance tests in conformance/tests/mesh ? it should be very similar tests with just a few changes. you can take an example from the tests thats already in that folder.

@LiorLieberman,

I can add one, for HTTP. (Actually just pushed.)

For GRPC, I could use some help, since we don't seem to have the tooling in place for mesh conformance.

@LiorLieberman
Copy link
Member

LiorLieberman commented Jun 3, 2025 •
edited
Loading

wdym tooling in place for mesh conformance?

assuming you have istio installed, you can easily test your conformance test by running (from gateway-api root folder):

 go test -v ./conformance --gateway-class istio --run TestConformance/<YOUR_NEW_TEST_NAME> --cleanup-base-resources=false  --supported-features=Mesh,HTTPRoute,<SUPPORTED_FEATURE_NAME_OF_YOUR_TEST_IF_ANY> -namespace-labels istio-injection=enabled

Let me know if you encounter any issues with it.

@LiorLieberman
Copy link
Member

LiorLieberman commented Jun 3, 2025 •
edited
Loading

/hold until featureNames are in at least

@guicassolato
Copy link
Contributor Author

wdym tooling in place for mesh conformance?

I meant test helpers to make the GRPC requests, like we do for n/s.

But if I'm wrong and you can give an example, I'd appreciate it and happy to add to the PR.

LiorLieberman
LiorLieberman reviewed Jun 4, 2025
View reviewed changes
Copy link
Member

@LiorLieberman LiorLieberman left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks! Mind actually running the tests with the command I pasted above (for gateway and mesh) and see if it works?

@LiorLieberman
Copy link
Member

I meant test helpers to make the GRPC requests, like we do for n/s.

But if I'm wrong and you can give an example, I'd appreciate it and happy to add to the PR.

I see what you mean. right, there isn't right now. Either add it or defer for later. I am fine having the PR without the grpc conformance for mesh + have an issue to backfill later.

LiorLieberman
LiorLieberman reviewed Jun 5, 2025
View reviewed changes
Copy link
Member

@LiorLieberman LiorLieberman left a comment

Choose a reason for hiding this comment

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

Did you run the tests?

@guicassolato
Copy link
Contributor Author

Did you run the tests?

Yep.

Screenshot 2025-06-05 at 21 03 36

@youngnick
Copy link
Contributor

LGTM, but I'd like to see a review from @mlavacca as well.

@guicassolato guicassolato mentioned this pull request Jun 7, 2025
Open
@shaneutt shaneutt requested a review from mikemorris June 9, 2025 12:37
LiorLieberman
LiorLieberman reviewed Jun 10, 2025
View reviewed changes
Copy link
Member

@LiorLieberman LiorLieberman 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 to me. Thanks!

/lgtm
/approve

leaving the hold per @youngnick comment

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 10, 2025
@LiorLieberman LiorLieberman mentioned this pull request Jun 13, 2025
Merged
@LiorLieberman
Copy link
Member

/assign @mlavacca

@shaneutt shaneutt self-requested a review June 19, 2025 11:24
@shaneutt shaneutt self-assigned this Jun 19, 2025
mlavacca
mlavacca reviewed Jun 24, 2025
View reviewed changes
Copy link
Member

@mlavacca mlavacca left a comment

Choose a reason for hiding this comment

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

Thanks for this PR, @guicassolato!

@guicassolato
conformance tests flagged as provisional
d4b592a 
Signed-off-by: Guilherme Cassolato <guicassolato@gmail.com>
@guicassolato
fix: typo
9f14021 
Signed-off-by: Guilherme Cassolato <guicassolato@gmail.com>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 26, 2025
shaneutt
shaneutt approved these changes Jun 26, 2025
View reviewed changes
Copy link
Member

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

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

/approve

I think the only thing to navigate is whether we want to actually mark it Standard yet, or if we want some soak time for the new conformance tests. If we merge as-is that's fine, we just need to track getting some feedback on those tests before v1.4.0 releases.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: guicassolato, howardjohn, LiorLieberman, shaneutt

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 26, 2025
@guicassolato guicassolato requested a review from mlavacca June 26, 2025 12:56
mlavacca
mlavacca reviewed Jul 3, 2025
View reviewed changes
Copy link
Member

@mlavacca mlavacca left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 3, 2025
@mlavacca
Copy link
Member

mlavacca commented Jul 3, 2025

/unhold

@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 Jul 3, 2025
@k8s-ci-robot k8s-ci-robot merged commit 63921d1 into kubernetes-sigs:main Jul 3, 2025
13 checks passed
This was referenced Jul 3, 2025
Open
Open
@mikemorris
Copy link
Contributor

mikemorris commented Jul 3, 2025 •
edited
Loading

Apologies for the delay in getting back to this, but looks great where it ended up!

Re #3826 (comment) I added a comment at #3566 (comment) about formalizing GRPCRoute support in mesh as potential scope for the v1.5 timeframe, and should be able to confirm shortly if we're passing these additional tests in Istio.

@shaneutt
Copy link
Member

shaneutt commented Jul 3, 2025

That would be great, thank you @mikemorris.

@shaneutt shaneutt added the v1.4-release/subtask This indicates a subtask of a feature, bug, or smaller issue for the v1.4 release. label Jul 3, 2025
@shaneutt shaneutt moved this to Done in Release v1.4.0 Jul 3, 2025
@shaneutt shaneutt added this to the v1.4.0 milestone Jul 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mlavacca mlavacca mlavacca left review comments

@LiorLieberman LiorLieberman LiorLieberman left review comments

@howardjohn howardjohn howardjohn approved these changes

@shaneutt shaneutt shaneutt approved these changes

@kflynn kflynn Awaiting requested review from kflynn

@michaelbeaumont michaelbeaumont Awaiting requested review from michaelbeaumont

@mikemorris mikemorris Awaiting requested review from mikemorris

Assignees

@shaneutt shaneutt

@mlavacca mlavacca

@LiorLieberman LiorLieberman

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/conformance-test Issues or PRs related to Conformance tests. 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. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. v1.4-release/subtask This indicates a subtask of a feature, bug, or smaller issue for the v1.4 release.
Projects
Release v1.4.0
Status: Done
Milestone
v1.4.0
Development

Successfully merging this pull request may close these issues.

GEP: Add Name to HTTPRouteRule and HTTPRouteMatch
9 participants
@guicassolato @k8s-ci-robot @kflynn @LiorLieberman @youngnick @mlavacca @mikemorris @shaneutt @howardjohn