Prefix on nicv6 support #3657
Prefix on nicv6 support #3657
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements support for NICv6 by updating CNS and IPAM components to handle IPv6 specific fields and dual-stack IP allocation. Key changes include propagating MACAddress in CNS, updating IP allocation logic to consider IP families in IPAM, and extending the Network Container Contract with IPv6-related fields and a new IPFamily enum.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| cns/restserver/util.go | Added MACAddress propagation to pod IP information. |
| cns/restserver/ipam.go | Updated IP assignment logic to account for dual-stack (IPv4/IPv6) scenarios. |
| cns/restserver/internalapi_linux.go | Added IPv4 check for secondary IP configurations. |
| cns/kubecontroller/nodenetworkconfig/conversion_linux.go | Skipped adding primary IP to secondary config if it is a /32. |
| cns/NetworkContainerContract.go | Extended contract with GatewayIPv6Address and defined IPFamily enum. |
| azure-ipam/ipconfig/ipconfig.go | Updated response processing with IPv6 gateway extraction. |
| azure-ipam/ipam.go | Adjusted CNI result construction to include gateway IP and deduplicate interfaces. |
Comments suppressed due to low confidence (1)
azure-ipam/ipam.go:139
- [nitpick] Verify that the gatewayIPs slice always contains a valid gateway IP for each pod IP configuration to avoid potential empty gateway assignments.
ipConfig.Gateway = (*gatewayIP)[i]
| numberOfIPs := numOfNCs | ||
| if numOfIPFamilies != 0 { | ||
| numberOfIPs = numOfIPFamilies | ||
| } | ||
|
|
||
| service.Lock() | ||
| defer service.Unlock() | ||
| // Creates a slice of PodIpInfo with the size as number of NCs to hold the result for assigned IP configs | ||
| podIPInfo := make([]cns.PodIpInfo, numOfNCs) | ||
| podIPInfo := make([]cns.PodIpInfo, numberOfIPs) |
There was a problem hiding this comment.
[nitpick] Consider clarifying the logic and variable naming for the numberOfIPs assignment to better reflect the dual-stack IP allocation scenario where IP families are used.
| IPAddress: addr.String(), | ||
| NCVersion: int(nc.Version), | ||
| // in the case of vnet prefix on swift v2 the primary IP is a /32 and should not be added to secondary IP configs | ||
| if !primaryIPPrefix.IsSingleIP() { |
There was a problem hiding this comment.
[nitpick] Consider adding a comment explaining why a /32 primary IP is excluded from the secondary IP configurations to improve code clarity.
|
|
3 participants
Reason for Change:
This PR has changes specific to Prefix on NICv6
CNS
--Consuming PrimaryIPv6, GatewayIPv6, MacAddress (DeletedNIC) from NNC CRD because of dualstack NC
--IP allocation: Assign IPs of each IPFamily as part of RequestIPs api request
IPAM
--Change RequestIPs response parsing to read GatewayIPv6 and MacAddress
--Populates Interfaces with MacAddress which is used by CNI to plumb routes to send traffic
Requirements:
Notes: