NO-JIRA: fix KubeVirtAdvancedMultinetTest IPv6 and apk errors#7860
Conversation
Remove apk commands from configureDNAT since the dnsmasq pod uses a UBI9 image (dnf-based) and already installs iptables in its init script. Filter for IPv4 addresses in firstMachineAddress to prevent passing IPv6 link-local addresses to iptables DNAT rules which only support IPv4. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Enrique Llorente <ellorent@redhat.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
WalkthroughModifies an e2e test to enforce IPv4-only internal address selection (using net.ParseIP and To4), updates error messaging for missing IPv4 internal addresses, and replaces a multi-step iptables/apk setup with a single iptables command invocation. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/nodepool_kv_advanced_multinet_test.go`:
- Around line 277-284: The code currently checks IPv4 via ip.To4() but assigns
the original address.Address which may be an IPv4-mapped IPv6 string; change the
assignment to use the canonical dotted-decimal form from the parsed IP: set
internalAddress = ip.To4().String() (after the ip :=
net.ParseIP(address.Address) and ip.To4() != nil check) so iptables receives a
proper IPv4 address.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c6bfb184-b202-4d6d-a963-f14f23273511
📒 Files selected for processing (1)
test/e2e/nodepool_kv_advanced_multinet_test.go
|
Scheduling tests matching the |
Use ip.To4().String() instead of raw address.Address to ensure dotted-decimal format even when the address is stored as IPv4-mapped IPv6 (::ffff:x.x.x.x), which iptables would reject. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Enrique Llorente <ellorent@redhat.com>
|
/approve |
|
Scheduling tests matching the |
|
/verified by e2e |
|
@jparrill: 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. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jparrill, nunnatsa, qinqon 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 |
|
@qinqon: This pull request explicitly references no jira issue. 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. |
|
/retest-required |
Test Resultse2e-aws
e2e-aks
|
|
/retest-required |
7 similar comments
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
@qinqon: 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. |
What this PR does / why we need it:
Fixes two bugs in
KubeVirtAdvancedMultinetTestthat cause the e2e test to fail:apkon UBI9 image:configureDNAT()ranapk update && apk add iptablesbut the dnsmasq pod usesregistry.access.redhat.com/ubi9/ubi:latest(adnf-based image). Since the pod's init script already runsdnf install -y iptables, theapkcommands are both wrong and unnecessary.IPv6 address passed to
iptables:firstMachineAddress()iterated over allInternalIPaddresses and kept the last one, which could be an IPv6 link-local address (e.g.fe80::...). This address was then used in aniptables(IPv4-only) DNAT rule, causingBad IP addresserrors.Which issue(s) this PR fixes:
Fixes the
KubeVirtNodeAdvancedMultinetTestfailure inpull-ci-openshift-cluster-api-provider-kubevirt-main-e2e-hypershift-kubevirtSpecial notes for your reviewer:
The two fixes are:
apklines fromconfigureDNAT()— iptables is already installed by the pod's init commandfirstMachineAddress()usingnet.ParseIPandip.To4(), and break on first matchChecklist:
Summary by CodeRabbit