Prefix on nicv6 support #3658

NihaNallappagari · 2025-05-13T22:54:15Z

Reason for Change:
Current Problem:
VNET currently limits scaling to 64K IP addresses per VNET due to per-IP route mappings. To support larger AKS clusters, the platform is introducing per-prefix (CIDR) route mapping, allowing a single mapping to represent multiple IPs (e.g., /24 enables 256 IPs per mapping). This enables scaling up to 16 million IPs per VNET without impacting the underlying platform.

Change needed:
Prefix on NIC v4 is supported in Swiftv1, but Swiftv1 does not support IPv6. This change upgrades Prefix on NIC v4 functionality to Swiftv2 and introduces IPv6 support in Swiftv2.

Changes
This PR has changes specific to Prefix on NICv6
CNS
--Consuming PrimaryIPv6, GatewayIPv6, MacAddress (DelegatedNIC) from NNC CRD because of dualstack NC
--IP allocation: Assign IPs of each IPFamily as part of RequestIPs api request (Currently when a pod is created, IPAM RequestsIPs from CNS where CNS picks one IP from each NC and hands it over to IPAM.

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

Design doc for pocv6 with sample NNC
Requirements:

uses conventional commit messages
includes documentation
adds unit tests
relevant PR labels added

Notes:

NihaNallappagari · 2025-05-13T23:45:20Z

@microsoft-github-policy-service agree company="Microsoft"

azure-ipam/ipam.go

tamilmani1989 · 2025-06-05T23:16:28Z

azure-ipam/ipconfig/ipconfig.go

+		if gatewayIP != nil {
+			gatewaysIPs[i] = gatewayIP
+		} else {
+			logger.Printf("gatewayIP is nil or gateway ip parse failed for pod ip %s", resp.PodIPInfo[i].PodIPConfig.IPAddress)


should we return error if gwip is nil?

Updated code to return error message

tamilmani1989 · 2025-06-05T23:21:02Z

cns/kubecontroller/nodenetworkconfig/conversion_linux.go

-		secondaryIPConfigs[addr.String()] = cns.SecondaryIPConfig{
-			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


@kadevu is primary ip /32 only on swiftv2 and not on swiftv1? I knew we had this discussion, sorry i forgot the reason behind this check

@kadevu is having issues to comment on the PR. His response "yes this is only for Swiftv2. This is basically the primary ip to assign the to the delegated nic and not assigned to any pod which is in line with the InterfaceCompartment published to pubsub. In case of Swiftv1, this field has /28 instead and all those ips are assigned to the pods"

since we are not assigning any ip to delegated nic, why it has to be /32 and not /28 as like swiftv1?

the interfacecompartment schema enforces customeripaddress to be /32 and customeripprefix to be /28. For the customeripadderss we carve out a /28 and then assign a /32 from it. The rest of the IPs in the /28 are not programmed by nmagent which is why we cannot use these for the pods. To make sure there is no wastage of ips we use the other ips from the /28 from which we got the /32 to assign customeripaddress for other nodes in the nodepool.

I removed the if check for single ip. We have some discussions going with Control plane SMEs. I will do a follow-up to change this

tamilmani1989 · 2025-06-05T23:23:08Z

cns/restserver/internalapi.go

+		ncStatus := service.state.ContainerStatus[idx]
+		if mac := ncStatus.CreateNetworkContainerRequest.NetworkInterfaceInfo.MACAddress; mac != "" {
+			logger.Printf("NC %s has MAC address for prefix on nic swiftv2 scenario, skipping syncHostNCVersion", ncStatus.ID)
+			continue


so we detect swiftv2 based on mac? why not add a config field in cns to specify swiftv2. i guess there is one for singularity which we can reuse

Sure. This is specific to swiftv2 NIC, we can make use of config fields. Is this the file you are talking about cns config. If so, I don't find a field specific to singularity. Could you share the field you are talking about? Thanks

what about this one?

azure-container-networking/cns/configuration/configuration.go

Line 37 in f617f1f

EnableSwiftV2 bool

I removed the mac address check. Once the NMAgent exposes NC details, I will make the changes in a new PR

cns/restserver/internalapi_linux.go

tamilmani1989 · 2025-06-05T23:26:20Z

@rbtr can you review this as well since it touches cns changes for aks swift too

Copilot

Pull Request Overview

This PR introduces changes to support dual-stack NIC functionality for Prefix on NICv6 scenarios by extending both CNS and IPAM components. Key updates include:

CNS: Populating the MACAddress in podIPInfo and skipping NC version sync when a MAC address is present.
IPAM: Adjusting IP allocation logic by mapping and handling multiple IP families and updating error messages accordingly.
Additional updates in tests, internal API, conversion, network container contract, and Azure IPAM integration to support IPv6 gateway addresses and related deduplication.

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
cns/restserver/util.go	Added MACAddress population to podIPInfo
cns/restserver/ipam.go	Adjusted IP assignment logic for dual-stack handling and introduced generateAssignedIPKey
cns/restserver/internalapi_test.go	Added test to verify skipping NC version sync for prefix on NIC swift v2
cns/restserver/internalapi_linux.go	Added IPv4 check in SNAT rule programming
cns/restserver/internalapi.go	Skips sync for prefix on NIC when MAC address is set
cns/kubecontroller/nodenetworkconfig/conversion_linux.go	Updated static NC request conversion to handle single IP prefix cases properly
cns/NetworkContainerContract.go	Added IPv6 gateway field and IPFamily enum definition
azure-ipam/ipconfig/ipconfig.go	Updated response processing to include IPv6 gateway parsing
azure-ipam/ipam.go	Modified CmdAdd to integrate IPv6 gateway assignment and deduplicate interfaces

Comments suppressed due to low confidence (2)

cns/restserver/ipam.go:1029

Consider adding an inline comment to clarify why the slice size for podIPInfo is set to the number of IP families, emphasizing how dual-stack handling determines the number of IPs to assign.

numberOfIPs = numOfIPFamilies

azure-ipam/ipam.go:150

[nitpick] Consider adding a comment explaining that the deduplication of interfaces by MAC address is deliberate to avoid duplicate interface entries when multiple IP family entries point to the same network interface.

if podIPInfo.MacAddress == "" || seenInterfaces[podIPInfo.MacAddress] {

rbtr

are there AzCNI changes I'm missing here or is this only for Cilium?

azure-ipam/ipconfig/ipconfig.go

rbtr · 2025-06-10T23:52:58Z

cns/restserver/internalapi.go

+		ncStatus := service.state.ContainerStatus[idx]
+		if mac := ncStatus.CreateNetworkContainerRequest.NetworkInterfaceInfo.MACAddress; mac != "" {
+			logger.Printf("NC %s has MAC address for prefix on nic swiftv2 scenario, skipping syncHostNCVersion", ncStatus.ID)
+			continue


what about this one?

azure-container-networking/cns/configuration/configuration.go

Line 37 in f617f1f

EnableSwiftV2 bool

cns/restserver/internalapi_linux.go

cns/restserver/internalapi_test.go

rbtr · 2025-06-10T23:58:15Z

cns/restserver/ipam.go

+			ip := net.ParseIP(secIPConfig.IPAddress)
+			if ip == nil {
+				continue
+			}
+
+			if ip.To4() != nil {
+				ncIPFamilies[cns.IPv4] = struct{}{}
+			} else {
+				ncIPFamilies[cns.IPv6] = struct{}{}
+			}


use netip.ParseAddr to get a netip.Addr, then you can call Addr#Is4 or Addr#Is6 instead. prefer netip to net package in new code

Made the changes mentioned. Thanks

Address PR comments remove unnecessary logger update synhost skip test scenario fix linting error updated lint error fix linting error . update synhost skip test scenario fix linting error updated lint error fix linting error .

tamilmani1989 · 2025-06-16T16:54:40Z

azure-ipam/ipconfig/ipconfig.go

+
+		gatewayIP := net.ParseIP(gatewayStr)
+		if gatewayIP == nil {
+			return nil, nil, errors.Errorf("failed to parse gateway IP %q for pod ip %s", gatewayStr, podInfo.PodIPConfig.IPAddress)


if nil, why we log gatewayStr. will it cause any panic? did we add UT for this?

Gateway string is either nil or could not be parsed. Logging may help diagnose the issue. As we returning error, ipam will handle the error case and ignores the return values of pods and gatway ip

azure-container-networking/azure-ipam/ipam.go

Line 118 in 8806489

podIPNet, gatewayIP, err := ipconfig.ProcessIPConfigsResp(resp)

. Thanks for pointing out the missing test. I've added the appropriate test scenario to cover this case.

cns/restserver/internalapi_linux.go

. .

tamilmani1989 · 2025-06-23T22:45:49Z

azure-ipam/ipam.go

+		infMac, err := net.ParseMAC(podIPInfo.MacAddress)
+		if err != nil {
+			p.logger.Error("Failed to parse interface MAC address", zap.Error(err), zap.String("macAddress", podIPInfo.MacAddress))
+			return cniTypes.NewError(cniTypes.ErrUnsupportedField, err.Error(), "failed to parse interface MAC address")
+		}
+
+		cniResult.Interfaces = append(cniResult.Interfaces, &types100.Interface{
+			Mac: infMac.String(),
+		})
+		seenInterfaces[podIPInfo.MacAddress] = true
+	}
+


can we add UT for this case with dualstack ips?

Added UT for dual stack scenarios

tamilmani1989 · 2025-06-23T22:50:31Z

cns/restserver/ipam.go

+
+	//  Map used to get the number of IPFamilies across all NCs
+	ncIPFamilies := map[cns.IPFamily]struct{}{}
+	// Gets the IPFamilies from all NCs and store them in a map. This will be used to determine the number of IPs to return
+	for ncID := range service.state.ContainerStatus {
+		if len(ncIPFamilies) == 2 {
+			break
+		}
+
+		for _, secIPConfig := range service.state.ContainerStatus[ncID].CreateNetworkContainerRequest.SecondaryIPConfigs {
+			if len(ncIPFamilies) == 2 {
+				break
+			}
+
+			addr, err := netip.ParseAddr(secIPConfig.IPAddress)
+			if err != nil {
+				continue
+			}
+
+			if addr.Is4() {
+				ncIPFamilies[cns.IPv4] = struct{}{}
+			} else if addr.Is6() {
+				ncIPFamilies[cns.IPv6] = struct{}{}
+			}
+		}
+	}
+	// Makes sure we have at least one IPFamily across all NCs
+	numOfIPFamilies := len(ncIPFamilies)
+


can we move this to separate function say getNumberofIPFamily

add UT for this too

Added test and moved chunk of code to new function

cns/restserver/internalapi.go

Signed-off-by: NihaNallappagari <69693983+NihaNallappagari@users.noreply.github.com>

small fixes fix lint errors lint fix . Delete examples/imds_nc_lookup.go Signed-off-by: NihaNallappagari <69693983+NihaNallappagari@users.noreply.github.com> fix lint .

kadevu and others added 2 commits April 14, 2025 14:46

Prefix on NIC v6 support

419d904

update dedup comment in ipam

e9cb405

Copilot AI review requested due to automatic review settings May 13, 2025 22:54

NihaNallappagari requested review from a team as code owners May 13, 2025 22:54

NihaNallappagari requested a review from csfmomo May 13, 2025 22:54

This comment was marked as outdated.

Sign in to view

add log for a gatway error scenario

61784dc

NihaNallappagari force-pushed the prefixOnNicv6Support branch from 99288e4 to 61784dc Compare May 20, 2025 12:05

handle synchostversion sync on pon swiftv2 nic

e00a5b7

tamilmani1989 reviewed Jun 5, 2025

View reviewed changes

rbtr requested a review from Copilot June 6, 2025 15:56

Copilot AI reviewed Jun 6, 2025

View reviewed changes

handle gatewayip parse failure or nil scenario

d0e56de

rbtr requested changes Jun 11, 2025

View reviewed changes

remove unused code

7de69b1

Address PR comments remove unnecessary logger update synhost skip test scenario fix linting error updated lint error fix linting error . update synhost skip test scenario fix linting error updated lint error fix linting error .

NihaNallappagari force-pushed the prefixOnNicv6Support branch from a00a067 to 7de69b1 Compare June 13, 2025 20:40

tamilmani1989 reviewed Jun 16, 2025

View reviewed changes

NihaNallappagari added 3 commits June 16, 2025 14:52

add test scenario

8806489

update test to handle gatway nil case

26be9a4

remove synchost skip and single ip skip

052330e

. .

NihaNallappagari force-pushed the prefixOnNicv6Support branch from 8024c9f to 052330e Compare June 17, 2025 21:18

tamilmani1989 requested changes Jun 23, 2025

View reviewed changes

NihaNallappagari and others added 4 commits June 25, 2025 15:06

Add missing test scenarios

af07e5a

Merge branch 'master' into prefixOnNicv6Support

bf539fb

fix ipam_test conflicts

3aa8e27

Merge branch 'master' into prefixOnNicv6Support

9da8675

Signed-off-by: NihaNallappagari <69693983+NihaNallappagari@users.noreply.github.com>

NihaNallappagari force-pushed the prefixOnNicv6Support branch from 8c9f567 to 85118ff Compare June 26, 2025 15:44

fix conflits

14d0336

small fixes fix lint errors lint fix . Delete examples/imds_nc_lookup.go Signed-off-by: NihaNallappagari <69693983+NihaNallappagari@users.noreply.github.com> fix lint .

NihaNallappagari force-pushed the prefixOnNicv6Support branch from 85118ff to 14d0336 Compare June 26, 2025 15:46

fix linting

605770b

Prefix on nicv6 support #3658

Are you sure you want to change the base?

Prefix on nicv6 support #3658

Uh oh!

Conversation

NihaNallappagari commented May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

NihaNallappagari commented May 13, 2025 • edited by rbtr Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tamilmani1989 commented Jun 5, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

rbtr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NihaNallappagari Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NihaNallappagari Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

NihaNallappagari commented May 13, 2025 •

edited

Loading

NihaNallappagari commented May 13, 2025 •

edited by rbtr

Loading

NihaNallappagari Jun 26, 2025 •

edited

Loading

NihaNallappagari Jun 26, 2025 •

edited

Loading