Skip to content

OPNET-772: Add NAT64/DNS64 support for IPv6-only clusters on IPv4-only hosts - #1952

Open
emy wants to merge 3 commits into
openshift-metal3:masterfrom
emy:nat64
Open

OPNET-772: Add NAT64/DNS64 support for IPv6-only clusters on IPv4-only hosts#1952
emy wants to merge 3 commits into
openshift-metal3:masterfrom
emy:nat64

Conversation

@emy

@emy emy commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Adds optional NAT64 (TAYGA) + DNS64 (unbound) support so an IPv6-only cluster
(IP_STACK=v6) can be deployed on an IPv4-only host (HOST_IP_STACK=v4). Enable
with ENABLE_NAT64=true; it is a no-op otherwise.

The helpers live in nat64.sh (sourced by 02_configure_host.sh): bring IPv6 up on
the baremetal bridge, configure the TAYGA NAT64 tunnel, run an unbound DNS64
resolver (dns64-synthall, since the host has no native IPv6 egress) and point the
host and every cluster-facing libvirt network at it, and rewrite node BMC
addresses to the IPv6 baremetal address so the IPv6-only in-cluster Ironic can
reach them after pivot. The second commit regenerates the sushy-tools BMC cert
with the IPv6 SAN, required on OCP >= 4.22 where certificate verification is
always on.

01_install_requirements.sh installs tayga/unbound, common.sh validates the
IP_STACK/HOST_IP_STACK requirement, and host_cleanup.sh/ocp_cleanup.sh tear the
configuration down.

emy added 2 commits August 19, 2026 13:55
Allow running an IPv6-only OpenShift cluster (IP_STACK=v6) on an IPv4-only
host (HOST_IP_STACK=v4) by adding NAT64 (TAYGA) and DNS64 (unbound) plumbing,
enabled with ENABLE_NAT64=true.

nat64.sh provides the helpers, sourced by 02_configure_host.sh: bring IPv6 up on
the baremetal bridge, configure the TAYGA NAT64 tunnel, run an unbound DNS64
resolver (dns64-synthall, since the host has no native IPv6 egress) and point the
host and every cluster-facing libvirt network at it, and rewrite node BMC
addresses to the IPv6 baremetal address so the IPv6-only in-cluster Ironic can
reach them after pivot. common.sh validates the IP_STACK=v6 / HOST_IP_STACK=v4
requirement, 01_install_requirements.sh installs tayga and unbound, and
host_cleanup.sh/ocp_cleanup.sh tear the configuration down.
metal3-dev-env only puts the IPv4 baremetal address in the sushy-tools BMC
emulator certificate SAN. With NAT64 the in-cluster Ironic pods are IPv6-only and
reach the BMC over IPv6, and on OCP >= 4.22 dev-scripts no longer emits
disableCertificateVerification, so certificate verification is always on and the
IPv6 redfish connection is rejected with an IP address mismatch.

Add nat64_fixup_sushy_cert (called from 02_configure_host.sh before step 05
embeds the cert into the install-config trust bundle) to regenerate the cert with
both the IPv4 and IPv6 SANs, reusing the existing key, and restart sushy-tools.
Idempotent and a no-op when sushy or the IPv6 address is absent.
@openshift-ci
openshift-ci Bot requested review from elfosardo and tdomnesc August 19, 2026 13:23
@openshift-ci

openshift-ci Bot commented Aug 19, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign bfournie for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci

openshift-ci Bot commented Aug 19, 2026

Copy link
Copy Markdown

Hi @emy. Thanks for your PR.

I'm waiting for a openshift-metal3 member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Tip

We noticed you've done this a few times! Consider joining the org to skip this step and gain /lgtm and other bot rights. We recommend asking approvers on your previous PRs to sponsor you.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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 kubernetes-sigs/prow repository.

@openshift-ci openshift-ci Bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 19, 2026
@mkowalski

Copy link
Copy Markdown
Member

/ok-to-test

@openshift-ci openshift-ci Bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 19, 2026
@mkowalski

Copy link
Copy Markdown
Member

Deep Review — Multi-Specialist Panel

Disposition: REQUEST_CHANGES

Reviewed 28a095e against merge base e2790c1 (8 files, +446/-16). Seven specialists reviewed independently; every BLOCKING claim asserting a runtime bug was handed to a separate reproducer agent for verification. 8 of 8 reproducers ran; 8 confirmed (2 with material corrections that changed severity).

The design is sound and the hard parts — dns64-synthall to force all egress through NAT64, pointing the extra networks' dnsmasq at DNS64, the IPv6 SAN on the sushy cert — are the right calls, and the comments explaining why are genuinely good. The problems are concentrated in lifecycle: setup is not re-runnable, nothing survives a reboot, and cleanup is gated on config rather than on applied state.


Specialist Findings

Functional bugs — 2 blocking, 6 suggestions, 4 notes
  • BLOCKING network.sh:158ENABLE_NAT64 leaves MIRROR_IMAGES empty, so the installer is still pointed at a registry that is never created or populated. Reproduced.
  • BLOCKING nat64.sh:49configure_tayga is not idempotent; tayga --mktun is unguarded. Reproduced.
  • SUGGESTION nat64.sh:177_nat64_point_libvirt_dns64 destroys the baremetal bridge but never re-attaches INT_IF (cf. 02_configure_host.sh:451-453).
  • SUGGESTION nat64.sh:311tayga --rmtun runs before the daemon is killed, so the persistent TUN leaks.
  • SUGGESTION nat64.sh:208 — jq aborts on a node lacking driver_info.address, leaving .nat64 temp files behind.
  • SUGGESTION nat64.sh:275_nat64_remove_legacy_coredns runs rm -rf /etc/coredns on every configure_dns64.
  • SUGGESTION nat64.sh:68 — no systemd unit for TAYGA, so translation does not survive reboot while DNS64 does.
  • SUGGESTION ocp_cleanup.sh:22 — removes the DNS64 drop-in that make redeploy never restores; duplicates the literal instead of using ${NAT64_DNSMASQ_CONF}.
  • NOTE 02_configure_host.sh:487ip6tables rules appended without -C, never removed by cleanup.
  • NOTE network.sh:129NAT64_V6_ADDR defaults inside EXTERNAL_SUBNET_V6, so it is on-link on the bridge rather than reached via the TUN; affects TAYGA-generated ICMPv6 (PMTUD).
  • NOTE 02_configure_host.sh:416 — making EXTERNAL_SUBNET_V6 non-empty on a v4 host silently flips a pre-existing INT_IF bridge branch.
  • NOTE host_cleanup.sh:34 — unrelated revert of the METAL-1845 quoting fixes.
