-
Notifications
You must be signed in to change notification settings - Fork 658
Conformance test for GRPCRouteRequestMirror #3514 #4015
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
base: main
Are you sure you want to change the base?
Conformance test for GRPCRouteRequestMirror #3514 #4015
Conversation
|
Welcome @TaranpreetNatt! |
|
Hi @TaranpreetNatt. 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 Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
|
/release-note-none |
|
/ok-to-test |
LiorLieberman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you also add a test under conformance/tests/mesh?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I can look into it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Lior, I tried to implement gRPC conformance tests under mesh, but I ran into some issues that aren't trivial for me to solve. I encountered the following issues below and what I tried. Maybe my approach is wrong? Is this needed for this issue and PR?
- There is no method in the MeshPod struct to make a gRPC call to the other mesh pod. Looking at what the HTTP MakeRequestAndExpectEventuallyConsistentResponse does, it's essentially running a command on the MeshPod using the client.
- When I tried to replicate that behaviour for gRPC, I got the following. Main issue is the unknown service.
`application@echo-v1-699dd9bfc7-5rxvl:/$ client --http2 -H "content-type: application/grpc" --method=POST http://echo:7070/gateway_api_conformance.echo_basic.grpcecho.GrpcEcho/Echo
[0] Url=http://echo:7070/gateway_api_conformance.echo_basic.grpcecho.GrpcEcho/Echo
[0] Header=content-type:application/grpc
[0] Latency=24.388708ms
[0] ActiveRequests=1
[0] StatusCode=200
[0] ResponseHeader=Content-Type:application/grpc
[0] ResponseHeader=Grpc-Message:unknown service gateway_api_conformance.echo_basic.grpcecho.GrpcEcho
[0] ResponseHeader=Grpc-Status:12
2025-09-04T09:05:37.461782Z info All requests succeeded`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this slipped out of my radar. Sorry!
Can you take a look at https://github.com/kubernetes-sigs/gateway-api/blob/main/conformance/tests/mesh/grpcroute-weight.go#L34-L84
I think it does the grpc call you need
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I'll take a look this weekend.
|
now I see that we already have a PR open for that -- #4013 |
…en HTTP and GRPC Makes the HTTP mirror validation function configurable with a logPattern parameter so it can be reused by gRPC tests, eliminating code duplication. - Parameterize logPattern in HTTP ExpectMirroredRequest - Update gRPC to use shared function with gRPC-specific pattern - Remove duplicate grpc/mirror.go implementation - Make error messages protocol-agnostic Addresses reviewer feedback in PR kubernetes-sigs#4015.
How does it work when two PRs are open for the same issue? |
We usually go by who picked up the issue first. And it looks like you did. I will post a comment on the other PR to hold in the meatime, but this PR is good to continue. |
|
Sounds good, is there anything left on this PR you would like me to get done? |
|
Hi @LiorLieberman, Is there anything left to do in this PR you would like me to get done? I left a comment about the mesh tests as well. |
conformance/utils/http/mirror.go
Outdated
| ) | ||
|
|
||
| func ExpectMirroredRequest(t *testing.T, client client.Client, clientset clientset.Interface, mirrorPods []MirroredBackend, path string, timeoutConfig config.TimeoutConfig) { | ||
| func ExpectMirroredRequest(t *testing.T, client client.Client, clientset clientset.Interface, mirrorPods []MirroredBackend, logPattern string, timeoutConfig config.TimeoutConfig) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dont we have other occasions where ExpectMirroredRequest is called and would need to be changed? Can we instead have in the function a logic to determine if its grpc or http and validate based on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the approach here. Writing the logic inside the http.ExpectMirroredRequest worked but it was messy and the code was confusing. Instead I extracted the common logic into ExpectMirroredRequestWithPattern and wrapped the http.ExpectMirroredRequest and grpc.ExpectMirroredRequest to call ExpectMirroredRequestWithPattern. This way we keep the same function signature and we aren't using the http package in grpc tests.
…ences - Add http.ExpectMirroredRequestWithPattern() with shared validation logic - Both protocols now share the same core validation: goroutine spawning, log reading, regex matching, and error handling
14ea540 to
36e5a4d
Compare
|
/retest |
|
/lgtm Thanks @TaranpreetNatt !! |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: LiorLieberman, TaranpreetNatt The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/assign @shaneutt |
| @@ -0,0 +1,113 @@ | |||
| /* | |||
| Copyright 2025 The Kubernetes Authors. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2026
| ) | ||
|
|
||
| func ExpectMirroredRequest(t *testing.T, client client.Client, clientset clientset.Interface, mirrorPods []MirroredBackend, path string, timeoutConfig config.TimeoutConfig) { | ||
| logPattern := fmt.Sprintf("Echoing back request made t \\%s to client", path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| logPattern := fmt.Sprintf("Echoing back request made t \\%s to client", path) | |
| logPattern := fmt.Sprintf("Echoing back request made to \\%s to client", path) |
|
actually, did you get to handle this comment? https://github.com/kubernetes-sigs/gateway-api/pull/4015/files#r2317173510 Adding the mesh tests? /hold |
I'll take a look |
LiorLieberman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, just tested locally with istio and it fails
grpc.go:333: 2026-01-23T19:13:20.719413473Z: Response expectation failed for request: {<nil> <nil> 0xc000eba258 {OK 0xc0000aaf48 <nil> <nil> [X-Header-Remove]} grpc-infra-backend-v1 gateway-conformance-infra [{{grpc-infra-backend-v2 gateway-conformance-infra} <nil>}] } not ready yet: expected pod name to start with grpc-infra-backend-v1, got grpc-infra-backend-v2-5d756c89dd-rtlbv (after 29.085549379s)
will be great if you can fix it, run it and report back
|
/lgtm cancel |
Okay, I'll take a look at this as well. |
What type of PR is this?
/kind test
/area conformance-test
What this PR does / why we need it:
Adds conformance tests for GRPCRoute request mirroring functionality to enable gradual implementation support across Gateway API implementations.
Which issue(s) this PR fixes:
Fixes #3514
Does this PR introduce a user-facing change?: