From 854eb9de368b2ddad6fa8daf06341d16455d2a4b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 15 Aug 2025 20:06:33 +0000 Subject: [PATCH 1/2] Initial plan From 550242b564ee7452f2d2ad3b2c0669ff0853ef4a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 15 Aug 2025 20:32:50 +0000 Subject: [PATCH 2/2] Implement Windows NPM Lite direct CIDR support: bypass IPSets for CIDR blocks Co-authored-by: rejain456 <155685406+rejain456@users.noreply.github.com> --- .../translation/translatePolicy.go | 20 ++++- .../translation/translatePolicy_test.go | 2 +- .../translation/windows_npm_lite_test.go | 79 +++++++++++++++++++ npm/pkg/dataplane/policies/policy.go | 22 +++++- npm/pkg/dataplane/policies/policy_windows.go | 47 ++++++++--- .../policies/policymanager_linux_test.go | 35 ++++---- npm/pkg/dataplane/policies/testutils.go | 35 ++++---- .../policies/windows_npm_lite_test.go | 42 ++++++++++ 8 files changed, 238 insertions(+), 44 deletions(-) create mode 100644 npm/pkg/controlplane/translation/windows_npm_lite_test.go create mode 100644 npm/pkg/dataplane/policies/windows_npm_lite_test.go diff --git a/npm/pkg/controlplane/translation/translatePolicy.go b/npm/pkg/controlplane/translation/translatePolicy.go index 9b029b4616..6393a93891 100644 --- a/npm/pkg/controlplane/translation/translatePolicy.go +++ b/npm/pkg/controlplane/translation/translatePolicy.go @@ -223,8 +223,10 @@ func ipBlockIPSet(policyName, ns string, direction policies.Direction, ipBlockSe // ipBlockRule translates IPBlock field in networkpolicy object to translatedIPSet and SetInfo. // ipBlockSetIndex parameter is used to diffentiate ipBlock fields in one networkpolicy object. +// For Windows NPM Lite, when npmLiteToggle is true, this bypasses IPSet creation and returns +// SetInfo with direct CIDR values instead. func ipBlockRule(policyName, ns string, direction policies.Direction, matchType policies.MatchType, ipBlockSetIndex, ipBlockPeerIndex int, - ipBlockRule *networkingv1.IPBlock, + ipBlockRule *networkingv1.IPBlock, npmLiteToggle bool, ) (*ipsets.TranslatedIPSet, policies.SetInfo, error) { //nolint // gofumpt if ipBlockRule == nil || ipBlockRule.CIDR == "" { return nil, policies.SetInfo{}, nil @@ -234,6 +236,15 @@ func ipBlockRule(policyName, ns string, direction policies.Direction, matchType return nil, policies.SetInfo{}, ErrUnsupportedIPAddress } + // For Windows NPM Lite, bypass IPSet creation and use direct CIDR values + if util.IsWindowsDP() && npmLiteToggle { + // Create SetInfo with direct CIDR values, no IPSet needed + cidrs := []string{ipBlockRule.CIDR} + setInfo := policies.NewSetInfoWithCIDRs(cidrs, included, matchType) + return nil, setInfo, nil // No IPSet needed for Windows NPM Lite + } + + // Traditional path for Linux or Windows without NPM Lite: create IPSet ipBlockIPSet, err := ipBlockIPSet(policyName, ns, direction, ipBlockSetIndex, ipBlockPeerIndex, ipBlockRule) if err != nil { return nil, policies.SetInfo{}, err @@ -405,11 +416,14 @@ func translateRule(npmNetPol *policies.NPMNetworkPolicy, // #2.1 Handle IPBlock and port if exist if peer.IPBlock != nil { if len(peer.IPBlock.CIDR) > 0 { - ipBlockIPSet, ipBlockSetInfo, err := ipBlockRule(netPolName, npmNetPol.Namespace, direction, matchType, ruleIndex, peerIdx, peer.IPBlock) + ipBlockIPSet, ipBlockSetInfo, err := ipBlockRule(netPolName, npmNetPol.Namespace, direction, matchType, ruleIndex, peerIdx, peer.IPBlock, npmLiteToggle) if err != nil { return err } - npmNetPol.RuleIPSets = append(npmNetPol.RuleIPSets, ipBlockIPSet) + // Only add IPSet to RuleIPSets if one was created (not for Windows NPM Lite) + if ipBlockIPSet != nil { + npmNetPol.RuleIPSets = append(npmNetPol.RuleIPSets, ipBlockIPSet) + } err = peerAndPortRule(npmNetPol, direction, ports, []policies.SetInfo{ipBlockSetInfo}, npmLiteToggle) if err != nil { diff --git a/npm/pkg/controlplane/translation/translatePolicy_test.go b/npm/pkg/controlplane/translation/translatePolicy_test.go index dc49c7bec3..5c1649ce08 100644 --- a/npm/pkg/controlplane/translation/translatePolicy_test.go +++ b/npm/pkg/controlplane/translation/translatePolicy_test.go @@ -567,7 +567,7 @@ func TestIPBlockRule(t *testing.T) { tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() - translatedIPSet, setInfo, err := ipBlockRule(tt.policyName, tt.namemspace, tt.direction, tt.matchType, tt.ipBlockSetIndex, tt.ipBlockPeerIndex, tt.ipBlockRule) + translatedIPSet, setInfo, err := ipBlockRule(tt.policyName, tt.namemspace, tt.direction, tt.matchType, tt.ipBlockSetIndex, tt.ipBlockPeerIndex, tt.ipBlockRule, false) if tt.skipWindows && util.IsWindowsDP() { require.Error(t, err) } else { diff --git a/npm/pkg/controlplane/translation/windows_npm_lite_test.go b/npm/pkg/controlplane/translation/windows_npm_lite_test.go new file mode 100644 index 0000000000..60f5fa6515 --- /dev/null +++ b/npm/pkg/controlplane/translation/windows_npm_lite_test.go @@ -0,0 +1,79 @@ +package translation + +import ( + "testing" + + "github.com/Azure/azure-container-networking/npm/pkg/dataplane/ipsets" + "github.com/Azure/azure-container-networking/npm/pkg/dataplane/policies" + "github.com/Azure/azure-container-networking/npm/util" + "github.com/stretchr/testify/require" + networkingv1 "k8s.io/api/networking/v1" +) + +func TestWindowsNPMLiteCIDRDirect(t *testing.T) { + // Skip test on Linux since this is Windows-specific functionality + if !util.IsWindowsDP() { + t.Skip("Skipping Windows NPM Lite test on non-Windows platform") + } + + tests := []struct { + name string + npmLiteToggle bool + ipBlock *networkingv1.IPBlock + expectedCIDRs []string + expectIPSet bool + }{ + { + name: "Windows NPM Lite with CIDR - should use direct CIDR", + npmLiteToggle: true, + ipBlock: &networkingv1.IPBlock{ + CIDR: "192.168.1.0/24", + }, + expectedCIDRs: []string{"192.168.1.0/24"}, + expectIPSet: false, + }, + { + name: "Windows traditional mode - should create IPSet", + npmLiteToggle: false, + ipBlock: &networkingv1.IPBlock{ + CIDR: "192.168.1.0/24", + }, + expectedCIDRs: nil, + expectIPSet: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + translatedIPSet, setInfo, err := ipBlockRule( + "test-policy", + "test-namespace", + policies.Ingress, + policies.SrcMatch, + 0, // ipBlockSetIndex + 0, // ipBlockPeerIndex + tt.ipBlock, + tt.npmLiteToggle, + ) + + require.NoError(t, err) + + if tt.expectIPSet { + // Traditional mode: should have IPSet + require.NotNil(t, translatedIPSet, "Expected IPSet to be created in traditional mode") + require.NotNil(t, setInfo.IPSet, "Expected SetInfo to have IPSet reference") + require.Equal(t, ipsets.CIDRBlocks, setInfo.IPSet.Type) + require.Empty(t, setInfo.CIDRs, "Expected no direct CIDRs in traditional mode") + } else { + // NPM Lite mode: should have direct CIDRs, no IPSet + require.Nil(t, translatedIPSet, "Expected no IPSet to be created in NPM Lite mode") + require.Nil(t, setInfo.IPSet, "Expected no IPSet reference in NPM Lite mode") + require.Equal(t, tt.expectedCIDRs, setInfo.CIDRs, "Expected direct CIDRs in NPM Lite mode") + } + + // Common checks + require.True(t, setInfo.Included, "Expected SetInfo to be included") + require.Equal(t, policies.SrcMatch, setInfo.MatchType, "Expected correct match type") + }) + } +} \ No newline at end of file diff --git a/npm/pkg/dataplane/policies/policy.go b/npm/pkg/dataplane/policies/policy.go index 646a03633a..5eee12f6c8 100644 --- a/npm/pkg/dataplane/policies/policy.go +++ b/npm/pkg/dataplane/policies/policy.go @@ -285,10 +285,15 @@ func translatedIPSetsToString(items []*ipsets.TranslatedIPSet) string { // ! azure-npm-123 src // "!" this indicates a negative match (Included is false) of an azure-npm-123 // MatchType is "src" +// For Windows NPM Lite with CIDR blocks, CIDRs field can contain direct CIDR values +// to bypass IPSet creation entirely. type SetInfo struct { IPSet *ipsets.IPSetMetadata Included bool MatchType MatchType + // CIDRs holds direct CIDR values for Windows NPM Lite to bypass IPSet creation. + // When this field is populated, IPSet field may be nil or used for metadata only. + CIDRs []string } // Ports represents a range of ports. @@ -304,8 +309,23 @@ func NewSetInfo(name string, setType ipsets.SetType, included bool, matchType Ma } } +// NewSetInfoWithCIDRs creates SetInfo with direct CIDR values for Windows NPM Lite. +// This bypasses IPSet creation entirely for CIDR blocks. +func NewSetInfoWithCIDRs(cidrs []string, included bool, matchType MatchType) SetInfo { + return SetInfo{ + IPSet: nil, // No IPSet needed for direct CIDR approach + Included: included, + MatchType: matchType, + CIDRs: cidrs, + } +} + func (info SetInfo) PrettyString() string { - return fmt.Sprintf("Name:%s HashedName:%s MatchType:%v Included:%v", info.IPSet.GetPrefixName(), info.IPSet.GetHashedName(), info.MatchType, info.Included) + if info.IPSet != nil { + return fmt.Sprintf("Name:%s HashedName:%s MatchType:%v Included:%v", info.IPSet.GetPrefixName(), info.IPSet.GetHashedName(), info.MatchType, info.Included) + } + // For direct CIDR SetInfo (Windows NPM Lite) + return fmt.Sprintf("CIDRs:%v MatchType:%v Included:%v", info.CIDRs, info.MatchType, info.Included) } type Ports struct { diff --git a/npm/pkg/dataplane/policies/policy_windows.go b/npm/pkg/dataplane/policies/policy_windows.go index 5f4fbd16ff..32096b0be7 100644 --- a/npm/pkg/dataplane/policies/policy_windows.go +++ b/npm/pkg/dataplane/policies/policy_windows.go @@ -3,6 +3,7 @@ package policies import ( "errors" "fmt" + "strings" "github.com/Azure/azure-container-networking/npm/pkg/dataplane/ipsets" "github.com/Microsoft/hcsshim/hcn" @@ -142,7 +143,16 @@ func (acl *ACLPolicy) convertToAclSettings(aclID string) (*NPMACLPolSettings, er func (acl *ACLPolicy) checkIPSets() bool { for _, set := range acl.SrcList { - if set.IPSet.Type == ipsets.NamedPorts { + // For Windows NPM Lite with direct CIDRs, skip IPSet checks + if len(set.CIDRs) > 0 { + if !set.hasKnownMatchType() { + return false + } + continue + } + + // Traditional IPSet checks + if set.IPSet != nil && set.IPSet.Type == ipsets.NamedPorts { return false } @@ -151,7 +161,16 @@ func (acl *ACLPolicy) checkIPSets() bool { } } for _, set := range acl.DstList { - if set.IPSet.Type == ipsets.NamedPorts { + // For Windows NPM Lite with direct CIDRs, skip IPSet checks + if len(set.CIDRs) > 0 { + if !set.hasKnownMatchType() { + return false + } + continue + } + + // Traditional IPSet checks + if set.IPSet != nil && set.IPSet.Type == ipsets.NamedPorts { return false } @@ -163,16 +182,26 @@ func (acl *ACLPolicy) checkIPSets() bool { } func getAddrListFromSetInfo(setInfoList []SetInfo) string { - setInfoStr := "" - setInfoLen := len(setInfoList) - for i, setInfo := range setInfoList { - if i < setInfoLen-1 { - setInfoStr += setInfo.IPSet.GetHashedName() + "," + var addrValues []string + + for _, setInfo := range setInfoList { + var addrValue string + // For Windows NPM Lite with direct CIDR values, use the CIDR directly + if len(setInfo.CIDRs) > 0 { + // Join multiple CIDRs with comma + addrValue = strings.Join(setInfo.CIDRs, ",") + } else if setInfo.IPSet != nil { + // Traditional IPSet approach: use hashed name + addrValue = setInfo.IPSet.GetHashedName() } else { - setInfoStr += setInfo.IPSet.GetHashedName() + // Skip entries with neither CIDRs nor IPSet + continue } + + addrValues = append(addrValues, addrValue) } - return setInfoStr + + return strings.Join(addrValues, ",") } func getPortStrFromPorts(port Ports) string { diff --git a/npm/pkg/dataplane/policies/policymanager_linux_test.go b/npm/pkg/dataplane/policies/policymanager_linux_test.go index 37f5ca356b..f13958bd73 100644 --- a/npm/pkg/dataplane/policies/policymanager_linux_test.go +++ b/npm/pkg/dataplane/policies/policymanager_linux_test.go @@ -19,14 +19,16 @@ var ( ingressDeniedACL = &ACLPolicy{ SrcList: []SetInfo{ { - ipsets.TestCIDRSet.Metadata, - true, - SrcMatch, + IPSet: ipsets.TestCIDRSet.Metadata, + Included: true, + MatchType: SrcMatch, + CIDRs: nil, }, { - ipsets.TestKeyPodSet.Metadata, - false, - DstMatch, + IPSet: ipsets.TestKeyPodSet.Metadata, + Included: false, + MatchType: DstMatch, + CIDRs: nil, }, }, Target: Dropped, @@ -39,9 +41,10 @@ var ( ingressAllowedACL = &ACLPolicy{ SrcList: []SetInfo{ { - ipsets.TestCIDRSet.Metadata, - true, - SrcMatch, + IPSet: ipsets.TestCIDRSet.Metadata, + Included: true, + MatchType: SrcMatch, + CIDRs: nil, }, }, Target: Allowed, @@ -51,9 +54,10 @@ var ( egressDeniedACL = &ACLPolicy{ DstList: []SetInfo{ { - ipsets.TestCIDRSet.Metadata, - true, - DstMatch, + IPSet: ipsets.TestCIDRSet.Metadata, + Included: true, + MatchType: DstMatch, + CIDRs: nil, }, }, Target: Dropped, @@ -64,9 +68,10 @@ var ( egressAllowedACL = &ACLPolicy{ DstList: []SetInfo{ { - ipsets.TestNamedportSet.Metadata, - true, - DstMatch, + IPSet: ipsets.TestNamedportSet.Metadata, + Included: true, + MatchType: DstMatch, + CIDRs: nil, }, }, Target: Allowed, diff --git a/npm/pkg/dataplane/policies/testutils.go b/npm/pkg/dataplane/policies/testutils.go index aab0f98140..3b59cab803 100644 --- a/npm/pkg/dataplane/policies/testutils.go +++ b/npm/pkg/dataplane/policies/testutils.go @@ -72,16 +72,18 @@ var ( Comment: "comment1", SrcList: []SetInfo{ { - ipsets.TestCIDRSet.Metadata, - true, - SrcMatch, + IPSet: ipsets.TestCIDRSet.Metadata, + Included: true, + MatchType: SrcMatch, + CIDRs: nil, }, }, DstList: []SetInfo{ { - ipsets.TestKeyPodSet.Metadata, - false, - DstMatch, + IPSet: ipsets.TestKeyPodSet.Metadata, + Included: false, + MatchType: DstMatch, + CIDRs: nil, }, }, Target: Dropped, @@ -95,9 +97,10 @@ var ( Comment: "comment2", SrcList: []SetInfo{ { - ipsets.TestCIDRSet.Metadata, - true, - SrcMatch, + IPSet: ipsets.TestCIDRSet.Metadata, + Included: true, + MatchType: SrcMatch, + CIDRs: nil, }, }, Target: Allowed, @@ -108,9 +111,10 @@ var ( Comment: "comment3", SrcList: []SetInfo{ { - ipsets.TestCIDRSet.Metadata, - true, - SrcMatch, + IPSet: ipsets.TestCIDRSet.Metadata, + Included: true, + MatchType: SrcMatch, + CIDRs: nil, }, }, Target: Dropped, @@ -124,9 +128,10 @@ var ( Comment: "comment4", SrcList: []SetInfo{ { - ipsets.TestCIDRSet.Metadata, - true, - SrcMatch, + IPSet: ipsets.TestCIDRSet.Metadata, + Included: true, + MatchType: SrcMatch, + CIDRs: nil, }, }, Target: Allowed, diff --git a/npm/pkg/dataplane/policies/windows_npm_lite_test.go b/npm/pkg/dataplane/policies/windows_npm_lite_test.go new file mode 100644 index 0000000000..0bba2ddc49 --- /dev/null +++ b/npm/pkg/dataplane/policies/windows_npm_lite_test.go @@ -0,0 +1,42 @@ +package policies + +import ( + "testing" + + "github.com/Azure/azure-container-networking/npm/pkg/dataplane/ipsets" + "github.com/stretchr/testify/require" +) + +func TestNewSetInfoWithCIDRs(t *testing.T) { + cidrs := []string{"192.168.1.0/24", "10.0.0.0/8"} + included := true + matchType := SrcMatch + + setInfo := NewSetInfoWithCIDRs(cidrs, included, matchType) + + require.Nil(t, setInfo.IPSet, "Expected IPSet to be nil for direct CIDR SetInfo") + require.Equal(t, cidrs, setInfo.CIDRs, "Expected CIDRs to match input") + require.Equal(t, included, setInfo.Included, "Expected Included flag to match") + require.Equal(t, matchType, setInfo.MatchType, "Expected MatchType to match") +} + +func TestSetInfoPrettyString_WithCIDRs(t *testing.T) { + setInfo := NewSetInfoWithCIDRs([]string{"192.168.1.0/24", "10.0.0.0/8"}, true, SrcMatch) + + result := setInfo.PrettyString() + + require.Contains(t, result, "192.168.1.0/24", "Expected CIDR to be in pretty string") + require.Contains(t, result, "10.0.0.0/8", "Expected CIDR to be in pretty string") + require.Contains(t, result, "0", "Expected match type (0=SrcMatch) to be in pretty string") + require.Contains(t, result, "true", "Expected included flag to be in pretty string") +} + +func TestSetInfoPrettyString_WithIPSet(t *testing.T) { + setInfo := NewSetInfo("test-ipset", ipsets.CIDRBlocks, true, SrcMatch) + + result := setInfo.PrettyString() + + require.Contains(t, result, "test-ipset", "Expected IPSet name to be in pretty string") + require.Contains(t, result, "0", "Expected match type (0=SrcMatch) to be in pretty string") + require.Contains(t, result, "true", "Expected included flag to be in pretty string") +} \ No newline at end of file