Conversation
…eparate pr to address the metrics issue.
There was a problem hiding this comment.
Pull request overview
This PR extends NodeNetworkConfig → CNS request conversion to carry an IPv6 subnet alongside the existing IPv4 subnet, enabling dual-stack network container provisioning paths (including Windows).
Changes:
- Add
IPSubnetV6tocns.IPConfiguration. - Populate
IPSubnetV6during dynamic/static NC →CreateNetworkContainerRequestconversion (Linux + Windows helpers). - Add unit tests covering IPv6 subnet parsing for both dynamic and static conversion.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| cns/kubecontroller/nodenetworkconfig/conversion.go | Builds IPConfiguration with optional IPv6 subnet and passes IPv6 subnet into static helper. |
| cns/kubecontroller/nodenetworkconfig/conversion_linux.go | Extends static helper signature and writes IPSubnetV6 into request. |
| cns/kubecontroller/nodenetworkconfig/conversion_windows.go | Extends static helper signature and writes IPSubnetV6 into request. |
| cns/kubecontroller/nodenetworkconfig/conversion_test.go | Adds tests for IPv6 subnet parsing for dynamic/static conversions. |
| cns/NetworkContainerContract.go | Adds IPSubnetV6 field to IPConfiguration contract struct. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ipConfig := cns.IPConfiguration{ | ||
| IPSubnet: subnet, | ||
| GatewayIPAddress: nc.DefaultGateway, | ||
| } |
There was a problem hiding this comment.
CreateNCRequestFromDynamicNC populates IPSubnetV6 when SubnetAddressSpaceV6 is set, but never sets GatewayIPv6Address on the request IPConfiguration (it’s not in the initial ipConfig literal and not assigned in the IPv6 block). This means callers providing DefaultGatewayV6 won’t have it propagated, breaking IPv6 routing for dynamic NCs. Set ipConfig.GatewayIPv6Address from nc.DefaultGatewayV6 (likely gated the same way as SubnetAddressSpaceV6).
| if nc.SubnetAddressSpaceV6 != "" { | ||
| subnetV6Prefix, err := netip.ParsePrefix(nc.SubnetAddressSpaceV6) | ||
| if err != nil { | ||
| return nil, errors.Wrapf(err, "invalid SubnetAddressSpaceV6 %s", nc.SubnetAddressSpaceV6) | ||
| } | ||
| ipConfig.IPSubnetV6 = cns.IPSubnet{ | ||
| IPAddress: nc.PrimaryIPV6, | ||
| PrefixLength: uint8(subnetV6Prefix.Bits()), //#nosec G115 -- prefix bits are 0-128, fits uint8 | ||
| } | ||
| } |
There was a problem hiding this comment.
The IPv6 branch accepts any syntactically valid CIDR in SubnetAddressSpaceV6 (including IPv4 CIDRs) and copies PrimaryIPV6 into IPSubnetV6 without validating it’s a real IPv6 address or even non-empty. This can produce an invalid CreateNetworkContainerRequest that won’t be caught by CNS validation (Validate currently only checks GatewayIPAddress). Consider: (1) require subnetV6Prefix.Addr().Is6(), (2) netip.ParseAddr on nc.PrimaryIPV6 and ensure Is6(), and (3) return a clear error if PrimaryIPV6 is missing/invalid when SubnetAddressSpaceV6 is set.
| subnetV6 = cns.IPSubnet{ | ||
| IPAddress: nc.PrimaryIPV6, |
There was a problem hiding this comment.
Static NC IPv6 handling has the same issue as dynamic: SubnetAddressSpaceV6 is parsed but not checked to be IPv6, and PrimaryIPV6 is copied into subnetV6 without validating it’s present/valid. If SubnetAddressSpaceV6 is accidentally set to an IPv4 CIDR or PrimaryIPV6 is empty/invalid, the request will contain a malformed IPSubnetV6. Add Is6()/ParseAddr validation and fail fast with a helpful error.
| subnetV6 = cns.IPSubnet{ | |
| IPAddress: nc.PrimaryIPV6, | |
| if !subnetV6Prefix.Addr().Is6() { | |
| return nil, errors.Errorf("invalid SubnetAddressSpaceV6 %s: not an IPv6 prefix", nc.SubnetAddressSpaceV6) | |
| } | |
| if nc.PrimaryIPV6 == "" { | |
| return nil, errors.Wrapf(ErrInvalidPrimaryIP, "empty PrimaryIPV6 for SubnetAddressSpaceV6 %s", nc.SubnetAddressSpaceV6) | |
| } | |
| primaryV6Addr, e := netip.ParseAddr(nc.PrimaryIPV6) | |
| if e != nil { | |
| return nil, errors.Wrapf(ErrInvalidPrimaryIP, "invalid PrimaryIPV6 %s", nc.PrimaryIPV6) | |
| } | |
| if !primaryV6Addr.Is6() { | |
| return nil, errors.Wrapf(ErrInvalidPrimaryIP, "PrimaryIPV6 %s is not an IPv6 address", nc.PrimaryIPV6) | |
| } | |
| subnetV6 = cns.IPSubnet{ | |
| IPAddress: primaryV6Addr.String(), |
| func TestCreateNCRequestFromDynamicNCWithIPv6(t *testing.T) { | ||
| nc := v1alpha.NetworkContainer{ | ||
| ID: ncID, | ||
| PrimaryIP: primaryIP, | ||
| PrimaryIPV6: "fd12:3456:789a::1", | ||
| SubnetAddressSpace: subnetAddressSpace, | ||
| SubnetAddressSpaceV6: "fd12:3456:789a::/48", | ||
| DefaultGateway: defaultGateway, | ||
| NodeIP: nodeIP, | ||
| Version: version, | ||
| } | ||
| got, err := CreateNCRequestFromDynamicNC(nc) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, cns.IPSubnet{IPAddress: "fd12:3456:789a::1", PrefixLength: 48}, got.IPConfiguration.IPSubnetV6) | ||
| } |
There was a problem hiding this comment.
The new IPv6 tests only assert IPSubnetV6 and don’t set/verify DefaultGatewayV6 -> GatewayIPv6Address propagation. Since dynamic conversion currently drops GatewayIPv6Address entirely, adding DefaultGatewayV6 to the test input and asserting got.IPConfiguration.GatewayIPv6Address would prevent regressions here.
| IPSubnet IPSubnet | ||
| IPSubnetV6 IPSubnet | ||
| DNSServers []string | ||
| GatewayIPAddress string | ||
| GatewayIPv6Address string |
There was a problem hiding this comment.
Adding IPSubnetV6 to IPConfiguration introduces new IPv6 data paths, but CreateNetworkContainerRequest.Validate() currently only validates GatewayIPAddress and doesn’t validate GatewayIPv6Address/IPSubnetV6 at all. Consider extending validation to cover the new IPv6 fields when they’re set so malformed IPv6 values fail early with a clear error.
|
This pull request is stale because it has been open for 2 weeks with no activity. Remove stale label or comment or this will be closed in 7 days |
Reason for Change:
Issue Fixed:
Requirements:
Notes: