feat: stateless cni compatibility with nodesubnet by santhoshmprabhu · Pull Request #3549 · Azure/azure-container-networking

santhoshmprabhu · 2025-04-01T21:24:37Z

Reason for Change:
Stateless CNI is currently not compatible with nodesubnet, even with the changes that were recently incorporated into CNS for Cilium+Nodesubnet support. Specifically, IP configs returned from CNS in Nodesubnet mode do not populate NC-specific fields - ncGatewayIPAddress, ncSubnetPrefix, ncPrimarIP etc. The CNI today expects these values, since CNS is invoked only for Podsubnet/Overlay cases. With this PR, we have the following changes:

Have a configuration option to configure the CNI for nodesubnet
In nodesubnet mode, the CNI uses hostGateway and subnet to configure the pods
The conflist for Azure CNI Nodesubnet on both windows and linux additionally specifies the mode as "nodesubnet". Today, the mode is specified only for overlay. Nodesubnet uses azure-vnet-ipam as the IPAM type, and podsubnet uses azure-cns as IPAM type with mode unspecified. With this change, cniv1 clusters can continue using azure-vnet-ipam (mode is ignored), whenever we deploy stateless CNI with nodesubnet, we'll use azure-cns as the type, and nodesubnet as the mode. Making this change here allows us to leave AgentBaker CSE scripts untouched.
Doesn't block pod deletion if HNS network is not found. Rather, we log the event and proceed with pod deletion. Effectively, this allows deleting pods that never got an IP. This is an enhancement to help debug issues more easily.

The PR touches code that appears to have TODO items to clean up the Overlay configuration parameter. I'm planning to do that separately once I validate that the new parameter is fully rolled out.

Issue Fixed:

Requirements:

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

Notes:

… subnet

Copilot

Pull Request Overview

Stateless CNI compatibility with nodesubnet has been added to allow node subnet-based pod IP configuration and non-blocking pod deletion when the HNS network is missing.

Update endpoint deletion to return nil when no HNS id is found instead of an error
Introduce a new IpamMode ("Nodesubnet") and adjust the IP configuration logic accordingly
Extend unit tests to cover default add result configuration in nodesubnet mode

Reviewed Changes

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

File	Description
network/endpoint_windows.go	Adjusted deletion logic to skip HNS deletion errors in order to allow pod deletion when HNS id is missing.
cni/util/const.go	Added a new IpamMode "Nodesubnet" to support the nodesubnet configuration.
cni/network/invoker_cns_test.go	Added tests to validate the default add result configuration when running in nodesubnet mode.
cni/network/invoker_cns.go	Refactored the configureDefaultAddResult function to handle nodesubnet mode by removing the overlayMode arg and updating the gateway logic.

Files not reviewed (1)

hack/aks/Makefile: Language not supported

Comments suppressed due to low confidence (2)

network/endpoint_windows.go:534

Returning nil instead of an error when HNS id is missing is intentional per the design, but please add an inline comment explaining why a nil return is acceptable as it might be unexpected.

return nil

cni/network/invoker_cns.go:411

[nitpick] The error message for an invalid gateway address could include additional context (such as the ipam mode) to improve debuggability. Consider updating the message to clarify the expected behavior in different modes.

if podGateway == nil {

santhoshmprabhu · 2025-04-01T21:35:23Z

/azp run Azure Container Networking PR

azure-pipelines · 2025-04-01T21:35:45Z

Azure Pipelines successfully started running 1 pipeline(s).

santhoshmprabhu · 2025-04-01T22:00:53Z

/azp run Azure Container Networking PR

azure-pipelines · 2025-04-01T22:01:13Z

Azure Pipelines successfully started running 1 pipeline(s).

santhoshmprabhu · 2025-04-02T20:49:18Z

/azp run Azure Container Networking PR

azure-pipelines · 2025-04-02T20:49:30Z

Azure Pipelines successfully started running 1 pipeline(s).

behzad-mir

lgtm

behzad-mir · 2025-04-02T21:57:28Z

 	if ep.HnsId == "" {
 		logger.Error("No HNS id found. Skip endpoint deletion", zap.Any("nicType", ep.NICType), zap.String("containerId", ep.ContainerID))
-		return fmt.Errorf("No HNS id found. Skip endpoint deletion for nicType %v, containerID %s", ep.NICType, ep.ContainerID) //nolint
+		return nil


I think adding a comment about why we are returning nil here would be helpful

maybe it could return an error of some known "NotFound" type, and the caller could tolerate that, instead

It seems we want to allow this behavior, at least according to this UT:

azure-container-networking/network/endpoint_windows_test.go

Line 118 in 20d09ab

func TestDeleteEndpointImplHnsV2WithEmptyHNSID(t *testing.T) {

@paulyufan mentioned that this UT is run manually locally, not as part of the PR pipeline. The UT fails on master, and needs this change to pass actually. Possibly deleteEndpointImpl drifted from intended behavior by mistake?

hm why this isn't run in the PR pipeline? because it's in a _windows test?

Not sure about that actually. We do have a Windows Tests stage in the PR pipeline of course. Maybe @paulyufan could weigh in?

Hi @rbtr , TestDeleteEndpointImplHnsV2WithEmptyHNSID() is part of _windows UT test cases and PR pipeline does not run them currently. John confirmed we only run npm windows UTs on ACN pipeline but not for others

Below contains UT(s) we run for windows in the pipeline.

azure-container-networking/.pipelines/templates/run-unit-tests.yaml

Line 43 in a2a2ab8

go test -timeout 30m ./npm/... ./cni/... ./platform/...

@rbtr, we can expand the scope for our Windows test to include this (and other) test(s), but given this info, what are your thoughts on the diff above?

unchanged from my first comment

@rbtr addressed, please take a look.

…ubnet

santhoshmprabhu · 2025-04-03T23:12:12Z

/azp run Azure Container Networking PR

azure-pipelines · 2025-04-03T23:12:31Z

Azure Pipelines successfully started running 1 pipeline(s).

santhoshmprabhu · 2025-04-04T04:49:09Z

/azp run Azure Container Networking PR

azure-pipelines · 2025-04-04T04:49:19Z

Azure Pipelines successfully started running 1 pipeline(s).

paulyufan2 · 2025-04-04T11:54:08Z

-		// TODO: Remove v4overlay and dualstackoverlay options, after 'overlay' rolls out in AKS-RP
-		if !overlayMode {
+	podGateway := net.ParseIP(info.ncGatewayIPAddress)
+	// TODO: Remove v4overlay and dualstackoverlay options, after 'overlay' rolls out in AKS-RP


yeah, let's remove these ipam modes selection later, believe overlay has rollout in aks-rp. @pjohnst5 can you confirm this?

Brand new cluster - it seems this isn't rolled out.

This isn't yet rolled out because in CNI 1.4 we don't have the unified overlay parameter. We could possibly update the CNS config map in a way that preserved v4overlay only in 1.4 and unifies the option in 1.5 and 1.6, but that has not been done likely to avoid complicating the CNS config logic. There's a separate ongoing conversation about how this needs to be handled in AgentBaker for Windows nodes.

santhoshmprabhu · 2025-04-08T21:37:25Z

Had a chat with @tamilmani1989. He suggested to keep CNI behavior independent of IPAM modes, which means moving the handling of Nodesubnet case into CNS. Closing this PR, will open a separate one for CNS.

santhoshmprabhu added 2 commits March 21, 2025 15:04

feat: do not stop deletion on missing hns id

7bd2f6d

feat: use host gw, subnet when in nodesubnet mode, rather than nc gw,…

c191d40

… subnet

Copilot AI review requested due to automatic review settings April 1, 2025 21:24

santhoshmprabhu requested review from a team as code owners April 1, 2025 21:24

santhoshmprabhu requested a review from vipul-21 April 1, 2025 21:24

Copilot AI reviewed Apr 1, 2025

View reviewed changes

santhoshmprabhu self-assigned this Apr 1, 2025

santhoshmprabhu added cni Related to CNI. windows labels Apr 1, 2025

fix: restore makefile

605b9e0

santhoshmprabhu added 2 commits April 1, 2025 14:41

chore: minor cleanup

1b63384

chore: make linter happy

5dccb40

fix: fix UT

6d63da0

santhoshmprabhu requested review from behzad-mir, paulyufan2, rbtr and tamilmani1989 April 2, 2025 21:38

behzad-mir previously approved these changes Apr 2, 2025

View reviewed changes

behzad-mir reviewed Apr 2, 2025

View reviewed changes

feat: add nodesubnet as the ipam mode in azure cni conflist for nodes…

0045243

…ubnet

santhoshmprabhu dismissed behzad-mir’s stale review via 0045243 April 2, 2025 23:15

santhoshmprabhu added 2 commits April 3, 2025 11:44

fix: fix conflist

21254db

fix: address comment

02ff7bc

fix: lint

e75dda9

MikeZappa87 reviewed Apr 3, 2025

View reviewed changes

Comment thread cni/network/invoker_cns.go Outdated

fix: address comments

bd4a5a9

paulyufan2 reviewed Apr 4, 2025

View reviewed changes

santhoshmprabhu closed this Apr 8, 2025

Conversation

santhoshmprabhu commented Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

santhoshmprabhu commented Apr 1, 2025

Uh oh!

azure-pipelines Bot commented Apr 1, 2025

Uh oh!

santhoshmprabhu commented Apr 1, 2025

Uh oh!

azure-pipelines Bot commented Apr 1, 2025

Uh oh!

santhoshmprabhu commented Apr 2, 2025

Uh oh!

azure-pipelines Bot commented Apr 2, 2025

Uh oh!

behzad-mir left a comment

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!

paulyufan2 Apr 3, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

santhoshmprabhu commented Apr 3, 2025

Uh oh!

azure-pipelines Bot commented Apr 3, 2025

Uh oh!

santhoshmprabhu commented Apr 4, 2025

Uh oh!

azure-pipelines Bot commented Apr 4, 2025

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!

santhoshmprabhu commented Apr 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

santhoshmprabhu commented Apr 1, 2025 •

edited

Loading

paulyufan2 Apr 3, 2025 •

edited

Loading