-
Notifications
You must be signed in to change notification settings - Fork 631
add preserveExternalRequestId and "generateRequestId" to HCM ListenerPolicy and HTTPListenerPolicy #13250
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
Conversation
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.
Pull request overview
This pull request adds support for controlling Envoy's request ID generation and preservation behavior by introducing two new fields (preserveExternalRequestId and generateRequestId) to both HTTPListenerPolicy and ListenerPolicy resources.
Key changes:
- Added
preserveExternalRequestIdandgenerateRequestIdboolean fields to the HTTPSettings API - Implemented merge and translation logic for the new fields
- Added test cases and example configurations
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| api/v1alpha1/kgateway/listener_policy_types.go | Added PreserveExternalRequestId and GenerateRequestId fields to HTTPSettings struct with documentation |
| api/v1alpha1/kgateway/zz_generated.deepcopy.go | Generated DeepCopy methods for the new fields |
| install/helm/kgateway-crds/templates/*.yaml | Updated CRD schemas for both ListenerPolicy and HTTPListenerPolicy |
| pkg/kgateway/extensions2/plugins/listenerpolicy/http.go | Added new fields to HttpListenerPolicyIr struct and initialization logic |
| pkg/kgateway/extensions2/plugins/listenerpolicy/merge_http.go | Implemented merge functions for the new fields |
| pkg/kgateway/extensions2/plugins/listenerpolicy/listener_plugin.go | Applied the new fields to Envoy HCM configuration |
| pkg/kgateway/translator/gateway/gateway_translator_test.go | Added test cases for both new fields |
| pkg/kgateway/translator/gateway/testutils/inputs/httplistenerpolicy/*.yaml | Created test input files (with typo in filenames) |
| pkg/kgateway/translator/gateway/testutils/outputs/httplistenerpolicy/*.yaml | Created expected output files (with typo in filenames) |
| test/e2e/features/listener_policy/testdata/*.yaml | Added e2e test configurations |
| examples/example-gw-with-http-listener-policy.yaml | Updated example with preserveExternalRequestId field |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }) | ||
| }) | ||
|
|
||
|
|
Copilot
AI
Jan 9, 2026
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.
Extra blank line should be removed to maintain consistent spacing with other test cases.
| @@ -0,0 +1,152 @@ | |||
| Clusters: | |||
Copilot
AI
Jan 9, 2026
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.
The filename has a typo: "preseve" should be "preserve". This filename is inconsistent with the test reference which uses "preserve-external-request-id.yaml".
| @@ -0,0 +1,47 @@ | |||
| apiVersion: gateway.networking.k8s.io/v1 | |||
Copilot
AI
Jan 9, 2026
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.
The filename has a typo: "preseve" should be "preserve". This filename is inconsistent with the test reference which uses "preserve-external-request-id.yaml".
| out.UseRemoteAddress = wrapperspb.Bool(*policy.useRemoteAddress) | ||
| } | ||
| if policy.preserveExternalRequestId != nil { | ||
| out.PreserveExternalRequestId = *policy.preserveExternalRequestId |
Copilot
AI
Jan 9, 2026
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.
Inconsistent wrapping with wrapperspb.Bool(). Line 358 sets preserveExternalRequestId without wrapping (dereferencing directly), while line 361 sets generateRequestId with wrapperspb.Bool(). Both fields should be handled consistently. Looking at how useRemoteAddress is set on line 355, it uses wrapperspb.Bool(), so preserveExternalRequestId should also use wrapperspb.Bool().
| out.PreserveExternalRequestId = *policy.preserveExternalRequestId | |
| out.PreserveExternalRequestId = wrapperspb.Bool(*policy.preserveExternalRequestId) |
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.
If I use wrapperspb I get, as this doesn't match the envoy proto
cannot use wrapperspb.Bool(*policy.preserveExternalRequestId) (value of type *wrapperspb.BoolValue) as bool value in assignment
| apiVersion: gateway.kgateway.dev/v1alpha1 | ||
| kind: HTTPListenerPolicy | ||
| metadata: | ||
| name: generate-request-id |
Copilot
AI
Jan 9, 2026
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.
The metadata name "generate-request-id" doesn't match the filename "generate-request-id-false.yaml". For consistency with other test files and the actual test behavior (testing when the value is false), consider renaming to "generate-request-id-false" to match the filename.
| name: generate-request-id | |
| name: generate-request-id-false |
| apiVersion: gateway.kgateway.dev/v1alpha1 | ||
| kind: ListenerPolicy | ||
| metadata: | ||
| name: http-listener-policy-all-fields |
Copilot
AI
Jan 9, 2026
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.
The policy name "http-listener-policy-all-fields" is misleading. This file tests only the generateRequestId field set to false, not "all fields". For clarity and consistency with the filename, the name should be something like "http-listener-policy-generate-request-id" or "http-listener-policy-generate-request-id-false".
| name: http-listener-policy-all-fields | |
| name: http-listener-policy-generate-request-id |
|
looks good to me, can you make CI happy and we can merge? |
Signed-off-by: leon <[email protected]>
Signed-off-by: leon <[email protected]>
Signed-off-by: leon <[email protected]>
Signed-off-by: leon <[email protected]>
Signed-off-by: leon <[email protected]>
Signed-off-by: leon <[email protected]>
Signed-off-by: leon <[email protected]>
688eb0d to
ebd0188
Compare
|
Thanks for checking @yuval-k I fixed the errors, hoping the tests pass, did the rebase too for the DCO |
Description
This adds the option to disable envoy generated request IDs and also preserve any external request ID that is sent.
Adds new fields to
ListenerPolicyandHTTPListenerPolicyFixes #13249
Change Type
/kind feature
Changelog
preserveExternalRequestIdgenerateRequestIdtoHttpListenerPolicyandListenerPolicyAdditional Notes