From ca504e6d52d19f46ac7f2571d1a8854c9989d402 Mon Sep 17 00:00:00 2001 From: Isaiah Raya Date: Wed, 2 Sep 2026 20:28:51 +0000 Subject: [PATCH 01/23] fix: [NPM] compile multi-value namespaceSelector NotIn as a single conjunction A namespaceSelector matchExpression using NotIn with more than one value is a single set-membership requirement: key NotIn [a, b] means key != a AND key != b. The v2 selector compiler flattened multi-value NotIn the same way it flattens multi-value In, emitting one selector per value. Each flattened selector becomes an independent allow decision, and allow decisions are additive (OR), so a namespace carrying one of the values could still be matched by the decision negating a different value. That does not match LabelSelectorAsSelector semantics for multi-value NotIn. Handle the two operators separately in flattenNameSpaceSelector: In still fans out into one selector per value, while NotIn keeps every value as its own single-value NotIn requirement within the same selector. When a selector mixes the two, each NotIn exclusion is carried conjunctively into every In branch. Also fail closed on unsupported operators and empty In/NotIn value lists rather than dropping the requirement, which would widen the selector. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../controlplane/translation/parseSelector.go | 99 ++++++++-- .../translation/parseSelector_test.go | 122 ++++++++++++ .../translation/translatePolicy.go | 8 + .../translation/translatePolicy_test.go | 187 ++++++++++++++++++ 4 files changed, 397 insertions(+), 19 deletions(-) diff --git a/npm/pkg/controlplane/translation/parseSelector.go b/npm/pkg/controlplane/translation/parseSelector.go index 447d1283058..f2e843c1e23 100644 --- a/npm/pkg/controlplane/translation/parseSelector.go +++ b/npm/pkg/controlplane/translation/parseSelector.go @@ -2,7 +2,6 @@ package translation import ( "fmt" - "regexp" "github.com/Azure/azure-container-networking/log" @@ -20,34 +19,47 @@ var validLabelRegex = regexp.MustCompile("(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0 // into multiple label selectors helping with the OR condition. func flattenNameSpaceSelector(nsSelector *metav1.LabelSelector) ([]metav1.LabelSelector, error) { /* - This function helps to create multiple labelSelectors when given a single multivalue nsSelector - Take below example: this nsSelector has 2 values in a matchSelector. + This function helps to create multiple labelSelectors when given a single multivalue nsSelector. + + The two multi-value operators are handled differently because they carry different semantics: + + In: a multi-value In is a disjunction (OR) over its values, so it is fanned out into one + labelSelector per value. Take below example with 2 values in a matchExpression: - namespaceSelector: matchExpressions: - key: ns - operator: NotIn + operator: In values: - netpol-x - netpol-y - goal is to convert this single nsSelector into multiple nsSelectors to preserve OR condition - between multiple values of the matchExpr i.e. this function will return + becomes - namespaceSelector: matchExpressions: - key: ns - operator: NotIn + operator: In values: - netpol-x - namespaceSelector: matchExpressions: - key: ns - operator: NotIn + operator: In values: - netpol-y - then, translate policy will replicate each of these nsSelectors to add two different rules in iptables, - resulting in OR condition between the values. + then, translate policy will replicate each of these nsSelectors to add two different rules, + resulting in the OR condition between the values. + + NotIn: a multi-value NotIn is a single set-membership conjunction, i.e. + ns NotIn [x, y] means (ns != x AND ns != y). It must NOT be fanned out into separate + selectors, because each generated selector becomes an independent allow rule and allow + rules are additive (OR): a namespace carrying one excluded value would still match the + rule negating the other value and be admitted. Instead, every value is kept as its own + single-value NotIn requirement within the same selector, so all negated conditions land + in a single decision (AND) and the default drop stays effective for every excluded value. + When a selector mixes In and NotIn, each NotIn exclusion is carried conjunctively into + every In branch. Check TestFlattenNameSpaceSelector 2nd subcase for complex scenario */ @@ -70,14 +82,20 @@ func flattenNameSpaceSelector(nsSelector *metav1.LabelSelector) ([]metav1.LabelS } multiValuePresent := false + // notInExpanded records whether a multi-value NotIn was rewritten into several + // single-value NotIn requirements on baseSelector. When it is, baseSelector no + // longer equals the input, so the original selector must not be returned as-is. + notInExpanded := false multiValueMatchExprs := []metav1.LabelSelectorRequirement{} for _, req := range nsSelector.MatchExpressions { - // Only In and NotIn operators of matchExprs have multiple values - // NPM will ignore single value matchExprs of these operators. - // for multiple values, it will create a slice of them to be used for Zipping with baseSelector - // to create multiple nsSelectors to preserve OR condition across all labels and expressions + // In/NotIn requirements carry the values; single-value requirements are added to + // baseSelector as-is, while multi-value requirements are handled per operator below. + // Exists/DoesNotExist carry no values and are added to baseSelector directly. switch { - case (req.Operator == metav1.LabelSelectorOpIn) || (req.Operator == metav1.LabelSelectorOpNotIn): + case req.Operator == metav1.LabelSelectorOpIn: + if len(req.Values) == 0 { + return nil, ErrEmptyMatchExpressionValues + } for _, v := range req.Values { if !isValidLabelValue(v) { return nil, ErrInvalidMatchExpressionValues @@ -88,21 +106,64 @@ func flattenNameSpaceSelector(nsSelector *metav1.LabelSelector) ([]metav1.LabelS // for length 1, add the matchExpr to baseSelector baseSelector.MatchExpressions = append(baseSelector.MatchExpressions, req) } else { + // multi-value In is a disjunction: zip it with baseSelector to + // create one nsSelector per value and preserve the OR condition. multiValuePresent = true multiValueMatchExprs = append(multiValueMatchExprs, req) } + case req.Operator == metav1.LabelSelectorOpNotIn: + if len(req.Values) == 0 { + return nil, ErrEmptyMatchExpressionValues + } + for _, v := range req.Values { + if !isValidLabelValue(v) { + return nil, ErrInvalidMatchExpressionValues + } + } + + if len(req.Values) == 1 { + // for length 1, add the matchExpr to baseSelector + baseSelector.MatchExpressions = append(baseSelector.MatchExpressions, req) + } else { + // A multi-value NotIn is a single set-membership conjunction + // (key NotIn [a, b] == key != a AND key != b), NOT a disjunction. + // Fanning it out into separate selectors would emit independent + // additive allow rules and let each excluded value be admitted by + // the rule negating another value. Keep every value as its own + // single-value NotIn within the same selector so all negations + // stay in one decision (AND). + notInExpanded = true + for _, v := range req.Values { + baseSelector.MatchExpressions = append( + baseSelector.MatchExpressions, + metav1.LabelSelectorRequirement{ + Key: req.Key, + Operator: metav1.LabelSelectorOpNotIn, + Values: []string{v}, + }, + ) + } + } case (req.Operator == metav1.LabelSelectorOpExists) || (req.Operator == metav1.LabelSelectorOpDoesNotExist): // since Exists and NotExists do not contain any values, NPM can safely add them to the baseSelector baseSelector.MatchExpressions = append(baseSelector.MatchExpressions, req) default: - log.Errorf("Invalid operator [%s] for selector [%v] requirement", req.Operator, *nsSelector) + // Fail closed: an unknown operator must not silently drop the requirement + // and widen the selector. Kubernetes only admits In/NotIn/Exists/DoesNotExist. + log.Errorf("unsupported operator [%s] for selector [%v] requirement", req.Operator, *nsSelector) + return nil, ErrUnsupportedMatchExpressionOperator } } - // If there are no multiValue NS selector match expressions - // return the original NsSelector + // If there are no multiValue In match expressions to fan out, the baseSelector + // (which already carries any conjunctive NotIn expansions) is the only selector. if !multiValuePresent { - return []metav1.LabelSelector{*nsSelector}, nil + if !notInExpanded { + // Nothing was rewritten; return the original selector unchanged so callers + // that compare against the input see an identical selector. + return []metav1.LabelSelector{*nsSelector}, nil + } + return []metav1.LabelSelector{*baseSelector.DeepCopy()}, nil } // Now use the baseSelector and loop over multiValueMatchExprs to create all diff --git a/npm/pkg/controlplane/translation/parseSelector_test.go b/npm/pkg/controlplane/translation/parseSelector_test.go index e93f99500ae..45357af8644 100644 --- a/npm/pkg/controlplane/translation/parseSelector_test.go +++ b/npm/pkg/controlplane/translation/parseSelector_test.go @@ -599,6 +599,128 @@ func TestFlattenNamespaceSelectorError(t *testing.T) { } } +// TestFlattenNameSpaceSelectorMultiValueNotIn verifies that a multi-value NotIn +// requirement is preserved as a single conjunction rather than fanned out into +// separate selectors. Separate selectors would become independent additive allow +// rules, so a namespace carrying one excluded value could still match the rule +// negating a different value. +func TestFlattenNameSpaceSelectorMultiValueNotIn(t *testing.T) { + selector := &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "tenant", + Operator: metav1.LabelSelectorOpNotIn, + Values: []string{"x", "y"}, + }, + }, + } + + testSelectors, err := flattenNameSpaceSelector(selector) + require.NoError(t, err) + + expected := []metav1.LabelSelector{ + { + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "tenant", + Operator: metav1.LabelSelectorOpNotIn, + Values: []string{"x"}, + }, + { + Key: "tenant", + Operator: metav1.LabelSelectorOpNotIn, + Values: []string{"y"}, + }, + }, + }, + } + + require.Equal(t, expected, testSelectors) +} + +// TestFlattenNameSpaceSelectorMixedInAndNotIn verifies that multi-value In values +// fan out into disjunctive branches while every multi-value NotIn exclusion is +// carried conjunctively into each branch. +func TestFlattenNameSpaceSelectorMixedInAndNotIn(t *testing.T) { + selector := &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "tenant", + Operator: metav1.LabelSelectorOpNotIn, + Values: []string{"x", "y"}, + }, + { + Key: "role", + Operator: metav1.LabelSelectorOpIn, + Values: []string{"a", "b"}, + }, + }, + } + + testSelectors, err := flattenNameSpaceSelector(selector) + require.NoError(t, err) + + // Two In branches, each carrying both NotIn exclusions conjunctively. + require.Len(t, testSelectors, 2) + for _, s := range testSelectors { + var notInValues []string + var inValues []string + for _, req := range s.MatchExpressions { + require.Len(t, req.Values, 1, "every requirement must be single-value after flatten") + switch req.Operator { + case metav1.LabelSelectorOpNotIn: + require.Equal(t, "tenant", req.Key) + notInValues = append(notInValues, req.Values[0]) + case metav1.LabelSelectorOpIn: + require.Equal(t, "role", req.Key) + inValues = append(inValues, req.Values[0]) + default: + t.Fatalf("unexpected operator %s", req.Operator) + } + } + require.ElementsMatch(t, []string{"x", "y"}, notInValues, "both exclusions must be present in every branch") + require.Len(t, inValues, 1) + } +} + +// TestFlattenNameSpaceSelectorUnsupportedOperator verifies that a matchExpression with +// an operator other than In/NotIn/Exists/DoesNotExist is rejected (fail closed) rather +// than silently dropped, which could otherwise widen the selector. +func TestFlattenNameSpaceSelectorUnsupportedOperator(t *testing.T) { + selector := &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "tenant", + Operator: metav1.LabelSelectorOperator("Frobnicate"), + Values: []string{"x"}, + }, + }, + } + s, err := flattenNameSpaceSelector(selector) + require.ErrorIs(t, err, ErrUnsupportedMatchExpressionOperator) + require.Nil(t, s) +} + +// TestFlattenNameSpaceSelectorEmptyValues verifies that In/NotIn requirements with +// no values are rejected (fail closed) rather than silently dropped, which could +// otherwise widen a selector or produce no rules at all. +func TestFlattenNameSpaceSelectorEmptyValues(t *testing.T) { + for _, op := range []metav1.LabelSelectorOperator{metav1.LabelSelectorOpIn, metav1.LabelSelectorOpNotIn} { + selector := &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "tenant", + Operator: op, + Values: []string{}, + }, + }, + } + s, err := flattenNameSpaceSelector(selector) + require.ErrorIs(t, err, ErrEmptyMatchExpressionValues, "operator %s", op) + require.Nil(t, s) + } +} + func TestIsValidLabel(t *testing.T) { good := []string{ "", diff --git a/npm/pkg/controlplane/translation/translatePolicy.go b/npm/pkg/controlplane/translation/translatePolicy.go index 8a647940104..777263d2381 100644 --- a/npm/pkg/controlplane/translation/translatePolicy.go +++ b/npm/pkg/controlplane/translation/translatePolicy.go @@ -35,6 +35,14 @@ var ( ErrInvalidMatchExpressionValues = errors.New( "matchExpression label values must be an empty string or consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character", ) + // ErrEmptyMatchExpressionValues is returned when an In or NotIn matchExpression carries no values. + // Kubernetes rejects such requirements; NPM fails closed rather than dropping the requirement, + // which could otherwise widen a selector (e.g. a dropped NotIn) or yield no rules at all. + ErrEmptyMatchExpressionValues = errors.New("In and NotIn matchExpression requirements must have at least one value") + // ErrUnsupportedMatchExpressionOperator is returned when a matchExpression uses an operator that is + // none of In, NotIn, Exists or DoesNotExist. NPM fails closed rather than dropping the requirement, + // which could otherwise silently widen the selector. + ErrUnsupportedMatchExpressionOperator = errors.New("unsupported matchExpression operator") // 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 are passed in with NPM Lite enabled. NPM Lite allows deny-all and allow-all policies diff --git a/npm/pkg/controlplane/translation/translatePolicy_test.go b/npm/pkg/controlplane/translation/translatePolicy_test.go index 29fa77407ea..d0a911e5e10 100644 --- a/npm/pkg/controlplane/translation/translatePolicy_test.go +++ b/npm/pkg/controlplane/translation/translatePolicy_test.go @@ -1268,6 +1268,193 @@ func TestNameSpaceSelector(t *testing.T) { } } +// TestNameSpaceSelectorMultiValueNotIn verifies that a namespaceSelector with a +// single multi-value NotIn requirement is translated (after flatten, as translateRule +// does) into one decision carrying a negated match-set for every excluded value. +// Emitting these as separate allow rules would be additive (OR) and admit a namespace +// that carries any one of the excluded values. +func TestNameSpaceSelectorMultiValueNotIn(t *testing.T) { + matchType := policies.SrcMatch + selector := &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "tenant", + Operator: metav1.LabelSelectorOpNotIn, + Values: []string{"x", "y"}, + }, + }, + } + + flattened, err := flattenNameSpaceSelector(selector) + require.NoError(t, err) + // The NotIn conjunction must stay in a single selector, not fan out. + require.Len(t, flattened, 1) + + _, nsSelectorList := nameSpaceSelector(matchType, &flattened[0]) + + expected := []policies.SetInfo{ + policies.NewSetInfo("tenant:x", ipsets.KeyValueLabelOfNamespace, nonIncluded, matchType), + policies.NewSetInfo("tenant:y", ipsets.KeyValueLabelOfNamespace, nonIncluded, matchType), + } + require.ElementsMatch(t, expected, nsSelectorList) +} + +// TestNameSpaceSelectorMatchLabelsAndMultiValueNotIn covers a namespaceSelector that +// combines matchLabels with a multi-value NotIn matchExpression. The matchLabels set +// must be ANDed into the same decision as the two negated values (a positive match plus +// two negated matches in one ACL), matching Kubernetes' conjunction of all requirements. +func TestNameSpaceSelectorMatchLabelsAndMultiValueNotIn(t *testing.T) { + matchType := policies.SrcMatch + selector := &metav1.LabelSelector{ + MatchLabels: map[string]string{"team": "blue"}, + MatchExpressions: []metav1.LabelSelectorRequirement{ + {Key: "tenant", Operator: metav1.LabelSelectorOpNotIn, Values: []string{"x", "y"}}, + }, + } + + flattened, err := flattenNameSpaceSelector(selector) + require.NoError(t, err) + // matchLabels + a single conjunctive NotIn must stay in ONE selector, not fan out. + require.Len(t, flattened, 1) + + _, nsSelectorList := nameSpaceSelector(matchType, &flattened[0]) + + expected := []policies.SetInfo{ + policies.NewSetInfo("team:blue", ipsets.KeyValueLabelOfNamespace, included, matchType), + policies.NewSetInfo("tenant:x", ipsets.KeyValueLabelOfNamespace, nonIncluded, matchType), + policies.NewSetInfo("tenant:y", ipsets.KeyValueLabelOfNamespace, nonIncluded, matchType), + } + require.ElementsMatch(t, expected, nsSelectorList, + "matchLabels set must be ANDed with both negated tenant sets in one decision") +} + +// nsNotInPolicy builds a NetworkPolicy that selects all local pods and, for the given +// direction, admits peers whose namespace matches `key NotIn values`. When ports is +// non-empty, the peer rule also carries those ports. +func nsNotInPolicy(name, ns, key string, direction networkingv1.PolicyType, ports []networkingv1.NetworkPolicyPort, values ...string) *networkingv1.NetworkPolicy { + peer := networkingv1.NetworkPolicyPeer{ + NamespaceSelector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + {Key: key, Operator: metav1.LabelSelectorOpNotIn, Values: values}, + }, + }, + } + pol := &networkingv1.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: ns}, + Spec: networkingv1.NetworkPolicySpec{ + PodSelector: metav1.LabelSelector{}, + PolicyTypes: []networkingv1.PolicyType{direction}, + }, + } + if direction == networkingv1.PolicyTypeIngress { + pol.Spec.Ingress = []networkingv1.NetworkPolicyIngressRule{{Ports: ports, From: []networkingv1.NetworkPolicyPeer{peer}}} + } else { + pol.Spec.Egress = []networkingv1.NetworkPolicyEgressRule{{Ports: ports, To: []networkingv1.NetworkPolicyPeer{peer}}} + } + return pol +} + +// TestTranslatePolicyMultiValueNotInConjunction is the end-to-end regression for a +// multi-value namespaceSelector NotIn. It drives the full TranslatePolicy path (both +// directions, with and without a port) and asserts the complete enforcement invariant: +// exactly ONE allow ACL exists, it negates every excluded value within that single +// decision (a conjunction / AND) and references no positive tenant set, and a default +// drop is still present. The pre-fix behavior emitted one additive allow ACL per value, +// so a namespace carrying any one excluded value matched the ACL negating another value +// and was admitted before the default drop. +func TestTranslatePolicyMultiValueNotInConjunction(t *testing.T) { + t.Parallel() + + tcpPort := networkingv1.NetworkPolicyPort{Port: &intstr.IntOrString{Type: intstr.Int, IntVal: 80}} + + tests := []struct { + name string + direction networkingv1.PolicyType + ports []networkingv1.NetworkPolicyPort + peerList func(*policies.ACLPolicy) []policies.SetInfo + }{ + { + name: "ingress", + direction: networkingv1.PolicyTypeIngress, + peerList: func(acl *policies.ACLPolicy) []policies.SetInfo { return acl.SrcList }, + }, + { + name: "egress", + direction: networkingv1.PolicyTypeEgress, + peerList: func(acl *policies.ACLPolicy) []policies.SetInfo { return acl.DstList }, + }, + { + name: "ingress-with-port", + direction: networkingv1.PolicyTypeIngress, + ports: []networkingv1.NetworkPolicyPort{tcpPort}, + peerList: func(acl *policies.ACLPolicy) []policies.SetInfo { return acl.SrcList }, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + pol := nsNotInPolicy("victim", "default", "tenant", tt.direction, tt.ports, "attacker", "quarantine") + npmNetPol, err := TranslatePolicy(pol, false) + require.NoError(t, err) + + excluded := map[string]bool{"tenant:attacker": true, "tenant:quarantine": true} + var allowACLs, dropACLs int + var theAllow, theDrop *policies.ACLPolicy + for i := range npmNetPol.ACLs { + acl := npmNetPol.ACLs[i] + switch acl.Target { + case policies.Allowed: + allowACLs++ + theAllow = npmNetPol.ACLs[i] + case policies.Dropped: + dropACLs++ + theDrop = npmNetPol.ACLs[i] + default: + t.Fatalf("unexpected ACL target %v", acl.Target) + } + } + + // Full enforcement invariant: exactly one allow decision and exactly one + // default drop. An additive-OR bypass would yield two allow ACLs; a missing + // drop or an allow-all leaking in would also be caught here. + require.Equal(t, 1, allowACLs, "there must be exactly one allow ACL, not additive allow ACLs") + require.Equal(t, 1, dropACLs, "there must be exactly one default drop ACL") + require.NotNil(t, theAllow) + require.NotNil(t, theDrop) + + // The single allow ACL's peer list must be EXACTLY the two excluded values, + // each a negated match (Included == false) and nothing else (no stray positive + // set such as an all-namespaces allow). + allowPeers := tt.peerList(theAllow) + require.Len(t, allowPeers, 2, "allow ACL must reference exactly the two excluded sets and no positive set") + var negated []string + for _, si := range allowPeers { + require.True(t, excluded[si.IPSet.Name], "unexpected set %s in allow ACL", si.IPSet.Name) + require.False(t, si.Included, "tenant set %s must be a negated match", si.IPSet.Name) + require.Equal(t, ipsets.KeyValueLabelOfNamespace, si.IPSet.Type) + negated = append(negated, si.IPSet.Name) + } + require.ElementsMatch(t, []string{"tenant:attacker", "tenant:quarantine"}, negated, + "the single allow ACL must negate every excluded value") + + // The default drop must be same-direction and unconditional (no peer match), + // so the excluded namespaces have no allow path and fall through to it. + require.Equal(t, theAllow.Direction, theDrop.Direction, "drop must be the same direction as the allow") + require.Empty(t, tt.peerList(theDrop), "the default drop must be unconditional") + + // When a port is present it must be carried in the same allow decision, + // conjunctively with the negated tenant sets. + if len(tt.ports) > 0 { + require.EqualValues(t, 80, theAllow.DstPorts.Port, + "the port must render in the same allow ACL as the negated tenant sets") + } + }) + } +} + func TestAllowAllInternal(t *testing.T) { matchType := policies.SrcMatch tests := []struct { From 59a508bea86b7d661e530b70cbe37693f3086a19 Mon Sep 17 00:00:00 2001 From: Isaiah Raya Date: Wed, 2 Sep 2026 20:32:43 +0000 Subject: [PATCH 02/23] fix: [NPM] scope negation-only namespaceSelector matches to cluster namespaces A namespaceSelector only ever selects namespaces, so every peer it matches must be a cluster address. A negative requirement (NotIn / DoesNotExist) renders as a negated set match, which is satisfied by every address that is not in that set, including addresses that are not cluster pods at all. When a namespaceSelector produced no positive set, the negations alone were the whole match, so the rule admitted non-cluster peers. On egress that let a selected pod reach arbitrary external hosts even though the policy named no ipBlock and no allow-all peer. Intersect with the all-namespaces set in parseNSSelector when the parsed selectors contain no positive match, mirroring allowAllInternal. Selectors that already carry a positive requirement (matchLabels, In, Exists) are unchanged, since that requirement already scopes the match to namespaces. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../controlplane/translation/parseSelector.go | 22 ++ .../translation/translatePolicy_test.go | 212 +++++++++++++++++- 2 files changed, 229 insertions(+), 5 deletions(-) diff --git a/npm/pkg/controlplane/translation/parseSelector.go b/npm/pkg/controlplane/translation/parseSelector.go index f2e843c1e23..e26986f12d9 100644 --- a/npm/pkg/controlplane/translation/parseSelector.go +++ b/npm/pkg/controlplane/translation/parseSelector.go @@ -259,6 +259,17 @@ func (ps *parsedSelectors) addSelector(include bool, setType ipsets.SetType, set ps.labelSet[setNameWithOp] = struct{}{} } +// hasPositiveSelector reports whether any parsed selector is a positive (non-negated) match. +// Without one, the parsed selectors match purely by negation and constrain nothing. +func (ps *parsedSelectors) hasPositiveSelector() bool { + for _, ls := range ps.labelSelectors { + if ls.include { + return true + } + } + return false +} + // parseNSSelector parses namespaceSelector and returns slice of labelSelector object // which includes operator, setType, ipset name and always nil members slice. // Member slices is always nil since parseNSSelector function is called @@ -300,6 +311,17 @@ func parseNSSelector(selector *metav1.LabelSelector) []labelSelector { parsedSelectors.addSelector(noNegativeOp, setType, setName) } + // #4. A namespaceSelector only ever selects namespaces, so every match it produces + // must be a cluster address. A negative requirement (NotIn / DoesNotExist) renders as + // a negated set match, which is satisfied by every address that is not in that set, + // including addresses outside the cluster. When the selector produces no positive set + // to intersect with, the negations alone are the whole match and the rule would also + // admit non-cluster (e.g. internet) peers. Intersect with the all-namespaces set so + // the match stays scoped to namespaces, mirroring allowAllInternal. + if !parsedSelectors.hasPositiveSelector() { + parsedSelectors.addSelector(true, ipsets.KeyLabelOfNamespace, util.KubeAllNamespacesFlag) + } + return parsedSelectors.labelSelectors } diff --git a/npm/pkg/controlplane/translation/translatePolicy_test.go b/npm/pkg/controlplane/translation/translatePolicy_test.go index d0a911e5e10..e7d1de4be98 100644 --- a/npm/pkg/controlplane/translation/translatePolicy_test.go +++ b/npm/pkg/controlplane/translation/translatePolicy_test.go @@ -1293,6 +1293,8 @@ func TestNameSpaceSelectorMultiValueNotIn(t *testing.T) { _, nsSelectorList := nameSpaceSelector(matchType, &flattened[0]) expected := []policies.SetInfo{ + // The all-namespaces set keeps the negation-only match scoped to cluster namespaces. + policies.NewSetInfo(util.KubeAllNamespacesFlag, ipsets.KeyLabelOfNamespace, included, matchType), policies.NewSetInfo("tenant:x", ipsets.KeyValueLabelOfNamespace, nonIncluded, matchType), policies.NewSetInfo("tenant:y", ipsets.KeyValueLabelOfNamespace, nonIncluded, matchType), } @@ -1328,6 +1330,196 @@ func TestNameSpaceSelectorMatchLabelsAndMultiValueNotIn(t *testing.T) { "matchLabels set must be ANDed with both negated tenant sets in one decision") } +// TestNameSpaceSelectorNegationOnlyIsScopedToNamespaces verifies that a namespaceSelector +// whose requirements are all negative (NotIn / DoesNotExist) is intersected with the +// all-namespaces set. A negated set match is satisfied by every address that is not in the +// set, so without a positive set to intersect with, the decision also matches addresses +// that are not cluster pods at all (e.g. the internet). +func TestNameSpaceSelectorNegationOnlyIsScopedToNamespaces(t *testing.T) { + matchType := policies.DstMatch + tests := []struct { + name string + selector *metav1.LabelSelector + expected []policies.SetInfo + }{ + { + name: "single-value NotIn", + selector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + {Key: "tenant", Operator: metav1.LabelSelectorOpNotIn, Values: []string{"x"}}, + }, + }, + expected: []policies.SetInfo{ + policies.NewSetInfo(util.KubeAllNamespacesFlag, ipsets.KeyLabelOfNamespace, included, matchType), + policies.NewSetInfo("tenant:x", ipsets.KeyValueLabelOfNamespace, nonIncluded, matchType), + }, + }, + { + name: "DoesNotExist", + selector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + {Key: "tenant", Operator: metav1.LabelSelectorOpDoesNotExist}, + }, + }, + expected: []policies.SetInfo{ + policies.NewSetInfo(util.KubeAllNamespacesFlag, ipsets.KeyLabelOfNamespace, included, matchType), + policies.NewSetInfo("tenant", ipsets.KeyLabelOfNamespace, nonIncluded, matchType), + }, + }, + { + name: "NotIn and DoesNotExist together", + selector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + {Key: "tenant", Operator: metav1.LabelSelectorOpNotIn, Values: []string{"x"}}, + {Key: "team", Operator: metav1.LabelSelectorOpDoesNotExist}, + }, + }, + expected: []policies.SetInfo{ + policies.NewSetInfo(util.KubeAllNamespacesFlag, ipsets.KeyLabelOfNamespace, included, matchType), + policies.NewSetInfo("tenant:x", ipsets.KeyValueLabelOfNamespace, nonIncluded, matchType), + policies.NewSetInfo("team", ipsets.KeyLabelOfNamespace, nonIncluded, matchType), + }, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + nsSelectorIPSets, nsSelectorList := nameSpaceSelector(matchType, tt.selector) + require.ElementsMatch(t, tt.expected, nsSelectorList) + // The all-namespaces set must also be translated so it exists in the dataplane. + require.Contains(t, nsSelectorIPSets, + ipsets.NewTranslatedIPSet(util.KubeAllNamespacesFlag, ipsets.KeyLabelOfNamespace)) + }) + } +} + +// TestNameSpaceSelectorWithPositiveMatchIsUnchanged verifies that the all-namespaces +// intersection is added only when it is needed. A selector that already carries a positive +// requirement is scoped to namespaces by that requirement, so it must be left as-is. +func TestNameSpaceSelectorWithPositiveMatchIsUnchanged(t *testing.T) { + matchType := policies.DstMatch + tests := []struct { + name string + selector *metav1.LabelSelector + expected []policies.SetInfo + }{ + { + name: "matchLabels only", + selector: &metav1.LabelSelector{MatchLabels: map[string]string{"team": "blue"}}, + expected: []policies.SetInfo{ + policies.NewSetInfo("team:blue", ipsets.KeyValueLabelOfNamespace, included, matchType), + }, + }, + { + name: "matchLabels with a negative expression", + selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"team": "blue"}, + MatchExpressions: []metav1.LabelSelectorRequirement{ + {Key: "tenant", Operator: metav1.LabelSelectorOpNotIn, Values: []string{"x"}}, + }, + }, + expected: []policies.SetInfo{ + policies.NewSetInfo("team:blue", ipsets.KeyValueLabelOfNamespace, included, matchType), + policies.NewSetInfo("tenant:x", ipsets.KeyValueLabelOfNamespace, nonIncluded, matchType), + }, + }, + { + name: "Exists with a negative expression", + selector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + {Key: "team", Operator: metav1.LabelSelectorOpExists}, + {Key: "tenant", Operator: metav1.LabelSelectorOpNotIn, Values: []string{"x"}}, + }, + }, + expected: []policies.SetInfo{ + policies.NewSetInfo("team", ipsets.KeyLabelOfNamespace, included, matchType), + policies.NewSetInfo("tenant:x", ipsets.KeyValueLabelOfNamespace, nonIncluded, matchType), + }, + }, + { + name: "empty selector still resolves to all namespaces once", + selector: &metav1.LabelSelector{}, + expected: []policies.SetInfo{ + policies.NewSetInfo(util.KubeAllNamespacesFlag, ipsets.KeyLabelOfNamespace, included, matchType), + }, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + _, nsSelectorList := nameSpaceSelector(matchType, tt.selector) + require.ElementsMatch(t, tt.expected, nsSelectorList) + }) + } +} + +// TestTranslatePolicyNegationOnlyNamespaceSelector is the end-to-end regression for a +// peer whose only requirement is a negative namespaceSelector. It asserts that the +// resulting allow decision carries the all-namespaces set, so the rule cannot be +// satisfied by an address outside the cluster. Egress is the impactful direction (an +// unscoped negation lets a selected pod reach arbitrary external hosts), but ingress is +// covered too since the compiler is direction-agnostic. +func TestTranslatePolicyNegationOnlyNamespaceSelector(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + direction networkingv1.PolicyType + matchType policies.MatchType + peerList func(*policies.ACLPolicy) []policies.SetInfo + }{ + { + name: "egress", + direction: networkingv1.PolicyTypeEgress, + matchType: policies.DstMatch, + peerList: func(acl *policies.ACLPolicy) []policies.SetInfo { return acl.DstList }, + }, + { + name: "ingress", + direction: networkingv1.PolicyTypeIngress, + matchType: policies.SrcMatch, + peerList: func(acl *policies.ACLPolicy) []policies.SetInfo { return acl.SrcList }, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + pol := nsNotInPolicy("victim", "default", "tenant", tt.direction, nil, "x") + npmNetPol, err := TranslatePolicy(pol, false) + require.NoError(t, err) + + var theAllow *policies.ACLPolicy + for i := range npmNetPol.ACLs { + if npmNetPol.ACLs[i].Target == policies.Allowed { + require.Nil(t, theAllow, "there must be exactly one allow ACL") + theAllow = npmNetPol.ACLs[i] + } + } + require.NotNil(t, theAllow) + + peers := tt.peerList(theAllow) + require.ElementsMatch(t, []policies.SetInfo{ + policies.NewSetInfo(util.KubeAllNamespacesFlag, ipsets.KeyLabelOfNamespace, included, tt.matchType), + policies.NewSetInfo("tenant:x", ipsets.KeyValueLabelOfNamespace, nonIncluded, tt.matchType), + }, peers, "a negation-only namespaceSelector must be intersected with the all-namespaces set") + + var sawAllNamespaces bool + for _, si := range peers { + if si.Included && si.IPSet.Name == util.KubeAllNamespacesFlag { + sawAllNamespaces = true + } + } + require.True(t, sawAllNamespaces, + "without the all-namespaces set the negated match also admits non-cluster addresses") + }) + } +} + // nsNotInPolicy builds a NetworkPolicy that selects all local pods and, for the given // direction, admits peers whose namespace matches `key NotIn values`. When ports is // non-empty, the peer rule also carries those ports. @@ -1425,20 +1617,30 @@ func TestTranslatePolicyMultiValueNotInConjunction(t *testing.T) { require.NotNil(t, theAllow) require.NotNil(t, theDrop) - // The single allow ACL's peer list must be EXACTLY the two excluded values, - // each a negated match (Included == false) and nothing else (no stray positive - // set such as an all-namespaces allow). + // The single allow ACL's peer list must be the two excluded values, each a + // negated match (Included == false), intersected with the all-namespaces set. + // The all-namespaces set is what keeps a negation-only match scoped to cluster + // namespaces; without it the negations alone also match non-cluster addresses. allowPeers := tt.peerList(theAllow) - require.Len(t, allowPeers, 2, "allow ACL must reference exactly the two excluded sets and no positive set") + require.Len(t, allowPeers, 3, "allow ACL must reference the two excluded sets plus the all-namespaces set") var negated []string + var positive []string for _, si := range allowPeers { + if si.Included { + require.Equal(t, util.KubeAllNamespacesFlag, si.IPSet.Name, + "the only positive set may be the all-namespaces set") + require.Equal(t, ipsets.KeyLabelOfNamespace, si.IPSet.Type) + positive = append(positive, si.IPSet.Name) + continue + } require.True(t, excluded[si.IPSet.Name], "unexpected set %s in allow ACL", si.IPSet.Name) - require.False(t, si.Included, "tenant set %s must be a negated match", si.IPSet.Name) require.Equal(t, ipsets.KeyValueLabelOfNamespace, si.IPSet.Type) negated = append(negated, si.IPSet.Name) } require.ElementsMatch(t, []string{"tenant:attacker", "tenant:quarantine"}, negated, "the single allow ACL must negate every excluded value") + require.Equal(t, []string{util.KubeAllNamespacesFlag}, positive, + "the negation-only match must be intersected with the all-namespaces set") // The default drop must be same-direction and unconditional (no peer match), // so the excluded namespaces have no allow path and fall through to it. From 8f7f77ec57b521c24e25634e7309735c624955de Mon Sep 17 00:00:00 2001 From: Isaiah Raya Date: Wed, 2 Sep 2026 20:36:12 +0000 Subject: [PATCH 03/23] fix: [NPM] bound namespaceSelector flattening Flattening a namespaceSelector's multi-value In requirements produces the Cartesian product of their values, and the code had no ceiling on the result. The count is the product of the value counts, so it grows exponentially with the number of such requirements: 19 two-value requirements in one small, valid policy expand to 2^19 selectors. Each one is deep-copied and later becomes its own IPSet and ACL, so a single policy object could exhaust the memory of the NPM DaemonSet on every node and take policy programming down cluster-wide. Compute the product before any allocation and reject the selector once it would exceed maxFlattenedNSSelectors. The check divides instead of multiplying so it cannot overflow, and it runs per requirement, so a single very wide requirement is rejected on the first iteration too. The cap is far above any workable policy since a selector fanning out that wide would already be unusable as rules. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../controlplane/translation/parseSelector.go | 26 +++++- .../translation/parseSelector_test.go | 80 +++++++++++++++++++ .../translation/translatePolicy.go | 4 + 3 files changed, 107 insertions(+), 3 deletions(-) diff --git a/npm/pkg/controlplane/translation/parseSelector.go b/npm/pkg/controlplane/translation/parseSelector.go index e26986f12d9..6a3d305e76d 100644 --- a/npm/pkg/controlplane/translation/parseSelector.go +++ b/npm/pkg/controlplane/translation/parseSelector.go @@ -15,6 +15,14 @@ import ( // an alphanumeric character (e.g. 'MyValue', or 'my_value', or '12345', regex used for validation is '(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?' var validLabelRegex = regexp.MustCompile("(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?") +// maxFlattenedNSSelectors caps how many labelSelectors a single namespaceSelector may be +// flattened into. Flattening multi-value In requirements produces the Cartesian product of +// their values, and each resulting selector is deep-copied and later turned into its own +// IPSet and ACL, so the cost grows exponentially with the number of such requirements. The +// cap is far above any workable policy (a selector fanning out this wide would already be +// unusable as iptables rules) while keeping a crafted selector from exhausting memory. +const maxFlattenedNSSelectors = 1000 + // flattenNameSpaceSelector will help flatten multiple nameSpace selector match Expressions values // into multiple label selectors helping with the OR condition. func flattenNameSpaceSelector(nsSelector *metav1.LabelSelector) ([]metav1.LabelSelector, error) { @@ -167,10 +175,22 @@ func flattenNameSpaceSelector(nsSelector *metav1.LabelSelector) ([]metav1.LabelS } // Now use the baseSelector and loop over multiValueMatchExprs to create all - // combinations of values - flatNsSelectors := []metav1.LabelSelector{ - *baseSelector.DeepCopy(), + // combinations of values. The number of combinations is the product of the value + // counts, so it grows exponentially with the number of multi-value In requirements + // (19 two-value requirements already yield 2^19 selectors). Bound the product before + // doing any allocation: every selector below is deep-copied and later becomes its own + // IPSet and ACL, so an unbounded product exhausts memory on every node running NPM. + combinations := 1 + for _, req := range multiValueMatchExprs { + if len(req.Values) > maxFlattenedNSSelectors/combinations { + log.Errorf("namespaceSelector [%v] expands past the %d selector limit", *nsSelector, maxFlattenedNSSelectors) + return nil, ErrTooManyFlattenedSelectors + } + combinations *= len(req.Values) } + + flatNsSelectors := make([]metav1.LabelSelector, 0, combinations) + flatNsSelectors = append(flatNsSelectors, *baseSelector.DeepCopy()) for _, req := range multiValueMatchExprs { flatNsSelectors = zipMatchExprs(flatNsSelectors, req) } diff --git a/npm/pkg/controlplane/translation/parseSelector_test.go b/npm/pkg/controlplane/translation/parseSelector_test.go index 45357af8644..f2541d7913e 100644 --- a/npm/pkg/controlplane/translation/parseSelector_test.go +++ b/npm/pkg/controlplane/translation/parseSelector_test.go @@ -6,6 +6,7 @@ import ( "testing" "github.com/stretchr/testify/require" + networkingv1 "k8s.io/api/networking/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -721,6 +722,85 @@ func TestFlattenNameSpaceSelectorEmptyValues(t *testing.T) { } } +// TestFlattenNameSpaceSelectorExpansionLimit verifies that a namespaceSelector whose +// multi-value In requirements would expand into more selectors than NPM is willing to +// translate is rejected before any allocation. Each flattened selector is deep-copied and +// later becomes its own IPSet and ACL, and the count is the product of the value counts, +// so an unbounded selector exhausts memory on every node running NPM. +func TestFlattenNameSpaceSelectorExpansionLimit(t *testing.T) { + twoValueReqs := func(n int) []metav1.LabelSelectorRequirement { + reqs := make([]metav1.LabelSelectorRequirement, 0, n) + for i := 0; i < n; i++ { + reqs = append(reqs, metav1.LabelSelectorRequirement{ + Key: fmt.Sprintf("key%d", i), + Operator: metav1.LabelSelectorOpIn, + Values: []string{"a", "b"}, + }) + } + return reqs + } + + // 2^9 = 512 selectors is under the limit and must still translate. + under := &metav1.LabelSelector{MatchExpressions: twoValueReqs(9)} + selectors, err := flattenNameSpaceSelector(under) + require.NoError(t, err) + require.Len(t, selectors, 512) + + // 2^19 = 524288 selectors is the reported exhaustion case and must be rejected. + over := &metav1.LabelSelector{MatchExpressions: twoValueReqs(19)} + selectors, err = flattenNameSpaceSelector(over) + require.ErrorIs(t, err, ErrTooManyFlattenedSelectors) + require.Nil(t, selectors) + + // A single requirement wider than the limit is rejected on the first iteration, + // so the guard cannot be sidestepped by using one very wide requirement. + values := make([]string, maxFlattenedNSSelectors+1) + for i := range values { + values[i] = fmt.Sprintf("v%d", i) + } + wide := &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + {Key: "key", Operator: metav1.LabelSelectorOpIn, Values: values}, + }, + } + selectors, err = flattenNameSpaceSelector(wide) + require.ErrorIs(t, err, ErrTooManyFlattenedSelectors) + require.Nil(t, selectors) +} + +// TestTranslatePolicyExpansionLimit verifies the expansion guard surfaces through the full +// translation path rather than being swallowed, so an oversized policy is rejected instead +// of being expanded. +func TestTranslatePolicyExpansionLimit(t *testing.T) { + reqs := make([]metav1.LabelSelectorRequirement, 0, 19) + for i := 0; i < 19; i++ { + reqs = append(reqs, metav1.LabelSelectorRequirement{ + Key: fmt.Sprintf("key%d", i), + Operator: metav1.LabelSelectorOpIn, + Values: []string{"a", "b"}, + }) + } + + pol := &networkingv1.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: "expand", Namespace: "default"}, + Spec: networkingv1.NetworkPolicySpec{ + PodSelector: metav1.LabelSelector{}, + PolicyTypes: []networkingv1.PolicyType{networkingv1.PolicyTypeIngress}, + Ingress: []networkingv1.NetworkPolicyIngressRule{ + { + From: []networkingv1.NetworkPolicyPeer{ + {NamespaceSelector: &metav1.LabelSelector{MatchExpressions: reqs}}, + }, + }, + }, + }, + } + + npmNetPol, err := TranslatePolicy(pol, false) + require.ErrorIs(t, err, ErrTooManyFlattenedSelectors) + require.Nil(t, npmNetPol) +} + func TestIsValidLabel(t *testing.T) { good := []string{ "", diff --git a/npm/pkg/controlplane/translation/translatePolicy.go b/npm/pkg/controlplane/translation/translatePolicy.go index 777263d2381..cb85f475be4 100644 --- a/npm/pkg/controlplane/translation/translatePolicy.go +++ b/npm/pkg/controlplane/translation/translatePolicy.go @@ -43,6 +43,10 @@ var ( // none of In, NotIn, Exists or DoesNotExist. NPM fails closed rather than dropping the requirement, // which could otherwise silently widen the selector. ErrUnsupportedMatchExpressionOperator = errors.New("unsupported matchExpression operator") + // ErrTooManyFlattenedSelectors is returned when flattening a namespaceSelector's multi-value In + // requirements would produce more labelSelectors than NPM is willing to translate. The count is + // the product of the value counts, so it grows exponentially with the number of such requirements. + ErrTooManyFlattenedSelectors = errors.New("namespaceSelector expands into too many label selectors") // 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 are passed in with NPM Lite enabled. NPM Lite allows deny-all and allow-all policies From 7685e6424bc09eb2a771a8ebf8c87867ed4b1a1c Mon Sep 17 00:00:00 2001 From: Isaiah Raya Date: Wed, 2 Sep 2026 20:46:27 +0000 Subject: [PATCH 04/23] fix: [NPM] canonicalize ipBlock CIDRs and stop reporting failed translation as success An ipBlock CIDR that named the all-addresses block with host bits set, such as 10.0.0.0/0, was rejected before the value was canonicalized, even though it denotes exactly the same addresses as 0.0.0.0/0. That failed the translation of the whole policy, and the v2 controller converted the failure into a successful no-op, so neither the peer rule nor the default drop the policy implies was installed. The policy looked applied while its selected pods were left with no rules at all and nothing signalled the failure. Canonicalize instead of rejecting. NormalizeCIDR clears the host bits, IsIPV4 validates through it, and the ipBlock translation compares and emits the canonical form, so a non-canonical spelling takes the same path as the canonical one. Except CIDRs are canonicalized too, so they de-duplicate correctly and are recognized by the all-addresses split rather than naming the same block twice with opposite meanings. Kernel ipset members must still be canonical, so the dataplane member check now requires that explicitly rather than relying on the old textual rejection. In the controller, a translation failure is now returned and recorded instead of reported as success, so it is visible and the key is requeued. Deliberate datapath limitations (the Windows unsupported features and NPM Lite's CIDR-only peers) cannot resolve on retry and stay suppressed with a warning as before. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../controllers/v2/networkPolicyController.go | 29 +++++- .../v2/networkPolicyController_test.go | 99 +++++++++++++++++++ .../translation/translatePolicy.go | 30 ++++-- .../translation/translatePolicy_test.go | 81 ++++++++++++++- npm/pkg/dataplane/ipsets/ipsetmanager.go | 14 ++- npm/util/util.go | 32 ++++-- npm/util/util_test.go | 61 ++++++++++++ 7 files changed, 322 insertions(+), 24 deletions(-) diff --git a/npm/pkg/controlplane/controllers/v2/networkPolicyController.go b/npm/pkg/controlplane/controllers/v2/networkPolicyController.go index 31ed83b5067..60442ac654d 100644 --- a/npm/pkg/controlplane/controllers/v2/networkPolicyController.go +++ b/npm/pkg/controlplane/controllers/v2/networkPolicyController.go @@ -291,8 +291,8 @@ func (c *NetworkPolicyController) syncAddAndUpdateNetPol(netPolObj *networkingv1 // install translated rules into kernel npmNetPolObj, err := translation.TranslatePolicy(netPolObj, c.npmLiteToggle) if err != nil { - if isUnsupportedWindowsTranslationErr(err) { - klog.Warningf("NetworkPolicy %s in namespace %s is not translated because it has unsupported translated features of Windows: %s", + if isUnsupportedTranslationErr(err) { + klog.Warningf("NetworkPolicy %s in namespace %s is not translated because it uses a feature this datapath does not support: %s", netPolObj.ObjectMeta.Name, netPolObj.ObjectMeta.Namespace, err.Error()) // We can safely suppress unsupported network policy because re-Queuing will result in same error. @@ -300,9 +300,17 @@ func (c *NetworkPolicyController) syncAddAndUpdateNetPol(netPolObj *networkingv1 return metrics.NoOp, nil } - klog.Errorf("Failed to translate podSelector in NetworkPolicy %s in namespace %s: %s", netPolObj.ObjectMeta.Name, netPolObj.ObjectMeta.Namespace, err.Error()) - // The exec time isn't relevant here, so consider a no-op. Returning nil to prevent re-queuing since this is not a transient error. - return metrics.NoOp, nil + klog.Errorf("Failed to translate NetworkPolicy %s in namespace %s: %s", netPolObj.ObjectMeta.Name, netPolObj.ObjectMeta.Namespace, err.Error()) + metrics.SendErrorLogAndMetric(util.NetpolID, + "[syncAddAndUpdateNetPol] Error: failed to translate NetworkPolicy %s in namespace %s due to %v", + netPolObj.ObjectMeta.Name, netPolObj.ObjectMeta.Namespace, err) + // Do not report success here. Reporting success left the policy's selected pods with + // no rules at all - not even the default drop the policy implies - while the policy + // object appeared to be applied and nothing signalled the failure. Return the error so + // it is surfaced and the key is requeued (rate limited) instead. + // The exec time isn't relevant here, so consider a no-op. + return metrics.NoOp, fmt.Errorf("[syncAddAndUpdateNetPol] Error: failed to translate NetworkPolicy %s in namespace %s: %w", + netPolObj.ObjectMeta.Name, netPolObj.ObjectMeta.Namespace, err) } _, policyExisted := c.rawNpSpecMap[netpolKey] @@ -358,3 +366,14 @@ func isUnsupportedWindowsTranslationErr(err error) bool { errors.Is(err, translation.ErrUnsupportedSCTP) || errors.Is(err, translation.ErrUnsupportedExceptCIDR) } + +// isUnsupportedTranslationErr reports whether err is a deliberate limitation of the datapath +// or mode NPM is running in, rather than a policy NPM failed to translate. Those limitations +// cannot resolve on retry, so they stay suppressed with a warning. Every other translation +// failure is surfaced and requeued, because reporting success would leave the policy's +// selected pods with no rules while nothing signalled that the policy was never applied. +func isUnsupportedTranslationErr(err error) bool { + return isUnsupportedWindowsTranslationErr(err) || + // NPM Lite only supports CIDR peers; a label-selector peer is out of scope there. + errors.Is(err, translation.ErrUnsupportedNonCIDR) +} diff --git a/npm/pkg/controlplane/controllers/v2/networkPolicyController_test.go b/npm/pkg/controlplane/controllers/v2/networkPolicyController_test.go index d14f6f67f12..29aa44f8cbd 100644 --- a/npm/pkg/controlplane/controllers/v2/networkPolicyController_test.go +++ b/npm/pkg/controlplane/controllers/v2/networkPolicyController_test.go @@ -9,6 +9,7 @@ import ( "github.com/Azure/azure-container-networking/npm/metrics" "github.com/Azure/azure-container-networking/npm/metrics/promutil" + "github.com/Azure/azure-container-networking/npm/pkg/controlplane/translation" "github.com/Azure/azure-container-networking/npm/pkg/dataplane" dpmocks "github.com/Azure/azure-container-networking/npm/pkg/dataplane/mocks" "github.com/Azure/azure-container-networking/npm/util" @@ -618,3 +619,101 @@ func TestLabelUpdateNetworkPolicy(t *testing.T) { checkNetPolTestResult("TestUpdateNetPol", f, testCases) } + +// netPolWithCIDR builds an ingress NetworkPolicy that selects all pods in its namespace and +// admits the given ipBlock CIDR. +func netPolWithCIDR(cidr string) *networkingv1.NetworkPolicy { + return &networkingv1.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: "allow-cidr", Namespace: "test-nwpolicy"}, + Spec: networkingv1.NetworkPolicySpec{ + PodSelector: metav1.LabelSelector{}, + PolicyTypes: []networkingv1.PolicyType{networkingv1.PolicyTypeIngress}, + Ingress: []networkingv1.NetworkPolicyIngressRule{ + {From: []networkingv1.NetworkPolicyPeer{{IPBlock: &networkingv1.IPBlock{CIDR: cidr}}}}, + }, + }, + } +} + +// TestAddNetworkPolicyNonCanonicalCIDRIsApplied verifies that a policy naming the +// all-addresses block with host bits set is programmed into the dataplane. It used to fail +// translation, and the controller turned that failure into a successful no-op, so the +// policy's selected pods were left with no rules at all. +func TestAddNetworkPolicyNonCanonicalCIDRIsApplied(t *testing.T) { + netPolObj := netPolWithCIDR("10.0.0.0/0") + + f := newNetPolFixture(t) + f.netPolLister = append(f.netPolLister, netPolObj) + f.kubeobjects = append(f.kubeobjects, netPolObj) + stopCh := make(chan struct{}) + defer close(stopCh) + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + dp := dpmocks.NewMockGenericDataplane(ctrl) + f.newNetPolController(stopCh, dp, false) + + // The policy must reach the dataplane instead of being dropped during translation. + dp.EXPECT().UpdatePolicy(gomock.Any()).Times(1) + + addNetPol(f, netPolObj) + checkNetPolTestResult("TestAddNetworkPolicyNonCanonicalCIDRIsApplied", f, []expectedNetPolValues{ + {1, 0, netPolPromVals{1, 1, 0, 0}}, + }) +} + +// TestSyncAddAndUpdateNetPolSurfacesTranslationFailure verifies that a policy NPM cannot +// translate is reported as an error rather than as a successful no-op. Reporting success +// left the policy's selected pods with no rules while nothing signalled that the policy had +// never been applied. +func TestSyncAddAndUpdateNetPolSurfacesTranslationFailure(t *testing.T) { + // An IPv6 ipBlock cannot be expressed by the IPv4 datapath, so translation fails. + netPolObj := netPolWithCIDR("2001:db8::/32") + + f := newNetPolFixture(t) + f.netPolLister = append(f.netPolLister, netPolObj) + f.kubeobjects = append(f.kubeobjects, netPolObj) + stopCh := make(chan struct{}) + defer close(stopCh) + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + dp := dpmocks.NewMockGenericDataplane(ctrl) + f.newNetPolController(stopCh, dp, false) + + // Nothing may be programmed for a policy that failed to translate. + dp.EXPECT().UpdatePolicy(gomock.Any()).Times(0) + + _, err := f.netPolController.syncAddAndUpdateNetPol(netPolObj) + require.Error(t, err, "a translation failure must be surfaced, not reported as success") + require.ErrorIs(t, err, translation.ErrUnsupportedIPAddress) + + // The policy must not be recorded as applied, so a later retry still reconciles it. + netpolKey, keyErr := cache.MetaNamespaceKeyFunc(netPolObj) + require.NoError(t, keyErr) + require.NotContains(t, f.netPolController.rawNpSpecMap, netpolKey) +} + +// TestSyncAddAndUpdateNetPolSuppressesUnsupportedFeature verifies that a deliberate datapath +// limitation stays suppressed. Those cannot resolve on retry, so requeuing them forever +// would be pure churn. +func TestSyncAddAndUpdateNetPolSuppressesUnsupportedFeature(t *testing.T) { + // NPM Lite only supports CIDR peers, so a label-selector peer is out of scope there. + netPolObj := createNetPol() + + f := newNetPolFixture(t) + f.netPolLister = append(f.netPolLister, netPolObj) + f.kubeobjects = append(f.kubeobjects, netPolObj) + stopCh := make(chan struct{}) + defer close(stopCh) + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + dp := dpmocks.NewMockGenericDataplane(ctrl) + f.newNetPolController(stopCh, dp, true) + + dp.EXPECT().UpdatePolicy(gomock.Any()).Times(0) + + _, err := f.netPolController.syncAddAndUpdateNetPol(netPolObj) + require.NoError(t, err, "an unsupported-feature limitation must stay suppressed") +} diff --git a/npm/pkg/controlplane/translation/translatePolicy.go b/npm/pkg/controlplane/translation/translatePolicy.go index cb85f475be4..b0b0641095a 100644 --- a/npm/pkg/controlplane/translation/translatePolicy.go +++ b/npm/pkg/controlplane/translation/translatePolicy.go @@ -167,14 +167,23 @@ func exceptCidr(exceptCidr string) string { return exceptCidr + " " + util.IpsetNomatch } -// deDuplicateExcept removes redundance elements and return slices which has only unique element. +// deDuplicateExcept canonicalizes each except CIDR and removes redundant elements, returning +// a slice which has only unique elements. Canonicalizing first means two spellings of the same +// block (e.g. "10.1.2.0/24" and "10.1.2.3/24") collapse to one entry and that the result can be +// compared against the split-CIDR entries below. func deDuplicateExcept(exceptInIPBlock []string) []string { deDupExcepts := []string{} exceptsSet := make(map[string]struct{}) for _, except := range exceptInIPBlock { - if _, exist := exceptsSet[except]; !exist { - deDupExcepts = append(deDupExcepts, except) - exceptsSet[except] = struct{}{} + canonical, ok := util.NormalizeCIDR(except) + if !ok { + // Leave a non-IPv4 except untouched; callers validate it separately and + // fail closed rather than silently dropping the exclusion. + canonical = except + } + if _, exist := exceptsSet[canonical]; !exist { + deDupExcepts = append(deDupExcepts, canonical) + exceptsSet[canonical] = struct{}{} } } return deDupExcepts @@ -186,6 +195,15 @@ func ipBlockIPSet(policyName, ns string, direction policies.Direction, ipBlockSe return nil, nil } + // Canonicalize the CIDR before it is compared or handed to the kernel. A block spelled + // with host bits set (e.g. "10.0.0.0/0") denotes the same addresses as its canonical form + // but does not compare equal to it, so without this the all-addresses block below would + // not be recognized and the literal would be rejected by ipset. + cidr, ok := util.NormalizeCIDR(ipBlockRule.CIDR) + if !ok { + return nil, ErrUnsupportedIPAddress + } + // de-duplicated Except if there are redundance elements. deDupExcepts := deDuplicateExcept(ipBlockRule.Except) lenOfDeDupExcepts := len(deDupExcepts) @@ -202,7 +220,7 @@ func ipBlockIPSet(policyName, ns string, direction policies.Direction, ipBlockSe // splitCIDRSet has two entries ("0.0.0.0/1" and "128.0.0.0/1") as key. splitCIDRLen := 2 splitCIDRSet := make(map[string]int, splitCIDRLen) - if ipBlockRule.CIDR == "0.0.0.0/0" { + if cidr == "0.0.0.0/0" { // two cidrs (0.0.0.0/1 and 128.0.0.0/1) for 0.0.0.0/0 + except. members = make([]string, lenOfDeDupExcepts+splitCIDRLen) // in case of "0.0.0.0/0", "0.0.0.0/1" or "0.0.0.0/1 nomatch" comes eariler than "128.0.0.0/1" or "128.0.0.0/1 nomatch". @@ -215,7 +233,7 @@ func ipBlockIPSet(policyName, ns string, direction policies.Direction, ipBlockSe } else { // one cidr + except members = make([]string, lenOfDeDupExcepts+1) - members[indexOfMembers] = ipBlockRule.CIDR + members[indexOfMembers] = cidr indexOfMembers++ } diff --git a/npm/pkg/controlplane/translation/translatePolicy_test.go b/npm/pkg/controlplane/translation/translatePolicy_test.go index e7d1de4be98..5531d7dcd26 100644 --- a/npm/pkg/controlplane/translation/translatePolicy_test.go +++ b/npm/pkg/controlplane/translation/translatePolicy_test.go @@ -636,13 +636,18 @@ func TestIPBlockIPSet(t *testing.T) { translatedIPSet: ipsets.NewTranslatedIPSet("test:in-ns:default-0-0IN", ipsets.CIDRBlocks, []string{"0.0.0.0/1", "128.0.0.0/1"}...), }, { - name: "cidr: 0.0.0.0/0 and except: 10.0.0.0/1", + // "10.0.0.0/1" is a non-canonical spelling of the block "0.0.0.0/1", so this + // except names the lower half that the 0.0.0.0/0 split already emits. It must + // therefore collapse onto that entry as a nomatch, exactly as the canonical + // "0.0.0.0/1" case below does. Emitting "0.0.0.0/1" alongside a separate + // "10.0.0.0/1 nomatch" would name the same net twice with opposite meanings. + name: "cidr: 0.0.0.0/0 and except: 10.0.0.0/1 (non-canonical 0.0.0.0/1)", ipBlockInfo: createIPBlockInfo("test", defaultNS, policies.Ingress, policies.SrcMatch, 0, 0), ipBlockRule: &networkingv1.IPBlock{ CIDR: "0.0.0.0/0", Except: []string{"10.0.0.0/1"}, }, - translatedIPSet: ipsets.NewTranslatedIPSet("test:in-ns:default-0-0IN", ipsets.CIDRBlocks, []string{"0.0.0.0/1", "128.0.0.0/1", "10.0.0.0/1 nomatch"}...), + translatedIPSet: ipsets.NewTranslatedIPSet("test:in-ns:default-0-0IN", ipsets.CIDRBlocks, []string{"0.0.0.0/1 nomatch", "128.0.0.0/1"}...), skipWindows: true, }, { @@ -3863,3 +3868,75 @@ func TestTranslatePolicyNodeEgressPorts(t *testing.T) { require.NoError(t, err) require.Equal(t, []int32{5005, 2500}, npmNetPol.NodeEgressPorts) } + +// ipBlockPolicy builds an ingress NetworkPolicy that selects all pods in ns and admits the +// given ipBlock CIDR. +func ipBlockPolicy(name, ns, cidr string) *networkingv1.NetworkPolicy { + return &networkingv1.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: ns}, + Spec: networkingv1.NetworkPolicySpec{ + PodSelector: metav1.LabelSelector{}, + PolicyTypes: []networkingv1.PolicyType{networkingv1.PolicyTypeIngress}, + Ingress: []networkingv1.NetworkPolicyIngressRule{ + { + From: []networkingv1.NetworkPolicyPeer{ + {IPBlock: &networkingv1.IPBlock{CIDR: cidr}}, + }, + }, + }, + }, + } +} + +// TestTranslatePolicyNonCanonicalAllAddressesCIDR verifies that an ipBlock naming the +// all-addresses block with host bits set (e.g. "10.0.0.0/0") translates identically to the +// canonical "0.0.0.0/0". Rejecting it failed the whole policy, so neither the allow nor the +// default drop the policy implies was installed and the selected pods stayed unisolated. +func TestTranslatePolicyNonCanonicalAllAddressesCIDR(t *testing.T) { + t.Parallel() + + canonical, err := TranslatePolicy(ipBlockPolicy("victim", "default", "0.0.0.0/0"), false) + require.NoError(t, err) + + for _, cidr := range []string{"10.0.0.0/0", "255.255.255.255/0"} { + cidr := cidr + t.Run(cidr, func(t *testing.T) { + t.Parallel() + + npmNetPol, err := TranslatePolicy(ipBlockPolicy("victim", "default", cidr), false) + require.NoError(t, err, "a non-canonical all-addresses block must not fail translation") + require.NotNil(t, npmNetPol) + + // The policy must be indistinguishable from the canonical spelling: same + // ipset members (the 0.0.0.0/0 split) and the same ACLs. + require.Equal(t, canonical.RuleIPSets, npmNetPol.RuleIPSets) + require.Equal(t, len(canonical.ACLs), len(npmNetPol.ACLs)) + + // Most importantly the default drop must exist, since its absence is what + // left the selected pods unisolated. + var dropACLs int + for i := range npmNetPol.ACLs { + if npmNetPol.ACLs[i].Target == policies.Dropped { + dropACLs++ + } + } + require.Equal(t, 1, dropACLs, "the policy's default drop must be installed") + }) + } +} + +// TestTranslatePolicyInvalidCIDRStillFails verifies the canonicalization did not weaken +// validation: a CIDR that is not IPv4 at all must still be rejected. +func TestTranslatePolicyInvalidCIDRStillFails(t *testing.T) { + t.Parallel() + + for _, cidr := range []string{"2001:db8::/32", "10.0.0.0/33", "not-a-cidr/0"} { + cidr := cidr + t.Run(cidr, func(t *testing.T) { + t.Parallel() + npmNetPol, err := TranslatePolicy(ipBlockPolicy("victim", "default", cidr), false) + require.ErrorIs(t, err, ErrUnsupportedIPAddress) + require.Nil(t, npmNetPol) + }) + } +} diff --git a/npm/pkg/dataplane/ipsets/ipsetmanager.go b/npm/pkg/dataplane/ipsets/ipsetmanager.go index 80b7575d8d7..45912e5c503 100644 --- a/npm/pkg/dataplane/ipsets/ipsetmanager.go +++ b/npm/pkg/dataplane/ipsets/ipsetmanager.go @@ -658,5 +658,17 @@ func validateIPSetMemberIP(ip string) bool { ipDetails := strings.Split(ip, ",") ipField := strings.Split(ipDetails[0], " ") - return util.IsIPV4(ipField[0]) + if !util.IsIPV4(ipField[0]) { + return false + } + + // A CIDR member must already be canonical. Translation canonicalizes every CIDR it + // emits, so a member with host bits set did not come from a translated policy and + // must not be handed to the kernel under a name that denotes a different block. + if strings.Contains(ipField[0], "/") { + canonical, ok := util.NormalizeCIDR(ipField[0]) + return ok && canonical == ipField[0] + } + + return true } diff --git a/npm/util/util.go b/npm/util/util.go index daefd4d1b4c..b9edc4b6ee3 100644 --- a/npm/util/util.go +++ b/npm/util/util.go @@ -363,23 +363,35 @@ func SliceToString(list []string) string { return strings.Join(list, SetPolicyDelimiter) } +// NormalizeCIDR returns the canonical form of an IPv4 CIDR, i.e. the block with its host +// bits cleared, so "10.0.0.0/0" becomes "0.0.0.0/0" and "10.1.2.3/24" becomes "10.1.2.0/24". +// It reports false when s is not an IPv4 CIDR. Callers must normalize before comparing a +// CIDR against a well-known block or handing it to the kernel, because a non-canonical +// spelling denotes the same block but does not compare equal and is not accepted by ipset. +func NormalizeCIDR(s string) (string, bool) { + _, network, err := net.ParseCIDR(s) + if err != nil || network.IP.To4() == nil || len(network.Mask) != net.IPv4len { + return "", false + } + return network.String(), true +} + +// IsIPV4 returns true when ip is an IPv4 address or an IPv4 CIDR block. +// A CIDR is validated through NormalizeCIDR, which canonicalizes it first, so a block that +// is spelled with host bits set (for example "10.0.0.0/0") is recognized as the block it +// denotes instead of being rejected. Rejecting such a block would fail the whole policy +// translation and leave the policy's selected pods with no rules at all. func IsIPV4(ip string) bool { - isIPBlock := strings.Contains(ip, "/") - ipOnly := strings.Split(ip, "/") - if strings.Contains(ip, "/0") && ipOnly[0] != "0.0.0.0" { - return false + if strings.Contains(ip, "/") { + _, ok := NormalizeCIDR(ip) + return ok } - address, err := netip.ParseAddr(ipOnly[0]) + address, err := netip.ParseAddr(ip) if err != nil { return false } - if address.Is4() && isIPBlock { - _, _, err := net.ParseCIDR(ip) - return err == nil - } - return address.Is4() } diff --git a/npm/util/util_test.go b/npm/util/util_test.go index af671eabd10..a6cb83ceb82 100644 --- a/npm/util/util_test.go +++ b/npm/util/util_test.go @@ -514,3 +514,64 @@ func TestHashedNameGoldenVectors(t *testing.T) { require.Equal(t, want, GetHashedChainName(in), "GetHashedChainName(%q) golden vector", in) } } + +// TestIsIPV4 covers address and CIDR forms. The CIDR cases matter most: a block whose host +// bits are set (e.g. "10.0.0.0/0") denotes the same addresses as its canonical form and must +// be accepted, because rejecting it fails the whole policy translation and leaves the +// policy's selected pods with no rules at all. +func TestIsIPV4(t *testing.T) { + valid := []string{ + "10.0.0.1", + "0.0.0.0", + "10.0.0.0/24", + "0.0.0.0/0", + // non-canonical spellings of valid IPv4 blocks + "10.0.0.0/0", + "255.255.255.255/0", + "10.1.2.3/24", + "10.0.0.1/32", + } + for _, ip := range valid { + require.True(t, IsIPV4(ip), "IsIPV4(%q) must be true", ip) + } + + invalid := []string{ + "", + "not-an-ip", + "10.0.0.256", + "10.0.0.0/33", + "10.0.0.0/", + "2001:db8::1", + "2001:db8::/32", + "::/0", + } + for _, ip := range invalid { + require.False(t, IsIPV4(ip), "IsIPV4(%q) must be false", ip) + } +} + +// TestNormalizeCIDR verifies that host bits are cleared, so callers can compare a CIDR +// against a well-known block and hand the canonical form to the kernel. +func TestNormalizeCIDR(t *testing.T) { + canonical := map[string]string{ + "0.0.0.0/0": "0.0.0.0/0", + "10.0.0.0/0": "0.0.0.0/0", + "255.255.255.255/0": "0.0.0.0/0", + "10.0.0.0/1": "0.0.0.0/1", + "200.0.0.0/1": "128.0.0.0/1", + "10.1.2.3/24": "10.1.2.0/24", + "10.1.2.0/24": "10.1.2.0/24", + "10.0.0.1/32": "10.0.0.1/32", + } + for in, want := range canonical { + got, ok := NormalizeCIDR(in) + require.True(t, ok, "NormalizeCIDR(%q) must succeed", in) + require.Equal(t, want, got, "NormalizeCIDR(%q)", in) + } + + for _, in := range []string{"", "10.0.0.1", "not-a-cidr", "10.0.0.0/33", "2001:db8::/32", "::/0"} { + got, ok := NormalizeCIDR(in) + require.False(t, ok, "NormalizeCIDR(%q) must fail", in) + require.Empty(t, got) + } +} From 4316b629071e733aed503c081f80f3d936f96e5f Mon Sep 17 00:00:00 2001 From: Isaiah Raya Date: Wed, 2 Sep 2026 20:55:07 +0000 Subject: [PATCH 05/23] fix: [NPM] bound the NPM HTTP API and stop enabling its debug routes by default The NPM HTTP API listens on the host network of a privileged process, so any pod on the node can reach it, and its server was created with only an address and a handler: no read, write or idle deadline, no header bound, and no limit on concurrent connections. The cache route additionally serializes the entire policy cache into memory on every request while holding the cache lock. A client that opens connections and reads its responses a byte at a time could therefore hold an unbounded number of full cache copies alive until the process was OOM killed. Give the server deadlines, a header bound, and a connection ceiling, and admit only a few cache encodings at a time, shedding the rest, so neither the number of clients nor the speed at which they read decides how much memory NPM allocates. The deadlines are generous enough for a Prometheus scrape of this endpoint. Also stop enabling the debug and pprof routes by default in the deployment manifests. Prometheus metrics are unchanged; the debug routes are now opt-in for the clusters that need them, which is where the cache serialization and profiling endpoints are reachable from. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- npm/azure-npm.yaml | 4 +- npm/deploy/kustomize/base/configmap.yaml | 4 +- .../manifests/common/npm-configmap.yaml | 4 +- .../manifests/controller/azure-npm.yaml | 4 +- npm/deploy/manifests/daemon/azure-npm.yaml | 4 +- npm/deploy/npm/azure-npm.yaml | 4 +- npm/http/server/server.go | 56 ++++++++++++++- npm/http/server/server_test.go | 71 +++++++++++++++++++ 8 files changed, 136 insertions(+), 15 deletions(-) diff --git a/npm/azure-npm.yaml b/npm/azure-npm.yaml index aa701ab4289..f95ff91f9e0 100644 --- a/npm/azure-npm.yaml +++ b/npm/azure-npm.yaml @@ -166,8 +166,8 @@ data: "MaxPendingNetPols": 100, "Toggles": { "EnablePrometheusMetrics": true, - "EnablePprof": true, - "EnableHTTPDebugAPI": true, + "EnablePprof": false, + "EnableHTTPDebugAPI": false, "EnableV2NPM": true, "PlaceAzureChainFirst": false, "ApplyInBackground": true, diff --git a/npm/deploy/kustomize/base/configmap.yaml b/npm/deploy/kustomize/base/configmap.yaml index d9f549f2a37..3badcf6d54c 100644 --- a/npm/deploy/kustomize/base/configmap.yaml +++ b/npm/deploy/kustomize/base/configmap.yaml @@ -12,8 +12,8 @@ data: "ListeningAddress": "0.0.0.0", "Toggles": { "EnablePrometheusMetrics": true, - "EnablePprof": true, - "EnableHTTPDebugAPI": true, + "EnablePprof": false, + "EnableHTTPDebugAPI": false, "EnableV2NPM": false, "PlaceAzureChainFirst": false }, diff --git a/npm/deploy/manifests/common/npm-configmap.yaml b/npm/deploy/manifests/common/npm-configmap.yaml index 4d8bd0d3895..2d489dc37e7 100644 --- a/npm/deploy/manifests/common/npm-configmap.yaml +++ b/npm/deploy/manifests/common/npm-configmap.yaml @@ -12,8 +12,8 @@ data: "ListeningAddress": "0.0.0.0", "Toggles": { "EnablePrometheusMetrics": true, - "EnablePprof": true, - "EnableHTTPDebugAPI": true, + "EnablePprof": false, + "EnableHTTPDebugAPI": false, "EnableV2NPM": false, "PlaceAzureChainFirst": false }, diff --git a/npm/deploy/manifests/controller/azure-npm.yaml b/npm/deploy/manifests/controller/azure-npm.yaml index 9ff4d883746..311bc87932f 100644 --- a/npm/deploy/manifests/controller/azure-npm.yaml +++ b/npm/deploy/manifests/controller/azure-npm.yaml @@ -58,8 +58,8 @@ data: "ListeningAddress": "0.0.0.0", "Toggles": { "EnablePrometheusMetrics": true, - "EnablePprof": true, - "EnableHTTPDebugAPI": true, + "EnablePprof": false, + "EnableHTTPDebugAPI": false, "EnableV2NPM": false, "PlaceAzureChainFirst": false }, diff --git a/npm/deploy/manifests/daemon/azure-npm.yaml b/npm/deploy/manifests/daemon/azure-npm.yaml index 0e69605581b..3d4ee690c2c 100644 --- a/npm/deploy/manifests/daemon/azure-npm.yaml +++ b/npm/deploy/manifests/daemon/azure-npm.yaml @@ -58,8 +58,8 @@ data: "ListeningAddress": "0.0.0.0", "Toggles": { "EnablePrometheusMetrics": true, - "EnablePprof": true, - "EnableHTTPDebugAPI": true, + "EnablePprof": false, + "EnableHTTPDebugAPI": false, "EnableV2NPM": false, "PlaceAzureChainFirst": false }, diff --git a/npm/deploy/npm/azure-npm.yaml b/npm/deploy/npm/azure-npm.yaml index 3a833c2d943..2a267c2fd86 100644 --- a/npm/deploy/npm/azure-npm.yaml +++ b/npm/deploy/npm/azure-npm.yaml @@ -151,8 +151,8 @@ data: "ListeningAddress": "0.0.0.0", "Toggles": { "EnablePrometheusMetrics": true, - "EnablePprof": true, - "EnableHTTPDebugAPI": true, + "EnablePprof": false, + "EnableHTTPDebugAPI": false, "EnableV2NPM": false, "PlaceAzureChainFirst": false }, diff --git a/npm/http/server/server.go b/npm/http/server/server.go index d20db191038..6c44b1708fc 100644 --- a/npm/http/server/server.go +++ b/npm/http/server/server.go @@ -3,19 +3,45 @@ package server import ( "encoding/json" "fmt" + "net" "net/http" "net/http/pprof" _ "net/http/pprof" + "time" "github.com/Azure/azure-container-networking/log" npmconfig "github.com/Azure/azure-container-networking/npm/config" "github.com/Azure/azure-container-networking/npm/http/api" "github.com/Azure/azure-container-networking/npm/metrics" + "golang.org/x/net/netutil" "k8s.io/klog" "github.com/gorilla/mux" ) +const ( + // The NPM API listens on the host network of a privileged process, so any pod on the node + // can reach it. Without deadlines a client that opens connections and then reads its + // response one byte at a time keeps a request, and the response buffer built for it, alive + // indefinitely. These deadlines bound how long any single client can hold those resources. + // They are generous enough for a Prometheus scrape of this endpoint. + readHeaderTimeout = 10 * time.Second + readTimeout = 30 * time.Second + writeTimeout = 60 * time.Second + idleTimeout = 120 * time.Second + maxHeaderBytes = 1 << 16 // 64 KiB + + // maxConcurrentConns bounds how many connections the API serves at once. Each in-flight + // request to the cache handler buffers a full copy of the policy cache, so without a + // ceiling the number of concurrent clients alone decides how much memory NPM allocates. + maxConcurrentConns = 32 + + // maxConcurrentCacheRequests bounds how many cache encodings run at once. The encoding + // holds the cache lock and buffers the whole payload, so it is the most expensive thing + // the API does and is kept well below the connection ceiling. + maxConcurrentCacheRequests = 2 +) + type NPMRestServer struct { listeningAddress string router *mux.Router @@ -53,16 +79,40 @@ func NPMRestServerListenAndServe(config npmconfig.Config, npmEncoder json.Marsha } srv := &http.Server{ - Handler: rs.router, - Addr: rs.listeningAddress, + Handler: rs.router, + Addr: rs.listeningAddress, + ReadHeaderTimeout: readHeaderTimeout, + ReadTimeout: readTimeout, + WriteTimeout: writeTimeout, + IdleTimeout: idleTimeout, + MaxHeaderBytes: maxHeaderBytes, + } + + listener, err := net.Listen("tcp", rs.listeningAddress) + if err != nil { + klog.Errorf("Failed to start NPM HTTP Server with error: %+v", err) + return } klog.Infof("Starting NPM HTTP API on %s... ", rs.listeningAddress) - klog.Errorf("Failed to start NPM HTTP Server with error: %+v", srv.ListenAndServe()) + klog.Errorf("Failed to start NPM HTTP Server with error: %+v", srv.Serve(netutil.LimitListener(listener, maxConcurrentConns))) } func (n *NPMRestServer) npmCacheHandler(npmCacheEncoder json.Marshaler) http.Handler { + // Admit only a few encodings at a time. Each one takes the cache lock and buffers the + // entire policy cache, so concurrent requests multiply both the lock hold time and the + // memory in flight. + inFlight := make(chan struct{}, maxConcurrentCacheRequests) + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + select { + case inFlight <- struct{}{}: + defer func() { <-inFlight }() + default: + http.Error(w, "too many concurrent cache requests", http.StatusServiceUnavailable) + return + } + b, err := json.Marshal(npmCacheEncoder) if err != nil { http.Error(w, err.Error(), 500) diff --git a/npm/http/server/server_test.go b/npm/http/server/server_test.go index 0cfe2333e63..fdfaa72bbd0 100644 --- a/npm/http/server/server_test.go +++ b/npm/http/server/server_test.go @@ -5,12 +5,14 @@ import ( "io" "net/http" "net/http/httptest" + "sync" "testing" "github.com/Azure/azure-container-networking/npm" "github.com/Azure/azure-container-networking/npm/http/api" "github.com/Azure/azure-container-networking/npm/pkg/controlplane/controllers/common" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestGetNPMCacheHandler(t *testing.T) { @@ -55,3 +57,72 @@ func TestGetNPMCacheHandler(t *testing.T) { assert.Exactly(expected, actual) } + +// blockingMarshaler blocks inside MarshalJSON until released, so a test can hold cache +// encodings in flight and observe what happens to further requests. +type blockingMarshaler struct { + entered chan struct{} + release chan struct{} +} + +func (b *blockingMarshaler) MarshalJSON() ([]byte, error) { + b.entered <- struct{}{} + <-b.release + return []byte("{}"), nil +} + +// TestNPMCacheHandlerLimitsConcurrency verifies that the cache handler admits only a bounded +// number of encodings at once. Each encoding holds the cache lock and buffers the whole +// policy cache, so without a ceiling the number of concurrent clients alone decides how much +// memory NPM allocates and how long the cache stays locked. +func TestNPMCacheHandlerLimitsConcurrency(t *testing.T) { + encoder := &blockingMarshaler{ + entered: make(chan struct{}, maxConcurrentCacheRequests), + release: make(chan struct{}), + } + n := &NPMRestServer{} + handler := n.npmCacheHandler(encoder) + + // Fill every slot and wait until each request is actually inside MarshalJSON. + var wg sync.WaitGroup + for i := 0; i < maxConcurrentCacheRequests; i++ { + wg.Add(1) + go func() { + defer wg.Done() + req := httptest.NewRequest(http.MethodGet, api.NPMMgrPath, nil) + handler.ServeHTTP(httptest.NewRecorder(), req) + }() + } + for i := 0; i < maxConcurrentCacheRequests; i++ { + <-encoder.entered + } + + // With every slot busy, a further request must be shed instead of queueing another + // full copy of the cache. + rr := httptest.NewRecorder() + handler.ServeHTTP(rr, httptest.NewRequest(http.MethodGet, api.NPMMgrPath, nil)) + require.Equal(t, http.StatusServiceUnavailable, rr.Code, + "a request beyond the in-flight limit must be shed") + + close(encoder.release) + wg.Wait() + + // Once the in-flight requests drain, the handler must serve again. + rr = httptest.NewRecorder() + handler.ServeHTTP(rr, httptest.NewRequest(http.MethodGet, api.NPMMgrPath, nil)) + require.Equal(t, http.StatusOK, rr.Code, "the handler must recover once slots free up") +} + +// TestServerTimeoutsAreSet is a guard on the deadlines and header bound. The API listens on +// the host network of a privileged process, so a client that never finishes a request must +// not be able to hold it, and the response buffered for it, open indefinitely. +func TestServerTimeoutsAreSet(t *testing.T) { + require.NotZero(t, readHeaderTimeout) + require.NotZero(t, readTimeout) + require.NotZero(t, writeTimeout) + require.NotZero(t, idleTimeout) + require.NotZero(t, maxHeaderBytes) + require.NotZero(t, maxConcurrentConns) + require.LessOrEqual(t, maxConcurrentCacheRequests, maxConcurrentConns, + "cache encodings must be bounded at or below the connection ceiling") +} From 180828a1c615b9a0720515a750c1d995eeaa3259 Mon Sep 17 00:00:00 2001 From: Isaiah Raya Date: Wed, 2 Sep 2026 21:36:30 +0000 Subject: [PATCH 06/23] chore: [NPM] address lint findings in the changed files Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- npm/http/server/server_test.go | 6 +++--- .../controllers/v2/networkPolicyController.go | 6 +++--- npm/pkg/controlplane/translation/parseSelector_test.go | 2 ++ npm/pkg/controlplane/translation/translatePolicy.go | 2 +- npm/pkg/controlplane/translation/translatePolicy_test.go | 8 +------- 5 files changed, 10 insertions(+), 14 deletions(-) diff --git a/npm/http/server/server_test.go b/npm/http/server/server_test.go index fdfaa72bbd0..5f1f578383b 100644 --- a/npm/http/server/server_test.go +++ b/npm/http/server/server_test.go @@ -89,7 +89,7 @@ func TestNPMCacheHandlerLimitsConcurrency(t *testing.T) { wg.Add(1) go func() { defer wg.Done() - req := httptest.NewRequest(http.MethodGet, api.NPMMgrPath, nil) + req := httptest.NewRequest(http.MethodGet, api.NPMMgrPath, http.NoBody) handler.ServeHTTP(httptest.NewRecorder(), req) }() } @@ -100,7 +100,7 @@ func TestNPMCacheHandlerLimitsConcurrency(t *testing.T) { // With every slot busy, a further request must be shed instead of queueing another // full copy of the cache. rr := httptest.NewRecorder() - handler.ServeHTTP(rr, httptest.NewRequest(http.MethodGet, api.NPMMgrPath, nil)) + handler.ServeHTTP(rr, httptest.NewRequest(http.MethodGet, api.NPMMgrPath, http.NoBody)) require.Equal(t, http.StatusServiceUnavailable, rr.Code, "a request beyond the in-flight limit must be shed") @@ -109,7 +109,7 @@ func TestNPMCacheHandlerLimitsConcurrency(t *testing.T) { // Once the in-flight requests drain, the handler must serve again. rr = httptest.NewRecorder() - handler.ServeHTTP(rr, httptest.NewRequest(http.MethodGet, api.NPMMgrPath, nil)) + handler.ServeHTTP(rr, httptest.NewRequest(http.MethodGet, api.NPMMgrPath, http.NoBody)) require.Equal(t, http.StatusOK, rr.Code, "the handler must recover once slots free up") } diff --git a/npm/pkg/controlplane/controllers/v2/networkPolicyController.go b/npm/pkg/controlplane/controllers/v2/networkPolicyController.go index 60442ac654d..5ccae8b7a7a 100644 --- a/npm/pkg/controlplane/controllers/v2/networkPolicyController.go +++ b/npm/pkg/controlplane/controllers/v2/networkPolicyController.go @@ -300,17 +300,17 @@ func (c *NetworkPolicyController) syncAddAndUpdateNetPol(netPolObj *networkingv1 return metrics.NoOp, nil } - klog.Errorf("Failed to translate NetworkPolicy %s in namespace %s: %s", netPolObj.ObjectMeta.Name, netPolObj.ObjectMeta.Namespace, err.Error()) + klog.Errorf("Failed to translate NetworkPolicy %s in namespace %s: %s", netPolObj.Name, netPolObj.Namespace, err.Error()) metrics.SendErrorLogAndMetric(util.NetpolID, "[syncAddAndUpdateNetPol] Error: failed to translate NetworkPolicy %s in namespace %s due to %v", - netPolObj.ObjectMeta.Name, netPolObj.ObjectMeta.Namespace, err) + netPolObj.Name, netPolObj.Namespace, err) // Do not report success here. Reporting success left the policy's selected pods with // no rules at all - not even the default drop the policy implies - while the policy // object appeared to be applied and nothing signalled the failure. Return the error so // it is surfaced and the key is requeued (rate limited) instead. // The exec time isn't relevant here, so consider a no-op. return metrics.NoOp, fmt.Errorf("[syncAddAndUpdateNetPol] Error: failed to translate NetworkPolicy %s in namespace %s: %w", - netPolObj.ObjectMeta.Name, netPolObj.ObjectMeta.Namespace, err) + netPolObj.Name, netPolObj.Namespace, err) } _, policyExisted := c.rawNpSpecMap[netpolKey] diff --git a/npm/pkg/controlplane/translation/parseSelector_test.go b/npm/pkg/controlplane/translation/parseSelector_test.go index f2541d7913e..1ec6e47fc0d 100644 --- a/npm/pkg/controlplane/translation/parseSelector_test.go +++ b/npm/pkg/controlplane/translation/parseSelector_test.go @@ -675,6 +675,8 @@ func TestFlattenNameSpaceSelectorMixedInAndNotIn(t *testing.T) { case metav1.LabelSelectorOpIn: require.Equal(t, "role", req.Key) inValues = append(inValues, req.Values[0]) + case metav1.LabelSelectorOpExists, metav1.LabelSelectorOpDoesNotExist: + t.Fatalf("unexpected valueless operator %s", req.Operator) default: t.Fatalf("unexpected operator %s", req.Operator) } diff --git a/npm/pkg/controlplane/translation/translatePolicy.go b/npm/pkg/controlplane/translation/translatePolicy.go index b0b0641095a..e67b0e72bf3 100644 --- a/npm/pkg/controlplane/translation/translatePolicy.go +++ b/npm/pkg/controlplane/translation/translatePolicy.go @@ -38,7 +38,7 @@ var ( // ErrEmptyMatchExpressionValues is returned when an In or NotIn matchExpression carries no values. // Kubernetes rejects such requirements; NPM fails closed rather than dropping the requirement, // which could otherwise widen a selector (e.g. a dropped NotIn) or yield no rules at all. - ErrEmptyMatchExpressionValues = errors.New("In and NotIn matchExpression requirements must have at least one value") + ErrEmptyMatchExpressionValues = errors.New("in and notIn matchExpression requirements must have at least one value") // ErrUnsupportedMatchExpressionOperator is returned when a matchExpression uses an operator that is // none of In, NotIn, Exists or DoesNotExist. NPM fails closed rather than dropping the requirement, // which could otherwise silently widen the selector. diff --git a/npm/pkg/controlplane/translation/translatePolicy_test.go b/npm/pkg/controlplane/translation/translatePolicy_test.go index 5531d7dcd26..9f530770fe1 100644 --- a/npm/pkg/controlplane/translation/translatePolicy_test.go +++ b/npm/pkg/controlplane/translation/translatePolicy_test.go @@ -1388,7 +1388,6 @@ func TestNameSpaceSelectorNegationOnlyIsScopedToNamespaces(t *testing.T) { } for _, tt := range tests { - tt := tt t.Run(tt.name, func(t *testing.T) { nsSelectorIPSets, nsSelectorList := nameSpaceSelector(matchType, tt.selector) require.ElementsMatch(t, tt.expected, nsSelectorList) @@ -1452,7 +1451,6 @@ func TestNameSpaceSelectorWithPositiveMatchIsUnchanged(t *testing.T) { } for _, tt := range tests { - tt := tt t.Run(tt.name, func(t *testing.T) { _, nsSelectorList := nameSpaceSelector(matchType, tt.selector) require.ElementsMatch(t, tt.expected, nsSelectorList) @@ -1490,7 +1488,6 @@ func TestTranslatePolicyNegationOnlyNamespaceSelector(t *testing.T) { } for _, tt := range tests { - tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() @@ -1589,7 +1586,6 @@ func TestTranslatePolicyMultiValueNotInConjunction(t *testing.T) { } for _, tt := range tests { - tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() @@ -3899,7 +3895,6 @@ func TestTranslatePolicyNonCanonicalAllAddressesCIDR(t *testing.T) { require.NoError(t, err) for _, cidr := range []string{"10.0.0.0/0", "255.255.255.255/0"} { - cidr := cidr t.Run(cidr, func(t *testing.T) { t.Parallel() @@ -3910,7 +3905,7 @@ func TestTranslatePolicyNonCanonicalAllAddressesCIDR(t *testing.T) { // The policy must be indistinguishable from the canonical spelling: same // ipset members (the 0.0.0.0/0 split) and the same ACLs. require.Equal(t, canonical.RuleIPSets, npmNetPol.RuleIPSets) - require.Equal(t, len(canonical.ACLs), len(npmNetPol.ACLs)) + require.Len(t, npmNetPol.ACLs, len(canonical.ACLs)) // Most importantly the default drop must exist, since its absence is what // left the selected pods unisolated. @@ -3931,7 +3926,6 @@ func TestTranslatePolicyInvalidCIDRStillFails(t *testing.T) { t.Parallel() for _, cidr := range []string{"2001:db8::/32", "10.0.0.0/33", "not-a-cidr/0"} { - cidr := cidr t.Run(cidr, func(t *testing.T) { t.Parallel() npmNetPol, err := TranslatePolicy(ipBlockPolicy("victim", "default", cidr), false) From ba729118fb32d5b046d6c48c4507dddd8ddddd2c Mon Sep 17 00:00:00 2001 From: Isaiah Raya Date: Wed, 2 Sep 2026 21:54:44 +0000 Subject: [PATCH 07/23] test: [NPM] cover every negation-only namespaceSelector operator and its rendered rule The existing coverage for negation-only namespace peers used a single-value NotIn. Extend it to every operator that can produce a negation-only selector, DoesNotExist included, in both directions, and assert the shape of the resulting decision: exactly one allow ACL, the exclusion still negated, and exactly one positive set, the all-namespaces anchor. Also pin how that decision reaches the kernel. A negated set match is satisfied by every address absent from the set, including addresses that are not pods, so a rule whose peer list is only negations matches non-pod traffic. The new emission test asserts the anchor renders as a positive match-set alongside the negation, and that a namespace peer never renders as a lone negated match. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../translation/translatePolicy_test.go | 119 ++++++++++++++++++ .../policies/policymanager_linux_test.go | 53 ++++++++ 2 files changed, 172 insertions(+) diff --git a/npm/pkg/controlplane/translation/translatePolicy_test.go b/npm/pkg/controlplane/translation/translatePolicy_test.go index 9f530770fe1..cdcb3fdffcf 100644 --- a/npm/pkg/controlplane/translation/translatePolicy_test.go +++ b/npm/pkg/controlplane/translation/translatePolicy_test.go @@ -3934,3 +3934,122 @@ func TestTranslatePolicyInvalidCIDRStillFails(t *testing.T) { }) } } + +// nsExprPolicy builds a NetworkPolicy that selects all local pods and, for the given +// direction, admits peers whose namespace satisfies the single given matchExpression. +func nsExprPolicy(name, ns string, direction networkingv1.PolicyType, req metav1.LabelSelectorRequirement) *networkingv1.NetworkPolicy { + peer := networkingv1.NetworkPolicyPeer{ + NamespaceSelector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{req}, + }, + } + pol := &networkingv1.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: ns}, + Spec: networkingv1.NetworkPolicySpec{ + PodSelector: metav1.LabelSelector{}, + PolicyTypes: []networkingv1.PolicyType{direction}, + }, + } + if direction == networkingv1.PolicyTypeIngress { + pol.Spec.Ingress = []networkingv1.NetworkPolicyIngressRule{{From: []networkingv1.NetworkPolicyPeer{peer}}} + } else { + pol.Spec.Egress = []networkingv1.NetworkPolicyEgressRule{{To: []networkingv1.NetworkPolicyPeer{peer}}} + } + return pol +} + +// TestTranslatePolicyNegationOnlyOperators covers every operator that can produce a +// negation-only namespaceSelector, in both directions. +// +// A namespaceSelector selects pods in matching namespaces, and NPM's namespace sets hold +// pod IPs. A negated set match is satisfied by any address absent from that set, so an +// address that is not a pod in any namespace satisfies it too. Without a positive set to +// intersect with, the negation alone is the whole match and the rule admits non-pod +// addresses: on ingress a routable non-pod host reaching the pod directly, on egress the +// selected pod reaching arbitrary external hosts. +// +// Each case asserts the allow decision carries the all-namespaces anchor, which is the +// positive match that keeps the decision inside the pod domain. +func TestTranslatePolicyNegationOnlyOperators(t *testing.T) { + t.Parallel() + + operators := []struct { + name string + req metav1.LabelSelectorRequirement + excluded string + setType ipsets.SetType + }{ + { + name: "DoesNotExist", + req: metav1.LabelSelectorRequirement{Key: "blocked", Operator: metav1.LabelSelectorOpDoesNotExist}, + excluded: "blocked", + setType: ipsets.KeyLabelOfNamespace, + }, + { + name: "single-value NotIn", + req: metav1.LabelSelectorRequirement{Key: "blocked", Operator: metav1.LabelSelectorOpNotIn, Values: []string{"yes"}}, + excluded: "blocked:yes", + setType: ipsets.KeyValueLabelOfNamespace, + }, + { + name: "multi-value NotIn", + req: metav1.LabelSelectorRequirement{Key: "blocked", Operator: metav1.LabelSelectorOpNotIn, Values: []string{"yes", "maybe"}}, + excluded: "blocked:yes", + setType: ipsets.KeyValueLabelOfNamespace, + }, + } + + directions := []struct { + name string + direction networkingv1.PolicyType + matchType policies.MatchType + peerList func(*policies.ACLPolicy) []policies.SetInfo + }{ + {"ingress", networkingv1.PolicyTypeIngress, policies.SrcMatch, func(a *policies.ACLPolicy) []policies.SetInfo { return a.SrcList }}, + {"egress", networkingv1.PolicyTypeEgress, policies.DstMatch, func(a *policies.ACLPolicy) []policies.SetInfo { return a.DstList }}, + } + + for _, op := range operators { + for _, dir := range directions { + t.Run(op.name+"/"+dir.name, func(t *testing.T) { + t.Parallel() + + npmNetPol, err := TranslatePolicy(nsExprPolicy("victim", "default", dir.direction, op.req), false) + require.NoError(t, err) + + var allowACLs int + var theAllow *policies.ACLPolicy + for i := range npmNetPol.ACLs { + if npmNetPol.ACLs[i].Target == policies.Allowed { + allowACLs++ + theAllow = npmNetPol.ACLs[i] + } + } + // One decision only: every negation must be ANDed into it, never split + // into additive allow decisions. + require.Equal(t, 1, allowACLs, "there must be exactly one allow ACL") + require.NotNil(t, theAllow) + + peers := dir.peerList(theAllow) + anchor := policies.NewSetInfo(util.KubeAllNamespacesFlag, ipsets.KeyLabelOfNamespace, included, dir.matchType) + require.Contains(t, peers, anchor, + "a negation-only namespaceSelector must carry the all-namespaces anchor, "+ + "otherwise the negation alone also matches addresses that are not pods") + + // The exclusion itself must still be present and still negated. + require.Contains(t, peers, + policies.NewSetInfo(op.excluded, op.setType, nonIncluded, dir.matchType)) + + // Exactly one positive set: the anchor. Anything else positive would + // widen the decision beyond what the selector asked for. + var positives []string + for _, si := range peers { + if si.Included { + positives = append(positives, si.IPSet.Name) + } + } + require.Equal(t, []string{util.KubeAllNamespacesFlag}, positives) + }) + } + } +} diff --git a/npm/pkg/dataplane/policies/policymanager_linux_test.go b/npm/pkg/dataplane/policies/policymanager_linux_test.go index 8fa044e373a..6064d4b4239 100644 --- a/npm/pkg/dataplane/policies/policymanager_linux_test.go +++ b/npm/pkg/dataplane/policies/policymanager_linux_test.go @@ -517,3 +517,56 @@ func TestUpdatingStaleChains(t *testing.T) { require.NoError(t, pMgr.AddPolicies([]*NPMNetworkPolicy{bothDirectionsNetPol}, nil)) assertStaleChainsContain(t, pMgr.staleChains, egressNetPolChain) } + +// TestNegationOnlyPeerRendersAnchor asserts how a negation-only namespace peer reaches the +// kernel. A negated set match (`! --match-set`) is satisfied by every address absent from +// that set, including addresses that are not pods at all, so an ACL whose peer list is only +// negations matches non-pod traffic. The all-namespaces anchor is what confines the decision +// to the pod domain, and this test pins that it renders as a positive `--match-set` in the +// same rule as the negation, for both directions. +func TestNegationOnlyPeerRendersAnchor(t *testing.T) { + anchor := ipsets.NewIPSetMetadata(util.KubeAllNamespacesFlag, ipsets.KeyLabelOfNamespace) + excluded := ipsets.NewIPSetMetadata("blocked", ipsets.KeyLabelOfNamespace) + + tests := []struct { + name string + matchType MatchType + direction string + }{ + {"ingress", SrcMatch, "src"}, + {"egress", DstMatch, "dst"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + acl := &ACLPolicy{ + Target: Allowed, + Direction: Ingress, + } + peers := []SetInfo{ + NewSetInfo(util.KubeAllNamespacesFlag, ipsets.KeyLabelOfNamespace, true, tt.matchType), + NewSetInfo("blocked", ipsets.KeyLabelOfNamespace, false, tt.matchType), + } + if tt.matchType == SrcMatch { + acl.SrcList = peers + } else { + acl.DstList = peers + } + + specs := strings.Join(iptablesRuleSpecs(acl), " ") + + // The anchor must be a positive match, so only pod addresses can satisfy it. + require.Contains(t, specs, + strings.Join([]string{util.IptablesMatchSetFlag, anchor.GetHashedName(), tt.direction}, " "), + "the all-namespaces anchor must render as a positive match-set") + // The exclusion must remain negated. + require.Contains(t, specs, + strings.Join([]string{util.IptablesNotFlag, util.IptablesMatchSetFlag, excluded.GetHashedName(), tt.direction}, " "), + "the excluded namespace label must render as a negated match-set") + // The negation must not be the only match in the rule, which is the shape + // that admits non-pod addresses. + require.NotEqual(t, 1, strings.Count(specs, util.IptablesMatchSetFlag), + "a negation-only rule must never be emitted for a namespace peer") + }) + } +} From 5de95df45c299005677a3a170380e0ab96648276 Mon Sep 17 00:00:00 2001 From: Isaiah Raya Date: Thu, 3 Sep 2026 16:05:19 +0000 Subject: [PATCH 08/23] fix: [NPM] bound generated rules per policy and keep CIDR canonicalization on the Linux path Two follow-ups from review of the earlier commits. Bounding the flattened namespaceSelector count is not sufficient on its own. Every flattened branch is emitted once per port in the rule, and that product is summed across peers and rules, so a policy whose selector expansion sits comfortably under the selector limit can still multiply itself out by listing many ports: 512 branches against 512 ports is 262144 rules. Add a per-policy ACL ceiling, checked before each peer is expanded so translation stops early rather than after materializing the product, and again before the policy is returned. The earlier CIDR fix worked by broadening the shared IsIPV4 classifier, but that classifier is also consumed by the Windows and NPM Lite direct-rule paths, which write the CIDR into the ACL rather than into an ipset. Broadening it therefore changed behavior in components that are out of scope here. IsIPV4, deDuplicateExcept and the dataplane ipset member check are left exactly as they were, so those paths are byte-identical to before. The Linux ipBlock path validates through NormalizeCIDR instead, which canonicalizes first, and canonicalizes its except CIDRs through a helper used only by that path. NormalizeCIDR itself is purely additive. Finally, default the debug and profiling routes to off in DefaultConfig. The deployment manifests already disable them, but a missing or unreadable config file falls back to this struct, so the fallback must not be the configuration that exposes unauthenticated routes on the host network. Also admit a single cache encoding at a time rather than two, keeping peak memory to one copy of the cache. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- npm/config/config.go | 14 ++-- npm/http/server/server.go | 5 +- .../controllers/v2/networkPolicyController.go | 10 ++- .../translation/parseSelector_test.go | 79 +++++++++++++++++++ .../translation/translatePolicy.go | 78 +++++++++++++++--- npm/pkg/dataplane/ipsets/ipsetmanager.go | 14 +--- npm/util/util.go | 24 ++++-- npm/util/util_test.go | 15 ++-- 8 files changed, 190 insertions(+), 49 deletions(-) diff --git a/npm/config/config.go b/npm/config/config.go index c0a592c969f..5eb1cbc6f8e 100644 --- a/npm/config/config.go +++ b/npm/config/config.go @@ -42,11 +42,15 @@ var DefaultConfig = Config{ Toggles: Toggles{ EnablePrometheusMetrics: true, - EnablePprof: true, - EnableHTTPDebugAPI: true, - EnableV2NPM: true, - PlaceAzureChainFirst: util.PlaceAzureChainAfterKubeServices, - ApplyIPSetsOnNeed: false, + // The debug and profiling routes are served unauthenticated on the host network, so + // they are opt-in rather than on by default. This matters most when the config file is + // missing or unreadable, since that falls back to this struct: the fallback must not be + // the configuration that exposes them. + EnablePprof: false, + EnableHTTPDebugAPI: false, + EnableV2NPM: true, + PlaceAzureChainFirst: util.PlaceAzureChainAfterKubeServices, + ApplyIPSetsOnNeed: false, // ApplyInBackground is currently used in Windows to apply the following in background: IPSets and NetPols for new/updated Pods ApplyInBackground: true, // NetPolInBackground is currently used in Linux to apply NetPol controller Add events in the background diff --git a/npm/http/server/server.go b/npm/http/server/server.go index 6c44b1708fc..f3d84332024 100644 --- a/npm/http/server/server.go +++ b/npm/http/server/server.go @@ -38,8 +38,9 @@ const ( // maxConcurrentCacheRequests bounds how many cache encodings run at once. The encoding // holds the cache lock and buffers the whole payload, so it is the most expensive thing - // the API does and is kept well below the connection ceiling. - maxConcurrentCacheRequests = 2 + // the API does. One at a time keeps peak memory to a single copy of the cache; excess + // requests are shed rather than queued. + maxConcurrentCacheRequests = 1 ) type NPMRestServer struct { diff --git a/npm/pkg/controlplane/controllers/v2/networkPolicyController.go b/npm/pkg/controlplane/controllers/v2/networkPolicyController.go index 5ccae8b7a7a..c09544dede3 100644 --- a/npm/pkg/controlplane/controllers/v2/networkPolicyController.go +++ b/npm/pkg/controlplane/controllers/v2/networkPolicyController.go @@ -300,14 +300,16 @@ func (c *NetworkPolicyController) syncAddAndUpdateNetPol(netPolObj *networkingv1 return metrics.NoOp, nil } - klog.Errorf("Failed to translate NetworkPolicy %s in namespace %s: %s", netPolObj.Name, netPolObj.Namespace, err.Error()) - metrics.SendErrorLogAndMetric(util.NetpolID, - "[syncAddAndUpdateNetPol] Error: failed to translate NetworkPolicy %s in namespace %s due to %v", - netPolObj.Name, netPolObj.Namespace, err) // Do not report success here. Reporting success left the policy's selected pods with // no rules at all - not even the default drop the policy implies - while the policy // object appeared to be applied and nothing signalled the failure. Return the error so // it is surfaced and the key is requeued (rate limited) instead. + // + // The error is deliberately not logged or counted here: processNextWorkItem already + // runs the returned error through utilruntime.HandleError and SendErrorLogAndMetric, + // so recording it here as well would emit the same failure three times. The wrapped + // message carries the policy name and namespace so that single record stays specific. + // // The exec time isn't relevant here, so consider a no-op. return metrics.NoOp, fmt.Errorf("[syncAddAndUpdateNetPol] Error: failed to translate NetworkPolicy %s in namespace %s: %w", netPolObj.Name, netPolObj.Namespace, err) diff --git a/npm/pkg/controlplane/translation/parseSelector_test.go b/npm/pkg/controlplane/translation/parseSelector_test.go index 1ec6e47fc0d..c1933209bbd 100644 --- a/npm/pkg/controlplane/translation/parseSelector_test.go +++ b/npm/pkg/controlplane/translation/parseSelector_test.go @@ -8,6 +8,7 @@ import ( "github.com/stretchr/testify/require" networkingv1 "k8s.io/api/networking/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/intstr" ) func TestFlattenNameSpaceSelectorCases(t *testing.T) { @@ -839,3 +840,81 @@ func TestIsValidLabel(t *testing.T) { require.False(t, isValidLabelValue(b), "string was [%s]", b) } } + +// TestTranslatePolicyACLBudget covers the multiplication the selector cap alone does not +// catch. Every flattened namespaceSelector branch is emitted once per port in the rule, so a +// policy whose selector expansion is comfortably under the selector limit can still generate +// an enormous number of ACLs by listing many ports. Each ACL becomes an iptables rule. +func TestTranslatePolicyACLBudget(t *testing.T) { + // 2^9 = 512 flattened selectors: under maxFlattenedNSSelectors. + reqs := make([]metav1.LabelSelectorRequirement, 0, 9) + for i := 0; i < 9; i++ { + reqs = append(reqs, metav1.LabelSelectorRequirement{ + Key: fmt.Sprintf("key%d", i), + Operator: metav1.LabelSelectorOpIn, + Values: []string{"a", "b"}, + }) + } + + // Sanity: the selector expansion on its own is accepted. + flattened, err := flattenNameSpaceSelector(&metav1.LabelSelector{MatchExpressions: reqs}) + require.NoError(t, err) + require.Len(t, flattened, 512) + + // 512 selectors x 512 ports would be 262144 ACLs. + ports := make([]networkingv1.NetworkPolicyPort, 0, 512) + for i := 0; i < 512; i++ { + p := intstr.FromInt(1000 + i) + ports = append(ports, networkingv1.NetworkPolicyPort{Port: &p}) + } + + pol := &networkingv1.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: "expand", Namespace: "default"}, + Spec: networkingv1.NetworkPolicySpec{ + PodSelector: metav1.LabelSelector{}, + PolicyTypes: []networkingv1.PolicyType{networkingv1.PolicyTypeIngress}, + Ingress: []networkingv1.NetworkPolicyIngressRule{{ + Ports: ports, + From: []networkingv1.NetworkPolicyPeer{ + {NamespaceSelector: &metav1.LabelSelector{MatchExpressions: reqs}}, + }, + }}, + }, + } + + npmNetPol, err := TranslatePolicy(pol, false) + require.ErrorIs(t, err, ErrTooManyACLs, + "a policy that multiplies selectors by ports must be rejected even when the selector count is under its own limit") + require.Nil(t, npmNetPol) +} + +// TestTranslatePolicyOrdinaryPolicyWithinACLBudget guards the budget against false positives: +// a normal policy with several peers and ports must translate unaffected. +func TestTranslatePolicyOrdinaryPolicyWithinACLBudget(t *testing.T) { + ports := make([]networkingv1.NetworkPolicyPort, 0, 8) + for i := 0; i < 8; i++ { + p := intstr.FromInt(8000 + i) + ports = append(ports, networkingv1.NetworkPolicyPort{Port: &p}) + } + + pol := &networkingv1.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: "normal", Namespace: "default"}, + Spec: networkingv1.NetworkPolicySpec{ + PodSelector: metav1.LabelSelector{MatchLabels: map[string]string{"app": "web"}}, + PolicyTypes: []networkingv1.PolicyType{networkingv1.PolicyTypeIngress}, + Ingress: []networkingv1.NetworkPolicyIngressRule{{ + Ports: ports, + From: []networkingv1.NetworkPolicyPeer{ + {NamespaceSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"team": "blue"}}}, + {PodSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"role": "client"}}}, + {IPBlock: &networkingv1.IPBlock{CIDR: "10.0.0.0/8"}}, + }, + }}, + }, + } + + npmNetPol, err := TranslatePolicy(pol, false) + require.NoError(t, err) + require.NotNil(t, npmNetPol) + require.Less(t, len(npmNetPol.ACLs), maxACLsPerPolicy) +} diff --git a/npm/pkg/controlplane/translation/translatePolicy.go b/npm/pkg/controlplane/translation/translatePolicy.go index e67b0e72bf3..ea929cd1ba5 100644 --- a/npm/pkg/controlplane/translation/translatePolicy.go +++ b/npm/pkg/controlplane/translation/translatePolicy.go @@ -47,6 +47,10 @@ var ( // requirements would produce more labelSelectors than NPM is willing to translate. The count is // the product of the value counts, so it grows exponentially with the number of such requirements. ErrTooManyFlattenedSelectors = errors.New("namespaceSelector expands into too many label selectors") + // ErrTooManyACLs is returned when a NetworkPolicy translates into more ACLs than NPM is + // willing to program. ACL count multiplies rather than adds: flattened selector branches are + // emitted per port, summed across peers and rules, so bounding selectors alone is not enough. + ErrTooManyACLs = errors.New("network policy expands into too many rules") // 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 are passed in with NPM Lite enabled. NPM Lite allows deny-all and allow-all policies @@ -167,13 +171,26 @@ func exceptCidr(exceptCidr string) string { return exceptCidr + " " + util.IpsetNomatch } -// deDuplicateExcept canonicalizes each except CIDR and removes redundant elements, returning -// a slice which has only unique elements. Canonicalizing first means two spellings of the same -// block (e.g. "10.1.2.0/24" and "10.1.2.3/24") collapse to one entry and that the result can be -// compared against the split-CIDR entries below. +// deDuplicateExcept removes redundance elements and return slices which has only unique element. func deDuplicateExcept(exceptInIPBlock []string) []string { deDupExcepts := []string{} exceptsSet := make(map[string]struct{}) + for _, except := range exceptInIPBlock { + if _, exist := exceptsSet[except]; !exist { + deDupExcepts = append(deDupExcepts, except) + exceptsSet[except] = struct{}{} + } + } + return deDupExcepts +} + +// canonicalizeExcepts returns the except CIDRs in canonical form, with duplicates removed. +// Canonicalizing first means two spellings of the same block (e.g. "10.1.2.0/24" and +// "10.1.2.3/24") collapse to one entry, and that an except can be compared against the +// all-addresses split entries below. This is used only on the ipset path. +func canonicalizeExcepts(exceptInIPBlock []string) []string { + canonicalExcepts := []string{} + exceptsSet := make(map[string]struct{}) for _, except := range exceptInIPBlock { canonical, ok := util.NormalizeCIDR(except) if !ok { @@ -182,11 +199,11 @@ func deDuplicateExcept(exceptInIPBlock []string) []string { canonical = except } if _, exist := exceptsSet[canonical]; !exist { - deDupExcepts = append(deDupExcepts, canonical) + canonicalExcepts = append(canonicalExcepts, canonical) exceptsSet[canonical] = struct{}{} } } - return deDupExcepts + return canonicalExcepts } // ipBlockIPSet return translatedIPSet based based on ipBlockRule. @@ -204,8 +221,9 @@ func ipBlockIPSet(policyName, ns string, direction policies.Direction, ipBlockSe return nil, ErrUnsupportedIPAddress } - // de-duplicated Except if there are redundance elements. - deDupExcepts := deDuplicateExcept(ipBlockRule.Except) + // de-duplicated Except if there are redundance elements, in canonical form so they + // compare correctly against the all-addresses split entries below. + deDupExcepts := canonicalizeExcepts(ipBlockRule.Except) lenOfDeDupExcepts := len(deDupExcepts) if util.IsWindowsDP() && lenOfDeDupExcepts > 0 { @@ -263,7 +281,13 @@ func ipBlockRule(policyName, ns string, direction policies.Direction, matchType return nil, policies.SetInfo{}, nil } - if !util.IsIPV4(ipBlockRule.CIDR) { + // Validate the canonical form rather than the literal the user wrote. A block whose host + // bits are set, such as "10.0.0.0/0", denotes exactly the same addresses as its canonical + // form, but IsIPV4 refuses a /0 that is not spelled "0.0.0.0". Rejecting here aborts the + // translation of the whole policy, so neither the peer rule nor the default drop the policy + // implies is installed and the selected pods are left with no rules at all. This is the + // ipset path, which is Linux only; the Windows direct-rule path is unchanged. + if _, ok := util.NormalizeCIDR(ipBlockRule.CIDR); !ok { return nil, policies.SetInfo{}, ErrUnsupportedIPAddress } @@ -369,6 +393,10 @@ func ruleExists(ports []networkingv1.NetworkPolicyPort, peer []networkingv1.Netw // peerAndPortRule deals with composite rules including ports and peers // (e.g., IPBlock, podSelector, namespaceSelector, or both podSelector and namespaceSelector). func peerAndPortRule(npmNetPol *policies.NPMNetworkPolicy, direction policies.Direction, ports []networkingv1.NetworkPolicyPort, setInfo []policies.SetInfo, npmLiteToggle bool) error { + if err := checkACLBudget(npmNetPol); err != nil { + return err + } + if len(ports) == 0 { acl := policies.NewACLPolicy(policies.Allowed, direction) acl.AddSetInfo(setInfo) @@ -427,10 +455,15 @@ func exceptDirectDropRules(npmNetPol *policies.NPMNetworkPolicy, direction polic func directPeerAndPortAllowRule(npmNetPol *policies.NPMNetworkPolicy, direction policies.Direction, ports []networkingv1.NetworkPolicyPort, cidr string, except []string, npmLiteToggle bool) error { // Match the ipset-based ipBlockRule path and reject non-IPv4 enclosing CIDRs (e.g. IPv6), // which HNS direct-IP ACLs cannot express. Failing closed avoids programming an ACL that - // silently ignores the peer. + // silently ignores the peer. This path is Windows/NPM Lite only and is intentionally left + // on the unchanged IsIPV4 behavior. if !util.IsIPV4(cidr) { return ErrUnsupportedIPAddress } + + if err := checkACLBudget(npmNetPol); err != nil { + return err + } if len(ports) == 0 { acl := policies.NewACLPolicy(policies.Allowed, direction) // bypasses ipset creation for /32 cidrs and directly creates an acl with the cidr @@ -793,9 +826,34 @@ func TranslatePolicy(npObj *networkingv1.NetworkPolicy, npmLiteToggle bool) (*po } } } + + if err := checkACLBudget(npmNetPol); err != nil { + return nil, err + } + return npmNetPol, nil } +// maxACLsPerPolicy bounds how many ACLs a single NetworkPolicy may translate into. Each ACL +// becomes one iptables rule, and the count multiplies rather than adds: every flattened +// namespaceSelector branch is emitted once per port in the rule, and that product is summed +// across every peer and every rule in the policy. Bounding the flattened selector count on +// its own is therefore not enough, because a policy that stays under that bound can still +// multiply itself out by listing many ports. The ceiling is far above any workable policy, +// since a policy expanding this wide would already be unusable as iptables rules. +const maxACLsPerPolicy = 2000 + +// checkACLBudget reports whether the policy has grown past what NPM is willing to translate. +// It is checked before each peer is expanded, so translation stops early rather than after +// materializing the full product. +func checkACLBudget(npmNetPol *policies.NPMNetworkPolicy) error { + if len(npmNetPol.ACLs) > maxACLsPerPolicy { + klog.Errorf("network policy %s expands past the %d ACL limit", npmNetPol.PolicyKey, maxACLsPerPolicy) + return ErrTooManyACLs + } + return nil +} + func checkForNamedPortType(npmNetPol *policies.NPMNetworkPolicy, portKind netpolPortType, npmLiteToggle bool, direction policies.Direction, port *networkingv1.NetworkPolicyPort, cidr string) error { if npmLiteToggle && portKind == namedPortType { return fmt.Errorf("named port not supported in policy %s (namespace: %s, direction: %s, cidr: %s, port: %v, protocol: %v): %w", diff --git a/npm/pkg/dataplane/ipsets/ipsetmanager.go b/npm/pkg/dataplane/ipsets/ipsetmanager.go index 45912e5c503..80b7575d8d7 100644 --- a/npm/pkg/dataplane/ipsets/ipsetmanager.go +++ b/npm/pkg/dataplane/ipsets/ipsetmanager.go @@ -658,17 +658,5 @@ func validateIPSetMemberIP(ip string) bool { ipDetails := strings.Split(ip, ",") ipField := strings.Split(ipDetails[0], " ") - if !util.IsIPV4(ipField[0]) { - return false - } - - // A CIDR member must already be canonical. Translation canonicalizes every CIDR it - // emits, so a member with host bits set did not come from a translated policy and - // must not be handed to the kernel under a name that denotes a different block. - if strings.Contains(ipField[0], "/") { - canonical, ok := util.NormalizeCIDR(ipField[0]) - return ok && canonical == ipField[0] - } - - return true + return util.IsIPV4(ipField[0]) } diff --git a/npm/util/util.go b/npm/util/util.go index b9edc4b6ee3..bf208ee5b19 100644 --- a/npm/util/util.go +++ b/npm/util/util.go @@ -377,21 +377,29 @@ func NormalizeCIDR(s string) (string, bool) { } // IsIPV4 returns true when ip is an IPv4 address or an IPv4 CIDR block. -// A CIDR is validated through NormalizeCIDR, which canonicalizes it first, so a block that -// is spelled with host bits set (for example "10.0.0.0/0") is recognized as the block it -// denotes instead of being rejected. Rejecting such a block would fail the whole policy -// translation and leave the policy's selected pods with no rules at all. +// +// Note this rejects a /0 block whose address text is not literally "0.0.0.0", even though such +// a block is valid and denotes the same addresses. Callers on the Linux ipBlock path must +// therefore canonicalize with NormalizeCIDR before validating, so a valid block is not refused +// on spelling alone. This function's behavior is deliberately left unchanged because it is also +// consumed by the Windows and NPM Lite paths, which are not in scope for these changes. func IsIPV4(ip string) bool { - if strings.Contains(ip, "/") { - _, ok := NormalizeCIDR(ip) - return ok + isIPBlock := strings.Contains(ip, "/") + ipOnly := strings.Split(ip, "/") + if strings.Contains(ip, "/0") && ipOnly[0] != "0.0.0.0" { + return false } - address, err := netip.ParseAddr(ip) + address, err := netip.ParseAddr(ipOnly[0]) if err != nil { return false } + if address.Is4() && isIPBlock { + _, _, err := net.ParseCIDR(ip) + return err == nil + } + return address.Is4() } diff --git a/npm/util/util_test.go b/npm/util/util_test.go index a6cb83ceb82..0878b344d7b 100644 --- a/npm/util/util_test.go +++ b/npm/util/util_test.go @@ -515,19 +515,16 @@ func TestHashedNameGoldenVectors(t *testing.T) { } } -// TestIsIPV4 covers address and CIDR forms. The CIDR cases matter most: a block whose host -// bits are set (e.g. "10.0.0.0/0") denotes the same addresses as its canonical form and must -// be accepted, because rejecting it fails the whole policy translation and leaves the -// policy's selected pods with no rules at all. +// TestIsIPV4 pins the existing behavior of the shared classifier. It is deliberately left +// unchanged by these fixes because the Windows and NPM Lite paths also consume it, and those +// are out of scope. Note it refuses a /0 block that is not spelled "0.0.0.0" even though such a +// block is valid; the Linux ipBlock path therefore validates via NormalizeCIDR instead. func TestIsIPV4(t *testing.T) { valid := []string{ "10.0.0.1", "0.0.0.0", "10.0.0.0/24", "0.0.0.0/0", - // non-canonical spellings of valid IPv4 blocks - "10.0.0.0/0", - "255.255.255.255/0", "10.1.2.3/24", "10.0.0.1/32", } @@ -544,6 +541,10 @@ func TestIsIPV4(t *testing.T) { "2001:db8::1", "2001:db8::/32", "::/0", + // a valid but non-canonical /0: refused on spelling, which is why the Linux + // ipBlock path canonicalizes before validating. + "10.0.0.0/0", + "255.255.255.255/0", } for _, ip := range invalid { require.False(t, IsIPV4(ip), "IsIPV4(%q) must be false", ip) From f359ab3ab0d934a4e441f618016a72eeff7b6055 Mon Sep 17 00:00:00 2001 From: Isaiah Raya Date: Thu, 3 Sep 2026 18:16:41 +0000 Subject: [PATCH 09/23] fix: [NPM] materialize ipsets in the kernel only when a policy references them NPM creates two ipsets per distinct pod label, on every node, and Kubernetes places no limit on how many labels a pod may carry. Applying every set to the kernel unconditionally meant one pod with tens of thousands of labels pushed thousands of sets into every node's kernel, pinned agent CPU and memory until agents were OOM killed, and delayed policy programming in unrelated namespaces while they recovered. Default to on-demand, so a set reaches the kernel only once a network policy references it. Incidental pod labels, which are the attacker-influenced input, never become kernel state at all. This removes the amplification without weakening enforcement, and specifically without the failure modes a creation limit has. The sets are still tracked and pods still join them, so a set is already populated by the time a policy references it. A limit instead has to refuse creating a set, which either blocks a policy from installing or leaves it referencing an empty set, and in both cases a pod escapes the policy that selects it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- npm/azure-npm.yaml | 1 + npm/config/config.go | 10 +++++++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/npm/azure-npm.yaml b/npm/azure-npm.yaml index f95ff91f9e0..fe1d76de260 100644 --- a/npm/azure-npm.yaml +++ b/npm/azure-npm.yaml @@ -169,6 +169,7 @@ data: "EnablePprof": false, "EnableHTTPDebugAPI": false, "EnableV2NPM": true, + "ApplyIPSetsOnNeed": true, "PlaceAzureChainFirst": false, "ApplyInBackground": true, "NetPolInBackground": true diff --git a/npm/config/config.go b/npm/config/config.go index 5eb1cbc6f8e..929ef6339f6 100644 --- a/npm/config/config.go +++ b/npm/config/config.go @@ -50,7 +50,15 @@ var DefaultConfig = Config{ EnableHTTPDebugAPI: false, EnableV2NPM: true, PlaceAzureChainFirst: util.PlaceAzureChainAfterKubeServices, - ApplyIPSetsOnNeed: false, + // Materialize an ipset in the kernel only once a network policy references it. NPM + // creates two sets per distinct pod label and label count is attacker-controlled, so + // applying every set unconditionally lets one namespace push tens of thousands of sets + // into the kernel on every node, exhaust the agent, and stall policy programming + // cluster-wide. On-demand keeps incidental labels out of kernel state entirely. + // + // This does not weaken enforcement: the sets are still tracked and pods still join + // them, so a set is already populated by the time a policy references it. + ApplyIPSetsOnNeed: true, // ApplyInBackground is currently used in Windows to apply the following in background: IPSets and NetPols for new/updated Pods ApplyInBackground: true, // NetPolInBackground is currently used in Linux to apply NetPol controller Add events in the background From f6475ce3e7831e50927e8316825a58c33388925e Mon Sep 17 00:00:00 2001 From: Isaiah Raya Date: Thu, 3 Sep 2026 23:32:58 +0000 Subject: [PATCH 10/23] fix: [NPM] keep the rule budget off the direct-rule path and make two tests meaningful Three review follow-ups. The per-policy ACL budget was still being checked in directPeerAndPortAllowRule, which is reached only under npmLite on Windows. That is an out-of-scope path, and the check was also placed before the port loop, so it could not have bounded the ports x excepts ACLs that loop appends anyway. Remove it; the budget remains on the shared peer/port path and at the end of translation. TestServerTimeoutsAreSet asserted that the timeout constants were non-zero, so removing an assignment from the server itself would not have failed it. Extract newServer and assert the server that is actually served. TestNegationOnlyPeerRendersAnchor built its own SetInfo list, used Ingress for both subtests, and asserted the match-set count was not one, which would also pass if the anchor were missing entirely. Use the real direction per subtest and assert exact counts for the anchor, the negation, and the total. Both tests now fail if their fix is reverted, which was verified by reverting each. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- npm/http/server/server.go | 25 +++++++---- npm/http/server/server_test.go | 27 ++++++++---- .../translation/translatePolicy.go | 7 +-- .../policies/policymanager_linux_test.go | 44 ++++++++++--------- 4 files changed, 60 insertions(+), 43 deletions(-) diff --git a/npm/http/server/server.go b/npm/http/server/server.go index f3d84332024..1766d19c6dd 100644 --- a/npm/http/server/server.go +++ b/npm/http/server/server.go @@ -48,6 +48,21 @@ type NPMRestServer struct { router *mux.Router } +// newServer builds the API server with the deadlines and bounds that keep a slow or unfinished +// request from holding resources indefinitely. It is a separate constructor so tests can assert +// the server that is actually served, rather than the constants it is built from. +func newServer(addr string, handler http.Handler) *http.Server { + return &http.Server{ + Handler: handler, + Addr: addr, + ReadHeaderTimeout: readHeaderTimeout, + ReadTimeout: readTimeout, + WriteTimeout: writeTimeout, + IdleTimeout: idleTimeout, + MaxHeaderBytes: maxHeaderBytes, + } +} + func NPMRestServerListenAndServe(config npmconfig.Config, npmEncoder json.Marshaler) { rs := NPMRestServer{} @@ -79,15 +94,7 @@ func NPMRestServerListenAndServe(config npmconfig.Config, npmEncoder json.Marsha rs.listeningAddress = fmt.Sprintf("%s:%d", config.ListeningAddress, config.ListeningPort) } - srv := &http.Server{ - Handler: rs.router, - Addr: rs.listeningAddress, - ReadHeaderTimeout: readHeaderTimeout, - ReadTimeout: readTimeout, - WriteTimeout: writeTimeout, - IdleTimeout: idleTimeout, - MaxHeaderBytes: maxHeaderBytes, - } + srv := newServer(rs.listeningAddress, rs.router) listener, err := net.Listen("tcp", rs.listeningAddress) if err != nil { diff --git a/npm/http/server/server_test.go b/npm/http/server/server_test.go index 5f1f578383b..2558ac3fc39 100644 --- a/npm/http/server/server_test.go +++ b/npm/http/server/server_test.go @@ -11,6 +11,7 @@ import ( "github.com/Azure/azure-container-networking/npm" "github.com/Azure/azure-container-networking/npm/http/api" "github.com/Azure/azure-container-networking/npm/pkg/controlplane/controllers/common" + "github.com/gorilla/mux" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -113,15 +114,25 @@ func TestNPMCacheHandlerLimitsConcurrency(t *testing.T) { require.Equal(t, http.StatusOK, rr.Code, "the handler must recover once slots free up") } -// TestServerTimeoutsAreSet is a guard on the deadlines and header bound. The API listens on -// the host network of a privileged process, so a client that never finishes a request must -// not be able to hold it, and the response buffered for it, open indefinitely. +// TestServerTimeoutsAreSet guards the deadlines and bounds on the server that is actually +// constructed. The API listens on the host network of a privileged process, so a client that +// never finishes a request must not be able to hold it, and the response buffered for it, open +// indefinitely. Asserting the constructed server rather than the constants means removing an +// assignment in newServer fails this test. func TestServerTimeoutsAreSet(t *testing.T) { - require.NotZero(t, readHeaderTimeout) - require.NotZero(t, readTimeout) - require.NotZero(t, writeTimeout) - require.NotZero(t, idleTimeout) - require.NotZero(t, maxHeaderBytes) + srv := newServer("127.0.0.1:0", mux.NewRouter()) + + require.Equal(t, readHeaderTimeout, srv.ReadHeaderTimeout) + require.Equal(t, readTimeout, srv.ReadTimeout) + require.Equal(t, writeTimeout, srv.WriteTimeout) + require.Equal(t, idleTimeout, srv.IdleTimeout) + require.Equal(t, maxHeaderBytes, srv.MaxHeaderBytes) + + require.NotZero(t, srv.ReadHeaderTimeout) + require.NotZero(t, srv.ReadTimeout) + require.NotZero(t, srv.WriteTimeout) + require.NotZero(t, srv.IdleTimeout) + require.NotZero(t, srv.MaxHeaderBytes) require.NotZero(t, maxConcurrentConns) require.LessOrEqual(t, maxConcurrentCacheRequests, maxConcurrentConns, "cache encodings must be bounded at or below the connection ceiling") diff --git a/npm/pkg/controlplane/translation/translatePolicy.go b/npm/pkg/controlplane/translation/translatePolicy.go index ea929cd1ba5..75fab4482e1 100644 --- a/npm/pkg/controlplane/translation/translatePolicy.go +++ b/npm/pkg/controlplane/translation/translatePolicy.go @@ -455,15 +455,10 @@ func exceptDirectDropRules(npmNetPol *policies.NPMNetworkPolicy, direction polic func directPeerAndPortAllowRule(npmNetPol *policies.NPMNetworkPolicy, direction policies.Direction, ports []networkingv1.NetworkPolicyPort, cidr string, except []string, npmLiteToggle bool) error { // Match the ipset-based ipBlockRule path and reject non-IPv4 enclosing CIDRs (e.g. IPv6), // which HNS direct-IP ACLs cannot express. Failing closed avoids programming an ACL that - // silently ignores the peer. This path is Windows/NPM Lite only and is intentionally left - // on the unchanged IsIPV4 behavior. + // silently ignores the peer. if !util.IsIPV4(cidr) { return ErrUnsupportedIPAddress } - - if err := checkACLBudget(npmNetPol); err != nil { - return err - } if len(ports) == 0 { acl := policies.NewACLPolicy(policies.Allowed, direction) // bypasses ipset creation for /32 cidrs and directly creates an acl with the cidr diff --git a/npm/pkg/dataplane/policies/policymanager_linux_test.go b/npm/pkg/dataplane/policies/policymanager_linux_test.go index 6064d4b4239..06bd69ce90b 100644 --- a/npm/pkg/dataplane/policies/policymanager_linux_test.go +++ b/npm/pkg/dataplane/policies/policymanager_linux_test.go @@ -519,29 +519,30 @@ func TestUpdatingStaleChains(t *testing.T) { } // TestNegationOnlyPeerRendersAnchor asserts how a negation-only namespace peer reaches the -// kernel. A negated set match (`! --match-set`) is satisfied by every address absent from -// that set, including addresses that are not pods at all, so an ACL whose peer list is only -// negations matches non-pod traffic. The all-namespaces anchor is what confines the decision -// to the pod domain, and this test pins that it renders as a positive `--match-set` in the -// same rule as the negation, for both directions. +// kernel. A negated set match (`! --match-set`) is satisfied by every address absent from that +// set, including addresses that are not pods at all, so an ACL whose peer list is only negations +// matches non-pod traffic. The all-namespaces anchor is what confines the decision to the pod +// domain, and this test pins that it renders as a positive `--match-set` in the same rule as the +// negation, in both directions. func TestNegationOnlyPeerRendersAnchor(t *testing.T) { anchor := ipsets.NewIPSetMetadata(util.KubeAllNamespacesFlag, ipsets.KeyLabelOfNamespace) excluded := ipsets.NewIPSetMetadata("blocked", ipsets.KeyLabelOfNamespace) tests := []struct { name string + direction Direction matchType MatchType - direction string + matchArg string }{ - {"ingress", SrcMatch, "src"}, - {"egress", DstMatch, "dst"}, + {"ingress", Ingress, SrcMatch, "src"}, + {"egress", Egress, DstMatch, "dst"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { acl := &ACLPolicy{ Target: Allowed, - Direction: Ingress, + Direction: tt.direction, } peers := []SetInfo{ NewSetInfo(util.KubeAllNamespacesFlag, ipsets.KeyLabelOfNamespace, true, tt.matchType), @@ -555,18 +556,21 @@ func TestNegationOnlyPeerRendersAnchor(t *testing.T) { specs := strings.Join(iptablesRuleSpecs(acl), " ") - // The anchor must be a positive match, so only pod addresses can satisfy it. - require.Contains(t, specs, - strings.Join([]string{util.IptablesMatchSetFlag, anchor.GetHashedName(), tt.direction}, " "), - "the all-namespaces anchor must render as a positive match-set") + // The anchor must render as a positive match, so only pod addresses satisfy it. + // Asserted by exact count: a bare NotEqual would also pass if it were absent. + positive := strings.Join([]string{util.IptablesMatchSetFlag, anchor.GetHashedName(), tt.matchArg}, " ") + require.Equal(t, 1, strings.Count(specs, positive), + "the all-namespaces anchor must render exactly once as a positive match-set") + // The exclusion must remain negated. - require.Contains(t, specs, - strings.Join([]string{util.IptablesNotFlag, util.IptablesMatchSetFlag, excluded.GetHashedName(), tt.direction}, " "), - "the excluded namespace label must render as a negated match-set") - // The negation must not be the only match in the rule, which is the shape - // that admits non-pod addresses. - require.NotEqual(t, 1, strings.Count(specs, util.IptablesMatchSetFlag), - "a negation-only rule must never be emitted for a namespace peer") + negated := strings.Join([]string{util.IptablesNotFlag, util.IptablesMatchSetFlag, excluded.GetHashedName(), tt.matchArg}, " ") + require.Equal(t, 1, strings.Count(specs, negated), + "the excluded namespace label must render exactly once as a negated match-set") + + // Exactly two match-sets: the anchor and the exclusion. A negation-only rule, which + // is the shape that admits non-pod addresses, would have only one. + require.Equal(t, 2, strings.Count(specs, util.IptablesMatchSetFlag), + "a namespace peer must never render as a lone negated match") }) } } From 9a43e2060d7b0ace49dcfa3e3a3528168096b539 Mon Sep 17 00:00:00 2001 From: Isaiah Raya Date: Tue, 8 Sep 2026 19:05:07 +0000 Subject: [PATCH 11/23] fix: [NPM] check the rule budget per port and record translation errors once Address review feedback on the translation and HTTP server changes. The ACL budget was checked once on entry to peerAndPortRule, before its port loop. A single peer emits one ACL per port, so one peer listing many ports could materialize every ACL and only be caught by the check at the end of translation. The budget is now checked before each port as well, so translation stops at the limit instead of after building the full product. flattenNameSpaceSelector and checkACLBudget each logged an error and also returned one, and their callers already log and record the returned error at the workqueue boundary. Both now return the error wrapped with the selector or policy context and leave recording to the caller, which is the single place that reports a translation failure. The sentinel errors are unchanged, so errors.Is comparisons still hold. The blank import of net/http/pprof is dropped; the package is already imported by name for the route handlers, which runs the same init. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- npm/http/server/server.go | 1 - .../controlplane/translation/parseSelector.go | 10 +++++----- .../translation/parseSelector_test.go | 19 +++++++++++++++++++ .../translation/translatePolicy.go | 15 +++++++++++---- 4 files changed, 35 insertions(+), 10 deletions(-) diff --git a/npm/http/server/server.go b/npm/http/server/server.go index 1766d19c6dd..34280af0659 100644 --- a/npm/http/server/server.go +++ b/npm/http/server/server.go @@ -6,7 +6,6 @@ import ( "net" "net/http" "net/http/pprof" - _ "net/http/pprof" "time" "github.com/Azure/azure-container-networking/log" diff --git a/npm/pkg/controlplane/translation/parseSelector.go b/npm/pkg/controlplane/translation/parseSelector.go index 6a3d305e76d..be1bbb1082e 100644 --- a/npm/pkg/controlplane/translation/parseSelector.go +++ b/npm/pkg/controlplane/translation/parseSelector.go @@ -4,7 +4,6 @@ import ( "fmt" "regexp" - "github.com/Azure/azure-container-networking/log" "github.com/Azure/azure-container-networking/npm/pkg/dataplane/ipsets" "github.com/Azure/azure-container-networking/npm/util" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -158,8 +157,9 @@ func flattenNameSpaceSelector(nsSelector *metav1.LabelSelector) ([]metav1.LabelS default: // Fail closed: an unknown operator must not silently drop the requirement // and widen the selector. Kubernetes only admits In/NotIn/Exists/DoesNotExist. - log.Errorf("unsupported operator [%s] for selector [%v] requirement", req.Operator, *nsSelector) - return nil, ErrUnsupportedMatchExpressionOperator + // The error carries the selector context and is recorded once by the caller. + return nil, fmt.Errorf("operator %q on key %q in selector %v: %w", + req.Operator, req.Key, *nsSelector, ErrUnsupportedMatchExpressionOperator) } } @@ -183,8 +183,8 @@ func flattenNameSpaceSelector(nsSelector *metav1.LabelSelector) ([]metav1.LabelS combinations := 1 for _, req := range multiValueMatchExprs { if len(req.Values) > maxFlattenedNSSelectors/combinations { - log.Errorf("namespaceSelector [%v] expands past the %d selector limit", *nsSelector, maxFlattenedNSSelectors) - return nil, ErrTooManyFlattenedSelectors + return nil, fmt.Errorf("selector %v expands past the %d selector limit: %w", + *nsSelector, maxFlattenedNSSelectors, ErrTooManyFlattenedSelectors) } combinations *= len(req.Values) } diff --git a/npm/pkg/controlplane/translation/parseSelector_test.go b/npm/pkg/controlplane/translation/parseSelector_test.go index c1933209bbd..d9f9dc9cc58 100644 --- a/npm/pkg/controlplane/translation/parseSelector_test.go +++ b/npm/pkg/controlplane/translation/parseSelector_test.go @@ -5,6 +5,7 @@ import ( "reflect" "testing" + "github.com/Azure/azure-container-networking/npm/pkg/dataplane/policies" "github.com/stretchr/testify/require" networkingv1 "k8s.io/api/networking/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -918,3 +919,21 @@ func TestTranslatePolicyOrdinaryPolicyWithinACLBudget(t *testing.T) { require.NotNil(t, npmNetPol) require.Less(t, len(npmNetPol.ACLs), maxACLsPerPolicy) } + +// TestPeerAndPortRuleBudgetStopsWithinPortLoop covers a single peer listing more ports than +// the budget allows. One peer emits one ACL per port, so a budget checked only on entry to +// peerAndPortRule would let that peer materialize every ACL before anything noticed. +func TestPeerAndPortRuleBudgetStopsWithinPortLoop(t *testing.T) { + portCount := maxACLsPerPolicy * 2 + ports := make([]networkingv1.NetworkPolicyPort, 0, portCount) + for i := 0; i < portCount; i++ { + p := intstr.FromInt(1 + i) + ports = append(ports, networkingv1.NetworkPolicyPort{Port: &p}) + } + + npmNetPol := policies.NewNPMNetworkPolicy("wide-ports", "default") + err := peerAndPortRule(npmNetPol, policies.Ingress, ports, []policies.SetInfo{}, false) + require.ErrorIs(t, err, ErrTooManyACLs) + require.LessOrEqual(t, len(npmNetPol.ACLs), maxACLsPerPolicy+1, + "the port loop must stop once the budget is spent instead of emitting an ACL for every port") +} diff --git a/npm/pkg/controlplane/translation/translatePolicy.go b/npm/pkg/controlplane/translation/translatePolicy.go index 75fab4482e1..75d28588880 100644 --- a/npm/pkg/controlplane/translation/translatePolicy.go +++ b/npm/pkg/controlplane/translation/translatePolicy.go @@ -405,6 +405,12 @@ func peerAndPortRule(npmNetPol *policies.NPMNetworkPolicy, direction policies.Di } for i := range ports { + // Re-checked per port, not only on entry: this peer emits one ACL per port, so a + // check that ran once could not stop a single peer from expanding past the limit. + if err := checkACLBudget(npmNetPol); err != nil { + return err + } + portKind, err := portType(ports[i]) if err != nil { return err @@ -839,12 +845,13 @@ func TranslatePolicy(npObj *networkingv1.NetworkPolicy, npmLiteToggle bool) (*po const maxACLsPerPolicy = 2000 // checkACLBudget reports whether the policy has grown past what NPM is willing to translate. -// It is checked before each peer is expanded, so translation stops early rather than after -// materializing the full product. +// It is checked before each peer is expanded and before each of that peer's ports, so +// translation stops early rather than after materializing the full product. func checkACLBudget(npmNetPol *policies.NPMNetworkPolicy) error { if len(npmNetPol.ACLs) > maxACLsPerPolicy { - klog.Errorf("network policy %s expands past the %d ACL limit", npmNetPol.PolicyKey, maxACLsPerPolicy) - return ErrTooManyACLs + // The error carries the policy context and is recorded once by the caller. + return fmt.Errorf("network policy %s expands past the %d rule limit: %w", + npmNetPol.PolicyKey, maxACLsPerPolicy, ErrTooManyACLs) } return nil } From 7352af95fb7a04e1b334f8b6e64568e4a6456deb Mon Sep 17 00:00:00 2001 From: Isaiah Raya Date: Tue, 8 Sep 2026 19:15:12 +0000 Subject: [PATCH 12/23] chore: [NPM] address lint findings in the changed files Findings reported by golangci-lint on the lines this branch adds, run the way CI runs it (--new-from-rev against master). noctx: the HTTP server listener is created through net.ListenConfig, and the server tests build their requests with httptest.NewRequestWithContext. goconst: repeated literals in the added tests are named constants. The label keys, the team value and the direction names move into the existing const block in translatePolicy_test.go, the added namespace literals in parseSelector_test.go use the existing defaultNS, and the CIDRs shared between the IsIPV4 and NormalizeCIDR cases get their own constants. No behavior change; npm builds, vets and tests as before. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- npm/http/server/server.go | 4 +- npm/http/server/server_test.go | 6 +- .../translation/parseSelector_test.go | 24 +++---- .../translation/translatePolicy_test.go | 70 +++++++++++-------- npm/util/util_test.go | 23 +++--- 5 files changed, 72 insertions(+), 55 deletions(-) diff --git a/npm/http/server/server.go b/npm/http/server/server.go index 34280af0659..fef7bec5ac7 100644 --- a/npm/http/server/server.go +++ b/npm/http/server/server.go @@ -1,6 +1,7 @@ package server import ( + "context" "encoding/json" "fmt" "net" @@ -95,7 +96,8 @@ func NPMRestServerListenAndServe(config npmconfig.Config, npmEncoder json.Marsha srv := newServer(rs.listeningAddress, rs.router) - listener, err := net.Listen("tcp", rs.listeningAddress) + var lc net.ListenConfig + listener, err := lc.Listen(context.Background(), "tcp", rs.listeningAddress) if err != nil { klog.Errorf("Failed to start NPM HTTP Server with error: %+v", err) return diff --git a/npm/http/server/server_test.go b/npm/http/server/server_test.go index 2558ac3fc39..e5aab199269 100644 --- a/npm/http/server/server_test.go +++ b/npm/http/server/server_test.go @@ -90,7 +90,7 @@ func TestNPMCacheHandlerLimitsConcurrency(t *testing.T) { wg.Add(1) go func() { defer wg.Done() - req := httptest.NewRequest(http.MethodGet, api.NPMMgrPath, http.NoBody) + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, api.NPMMgrPath, http.NoBody) handler.ServeHTTP(httptest.NewRecorder(), req) }() } @@ -101,7 +101,7 @@ func TestNPMCacheHandlerLimitsConcurrency(t *testing.T) { // With every slot busy, a further request must be shed instead of queueing another // full copy of the cache. rr := httptest.NewRecorder() - handler.ServeHTTP(rr, httptest.NewRequest(http.MethodGet, api.NPMMgrPath, http.NoBody)) + handler.ServeHTTP(rr, httptest.NewRequestWithContext(t.Context(), http.MethodGet, api.NPMMgrPath, http.NoBody)) require.Equal(t, http.StatusServiceUnavailable, rr.Code, "a request beyond the in-flight limit must be shed") @@ -110,7 +110,7 @@ func TestNPMCacheHandlerLimitsConcurrency(t *testing.T) { // Once the in-flight requests drain, the handler must serve again. rr = httptest.NewRecorder() - handler.ServeHTTP(rr, httptest.NewRequest(http.MethodGet, api.NPMMgrPath, http.NoBody)) + handler.ServeHTTP(rr, httptest.NewRequestWithContext(t.Context(), http.MethodGet, api.NPMMgrPath, http.NoBody)) require.Equal(t, http.StatusOK, rr.Code, "the handler must recover once slots free up") } diff --git a/npm/pkg/controlplane/translation/parseSelector_test.go b/npm/pkg/controlplane/translation/parseSelector_test.go index d9f9dc9cc58..355ed598040 100644 --- a/npm/pkg/controlplane/translation/parseSelector_test.go +++ b/npm/pkg/controlplane/translation/parseSelector_test.go @@ -611,7 +611,7 @@ func TestFlattenNameSpaceSelectorMultiValueNotIn(t *testing.T) { selector := &metav1.LabelSelector{ MatchExpressions: []metav1.LabelSelectorRequirement{ { - Key: "tenant", + Key: tenantLabelKey, Operator: metav1.LabelSelectorOpNotIn, Values: []string{"x", "y"}, }, @@ -625,12 +625,12 @@ func TestFlattenNameSpaceSelectorMultiValueNotIn(t *testing.T) { { MatchExpressions: []metav1.LabelSelectorRequirement{ { - Key: "tenant", + Key: tenantLabelKey, Operator: metav1.LabelSelectorOpNotIn, Values: []string{"x"}, }, { - Key: "tenant", + Key: tenantLabelKey, Operator: metav1.LabelSelectorOpNotIn, Values: []string{"y"}, }, @@ -648,7 +648,7 @@ func TestFlattenNameSpaceSelectorMixedInAndNotIn(t *testing.T) { selector := &metav1.LabelSelector{ MatchExpressions: []metav1.LabelSelectorRequirement{ { - Key: "tenant", + Key: tenantLabelKey, Operator: metav1.LabelSelectorOpNotIn, Values: []string{"x", "y"}, }, @@ -672,7 +672,7 @@ func TestFlattenNameSpaceSelectorMixedInAndNotIn(t *testing.T) { require.Len(t, req.Values, 1, "every requirement must be single-value after flatten") switch req.Operator { case metav1.LabelSelectorOpNotIn: - require.Equal(t, "tenant", req.Key) + require.Equal(t, tenantLabelKey, req.Key) notInValues = append(notInValues, req.Values[0]) case metav1.LabelSelectorOpIn: require.Equal(t, "role", req.Key) @@ -695,7 +695,7 @@ func TestFlattenNameSpaceSelectorUnsupportedOperator(t *testing.T) { selector := &metav1.LabelSelector{ MatchExpressions: []metav1.LabelSelectorRequirement{ { - Key: "tenant", + Key: tenantLabelKey, Operator: metav1.LabelSelectorOperator("Frobnicate"), Values: []string{"x"}, }, @@ -714,7 +714,7 @@ func TestFlattenNameSpaceSelectorEmptyValues(t *testing.T) { selector := &metav1.LabelSelector{ MatchExpressions: []metav1.LabelSelectorRequirement{ { - Key: "tenant", + Key: tenantLabelKey, Operator: op, Values: []string{}, }, @@ -786,7 +786,7 @@ func TestTranslatePolicyExpansionLimit(t *testing.T) { } pol := &networkingv1.NetworkPolicy{ - ObjectMeta: metav1.ObjectMeta{Name: "expand", Namespace: "default"}, + ObjectMeta: metav1.ObjectMeta{Name: "expand", Namespace: defaultNS}, Spec: networkingv1.NetworkPolicySpec{ PodSelector: metav1.LabelSelector{}, PolicyTypes: []networkingv1.PolicyType{networkingv1.PolicyTypeIngress}, @@ -870,7 +870,7 @@ func TestTranslatePolicyACLBudget(t *testing.T) { } pol := &networkingv1.NetworkPolicy{ - ObjectMeta: metav1.ObjectMeta{Name: "expand", Namespace: "default"}, + ObjectMeta: metav1.ObjectMeta{Name: "expand", Namespace: defaultNS}, Spec: networkingv1.NetworkPolicySpec{ PodSelector: metav1.LabelSelector{}, PolicyTypes: []networkingv1.PolicyType{networkingv1.PolicyTypeIngress}, @@ -899,14 +899,14 @@ func TestTranslatePolicyOrdinaryPolicyWithinACLBudget(t *testing.T) { } pol := &networkingv1.NetworkPolicy{ - ObjectMeta: metav1.ObjectMeta{Name: "normal", Namespace: "default"}, + ObjectMeta: metav1.ObjectMeta{Name: "normal", Namespace: defaultNS}, Spec: networkingv1.NetworkPolicySpec{ PodSelector: metav1.LabelSelector{MatchLabels: map[string]string{"app": "web"}}, PolicyTypes: []networkingv1.PolicyType{networkingv1.PolicyTypeIngress}, Ingress: []networkingv1.NetworkPolicyIngressRule{{ Ports: ports, From: []networkingv1.NetworkPolicyPeer{ - {NamespaceSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"team": "blue"}}}, + {NamespaceSelector: &metav1.LabelSelector{MatchLabels: map[string]string{teamLabelKey: teamBlueValue}}}, {PodSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"role": "client"}}}, {IPBlock: &networkingv1.IPBlock{CIDR: "10.0.0.0/8"}}, }, @@ -931,7 +931,7 @@ func TestPeerAndPortRuleBudgetStopsWithinPortLoop(t *testing.T) { ports = append(ports, networkingv1.NetworkPolicyPort{Port: &p}) } - npmNetPol := policies.NewNPMNetworkPolicy("wide-ports", "default") + npmNetPol := policies.NewNPMNetworkPolicy("wide-ports", defaultNS) err := peerAndPortRule(npmNetPol, policies.Ingress, ports, []policies.SetInfo{}, false) require.ErrorIs(t, err, ErrTooManyACLs) require.LessOrEqual(t, len(npmNetPol.ACLs), maxACLsPerPolicy+1, diff --git a/npm/pkg/controlplane/translation/translatePolicy_test.go b/npm/pkg/controlplane/translation/translatePolicy_test.go index cdcb3fdffcf..2d79ce35bed 100644 --- a/npm/pkg/controlplane/translation/translatePolicy_test.go +++ b/npm/pkg/controlplane/translation/translatePolicy_test.go @@ -24,6 +24,14 @@ const ( appLabelKey string = "app" enclosingCIDR string = "10.244.1.0/24" exceptedHostBits string = "10.244.1.106/32" + + tenantLabelKey string = "tenant" + teamLabelKey string = "team" + blockedLabelKey string = "blocked" + teamBlueValue string = "blue" + lowerHalfNomatch string = "0.0.0.0/1 nomatch" + ingressName string = "ingress" + egressName string = "egress" ) var namedPortPolicyKey = fmt.Sprintf("%s/%s", defaultNS, namedPortStr) @@ -647,7 +655,7 @@ func TestIPBlockIPSet(t *testing.T) { CIDR: "0.0.0.0/0", Except: []string{"10.0.0.0/1"}, }, - translatedIPSet: ipsets.NewTranslatedIPSet("test:in-ns:default-0-0IN", ipsets.CIDRBlocks, []string{"0.0.0.0/1 nomatch", "128.0.0.0/1"}...), + translatedIPSet: ipsets.NewTranslatedIPSet("test:in-ns:default-0-0IN", ipsets.CIDRBlocks, []string{lowerHalfNomatch, "128.0.0.0/1"}...), skipWindows: true, }, { @@ -657,7 +665,7 @@ func TestIPBlockIPSet(t *testing.T) { CIDR: "0.0.0.0/0", Except: []string{"0.0.0.0/1"}, }, - translatedIPSet: ipsets.NewTranslatedIPSet("test:in-ns:default-0-0IN", ipsets.CIDRBlocks, []string{"0.0.0.0/1 nomatch", "128.0.0.0/1"}...), + translatedIPSet: ipsets.NewTranslatedIPSet("test:in-ns:default-0-0IN", ipsets.CIDRBlocks, []string{lowerHalfNomatch, "128.0.0.0/1"}...), skipWindows: true, }, { @@ -677,7 +685,7 @@ func TestIPBlockIPSet(t *testing.T) { CIDR: "0.0.0.0/0", Except: []string{"0.0.0.0/1", "128.0.0.0/1"}, }, - translatedIPSet: ipsets.NewTranslatedIPSet("test:in-ns:default-0-0IN", ipsets.CIDRBlocks, []string{"0.0.0.0/1 nomatch", "128.0.0.0/1 nomatch"}...), + translatedIPSet: ipsets.NewTranslatedIPSet("test:in-ns:default-0-0IN", ipsets.CIDRBlocks, []string{lowerHalfNomatch, "128.0.0.0/1 nomatch"}...), skipWindows: true, }, { @@ -687,7 +695,7 @@ func TestIPBlockIPSet(t *testing.T) { CIDR: "0.0.0.0/0", Except: []string{"0.0.0.0/1", "128.0.0.0/1", "128.0.0.0/1"}, }, - translatedIPSet: ipsets.NewTranslatedIPSet("test:in-ns:default-0-0IN", ipsets.CIDRBlocks, []string{"0.0.0.0/1 nomatch", "128.0.0.0/1 nomatch"}...), + translatedIPSet: ipsets.NewTranslatedIPSet("test:in-ns:default-0-0IN", ipsets.CIDRBlocks, []string{lowerHalfNomatch, "128.0.0.0/1 nomatch"}...), skipWindows: true, }, } @@ -1283,7 +1291,7 @@ func TestNameSpaceSelectorMultiValueNotIn(t *testing.T) { selector := &metav1.LabelSelector{ MatchExpressions: []metav1.LabelSelectorRequirement{ { - Key: "tenant", + Key: tenantLabelKey, Operator: metav1.LabelSelectorOpNotIn, Values: []string{"x", "y"}, }, @@ -1313,9 +1321,9 @@ func TestNameSpaceSelectorMultiValueNotIn(t *testing.T) { func TestNameSpaceSelectorMatchLabelsAndMultiValueNotIn(t *testing.T) { matchType := policies.SrcMatch selector := &metav1.LabelSelector{ - MatchLabels: map[string]string{"team": "blue"}, + MatchLabels: map[string]string{teamLabelKey: teamBlueValue}, MatchExpressions: []metav1.LabelSelectorRequirement{ - {Key: "tenant", Operator: metav1.LabelSelectorOpNotIn, Values: []string{"x", "y"}}, + {Key: tenantLabelKey, Operator: metav1.LabelSelectorOpNotIn, Values: []string{"x", "y"}}, }, } @@ -1351,7 +1359,7 @@ func TestNameSpaceSelectorNegationOnlyIsScopedToNamespaces(t *testing.T) { name: "single-value NotIn", selector: &metav1.LabelSelector{ MatchExpressions: []metav1.LabelSelectorRequirement{ - {Key: "tenant", Operator: metav1.LabelSelectorOpNotIn, Values: []string{"x"}}, + {Key: tenantLabelKey, Operator: metav1.LabelSelectorOpNotIn, Values: []string{"x"}}, }, }, expected: []policies.SetInfo{ @@ -1363,26 +1371,26 @@ func TestNameSpaceSelectorNegationOnlyIsScopedToNamespaces(t *testing.T) { name: "DoesNotExist", selector: &metav1.LabelSelector{ MatchExpressions: []metav1.LabelSelectorRequirement{ - {Key: "tenant", Operator: metav1.LabelSelectorOpDoesNotExist}, + {Key: tenantLabelKey, Operator: metav1.LabelSelectorOpDoesNotExist}, }, }, expected: []policies.SetInfo{ policies.NewSetInfo(util.KubeAllNamespacesFlag, ipsets.KeyLabelOfNamespace, included, matchType), - policies.NewSetInfo("tenant", ipsets.KeyLabelOfNamespace, nonIncluded, matchType), + policies.NewSetInfo(tenantLabelKey, ipsets.KeyLabelOfNamespace, nonIncluded, matchType), }, }, { name: "NotIn and DoesNotExist together", selector: &metav1.LabelSelector{ MatchExpressions: []metav1.LabelSelectorRequirement{ - {Key: "tenant", Operator: metav1.LabelSelectorOpNotIn, Values: []string{"x"}}, - {Key: "team", Operator: metav1.LabelSelectorOpDoesNotExist}, + {Key: tenantLabelKey, Operator: metav1.LabelSelectorOpNotIn, Values: []string{"x"}}, + {Key: teamLabelKey, Operator: metav1.LabelSelectorOpDoesNotExist}, }, }, expected: []policies.SetInfo{ policies.NewSetInfo(util.KubeAllNamespacesFlag, ipsets.KeyLabelOfNamespace, included, matchType), policies.NewSetInfo("tenant:x", ipsets.KeyValueLabelOfNamespace, nonIncluded, matchType), - policies.NewSetInfo("team", ipsets.KeyLabelOfNamespace, nonIncluded, matchType), + policies.NewSetInfo(teamLabelKey, ipsets.KeyLabelOfNamespace, nonIncluded, matchType), }, }, } @@ -1410,7 +1418,7 @@ func TestNameSpaceSelectorWithPositiveMatchIsUnchanged(t *testing.T) { }{ { name: "matchLabels only", - selector: &metav1.LabelSelector{MatchLabels: map[string]string{"team": "blue"}}, + selector: &metav1.LabelSelector{MatchLabels: map[string]string{teamLabelKey: teamBlueValue}}, expected: []policies.SetInfo{ policies.NewSetInfo("team:blue", ipsets.KeyValueLabelOfNamespace, included, matchType), }, @@ -1418,9 +1426,9 @@ func TestNameSpaceSelectorWithPositiveMatchIsUnchanged(t *testing.T) { { name: "matchLabels with a negative expression", selector: &metav1.LabelSelector{ - MatchLabels: map[string]string{"team": "blue"}, + MatchLabels: map[string]string{teamLabelKey: teamBlueValue}, MatchExpressions: []metav1.LabelSelectorRequirement{ - {Key: "tenant", Operator: metav1.LabelSelectorOpNotIn, Values: []string{"x"}}, + {Key: tenantLabelKey, Operator: metav1.LabelSelectorOpNotIn, Values: []string{"x"}}, }, }, expected: []policies.SetInfo{ @@ -1432,12 +1440,12 @@ func TestNameSpaceSelectorWithPositiveMatchIsUnchanged(t *testing.T) { name: "Exists with a negative expression", selector: &metav1.LabelSelector{ MatchExpressions: []metav1.LabelSelectorRequirement{ - {Key: "team", Operator: metav1.LabelSelectorOpExists}, - {Key: "tenant", Operator: metav1.LabelSelectorOpNotIn, Values: []string{"x"}}, + {Key: teamLabelKey, Operator: metav1.LabelSelectorOpExists}, + {Key: tenantLabelKey, Operator: metav1.LabelSelectorOpNotIn, Values: []string{"x"}}, }, }, expected: []policies.SetInfo{ - policies.NewSetInfo("team", ipsets.KeyLabelOfNamespace, included, matchType), + policies.NewSetInfo(teamLabelKey, ipsets.KeyLabelOfNamespace, included, matchType), policies.NewSetInfo("tenant:x", ipsets.KeyValueLabelOfNamespace, nonIncluded, matchType), }, }, @@ -1474,13 +1482,13 @@ func TestTranslatePolicyNegationOnlyNamespaceSelector(t *testing.T) { peerList func(*policies.ACLPolicy) []policies.SetInfo }{ { - name: "egress", + name: egressName, direction: networkingv1.PolicyTypeEgress, matchType: policies.DstMatch, peerList: func(acl *policies.ACLPolicy) []policies.SetInfo { return acl.DstList }, }, { - name: "ingress", + name: ingressName, direction: networkingv1.PolicyTypeIngress, matchType: policies.SrcMatch, peerList: func(acl *policies.ACLPolicy) []policies.SetInfo { return acl.SrcList }, @@ -1491,7 +1499,7 @@ func TestTranslatePolicyNegationOnlyNamespaceSelector(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - pol := nsNotInPolicy("victim", "default", "tenant", tt.direction, nil, "x") + pol := nsNotInPolicy("victim", "default", tenantLabelKey, tt.direction, nil, "x") npmNetPol, err := TranslatePolicy(pol, false) require.NoError(t, err) @@ -1568,12 +1576,12 @@ func TestTranslatePolicyMultiValueNotInConjunction(t *testing.T) { peerList func(*policies.ACLPolicy) []policies.SetInfo }{ { - name: "ingress", + name: ingressName, direction: networkingv1.PolicyTypeIngress, peerList: func(acl *policies.ACLPolicy) []policies.SetInfo { return acl.SrcList }, }, { - name: "egress", + name: egressName, direction: networkingv1.PolicyTypeEgress, peerList: func(acl *policies.ACLPolicy) []policies.SetInfo { return acl.DstList }, }, @@ -1589,7 +1597,7 @@ func TestTranslatePolicyMultiValueNotInConjunction(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - pol := nsNotInPolicy("victim", "default", "tenant", tt.direction, tt.ports, "attacker", "quarantine") + pol := nsNotInPolicy("victim", "default", tenantLabelKey, tt.direction, tt.ports, "attacker", "quarantine") npmNetPol, err := TranslatePolicy(pol, false) require.NoError(t, err) @@ -3981,19 +3989,19 @@ func TestTranslatePolicyNegationOnlyOperators(t *testing.T) { }{ { name: "DoesNotExist", - req: metav1.LabelSelectorRequirement{Key: "blocked", Operator: metav1.LabelSelectorOpDoesNotExist}, - excluded: "blocked", + req: metav1.LabelSelectorRequirement{Key: blockedLabelKey, Operator: metav1.LabelSelectorOpDoesNotExist}, + excluded: blockedLabelKey, setType: ipsets.KeyLabelOfNamespace, }, { name: "single-value NotIn", - req: metav1.LabelSelectorRequirement{Key: "blocked", Operator: metav1.LabelSelectorOpNotIn, Values: []string{"yes"}}, + req: metav1.LabelSelectorRequirement{Key: blockedLabelKey, Operator: metav1.LabelSelectorOpNotIn, Values: []string{"yes"}}, excluded: "blocked:yes", setType: ipsets.KeyValueLabelOfNamespace, }, { name: "multi-value NotIn", - req: metav1.LabelSelectorRequirement{Key: "blocked", Operator: metav1.LabelSelectorOpNotIn, Values: []string{"yes", "maybe"}}, + req: metav1.LabelSelectorRequirement{Key: blockedLabelKey, Operator: metav1.LabelSelectorOpNotIn, Values: []string{"yes", "maybe"}}, excluded: "blocked:yes", setType: ipsets.KeyValueLabelOfNamespace, }, @@ -4005,8 +4013,8 @@ func TestTranslatePolicyNegationOnlyOperators(t *testing.T) { matchType policies.MatchType peerList func(*policies.ACLPolicy) []policies.SetInfo }{ - {"ingress", networkingv1.PolicyTypeIngress, policies.SrcMatch, func(a *policies.ACLPolicy) []policies.SetInfo { return a.SrcList }}, - {"egress", networkingv1.PolicyTypeEgress, policies.DstMatch, func(a *policies.ACLPolicy) []policies.SetInfo { return a.DstList }}, + {ingressName, networkingv1.PolicyTypeIngress, policies.SrcMatch, func(a *policies.ACLPolicy) []policies.SetInfo { return a.SrcList }}, + {egressName, networkingv1.PolicyTypeEgress, policies.DstMatch, func(a *policies.ACLPolicy) []policies.SetInfo { return a.DstList }}, } for _, op := range operators { diff --git a/npm/util/util_test.go b/npm/util/util_test.go index 0878b344d7b..f16035b27be 100644 --- a/npm/util/util_test.go +++ b/npm/util/util_test.go @@ -515,6 +515,13 @@ func TestHashedNameGoldenVectors(t *testing.T) { } } +// Test CIDRs shared by the IsIPV4 and NormalizeCIDR cases below. +const ( + allIPv4CIDR = "0.0.0.0/0" + singleHostCIDR = "10.0.0.1/32" + canonicalNet24 = "10.1.2.0/24" +) + // TestIsIPV4 pins the existing behavior of the shared classifier. It is deliberately left // unchanged by these fixes because the Windows and NPM Lite paths also consume it, and those // are out of scope. Note it refuses a /0 block that is not spelled "0.0.0.0" even though such a @@ -524,9 +531,9 @@ func TestIsIPV4(t *testing.T) { "10.0.0.1", "0.0.0.0", "10.0.0.0/24", - "0.0.0.0/0", + allIPv4CIDR, "10.1.2.3/24", - "10.0.0.1/32", + singleHostCIDR, } for _, ip := range valid { require.True(t, IsIPV4(ip), "IsIPV4(%q) must be true", ip) @@ -555,14 +562,14 @@ func TestIsIPV4(t *testing.T) { // against a well-known block and hand the canonical form to the kernel. func TestNormalizeCIDR(t *testing.T) { canonical := map[string]string{ - "0.0.0.0/0": "0.0.0.0/0", - "10.0.0.0/0": "0.0.0.0/0", - "255.255.255.255/0": "0.0.0.0/0", + allIPv4CIDR: allIPv4CIDR, + "10.0.0.0/0": allIPv4CIDR, + "255.255.255.255/0": allIPv4CIDR, "10.0.0.0/1": "0.0.0.0/1", "200.0.0.0/1": "128.0.0.0/1", - "10.1.2.3/24": "10.1.2.0/24", - "10.1.2.0/24": "10.1.2.0/24", - "10.0.0.1/32": "10.0.0.1/32", + "10.1.2.3/24": canonicalNet24, + canonicalNet24: canonicalNet24, + singleHostCIDR: singleHostCIDR, } for in, want := range canonical { got, ok := NormalizeCIDR(in) From 0566315a473186dad44a14d13c19893842b73f5a Mon Sep 17 00:00:00 2001 From: Isaiah Raya Date: Tue, 8 Sep 2026 20:00:48 +0000 Subject: [PATCH 13/23] fix: [NPM] make the rule ceiling exact and refuse an except NPM cannot program Address the second round of review feedback. The rule budget compared with > , so a policy could hold one ACL more than the stated ceiling before the next check refused it. It now compares with >= and is checked before an ACL is added, so a policy never materializes more than the ceiling. An ipBlock except that is not an IPv4 CIDR was canonicalized on a best-effort basis and otherwise carried into the set unchanged, which either loses the exclusion and widens the allow to the enclosing CIDR, or fails when the set is restored. It now fails the translation with the same error the CIDR itself uses. The Windows check that refuses any except moves above the canonicalization so that path returns exactly the error it returned before. The HTTP server no longer reports a graceful close as a failure, and the empty-values error names the In and NotIn operators it applies to. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- npm/http/server/server.go | 6 ++- .../translation/parseSelector_test.go | 2 +- .../translation/translatePolicy.go | 39 +++++++++++-------- .../translation/translatePolicy_test.go | 19 +++++++++ 4 files changed, 48 insertions(+), 18 deletions(-) diff --git a/npm/http/server/server.go b/npm/http/server/server.go index fef7bec5ac7..43565807843 100644 --- a/npm/http/server/server.go +++ b/npm/http/server/server.go @@ -3,6 +3,7 @@ package server import ( "context" "encoding/json" + "errors" "fmt" "net" "net/http" @@ -104,7 +105,10 @@ func NPMRestServerListenAndServe(config npmconfig.Config, npmEncoder json.Marsha } klog.Infof("Starting NPM HTTP API on %s... ", rs.listeningAddress) - klog.Errorf("Failed to start NPM HTTP Server with error: %+v", srv.Serve(netutil.LimitListener(listener, maxConcurrentConns))) + // A graceful close is not a failure, so it must not be reported as one. + if err := srv.Serve(netutil.LimitListener(listener, maxConcurrentConns)); err != nil && !errors.Is(err, http.ErrServerClosed) { + klog.Errorf("NPM HTTP Server stopped with error: %+v", err) + } } func (n *NPMRestServer) npmCacheHandler(npmCacheEncoder json.Marshaler) http.Handler { diff --git a/npm/pkg/controlplane/translation/parseSelector_test.go b/npm/pkg/controlplane/translation/parseSelector_test.go index 355ed598040..51c69986838 100644 --- a/npm/pkg/controlplane/translation/parseSelector_test.go +++ b/npm/pkg/controlplane/translation/parseSelector_test.go @@ -934,6 +934,6 @@ func TestPeerAndPortRuleBudgetStopsWithinPortLoop(t *testing.T) { npmNetPol := policies.NewNPMNetworkPolicy("wide-ports", defaultNS) err := peerAndPortRule(npmNetPol, policies.Ingress, ports, []policies.SetInfo{}, false) require.ErrorIs(t, err, ErrTooManyACLs) - require.LessOrEqual(t, len(npmNetPol.ACLs), maxACLsPerPolicy+1, + require.LessOrEqual(t, len(npmNetPol.ACLs), maxACLsPerPolicy, "the port loop must stop once the budget is spent instead of emitting an ACL for every port") } diff --git a/npm/pkg/controlplane/translation/translatePolicy.go b/npm/pkg/controlplane/translation/translatePolicy.go index 75d28588880..162b44b13ff 100644 --- a/npm/pkg/controlplane/translation/translatePolicy.go +++ b/npm/pkg/controlplane/translation/translatePolicy.go @@ -38,7 +38,7 @@ var ( // ErrEmptyMatchExpressionValues is returned when an In or NotIn matchExpression carries no values. // Kubernetes rejects such requirements; NPM fails closed rather than dropping the requirement, // which could otherwise widen a selector (e.g. a dropped NotIn) or yield no rules at all. - ErrEmptyMatchExpressionValues = errors.New("in and notIn matchExpression requirements must have at least one value") + ErrEmptyMatchExpressionValues = errors.New("matchExpression with operator In or NotIn must have at least one value") // ErrUnsupportedMatchExpressionOperator is returned when a matchExpression uses an operator that is // none of In, NotIn, Exists or DoesNotExist. NPM fails closed rather than dropping the requirement, // which could otherwise silently widen the selector. @@ -187,23 +187,24 @@ func deDuplicateExcept(exceptInIPBlock []string) []string { // canonicalizeExcepts returns the except CIDRs in canonical form, with duplicates removed. // Canonicalizing first means two spellings of the same block (e.g. "10.1.2.0/24" and // "10.1.2.3/24") collapse to one entry, and that an except can be compared against the -// all-addresses split entries below. This is used only on the ipset path. -func canonicalizeExcepts(exceptInIPBlock []string) []string { +// all-addresses split entries below. An except that is not an IPv4 CIDR cannot be programmed, +// so it fails the translation rather than being carried into the set: dropping the exclusion +// would widen the allow, and keeping it would take the whole set down at restore time. This is +// used only on the ipset path. +func canonicalizeExcepts(exceptInIPBlock []string) ([]string, error) { canonicalExcepts := []string{} exceptsSet := make(map[string]struct{}) for _, except := range exceptInIPBlock { canonical, ok := util.NormalizeCIDR(except) if !ok { - // Leave a non-IPv4 except untouched; callers validate it separately and - // fail closed rather than silently dropping the exclusion. - canonical = except + return nil, fmt.Errorf("except %q: %w", except, ErrUnsupportedIPAddress) } if _, exist := exceptsSet[canonical]; !exist { canonicalExcepts = append(canonicalExcepts, canonical) exceptsSet[canonical] = struct{}{} } } - return canonicalExcepts + return canonicalExcepts, nil } // ipBlockIPSet return translatedIPSet based based on ipBlockRule. @@ -221,14 +222,19 @@ func ipBlockIPSet(policyName, ns string, direction policies.Direction, ipBlockSe return nil, ErrUnsupportedIPAddress } + // The Windows datapath refuses an except before any of it is canonicalized, exactly as + // it did before, so the validation below is reached on the Linux path only. + if util.IsWindowsDP() && len(ipBlockRule.Except) > 0 { + return nil, ErrUnsupportedExceptCIDR + } + // de-duplicated Except if there are redundance elements, in canonical form so they // compare correctly against the all-addresses split entries below. - deDupExcepts := canonicalizeExcepts(ipBlockRule.Except) - lenOfDeDupExcepts := len(deDupExcepts) - - if util.IsWindowsDP() && lenOfDeDupExcepts > 0 { - return nil, ErrUnsupportedExceptCIDR + deDupExcepts, err := canonicalizeExcepts(ipBlockRule.Except) + if err != nil { + return nil, err } + lenOfDeDupExcepts := len(deDupExcepts) var members []string indexOfMembers := 0 @@ -844,11 +850,12 @@ func TranslatePolicy(npObj *networkingv1.NetworkPolicy, npmLiteToggle bool) (*po // since a policy expanding this wide would already be unusable as iptables rules. const maxACLsPerPolicy = 2000 -// checkACLBudget reports whether the policy has grown past what NPM is willing to translate. -// It is checked before each peer is expanded and before each of that peer's ports, so -// translation stops early rather than after materializing the full product. +// checkACLBudget reports whether the policy has reached the ceiling. It is checked before a +// peer is expanded and before each of that peer's ports, so translation never materializes +// more than maxACLsPerPolicy ACLs, and once more at the end as a backstop for the paths that +// append without a check. func checkACLBudget(npmNetPol *policies.NPMNetworkPolicy) error { - if len(npmNetPol.ACLs) > maxACLsPerPolicy { + if len(npmNetPol.ACLs) >= maxACLsPerPolicy { // The error carries the policy context and is recorded once by the caller. return fmt.Errorf("network policy %s expands past the %d rule limit: %w", npmNetPol.PolicyKey, maxACLsPerPolicy, ErrTooManyACLs) diff --git a/npm/pkg/controlplane/translation/translatePolicy_test.go b/npm/pkg/controlplane/translation/translatePolicy_test.go index 2d79ce35bed..ac1e89d148c 100644 --- a/npm/pkg/controlplane/translation/translatePolicy_test.go +++ b/npm/pkg/controlplane/translation/translatePolicy_test.go @@ -817,6 +817,25 @@ func TestIPBlockRule(t *testing.T) { } } +// TestIPBlockRuleRejectsInvalidExcept covers an ipBlock whose except is not an IPv4 CIDR. Such +// an exclusion cannot be programmed, so the translation fails rather than carrying the entry +// into the set, which would either widen the allow to the enclosing CIDR or take the whole set +// down when it is restored. The Windows datapath refuses any except before this check, so the +// case is exercised on Linux only. +func TestIPBlockRuleRejectsInvalidExcept(t *testing.T) { + if util.IsWindowsDP() { + t.Skip("the Windows datapath refuses any except on this path") + } + + for _, except := range []string{"2001:db8::/32", "not-a-cidr", "10.0.0.1", "10.0.0.0/33"} { + translatedIPSet, setInfo, err := ipBlockRule("test", defaultNS, policies.Ingress, policies.SrcMatch, 0, 0, + &networkingv1.IPBlock{CIDR: "172.17.0.0/16", Except: []string{except}}) + require.ErrorIs(t, err, ErrUnsupportedIPAddress, "except %q must be refused", except) + require.Nil(t, translatedIPSet) + require.Equal(t, policies.SetInfo{}, setInfo) + } +} + func TestPodSelector(t *testing.T) { matchType := policies.DstMatch policyKey := "test-ns/test-policy" From 6b0e17ffefd68e63cd9185af1fc28ff553e9bee9 Mon Sep 17 00:00:00 2001 From: Isaiah Raya Date: Tue, 8 Sep 2026 21:03:43 +0000 Subject: [PATCH 14/23] fix: [NPM] bound a rule that lists ports and no peers A rule with ports and no peers takes a path that appends one ACL per port with no peer expansion to bound it, so it could materialize an ACL for every port before the check at the end of translation refused the policy. The budget is now checked inside that loop as well, which leaves the end-of- translation check as a backstop rather than the first line of defence. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../translation/parseSelector_test.go | 18 ++++++++++++++++++ .../translation/translatePolicy.go | 13 ++++++++++--- 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/npm/pkg/controlplane/translation/parseSelector_test.go b/npm/pkg/controlplane/translation/parseSelector_test.go index 51c69986838..901f1d8d035 100644 --- a/npm/pkg/controlplane/translation/parseSelector_test.go +++ b/npm/pkg/controlplane/translation/parseSelector_test.go @@ -920,6 +920,24 @@ func TestTranslatePolicyOrdinaryPolicyWithinACLBudget(t *testing.T) { require.Less(t, len(npmNetPol.ACLs), maxACLsPerPolicy) } +// TestPortOnlyRuleBudgetStopsWithinPortLoop covers a rule that lists ports and no peers. That +// path emits one ACL per port with no peer expansion to bound it, so the budget has to be +// checked inside its loop rather than only by the backstop at the end of translation. +func TestPortOnlyRuleBudgetStopsWithinPortLoop(t *testing.T) { + portCount := maxACLsPerPolicy * 2 + ports := make([]networkingv1.NetworkPolicyPort, 0, portCount) + for i := 0; i < portCount; i++ { + p := intstr.FromInt(1 + i) + ports = append(ports, networkingv1.NetworkPolicyPort{Port: &p}) + } + + npmNetPol := policies.NewNPMNetworkPolicy("port-only", defaultNS) + err := checkOnlyPortRuleExists(true, false, false, ports, false, policies.Ingress, npmNetPol) + require.ErrorIs(t, err, ErrTooManyACLs) + require.LessOrEqual(t, len(npmNetPol.ACLs), maxACLsPerPolicy, + "a rule with only ports must stop once the budget is spent") +} + // TestPeerAndPortRuleBudgetStopsWithinPortLoop covers a single peer listing more ports than // the budget allows. One peer emits one ACL per port, so a budget checked only on entry to // peerAndPortRule would let that peer materialize every ACL before anything noticed. diff --git a/npm/pkg/controlplane/translation/translatePolicy.go b/npm/pkg/controlplane/translation/translatePolicy.go index 162b44b13ff..d0fa03d4cfe 100644 --- a/npm/pkg/controlplane/translation/translatePolicy.go +++ b/npm/pkg/controlplane/translation/translatePolicy.go @@ -851,9 +851,10 @@ func TranslatePolicy(npObj *networkingv1.NetworkPolicy, npmLiteToggle bool) (*po const maxACLsPerPolicy = 2000 // checkACLBudget reports whether the policy has reached the ceiling. It is checked before a -// peer is expanded and before each of that peer's ports, so translation never materializes -// more than maxACLsPerPolicy ACLs, and once more at the end as a backstop for the paths that -// append without a check. +// peer is expanded, before each of that peer's ports, and before each port of a port-only +// rule, so translation never materializes more than maxACLsPerPolicy ACLs on the ipset path. +// It is checked once more at the end of translation as a backstop, which covers the paths +// that append without a check, including the direct-rule path this change leaves alone. func checkACLBudget(npmNetPol *policies.NPMNetworkPolicy) error { if len(npmNetPol.ACLs) >= maxACLsPerPolicy { // The error carries the policy context and is recorded once by the caller. @@ -883,6 +884,12 @@ func checkOnlyPortRuleExists( // #1. Only Ports fields exist in rule if portRuleExists && !peerRuleExists && !allowExternal { for i := range ports { + // This path emits one ACL per port with no peer to bound it, so the budget is + // checked here too rather than leaving it to the backstop at the end. + if err := checkACLBudget(npmNetPol); err != nil { + return err + } + portKind, err := portType(ports[i]) if err != nil { return err From b9818fb996219963141327d7f65dfb419135e199 Mon Sep 17 00:00:00 2001 From: Isaiah Raya Date: Tue, 8 Sep 2026 22:08:08 +0000 Subject: [PATCH 15/23] chore: [NPM] keep the new error messages bounded and conventional The two selector errors printed the whole namespaceSelector. The selector that triggers them is attacker-controlled and need not be small, so a policy that fails to translate could bury the log it was meant to explain. They now name the requirement that failed - its operator and key, or its key and value count - which is the part that identifies the problem. The translation failure the network policy controller returns also carried a bracketed tag and a capitalized sentence. It is always wrapped by the caller, so it reads as a lowercase fragment now. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../controllers/v2/networkPolicyController.go | 6 +++--- npm/pkg/controlplane/translation/parseSelector.go | 13 ++++++++----- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/npm/pkg/controlplane/controllers/v2/networkPolicyController.go b/npm/pkg/controlplane/controllers/v2/networkPolicyController.go index c09544dede3..84aac9bb9b2 100644 --- a/npm/pkg/controlplane/controllers/v2/networkPolicyController.go +++ b/npm/pkg/controlplane/controllers/v2/networkPolicyController.go @@ -308,11 +308,11 @@ func (c *NetworkPolicyController) syncAddAndUpdateNetPol(netPolObj *networkingv1 // The error is deliberately not logged or counted here: processNextWorkItem already // runs the returned error through utilruntime.HandleError and SendErrorLogAndMetric, // so recording it here as well would emit the same failure three times. The wrapped - // message carries the policy name and namespace so that single record stays specific. + // message names the policy so that single record stays specific. // // The exec time isn't relevant here, so consider a no-op. - return metrics.NoOp, fmt.Errorf("[syncAddAndUpdateNetPol] Error: failed to translate NetworkPolicy %s in namespace %s: %w", - netPolObj.Name, netPolObj.Namespace, err) + return metrics.NoOp, fmt.Errorf("translating network policy %s/%s: %w", + netPolObj.Namespace, netPolObj.Name, err) } _, policyExisted := c.rawNpSpecMap[netpolKey] diff --git a/npm/pkg/controlplane/translation/parseSelector.go b/npm/pkg/controlplane/translation/parseSelector.go index be1bbb1082e..bf389cbd5f7 100644 --- a/npm/pkg/controlplane/translation/parseSelector.go +++ b/npm/pkg/controlplane/translation/parseSelector.go @@ -157,9 +157,10 @@ func flattenNameSpaceSelector(nsSelector *metav1.LabelSelector) ([]metav1.LabelS default: // Fail closed: an unknown operator must not silently drop the requirement // and widen the selector. Kubernetes only admits In/NotIn/Exists/DoesNotExist. - // The error carries the selector context and is recorded once by the caller. - return nil, fmt.Errorf("operator %q on key %q in selector %v: %w", - req.Operator, req.Key, *nsSelector, ErrUnsupportedMatchExpressionOperator) + // The operator and key identify the requirement without copying the whole + // selector into the message, which a hostile selector could make enormous. + return nil, fmt.Errorf("operator %q on key %q: %w", + req.Operator, req.Key, ErrUnsupportedMatchExpressionOperator) } } @@ -183,8 +184,10 @@ func flattenNameSpaceSelector(nsSelector *metav1.LabelSelector) ([]metav1.LabelS combinations := 1 for _, req := range multiValueMatchExprs { if len(req.Values) > maxFlattenedNSSelectors/combinations { - return nil, fmt.Errorf("selector %v expands past the %d selector limit: %w", - *nsSelector, maxFlattenedNSSelectors, ErrTooManyFlattenedSelectors) + // Summarize rather than print the selector: the message must stay bounded + // precisely because the selector that triggers it need not be. + return nil, fmt.Errorf("key %q with %d values expands past the %d selector limit: %w", + req.Key, len(req.Values), maxFlattenedNSSelectors, ErrTooManyFlattenedSelectors) } combinations *= len(req.Values) } From f1fa41f3367e0e11a87972219716588c63347670 Mon Sep 17 00:00:00 2001 From: Isaiah Raya Date: Tue, 8 Sep 2026 22:39:45 +0000 Subject: [PATCH 16/23] fix: [NPM] let a policy land exactly on the rule ceiling The guard asked before an ACL is appended answers "is there room for one more", so it has to refuse at the ceiling. Reusing it on the finished policy asked a different question and refused a policy of exactly the ceiling, which made the effective limit one rule lower than the constant says and than the per-append guards allow. The end-of-translation backstop is now its own check, comparing the finished count against the ceiling, so a policy that lands exactly on it translates while one that overshot through an unchecked append path is still refused. Both share the refusal, so the error is unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../translation/parseSelector_test.go | 29 +++++++++++++++++ .../translation/translatePolicy.go | 32 +++++++++++++------ 2 files changed, 52 insertions(+), 9 deletions(-) diff --git a/npm/pkg/controlplane/translation/parseSelector_test.go b/npm/pkg/controlplane/translation/parseSelector_test.go index 901f1d8d035..6aaba4a361e 100644 --- a/npm/pkg/controlplane/translation/parseSelector_test.go +++ b/npm/pkg/controlplane/translation/parseSelector_test.go @@ -920,6 +920,35 @@ func TestTranslatePolicyOrdinaryPolicyWithinACLBudget(t *testing.T) { require.Less(t, len(npmNetPol.ACLs), maxACLsPerPolicy) } +// TestTranslatePolicyExactlyAtACLLimit guards the boundary. The per-append guard is asked +// whether there is room for one more ACL, so it must refuse at the ceiling; the check on the +// finished policy is asked whether the policy is past the ceiling, so it must admit a policy +// that lands exactly on it. Using the same comparison for both would reject a policy of +// exactly maxACLsPerPolicy rules. +func TestTranslatePolicyExactlyAtACLLimit(t *testing.T) { + // one ACL per port, plus the default drop the policy implies. + portCount := maxACLsPerPolicy - 1 + ports := make([]networkingv1.NetworkPolicyPort, 0, portCount) + for i := 0; i < portCount; i++ { + p := intstr.FromInt(1 + i) + ports = append(ports, networkingv1.NetworkPolicyPort{Port: &p}) + } + + pol := &networkingv1.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: "at-limit", Namespace: defaultNS}, + Spec: networkingv1.NetworkPolicySpec{ + PodSelector: metav1.LabelSelector{}, + PolicyTypes: []networkingv1.PolicyType{networkingv1.PolicyTypeIngress}, + Ingress: []networkingv1.NetworkPolicyIngressRule{{Ports: ports}}, + }, + } + + npmNetPol, err := TranslatePolicy(pol, false) + require.NoError(t, err, "a policy landing exactly on the ceiling must translate") + require.NotNil(t, npmNetPol) + require.Len(t, npmNetPol.ACLs, maxACLsPerPolicy) +} + // TestPortOnlyRuleBudgetStopsWithinPortLoop covers a rule that lists ports and no peers. That // path emits one ACL per port with no peer expansion to bound it, so the budget has to be // checked inside its loop rather than only by the backstop at the end of translation. diff --git a/npm/pkg/controlplane/translation/translatePolicy.go b/npm/pkg/controlplane/translation/translatePolicy.go index d0fa03d4cfe..6367ef55ca9 100644 --- a/npm/pkg/controlplane/translation/translatePolicy.go +++ b/npm/pkg/controlplane/translation/translatePolicy.go @@ -834,7 +834,7 @@ func TranslatePolicy(npObj *networkingv1.NetworkPolicy, npmLiteToggle bool) (*po } } - if err := checkACLBudget(npmNetPol); err != nil { + if err := checkACLTotal(npmNetPol); err != nil { return nil, err } @@ -850,20 +850,34 @@ func TranslatePolicy(npObj *networkingv1.NetworkPolicy, npmLiteToggle bool) (*po // since a policy expanding this wide would already be unusable as iptables rules. const maxACLsPerPolicy = 2000 -// checkACLBudget reports whether the policy has reached the ceiling. It is checked before a -// peer is expanded, before each of that peer's ports, and before each port of a port-only -// rule, so translation never materializes more than maxACLsPerPolicy ACLs on the ipset path. -// It is checked once more at the end of translation as a backstop, which covers the paths -// that append without a check, including the direct-rule path this change leaves alone. +// checkACLBudget reports whether there is room for another ACL. It is checked before a peer +// is expanded, before each of that peer's ports, and before each port of a port-only rule, so +// those paths never take the policy past the ceiling. func checkACLBudget(npmNetPol *policies.NPMNetworkPolicy) error { if len(npmNetPol.ACLs) >= maxACLsPerPolicy { - // The error carries the policy context and is recorded once by the caller. - return fmt.Errorf("network policy %s expands past the %d rule limit: %w", - npmNetPol.PolicyKey, maxACLsPerPolicy, ErrTooManyACLs) + return tooManyACLs(npmNetPol) } return nil } +// checkACLTotal reports whether the finished policy is past the ceiling. It is the backstop +// for the paths that append without asking for room first, including the direct-rule path +// this change leaves alone. It admits a policy that lands exactly on the ceiling, which +// checkACLBudget cannot do because it is asked before the ACL exists. +func checkACLTotal(npmNetPol *policies.NPMNetworkPolicy) error { + if len(npmNetPol.ACLs) > maxACLsPerPolicy { + return tooManyACLs(npmNetPol) + } + return nil +} + +// tooManyACLs builds the refusal. The error carries the policy context and is recorded once +// by the caller. +func tooManyACLs(npmNetPol *policies.NPMNetworkPolicy) error { + return fmt.Errorf("network policy %s expands past the %d rule limit: %w", + npmNetPol.PolicyKey, maxACLsPerPolicy, ErrTooManyACLs) +} + func checkForNamedPortType(npmNetPol *policies.NPMNetworkPolicy, portKind netpolPortType, npmLiteToggle bool, direction policies.Direction, port *networkingv1.NetworkPolicyPort, cidr string) error { if npmLiteToggle && portKind == namedPortType { return fmt.Errorf("named port not supported in policy %s (namespace: %s, direction: %s, cidr: %s, port: %v, protocol: %v): %w", From 33f8f7fb0c5e544d846dbefed6e11b601e8ad813 Mon Sep 17 00:00:00 2001 From: Isaiah Raya Date: Tue, 8 Sep 2026 22:49:15 +0000 Subject: [PATCH 17/23] chore: [NPM] name the status code the cache handler returns The handler's other failure path already uses a named constant; this one still passed the literal 500. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- npm/http/server/server.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/npm/http/server/server.go b/npm/http/server/server.go index 43565807843..53c1e3bf633 100644 --- a/npm/http/server/server.go +++ b/npm/http/server/server.go @@ -128,7 +128,7 @@ func (n *NPMRestServer) npmCacheHandler(npmCacheEncoder json.Marshaler) http.Han b, err := json.Marshal(npmCacheEncoder) if err != nil { - http.Error(w, err.Error(), 500) + http.Error(w, err.Error(), http.StatusInternalServerError) return } _, err = w.Write(b) From 39fcc27f9ab183cc35cd294a7b5014bf5c79401a Mon Sep 17 00:00:00 2001 From: Isaiah Raya Date: Thu, 10 Sep 2026 18:08:40 +0000 Subject: [PATCH 18/23] fix: [NPM] serve the debug and profiling routes to the node only The API listens on the host network of a privileged process, so a pod on the node can reach it by reading its own node address from the downward API. The debug route returns the whole policy cache and the pprof routes expose the process, neither of which a workload on the node should be able to read. Both are now served only to requests that originate on the node itself. The tooling that consumes them connects over localhost, so its only caller is unaffected, while a pod, which has its own network namespace and cannot reach the node's loopback, is refused before the cache is encoded. The Prometheus routes are deliberately left alone because they are scraped from off the node. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- npm/http/server/server.go | 39 ++++++++++++++++++---- npm/http/server/server_test.go | 61 ++++++++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+), 7 deletions(-) diff --git a/npm/http/server/server.go b/npm/http/server/server.go index 53c1e3bf633..aa05f8f4b65 100644 --- a/npm/http/server/server.go +++ b/npm/http/server/server.go @@ -78,16 +78,16 @@ func NPMRestServerListenAndServe(config npmconfig.Config, npmEncoder json.Marsha // the nil check is for fan-out npm if config.Toggles.EnableHTTPDebugAPI && npmEncoder != nil { // ACN CLI debug handlers - rs.router.Handle(api.NPMMgrPath, rs.npmCacheHandler(npmEncoder)).Methods(http.MethodGet) + rs.router.Handle(api.NPMMgrPath, loopbackOnly(rs.npmCacheHandler(npmEncoder))).Methods(http.MethodGet) } if config.Toggles.EnablePprof { - rs.router.PathPrefix("/debug/").Handler(http.DefaultServeMux) - rs.router.HandleFunc("/debug/pprof/", pprof.Index) - rs.router.HandleFunc("/debug/pprof/cmdline", pprof.Cmdline) - rs.router.HandleFunc("/debug/pprof/profile", pprof.Profile) - rs.router.HandleFunc("/debug/pprof/symbol", pprof.Symbol) - rs.router.HandleFunc("/debug/pprof/trace", pprof.Trace) + rs.router.PathPrefix("/debug/").Handler(loopbackOnly(http.DefaultServeMux)) + rs.router.Handle("/debug/pprof/", loopbackOnly(http.HandlerFunc(pprof.Index))) + rs.router.Handle("/debug/pprof/cmdline", loopbackOnly(http.HandlerFunc(pprof.Cmdline))) + rs.router.Handle("/debug/pprof/profile", loopbackOnly(http.HandlerFunc(pprof.Profile))) + rs.router.Handle("/debug/pprof/symbol", loopbackOnly(http.HandlerFunc(pprof.Symbol))) + rs.router.Handle("/debug/pprof/trace", loopbackOnly(http.HandlerFunc(pprof.Trace))) } // use default listening address if none is specified @@ -111,6 +111,31 @@ func NPMRestServerListenAndServe(config npmconfig.Config, npmEncoder json.Marsha } } +// loopbackOnly serves a request only when it originated on the node itself. The debug route +// returns NPM's whole policy cache and the pprof routes expose the process, and both are +// served on the host network of a privileged process, so every pod on the node can otherwise +// reach them by reading its own node address. A pod has its own network namespace and cannot +// reach the node's loopback, while the on-node tooling that consumes these routes connects +// over localhost, so this keeps the routes available to their only caller and out of reach of +// a tenant workload. The Prometheus routes are deliberately not wrapped: they are scraped +// from off the node. +func loopbackOnly(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + host, _, err := net.SplitHostPort(r.RemoteAddr) + if err != nil { + http.Error(w, "forbidden", http.StatusForbidden) + return + } + + if ip := net.ParseIP(host); ip == nil || !ip.IsLoopback() { + http.Error(w, "forbidden", http.StatusForbidden) + return + } + + next.ServeHTTP(w, r) + }) +} + func (n *NPMRestServer) npmCacheHandler(npmCacheEncoder json.Marshaler) http.Handler { // Admit only a few encodings at a time. Each one takes the cache lock and buffers the // entire policy cache, so concurrent requests multiply both the lock hold time and the diff --git a/npm/http/server/server_test.go b/npm/http/server/server_test.go index e5aab199269..b1a3c34b119 100644 --- a/npm/http/server/server_test.go +++ b/npm/http/server/server_test.go @@ -137,3 +137,64 @@ func TestServerTimeoutsAreSet(t *testing.T) { require.LessOrEqual(t, maxConcurrentCacheRequests, maxConcurrentConns, "cache encodings must be bounded at or below the connection ceiling") } + +// TestLoopbackOnly covers the guard on the debug and pprof routes. NPM runs on the host +// network of a privileged process, so before this guard any pod on the node could reach +// those routes through its own node address; a pod cannot reach the node's loopback, and +// the on-node tooling that consumes them connects over localhost. +func TestLoopbackOnly(t *testing.T) { + served := false + handler := loopbackOnly(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + served = true + w.WriteHeader(http.StatusOK) + })) + + tests := []struct { + name string + remoteAddr string + wantCode int + wantServed bool + }{ + {"IPv4 loopback", "127.0.0.1:54321", http.StatusOK, true}, + {"IPv4 loopback range", "127.9.9.9:54321", http.StatusOK, true}, + {"IPv6 loopback", "[::1]:54321", http.StatusOK, true}, + // the address a pod on the node would come from + {"pod address", "10.244.1.7:54321", http.StatusForbidden, false}, + // the node's own routable address, which a pod reads from the downward API + {"node address", "10.240.0.4:54321", http.StatusForbidden, false}, + {"malformed remote address", "not-an-address", http.StatusForbidden, false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + served = false + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, api.NPMMgrPath, http.NoBody) + req.RemoteAddr = tt.remoteAddr + + rr := httptest.NewRecorder() + handler.ServeHTTP(rr, req) + + require.Equal(t, tt.wantCode, rr.Code) + require.Equal(t, tt.wantServed, served, "whether the wrapped handler ran") + }) + } +} + +// TestLoopbackOnlyGuardsBeforeHandler makes sure a rejected request never reaches the cache +// encoder. The encoding is the expensive part of the route, so the guard has to run first. +func TestLoopbackOnlyGuardsBeforeHandler(t *testing.T) { + encoder := &blockingMarshaler{ + entered: make(chan struct{}, 1), + release: make(chan struct{}), + } + n := &NPMRestServer{} + handler := loopbackOnly(n.npmCacheHandler(encoder)) + + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, api.NPMMgrPath, http.NoBody) + req.RemoteAddr = "10.244.1.7:54321" + rr := httptest.NewRecorder() + handler.ServeHTTP(rr, req) + + require.Equal(t, http.StatusForbidden, rr.Code) + require.Empty(t, encoder.entered, "the cache must not be encoded for a rejected request") +} From 31faeea163e33652539b93cce3b2c0ccd25ee9e0 Mon Sep 17 00:00:00 2001 From: Isaiah Raya Date: Thu, 10 Sep 2026 18:20:59 +0000 Subject: [PATCH 19/23] fix: [NPM] bound how many ipsets the inventory metric reports NPM creates two ipsets per distinct pod label, and ipset_counts carries the set name as a label, so the number of series it reports followed workload labels rather than anything an operator controls. One pod carrying tens of thousands of labels added that many series on every node, which both retained them in the agent and inflated the response built for each scrape. Deferring kernel materialization does not help here: the sets are still tracked, which is what keeps enforcement correct, so the series were still created. The per-set breakdown now stops growing at a bound far above what a cluster's namespaces, policies and workloads produce. The aggregate counters are untouched and stay exact, and nothing NPM does reads the breakdown, so this only limits reported detail; an operator can tell it is incomplete by comparing the reported series against num_ipsets. Measured with one pod carrying 34,000 labels: agent memory for those labels drops from 88.8 MB to 14.6 MB, and the metrics response from 6.42 MB to 1.87 MB, which no longer varies with the labels a workload chooses. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- npm/metrics/ipsets.go | 34 ++++++++++++++++++++ npm/metrics/ipsets_test.go | 52 +++++++++++++++++++++++++++++++ npm/metrics/prometheus-metrics.go | 1 + 3 files changed, 87 insertions(+) diff --git a/npm/metrics/ipsets.go b/npm/metrics/ipsets.go index a20b711b658..4dd52283811 100644 --- a/npm/metrics/ipsets.go +++ b/npm/metrics/ipsets.go @@ -7,6 +7,21 @@ import ( var ipsetInventoryMap map[string]int +// inventorySeries holds the set names that ipset_counts currently reports, so the number of +// series stays bounded and a set that is already reported keeps reporting. +var inventorySeries map[string]struct{} + +// maxIPSetInventorySeries bounds how many individual ipsets the ipset_counts metric reports. +// That series is labelled by set name, and NPM creates a set per distinct pod label, so its +// cardinality follows workload labels rather than anything an operator controls: one pod +// carrying tens of thousands of labels otherwise adds that many series on every node, which +// both retains them in the agent and inflates the response built for each scrape of an +// endpoint served on the host network. The aggregate counters stay exact past the bound and +// nothing NPM does reads the breakdown, so only the per-set detail stops growing; an operator +// can tell it is incomplete by comparing the reported series against num_ipsets. The bound is +// far above the number of sets a cluster's namespaces, policies and workloads produce. +const maxIPSetInventorySeries = 20000 + // AddPod increments the number of Pod IPs. func AddPod() { podsWatched.Inc() @@ -101,6 +116,7 @@ func ResetIPSetEntries() { removeFromIPSetInventory(setName) } ipsetInventoryMap = make(map[string]int) + inventorySeries = make(map[string]struct{}) } // GetNumIPSets returns the number of IPSets. @@ -131,12 +147,30 @@ func GetIPSetExecCount() (int, error) { } func updateIPSetInventory(setName string) { + if !canReportIPSetInventory(setName) { + return + } labels := getIPSetInventoryLabels(setName) val := getEntryCountForIPSet(setName) ipsetInventory.With(labels).Set(val) } +// canReportIPSetInventory reports whether setName may hold a per-set series, claiming a slot +// for it the first time. A set that already has a series keeps it, so an ipset's reported +// count does not flap once established. +func canReportIPSetInventory(setName string) bool { + if _, reported := inventorySeries[setName]; reported { + return true + } + if len(inventorySeries) >= maxIPSetInventorySeries { + return false + } + inventorySeries[setName] = struct{}{} + return true +} + func removeFromIPSetInventory(setName string) { + delete(inventorySeries, setName) labels := getIPSetInventoryLabels(setName) ipsetInventory.Delete(labels) } diff --git a/npm/metrics/ipsets_test.go b/npm/metrics/ipsets_test.go index 99a20305a2a..a057aff1649 100644 --- a/npm/metrics/ipsets_test.go +++ b/npm/metrics/ipsets_test.go @@ -1,6 +1,7 @@ package metrics import ( + "fmt" "testing" "github.com/Azure/azure-container-networking/npm/metrics/promutil" @@ -204,3 +205,54 @@ func TestResetIPSetEntries(t *testing.T) { assertNumEntriesAndCounts(t, &testSet{testName1, 0}, &testSet{testName2, 0}) assertMapIsGood(t) } + +// TestIPSetInventorySeriesAreBounded covers the cardinality bound on ipset_counts. NPM makes +// a set per distinct pod label, so without a bound a single pod's labels decide how many +// series every node holds and how large the metrics response is. The aggregate counters must +// stay exact regardless, since they are what NPM and its operators actually count on. +func TestIPSetInventorySeriesAreBounded(t *testing.T) { + ResetIPSetEntries() + defer ResetIPSetEntries() + + const over = maxIPSetInventorySeries + 500 + for i := 0; i < over; i++ { + AddEntryToIPSet(fmt.Sprintf("podlabel-key%d:v%d", i, i)) + } + + require.Len(t, inventorySeries, maxIPSetInventorySeries, + "the number of reported series must stop at the bound") + + // the aggregate is unaffected by the bound + entries, err := GetNumIPSetEntries() + promutil.NotifyIfErrors(t, err) + require.Equal(t, over, entries, "the total entry count must still be exact") + + // a set that got a series still reports its own count + first, err := GetNumEntriesForIPSet("podlabel-key0:v0") + promutil.NotifyIfErrors(t, err) + require.Equal(t, 1, first) + + // removing a reported set frees its slot for a new one + RemoveAllEntriesFromIPSet("podlabel-key0:v0") + require.Len(t, inventorySeries, maxIPSetInventorySeries-1) + AddEntryToIPSet("podlabel-fresh:v") + require.Contains(t, inventorySeries, "podlabel-fresh:v") +} + +// TestIPSetInventoryUnboundedBelowLimit guards against the bound changing behaviour for a +// cluster that stays under it, which is every real one. +func TestIPSetInventoryUnboundedBelowLimit(t *testing.T) { + ResetIPSetEntries() + defer ResetIPSetEntries() + + for i := 0; i < 500; i++ { + AddEntryToIPSet(fmt.Sprintf("podlabel-key%d:v%d", i, i)) + } + + require.Len(t, inventorySeries, 500) + for i := 0; i < 500; i++ { + count, err := GetNumEntriesForIPSet(fmt.Sprintf("podlabel-key%d:v%d", i, i)) + promutil.NotifyIfErrors(t, err) + require.Equal(t, 1, count, "every set under the bound reports its own count") + } +} diff --git a/npm/metrics/prometheus-metrics.go b/npm/metrics/prometheus-metrics.go index 4dcc07653f6..59d2f5f8101 100644 --- a/npm/metrics/prometheus-metrics.go +++ b/npm/metrics/prometheus-metrics.go @@ -374,6 +374,7 @@ func initializeDaemonMetrics() { numIPSetEntries = createClusterGauge(numIPSetEntriesName, numIPSetEntriesHelp) ipsetInventory = createClusterGaugeVec(ipsetInventoryName, ipsetInventoryHelp, ipsetInventoryLabels) ipsetInventoryMap = make(map[string]int) + inventorySeries = make(map[string]struct{}) // NODE METRICS addACLRuleExecTime = createNodeSummary(addACLRuleExecTimeName, addACLRuleExecTimeHelp) From 4b767e953ff9573ffcb5e1163276ee9225bfebaa Mon Sep 17 00:00:00 2001 From: Isaiah Raya Date: Thu, 10 Sep 2026 19:57:51 +0000 Subject: [PATCH 20/23] fix: [NPM] mount the profiling handlers at the profile prefix The router matched a /debug/ prefix before the individually named pprof routes, so those named routes never ran and every handler on the default mux was reachable under /debug/, not just the profiles. The default mux is now mounted at the pprof prefix instead, which is where net/http/pprof registers, so the profiles are served, the subpaths that naming each handler missed (/debug/pprof/goroutine and the rest) are served too, and nothing else later registered on the default mux is exposed here. The import returns to a blank one because the handlers are reached through the mux rather than by name. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- npm/http/server/server.go | 17 ++++++----- npm/http/server/server_test.go | 54 ++++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 7 deletions(-) diff --git a/npm/http/server/server.go b/npm/http/server/server.go index aa05f8f4b65..29088706235 100644 --- a/npm/http/server/server.go +++ b/npm/http/server/server.go @@ -7,7 +7,9 @@ import ( "fmt" "net" "net/http" - "net/http/pprof" + // registers the pprof handlers on the default mux, which is mounted at the pprof + // prefix when profiling is enabled. + _ "net/http/pprof" "time" "github.com/Azure/azure-container-networking/log" @@ -82,12 +84,13 @@ func NPMRestServerListenAndServe(config npmconfig.Config, npmEncoder json.Marsha } if config.Toggles.EnablePprof { - rs.router.PathPrefix("/debug/").Handler(loopbackOnly(http.DefaultServeMux)) - rs.router.Handle("/debug/pprof/", loopbackOnly(http.HandlerFunc(pprof.Index))) - rs.router.Handle("/debug/pprof/cmdline", loopbackOnly(http.HandlerFunc(pprof.Cmdline))) - rs.router.Handle("/debug/pprof/profile", loopbackOnly(http.HandlerFunc(pprof.Profile))) - rs.router.Handle("/debug/pprof/symbol", loopbackOnly(http.HandlerFunc(pprof.Symbol))) - rs.router.Handle("/debug/pprof/trace", loopbackOnly(http.HandlerFunc(pprof.Trace))) + // net/http/pprof registers every profile handler on the default mux under this + // prefix, including subpaths such as /debug/pprof/goroutine that naming the + // handlers individually used to miss. The prefix has no trailing slash so that + // /debug/pprof still reaches the mux, which redirects it to the index. Mounting at + // the pprof prefix rather than at /debug/ also keeps anything else later registered + // on the default mux from being served here. + rs.router.PathPrefix("/debug/pprof").Handler(loopbackOnly(http.DefaultServeMux)) } // use default listening address if none is specified diff --git a/npm/http/server/server_test.go b/npm/http/server/server_test.go index b1a3c34b119..64fb79d4725 100644 --- a/npm/http/server/server_test.go +++ b/npm/http/server/server_test.go @@ -198,3 +198,57 @@ func TestLoopbackOnlyGuardsBeforeHandler(t *testing.T) { require.Equal(t, http.StatusForbidden, rr.Code) require.Empty(t, encoder.entered, "the cache must not be encoded for a rejected request") } + +// Remote addresses used by the routing and guard cases below. +const ( + nodeLoopbackAddr = "127.0.0.1:1" + podAddr = "10.244.1.7:1" + // anyRedirect asks for a redirect of any code rather than a specific status. + anyRedirect = -1 +) + +// TestPprofRoutesAreMountedAtTheProfilePrefix covers the routing for the profiling handlers: +// every pprof subpath must be served, the routes must stay behind the loopback guard, and +// nothing else on the default mux may be reachable through /debug/. +func TestPprofRoutesAreMountedAtTheProfilePrefix(t *testing.T) { + http.DefaultServeMux.HandleFunc("/debug/unrelated", func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + }) + + router := mux.NewRouter() + router.PathPrefix("/debug/pprof").Handler(loopbackOnly(http.DefaultServeMux)) + + tests := []struct { + name string + path string + remoteAddr string + wantCode int + }{ + {"pprof index from the node", "/debug/pprof/", nodeLoopbackAddr, http.StatusOK}, + {"pprof cmdline from the node", "/debug/pprof/cmdline", nodeLoopbackAddr, http.StatusOK}, + // a subpath that naming each handler individually did not cover + {"pprof goroutine from the node", "/debug/pprof/goroutine", nodeLoopbackAddr, http.StatusOK}, + // without the trailing slash the mux redirects to the index rather than 404ing. + // The exact redirect code is the mux's choice, so only the class is asserted. + {"pprof index without a trailing slash", "/debug/pprof", nodeLoopbackAddr, anyRedirect}, + {"pprof index from a pod", "/debug/pprof/", podAddr, http.StatusForbidden}, + {"pprof goroutine from a pod", "/debug/pprof/goroutine", podAddr, http.StatusForbidden}, + // anything else on the default mux must not be reachable through this router + {"unrelated default mux route", "/debug/unrelated", nodeLoopbackAddr, http.StatusNotFound}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, tt.path, http.NoBody) + req.RemoteAddr = tt.remoteAddr + rr := httptest.NewRecorder() + router.ServeHTTP(rr, req) + if tt.wantCode == anyRedirect { + require.GreaterOrEqual(t, rr.Code, http.StatusMultipleChoices) + require.Less(t, rr.Code, http.StatusBadRequest) + return + } + require.Equal(t, tt.wantCode, rr.Code) + }) + } +} From a1982d9cae0832c75d3445e150be6a32ba9147e1 Mon Sep 17 00:00:00 2001 From: Isaiah Raya Date: Thu, 10 Sep 2026 22:29:57 +0000 Subject: [PATCH 21/23] fix: [NPM] bound the matches a namespaceSelector expands into Compiling a multi-value NotIn as one conjunction keeps it out of both existing bounds: it stays a single selector, so the flattened-selector count does not see it, and it produces a single rule, so the per-policy rule budget does not either. Every value still becomes its own ipset and its own condition on that rule, so a valid policy listing a long NotIn could still create thousands of sets and one enormous rule. The matches a selector expands into are now counted before anything is allocated, against the same ceiling used for the selector count. A NotIn contributes one per value, In one per branch, and Exists and DoesNotExist one each. Selectors on the bound still translate. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../controlplane/translation/parseSelector.go | 27 +++++++++ .../translation/parseSelector_test.go | 55 +++++++++++++++++++ .../translation/translatePolicy.go | 5 ++ 3 files changed, 87 insertions(+) diff --git a/npm/pkg/controlplane/translation/parseSelector.go b/npm/pkg/controlplane/translation/parseSelector.go index bf389cbd5f7..b911378595c 100644 --- a/npm/pkg/controlplane/translation/parseSelector.go +++ b/npm/pkg/controlplane/translation/parseSelector.go @@ -14,6 +14,13 @@ import ( // an alphanumeric character (e.g. 'MyValue', or 'my_value', or '12345', regex used for validation is '(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?' var validLabelRegex = regexp.MustCompile("(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?") +// maxSelectorMatches bounds how many set matches a single namespaceSelector may expand into. +// Each match becomes its own IPSet and its own condition on the rule the selector produces, and +// a multi-value NotIn contributes one per value while staying in a single selector, so it is +// counted by neither maxFlattenedNSSelectors nor the per-policy rule budget. The bound is the +// same ceiling used for the selector count, and is far above any workable selector. +const maxSelectorMatches = maxFlattenedNSSelectors + // maxFlattenedNSSelectors caps how many labelSelectors a single namespaceSelector may be // flattened into. Flattening multi-value In requirements produces the Cartesian product of // their values, and each resulting selector is deep-copied and later turned into its own @@ -81,6 +88,26 @@ func flattenNameSpaceSelector(nsSelector *metav1.LabelSelector) ([]metav1.LabelS return []metav1.LabelSelector{*nsSelector}, nil } + // Bound how far this selector expands, before anything is allocated. A multi-value NotIn + // stays inside a single selector, so it is invisible to both the selector-count bound + // further down and the per-policy rule budget, yet every one of its values becomes its own + // IPSet and its own condition on one rule. Counting the matches the selector will produce + // is what catches that. + matches := len(nsSelector.MatchLabels) + for _, req := range nsSelector.MatchExpressions { + if req.Operator == metav1.LabelSelectorOpNotIn { + // each excluded value is carried as its own negated match + matches += len(req.Values) + continue + } + // In contributes one match per branch; Exists and DoesNotExist one each + matches++ + } + if matches > maxSelectorMatches { + return nil, fmt.Errorf("selector expands into %d matches, past the %d limit: %w", + matches, maxSelectorMatches, ErrTooManySelectorMatches) + } + // create a baseSelector which needs to be same across all // new labelSelectors baseSelector := &metav1.LabelSelector{ diff --git a/npm/pkg/controlplane/translation/parseSelector_test.go b/npm/pkg/controlplane/translation/parseSelector_test.go index 6aaba4a361e..6d3aea2d1a6 100644 --- a/npm/pkg/controlplane/translation/parseSelector_test.go +++ b/npm/pkg/controlplane/translation/parseSelector_test.go @@ -984,3 +984,58 @@ func TestPeerAndPortRuleBudgetStopsWithinPortLoop(t *testing.T) { require.LessOrEqual(t, len(npmNetPol.ACLs), maxACLsPerPolicy, "the port loop must stop once the budget is spent instead of emitting an ACL for every port") } + +// TestNotInValuesAreBounded covers a long NotIn list. Compiling it as one conjunction keeps it +// out of the flattened-selector count and out of the rule budget, because it stays a single +// selector producing a single rule, but every value still becomes its own IPSet and its own +// condition on that rule. The match bound is what stops it. +func TestNotInValuesAreBounded(t *testing.T) { + values := make([]string, 0, maxSelectorMatches+1) + for i := 0; i <= maxSelectorMatches; i++ { + values = append(values, fmt.Sprintf("v%d", i)) + } + + selector := &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + {Key: tenantLabelKey, Operator: metav1.LabelSelectorOpNotIn, Values: values}, + }, + } + + flattened, err := flattenNameSpaceSelector(selector) + require.ErrorIs(t, err, ErrTooManySelectorMatches, + "a NotIn list past the match bound must be refused") + require.Nil(t, flattened) + + // the same policy is refused end to end, so no partial rules are installed + pol := &networkingv1.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: "wide-notin", Namespace: defaultNS}, + Spec: networkingv1.NetworkPolicySpec{ + PodSelector: metav1.LabelSelector{}, + PolicyTypes: []networkingv1.PolicyType{networkingv1.PolicyTypeIngress}, + Ingress: []networkingv1.NetworkPolicyIngressRule{{ + From: []networkingv1.NetworkPolicyPeer{{NamespaceSelector: selector}}, + }}, + }, + } + npmNetPol, err := TranslatePolicy(pol, false) + require.ErrorIs(t, err, ErrTooManySelectorMatches) + require.Nil(t, npmNetPol) +} + +// TestNotInValuesAtTheBoundAreAccepted keeps the bound from rejecting a selector that sits +// exactly on it, and guards the ordinary small NotIn that real policies use. +func TestNotInValuesAtTheBoundAreAccepted(t *testing.T) { + values := make([]string, 0, maxSelectorMatches) + for i := 0; i < maxSelectorMatches; i++ { + values = append(values, fmt.Sprintf("v%d", i)) + } + + flattened, err := flattenNameSpaceSelector(&metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + {Key: tenantLabelKey, Operator: metav1.LabelSelectorOpNotIn, Values: values}, + }, + }) + require.NoError(t, err, "a selector exactly on the bound must translate") + require.Len(t, flattened, 1, "a NotIn stays a single conjunction") + require.Len(t, flattened[0].MatchExpressions, maxSelectorMatches) +} diff --git a/npm/pkg/controlplane/translation/translatePolicy.go b/npm/pkg/controlplane/translation/translatePolicy.go index 6367ef55ca9..bda8c47e8d5 100644 --- a/npm/pkg/controlplane/translation/translatePolicy.go +++ b/npm/pkg/controlplane/translation/translatePolicy.go @@ -47,6 +47,11 @@ var ( // requirements would produce more labelSelectors than NPM is willing to translate. The count is // the product of the value counts, so it grows exponentially with the number of such requirements. ErrTooManyFlattenedSelectors = errors.New("namespaceSelector expands into too many label selectors") + // ErrTooManySelectorMatches is returned when a namespaceSelector expands into more set matches + // than NPM is willing to translate. A multi-value NotIn contributes one match per value while + // staying in a single selector, so it is counted by neither the flattened-selector bound nor the + // per-policy rule budget, yet each match becomes its own IPSet and its own condition on a rule. + ErrTooManySelectorMatches = errors.New("namespaceSelector expands into too many set matches") // ErrTooManyACLs is returned when a NetworkPolicy translates into more ACLs than NPM is // willing to program. ACL count multiplies rather than adds: flattened selector branches are // emitted per port, summed across peers and rules, so bounding selectors alone is not enough. From 02f6e935f48a169a2b747771fa7cb0c0eeed6283 Mon Sep 17 00:00:00 2001 From: Isaiah Raya Date: Thu, 10 Sep 2026 23:28:43 +0000 Subject: [PATCH 22/23] fix: [NPM] bound a selector that carries only matchLabels The match bound sat below the shortcut that returns a selector carrying no match expressions, so a selector made only of matchLabels skipped it even though each of those labels becomes its own ipset and its own condition on the rule. The bound is now applied before that shortcut. Also pins the ipBlock member packing now that except CIDRs are canonicalized first. Canonicalizing can turn an except into one of the two halves that 0.0.0.0/0 is split into, which takes the branch that rewrites an existing member and shortens the list, so the test covers a split-half except first, last, between two ordinary excepts, both halves at once, and a non-canonical all-addresses block, and asserts every other except still survives. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../controlplane/translation/parseSelector.go | 18 +++--- .../translation/parseSelector_test.go | 22 +++++++ .../translation/translatePolicy_test.go | 58 +++++++++++++++++++ 3 files changed, 89 insertions(+), 9 deletions(-) diff --git a/npm/pkg/controlplane/translation/parseSelector.go b/npm/pkg/controlplane/translation/parseSelector.go index b911378595c..e2c715ee771 100644 --- a/npm/pkg/controlplane/translation/parseSelector.go +++ b/npm/pkg/controlplane/translation/parseSelector.go @@ -84,15 +84,11 @@ func flattenNameSpaceSelector(nsSelector *metav1.LabelSelector) ([]metav1.LabelS return []metav1.LabelSelector{}, nil } - if len(nsSelector.MatchExpressions) == 0 { - return []metav1.LabelSelector{*nsSelector}, nil - } - - // Bound how far this selector expands, before anything is allocated. A multi-value NotIn - // stays inside a single selector, so it is invisible to both the selector-count bound - // further down and the per-policy rule budget, yet every one of its values becomes its own - // IPSet and its own condition on one rule. Counting the matches the selector will produce - // is what catches that. + // Bound how many matches this selector produces, before anything is allocated and before + // the matchLabels-only shortcut below, since those labels each become a match too. A + // multi-value NotIn stays inside a single selector, so it is invisible to both the + // selector-count bound further down and the per-policy rule budget, yet every one of its + // values becomes its own IPSet and its own condition on one rule. matches := len(nsSelector.MatchLabels) for _, req := range nsSelector.MatchExpressions { if req.Operator == metav1.LabelSelectorOpNotIn { @@ -108,6 +104,10 @@ func flattenNameSpaceSelector(nsSelector *metav1.LabelSelector) ([]metav1.LabelS matches, maxSelectorMatches, ErrTooManySelectorMatches) } + if len(nsSelector.MatchExpressions) == 0 { + return []metav1.LabelSelector{*nsSelector}, nil + } + // create a baseSelector which needs to be same across all // new labelSelectors baseSelector := &metav1.LabelSelector{ diff --git a/npm/pkg/controlplane/translation/parseSelector_test.go b/npm/pkg/controlplane/translation/parseSelector_test.go index 6d3aea2d1a6..58e5dd8278b 100644 --- a/npm/pkg/controlplane/translation/parseSelector_test.go +++ b/npm/pkg/controlplane/translation/parseSelector_test.go @@ -1039,3 +1039,25 @@ func TestNotInValuesAtTheBoundAreAccepted(t *testing.T) { require.Len(t, flattened, 1, "a NotIn stays a single conjunction") require.Len(t, flattened[0].MatchExpressions, maxSelectorMatches) } + +// TestMatchLabelsOnlySelectorIsBounded covers a selector that carries only matchLabels. It +// takes a shortcut past the expression handling, but each label still becomes its own match, +// so the bound has to be applied before that shortcut. +func TestMatchLabelsOnlySelectorIsBounded(t *testing.T) { + labels := make(map[string]string, maxSelectorMatches+1) + for i := 0; i <= maxSelectorMatches; i++ { + labels[fmt.Sprintf("key%d", i)] = "v" + } + + flattened, err := flattenNameSpaceSelector(&metav1.LabelSelector{MatchLabels: labels}) + require.ErrorIs(t, err, ErrTooManySelectorMatches, + "a matchLabels-only selector past the bound must be refused") + require.Nil(t, flattened) + + // an ordinary selector is untouched + ok, err := flattenNameSpaceSelector(&metav1.LabelSelector{ + MatchLabels: map[string]string{"team": teamBlueValue}, + }) + require.NoError(t, err) + require.Len(t, ok, 1) +} diff --git a/npm/pkg/controlplane/translation/translatePolicy_test.go b/npm/pkg/controlplane/translation/translatePolicy_test.go index ac1e89d148c..209b1cf62ba 100644 --- a/npm/pkg/controlplane/translation/translatePolicy_test.go +++ b/npm/pkg/controlplane/translation/translatePolicy_test.go @@ -30,6 +30,7 @@ const ( blockedLabelKey string = "blocked" teamBlueValue string = "blue" lowerHalfNomatch string = "0.0.0.0/1 nomatch" + exceptedClassA string = "200.0.0.0/8" ingressName string = "ingress" egressName string = "egress" ) @@ -4080,3 +4081,60 @@ func TestTranslatePolicyNegationOnlyOperators(t *testing.T) { } } } + +// TestIPBlockExceptCanonicalizationKeepsEveryExcept locks the member packing now that except +// CIDRs are canonicalized first. Canonicalizing can turn an except into one of the two halves +// that 0.0.0.0/0 is split into, which takes a different branch of the packing loop and shortens +// the member list, so every other except must still survive that, whatever order they arrive in. +func TestIPBlockExceptCanonicalizationKeepsEveryExcept(t *testing.T) { + if util.IsWindowsDP() { + t.Skip("the Windows datapath refuses any except on this path") + } + + tests := []struct { + name string + cidr string + except []string + want []string + }{ + { + name: "except canonicalizes onto the lower half, listed last", + cidr: "0.0.0.0/0", + except: []string{exceptedClassA, "10.0.0.0/1"}, + want: []string{lowerHalfNomatch, "128.0.0.0/1", exceptedClassA + " nomatch"}, + }, + { + name: "same excepts in the other order", + cidr: "0.0.0.0/0", + except: []string{"10.0.0.0/1", exceptedClassA}, + want: []string{lowerHalfNomatch, "128.0.0.0/1", exceptedClassA + " nomatch"}, + }, + { + name: "a split-half except between two ordinary ones", + cidr: "0.0.0.0/0", + except: []string{exceptedClassA, "10.0.0.0/1", "9.0.0.0/8"}, + want: []string{lowerHalfNomatch, "128.0.0.0/1", exceptedClassA + " nomatch", "9.0.0.0/8 nomatch"}, + }, + { + name: "both halves reached by canonicalization", + cidr: "0.0.0.0/0", + except: []string{exceptedClassA, "250.0.0.0/1", "10.0.0.0/1"}, + want: []string{lowerHalfNomatch, "128.0.0.0/1 nomatch", exceptedClassA + " nomatch"}, + }, + { + name: "a non-canonical all-addresses block behaves the same", + cidr: "10.0.0.0/0", + except: []string{exceptedClassA, "10.0.0.0/1"}, + want: []string{lowerHalfNomatch, "128.0.0.0/1", exceptedClassA + " nomatch"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + set, err := ipBlockIPSet("p", defaultNS, policies.Ingress, 0, 0, + &networkingv1.IPBlock{CIDR: tt.cidr, Except: tt.except}) + require.NoError(t, err) + require.Equal(t, tt.want, set.Members) + }) + } +} From be25db297c53fab38024fbdf5cee5f2029b1cc1f Mon Sep 17 00:00:00 2001 From: Isaiah Raya Date: Fri, 11 Sep 2026 16:35:31 +0000 Subject: [PATCH 23/23] fix: [NPM] bound a selector across its branches and keep the rule ceiling exact Two bounds were checkable individually but not together. A multi-value In repeats the whole selector once per value, so a selector can sit under both the match bound and the branch bound while their product is enormous: 991 matchLabels with nine two-value In requirements is 1000 matches across 512 branches, and the translator materializes an ipset and a set reference for each before the policy's rule budget is consulted. The product is now bounded too. The branch count is still checked on its own terms first, so a selector that merely fans out too far reports that rather than the total. The match count also missed the all-namespaces anchor that a selector matching only negatively is given, so such a selector could produce one match more than the bound allowed. Separately, the default drop a policy implies was appended before the check at the end of translation, so a policy that filled the budget with allow rules landed one or two ACLs past the ceiling before being refused. The per-append guard now holds back a slot per direction, so the ceiling is not exceeded. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../controlplane/translation/parseSelector.go | 47 ++++++++++++- .../translation/parseSelector_test.go | 67 +++++++++++++++++-- .../translation/translatePolicy.go | 10 ++- 3 files changed, 112 insertions(+), 12 deletions(-) diff --git a/npm/pkg/controlplane/translation/parseSelector.go b/npm/pkg/controlplane/translation/parseSelector.go index e2c715ee771..0e67c1e6a3e 100644 --- a/npm/pkg/controlplane/translation/parseSelector.go +++ b/npm/pkg/controlplane/translation/parseSelector.go @@ -14,6 +14,13 @@ import ( // an alphanumeric character (e.g. 'MyValue', or 'my_value', or '12345', regex used for validation is '(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?' var validLabelRegex = regexp.MustCompile("(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?") +// maxTotalSelectorMatches bounds the set matches a namespaceSelector produces across every +// branch it fans out into. A multi-value In repeats the whole selector once per value, so the +// cost is the branch count multiplied by the matches in each branch; bounding either factor on +// its own leaves a wide selector repeated across many branches unbounded, and the translator +// materializes an IPSet and a SetInfo for each before the per-policy rule budget is consulted. +const maxTotalSelectorMatches = 10000 + // maxSelectorMatches bounds how many set matches a single namespaceSelector may expand into. // Each match becomes its own IPSet and its own condition on the rule the selector produces, and // a multi-value NotIn contributes one per value while staying in a single selector, so it is @@ -90,19 +97,53 @@ func flattenNameSpaceSelector(nsSelector *metav1.LabelSelector) ([]metav1.LabelS // selector-count bound further down and the per-policy rule budget, yet every one of its // values becomes its own IPSet and its own condition on one rule. matches := len(nsSelector.MatchLabels) + branches := 1 + hasPositiveMatch := len(nsSelector.MatchLabels) > 0 for _, req := range nsSelector.MatchExpressions { - if req.Operator == metav1.LabelSelectorOpNotIn { + switch req.Operator { + case metav1.LabelSelectorOpNotIn: // each excluded value is carried as its own negated match matches += len(req.Values) - continue + case metav1.LabelSelectorOpIn: + // one match per branch, and a multi-value In fans out into branches + matches++ + hasPositiveMatch = true + if len(req.Values) > 1 { + // the branch count is bounded on its own terms first, so a selector that + // fans out too far still reports that rather than the total below. + // Divide rather than multiply so the product cannot overflow. + if len(req.Values) > maxFlattenedNSSelectors/branches { + return nil, fmt.Errorf("key %q with %d values expands past the %d selector limit: %w", + req.Key, len(req.Values), maxFlattenedNSSelectors, ErrTooManyFlattenedSelectors) + } + branches *= len(req.Values) + } + case metav1.LabelSelectorOpExists: + matches++ + hasPositiveMatch = true + case metav1.LabelSelectorOpDoesNotExist: + matches++ + default: + // an unknown operator, which the loop below rejects + matches++ } - // In contributes one match per branch; Exists and DoesNotExist one each + } + if !hasPositiveMatch { + // parseNSSelector anchors a selector that matches only negatively with the + // all-namespaces set, so that match counts too matches++ } if matches > maxSelectorMatches { return nil, fmt.Errorf("selector expands into %d matches, past the %d limit: %w", matches, maxSelectorMatches, ErrTooManySelectorMatches) } + // Each branch repeats every match, so the cost is the product rather than either factor. + // The branch count alone is bounded further down and the rule count by the policy budget, + // but neither sees a wide selector repeated across many branches. + if matches > maxTotalSelectorMatches/branches { + return nil, fmt.Errorf("selector expands into %d branches of %d matches, past the %d total match limit: %w", + branches, matches, maxTotalSelectorMatches, ErrTooManySelectorMatches) + } if len(nsSelector.MatchExpressions) == 0 { return []metav1.LabelSelector{*nsSelector}, nil diff --git a/npm/pkg/controlplane/translation/parseSelector_test.go b/npm/pkg/controlplane/translation/parseSelector_test.go index 58e5dd8278b..e1b03045d87 100644 --- a/npm/pkg/controlplane/translation/parseSelector_test.go +++ b/npm/pkg/controlplane/translation/parseSelector_test.go @@ -926,8 +926,9 @@ func TestTranslatePolicyOrdinaryPolicyWithinACLBudget(t *testing.T) { // that lands exactly on it. Using the same comparison for both would reject a policy of // exactly maxACLsPerPolicy rules. func TestTranslatePolicyExactlyAtACLLimit(t *testing.T) { - // one ACL per port, plus the default drop the policy implies. - portCount := maxACLsPerPolicy - 1 + // the budget holds back a slot for the default drop the policy implies, so this is the + // widest a policy can get: every port emits an ACL and the drop still fits under the ceiling + portCount := maxACLsPerPolicy - reservedDropACLs ports := make([]networkingv1.NetworkPolicyPort, 0, portCount) for i := 0; i < portCount; i++ { p := intstr.FromInt(1 + i) @@ -944,9 +945,11 @@ func TestTranslatePolicyExactlyAtACLLimit(t *testing.T) { } npmNetPol, err := TranslatePolicy(pol, false) - require.NoError(t, err, "a policy landing exactly on the ceiling must translate") + require.NoError(t, err, "a policy at the widest the budget allows must translate") require.NotNil(t, npmNetPol) - require.Len(t, npmNetPol.ACLs, maxACLsPerPolicy) + require.Len(t, npmNetPol.ACLs, portCount+1, "every port plus the default drop") + require.LessOrEqual(t, len(npmNetPol.ACLs), maxACLsPerPolicy, + "the drop must never take the policy past the ceiling") } // TestPortOnlyRuleBudgetStopsWithinPortLoop covers a rule that lists ports and no peers. That @@ -1025,8 +1028,10 @@ func TestNotInValuesAreBounded(t *testing.T) { // TestNotInValuesAtTheBoundAreAccepted keeps the bound from rejecting a selector that sits // exactly on it, and guards the ordinary small NotIn that real policies use. func TestNotInValuesAtTheBoundAreAccepted(t *testing.T) { - values := make([]string, 0, maxSelectorMatches) - for i := 0; i < maxSelectorMatches; i++ { + // one short of the bound: the selector matches only negatively, so parseNSSelector + // anchors it with the all-namespaces set and that match counts too + values := make([]string, 0, maxSelectorMatches-1) + for i := 0; i < maxSelectorMatches-1; i++ { values = append(values, fmt.Sprintf("v%d", i)) } @@ -1037,7 +1042,7 @@ func TestNotInValuesAtTheBoundAreAccepted(t *testing.T) { }) require.NoError(t, err, "a selector exactly on the bound must translate") require.Len(t, flattened, 1, "a NotIn stays a single conjunction") - require.Len(t, flattened[0].MatchExpressions, maxSelectorMatches) + require.Len(t, flattened[0].MatchExpressions, maxSelectorMatches-1) } // TestMatchLabelsOnlySelectorIsBounded covers a selector that carries only matchLabels. It @@ -1061,3 +1066,51 @@ func TestMatchLabelsOnlySelectorIsBounded(t *testing.T) { require.NoError(t, err) require.Len(t, ok, 1) } + +// TestSelectorBranchesTimesMatchesIsBounded covers a selector that stays under both the match +// bound and the branch bound yet multiplies them together. Each branch repeats every match, and +// the translator materializes an IPSet and a SetInfo per match before the policy's rule budget +// is consulted, so the product is what has to be bounded. +func TestSelectorBranchesTimesMatchesIsBounded(t *testing.T) { + // 991 labels plus nine two-value In requirements: 1000 matches per branch, 512 branches + labels := make(map[string]string, 991) + for i := 0; i < 991; i++ { + labels[fmt.Sprintf("key%d", i)] = "v" + } + reqs := make([]metav1.LabelSelectorRequirement, 0, 9) + for i := 0; i < 9; i++ { + reqs = append(reqs, metav1.LabelSelectorRequirement{ + Key: fmt.Sprintf("in%d", i), + Operator: metav1.LabelSelectorOpIn, + Values: []string{"a", "b"}, + }) + } + + selector := &metav1.LabelSelector{MatchLabels: labels, MatchExpressions: reqs} + + // each factor on its own is within its bound + require.LessOrEqual(t, len(labels)+len(reqs), maxSelectorMatches) + require.LessOrEqual(t, 1<= maxACLsPerPolicy { + if len(npmNetPol.ACLs) >= maxACLsPerPolicy-reservedDropACLs { return tooManyACLs(npmNetPol) } return nil