Npm Lite Feature #3005

rejain456 · 2024-09-10T22:45:14Z

Reason for Change:

This pr adds code changes for a new feature called NPM-Lite which is a subset of NPM.
NPM Lite provides Singularity team with a “light” NPM which addresses:

Heavy load on API Server: NPM Lite must support ~4K Nodes (NPM currently supports ~250 Nodes).
Windows NPM performance issues caused by HNS SetPolicies.
Issue Fixed:

npm lite consists of 2 main limitations:

Only accepts CIDR-based network policies
Enables the daemon set to watch for pods only under its node

This pr adds code to cover both the points above

Testing

Issue Fixed:

Requirements:

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

Notes:

rejain456 · 2024-09-10T22:45:28Z

/azp run NPM Conformance Tests

rejain456 · 2024-09-10T22:45:34Z

/azp run NPM Scale Test

azure-pipelines · 2024-09-10T22:45:39Z

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines · 2024-09-10T22:45:46Z

Azure Pipelines successfully started running 1 pipeline(s).

rejain456 · 2024-09-10T22:45:48Z

/azp run Azure Container Networking PR

azure-pipelines · 2024-09-10T22:45:57Z

Azure Pipelines successfully started running 1 pipeline(s).

huntergregory

Realizing an edge case, and we'll have to be careful with reading in the config.

Also for testing, could you add to the PR description steps or screenshots for how you verified that a non-CIDR policy throws an error, and how you verified that applying a CIDR policy correctly blocks / allows traffic?

huntergregory · 2024-09-11T17:38:21Z

npm/pkg/controlplane/translation/translatePolicy.go

@@ -387,6 +390,8 @@ func translateRule(npmNetPol *policies.NPMNetworkPolicy, netPolName string, dire
 		}
 	}

+	cfg := npmconfig.DefaultConfig


We'll have to use the config read in at runtime (read in from the k8s ConfigMap). That config is read in here:

azure-container-networking/npm/cmd/start.go

Lines 43 to 68 in 47b4329

func newStartNPMCmd() *cobra.Command {

// getTuplesCmd represents the getTuples command

startNPMCmd := &cobra.Command{

Use: "start",

Short: "Starts the Azure NPM process",

RunE: func(cmd *cobra.Command, args []string) error {

config := &npmconfig.Config{}

err := viper.Unmarshal(config)

if err != nil {

return fmt.Errorf("failed to load config with error: %w", err)

}

flags := npmconfig.Flags{

KubeConfigPath: viper.GetString(flagKubeConfigPath),

}

return start(*config, flags)

},

}

startNPMCmd.Flags().String(flagKubeConfigPath, flagDefaults[flagKubeConfigPath], "path to kubeconfig")

return startNPMCmd

}

func start(config npmconfig.Config, flags npmconfig.Flags) error {

It looks like we'll have to pass the NPMLite toggle down from start() -> NetPol controller -> translation

changed it to reflect this, thank you!

QQ: @huntergregory , What is the benefit of reading the config in at runtime only?

huntergregory · 2024-09-11T17:58:04Z

npm/pkg/controlplane/translation/translatePolicy.go

@@ -403,11 +408,24 @@ func translateRule(npmNetPol *policies.NPMNetworkPolicy, netPolName string, dire
 					return err
 				}
 			}
+
+			// if npm lite is configured, check network policy only consists of CIDR blocks
+			err := NpmLiteValidPolicy(peer, NpmLiteEnabled)


Right now we're letting NPM Lite create "allow-all-internal" policies ~~(no peers)~~. That uses the all-namespaces list ipset, which will be inaccurate for NPM Lite.

Realizing we should validate the following for NPM Lite: policy must have 1+ peers and each rule must have an IpBlock but no selectors.

Could we simplify that check to one location in this function instead of two?

Right now we're letting NPM Lite create "allow-all-internal" policies ~~(no peers)~~

