NE-2422: Fix router e2e tests for dual-stack AWS clusters#30934
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@alebedev87: This pull request references NE-2422 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughTests updated to detect AWS dual-stack IP family and adapt router test behavior: conditional HAProxy IPv6 bind, switching socat/listener to TCP6, in-memory config edits before applying, and disabling proxy protocol for dual-stack AWS clusters. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
openshift-ci
bot
added
the
approved
|
@alebedev87: This pull request references NE-2422 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/extended/router/headers.go (1)
87-95: Fail fast if the fixture rewrite stops matching.Line 93's
ReplaceAllbecomes a silent no-op iftest/extended/testdata/router/router-http-echo-server.yamlstops containing the expectedTCP4-LISTENstanza, so the test can drift back to the old behavior without a clear signal. Checking the match count and replacing once makes that failure mode obvious.♻️ Example hardening
configData, err := os.ReadFile(configPath) o.Expect(err).NotTo(o.HaveOccurred()) config := string(configData) // On IPv6-primary clusters, socat must listen on IPv6. if dualStackIPv6Primary { + o.Expect(strings.Count(config, "TCP4-LISTEN")).To(o.Equal(1), "expected a single TCP4-LISTEN stanza in fixture") g.By("with IPv6 listen") - config = strings.ReplaceAll(config, "TCP4-LISTEN", "TCP6-LISTEN") + config = strings.Replace(config, "TCP4-LISTEN", "TCP6-LISTEN", 1) } err = oc.Run("create").Args("-f", "-").InputString(config).Execute()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/router/headers.go` around lines 87 - 95, The current rewrite using strings.ReplaceAll on the config string (see variable config and the dualStackIPv6Primary branch) silently does nothing if "TCP4-LISTEN" is not present; modify this to detect and fail fast by checking the match count or using strings.Replace with a count of 1 and asserting that a replacement occurred: compute occurrences := strings.Count(config, "TCP4-LISTEN"); if occurrences == 0 { o.Expect(fmt.Errorf("expected TCP4-LISTEN stanza not found")).NotTo(o.HaveOccurred()) } else { config = strings.Replace(config, "TCP4-LISTEN", "TCP6-LISTEN", 1) } so the test fails loudly if the fixture no longer matches and only one replacement is performed.test/extended/router/metrics.go (1)
55-67: Centralize the AWS IP-family lookup.This same
PlatformStatus.AWS.IPFamilyplumbing now exists intest/extended/router/h2spec.go,test/extended/router/idle.go, andtest/extended/router/headers.gotoo, and the call sites already diverge a bit on nil-handling. A small helper inpackage routerthat returns the AWS IP family would keep the “any dual-stack” and “IPv6-primary only” callers explicit without repeating the infrastructure parsing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/router/metrics.go` around lines 55 - 67, Centralize the repeated AWS IPFamily extraction by adding a small helper in package router (e.g., func GetAWSIPFamily(infra *configv1.Infrastructure) (*configv1.IPFamily, bool)) that safely handles nil PlatformStatus/AWS and returns the IPFamily plus a presence flag; then replace the inline plumbing in metrics.go (the dualStackIPFamily calculation and use in proxyProtocol) and the similar code in test/extended/router/h2spec.go, idle.go, and headers.go to call GetAWSIPFamily and base dual-stack and IPv6-primary checks on its result instead of duplicating nil checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/extended/router/headers.go`:
- Around line 87-95: The current rewrite using strings.ReplaceAll on the config
string (see variable config and the dualStackIPv6Primary branch) silently does
nothing if "TCP4-LISTEN" is not present; modify this to detect and fail fast by
checking the match count or using strings.Replace with a count of 1 and
asserting that a replacement occurred: compute occurrences :=
strings.Count(config, "TCP4-LISTEN"); if occurrences == 0 {
o.Expect(fmt.Errorf("expected TCP4-LISTEN stanza not
found")).NotTo(o.HaveOccurred()) } else { config = strings.Replace(config,
"TCP4-LISTEN", "TCP6-LISTEN", 1) } so the test fails loudly if the fixture no
longer matches and only one replacement is performed.
In `@test/extended/router/metrics.go`:
- Around line 55-67: Centralize the repeated AWS IPFamily extraction by adding a
small helper in package router (e.g., func GetAWSIPFamily(infra
*configv1.Infrastructure) (*configv1.IPFamily, bool)) that safely handles nil
PlatformStatus/AWS and returns the IPFamily plus a presence flag; then replace
the inline plumbing in metrics.go (the dualStackIPFamily calculation and use in
proxyProtocol) and the similar code in test/extended/router/h2spec.go, idle.go,
and headers.go to call GetAWSIPFamily and base dual-stack and IPv6-primary
checks on its result instead of duplicating nil checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0483dd68-3840-4784-84a0-d2fdc8a75c26
📒 Files selected for processing (4)
test/extended/router/h2spec.gotest/extended/router/headers.gotest/extended/router/idle.gotest/extended/router/metrics.go
|
/test lint |
|
/test go-verify-deps |
|
Scheduling required tests: |
|
/testwith openshift/installer/main/e2e-aws-ovn-dualstack-ipv6-primary-techpreview openshift/cloud-provider-aws#135 openshift/installer#10380 openshift/cluster-kube-apiserver-operator#2079 openshift/cluster-ingress-operator#1376 |
|
Tests which were adopted in this PR passed (log file): |
|
/verified by CI |
openshift-ci-robot
added
the
verified
|
@lihongan: This PR has been marked as verified by DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
@alebedev87: This pull request references NE-2422 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
davidesalerno
left a comment
davidesalerno
left a comment
There was a problem hiding this comment.
The code seems to be ok, just a couple of observations (severity is very low)
…usters Dual-stack installations on AWS are forced to use NLB by the installer, which doesn't support proxy protocol. Skip proxy protocol when the infrastructure IPFamily is DualStackIPv4Primary or DualStackIPv6Primary. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…al-stack The h2spec test deploys a standalone haproxy pod as a backend to verify HTTP/2 conformance. On dual-stack clusters, this test haproxy needs a separate IPv6 bind with v6only to listen on both address families. Add the extra bind line when the infrastructure IPFamily is one of the DualStack values. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
On IPv6-primary dual-stack clusters, the readiness probe connects via the pod's IPv6 address. The socat backend using TCP4-LISTEN only accepts IPv4 connections, causing the pod to never become ready and the test to time out. Use TCP6-LISTEN when the infrastructure IPFamily is DualStackIPv6Primary. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…usters The headers test deploys a standalone http echo server pod using socat with TCP4-LISTEN. On IPv6-primary clusters, connections arrive over IPv6 so the pod never responds. Read the fixture, replace TCP4-LISTEN with TCP6-LISTEN when the infrastructure IPFamily is DualStackIPv6Primary, and feed the modified config via stdin. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
alebedev87
force-pushed
the
ingress-dual-stack-fix-existing-tests
branch
from
18acff2 to
4fe4482
Compare
openshift-ci-robot
removed
the
verified
|
Re-adding the verified label because the addressed review must not affect the test run. /verified by CI |
openshift-ci-robot
added
the
verified
|
@alebedev87: This PR has been marked as verified by DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
Scheduling required tests: |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alebedev87, davidesalerno The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@alebedev87: all tests passed! 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. |
4 participants
Several router e2e tests fail on dual-stack AWS clusters due to IPv4-only assumptions:
metrics: Disable proxy protocol whenIPFamilyisDualStack, since the installer forcesNLBon AWS dual-stack which doesn't support proxy protocol.h2spec: Add a v6only IPv6 bind line to the standalone test haproxy config on dual-stack clusters.idle: UseTCP6-LISTENfor socat on IPv6-primary clusters so the readiness probe can connect via IPv6.headers: Read the http echo server fixture and replaceTCP4-LISTENwithTCP6-LISTENon IPv6-primary clusters.Example of a failed job: link