Add GRPCRoute weighted backendRefs test #3962

sarthyparty · 2025-07-28T22:09:33Z

What type of PR is this?

/kind test

What this PR does / why we need it:

Adds GRPCRoute weighted BackendRefs test. Increase conformance feature coverage of GRPCRoute

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2025-07-28T22:09:41Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sarthyparty
Once this PR has been reviewed and has the lgtm label, please assign arkodg for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

conformance/OWNERS

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 · 2025-07-28T22:09:42Z

Welcome @sarthyparty!

It looks like this is your first PR to kubernetes-sigs/gateway-api 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/gateway-api has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2025-07-28T22:09:43Z

Hi @sarthyparty. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

sjberman · 2025-07-29T14:42:30Z

Question for the maintainers, since weight is a Core feature of GRPCRoute, should this test live in the Core GRPC tests, or do users need to opt-in to run it? Don't want to break any users that are running GRPCRoute tests that don't have this support, but also want to make sure that Core is treated properly.

LiorLieberman · 2025-08-07T16:00:02Z

/ok-to-test

LiorLieberman

Can you also add the tests in conformance/tests/mesh/

conformance/tests/grpcroute-weight.go

conformance/tests/grpcroute-weight.yaml

conformance/tests/grpcroute-weight.go

LiorLieberman · 2025-08-07T16:09:18Z

Question for the maintainers, since weight is a Core feature of GRPCRoute, should this test live in the Core GRPC tests, or do users need to opt-in to run it? Don't want to break any users that are running GRPCRoute tests that don't have this support, but also want to make sure that Core is treated properly.

Good question. I think this is a core feature of GRPCRoute, meaning everyone who supports GRPCRoute is expected to support this. You are right that we did not have conformance for that (which is sub-optimal) but the implementors of this should have already supported this feature.

So I think adding coverage is good, and if we break someone - thats a good sign they should fix it, cause the promise we give to our users is that weights are supported with grpcRoute

/cc @mikemorris @robscott for thoughts

k8s-ci-robot · 2025-08-07T16:09:22Z

@LiorLieberman: GitHub didn't allow me to request PR reviews from the following users: for, thoughts.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

Question for the maintainers, since weight is a Core feature of GRPCRoute, should this test live in the Core GRPC tests, or do users need to opt-in to run it? Don't want to break any users that are running GRPCRoute tests that don't have this support, but also want to make sure that Core is treated properly.

Good question. I think this is a core feature of GRPCRoute, meaning everyone who supports GRPCRoute is expected to support this. You are right that we did not have conformance for that (which is sub-optimal) but the implementors of this should have already supported this feature.

So I think adding coverage is good, and if we break someone - thats a good sign they should fix it, cause the promise we give to our users is that weights are supported with grpcRoute

/cc @mikemorris @robscott for thoughts

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

youngnick · 2025-08-07T23:49:31Z

Yes, agreed that weighted load-balancing is a Core feature, meaning that support is not optional. So if we break existing implementations, they'll need to fix it before they can claim support for the release this is included in (that is, v1.4).

LiorLieberman · 2025-08-09T00:24:34Z

/retest

LiorLieberman · 2025-08-09T00:25:36Z

Can you also add the tests in conformance/tests/mesh/

@sarthyparty ping on this in case you missed that part of the comment

sarthyparty · 2025-08-09T00:48:25Z

Can you also add the tests in conformance/tests/mesh/

@sarthyparty ping on this in case you missed that part of the comment

Oh shoot thanks for ping I missed it. Also I'll fix the lint issue

sarthyparty · 2025-08-11T22:59:18Z

@LiorLieberman Do you want me to add gRPC mesh setup for testing? There doesn't seem to be any GRPC tests there. Or did you just mean to refactor the existing http weight test to use the shared func?

LiorLieberman · 2025-08-11T23:07:59Z

@LiorLieberman Do you want me to add gRPC mesh setup for testing? There doesn't seem to be any GRPC tests there. Or did you just mean to refactor the existing http weight test to use the shared func?

I dont think you need any special setup beyond what we already have in mesh base manifests for conformance. (feel free to shout if I am missing something here).

I did mean to add grpc test to this folder as well, so it adds mesh coverage for that.

sarthyparty · 2025-08-11T23:38:03Z

/retest

sarthyparty · 2025-08-11T23:44:55Z

Haven't ran the mesh test on an implementation yet

conformance/tests/mesh/grpcroute-weight.go

LiorLieberman · 2025-08-12T17:09:53Z

I've tested it with istio, httproute-weight works. grpcRoute test is skipped, due to my above comment.

If you want to run it - you can use the below command from your branch

go test -v ./conformance --gateway-class istio --run TestConformance/MeshGRPCRouteWeight --cleanup-base-resources=false --supported-features=Mesh,GRPCRoute -namespace-labels istio-injection=enabled

(assuming you have istio installed)

sarthyparty · 2025-08-12T21:58:38Z

/retest

sarthyparty · 2025-08-12T21:58:54Z

I just tested the GRPC weight test on istio and it worked

LiorLieberman · 2025-08-12T22:56:12Z

I just tested the GRPC weight test on istio and it worked

thanks, mind sharing the command and logs you've run? in my local env it fails.

sarthyparty · 2025-08-12T23:22:06Z

I just tested the GRPC weight test on istio and it worked

thanks, mind sharing the command and logs you've run? in my local env it fails.

Oh weird, I ran it again and it failed. Gonna look at this again

sarthyparty · 2025-08-12T23:29:25Z

I'm not sure how the cleanup makes a difference, but when I ran the command you gave without the cleanup flag set to false, it passed.

go test -v ./conformance --gateway-class istio --run TestConformance/MeshGRPCRouteWeight  --supported-features=Mesh,GRPCRoute -namespace-labels istio-injection=enabled

LiorLieberman · 2025-08-13T00:09:59Z

can you paste the logs?

it looks like with the changes you made, it is now doing http and not grpc

LiorLieberman · 2025-08-13T00:15:38Z

conformance/tests/mesh/grpcroute-weight.go

+			expected := http.ExpectedResponse{
+				Request:   http.Request{Protocol: "grpc", Path: "", Host: "echo:7070"},
+				Response:  http.Response{StatusCode: 200},
+				Namespace: "gateway-conformance-mesh",
+			}


it looks like it is now doing http..? there is no sendGRPC stuff

From my understand the way that the mesh http route weight test is working is by execing into the pod and running a command client http://echo:7070. Using the grpc package doesn't work, since I need to run the request from the pod I think?

I was able to configure this by just setting the protocol to grpc in the http Request structure. I think the correct way to do this is probably to refactor the echo client code to accept a structure that is independent of protocol.

req := m.rc.Post(). Resource("pods"). Name(m.Name). Namespace(m.Namespace). SubResource("exec"). Param("container", container). VersionedParams(&v1.PodExecOptions{ Container: container, Command: args, Stdin: false, Stdout: true, Stderr: true, TTY: false, }, scheme.ParameterCodec)

This code here utils/echo/pod.go L168, with args being ['client', 'http://echo:7070'] for http. By setting the protocol to grpc it becomes ['client', 'grpc://echo:7070'] which makes a grpc request I believe

I think I will refactor the echo pod function CaptureRequestResponseAndCompare to accept a structure that is protocol independent since it doesn't need to be http specific.

Actually its a lot more code change than I thought to refactor that, what do you think

this is istio client code

/cc @howardjohn can you relate to this?

sarthyparty · 2025-08-13T01:30:10Z

... a lot of grpc request sending logs above here...
--- PASS: TestConformance (20.17s)
    --- PASS: TestConformance/MeshGRPCRouteWeight (19.91s)
        --- PASS: TestConformance/MeshGRPCRouteWeight/Requests_should_have_a_distribution_that_matches_the_weight (19.89s)
PASS
ok  	sigs.k8s.io/gateway-api/conformance	20.734s

LiorLieberman · 2025-08-13T01:33:51Z

See my other comment, it looks like you are sending http and not grpc now

LiorLieberman · 2025-08-13T15:30:01Z

/cc @howardjohn

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 28, 2025

k8s-ci-robot requested review from LiorLieberman and sunjayBhatia July 28, 2025 22:09

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 28, 2025

sarthyparty mentioned this pull request Jul 28, 2025

GRPCRoute Conformance Test for BackendRef Weight Traffic Splitting nginx/nginx-gateway-fabric#1770

Open

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 7, 2025

LiorLieberman reviewed Aug 7, 2025

View reviewed changes

conformance/tests/grpcroute-weight.go Outdated Show resolved Hide resolved

conformance/tests/grpcroute-weight.yaml Outdated Show resolved Hide resolved

conformance/tests/grpcroute-weight.go Outdated Show resolved Hide resolved

k8s-ci-robot requested review from mikemorris and robscott August 7, 2025 16:09

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 11, 2025

sarthyparty added 5 commits August 11, 2025 15:45

Add GRPCRoute weighted backendRefs test

a880d85

use default grpc client

0641e88

reorganize imports

bcb34aa

refactor weight test into shared function and fix nits

56a9015

fix lint

cc54768

add grpc entropy

bef25ff

sarthyparty force-pushed the grpcroute-weight-test branch from 41fa6c3 to bef25ff Compare August 11, 2025 22:54

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 11, 2025

use mesh sender

493804e

fix import cycle

9496c74

add grpc mesh test

5c8a990

LiorLieberman reviewed Aug 12, 2025

View reviewed changes

conformance/tests/mesh/grpcroute-weight.go Outdated Show resolved Hide resolved

sarthyparty added 2 commits August 12, 2025 14:35

fix format and mesh test

2310926

add newline

a311380

LiorLieberman reviewed Aug 13, 2025

View reviewed changes

k8s-ci-robot requested a review from howardjohn August 13, 2025 15:30

Add GRPCRoute weighted backendRefs test #3962

Are you sure you want to change the base?

Add GRPCRoute weighted backendRefs test #3962

Conversation

sarthyparty commented Jul 28, 2025

Uh oh!

k8s-ci-robot commented Jul 28, 2025

Uh oh!

k8s-ci-robot commented Jul 28, 2025

Uh oh!

k8s-ci-robot commented Jul 28, 2025

Uh oh!

sjberman commented Jul 29, 2025

Uh oh!

LiorLieberman commented Aug 7, 2025

Uh oh!

LiorLieberman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LiorLieberman commented Aug 7, 2025

Uh oh!

k8s-ci-robot commented Aug 7, 2025

Uh oh!

youngnick commented Aug 7, 2025

Uh oh!

LiorLieberman commented Aug 9, 2025

Uh oh!

LiorLieberman commented Aug 9, 2025

Uh oh!

sarthyparty commented Aug 9, 2025

Uh oh!

sarthyparty commented Aug 11, 2025

Uh oh!

LiorLieberman commented Aug 11, 2025

Uh oh!

sarthyparty commented Aug 11, 2025

Uh oh!

sarthyparty commented Aug 11, 2025

Uh oh!

Uh oh!

LiorLieberman commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sarthyparty commented Aug 12, 2025

Uh oh!

sarthyparty commented Aug 12, 2025

Uh oh!

LiorLieberman commented Aug 12, 2025

Uh oh!

sarthyparty commented Aug 12, 2025

Uh oh!

sarthyparty commented Aug 12, 2025

Uh oh!

LiorLieberman commented Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LiorLieberman Aug 13, 2025

Choose a reason for hiding this comment

Uh oh!

sarthyparty Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sarthyparty Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sarthyparty Aug 13, 2025

Choose a reason for hiding this comment

Uh oh!

sarthyparty Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LiorLieberman Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

LiorLieberman commented Aug 12, 2025 •

edited

Loading

LiorLieberman commented Aug 13, 2025 •

edited

Loading

sarthyparty Aug 13, 2025 •

edited

Loading

sarthyparty Aug 13, 2025 •

edited

Loading

sarthyparty Aug 13, 2025 •

edited

Loading

LiorLieberman Aug 13, 2025 •

edited

Loading