Correction: this code has the "allow-all-internal" part:

azure-container-networking/npm/pkg/controlplane/translation/translatePolicy.go

Lines 364 to 374 in 47b4329

// #0. TODO(jungukcho): cannot come up when this condition is met.

// The code inside if condition is to handle allowing all internal traffic, but the case is handled in #2.4.

// So, this code may not execute. After confirming this, need to delete it.

if !portRuleExists && !peerRuleExists && !allowExternal {

acl := policies.NewACLPolicy(policies.Allowed, direction)

ruleIPSets, allowAllInternalSetInfo := allowAllInternal(matchType)

npmNetPol.RuleIPSets = append(npmNetPol.RuleIPSets, ruleIPSets)

acl.AddSetInfo([]policies.SetInfo{allowAllInternalSetInfo})

npmNetPol.ACLs = append(npmNetPol.ACLs, acl)

return nil

}

It seems like the if statement could be taken if it's possible to have no port rules, 1+ peers, and peers are empty (ipblock + selectors are nil)

azure-container-networking/npm/pkg/controlplane/translation/translatePolicy.go

Lines 309 to 331 in 47b4329

func ruleExists(ports []networkingv1.NetworkPolicyPort, peer []networkingv1.NetworkPolicyPeer) (allowExternal, portRuleExists, peerRuleExists bool) {

// TODO(jungukcho): need to clarify and summarize below flags + more comments

portRuleExists = len(ports) > 0

if peer != nil {

if len(peer) == 0 {

peerRuleExists = true

allowExternal = true

}

for _, peerRule := range peer {

if peerRule.PodSelector != nil ||

peerRule.NamespaceSelector != nil ||

peerRule.IPBlock != nil {

peerRuleExists = true

break

}

}

} else if !portRuleExists {

allowExternal = true

}

return allowExternal, portRuleExists, peerRuleExists

}

"No peers" actually corresponds to "default deny all" or "default allow all" I believe?
#3005 (comment)

Re-factored so we call the validate function only once. @huntergregory , plase let me know if that looks better

npm/pkg/controlplane/translation/translatePolicy.go

huntergregory · 2024-09-11T18:01:05Z

npm/pkg/controlplane/translation/translatePolicy_test.go

