[NPM Lite] Bypassing IPSets for IP CIDR Block Based Network Policies … #4147

rejain789 · 2025-11-26T19:51:53Z

added logic to bypass ipsets for /32 cidrs with npm lite
removed logic to only look at /32 pod cidrs and allow all pod cidr
updated code specific to direct ip logic
fixed if else logic
added error for named port
get rid of unneeded comments
got rid of function in utils that was not neede
added unit test for translate policy
resolved pr comments
resolved copilot comments
fixed golinter

Reason for Change:

Issue Fixed:

Requirements:

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

Notes:

…4107) * added logic to bypass ipsets for /32 cidrs with npm lite * removed logic to only look at /32 pod cidrs and allow all pod cidr * updated code specific to direct ip logic * fixed if else logic * added error for named port * get rid of unneeded comments * got rid of function in utils that was not neede * added unit test for translate policy * resolved pr comments * resolved copilot comments * fixed golinter

rejain789 · 2025-11-26T19:53:58Z

/azp run Azure Container Networking PR, NPM Scale Test, NPM Conformance Tests

azure-pipelines · 2025-11-26T19:54:19Z

Azure Pipelines successfully started running 3 pipeline(s).

Copilot

Pull request overview

This PR implements NPM Lite mode optimization for Windows by bypassing IPSet creation for all IP CIDR-based network policies. Instead of creating IPSets for CIDR blocks, the implementation now directly embeds IP addresses in ACL policies, reducing overhead and improving performance for CIDR-based network policies when npm lite is enabled.

Key Changes:

Added direct IP address fields (SrcDirectIPs, DstDirectIPs) to ACL policies for npm lite mode
Implemented directPeerAndPortAllowRule function to handle CIDR blocks without IPSets
Enhanced error messages for named port validation with additional context
Refactored Windows ACL settings to support both IPSet-based and direct IP approaches

Reviewed changes

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

File	Description
npm/pkg/dataplane/policies/policy.go	Added `SrcDirectIPs` and `DstDirectIPs` fields to ACLPolicy struct for direct IP matching; fixed comment formatting
npm/pkg/dataplane/policies/policy_windows.go	Implemented direct IP handling in ACL settings conversion; fixed spelling of "definitions"
npm/pkg/controlplane/translation/translatePolicy.go	Added `directPeerAndPortAllowRule` function for npm lite CIDR handling; removed `npmLiteValidPolicy` function; enhanced `checkForNamedPortType` with detailed error context
npm/pkg/controlplane/translation/translatePolicy_test.go	Added comprehensive tests for direct IP/CIDR handling; removed obsolete `TestNpmLiteCidrPolicy`; updated `TestCheckForNamedPortType` for new function signature

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

Copilot · 2025-11-26T20:00:48Z

npm/pkg/controlplane/translation/translatePolicy_test.go

-	}
-}
-
 func TestCheckForNamedPortType(t *testing.T) {


The removal of TestNpmLiteCidrPolicy leaves a gap in test coverage. While the npmLiteValidPolicy function was removed, the validation logic still exists at line 446-447 in translatePolicy.go which returns ErrUnsupportedNonCIDR when npm lite is enabled and a peer has no IPBlock (i.e., has PodSelector or NamespaceSelector).

Consider adding test cases to TestIngressPolicy and/or TestEgressPolicy that verify the ErrUnsupportedNonCIDR error is returned when npm lite is enabled with:

PodSelector peers

NamespaceSelector peers

Mixed CIDR and selector peers

This will ensure the npm lite validation logic remains properly tested.

Copilot · 2025-11-26T20:00:48Z

npm/pkg/dataplane/policies/policy.go

+	// SrcDirectIPs holds direct IPs for source matching (used for /32 CIDRs on Windows with npm lite enabled)
+	SrcDirectIPs []string
+	// DstDirectIPs holds direct IPs for destination matching (used for /32 CIDRs on Windows with npm lite enabled)


The documentation states these fields are "used for /32 CIDRs on Windows with npm lite enabled", but based on the implementation in translatePolicy.go and the test cases (which include /16, /24, /28 subnets), these fields are actually used for ALL CIDR blocks when npm lite is enabled, not just /32 CIDRs.

Please update the documentation to accurately reflect that these fields are used for any CIDR block when npm lite is enabled:

// SrcDirectIPs holds direct IPs for source matching (used for IP CIDR blocks on Windows with npm lite enabled)

Suggested change

// SrcDirectIPs holds direct IPs for source matching (used for /32 CIDRs on Windows with npm lite enabled)

SrcDirectIPs []string

// DstDirectIPs holds direct IPs for destination matching (used for /32 CIDRs on Windows with npm lite enabled)

// SrcDirectIPs holds direct IPs for source matching (used for IP CIDR blocks on Windows with npm lite enabled)

SrcDirectIPs []string

// DstDirectIPs holds direct IPs for destination matching (used for IP CIDR blocks on Windows with npm lite enabled)

Copilot · 2025-11-26T20:00:49Z

npm/pkg/controlplane/translation/translatePolicy.go

+func directPeerAndPortAllowRule(npmNetPol *policies.NPMNetworkPolicy, direction policies.Direction, ports []networkingv1.NetworkPolicyPort, cidr string, npmLiteToggle bool) error {
+	if len(ports) == 0 {
+		acl := policies.NewACLPolicy(policies.Allowed, direction)
+		// bypasses ipset creation for /32 cidrs and directly creates an acl with the cidr


The comment states "bypasses ipset creation for /32 cidrs" but this function is used for all CIDR blocks when npm lite is enabled, not just /32 CIDRs. The test cases demonstrate this with /16, /24, and /28 subnets. Please update the comment to reflect the actual behavior:

// bypasses ipset creation for IP CIDR blocks and directly creates an acl with the cidr

Suggested change

// bypasses ipset creation for /32 cidrs and directly creates an acl with the cidr

// bypasses ipset creation for IP CIDR blocks and directly creates an acl with the cidr

Copilot · 2025-11-26T20:00:49Z

npm/pkg/controlplane/translation/translatePolicy_test.go

+		{
+			name:      "egress tcp port 8000-8100 with /28 subnet",
+			direction: policies.Egress,
+			ports: []networkingv1.NetworkPolicyPort{
+				{
+					Protocol: &tcp,
+					Port:     &port8000,
+					EndPort:  &endPort,
+				},
+			},
+			cidr: "10.0.1.0/28",
+			npmNetPol: &policies.NPMNetworkPolicy{
+				Namespace:   defaultNS,
+				PolicyKey:   namedPortPolicyKey,
+				ACLPolicyID: fmt.Sprintf("azure-acl-%s-%s", defaultNS, namedPortPolicyKey),
+				ACLs: []*policies.ACLPolicy{
+					{
+						Target:       policies.Allowed,
+						Direction:    policies.Egress,
+						DstDirectIPs: []string{"10.0.1.0/28"},
+						DstPorts: policies.Ports{
+							Port:    8000,
+							EndPort: 8100,
+						},
+						Protocol: "TCP",
+					},
+				},
+			},
+		},
+		{


This test case is duplicated. The exact same test case "egress tcp port 8000-8100 with /28 subnet" appears twice in the test table (lines 1476-1503 and lines 1559-1586). Please remove one of the duplicate test cases to maintain test clarity and avoid redundant test execution.

Suggested change

{

name: "egress tcp port 8000-8100 with /28 subnet",

direction: policies.Egress,

ports: []networkingv1.NetworkPolicyPort{

{

Protocol: &tcp,

Port: &port8000,

EndPort: &endPort,

},

},

cidr: "10.0.1.0/28",

npmNetPol: &policies.NPMNetworkPolicy{

Namespace: defaultNS,

PolicyKey: namedPortPolicyKey,

ACLPolicyID: fmt.Sprintf("azure-acl-%s-%s", defaultNS, namedPortPolicyKey),

ACLs: []*policies.ACLPolicy{

{

Target: policies.Allowed,

Direction: policies.Egress,

DstDirectIPs: []string{"10.0.1.0/28"},

DstPorts: policies.Ports{

Port: 8000,

EndPort: 8100,

},

Protocol: "TCP",

},

},

},

},

{

{

Copilot · 2025-11-26T20:00:49Z

npm/pkg/dataplane/policies/policy_windows.go

+		policySettings.LocalAddresses = ""
+		if policySettings.Direction == hcn.DirectionTypeOut {
+			// EGRESS: Remote = Destination IPs from policy
+			policySettings.RemoteAddresses = dstListStr
+		} else {
+			// INGRESS: Remote = Source IPs from policy
+			policySettings.RemoteAddresses = srcListStr


Setting LocalAddresses to empty string for all direct IP cases may be incorrect. According to the comments at lines 117-123, for EGRESS traffic with direct IPs, LocalAddresses should be set to Destination IPs, not an empty string. The current logic always sets LocalAddresses to "" and only sets RemoteAddresses based on direction, which doesn't match the documented behavior for EGRESS.

Based on the comments:

For INGRESS with direct IPs: LocalAddresses = Source IPs, RemoteAddresses = Destination IPs

For EGRESS with direct IPs: LocalAddresses = Destination IPs, RemoteAddresses = Source IPs

Suggested fix:

if policySettings.Direction == hcn.DirectionTypeOut { // EGRESS: Local = Destination IPs, Remote = Source IPs policySettings.LocalAddresses = dstListStr policySettings.RemoteAddresses = srcListStr } else { // INGRESS: Local = Source IPs, Remote = Destination IPs policySettings.LocalAddresses = srcListStr policySettings.RemoteAddresses = dstListStr }

Suggested change

policySettings.LocalAddresses = ""

if policySettings.Direction == hcn.DirectionTypeOut {

// EGRESS: Remote = Destination IPs from policy

policySettings.RemoteAddresses = dstListStr

} else {

// INGRESS: Remote = Source IPs from policy

policySettings.RemoteAddresses = srcListStr

if policySettings.Direction == hcn.DirectionTypeOut {

// EGRESS: Local = Destination IPs, Remote = Source IPs

policySettings.LocalAddresses = dstListStr

policySettings.RemoteAddresses = srcListStr

} else {

// INGRESS: Local = Source IPs, Remote = Destination IPs

policySettings.LocalAddresses = srcListStr

policySettings.RemoteAddresses = dstListStr

Copilot AI review requested due to automatic review settings November 26, 2025 19:51

rejain789 requested a review from a team as a code owner November 26, 2025 19:51

rejain789 requested a review from vakalapa November 26, 2025 19:51

Copilot started reviewing on behalf of rejain789 November 26, 2025 19:52 View session

Copilot finished reviewing on behalf of rejain789 November 26, 2025 19:55

Copilot AI reviewed Nov 26, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[NPM Lite] Bypassing IPSets for IP CIDR Block Based Network Policies … #4147

[NPM Lite] Bypassing IPSets for IP CIDR Block Based Network Policies … #4147

rejain789 commented Nov 26, 2025

Uh oh!

rejain789 commented Nov 26, 2025

Uh oh!

azure-pipelines bot commented Nov 26, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Nov 26, 2025

Uh oh!

Copilot AI Nov 26, 2025

Uh oh!

Copilot AI Nov 26, 2025

Uh oh!

Copilot AI Nov 26, 2025

Uh oh!

Copilot AI Nov 26, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	// bypasses ipset creation for /32 cidrs and directly creates an acl with the cidr
	// bypasses ipset creation for IP CIDR blocks and directly creates an acl with the cidr

[NPM Lite] Bypassing IPSets for IP CIDR Block Based Network Policies … #4147

Are you sure you want to change the base?

[NPM Lite] Bypassing IPSets for IP CIDR Block Based Network Policies … #4147

Conversation

rejain789 commented Nov 26, 2025

Uh oh!

rejain789 commented Nov 26, 2025

Uh oh!

azure-pipelines bot commented Nov 26, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants