[NPM Lite] Bypassing IPSets for IP CIDR Block Based Network Policies #4107
Conversation
rejain789
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR enhances NPM Lite mode on Windows to bypass IPSet creation for CIDR-based network policies by introducing direct IP address handling. When NPM Lite is enabled, the system now stores CIDR blocks directly in ACL policies rather than creating IPSets, improving efficiency for CIDR-only policies.
Key changes:
- Added
SrcDirectIPsandDstDirectIPsfields toACLPolicyfor direct CIDR storage - Introduced
directPeerAndPortRule()function to handle CIDR-based policies without IPSet creation - Updated Windows ACL conversion logic to correctly map direct IPs to HNS Local/Remote addresses based on direction
- Removed validation function
npmLiteValidPolicy()and its associated testTestNpmLiteCidrPolicy()
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| npm/pkg/dataplane/policies/policy.go | Added SrcDirectIPs and DstDirectIPs fields to ACLPolicy struct with documentation; reformatted comment example |
| npm/pkg/dataplane/policies/policy_windows.go | Modified convertToAclSettings() to handle direct IP addresses for NPM Lite, with conditional logic for Local/Remote address mapping based on traffic direction |
| npm/pkg/controlplane/translation/translatePolicy.go | Added directPeerAndPortRule() function for direct CIDR handling; removed npmLiteValidPolicy() validation function; updated translateRule() to use direct CIDR path when NPM Lite is enabled |
| npm/pkg/controlplane/translation/translatePolicy_test.go | Removed TestNpmLiteCidrPolicy() test covering CIDR validation with npm lite |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if portKind == namedPortType { | ||
| return ErrUnsupportedNamedPort | ||
| } |
There was a problem hiding this comment.
The check for namedPortType is redundant since checkForNamedPortType() already validates this on line 384 and would return an error. This unreachable code should be removed.
| if portKind == namedPortType { | |
| return ErrUnsupportedNamedPort | |
| } |
| } | ||
| dstPortStr := getPortStrFromPorts(acl.DstPorts) | ||
|
|
||
| // HNS has confusing Local and Remote address defintions |
There was a problem hiding this comment.
Corrected spelling of 'defintions' to 'definitions'.
| // HNS has confusing Local and Remote address defintions | |
| // HNS has confusing Local and Remote address definitions |
rejain789
changed the title
| dstListStr := getAddrListFromSetInfo(acl.DstList) | ||
| var srcListStr, dstListStr string | ||
| // Check if we have direct IPs (NPM Lite /32 bypass) | ||
| if len(acl.SrcDirectIPs) > 0 || len(acl.DstDirectIPs) > 0 { |
There was a problem hiding this comment.
this check is also done on line 139, is it possible to converge into the same block?
| if npmLiteToggle { | ||
| err = directPeerAndPortRule(npmNetPol, direction, ports, peer.IPBlock.CIDR, npmLiteToggle) | ||
| if err != nil { | ||
| return err |
|
|
||
| // Handle ports | ||
| if portKind == namedPortType { | ||
| return ErrUnsupportedNamedPort |
There was a problem hiding this comment.
I would also wrap this with some additional context from the npmNetPol in case you're doing a trace later on
| return nil | ||
| } | ||
|
|
||
| func directPeerAndPortRule(npmNetPol *policies.NPMNetworkPolicy, direction policies.Direction, ports []networkingv1.NetworkPolicyPort, cidr string, npmLiteToggle bool) error { |
There was a problem hiding this comment.
probably include Allow somewhere in this func to be extra explicit
Reason for Change:
This pr is solely for when NPM Lite is enabled. It adds logic to bypass IPSet creations and directly passes in IP cidr blocks to the acl's which is then sent to HNS. This bypass will take place with default deny and npm lite enabled
Issue Fixed:
Manual Testing
Test Setup
Created a network policy as shown below:
Applied the policy and show the 2 allow in/out acl's created with ip cidrs instead of ipsets in hns as shown below:
Requirements:
Notes: