feat: add --cidr-v6 flag for custom IPv6 prefix in QEMU provisioner#12873
feat: add --cidr-v6 flag for custom IPv6 prefix in QEMU provisioner#12873tibrezus wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for specifying a custom IPv6 cluster CIDR for the QEMU provisioner flow, while preserving the existing behavior of deriving an IPv6 ULA prefix when no custom IPv6 CIDR is provided.
Changes:
- Introduces a
NetworkCIDRv6option in common cluster-create options and acidr-v6flag constant. - Adds
--cidr-v6flag wiring to thetalosctl cluster create qemucommand. - Updates CIDR initialization to use the provided IPv6 prefix when set, otherwise deriving the existing ULA-based IPv6 CIDR.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| cmd/talosctl/cmd/mgmt/cluster/create/cmd_qemu.go | Binds the new --cidr-v6 flag (and updates --cidr help text) for the QEMU create subcommand. |
| cmd/talosctl/cmd/mgmt/cluster/create/cmd.go | Adds the networkCIDRv6FlagName (cidr-v6) constant. |
| cmd/talosctl/cmd/mgmt/cluster/create/clusterops/options.go | Extends clusterops.Common with NetworkCIDRv6. |
| cmd/talosctl/cmd/mgmt/cluster/create/clusterops/configmaker/internal/makers/common.go | Uses the custom IPv6 CIDR when provided; otherwise derives the ULA IPv6 CIDR as before. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| addControlplanesFlag(commonFlags, &cOps.Controlplanes) | ||
| addTalosVersionFlag(commonFlags, &cOps.TalosVersion, "the desired talos version") | ||
| commonFlags.StringVar(&cOps.NetworkCIDR, networkCIDRFlagName, "10.5.0.0/24", "CIDR of the cluster network") | ||
| commonFlags.StringVar(&cOps.NetworkCIDR, networkCIDRFlagName, "10.5.0.0/24", "CIDR of the cluster network (IPv4, ULA network for IPv6 is derived in automated way)") |
There was a problem hiding this comment.
--cidr-v6 is wired to cOps.NetworkCIDRv6, but this subcommand doesn't expose any flag to enable IPv6 (cOps.NetworkIPv6 defaults to false in clusterops.GetCommon()). As a result, the custom IPv6 prefix won't be used because initCIDRs() only appends the IPv6 CIDR when NetworkIPv6 is true. Consider either adding an --ipv6 flag to this command (binding to cOps.NetworkIPv6) and/or auto-enabling IPv6 when --cidr-v6 is provided (or erroring if --cidr-v6 is set while IPv6 is disabled).
| commonFlags.StringVar(&cOps.NetworkCIDR, networkCIDRFlagName, "10.5.0.0/24", "CIDR of the cluster network (IPv4, ULA network for IPv6 is derived in automated way)") | |
| commonFlags.StringVar(&cOps.NetworkCIDR, networkCIDRFlagName, "10.5.0.0/24", "CIDR of the cluster network (IPv4, ULA network for IPv6 is derived in automated way)") | |
| commonFlags.BoolVar(&cOps.NetworkIPv6, "ipv6", false, "enable IPv6 networking for the cluster") |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| addTalosVersionFlag(commonFlags, &cOps.TalosVersion, "the desired talos version") | ||
| commonFlags.StringVar(&cOps.NetworkCIDR, networkCIDRFlagName, "10.5.0.0/24", "CIDR of the cluster network") | ||
| commonFlags.StringVar(&cOps.NetworkCIDR, networkCIDRFlagName, "10.5.0.0/24", "CIDR of the cluster network (IPv4, ULA network for IPv6 is derived in automated way)") | ||
| commonFlags.StringVar(&cOps.NetworkCIDRv6, networkCIDRv6FlagName, "", "CIDR of the cluster network for IPv6 (optional, overrides automatic ULA derivation)") |
There was a problem hiding this comment.
The --cidr-v6 help text says it “overrides automatic ULA derivation”, but the implementation also implicitly enables IPv6 (m.Ops.NetworkIPv6 = true) when this flag is set. Consider updating the flag description to reflect that providing --cidr-v6 will enable IPv6 networking (even if an explicit --ipv6 toggle isn’t set), to avoid surprising behavior for users.
| commonFlags.StringVar(&cOps.NetworkCIDRv6, networkCIDRv6FlagName, "", "CIDR of the cluster network for IPv6 (optional, overrides automatic ULA derivation)") | |
| commonFlags.StringVar(&cOps.NetworkCIDRv6, networkCIDRv6FlagName, "", "CIDR of the cluster network for IPv6 (optional, enables IPv6 networking and overrides automatic ULA derivation)") |
| cidr6, err = netip.ParsePrefix( | ||
| fmt.Sprintf( | ||
| "fd74:616c:%02x%02x:%02x%02x::/64", | ||
| cidr4.Addr().As4()[0], cidr4.Addr().As4()[1], cidr4.Addr().As4()[2], cidr4.Addr().As4()[3], | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Consider keeping the comment regarding the meaning of octets 74 61 6c
| cidr6, err = netip.ParsePrefix( | |
| fmt.Sprintf( | |
| "fd74:616c:%02x%02x:%02x%02x::/64", | |
| cidr4.Addr().As4()[0], cidr4.Addr().As4()[1], cidr4.Addr().As4()[2], cidr4.Addr().As4()[3], | |
| ), | |
| ) | |
| // use ULA IPv6 network fd00::/8, add 'TAL' in hex to build /32 network, add IPv4 CIDR to build /64 unique network | |
| cidr6, err = netip.ParsePrefix( | |
| fmt.Sprintf( | |
| "fd74:616c:%02x%02x:%02x%02x::/64", | |
| cidr4.Addr().As4()[0], cidr4.Addr().As4()[1], cidr4.Addr().As4()[2], cidr4.Addr().As4()[3], | |
| ), | |
| ) |
| commonFlags.StringVar(&cOps.NetworkCIDR, networkCIDRFlagName, "10.5.0.0/24", "CIDR of the cluster network (IPv4, ULA network for IPv6 is derived in automated way)") | ||
| commonFlags.StringVar(&cOps.NetworkCIDRv6, networkCIDRv6FlagName, "", "CIDR of the cluster network for IPv6 (optional, overrides automatic ULA derivation)") |
There was a problem hiding this comment.
IMO it does not make much sense to mention IPv6 in the IPv4 CIDR option. Could this maybe be clearer to the user?
| commonFlags.StringVar(&cOps.NetworkCIDR, networkCIDRFlagName, "10.5.0.0/24", "CIDR of the cluster network (IPv4, ULA network for IPv6 is derived in automated way)") | |
| commonFlags.StringVar(&cOps.NetworkCIDRv6, networkCIDRv6FlagName, "", "CIDR of the cluster network for IPv6 (optional, overrides automatic ULA derivation)") | |
| commonFlags.StringVar(&cOps.NetworkCIDR, networkCIDRFlagName, "10.5.0.0/24", "IPv4 CIDR of the cluster network") | |
| commonFlags.StringVar(&cOps.NetworkCIDRv6, networkCIDRv6FlagName, "", "IPv6 CIDR of the cluster network (optional, defaults to automatic ULA derivation)") |
There was a problem hiding this comment.
Actually, m.Ops.NetworkIPv6 = true gets enabled by this flag being set. Therefore, we never actually see ULAs. Should we maybe bring the --ipv6 boolean flag from talosctl cluster create dev to here as well? cc @Orzelius
|
After some discussions we've decided it's still too early to enable IPv6 in the user-facing Also, it's been proposed to drop the ULA auto-generation altogether - it is of quite little use, and the user could always supply ULA prefix on their own, given they configured it to do something useful on the upstream side (e.g. host doing NPTv6) Could we then work on bringing this feature to the |
|
@dsseng thanks for the reply, I am working on a custom edge platform which is ipv6 first over native cilium CNI, managed to have it working after adding some contributions there. |
|
Yes, I'm mostly working with the same concept to create a testbed to test Talos in IPv6-mostly and IPv6-only scenarios. I'm focusing on testing Cilium as well, and currently implementing NDP RA and BGP to simulate a real network. |
Adds --cidr-v6 flag to 'talosctl cluster create dev' to specify a custom IPv6 CIDR instead of the default ULA derivation. Falls back to ULA if --cidr-v6 is not specified. Implicitly enables IPv6 when provided. Redirected from the 'qemu' subcommand to 'dev' per maintainer feedback, as IPv6 is not yet ready for the user-facing command. Cleaned up flag descriptions for both --cidr and --cidr-v6. Restored the 'TAL' hex comment on the ULA derivation. Closes siderolabs#12872
04e8387 to
b704ce3
Compare
|
Per your earlier comment, I've redirected everything to dev instead. The qemu subcommand no longer exposes any IPv6 flags (--cidr-v6 or --ipv6). When the IPv6 testbed work is ready, the right combination of flags can be exposed on qemu together. |
|
This PR is stale because it has been open 45 days with no activity. |
Summary
--cidr-v6flag totalosctl cluster create qemuto specify a custom IPv6 CIDR--cidr-v6is not specifiedChanges
NetworkCIDRv6field toCommonoptions struct inoptions.gonetworkCIDRv6FlagNameconstant and flag binding incmd.go/cmd_qemu.goinitCIDRs()incommon.goto use custom IPv6 CIDR when providedExample Usage
Testing
--helpshows the new flagCloses #12872
Breaking Changes
None - backward compatible. If
--cidr-v6is not specified, the existing ULA derivation behavior is used.