+		wantErr        bool
+	}{
+		{
+			name:           "peer nameSpaceSelector and ipblock in ingress rules",


Thanks for writing tests. Could we add these cases as well?

single peer with both ipblock and selector

no peers

allow-all-internal case if relevant? https://github.com/Azure/azure-container-networking/pull/3005/files#r1755255374

This is 3 peers:

[]networkingv1.NetworkPolicyPeer{ { NamespaceSelector: &metav1.LabelSelector{ MatchLabels: map[string]string{ "peer-nsselector-kay": "peer-nsselector-value", }, }, }, { IPBlock: &networkingv1.IPBlock{ CIDR: "172.17.0.0/16", Except: []string{"172.17.1.0/24", "172.17.2.0/24"}, }, }, { IPBlock: &networkingv1.IPBlock{ CIDR: "172.17.0.0/16", }, }, },

but you can have a pod selector and a namespace selector in the same Peer.

Each peer corresponds to an element of the YAML's list (more intuitively, a "section" denoted by a -) in a NetworkPolicy spec, where each peer/element is OR'd together. Within a peer, the components are AND'd. For more details, see "namespaceSelector and podSelector" under https://kubernetes.io/docs/concepts/services-networking/network-policies/#behavior-of-to-and-from-selectors

single peer with both ipblock and selector

It sounds like this is actually impossible from the docs though. It is not one of the 4 kinds of to/from selectors (peers). Do you mind verifying that it's impossible to create such a NetPol peer with ipblock and pod selector?

Added test cases for:

CIDR + label selector combo in same peer

no peers

for allow-all-internal case, added an exception check within the code for it

huntergregory · 2024-09-11T18:05:37Z

npm/pkg/controlplane/translation/translatePolicy.go

@@ -34,6 +35,8 @@ var (
 	)
 	// ErrUnsupportedIPAddress is returned when an unsupported IP address, such as IPV6, is used
 	ErrUnsupportedIPAddress = errors.New("unsupported IP address")
+	// ErrUnsupportedNonCIDR is returned when non-CIDR blocks (i.e pod selectors or namespace selectors) are passed with a NPM Lite configuration
+	ErrUnsupportedNonCIDR = errors.New("unsupported Non-CIDR block for NPM Lite")


we're actually allowing default allow and default deny policies as well (in TranslatePolicy()). Could we modify this error description to express that?

Updated the description thanks

nit: could we modify the error description as well to help a user in case they try debugging this error in logs?

I believe this Lite mode will actually work for all Policies except those with an ingress/egress pod selector or namespace selector

EDIT: or named ports

updated the description

sorry, I mean inside the code 😅

haha got it! I updated it, @huntergregory let me know how that sounds

Also great point out for named ports, added logic to output an error in case of named ports as well

huntergregory

minor suggestions

huntergregory · 2024-09-13T22:58:52Z

npm/pkg/controlplane/translation/translatePolicy_test.go

@@ -2925,6 +2925,8 @@ func TestNpmLiteCidrPolicy(t *testing.T) {
 	// Test 1) Npm lite enabled, CIDR + Namespace label Peers, returns error
 	// Test 2) NPM lite disabled, CIDR + Namespace label Peers, returns no error
 	// Test 3) Npm Lite enabled, CIDR Peers , returns no error
+	// Test 4) NPM Lite enabled, Combination of CIDR + Label in same peer, returns an error
+	// test 5) NPM Lite enabled, no peer, returns no error


sorry to miss this earlier. Could we also test that NPM lite supports a policy without CIDRs or selectors, with a port + protocol specified

Yup added a test case with just port + protocol, it does not return an error @huntergregory

huntergregory · 2024-09-13T23:02:16Z

npm/pkg/controlplane/translation/translatePolicy.go

@@ -34,6 +35,8 @@ var (
 	)
 	// ErrUnsupportedIPAddress is returned when an unsupported IP address, such as IPV6, is used
 	ErrUnsupportedIPAddress = errors.New("unsupported IP address")
+	// ErrUnsupportedNonCIDR is returned when non-CIDR blocks (i.e pod selectors or namespace selectors) are passed with a NPM Lite configuration
+	ErrUnsupportedNonCIDR = errors.New("unsupported Non-CIDR block for NPM Lite")


nit: could we modify the error description as well to help a user in case they try debugging this error in logs?

I believe this Lite mode will actually work for all Policies except those with an ingress/egress pod selector or namespace selector

EDIT: or named ports

huntergregory

Realizing that NPM Lite cannot support "named port" fields... We will need to check for that too

huntergregory · 2024-09-13T23:40:51Z

npm/pkg/controlplane/translation/translatePolicy.go

@@ -34,6 +35,8 @@ var (
 	)
 	// ErrUnsupportedIPAddress is returned when an unsupported IP address, such as IPV6, is used
 	ErrUnsupportedIPAddress = errors.New("unsupported IP address")
+	// ErrUnsupportedNonCIDR is returned when non-CIDR blocks (i.e pod selectors or namespace selectors) are passed with a NPM Lite configuration
+	ErrUnsupportedNonCIDR = errors.New("unsupported Non-CIDR block for NPM Lite")


sorry, I mean inside the code 😅

npm/azure-npm.yaml

npm/cacheencoder.go

npm/npm.go

vakalapa · 2024-09-17T00:40:26Z

npm/config/config.go

@@ -51,6 +51,7 @@ var DefaultConfig = Config{
 		ApplyInBackground: true,
 		// NetPolInBackground is currently used in Linux to apply NetPol controller Add events in the background
 		NetPolInBackground: true,
+		EnableNPMLite:      true,


This default config should always be false, or else we will break existing deployments where the configmap is not explicitly set to false.

Yup I had just enabled it for testing purposes with the intent of disabling it before merging the pr in. I can disable it in the pr and enable locally for testing purposes if that is the better way @vakalapa

vakalapa · 2024-09-17T00:43:23Z

npm/pkg/dataplane/types_windows_test.go

@@ -425,7 +425,7 @@ func UpdatePolicy(policy *networkingv1.NetworkPolicy) *Action {

 // Do models policy updates in the NetworkPolicyController
 func (p *PolicyUpdateAction) Do(dp *DataPlane) error {
-	npmNetPol, err := translation.TranslatePolicy(p.Policy)
+	npmNetPol, err := translation.TranslatePolicy(p.Policy, false)


Do we need any test cases for this set to true ?

added npm lite feature

135ab53

rejain456 requested a review from a team as a code owner September 10, 2024 22:45

rejain456 requested a review from rayaisaiah September 10, 2024 22:45

rejain456 added 7 commits September 10, 2024 22:50

enabled npm lite flag

53e0daf

revert changes

cbbc904

fix yaml file

17a12af

added unit test to test cidr only network policies allowed

aa9b503

fixed comments

3781d3d

updated comments

de037c4

added space in comment

f768288

huntergregory requested changes Sep 11, 2024

View reviewed changes

rejain456 added 4 commits September 11, 2024 23:12

added test cases, fixed pr comments

fd183a0

fixed lint errors

f6b0441

fixed logic for pod informer

0d8c592

refactored logic

d75c5ea

huntergregory requested changes Sep 13, 2024

View reviewed changes

rejain456 added 2 commits September 13, 2024 23:03

updated variable

bf65936

updated unit test case and desciption

39ff18e

huntergregory requested changes Sep 13, 2024

View reviewed changes

rejain456 added 2 commits September 14, 2024 22:42

added logic to not allow named ports and updated error description

ba35fc3

added extra unit test cases

054c42f

huntergregory previously approved these changes Sep 16, 2024

View reviewed changes

vakalapa reviewed Sep 16, 2024

View reviewed changes

npm/azure-npm.yaml Outdated Show resolved Hide resolved

created a new azure-npm-lite file

eb131f9

rejain456 dismissed huntergregory’s stale review via eb131f9 September 16, 2024 23:10

vakalapa requested changes Sep 17, 2024

View reviewed changes

rejain456 added 8 commits September 17, 2024 06:10

resolve pr comments

038a8c6

added a new line

3cb759d

removed a new line

8d41c82

added a new line

5245d98

added a new line

d528958

added a new line

cf43519

fixed a lint formatting issue

069c1d5

seperate params into two lines - lint

3fb78ea

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Npm Lite Feature #3005

Npm Lite Feature #3005

rejain456 commented Sep 10, 2024 •

edited

Loading

rejain456 commented Sep 10, 2024

rejain456 commented Sep 10, 2024

azure-pipelines bot commented Sep 10, 2024

azure-pipelines bot commented Sep 10, 2024

rejain456 commented Sep 10, 2024

azure-pipelines bot commented Sep 10, 2024

huntergregory left a comment

huntergregory Sep 11, 2024

rejain456 Sep 11, 2024 •

edited

Loading

huntergregory Sep 11, 2024 •

edited

Loading

huntergregory Sep 11, 2024 •

edited

Loading

huntergregory Sep 11, 2024

rejain456 Sep 11, 2024

huntergregory Sep 11, 2024 •

edited

Loading

huntergregory Sep 11, 2024 •

edited

Loading

rejain456 Sep 11, 2024

huntergregory Sep 11, 2024

rejain456 Sep 11, 2024

huntergregory Sep 13, 2024 •

edited

Loading

rejain456 Sep 13, 2024

huntergregory Sep 13, 2024

rejain456 Sep 14, 2024

rejain456 Sep 14, 2024

huntergregory left a comment

huntergregory Sep 13, 2024

rejain456 Sep 13, 2024

huntergregory Sep 13, 2024 •

edited

Loading

huntergregory left a comment

huntergregory Sep 13, 2024

vakalapa Sep 17, 2024

rejain456 Sep 17, 2024

vakalapa Sep 17, 2024

	func newStartNPMCmd() *cobra.Command {
	// getTuplesCmd represents the getTuples command
	startNPMCmd := &cobra.Command{
	Use: "start",
	Short: "Starts the Azure NPM process",
	RunE: func(cmd *cobra.Command, args []string) error {
	config := &npmconfig.Config{}
	err := viper.Unmarshal(config)
	if err != nil {
	return fmt.Errorf("failed to load config with error: %w", err)
	}

	flags := npmconfig.Flags{
	KubeConfigPath: viper.GetString(flagKubeConfigPath),
	}

	return start(*config, flags)
	},
	}

	startNPMCmd.Flags().String(flagKubeConfigPath, flagDefaults[flagKubeConfigPath], "path to kubeconfig")

	return startNPMCmd
	}

	func start(config npmconfig.Config, flags npmconfig.Flags) error {

	// #0. TODO(jungukcho): cannot come up when this condition is met.
	// The code inside if condition is to handle allowing all internal traffic, but the case is handled in #2.4.
	// So, this code may not execute. After confirming this, need to delete it.
	if !portRuleExists && !peerRuleExists && !allowExternal {
	acl := policies.NewACLPolicy(policies.Allowed, direction)
	ruleIPSets, allowAllInternalSetInfo := allowAllInternal(matchType)
	npmNetPol.RuleIPSets = append(npmNetPol.RuleIPSets, ruleIPSets)
	acl.AddSetInfo([]policies.SetInfo{allowAllInternalSetInfo})
	npmNetPol.ACLs = append(npmNetPol.ACLs, acl)
	return nil
	}

	func ruleExists(ports []networkingv1.NetworkPolicyPort, peer []networkingv1.NetworkPolicyPeer) (allowExternal, portRuleExists, peerRuleExists bool) {
	// TODO(jungukcho): need to clarify and summarize below flags + more comments
	portRuleExists = len(ports) > 0
	if peer != nil {
	if len(peer) == 0 {
	peerRuleExists = true
	allowExternal = true
	}

	for _, peerRule := range peer {
	if peerRule.PodSelector != nil \|\|
	peerRule.NamespaceSelector != nil \|\|
	peerRule.IPBlock != nil {
	peerRuleExists = true
	break
	}
	}
	} else if !portRuleExists {
	allowExternal = true
	}

	return allowExternal, portRuleExists, peerRuleExists
	}

Npm Lite Feature #3005

Are you sure you want to change the base?

Npm Lite Feature #3005

Conversation

rejain456 commented Sep 10, 2024 • edited Loading

rejain456 commented Sep 10, 2024

rejain456 commented Sep 10, 2024

azure-pipelines bot commented Sep 10, 2024

azure-pipelines bot commented Sep 10, 2024

rejain456 commented Sep 10, 2024

azure-pipelines bot commented Sep 10, 2024

huntergregory left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rejain456 Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

huntergregory Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

huntergregory Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huntergregory Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

huntergregory Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huntergregory Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huntergregory left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huntergregory Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

huntergregory left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rejain456 commented Sep 10, 2024 •

edited

Loading

rejain456 Sep 11, 2024 •

edited

Loading

huntergregory Sep 11, 2024 •

edited

Loading

huntergregory Sep 11, 2024 •

edited

Loading

huntergregory Sep 11, 2024 •

edited

Loading

huntergregory Sep 11, 2024 •

edited

Loading

huntergregory Sep 13, 2024 •

edited

Loading

huntergregory Sep 13, 2024 •

edited

Loading