Adversarial — 5 blocking, 3 suggestions
  • BLOCKING nat64.sh:49 — NAT64 setup is not rerunnable (Device or resource busy). Reproduced.
  • BLOCKING nat64.sh:208 — IPMI endpoints rewritten to IPv6 without an IPv6 listener. Reproduced.
  • BLOCKING nat64.sh:239 — certificate idempotence check always misses IPv6. Reproduced.
  • BLOCKING nat64.sh:68 — NAT64 disappears after host reboot. Reproduced.
  • BLOCKING 01_install_requirements.sh:216 — clean EL10 hosts cannot install TAYGA. Reproduced.
  • SUGGESTION nat64.sh:311 — cleanup leaves the persistent TUN device.
  • SUGGESTION nat64.sh:185${net}-dmy can exceed the 15-char interface limit for longer CLUSTER_NAME values; the failure is swallowed by || true.
  • SUGGESTION common.sh:437 — agent scenarios bypass stack validation.
Security — 6 blocking, 3 suggestions (2 downgraded on verification)
  • BLOCKING nat64.sh:275rm -rf /etc/coredns can erase an unrelated host CoreDNS deployment.
  • BLOCKING nat64.sh:312 — global /etc/tayga.conf, fixed interface/data names and a broad pkill can overwrite or stop a pre-existing host TAYGA instance.
  • BLOCKING 01_install_requirements.sh:220 — stock unbound.service is stopped and disabled with no record of prior state and never restored.
  • BLOCKING → downgraded to SUGGESTION 02_configure_host.sh:487 — "IPv6 forwarding bypasses NAT64 isolation". Over-breadth is real, but the isolation-bypass framing was disproven: the FORWARD policy is already accept on a stock host, -A cannot override an earlier DROP, firewalld filters in a separate nft base chain at priority filter + 10, and on a real host the rules sit behind LIBVIRT_FWO and matched 0 packets.
  • BLOCKING → downgraded to SUGGESTION nat64.sh:210 — "BMC credential permissions widened". The mv really does drop the mode (0600 → 0644), but the premise fails: NODES_FILE is already 0644 at birth (metal3-dev-env's template task sets no mode:), so this widens nothing in the default flow.
  • BLOCKING → downgraded to SUGGESTION nat64.sh:90 — silent fallback to 8.8.8.8. Fail-open is worth fixing, but the file's own comment notes NAT64 hosts often cannot reach public resolvers, so this is a broken default more than a leak in a lab tool.
  • SUGGESTION nat64.sh:110module-config: "dns64 iterator" omits the validator; no trust anchor.
  • SUGGESTION nat64.sh:68 — TAYGA parses packets as root with no user/group/chroot.
  • SUGGESTION host_cleanup.sh:42 — quoting removed from privileged cleanup paths.
Architecture — 5 blocking, 1 suggestion
  • BLOCKING nat64.sh:68 — TAYGA has no managed lifecycle; setup, cleanup and reboot all diverge.
  • BLOCKING nat64.sh:178 — undefine/redefine drops libvirt autostart. Reproduced.
  • BLOCKING host_cleanup.sh:38 — cleanup keys off current config rather than applied state.
  • BLOCKING nat64.sh:239 — certificate check is not idempotent. Reproduced.
  • BLOCKING → moved to open question network.sh:190PROVISIONING_HOST_EXTERNAL_IP stays IPv4 under NAT64 (see below).
  • SUGGESTION nat64.sh:279 — host mutations are not symmetrically removed.
Consistency — 3 blocking, 3 suggestions, 1 note
  • BLOCKING host_cleanup.sh:38 — NAT64 teardown depends on current config, unlike the unconditional LLDP/BGP cleanup right beside it.
  • BLOCKING 02_configure_host.sh:487 — unconditional -A while the adjacent NAT64 rule at nat64.sh:64-65 correctly uses -C ... || -A.
  • BLOCKING 01_install_requirements.sh:216 — EL10 branch lacks the repo that ships TAYGA.
  • SUGGESTION nat64.sh:5 — TAYGA state is not instance-scoped.
  • SUGGESTION ocp_cleanup.sh:22 — cluster cleanup partially dismantles host NAT64.
  • SUGGESTION host_cleanup.sh:34 — unrelated quoting regression.
  • NOTE config_example.sh:447 — names the wrong DNS64 implementation.
QA — 5 blocking, 1 suggestion, 1 note
  • BLOCKING common.sh:437 — agent scenarios bypass NAT64 validation.
  • BLOCKING 01_install_requirements.sh:216 — TAYGA unavailable on clean EL10.
  • BLOCKING nat64.sh:49 — TAYGA lifecycle is not idempotent; test setup/setup/cleanup/cleanup.
  • BLOCKING host_cleanup.sh:38 — disabling the flag skips cleanup; test enable → disable → clean.
  • BLOCKING nat64.sh:177 — reboot drops the NAT64 path.
  • SUGGESTION 02_configure_host.sh:553 — no job in the Prow matrix sets ENABLE_NAT64; nothing exercises this code.
  • NOTE nat64.sh:168 — ShellCheck SC2024 (sudo does not apply to the redirect); the Prow shellcheck step passes regardless.
Technical writer — 1 blocking, 3 suggestions (1 finding discarded)
  • BLOCKING config_example.sh:444 — enabling NAT64 disables stock unbound, redirects host and libvirt DNS, enables forwarding, adds routes/rules, rewrites BMC data and replaces the sushy cert; cleanup restores none of that and only runs while ENABLE_NAT64 is still true. Neither the retained changes nor a manual rollback are documented.
  • SUGGESTION config_example.sh:447 — says CoreDNS provides DNS64; the code installs and runs unbound and treats CoreDNS as legacy.
  • SUGGESTION README.md — NAT64 is undiscoverable; no mention of the libvirt/sushy-only scope (NODES_PLATFORM=baremetal exits before all of it), required IPv6 kernel support, or prefix/pool constraints.
  • SUGGESTION config_example.sh:432 — under ENABLE_NAT64 the documented EXTERNAL_SUBNET_V6="" and "IPv6 defaults MIRROR_IMAGES to true" statements are both untrue.
  • Discarded: a reported mismatch for TAYGA_IPV4_ADDR / NAT64_DNS_IPV6 was an artifact of the dispatch prompt, not of this PR. Those names appear nowhere in the diff. config_example.sh:469 correctly documents the real NAT64_V4_ADDR. Not a defect.

Panel Synthesis

Convergence was high: nat64.sh:49 (idempotence), host_cleanup.sh:38 (cleanup gating), 01_install_requirements.sh:216 (EL10) and nat64.sh:68/:239 were each raised independently by three or more specialists.

Two BLOCKING security findings were refuted by their own reproducers and downgraded — worth stating plainly, because both read as serious until tested. Conversely, the single highest-impact finding (MIRROR_IMAGES) came from one specialist and was decisive once replayed against the real sourcing order.


Required Actions

  1. network.sh:158 — a default ENABLE_NAT64=true deploy points the installer at a registry that is never created.
    common.sh:672 runs before network.sh is sourced, and empty != "false", so OPENSHIFT_INSTALL_RELEASE_IMAGE_OVERRIDE is still set to the local registry. The new guard then suppresses MIRROR_IMAGES=true, so 04_setup_ironic.sh:109 skips mirroring and use_registry() (utils.sh:1144) skips starting the registry entirely.
    Reproducer xtrace from the real scripts:

    ++(common.sh:672): [[ '' != \f\a\l\s\e ]]
    ++(common.sh:691): export OPENSHIFT_INSTALL_RELEASE_IMAGE_OVERRIDE=virthost.ostest...:5000/localimages/local-release-image:latest
    ++(network.sh:158): [[ true != \t\r\u\e ]]
    

    Fix: set MIRROR_IMAGES=false explicitly rather than leaving it unset, or move the NAT64 exclusion into common.sh above line 672.

  2. nat64.sh:49 — re-running 02_configure_host.sh aborts.
    sudo tayga --mktun is unguarded under set -euxo pipefail. Run 1 exits 0; run 2 exits 1 with Unable to attach tun device nat64, aborting: Device or resource busy, and never reaches configure_dns64 or nat64_fixup_sushy_cert. cleanup_nat64 only runs from host_cleanup.sh, so nothing tears TAYGA down between make runs.
    Correction to the panel: nat64.sh:68 is not the abort point — tayga daemonizes before tun_setup(), so the parent exits 0 while the child dies in the background. That is a silent defect (misleading exit status), not a fatal one. Both need guards; only :49 breaks the run.

  3. nat64.sh:239 — the sushy cert is rotated on every run, breaking bmcVerifyCA.
    OpenSSL renders IPv6 SANs uncompressed (FD2E:6F44:5DD8:C956:0:0:0:1); $v6host is compressed (fd2e:...::1), so grep -qiF never matches. Case is not the cause — grep -i handles that; zero-run compression is. Proven by control: an address with no zero-run matches. The SAN is genuinely valid throughout (openssl verify -verify_ipOK), yet the fingerprint changed on all 3 consecutive runs, contradicting the "Idempotent" docstring at nat64.sh:224. ocp_install_env.sh:482-483 embeds this exact file into the install-config trust bundle.
    Fix: use openssl x509 -checkip "$v6host".

  4. 01_install_requirements.sh:216 — clean EL10 cannot install TAYGA.
    centos10/rhel10 are supported (:111), but only the EL9 branch installs EPEL (:97, :106); tayga ships only in EPEL. Verified against remote repodata and end-to-end in a clean quay.io/centos/centos:stream10 container: Error: Unable to find a match: tayga, exit 1 — fatal under set -e. unbound is fine on both.
    Correction to the panel: EPEL 10 does ship tayga (0.9.6-...el10_2); adding epel-release to the EL10 branch makes the install succeed.

  5. nat64.sh:68 + nat64.sh:177-180 — NAT64 does not survive a host reboot, and fails silently when it doesn't.
    DNS64 is persistent (unit + WantedBy=multi-user.target, enable --now, dnsmasq drop-in, network XML). The translator is not: bare forked daemon, ip route add, sysctl -w, iptables -A. After a reset, DNS64 still synthesizes AAAA in the prefix while ip -6 route get 64:ff9b::102:304 returns Network is unreachable — DNS answers successfully and traffic blackholes.
    Also: virsh net-undefine clears autostart (runtime-confirmed: yesnet-destroy still yes → after undefine/define no), and metal3-dev-env explicitly sets it (network_setup_tasks.yml:65-70). So the networks stay down after reboot too.
    Note the installed tayga RPM already ships /usr/lib/systemd/system/tayga@.service, which the PR never enables — and it reads /etc/tayga/<instance>.conf, not the /etc/tayga.conf written at nat64.sh:5.

  6. nat64.sh:275rm -rf /etc/coredns runs on every configure_dns64, not just cleanup.
    Invoked from configure_dns64 (nat64.sh:78) as well as cleanup_nat64 (:283). No merged dev-scripts code has ever created coredns-nat64.service or /etc/coredns, so on a real host the only thing this can delete is an unrelated, host-owned CoreDNS directory. Please drop the helper, or scope it to a dev-scripts-owned path and call it only from cleanup.

  7. nat64.sh:208ipmi:// BMCs are rewritten to IPv6 but VirtualBMC only listens on IPv4.
    The rewrite is scheme-agnostic and gated only on ENABLE_NAT64 (02_configure_host.sh:300-302). metal3-dev-env overrides vbmc's own :: default with an IPv4 literal (virtbmc/tasks/setup_tasks.yml:41-53, :90), and pyghmi derives the socket family from it. Verified with real ipmitool against the real vbmc image: bound to IPv4, -H ::1 fails; bound to ::, both succeed. sushy is unaffected because its entrypoint force-adds --interface ::.
    Scope: the non-CI default BMC_DRIVER=redfish is safe. This bites BMC_DRIVER=ipmi, and CI where the default is mixed (common.sh:202) and roughly one node in three gets ipmi. Either reject ipmi/mixed under NAT64 or rewrite the vbmc address to ::.

  8. host_cleanup.sh + ocp_cleanup.sh — unrelated revert of merged commit e741cb5.
    host_cleanup.sh:34-35, 54-55, 67-68, 72, 77-78, 82 and ocp_cleanup.sh:21 drop the quoting added by "METAL-1845: shellcheck fixes part 4 (METAL-1845: shellcheck fixes part 4 #1906)". Confirmed as a verbatim revert by diffing against e741cb5 — almost certainly a bad rebase. It reintroduces word splitting and globbing on ${VM_SETUP_PATH}, ${CLUSTER_NAME}, ${PROVISIONING_NETWORK_NAME}, ${BAREMETAL_NETWORK_NAME} and $interface_config inside rm -f/rm -rf/ip link delete. Please restore and keep the diff to NAT64.

  9. host_cleanup.sh:38 — cleanup is gated on current config, not applied state.
    Raised independently by architecture, consistency and QA. Unset ENABLE_NAT64 before make clean and the unbound service, dnsmasq drop-in, TUN, routes and rules all survive. Worse, changing IP_STACK/HOST_IP_STACK first makes the new common.sh validation exit 1 before cleanup starts. cleanup_nat64 is already absence-tolerant, so calling it unconditionally is the smaller change.

  10. common.sh:436 — agent scenarios bypass the new validation.
    Validation runs at 436-446, but IP_STACK is derived from AGENT_E2E_TEST_SCENARIO at common.sh:488. With IP_STACK unset, ENABLE_NAT64=true + HA_IPV4 passes on the :-v6 default, then configures NAT64 and rewrites BMCs for an IPv4 cluster. Move the check after scenario parsing.


Question for the author

network.sh:190PROVISIONING_HOST_EXTERNAL_IP keys off HOST_IP_STACK, which is v4 under NAT64, so it resolves to the IPv4 baremetal address. Consumers include NTP_SERVERS (utils.sh:126), bootstrapExternalStaticGateway/bootstrapExternalStaticDNS (ocp_install_env.sh:111,115) and the agent DHCP option (agent/05_agent_configure.sh:685). Is handing IPv6-only nodes an IPv4 address there intentional (reached via NAT64), or should NAT64 introduce a distinct cluster-facing address? Not reproduced — flagging for your judgement rather than asserting a bug.


Optional Follow-ups

  • 02_configure_host.sh:487-488 — scope to -i $BAREMETAL -o $NAT64_TUN -s $EXTERNAL_SUBNET_V6 -d $NAT64_PREFIX, guard with -C ... || -A (3 runs currently yield 3 copies), and add matching -D to cleanup. Note these mirror the pre-existing IPv4 rules at :481-482, and on a real host they sit behind LIBVIRT_FWO and match nothing.
  • nat64.sh:311-312 — kill the daemon then --rmtun; pkill -f "^tayga" also misses /usr/sbin/tayga.
  • nat64.sh:5-6, :312 — namespace /etc/tayga.conf, the data dir and the TUN per cluster so cleanup cannot disturb a pre-existing host TAYGA.
  • 01_install_requirements.sh:220 — record and restore the stock unbound.service state instead of disabling it permanently.
  • nat64.sh:90 — fail closed instead of falling back to 8.8.8.8; the comment above it already notes NAT64 hosts often cannot reach it.
  • nat64.sh:206-210 — preserve mode/ownership across the mv (install -m, or chmod --reference). Not exploitable today since NODES_FILE is already 0644, but it silently relaxes a user-supplied NODES_FILE protected at 0600, and fires even when the rewrite matches nothing.
  • nat64.sh:208select(.driver_info.address? != null) so a node without a BMC address doesn't abort step 02 and strand .nat64 temp files.
  • nat64.sh:177 — re-attach INT_IF after net-destroy, mirroring 02_configure_host.sh:451-453.
  • nat64.sh:185${net}-dmy can exceed the 15-char interface limit for longer CLUSTER_NAME values; the || true hides it.
  • nat64.sh:129/network.sh:129NAT64_V6_ADDR defaults on-link on the bridge rather than behind the TUN; also undocumented in config_example.sh.
  • ocp_cleanup.sh:22make redeploy never re-runs 02_configure_host.sh, so this permanently removes DNS64 forwarding; it is also ungated, hardcodes the path instead of ${NAT64_DNSMASQ_CONF}, and skips the NetworkManager reload.
  • config_example.sh:447 — CoreDNS → unbound; document NAT64_V6_ADDR and qualify the EXTERNAL_SUBNET_V6 / MIRROR_IMAGES defaults under NAT64.
  • Add a CI job setting ENABLE_NAT64=true, or record the equivalent manual matrix — nothing currently exercises any of this. Highest-value cases given the above: run 02 twice, reboot, and enable → disable → clean.
  • nat64.sh:110module-config: "dns64 iterator" omits the validator and no trust anchor is initialized.

Stats

Specialists 7 (bugs, adversarial, security, architecture, consistency, QA, writer)
Raw findings 54
After dedupe / filtering 10 required, 1 open question, 14 follow-ups
Reproducers run 8
Confirmed 8 (2 with corrections that lowered severity)
Refuted or discarded 1 security framing, 1 security premise, 1 doc finding

Reproducers ran read-only against 28a095e in throwaway namespaces and containers; the checkout was verified unmodified afterwards.

Generated by /code-review:deep-review. This message was generated using AI. Please verify before acting on it.

@mkowalski mkowalski left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deep Review — inline findings

Line-anchored companion to the full panel verdict. Disposition: REQUEST_CHANGES.

Reviewed 28a095e against merge base e2790c1. Seven specialists reviewed independently; every BLOCKING claim asserting a runtime bug went to a separate reproducer agent. 8 reproducers ran, 8 confirmed — 2 with corrections that lowered severity, noted inline where relevant.

23 inline comments below, grouped by what they block:

  • Breaks a default ENABLE_NAT64=true deploynetwork.sh:158 (installer pointed at a registry that is never created)
  • Breaks re-running 02_configure_host.shnat64.sh:49
  • Breaks an existing cluster's BMC trustnat64.sh:239 (cert rotates every run, invalidating bmcVerifyCA)
  • Breaks on a supported distro01_install_requirements.sh:216 (EL10 has no EPEL, and tayga ships only there)
  • Breaks after reboot, silentlynat64.sh:68 + nat64.sh:177 (DNS64 persists, translator and libvirt autostart do not, so DNS answers succeed while traffic blackholes)
  • Destructive to unrelated host statenat64.sh:275 (rm -rf /etc/coredns on every configure run)
  • Conditional on BMC_DRIVERnat64.sh:194 (ipmi endpoints rewritten to IPv6; vbmc listens on IPv4 only)
  • Cleanup and scopehost_cleanup.sh:37 (teardown gated on current config), common.sh:436 (validation runs before agent scenarios derive IP_STACK), plus an unrelated revert of merged commit e741cb5 in host_cleanup.sh and ocp_cleanup.sh

The rest are suggestions and notes: firewall rule scope/idempotence, cleanup ordering, DNS fallback, docs.

One finding I could not anchor inline

network.sh:190 is outside the diff, so it has no inline comment. PROVISIONING_HOST_EXTERNAL_IP keys off HOST_IP_STACK, which is v4 under NAT64, so it resolves to the IPv4 baremetal address. Consumers include NTP_SERVERS (utils.sh:126), bootstrapExternalStaticGateway/bootstrapExternalStaticDNS (ocp_install_env.sh:111,115) and the agent DHCP option (agent/05_agent_configure.sh:685). Is handing IPv6-only nodes an IPv4 address there intentional — reached via NAT64 — or should NAT64 introduce a distinct cluster-facing address? Not reproduced; flagging for your judgement rather than asserting a bug.

Two claims that did not survive verification

Stating these plainly so they don't cost you time:

  • The ip6tables rules at 02_configure_host.sh:487-488 were initially filed as a NAT64 isolation bypass. That framing is wrong — on a real host the FORWARD policy is already accept, -A cannot override an earlier DROP, firewalld filters in a separate nft base chain at priority filter + 10, and these rules sit behind LIBVIRT_FWO and matched 0 packets. Reduced to hygiene (scope, idempotence, missing cleanup).
  • The BMC temp-file mv was filed as widening credential permissions. The mechanism is real (0600 → 0644) but the premise is not: NODES_FILE is already 0644 at birth from metal3-dev-env's template task. Reduced to defensive hygiene.

Reproducers ran read-only in throwaway namespaces and containers; the checkout was verified unmodified afterwards.

Generated by /code-review:deep-review. This message was generated using AI. Please verify before acting on it.

Comment thread network.sh
Comment on lines +158 to 160
if [[ "${ENABLE_NAT64}" != "true" ]] && [[ ${AGENT_E2E_TEST_BOOT_MODE} != "ISO_NO_REGISTRY" ]]; then
# NAT64 provides external registry access, so mirroring is not required
export MIRROR_IMAGES=${MIRROR_IMAGES:-true}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: ENABLE_NAT64 leaves MIRROR_IMAGES empty, so the installer is pointed at a registry that is never created

This guard stops export MIRROR_IMAGES=${MIRROR_IMAGES:-true} from running, leaving MIRROR_IMAGES as the empty string set in common.sh:80. But common.sh:672 ([[ "${MIRROR_IMAGES,,}" != "false" ]] && [[ ... || "$IP_STACK" = "v6" ]]) is evaluated before network.sh is sourced — every script does source common.sh then source network.sh — and empty is not "false", so OPENSHIFT_INSTALL_RELEASE_IMAGE_OVERRIDE is still exported pointing at the local registry.

Meanwhile 04_setup_ironic.sh:109 ([[ ! -z "${MIRROR_IMAGES}" && ... ]]) is false, so setup_release_mirror and the podman push never run. use_registry() (utils.sh:1144) uses the same ! -z gate, so 02_configure_host.sh:139 never even starts the registry container.

Note the guard is only reachable in the broken case: a user who sets MIRROR_IMAGES=false explicitly already gets the intended behaviour without this change.

Fix: set the value explicitly — export MIRROR_IMAGES=${MIRROR_IMAGES:-false} in the NAT64 branch — or move the NAT64 exclusion into common.sh above line 672.

Reproducer

Steps: Sourced the real common.sh then network.sh from this branch in the real 04_setup_ironic.sh order, with the mirroring gates extracted from the files at runtime via sed so they cannot drift. Ran with ENABLE_NAT64=true/false as the only difference.

Expected: the two decisions agree — either mirroring is off and the override points upstream, or mirroring runs and populates the registry.

Actual: xtrace from the real scripts:

++(common.sh:672): [[ '' != \f\a\l\s\e ]]
++(common.sh:672): [[ v6 = \v\6 ]]
++(common.sh:691): export OPENSHIFT_INSTALL_RELEASE_IMAGE_OVERRIDE=virthost.ostest.test.metalkube.org:5000/localimages/local-release-image:latest
++(network.sh:158): [[ true != \t\r\u\e ]]

MIRROR_IMAGES=[], MIRRORING_BLOCK_RUNS=no, USE_REGISTRY=no, override still .../local-release-image:latest. Control run with ENABLE_NAT64=false is self-consistent (MIRROR_IMAGES=true, mirroring runs, registry started). 7/7 assertions passed.

Comment thread nat64.sh Outdated
Comment on lines +48 to +49
# Create the TUN device via TAYGA
sudo tayga --mktun

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: configure_tayga is not idempotent — re-running 02_configure_host.sh aborts here

Every other command in this function is deliberately re-runnable (mkdir -p, ip route add ... || true, iptables -C ... || iptables -A ...), but sudo tayga --mktun has no guard. 02_configure_host.sh runs with set -euxo pipefail and calls configure_tayga unconditionally at line 556, so any second run — make re-run after a later failure, a very common dev-scripts workflow — aborts here. cleanup_nat64 is only reachable from host_cleanup.sh, so nothing tears TAYGA down in between.

Compare 04_setup_ironic.sh:234, which guards container creation with if ! is_running sushy-tools.

Fix: [[ -d /sys/class/net/${NAT64_TUN_INTERFACE} ]] || sudo tayga --mktun, or better, use the tayga@.service unit the RPM already ships (see the separate comment on line 68).

Reproducer

Steps: Extracted configure_tayga verbatim, ran it twice under the same set -euxo pipefail, inside a throwaway network namespace with the real network.sh defaults. tayga-0.9.2-17.el9 installed from EPEL.

Expected: second run reconfigures and continues on to configure_dns64 and nat64_fixup_sushy_cert.

Actual: run 1 exit 0; run 2 exit 1:

+ sudo tayga --config .../tayga.conf --mktun
Unable to attach tun device nat64, aborting: Device or resource busy

MARKER_REACHED_configure_dns64 never printed — host setup aborts partway through NAT64 setup. Control proves the fix: ip link show nat64 >/dev/null 2>&1 || tayga --mktun exits 0.

Root cause: the daemon started at line 68 holds the TUN queue open, so TUNSETIFF returns EBUSY for any second attach. Boundary condition: --mktun is idempotent when the tun exists but no daemon holds it — the failure requires a live daemon, which is exactly the steady state after a successful run.

Comment thread nat64.sh Outdated
Comment on lines +238 to +242
# Already valid for the IPv6 address? Nothing to do.
if sudo openssl x509 -in "${cert}" -noout -text 2>/dev/null | grep -qiF "${v6host}"; then
echo "sushy BMC cert already valid for ${v6host}"
return 0
fi

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: this idempotence check can never match, so the sushy cert is rotated on every run

OpenSSL renders IPv6 SANs uncompressed (ipaddr_to_asc() emits each group with %X and no :: compression), while $v6host comes from nth_ipansible.utils.nthhost → netaddr, which returns the compressed form. So grep -qiF never matches even when the SAN is present and valid. Case is not the problem — grep -i handles that; zero-run compression is.

This matters because ocp_install_env.sh:482-483 embeds this exact file into the install-config bmcVerifyCA. Rotating it invalidates the trust anchor already baked into an existing install-config or installed cluster, and contradicts the "Idempotent" docstring at line 224.

Fix: sudo openssl x509 -in "${cert}" -noout -checkip "${v6host}" — and ideally add the IPv6 SAN where the cert is originally generated rather than post-processing it.

Reproducer

Steps: Extracted nat64_fixup_sushy_cert (lines 226-266) and nth_ip (network.sh:6-11) verbatim, seeded the metal3-dev-env starting state (key + IPv4-only-SAN cert), then invoked the function 3× with the real defaults.

Expected: run 2 detects the SAN, prints "already valid", leaves the cert byte-identical.

Actual: printed "Regenerating..." every time; serial and SHA-256 changed on all 3 runs.

X509v3 Subject Alternative Name:
    IP Address:192.168.111.1, IP Address:FD2E:6F44:5DD8:C956:0:0:0:1

v6host searched for : fd2e:6f44:5dd8:c956::1
grep -qiF           => NOMATCH

The SAN was valid the whole time: openssl verify -verify_ip fd2e:6f44:5dd8:c956::1OK, and cryptography parses it as containing that exact address.

Root-cause control (proves compression, not case): subnet 2001:db8:1:2:3:4:5:0/120 → host has no zero-run → renders identically → MATCH. The real default → NOMATCH.

Comment on lines +211 to +216
if [[ "${ENABLE_NAT64:-false}" == "true" ]]; then
echo "Installing NAT64 dependencies (TAYGA, unbound)..."
# TAYGA provides the NAT64 translation; unbound provides DNS64 synthesis.
# unbound is used rather than CoreDNS because its built-in dns64 module
# reliably synthesizes AAAA records (the CoreDNS dns64 plugin build did not).
sudo dnf -y install tayga unbound

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: this fails on a clean EL10 host — tayga ships only in EPEL, which the EL10 branch never enables

centos10/rhel10 are an explicitly supported branch (line 111; the catch-all at 119-121 exits 1 for anything else), but only the EL9 branch installs EPEL (line 97 epel-release, line 106 the EPEL-9 RPM). The EL10 branch (111-118) only enables CRB via subscription-manager ... || true. This install sits after the case, so it runs on every branch. Under set -euxo pipefail (line 2) the failure aborts the whole script.

Nothing else enables EPEL first: the only other reference is agent/01_agent_requirements.sh:58 (different script, gated on AGENT_E2E_TEST_BOOT_MODE), and the pinned metal3-dev-env packages_installation role has no epel/tayga references.

Fix: enable EPEL 10 in the EL10 branch — dnf -y install epel-release (CentOS Stream) or the EPEL-10 release RPM (RHEL) — or explicitly reject NAT64 on EL10. unbound is fine; it is in BaseOS/AppStream on both.

Reproducer

Steps: Queried remote repodata with dnf repoquery --repofrompath against mirror.stream.centos.org and dl.fedoraproject.org, then reproduced end-to-end in a clean quay.io/centos/centos:stream10 container using the EL10 branch semantics (CRB on, no EPEL).

Expected: dnf -y install tayga unbound succeeds, as it does on EL9.

Actual:

Error: Unable to find a match: tayga
DNF_EXIT_CODE=1
tayga: NOT FOUND in enabled repos
unbound: FOUND
repo tayga unbound
cs10-BaseOS / AppStream / CRB ABSENT present
epel10 tayga-0.9.6-...el10_2 absent
cs9-BaseOS / AppStream / CRB ABSENT present
epel9 tayga-0.9.2-17.el9 absent

Correction to one panel claim: EPEL 10 does ship tayga, and epel-release + dnf -y install tayga unbound on EL10 returns RC=0. So enabling EPEL is a sufficient fix.

Comment thread nat64.sh Outdated
Comment on lines +67 to +68
# Start TAYGA daemon
sudo tayga

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: TAYGA is started as an unmanaged daemon, so NAT64 does not survive a host reboot — and fails silently when it doesn't

The DNS64 half is fully persistent: unit file with WantedBy=multi-user.target (119-132), systemctl enable --now (135), the NetworkManager drop-in (138-141), and the libvirt dnsmasq option persisted via net-define (179). The translator half is entirely runtime-only: this bare forked daemon, plus ip route add (55), ip -6 route add (58), sysctl -w (21-22, 61) and iptables -A (64-65).

After a reboot unbound comes back and keeps synthesizing AAAA inside ${NAT64_PREFIX}, and the libvirt dnsmasq keeps forwarding to it, but there is no translator and no route — every synthesized address blackholes behind a successful DNS answer. That is the worst failure shape: it looks like DNS works.

Worth knowing: the tayga RPM you install already ships /usr/lib/systemd/system/tayga@.service (ExecStart=/usr/sbin/tayga --pidfile ... -d --config /etc/tayga/%i.conf). It is never enabled here, and it reads /etc/tayga/<instance>.conf — not the /etc/tayga.conf written at line 5. Using it (or an equivalent unit owning the TUN, routes and firewall state) fixes reboot persistence and the idempotence problem on line 49 at once.

Reproducer

Steps: Applied exactly what configure_tayga applies inside a throwaway netns, then destroyed and recreated the namespace — routes, tun devices, net.* sysctls and iptables are all namespaced, so this resets precisely the state a reboot resets. Separately used systemctl --root=<tmpdir> so the host was never touched.

Expected: both halves of the NAT64/DNS64 pair have matching lifetimes.

Actual:

before reset: tun=1 v4route=1 v6route=1 ipv6fwd=1 masq=1
after  reset: tun=0 v4route=0 v6route=0 ipv6fwd=0 masq=0

ip -6 route get 64:ff9b::102:304  ->  RTNETLINK answers: Network is unreachable
ping6 64:ff9b::102:304            ->  connect: Network is unreachable

(64:ff9b::102:304 is exactly what dns64-synthall synthesizes for an A record.)

systemctl --root enable unbound-dns64.service → creates multi-user.target.wants/ symlink, disabledenabled. is-enabled tayga.serviceNo such file or directory. 6/6 checks passed.

The end-to-end post-reboot blackhole was not executed (no reboot performed); it follows by composition of the runtime-verified facts above.

Comment thread nat64.sh Outdated
# net-destroy drops the bridge; restore a dummy for carrier and addr_gen_mode=0
# so the network's IPv6 address comes up before the VMs provide carrier (needed
# for IPv6 on EL9).
sudo ip link add name "${net}-dmy" up master "${net}" type dummy 2>/dev/null || true

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: ${net}-dmy can exceed the 15-character interface name limit

With the default CLUSTER_NAME=ostest the name is ostestbm-dmy (12 chars) and this is fine. A CLUSTER_NAME of 12+ characters pushes ${net}-dmy past Linux's 15-char IFNAMSIZ limit; || true then hides the failure and the recreated IPv6 bridge is left without carrier before the VMs boot — which is the exact condition this line exists to prevent.

Fix: derive a fixed or truncated dummy name bounded to 15 characters, and don't swallow the failure.

Comment thread nat64.sh Outdated
Comment on lines +206 to +210
tmp="${f}.nat64"
# Literal (non-regex) host replacement via split/join on the "//host:" token.
jq --arg old "//${v4host}:" --arg new "//[${v6host}]:" \
'(.nodes[]?.driver_info.address) |= (. / $old | join($new))' \
"${f}" > "${tmp}" && mv "${tmp}" "${f}"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: preserve file mode across the mv, and don't abort on a node without a BMC address

Two small robustness issues in this loop:

  1. Mode is not preserved. The temp file is created with the current umask and mv (rename) carries its mode over the destination. I measured 0600 → 0644 under umask 0022, and it fires even when the rewrite matches nothing. To be clear about impact: this is not exploitable in the default flow — metal3-dev-env's template task sets no mode:, so NODES_FILE is already 0644 at birth and WORKING_DIR is chmod 755 (common.sh:355). It matters only for a user-supplied NODES_FILE (config_example.sh:546) deliberately protected at 0600 with real BMC credentials. Cheap defensive hygiene: install -m "$(stat -c %a "$f")" or chmod --reference="$f" before the mv.

  2. jq aborts on a missing address. (.nodes[]?.driver_info.address) |= (. / $old | join($new)) applies to a null when the path is absent: jq: error ... null (null) and string cannot be divided, exit 5. The && mv means the loop continues but leaves ${f}.nat64 behind, and the non-zero return then aborts 02_configure_host.sh at line 301. Stock generated files always have the key, so this only bites user-supplied inventories. Fix: select(.driver_info.address? != null) and rm -f the temp on failure.

Comment thread ocp_cleanup.sh Outdated

sudo rm -rf "/etc/NetworkManager/dnsmasq.d/openshift-${CLUSTER_NAME}.conf"
sudo rm -rf /etc/NetworkManager/dnsmasq.d/openshift-${CLUSTER_NAME}.conf
sudo rm -f /etc/NetworkManager/dnsmasq.d/nat64-dns64.conf

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: make redeploy never restores this, and the constant is now duplicated

make redeploy is ocp_cleanup ironic_cleanup build_installer ironic install_config ocp_run02_configure_host.sh, the only writer of ${NAT64_DNSMASQ_CONF}, is not re-run. So the first redeploy of a NAT64 environment permanently removes host DNS64 forwarding.

Three smaller points: this is ungated (it runs even when ENABLE_NAT64 is false), it hardcodes the literal instead of using ${NAT64_DNSMASQ_CONF} (ocp_cleanup.sh does not source nat64.sh, so the constant now lives in two files and can drift), and unlike cleanup_nat64 it does not reload NetworkManager, so the running dnsmasq keeps the deleted config until some unrelated later reload.

Fix: drop this line and leave host-level NAT64 state to cleanup_nat64, or source nat64.sh, gate it, use the variable, and reload NetworkManager.

Comment thread config_example.sh Outdated
Comment on lines +444 to +447
# ENABLE_NAT64 -
# Enable NAT64/DNS64 to allow IPv6-only clusters (IP_STACK=v6) to run on
# IPv4-only hosts (HOST_IP_STACK=v4). Uses TAYGA for NAT64 translation and
# CoreDNS for DNS64 synthesis, enabling cluster VMs to reach external IPv4

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: this names the wrong DNS64 implementation

The implementation installs and runs unbound (01_install_requirements.sh:216, nat64.sh:99-135) and treats CoreDNS as legacy to be removed (nat64.sh:269-277). Anyone following this comment will inspect the wrong service when troubleshooting.

While here, three other doc gaps worth closing in the same block:

  • NAT64_V6_ADDR (network.sh:128-129) is a supported override but is not documented.
  • Line 432 says EXTERNAL_SUBNET_V6 is empty for HOST_IP_STACK=v4; under NAT64 it defaults to fd2e:6f44:5dd8:c956::/120.
  • The later claim that IPv6 defaults MIRROR_IMAGES to true is also not true under NAT64.

Most importantly, enabling this disables any existing unbound.service, redirects host and libvirt DNS, enables forwarding, adds routes and firewall rules, rewrites BMC data and replaces the sushy certificate — and cleanup restores none of that and only runs while ENABLE_NAT64 is still true. That is worth stating here explicitly.

Comment thread network.sh
Comment on lines +128 to +129
if [[ "${ENABLE_NAT64}" == "true" ]]; then
export NAT64_V6_ADDR=${NAT64_V6_ADDR:-$(nth_ip "$EXTERNAL_SUBNET_V6" 3)}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: NAT64_V6_ADDR defaults to an address that is on-link on the baremetal bridge

nth_ip "$EXTERNAL_SUBNET_V6" 3 is fd2e:6f44:5dd8:c956::3 with the default subnet — inside the /120 libvirt puts on the baremetal bridge. The host therefore has an on-link route for it via ${BAREMETAL_NETWORK_NAME} rather than via the nat64 tun. TAYGA uses this address as the source of the ICMPv6 errors it generates (unreachable, packet-too-big/PMTUD), so those replies and neighbour resolution get directed at the bridge where nothing answers.

The index itself is free (::1 virthost, ::2 DNS VIP, ::4 ingress, ::5 api, ::7/::8 provisioning, ::9 bootstrap, ::20-::60 DHCP), so this is about routing, not collision. Worth a deliberate default outside any locally-attached subnet, plus documenting the knob in config_example.sh.

…cert + BMC rewrite, idempotent firewall/cleanup, and config validation
@openshift-ci

openshift-ci Bot commented Aug 20, 2026

Copy link
Copy Markdown

@emy: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-agent-sno-ipv6 29cd165 link false /test e2e-agent-sno-ipv6
ci/prow/e2e-agent-compact-ipv4 29cd165 link true /test e2e-agent-compact-ipv4
ci/prow/e2e-agent-4control-ipv4 29cd165 link false /test e2e-agent-4control-ipv4
ci/prow/e2e-agent-5control-ipv4 29cd165 link false /test e2e-agent-5control-ipv4
ci/prow/e2e-metal-ipi-ovn-ipv6 29cd165 link true /test e2e-metal-ipi-ovn-ipv6
ci/prow/e2e-agent-compact-ipv4-iso-no-registry 29cd165 link false /test e2e-agent-compact-ipv4-iso-no-registry
ci/prow/e2e-metal-ipi-virtualmedia 29cd165 link false /test e2e-metal-ipi-virtualmedia

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-to-test Indicates a non-member PR verified by an org member that is safe to test.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants