[feat] Prefix on NIC v6 windows support #4060

NihaNallappagari · 2025-10-01T14:36:24Z

Reason for Change:
This PR includes the required changes for Windows to support prefix on NIC NIC Swift v2 scenario. The HNS team requires certain values to be added to the registry, and this PR sets those values under the appropriate registry key.

Issue Fixed:

Requirements:

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

Notes:
This PR is extension of Prefix on nix linux support

* add iptables block to signed image * fix syntax * mariner version

* Prefix on NIC v6 support * update dedup comment in ipam * add log for a gatway error scenario * handle synchostversion sync on pon swiftv2 nic * handle gatewayip parse failure or nil scenario * remove unused code 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 . * add test scenario * update test to handle gatway nil case * remove synchost skip and single ip skip . . * Add missing test scenarios * fix ipam_test conflicts * fix conflits small fixes fix lint errors lint fix . Delete examples/imds_nc_lookup.go Signed-off-by: NihaNallappagari <[email protected]> fix lint . * fix linting * add todo and remove multiple interface scenario * Skip add primary ip's to secondary config for swiftv2 pon scenarios fix lint error . * code review comment * Update gateway ipv6 to use default value, that auto detects and adds to neigh table * remove unwanted changes * address ipfamily comment * fix test failure * Handle nilgateway scenario * remove unnedded else block * comments changes --------- Co-authored-by: Kaushik Vuligonda <[email protected]> Co-authored-by: nn052161 <[email protected]>

…re-container-networking into ponv6WindowsSupport

github-actions · 2025-11-01T00:01:52Z

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

github-actions · 2025-11-09T00:01:54Z

Pull request closed due to inactivity.

github-actions · 2025-11-28T00:01:56Z

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

Copilot

Pull request overview

This PR adds Windows support for the SwiftV2 PrefixOnNic scenario by implementing Windows registry configuration functionality. The changes enable CNS to detect SwiftV2 VNETBlock network containers and configure appropriate registry settings for HNS (Host Network Service) on Windows platforms.

Key Changes:

Added Windows registry manipulation functions to configure PrefixOnNic settings
Introduced SwiftV2PrefixOnNic field to track SwiftV2 VNETBlock containers
Modified IMDS NC retrieval to handle Windows SwiftV2 scenarios and trigger registry configuration

Reviewed changes

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

Show a summary per file

File	Description
cns/restserver/internalapi_windows.go	Implements registry manipulation functions for configuring Windows HNS PrefixOnNic settings
cns/restserver/internalapi_test.go	Adds test coverage for Windows SwiftV2 PrefixOnNic scenario in SyncHostNCVersion
cns/restserver/internalapi_linux.go	Provides no-op implementation of setPrefixOnNICRegistry for Linux platform
cns/restserver/internalapi.go	Refactors getIMDSNCs to detect and configure Windows SwiftV2 scenarios, adds isPrefixonNicSwiftV2 helper
cns/kubecontroller/nodenetworkconfig/conversion_linux.go	Sets SwiftV2PrefixOnNic flag based on NC type for static NCs
cns/kubecontroller/nodenetworkconfig/conversion.go	Explicitly sets SwiftV2PrefixOnNic to false for dynamic NCs
cns/NetworkContainerContract.go	Adds SwiftV2PrefixOnNic field to CreateNetworkContainerRequest

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

tamilmani1989 · 2025-12-23T17:56:27Z

cns/restserver/internalapi_windows.go

+	return service.setRegistryValue(hnsRegistryPath, "EnableSNAT", isEnabled)
+}
+
+func (service *HTTPRestService) setPrefixOnNICRegistry(enablePrefixOnNic bool, infraNicMacAddress string) error {


why this is required for hns?

HNS team need to know thinks like infra nic, delegated nic, swiftv2 prefix on nic scenario to probably add some routes specific to the scenario. HNS team might answer better, tagging @surajkrishnan14

Yes, this is required for HNS to setup appropriate rules to forward packets correctly between the 2 interfaces and container in various scenarios, setup SNAT etc.

tamilmani1989 · 2025-12-23T18:00:12Z

cns/restserver/internalapi.go


 		if ncID != "" {
 			ncs[ncID] = PrefixOnNicNCVersion // for prefix on nic version scenario nc version is 1
+		} else if runtime.GOOS == "windows" && service.isPrefixonNicSwiftV2() {


GOOS=windows not required since you are defining in windows specific file, linux specific calls would be dummy.. i think its better we expose all regkey apis through interface.. @behzad-mir / @rbtr to comment on this

Good feedback. IMO any explicit check to GOOS or stubbing an interface in an *_<os>.go file indicates that the abstraction is in the wrong place.
There was a similar discussion here #3286 (comment) which you should read so that I don't have to repeat it all.

The tl;dr is that our abstraction should live at the last boundary of common code before we start splitting things up by OS. E.g. here: if getIMDSNCs is the final call possible before we have to do OS specific things, that is what lives in both the *_linux.go and the *_windows.go.

However - I haven't done a full review of this code, but at first glance getIMDSNCs is probably not the place to be doing OS specific things at all. That advertises that it just getting NCs.
And if we think about duplicating this func in OS-specific files, we end up copying a bunch of common code to both locations that doesn't care about the OS at all. This probably all just means that the current func is overloaded and poorly defined. Consider how to separate things in to the minimum necessary complete behaviors.

removed the os check and moved windowskey process call form getIMDSNCs . Thanks for the feedback

tamilmani1989 · 2025-12-23T18:03:47Z

cns/restserver/internalapi_windows.go

+	return service.setRegistryValue(hnsRegistryPath, "EnableSNAT", isEnabled)
+}
+
+func (service *HTTPRestService) setPrefixOnNICRegistry(enablePrefixOnNic bool, infraNicMacAddress string) error {


since we are setting multiple regkeys under same function, can function name be setRegistryKeysForPrefixOnNic and then expose all registry key functions in interface, so that unit test can be written mocking it

Refactored as per suggestion

QxBytes

could you also add to the pr description some background on prefix on nic (pon, particularly v6-- what PON is meant for/aims to accomplish), what this feature is allowing us to do that we couldn't do before, and what scenarios this affects (ex: standalone, multi/single tenant)

QxBytes · 2026-01-07T17:38:49Z

cns/restserver/restserver.go

 		dockerClient:             dc,
 		wscli:                    wscli,
 		iptables:                 iptg,
+		windowsRegistry:          newRegistryClient(),


wondering if we should pass this in like how we do with iptablesGetter for example

QxBytes · 2026-01-07T17:43:17Z

cns/restserver/internalapi_linux.go

+
+// newLinuxRegistryClient creates a no-op registry client for Linux.
+func newLinuxRegistryClient() windowsRegistryClient {
+	return &linuxRegistryClient{}


newLinuxRegistryClient() seems to be called only from newRegistryClient()-- can we just have newRegistryClient() return &linuxRegistryClient directly?

@NihaNallappagari I don't think you have understood my earlier feedback on OS abstraction/separations.

A func called newLinuxRegistryClient returning struct linuxRegistryClient as interface windowsRegistryClient is nonsensical. Stubbing Windows only behavior in a Linux struct so that the special Windows code compiles on Linux is bad. This abstraction is wrong.

If you need to make stubs like this, the place that you are calling these stubs is OS specific code already. THAT is what should be refactored in to Windows and Linux versions.

QxBytes · 2026-01-07T20:52:28Z

cns/restserver/internalapi.go

 		if ncID != "" {
 			ncs[ncID] = PrefixOnNicNCVersion // for prefix on nic version scenario nc version is 1
+		} else {
+			infraNicMacAddress = iface.MacAddress.String()


what does having an empty ncID mean and when does this happen? can this be added as a comment for the function? can we also clarify in the comment what mac address we are getting? will the infra nic mac address ever be empty if all the network interfaces have nc ids?

QxBytes · 2026-01-07T22:14:28Z

cns/kubecontroller/nodenetworkconfig/conversion_windows_test.go

 			NCVersion: version,
 		},
 	},
+	SwiftV2PrefixOnNic: true,


could you check conversion_test.go passes when OS=windows-- this request is the expected for one of the tests but in the test we set swiftv2 to false

QxBytes · 2026-01-07T22:19:41Z

cns/restserver/internalapi.go

+	// Get IMDS NC versions for delegated NIC scenarios.
+	imdsNCVersions, infraNicMacAddress := service.getIMDSNCs(ctx)
+
+	// Process Windows registry keys with the retrieved MAC address (empty if not found). It is required for HNS team to configure cilium routes sepcific to windows nodes.


nit: cilium routes "specific" to windows nodes and nit: isPrefixOnNicSwiftV2() (capital "O" for camel case or isPrefixOnNICSwiftV2() for capitalized acronyms)

QxBytes · 2026-01-07T22:24:23Z

cns/restserver/internalapi.go

+
+	// Process Windows registry keys with the retrieved MAC address (empty if not found). It is required for HNS team to configure cilium routes sepcific to windows nodes.
+	isPrefixOnNic := service.isPrefixonNicSwiftV2()
+	service.processWindowsRegistryKeys(isPrefixOnNic, infraNicMacAddress)


can you confirm that it's intentional that this runs even for non-prefix on nic scenarios?

QxBytes · 2026-01-07T22:31:29Z

cns/restserver/internalapi.go

-// GetIMDSNCs gets NC versions from IMDS and returns them as a map
-func (service *HTTPRestService) GetIMDSNCs(ctx context.Context) (map[string]string, error) {
+// GetIMDSNCs gets NC versions from IMDS and returns them as a map along with the MAC address
+func (service *HTTPRestService) getIMDSNCs(ctx context.Context) (ncs map[string]string, infraNicMacAddress string) {


since we're also getting the mac of the infra nic, maybe this should be something like getIMDSInfo now?

QxBytes · 2026-01-07T22:40:56Z

cns/restserver/internalapi.go

+}
+
+// Check whether NC is SwiftV2 NIC associated NC and prefix on nic is enabled
+func (service *HTTPRestService) isPrefixonNicSwiftV2() bool {


For the comment it seems more like "check whether any NC is a swiftv2 nic with prefix on nic enabled" rather than any particular NC-- when this is enabled is there typically only 1 NC (or is it multitenant?)?

QxBytes · 2026-01-07T22:55:30Z

cns/restserver/internalapi_windows.go

+
+// newWindowsRegistryClient creates a new Windows registry client.
+func newWindowsRegistryClient() windowsRegistryClient {
+	return &windowsRegistry{}


similar comment to the one I made about the newLinuxRegistryClient -- can we just return the windows registry directly from the newRegistryClient

rbtr · 2026-01-08T00:11:09Z

cns/restserver/internalapi_linux.go

+
+// newLinuxRegistryClient creates a no-op registry client for Linux.
+func newLinuxRegistryClient() windowsRegistryClient {
+	return &linuxRegistryClient{}


@NihaNallappagari I don't think you have understood my earlier feedback on OS abstraction/separations.

A func called newLinuxRegistryClient returning struct linuxRegistryClient as interface windowsRegistryClient is nonsensical. Stubbing Windows only behavior in a Linux struct so that the special Windows code compiles on Linux is bad. This abstraction is wrong.

If you need to make stubs like this, the place that you are calling these stubs is OS specific code already. THAT is what should be refactored in to Windows and Linux versions.

NihaNallappagari and others added 8 commits September 30, 2025 15:41

Add registry keys for prefix on nic scenario

01e4ea0

update tests

942e890

ci: add iptables block signed image (Azure#4049)

eed5080

* add iptables block to signed image * fix syntax * mariner version

Add registry keys for prefix on nic scenario

5ca999b

update tests

23a756b

Merge branch 'ponv6WindowsSupport' of github.com:NihaNallappagari/azu…

bc3cad8

…re-container-networking into ponv6WindowsSupport

remove unused code

4395f17

NihaNallappagari force-pushed the ponv6WindowsSupport branch from 875c0f0 to 4395f17 Compare October 14, 2025 18:20

github-actions bot added the stale Stale due to inactivity. label Nov 1, 2025

github-actions bot closed this Nov 9, 2025

NihaNallappagari reopened this Nov 13, 2025

github-actions bot removed the stale Stale due to inactivity. label Nov 14, 2025

github-actions bot added the stale Stale due to inactivity. label Nov 28, 2025

surajkrishnan14 approved these changes Dec 1, 2025

View reviewed changes

github-actions bot removed the stale Stale due to inactivity. label Dec 2, 2025

Merge branch 'master' into ponv6WindowsSupport

8e673c3

NihaNallappagari marked this pull request as ready for review December 16, 2025 16:25

NihaNallappagari requested a review from a team as a code owner December 16, 2025 16:25

NihaNallappagari requested review from Copilot and nairashu December 16, 2025 16:25

Copilot AI reviewed Dec 16, 2025

View reviewed changes

NihaNallappagari changed the title ~~[Draft PR-no review needed] Ponv6 windows support~~ [feat] Ponv6 windows support Dec 16, 2025

Azure deleted a comment from Copilot AI Dec 16, 2025

NihaNallappagari force-pushed the ponv6WindowsSupport branch from ca1cb7d to 19a8a62 Compare December 16, 2025 17:38

NihaNallappagari removed the request for review from nairashu December 16, 2025 18:19

NihaNallappagari requested review from rbtr and tamilmani1989 December 16, 2025 18:19

NihaNallappagari force-pushed the ponv6WindowsSupport branch 2 times, most recently from 39289be to 5e76f7b Compare December 17, 2025 17:01

update linting error

aef689e

NihaNallappagari force-pushed the ponv6WindowsSupport branch from 5e76f7b to aef689e Compare December 17, 2025 18:07

tamilmani1989 reviewed Dec 23, 2025

View reviewed changes

Azure deleted a comment from Copilot AI Dec 26, 2025

nihanMicrosoft added 2 commits December 31, 2025 11:58

address code review comments

d37a407

fix linting error

15e66d7

QxBytes reviewed Jan 7, 2026

View reviewed changes

rbtr requested changes Jan 8, 2026

View reviewed changes

NihaNallappagari changed the title ~~[feat] Ponv6 windows support~~ [feat] Prefix on NIC v6 windows support Jan 8, 2026

[feat] Prefix on NIC v6 windows support #4060

Are you sure you want to change the base?

[feat] Prefix on NIC v6 windows support #4060

Uh oh!

Conversation

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

Uh oh!

Uh oh!

github-actions bot commented Nov 1, 2025

Uh oh!

github-actions bot commented Nov 9, 2025

Uh oh!

github-actions bot commented Nov 28, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

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!

rbtr Dec 23, 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!

QxBytes 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

NihaNallappagari commented Oct 1, 2025 •

edited

Loading

rbtr Dec 23, 2025 •

edited

Loading