fix: [NPM] policy translation correctness and limits - #4859
Open
Isaiah Raya (rayaisaiah) wants to merge 19 commits into
Open
fix: [NPM] policy translation correctness and limits#4859Isaiah Raya (rayaisaiah) wants to merge 19 commits into
Isaiah Raya (rayaisaiah) wants to merge 19 commits into
Conversation
…njunction 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>
…amespaces 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>
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>
…lation 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>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…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>
…ation 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>
… 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>
…rs 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>
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>
…t 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>
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>
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>
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>
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>
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>
…ling 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>
Check the budget immediately before default-drop and allow-all appends instead of reserving two slots throughout translation. Preserve exact-limit policies for either direction and both direction orders. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 175c523d-ad1c-4e31-986d-7c9940c6c18c
Isaiah Raya (rayaisaiah)
requested review from
Mathew Merrick (matmerr)
and
a lite review from Copilot
September 11, 2026 21:52
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Copilot started reviewing on behalf of
Isaiah Raya (rayaisaiah)
September 11, 2026 21:53
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Resolve the namespace anchor collision and Windows unsupported-IP retry handling.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR improves NPM NetworkPolicy translation correctness, CIDR handling, resource limits, and controller retry behavior.
Changes:
- Preserves selector conjunction semantics and scopes namespace selectors.
- Adds selector, match, ACL, and CIDR safeguards.
- Propagates translation failures through controller retries.
File summaries
| File | Summary |
|---|---|
npm/util/util.go |
Adds IPv4 CIDR normalization. |
npm/util/util_test.go |
Tests CIDR normalization and validation. |
npm/pkg/dataplane/policies/policymanager_linux_test.go |
Tests namespace anchor rendering. |
npm/pkg/controlplane/translation/translatePolicy.go |
Adds CIDR handling and ACL limits. |
npm/pkg/controlplane/translation/translatePolicy_test.go |
Tests translation behavior and limits. |
npm/pkg/controlplane/translation/parseSelector.go |
Updates selector semantics and expansion limits. |
npm/pkg/controlplane/translation/parseSelector_test.go |
Tests selector behavior and limits. |
npm/pkg/controlplane/translation/acl_budget_test.go |
Tests ACL budget boundaries. |
npm/pkg/controlplane/controllers/v2/networkPolicyController.go |
Propagates translation errors. |
npm/pkg/controlplane/controllers/v2/networkPolicyController_test.go |
Tests controller error behavior. |
Review details
Suppressed comments (4)
npm/pkg/controlplane/controllers/v2/networkPolicyController.go:380
- The Windows NPM Lite path returns
ErrUnsupportedIPAddressfromdirectPeerAndPortAllowRulefor IPv6/otherwise unrepresentable CIDRs, but this predicate does not classify that sentinel as a deliberate Windows limitation. Such a policy now goes throughAddRateLimitedforever instead of retaining the prior suppressed-with-warning behavior described above, creating persistent queue/log churn for an input the Windows datapath cannot ever apply. Keep this suppression Windows-only so Linux validation errors still use the new retry path.
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)
npm/pkg/controlplane/translation/parseSelector.go:223
- The Kubernetes operator is named
DoesNotExist, notNotExists; using the wrong name in this new comment makes it harder to map the code to the API field.
// since Exists and NotExists do not contain any values, NPM can safely add them to the baseSelector
npm/pkg/controlplane/translation/translatePolicy.go:236
redundanceis not the noun needed here; useredundantor, more clearly,duplicatewhen describing the excluded entries.
// de-duplicated Except if there are redundance elements, in canonical form so they
npm/pkg/controlplane/translation/translatePolicy.go:300
- This comment is too broad:
ipBlockRuleis called on Windows v2 whenevernpmLiteToggleis false (the direct-rule branch is onlynpmLiteToggle && util.IsWindowsDP()), so the shared ipset path is not Linux-only. Please distinguish the unchanged Windows NPM Lite direct-rule path from the shared translator path to avoid documenting the platform behavior incorrectly.
// ipset path, which is Linux only; the Windows direct-rule path is unchanged.
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+412
to
+414
| if !parsedSelectors.hasPositiveSelector() { | ||
| parsedSelectors.addSelector(true, ipsets.KeyLabelOfNamespace, util.KubeAllNamespacesFlag) | ||
| } |
Use a v2-only aggregate name outside label-derived identities and update both membership producers and selector consumers. Preserve unsupported-address handling on Windows. Cover ordinary label keys with the previous aggregate spelling. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 175c523d-ad1c-4e31-986d-7c9940c6c18c
Copilot started reviewing on behalf of
Isaiah Raya (rayaisaiah)
September 11, 2026 22:14
View session
Copilot stopped reviewing on behalf of
Isaiah Raya (rayaisaiah) due to an error
September 11, 2026 22:34
Contributor
There was a problem hiding this comment.
Note
Copilot was unable to run its full agentic suite in this review.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Suppressed comments (1)
npm/pkg/controlplane/controllers/v2/networkPolicyController.go:1
TranslatePolicyfailures that are permanent (e.g., IPv6 ipBlock on an IPv4-only datapath) will now be returned as errors and requeued indefinitely on non-Windows nodes. That can lead to steady controller churn for policies that can never succeed. Consider adding a “terminal translation failure” path (e.g., cache last translation error per netpol key and stop retrying until the object changes), or splittingErrUnsupportedIPAddressinto “invalid policy input” vs “unsupported-by-datapath” and suppress the latter with a warning (while still ensuring the policy is not recorded as applied).
// Copyright 2018 Microsoft. All rights reserved.
Comment on lines
+225
to
+228
| cidr, ok := util.NormalizeCIDR(ipBlockRule.CIDR) | ||
| if !ok { | ||
| return nil, ErrUnsupportedIPAddress | ||
| } |
Comment on lines
+25
to
+26
| require.False(t, matches[0].Included) | ||
| require.True(t, matches[1].Included) |
Comment on lines
+300
to
302
| if _, ok := util.NormalizeCIDR(ipBlockRule.CIDR); !ok { | ||
| return nil, policies.SetInfo{}, ErrUnsupportedIPAddress | ||
| } |
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reason for Change:
Summary
Four policy-translation correctness changes extracted from #4831. The independent HTTP, configuration and telemetry work is kept in draft #4858 so that its runtime decisions do not block review of these fixes.
This branch contains no HTTP-serving, metric-cap, on-demand-IPSet-default or deployment-manifest changes.
Problems addressed and changes
1. Preserve multi-value
NotInconjunctionsProblem:
key NotIn [a, b]is a single requirement: the key must match neither value. The translator expanded it in the same way as a multi-valueIn, producing separate allow decisions. Separate allow decisions combine with OR, which does not preserve the original requirement.Change: Keep all
NotInexclusions together in one selector. Only positiveInrequirements fan out into branches, and every resulting branch retains the complete conjunction of negative requirements. Invalid operators and emptyIn/NotInvalue lists return errors instead of silently dropping requirements.2. Keep namespace selectors scoped to namespace membership
Problem: A negative set match alone expresses which addresses are excluded, not which addresses belong to the namespaces being selected. A selector consisting only of negative requirements therefore lacked a positive namespace-membership condition.
Change: Intersect negative-only selectors with the aggregate namespace-membership set. Selectors that already contain a positive requirement keep their existing shape.
The v2 aggregate also now uses an internal name outside label-derived identities. Previously its name could also be an ordinary namespace label key. Both the namespace/pod membership producers and the translator consumers use the new identity; v1 retains its existing name.
3. Bound generated work before expanding policies
Problem: Multiple multi-value requirements multiply the number of selectors. Ports multiply the resulting ACLs again, while a wide conjunctive selector can produce many set matches without producing many branches. A bound on any one dimension misses the others.
Change: Apply explicit limits to:
The selector limits are checked before expansion, including the
matchLabels-only path. ACL guards run before relevant appends, including default drops and allow-all rules, with a final total check. A fixed two-slot reservation was replaced with actual append accounting so exact-limit policies work for single and dual directions.These are deliberate limits on large inputs, not a claim of an absolute bound on every cluster-wide workload. Rejected inputs are reported through the controller's error path.
4. Normalize CIDRs before classification and report translation failures
Problem: Equivalent IPv4 CIDRs could be treated differently when host bits were present. In particular, a non-canonical all-addresses CIDR could be rejected before normalization. The controller could then treat a failed translation as a successful no-op.
Change: Add
NormalizeCIDR, canonicalize IPv4 blocks and exclusions on the shared IPSet path, and deduplicate canonical exclusions. Invalid IPv4 exclusions fail translation rather than being passed to kernel programming.Linux translation failures are returned to the existing retry/report path instead of being silently accepted. Deliberate unsupported-feature handling is retained, including Windows-only unsupported-address suppression. The Windows NPM Lite direct-rule implementation and shared
IsIPV4helper are unchanged.Returning an error is not a fallback default-deny policy. A new untranslatable policy is not automatically replaced with deny rules.
Validation
The split branch is validated independently, not inferred from the union of the two replacement PRs.
Current candidate:
cf44fd56567baa05de0cce38404f3c189391d2ea.npm/cmdtest and six v1 controller testsLocal datapath testing uses three-node kind with Kubernetes v1.29.2, not AKS. The NPM configuration remains the baseline configuration. To avoid a host inotify-instance limit, the isolated cluster uses kube-proxy command-line configuration instead of its config-file watcher; no host-wide limit was changed. The manual harness verifies positive connectivity before testing denials and does not count missing tools or failed exec requests as policy denials.
Scope and follow-ups
Issue Fixed:
The policy-translation portion of #4831.
Requirements: