Conversation
byteocean
changed the title
c332dda to
6088d27
Compare
|
Hi @mshych , I am wondering if it is possible to get a review on this PR? thanks. |
|
Thanks for the work on this — DHCPv6 installer discovery is a feature ONIE needs, and the RFC 5970 option decoding itself looks correct. I test-applied the busybox patch to 1.25.1 and it applies cleanly and compiles. However, the PR needs careful verification and rework before it can be merged. Functional regression config_ethmgmt() uses a short-circuit chain: static || dhcp6 || dhcp4 || fallback. That was safe only because config_ethmgmt_dhcp6() always returned 1. Now that it can succeed, any dual-stack network with a DHCPv6 server will leave the switch with an IPv6 address and no IPv4 address and no IPv4 link-local fallback. sd_dhcp4() still collects DHCPv4 options, but every resulting IPv4 fetch will fail. This silently breaks working IPv4 provisioning the moment DHCPv6 appears on the same VLAN. The two families need to be configured independently. Debug and dead code The busybox patch ships bb_error_msg("DBG: nxt=%d vfc=%d ...") immediately after the existing "unrelated/bogus packet, ignoring" branch, so it will spam stderr for every stray IPv6 packet, plus two log1() traces in option_to_env(). exec_installer gains a log_debug_msg "TFTP bootfiles: ...". udhcpc6_args() reuses the DHCPv4 string -V onie_vendor:${onie_platform}. DHCPv4 option 60 is free-form, but d6_alloc_vendor_option() parses a numeric enterprise number before the colon, so strtoul() fails and option 16 goes out with enterprise number 0. $onie_iana_enterprise (42623) already exists in functions and is what the DHCPv4 option-125 path uses. Unrelated changes Per CONTRIBUTING.md, each patch should contain one logical change. Currently mixed in, and unmentioned in the commit messages:
"bin/exec_installer: handle IPv6 installer URLs" — there is no IPv6 URL handling in the diff. "lib/onie/functions: supporting helpers for IPv6 network config" — those are DHCPv6 option encoders. contrib/dhcp6-emulation/ (1,031 of 1,860 added lines) This duplicates emulation/onie-vm.sh and contrib/build-env/Dockerfile, and it's built on Debian 9 via archive.debian.org. More concerning, kernel-config.patch disables CONFIG_MODULE_SIG*, CONFIG_SIGNED_PE_FILE_VERIFICATION and the trusted keyring, and machine.make.patch flips SECURE_BOOT_ENABLE/EXT and SECURE_GRUB to no. Checking in a script that disables Secure Boot on kvm_x86_64 invites accidents. That patch also adds CONFIG_IPV6_AUTOCONF=y, which is not a Linux Kconfig symbol — autoconf is the net.ipv6.conf.*.autoconf sysctl, and the symbol appears in none of the 14 configs under build-config/conf/kernel/. So either the override is unnecessary, or there's a real gap that should be fixed in ONIE proper. Which is it? Smaller items No patch header on udhcp6-additional-options.patch; every other patch in patches/busybox/ has a description, copyright, and SPDX line. udhcp6_net and udhcp6_sd also lack SPDX headers, unlike the udhcp4_* files they're based on. busybox DHCPv6 bug fixes Given the amount of debug output, dead code, and unrelated edits, I'd ask that the series be reviewed carefully end-to-end before the next submission — several of these would have been caught by a read-through of the final diff. |
|
@mshych thanks for taking time reviewing this PR, and your valuable comments. To address the points in your comments, in which way would you prefer to see the changes? should I keep the existing three commits and add extra commits addressing above points, or overwrite three commits with another set of commits containing existing changes and amendments? |
9c2fcf4 to
af493d6
Compare
Add patches/busybox/udhcp6-bugfixes.patch with seven independent correctness fixes to the busybox 1.25.1 DHCPv6 client: 1. d6_find_option: advance both the option pointer and the remaining length by (option len + 4); previously the length budget was never reduced correctly, allowing walking past the option area. 2. Bound option parsing by the actual received packet length instead of sizeof(packet->d6_options): thread packet_len through d6_run_script() into fill_envp(); NULL-terminate the env array. 3. send_d6_renew: send D6_MSG_RENEW, not DHCPREQUEST. 4. NAK handling: test the 16-bit status code (data[0] | data[1]) instead of the unrelated byte data[4]. 5. IA_NA: copy and validate the new option before freeing the old one; previously a missing IA_NA left a freed pointer in client state. 6. Packet transmission per RFC 2464: use the IPv6 multicast Ethernet destination 33:33:00:01:00:02 and a link-local source address. Note: with a link-local source the interface must have completed DAD; callers should wait for the address to leave tentative state. 7. option_to_env: add the missing break after the D6_OPT_IAPREFIX case; latent until further cases follow it in the switch. Signed-off-by: Tao Li <tao.li06@sap.com>
Add patches/busybox/udhcp6-rfc5970-boot-options.patch: * RFC 5970 network boot options: BOOT_FILE_URL (59, exported as bootfile=), BOOT_FILE_PARAM (60, bootfile_param=), CLIENT_ARCH_TYPE (61, client_arch_type=) and NII (62, nii=). BOOT_FILE_PARAM also tolerates the dnsmasq length-prefix quirk. * Common options: DNS_SERVERS (23, dns=), DOMAIN_LIST (24, search=), CLIENT_FQDN (39, hostname=). The latter two require and are guarded by ENABLE_FEATURE_UDHCP_RFC3397 (dname_dec). * VENDOR_CLASS/VENDOR_OPTS decoding (vendor_class_id/_data, vendor_specific_id/_data, vendoropts=). * -V/--vendorclass CLI option: DHCPv6 option 16 with a numeric enterprise number; format '<enterprise>:<string>'. * -O requested options are now actually sent: add_d6_client_options builds the ORO from the opt_mask and appends -x options, with bounds checks against the on-stack packet buffer. * A caller-supplied client-id (-x 1:...) overrides the generated DUID, allowing a DUID-EN to be sent. Known limitation (pre-existing): option_to_env() stops parsing at any option whose code or length exceeds 255; a BOOT_FILE_URL longer than 255 bytes terminates parsing of the remaining options. Signed-off-by: Tao Li <tao.li06@sap.com>
Add DHCPv6 installer discovery alongside the existing DHCPv4 path, so a switch can obtain its ONIE installer URL from a DHCPv6 server via the RFC 5970 Boot File URL option (59). * bin/discover: implement sd_dhcp6(); udhcpc6 requests option 59 and the result feeds the installer discovery loop. The DHCPv6 Boot File URL surfaces as onie_disco_bootfile and is consumed by the existing bootfile handling in exec_installer (no changes needed there). * etc/init.d/networking.sh: implement config_ethmgmt_dhcp6(). DHCPv6 now runs in addition to, never instead of, the IPv4 configuration: on dual-stack networks both address families are configured. Before soliciting, wait for the link-local address to leave tentative state (DAD) by polling, mirroring neigh_discovery(); busybox udhcpc6 now sends with a link-local source address and the IPv6 multicast MAC. * lib/onie/udhcp6_net: reactor script; installs the leased address (busybox exports no prefix, so it is a /128 host address and the installer server must be on-link), sets hostname from option 39, and maintains a marked IPv6 block in resolv.conf so renewals replace rather than duplicate entries and IPv4 entries stay intact. * lib/onie/udhcp6_sd: service discovery script; maps the exported option variables (bootfile -> onie_disco_bootfile) via the generic printenv pipeline, same as udhcp4_sd. * lib/onie/functions: DHCPv6 option encoders for udhcpc6_args(): DUID-EN client-id, user class, vendor class (option 16 with ONIE's IANA enterprise number 42623, matching the DHCPv4 option 125 usage), vendor specific information (option 17, sub-option codes 3/4/5 aligned with DHCPv4 option 125), plus a wait_dad() helper. Both DHCPv6 call sites degrade gracefully on the machines that ship their own older copy of /lib/onie/functions. * Remove the top-level TODO file (its 'DHCP v6' entry is resolved by this commit). Signed-off-by: Tao Li <tao.li06@sap.com>
3c0abc6 to
1b330b9
Compare
|
@mshych thanks again for reviewing this PR. the commits are now rewritten along your suggested split. Force-pushed as 4 commits with commit messages rewritten to match each diff:
Validation:
|
1b330b9 to
a912503
Compare
Reproducible, hardware-free validation of the DHCPv6 (RFC 5970)
installer-discovery path on kvm_x86_64:
* setup-dhcp6-test.sh: Linux netns + bridge + tap with dnsmasq
serving option 59 (Boot File URL) and an HTTP server hosting the
installer. QEMU user-mode networking cannot serve DHCPv6 option 59,
hence the real L2 segment. Default payload dir is /tmp/payload;
the setup fails fast if payload/onie-installer is missing. Two
host-kernel workarounds are applied at bring-up:
- bridge multicast snooping is disabled on br-onie (no querier on
this private segment; otherwise IPv6 solicited-node multicast is
silently dropped and global-scope ND fails).
- IPv6 DAD is disabled on the netns-side veth (accept_dad=0,
dad_transmits=0, plus 'nodad' on the address itself) so python's
bind() does not race the ~1s tentative window and fail with
EADDRNOTAVAIL.
* run-onie-vm.sh: direct -kernel/-initrd QEMU boot on the tap, with
boot_reason=install (the same cmdline install.ipxe uses). No ISO,
UEFI or signing is involved in this path. Optional --with-disk
[PATH] flag creates a fresh qcow2 (default
/tmp/onie-disk-<MACHINE>.qcow2, size DISK_SIZE, default 4G),
partitions it with GRUB-BOOT (ef02, 2 MiB) and ONIE-BOOT (ext4,
128 MiB) via qemu-nbd + sgdisk, populates ONIE-BOOT with the
minimal grub scaffolding the demo installer needs (50_onie_grub
from installer/grub-arch, plus placeholder grub-variables /
grub-machine.cfg / grub-extra.cfg / grub-common.cfg), then attaches
the disk so the demo installer can complete install-to-disk in
addition to the DHCPv6 fetch. ANY EXISTING FILE AT PATH IS
DELETED. Bootstrap requires the script to run as root
(qemu-nbd, modprobe, mount). Without the flag the harness boots
diskless (fetch + checksum only, sufficient to validate the DHCPv6
discovery path itself).
* verify-dhcp6-boot.sh: PASS/FAIL evidence gate over the dnsmasq log
(option 59 sent), the HTTP log (installer fetched) and optionally
the captured ONIE console.
* test-udhcp6-sd.sh: hermetic micro-test proving udhcp6_sd maps
option 59 to onie_disco_bootfile; no network or VM needed.
The build uses the stock container environment (contrib/build-env).
Signed-off-by: Tao Li <tao.li06@sap.com>
a912503 to
f924523
Compare
This branch has not been deployed
2 participants
Summary
Adds DHCPv6 installer discovery to ONIE so a switch can obtain its ONIE installer URL from a DHCPv6 server via Boot File URL (RFC 5970, option 59), in addition to the existing DHCPv4 path. This enables automated ONIE provisioning
on IPv6-only or dual-stack networks.
Changes
patches/busybox/udhcp6-additional-options.patch
Extends the udhcpc6 DHCPv6 client to request and parse RFC 5970 network-boot options:
ONIE rootfs scripts
Verification
KVM emulation (kvm_x86_64)
A self-contained verification harness is included in contrib/dhcp6-emulation/. It provides a reproducible, hardware-free procedure to build the patched kvm_x86_64 image and confirm the DHCPv6 boot path end-to-end. See
contrib/dhcp6-emulation/README.md for the full run book.
Validated with a libvirt VM attached to a dnsmasq DHCPv6 server via a Linux network namespace + bridge + tap, and following observable results serve as the signals of success.
Hardware (accton_as7726_32x — Edgecore AS7726-32X)
The DHCPv6 installer discovery flow was also validated on a physical Edgecore AS7726-32X switch running the accton_as7726_32x ONIE build. The patched ONIE kernel and initrd were deployed to the switch and the full DHCPv6 boot path — DHCPv6 address assignment, option-59 URL discovery, and installer fetch — was confirmed working on real hardware, a SONiC image can be successfully installed onto the switch.
Notes
This implementation was developed with the assistance of AI. The code, patches, and verification harness were tested, and validated by the author.