fix: [NPM] policy translation correctness and limits by rayaisaiah · Pull Request #4859 · Azure/azure-container-networking

Isaiah Raya (rayaisaiah) · 2026-09-11T21:52:15Z

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.

Scope is full NPM v2 on Linux and Windows. Existing v1 and Lite-specific behavior is preserved; Lite remediation is a separate workstream.

Problems addressed and changes

1. Preserve multi-value `NotIn` conjunctions

Problem: 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-value In, producing separate allow decisions. Separate allow decisions combine with OR, which does not preserve the original requirement.

Change: Keep all NotIn exclusions together in one selector. Only positive In requirements fan out into branches, and every resulting branch retains the complete conjunction of negative requirements. Invalid operators and empty In/NotIn value 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.

V2 diagnostics recognize the aggregate explicitly, distinguish absent from empty-valued namespace labels, and evaluate mixed positive/negative namespace conditions conjunctively even when no aggregate is needed. Combined peer conditions cannot be overridden by an aggregate match. Incomplete diagnostic set metadata is reported rather than guessed.

The converter passes its explicit v1/v2 mode instead of inferring it from user-controlled names. Current policy-scoped nested selectors use the actual label key and encoded values. Separate parent jumps produce separate diagnostic paths (OR across paths, AND within a path), and matched-set reporting includes all v2 conditions. V1 dispatch remains unchanged.

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:

Flattened namespace selectors: 1,000.
Set matches per branch, including generated namespace anchors: 1,000.
Set matches across a selector's branches: 10,000.
ACLs per policy: 2,000.

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.

The pre-allocation work-budget guarantee applies to the full-NPM v2 IPSet translation path. It does not cover the separately owned Windows NPM Lite direct-rule allocator; the shared final total check is not an allocation bound for that path. Lite-specific budgeting is outside this change's scope.

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.

NormalizeCIDR retains distinct invalid-syntax and unsupported-family causes. Full NPM on Windows suppresses only typed unsupported-family limitations, not malformed CIDRs. Parent-CIDR validation retains its existing precedence over the Windows Except guard.

All-addresses exclusions are rejected before set programming. Kubernetes requires an except to be a strict subset of its parent, so a canonical or non-canonical /0 exclusion is invalid for every IPv4 parent. Typed invalid-exclusion errors remain separate from deliberate platform limitations.

Deterministic full-NPM v2 translation failures remain visible errors. The worker forgets them instead of automatically rate-limiting the same failure, and rejected specs never enter the applied-policy cache. Changed resource versions enqueue reconciliation again; equal-version resyncs are skipped. Dataplane failures continue through the existing rate-limited retry path. The Windows NPM Lite direct-rule implementation, Lite-specific retry/suppression behavior, and shared IsIPV4 helper 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: c180e456c3170d547458f06f961a6ff4968a54f2.

Check	Result
Standalone manual datapath matrix	67/67 passed on the current candidate, including both directions, non-canonical CIDR port restrictions/exclusions, external-peer denial, bounded expansion, stable terminal-rejection counts on all three nodes, correction/deletion recovery, and no NPM restarts or replacements.
Affected-package suites	Utility, translation, common/v2 controllers, diagnostics and policy packages passed locally.
Race detector	Changed controller, translation, utility and diagnostic paths passed; latest diagnostic follow-up also passed.
Build/static analysis	Linux/Windows NPM builds, Windows regression-binary compilation, and changed-line lint passed.
Repository-wide unit CI	Both Linux jobs and coverage passed. Native Windows v2 controllers and diagnostics passed; remaining NPM failures are the four inherited Lite-only assertions. The overall Windows job is not green.
NetworkPolicy conformance	45/45 passed on the current candidate, using v1.29.2 with NetworkPolicy focus and SCTP excluded.
Four-profile Cyclonus CI	112/112 passed in each of all four profiles on the current candidate, confirmed from the uploaded result logs.

Local 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

Full NPM v2 is the remediation scope on Linux and Windows. V1 and separately owned Lite behavior remain outside the fix scope.
Terminal translation handling is implemented, including policy-update recovery and preservation of transient dataplane retries. It does not install a fallback deny policy.
Merge, release and managed-cluster rollout are separate from code validation.
The remaining HTTP, telemetry and on-demand-configuration questions are isolated in fix: [NPM] runtime bounds and configuration follow-up #4858.

Issue Fixed:

The policy-translation portion of #4831.

Requirements:

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

…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

azure-pipelines · 2026-09-11T21:52:24Z

Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run.

Copilot

🟡 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 ErrUnsupportedIPAddress from directPeerAndPortAllowRule for IPv6/otherwise unrepresentable CIDRs, but this predicate does not classify that sentinel as a deliberate Windows limitation. Such a policy now goes through AddRateLimited forever 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, not NotExists; 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

redundance is not the noun needed here; use redundant or, more clearly, duplicate when 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: ipBlockRule is called on Windows v2 whenever npmLiteToggle is false (the direct-rule branch is only npmLiteToggle && 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.

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

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

TranslatePolicy failures 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 splitting ErrUnsupportedIPAddress into “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.

Copilot

🔵 Needs a closer look

Address the translation-failure retry behavior and preserve parent-branch grouping in debug analysis.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

npm/pkg/dataplane/debug/trafficanalyzer.go:321

pbRuleList appends every parent rule that jumps to the same child chain into this single sets slice (see converter.go:288-308), so these entries can represent OR alternatives, not one conjunction. For a policy with a multi-value namespace selector, the blue and red parent branches are both checked here; neither namespace can satisfy both positive sets, so the debug analyzer reports no matching rule even though the dataplane accepts one branch. Preserve the parent-branch grouping (AND within each branch, OR across branches) when performing this pre-check.

npm/pkg/controlplane/controllers/v2/networkPolicyController.go:191

This branch calls Forget for every errNetPolTranslationFailure, making full-NPM translation failures terminal and waiting only for a later policy event. That conflicts with the PR description, which says permanent translation failures still use the existing rate-limited retry path and terminal classification is a follow-up; an unchanged object will therefore never be retried as described. Either requeue this error (and update the queue tests) or correct the stated contract and scope.

			if errors.Is(err, errNetPolTranslationFailure) {
				c.workqueue.Forget(obj)
				return fmt.Errorf("error syncing '%s': %w; waiting for a policy change", key, err)

Files reviewed: 25/25 changed files
Comments generated: 0 new
Review effort level: Lite

Clone each parent-to-child path independently, retain custom rule metadata, report all V2 matched sets, and keep IPBlock/named-port handling outside selector-only prechecks. Preserve the existing v1 path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 175c523d-ad1c-4e31-986d-7c9940c6c18c

Copilot

🔵 Needs a closer look

Two unresolved moderate findings remain regarding translation retry handling and pre-expansion ACL budgeting for Windows NPM Lite.

Review details

Suppressed comments (2)

npm/pkg/controlplane/controllers/v2/networkPolicyController.go:191

This changes full-NPM translation failures from the described existing rate-limited retry path to Forget, so a newly created or updated policy that fails translation is attempted only once and then waits for a future informer change. That contradicts the PR description's stated scope (and can leave a transiently untranslatable policy unapplied); either retain AddRateLimited for these failures or update the contract and add the promised terminal/object-event handling in the same change.

			if errors.Is(err, errNetPolTranslationFailure) {
				c.workqueue.Forget(obj)
				return fmt.Errorf("error syncing '%s': %w; waiting for a policy change", key, err)

npm/pkg/controlplane/translation/translatePolicy.go:868

This explicitly excludes Windows NPM Lite from the work bound, but directPeerAndPortAllowRule appends one ACL per port and exceptDirectDropRules appends one per exception before TranslatePolicy reaches the final check. A Lite policy with a large port/Except list can therefore materialize an unbounded number of ACL objects and only then fail with ErrTooManyACLs, defeating the stated pre-expansion limit and allowing excessive node-memory use. Apply the same budget check before each direct/exception append instead of relying on the final check.

// maxACLsPerPolicy bounds generated ACL work on the full-NPM v2 ipset path.
// The Windows Lite direct-rule allocator is outside this work-budget guarantee;
// the final total check does not bound that allocator's intermediate work. Each ACL

Files reviewed: 27/27 changed files
Comments generated: 0 new
Review effort level: Lite

Preflight combined selector and port expansion before materializing full-NPM policies, including cumulative match/member work and existing ACL limits. Reject unsupported Windows namespace negations during translation and guard negative destination sets before ACL conversion. Preserve Lite allocation scope and v1 behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 175c523d-ad1c-4e31-986d-7c9940c6c18c

Copilot

🟡 Changes recommended

Unresolved moderate findings remain in cache interface compatibility and v2 diagnostic conjunction handling.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

npm/pkg/controlplane/controllers/common/cache.go:49

Adding GetNamespaceLabels to this exported interface is a source-breaking change for every implementation outside this repository, even though the new lookup is only needed by the debug consumer. Keep GenericCache stable and define the narrower interface in dataplane/debug (or otherwise adapt the concrete cache) so adding diagnostic behavior does not force unrelated cache implementations to change.

	GetNamespaceLabels(namespace string) (map[string]string, bool)

npm/pkg/controlplane/translation/translatePolicy.go:54

This sentinel is also returned by podSelectorWork and the named-port branch in policyWorkBudget.reserve, so the current message incorrectly blames a namespaceSelector when the rejected input may be a pod selector or ACL composition. Use a selector-neutral message and matching documentation so the controller error identifies the actual class of limit being enforced.

	// 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")

npm/pkg/controlplane/translation/work_budget.go:25

This rejection is for an oversized selected pod selector: the selector's matches plus the generated namespace anchor exceed the per-branch budget. The message says selected pods “require a namespace match,” which does not identify the offending count or limit and makes the controller error difficult to act on; report the selected-selector count and limit instead.

		return fmt.Errorf("selected pods require a namespace match: %w", ErrTooManySelectorMatches)

Files reviewed: 32/32 changed files
Comments generated: 1
Review effort level: Lite

Invalidate a v2 source or destination match when any set condition fails, while retaining v1 first-match behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Note

This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Note

This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.

Keep namespace-label lookup local to the diagnostic consumer and retain the original generic cache contract. Clarify selector-limit errors and include the generated namespace anchor in the reported count. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2fb3f092-d680-4706-bdf3-11219fa5190d

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2fb3f092-d680-4706-bdf3-11219fa5190d

Copilot

🟡 Changes recommended

Critical validation gaps remain for non-strict-subset CIDR exclusions and malformed pod-selector requirements.

Get a fresh assessment by requesting another Copilot review.

Review details

Files reviewed: 33/33 changed files
Comments generated: 2
Review effort level: Lite

Reject unsupported operators and empty pod-selector value lists before translation. Require canonical exclusions to be strict subsets of the canonical parent while preserving Windows Except handling. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2fb3f092-d680-4706-bdf3-11219fa5190d

Copilot

Note

Copilot was unable to run its full agentic suite in this review.

Pull request overview

Copilot reviewed 34 out of 34 changed files in this pull request and generated 3 comments.

+			if errors.Is(err, errNetPolTranslationFailure) {
+				c.workqueue.Forget(obj)
+				return fmt.Errorf("error syncing '%s': %w; waiting for a policy change", key, err)
+			}


+		}
+		return matches == setInfo.GetIncluded(), nil
+	}
 	srcNamespace := pod.Namespace


+	if !hasPositiveMatch {
+		matches++
+	}
+	if matches > maxSelectorMatches {
+		return 0, 0, fmt.Errorf("selector expands into %d matches, past the %d limit: %w",
+			matches, maxSelectorMatches, ErrTooManySelectorMatches)
+	}
+	if matches > maxTotalSelectorMatches/branches {
+		return 0, 0, fmt.Errorf("selector expands into %d branches of %d matches, past the %d total match limit: %w",
+			branches, matches, maxTotalSelectorMatches, ErrTooManySelectorMatches)
+	}
+	for _, requirement := range nsSelector.MatchExpressions {
+		if err := validateMatchExpression(requirement); err != nil {
+			return 0, 0, err
+		}
+	}


Isaiah Raya (rayaisaiah) and others added 18 commits September 11, 2026 20:29

chore: [NPM] address lint findings in the changed files

affea42

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Isaiah Raya (rayaisaiah) requested a review from a team as a code owner September 11, 2026 21:52

Isaiah Raya (rayaisaiah) requested review from Mathew Merrick (matmerr) and a lite review from Copilot September 11, 2026 21:52

Copilot started reviewing on behalf of Isaiah Raya (rayaisaiah) September 11, 2026 21:53 View session

Copilot AI reviewed Sep 11, 2026

View reviewed changes

Comment thread npm/pkg/controlplane/translation/parseSelector.go

Copilot AI review requested due to automatic review settings September 11, 2026 22:14

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
Copilot had to stop work due to a timeout.

Copilot AI reviewed Sep 11, 2026

View reviewed changes

Comment thread npm/pkg/controlplane/translation/translatePolicy.go Outdated

Comment thread npm/pkg/controlplane/translation/namespace_anchor_test.go Outdated

Comment thread npm/pkg/controlplane/translation/translatePolicy.go Outdated

Isaiah Raya (rayaisaiah) mentioned this pull request Sep 11, 2026

fix: [NPM] runtime bounds and configuration follow-up #4858

Closed

3 of 4 tasks

Copilot AI reviewed Sep 15, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings September 15, 2026 02:04

Copilot started reviewing on behalf of Isaiah Raya (rayaisaiah) September 15, 2026 02:04 View session

Copilot AI reviewed Sep 15, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings September 15, 2026 18:15

Copilot started reviewing on behalf of Isaiah Raya (rayaisaiah) September 15, 2026 18:23 View session

Copilot AI reviewed Sep 15, 2026

View reviewed changes

Comment thread npm/pkg/dataplane/debug/trafficanalyzer.go

Isaiah Raya (rayaisaiah) added the npm Related to NPM. label Sep 15, 2026

fix: [NPM] require all v2 diagnostic set conditions

5fd6161

Invalidate a v2 source or destination match when any set condition fails, while retaining v1 first-match behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings September 15, 2026 19:05

Copilot AI reviewed Sep 15, 2026

Isaiah Raya (rayaisaiah) requested a lite review from Copilot September 15, 2026 19:35

Copilot AI reviewed Sep 15, 2026

Copilot AI review requested due to automatic review settings September 15, 2026 19:42

Copilot started reviewing on behalf of Isaiah Raya (rayaisaiah) September 15, 2026 19:44 View session

test: [NPM] share namespace selector fixture labels

240697f

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2fb3f092-d680-4706-bdf3-11219fa5190d

Copilot AI reviewed Sep 15, 2026

View reviewed changes

Comment thread npm/pkg/controlplane/translation/translatePolicy.go Outdated

Comment thread npm/pkg/controlplane/translation/work_budget.go

Copilot AI review requested due to automatic review settings September 15, 2026 20:06

Copilot started reviewing on behalf of Isaiah Raya (rayaisaiah) September 15, 2026 20:07 View session

Copilot stopped reviewing on behalf of Isaiah Raya (rayaisaiah) due to an error September 15, 2026 20:27
Copilot had to stop work due to a timeout.

Copilot AI reviewed Sep 15, 2026

View reviewed changes

Isaiah Raya (rayaisaiah) closed this Sep 15, 2026

Navigation Menu

fix: [NPM] policy translation correctness and limits - #4859

fix: [NPM] policy translation correctness and limits#4859
Isaiah Raya (rayaisaiah) wants to merge 33 commits into
masterfrom
isaiahraya/npm-translation-hardening

Isaiah Raya (rayaisaiah) commented Sep 11, 2026 •

edited

Loading

Uh oh!

azure-pipelines Bot commented Sep 11, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

Isaiah Raya (rayaisaiah) commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Problems addressed and changes

1. Preserve multi-value NotIn conjunctions

2. Keep namespace selectors scoped to namespace membership

3. Bound generated work before expanding policies

4. Normalize CIDRs before classification and report translation failures

Validation

Scope and follow-ups

Uh oh!

azure-pipelines Bot commented Sep 11, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (4)

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Suppressed comments (2)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Suppressed comments (2)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (3)

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Isaiah Raya (rayaisaiah) commented Sep 11, 2026 •

edited

Loading

1. Preserve multi-value `NotIn` conjunctions