WIP: Add e2e test for manual GatewayAPI service#1384
WIP: Add e2e test for manual GatewayAPI service#1384rikatz wants to merge 4 commits intoopenshift:masterfrom
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a new end-to-end test 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can make CodeRabbit's review stricter and more nitpicky using the `assertive` profile, if that's what you prefer.Change the |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/e2e/util_gatewayapi_test.go (1)
320-321: Fix helper docstring to match the function purpose.The comment mentions
createGatewayClass, but this helper creates aService. Please update it to avoid confusion during maintenance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/util_gatewayapi_test.go` around lines 320 - 321, The docstring currently references createGatewayClass but the helper actually creates and returns a Service; update the comment to correctly describe the helper (e.g., "createService" or "createTestService") and state that it creates and returns a Service (or returns an error on failure), replacing any mentions of createGatewayClass so the comment matches the actual function behavior and the Service return value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e/util_gatewayapi_test.go`:
- Around line 322-328: The helper createGatewayService currently omits the
Service namespace; update the function signature createGatewayService to accept
a namespace string, set svcDefinition.ObjectMeta.Namespace = namespace when
building the corev1.Service, and ensure any construction of svckey (or use of
svckey.Namespace) uses that provided namespace; also update callers of
createGatewayService to pass the intended namespace so Service creation and logs
reference the correct namespace.
---
Nitpick comments:
In `@test/e2e/util_gatewayapi_test.go`:
- Around line 320-321: The docstring currently references createGatewayClass but
the helper actually creates and returns a Service; update the comment to
correctly describe the helper (e.g., "createService" or "createTestService") and
state that it creates and returns a Service (or returns an error on failure),
replacing any mentions of createGatewayClass so the comment matches the actual
function behavior and the Service return value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 4aa055c1-29f2-4d50-83bb-ac5154786c13
📒 Files selected for processing (2)
test/e2e/gateway_api_test.gotest/e2e/util_gatewayapi_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/e2e/gateway_api_test.go (1)
1044-1046: Acknowledged: Connection test is pending.The TODO aligns with the WIP status and PR description which mentions adding a connectivity test for
externalTrafficPolicy=Local.Would you like me to help draft a connection test that verifies client source IP preservation with
externalTrafficPolicy=Local?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/gateway_api_test.go` around lines 1044 - 1046, Add a connection test that verifies client source IP preservation when externalTrafficPolicy is set to Local: create a test case (near where gatewayName and externalTrafficPolicy are referenced) that deploys a backend pod which logs remote client IP, exposes it via the Gateway/LoadBalancer with externalTrafficPolicy=Local, then from a client pod send an HTTP request to the gateway address and fetch the backend's observed source IP; assert that the observed IP matches the client pod's IP (or falls within the node's external IP range) and fail the test if NAT is observed. Ensure the test creates/uses gatewayName, sets externalTrafficPolicy=Local on the Service/Gateway, polls/waits for readiness, collects backend logs or response containing the remote IP, and performs proper cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e/gateway_api_test.go`:
- Around line 958-960: The error message in the t.Fatalf call is misleading
because it prints "%s/%s" with gatewayName and gatewayClass.Name while the real
service name is constructed as "{gatewayName}-{gatewayClass}" by
createGatewayService; update the t.Fatalf invocation in the test (the line that
calls t.Fatalf("Failed to create service ...", gatewayName, gatewayClass.Name,
err)) to either compute serviceName := createGatewayService(gatewayName,
gatewayClass.Name) and log that single string (e.g. "Failed to create service
%s: %v", serviceName, err) or format the two values into the actual
"{gatewayName}-{gatewayClass}" form before passing to t.Fatalf so the message
accurately reflects the created service name.
- Around line 1007-1042: The test is polling services by metadata label but
createGatewayService only sets the gateway name in Spec.Selector; update
createGatewayService to add the gateway.networking.k8s.io/gateway-name label
into the Service.ObjectMeta.Labels (use the same gatewayName value) so
client.MatchingLabels in the polling loop and the gateway-service-dns controller
(which checks service.Labels[gatewayNameLabelKey]) will find the Service; ensure
any existing ObjectMeta.Labels map is initialized before assignment.
---
Nitpick comments:
In `@test/e2e/gateway_api_test.go`:
- Around line 1044-1046: Add a connection test that verifies client source IP
preservation when externalTrafficPolicy is set to Local: create a test case
(near where gatewayName and externalTrafficPolicy are referenced) that deploys a
backend pod which logs remote client IP, exposes it via the Gateway/LoadBalancer
with externalTrafficPolicy=Local, then from a client pod send an HTTP request to
the gateway address and fetch the backend's observed source IP; assert that the
observed IP matches the client pod's IP (or falls within the node's external IP
range) and fail the test if NAT is observed. Ensure the test creates/uses
gatewayName, sets externalTrafficPolicy=Local on the Service/Gateway,
polls/waits for readiness, collects backend logs or response containing the
remote IP, and performs proper cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: f1ec3ee5-364b-4fb3-93f5-0d14679e14cc
📒 Files selected for processing (2)
test/e2e/gateway_api_test.gotest/e2e/util_gatewayapi_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/util_gatewayapi_test.go
| if err != nil { | ||
| t.Fatalf("Failed to create service %s/%s: %v", gatewayName, gatewayClass.Name, err) | ||
| } |
There was a problem hiding this comment.
Misleading error message format.
The %s/%s format suggests namespace/name, but the values passed are gatewayName and gatewayClass.Name. The actual service name is {gatewayName}-{gatewayClass} per the createGatewayService helper.
Suggested fix
if err != nil {
- t.Fatalf("Failed to create service %s/%s: %v", gatewayName, gatewayClass.Name, err)
+ t.Fatalf("Failed to create service %s-%s: %v", gatewayName, gatewayClass.Name, err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err != nil { | |
| t.Fatalf("Failed to create service %s/%s: %v", gatewayName, gatewayClass.Name, err) | |
| } | |
| if err != nil { | |
| t.Fatalf("Failed to create service %s-%s: %v", gatewayName, gatewayClass.Name, err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/gateway_api_test.go` around lines 958 - 960, The error message in
the t.Fatalf call is misleading because it prints "%s/%s" with gatewayName and
gatewayClass.Name while the real service name is constructed as
"{gatewayName}-{gatewayClass}" by createGatewayService; update the t.Fatalf
invocation in the test (the line that calls t.Fatalf("Failed to create service
...", gatewayName, gatewayClass.Name, err)) to either compute serviceName :=
createGatewayService(gatewayName, gatewayClass.Name) and log that single string
(e.g. "Failed to create service %s: %v", serviceName, err) or format the two
values into the actual "{gatewayName}-{gatewayClass}" form before passing to
t.Fatalf so the message accurately reflects the created service name.
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. 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. |
|
@rikatz: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
2 participants
This PR adds an e2e test for manual service deployment on Gateway API
TODO: