fix: [NPM] policy translation and resource-bound hardening

[Isaiah Raya (rayaisaiah)]

Summary

Six independent correctness and robustness fixes in the NPM v2 Linux policy path. Each is
self-contained and has its own commit, so they can be reviewed one at a time. Nothing here
changes how a well-formed, ordinary NetworkPolicy is programmed.

This targets master. Three related naming-uniqueness changes (#4574, #4587, #4593) were
forward-ported to master earlier in #4611; this is the follow-on set.

Changes

Listed in commit order.

1. Compile multi-value namespaceSelector NotIn as a single conjunction

key NotIn [a, b] is one requirement meaning key != a AND key != b. The selector compiler
flattened it the way it flattens multi-value In, emitting one selector per value. Each becomes
an independent allow decision, and allow decisions are additive (OR), so a namespace carrying one
of the values was still matched by the decision negating a different value.

flattenNameSpaceSelector now handles the operators separately: In still fans out into one
selector per value, NotIn keeps every value as its own single-value requirement inside the same
selector, and a selector mixing the two carries each NotIn exclusion conjunctively into every
In branch. Unsupported operators and empty In/NotIn value lists now fail closed rather than
being dropped, which would widen the selector.

2. Scope negation-only namespaceSelector matches to cluster namespaces

A namespaceSelector selects pods in namespaces, and NPM's namespace sets hold pod IPs. A
negative requirement (NotIn / DoesNotExist) renders as a negated set match, which is satisfied
by every address absent from that set — including addresses that are not pods at all. When the
selector produced no positive set, the negations alone were the whole match, so the rule admitted
non-cluster peers: on egress a selected pod could reach arbitrary external hosts, and on a
flat-network cluster a routable non-pod host could reach a protected pod.

parseNSSelector now intersects with the all-namespaces set when the parsed selectors contain no
positive match, mirroring allowAllInternal. Selectors already carrying a positive requirement
(matchLabels, In, Exists) are unchanged.

3. Bound namespaceSelector flattening, and the rules a policy generates

Flattening multi-value In requirements produced the Cartesian product with no ceiling; 19
two-value requirements in one small valid policy expand to 2^19 selectors, each deep-copied and
turned into its own IPSet and ACL.

Two bounds, because the selector count alone is not the cost:

• maxFlattenedNSSelectors, computed before any allocation. The check divides rather than
  multiplies so it cannot overflow, and runs per requirement so one very wide requirement is
  caught on the first iteration.
• maxACLsPerPolicy. Every flattened branch is emitted once per port, summed across peers and
  rules, so 512 selectors against 512 ports is 262144 ACLs — comfortably under the selector cap.
  It is checked before a peer expands, before each of that peer's ports, and before each port of a
  rule that lists ports and no peers, so those paths never materialize more than the ceiling. It is
  checked once more before the policy is returned. That final check is not conditioned on the
  datapath, so an oversized policy is refused however it was built; what stays out of scope is
  instrumenting the Windows/NPM Lite direct-rule path with the early checks, so a policy built
  there is refused at the end rather than partway through.
• maxSelectorMatches, counted before anything is allocated. A multi-value NotIn is compiled by
  change 1 as one conjunction, so it stays a single selector producing a single rule and is
  invisible to both bounds above, yet each value still becomes its own IPSet and its own condition
  on that rule. Counting the matches a selector expands into catches that: NotIn contributes one
  per value, In one per branch, and Exists/DoesNotExist one each. It is counted before the
  shortcut that returns a selector with no match expressions, so a selector made only of
  matchLabels is bounded too.

4. Canonicalize ipBlock CIDRs, and stop reporting a failed translation as success

Two compounding problems. An ipBlock CIDR naming the all-addresses block with host bits set
(10.0.0.0/0) was rejected before canonicalization, even though it denotes exactly 0.0.0.0/0.
That failed translation of the whole policy — and the v2 controller converted the failure into a
successful no-op, so neither the peer rule nor the policy's default drop was installed and nothing
signalled it.

• New additive util.NormalizeCIDR clears host bits. The Linux ipBlock path validates through
  it and emits the canonical form, and canonicalizes its except CIDRs through a helper used only
  by that path, so they de-duplicate and are recognized by the all-addresses split.
• The controller returns the translation error instead of reporting success, so it is recorded and
  the key is requeued. Deliberate datapath limitations (the Windows unsupported features, NPM
  Lite's CIDR-only peers) cannot resolve on retry and stay suppressed with a warning as before.

IsIPV4 itself is deliberately left byte-identical, because it is shared with the Windows and NPM
Lite direct-rule paths which are out of scope here.

5. Bound the NPM HTTP API and stop enabling its debug routes by default

The API listens on the host network of a privileged process, and its server was created with only
an address and a handler: no read, write or idle deadline, no header bound, no limit on concurrent
connections. The cache route additionally serializes the entire policy cache into memory per
request while holding the cache lock, so slow clients could hold an unbounded number of full cache
copies alive.

The server now has deadlines, a header bound, a connection ceiling, and admits a single cache
encoding at a time with the rest shed as 503. This server-side bound is the load-bearing part of
the change: it holds regardless of whether the debug route is enabled, which is the configuration
that matters for managed clusters. The debug and pprof routes are also no longer enabled by
default in the deployment manifests. Prometheus metrics are unchanged.

The routes are also no longer reachable from a pod. They are served on the host network of a
privileged process, so any pod on the node could reach them through the node address it reads from
the downward API. Both are now served only to requests that originate on the node itself: a pod has
its own network namespace and cannot reach the node's loopback, while the on-node tooling that
consumes them already connects over localhost. A refused request is answered before the cache is
encoded. The Prometheus routes are deliberately not restricted, because they are scraped from off
the node.

The profiling handlers are also mounted at the pprof prefix rather than at /debug/. The router
matched a /debug/ prefix ahead of the individually named pprof routes, so those named routes
never ran and every handler on the default mux was reachable under /debug/. Mounting the default
mux at the pprof prefix serves the profiles, including the subpaths the named list missed such as
/debug/pprof/goroutine, and exposes nothing else later registered on that mux.

6. Materialize ipsets in the kernel only when a policy references them

NPM creates two ipsets per distinct pod label, on every node, and applied every set to the kernel
unconditionally. Kubernetes places no limit on how many labels a pod may carry, so 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.

ApplyIPSetsOnNeed now defaults to true, so a set reaches the kernel only once a network policy
references it. Incidental pod labels never become kernel state at all.

This deliberately does not use a creation limit. Three limit designs were implemented and rejected
after live testing, each failing the same way: refusing to create a label set always creates an
escape, because either the policy cannot install or it installs referencing an empty set and the
pod escapes the policy that selects it. A per-pod cap additionally does not bound anything, since
the same labels spread across more pods reproduce the blowup. On-demand materialization refuses
nothing — sets are still tracked and pods still join them, so a set is already populated by the
time a policy references it, and only kernel materialization is deferred.

Deferring kernel materialization does not on its own bound what those labels cost in the agent.
The sets are still tracked, which is what keeps enforcement correct, and ipset_counts carries the
set name as a label, so the number of series it reported followed workload labels rather than
anything an operator controls. 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 only reported detail is limited; an operator can
tell it is incomplete by comparing the reported series against num_ipsets. The shipped scrape
configuration drops npm_ipset_counts by default, so this affects only operators who opted into
that advanced metric.

Measured with one pod carrying 34,000 labels, the size named in the report: agent memory for those
labels drops from 88.8 MB to 14.6 MB against a 300 MiB container limit, and the metrics response
from 6.42 MB to 1.87 MB, which no longer varies with the labels a workload chooses.

Tests

Unit tests accompany each change, covering both the corrected behaviour and the unchanged side
(positive selectors untouched, at-limit inputs passing, invalid CIDRs still rejected,
unsupported-feature errors still suppressed, ordinary multi-peer multi-port policies well inside
the ACL budget).

The HTTP changes are covered by tests for the loopback guard (IPv4 and IPv6 loopback admitted, a
pod address and the node address refused, a malformed remote address refused, and a refused request
never reaching the cache encoder) and for the pprof mount (every profile subpath served, the routes
still behind the guard, and nothing else on the default mux reachable). The metric bound is covered
by tests that the series stop at the bound while the aggregate counters stay exact, that a freed
slot is reusable, and that behaviour below the bound is unchanged.

go build ./npm/... and go test ./npm/... pass. The only failures in the tree are in
npm/pkg/controlplane/controllers/v1 and npm/cmd, which fail identically on unmodified master
(the latter shells out to iptables and needs root).

golangci-lint was run over every changed package against an unmodified master worktree for
comparison, with no new finding.

Validation on live AKS clusters

Each change was exercised individually against the behaviour it fixes, on Linux AKS with Azure CNI
and --network-policy azure.

Change	Live check	Result
1	both excluded namespaces denied, non-excluded allowed	pass
2	egress to external addresses denied, in-cluster allowed; on a flat-network cluster a VM at a non-pod VNet address reached a protected pod on master and was blocked on this branch	pass
3	2^19-selector policy and a 512-selector x 512-port policy both rejected with a clear logged error, agents unaffected; a NotIn list past the match bound is rejected while one exactly on the bound still translates	pass
4	non-canonical /0 policy installs and its default drop is enforced	pass
5	debug and pprof routes not served by default while Prometheus routes still return 200; with the routes explicitly enabled, 80 concurrent slow-read requests caused no restart and 78 of 80 were shed as 503; with the routes enabled, a pod on the node is refused with 403 on both and reads no cache contents, while on-node loopback still serves them and Prometheus stays reachable off the node	pass
6	kernel ipsets unchanged under a many-label pod with no agent restarts; a policy selecting that pod by an incidental label still applies to it; a referenced set is materialized on demand; a default-deny in an unrelated namespace still takes effect; ipset_counts stays within its bound. Contrasted on one cluster, the same pod produces 0 kernel sets on this branch against 4,043 with ApplyIPSetsOnNeed off	pass

Regression:

• A 22-check datapath matrix covering all six changes plus ordinary policy features (podSelector +
  port, cross-namespace denial, multi-value In fan-out) passes 22/22, including while the
  oversized policies from change 3 and a many-label pod from change 6 were live. The same matrix
  scores 9/22 on unmodified master, and the 13 failures are exactly the behaviours this PR
  corrects. An 8-check matrix for the reachability and cardinality bounds passes 8/8.
• Upstream Kubernetes NetworkPolicy conformance (e2e.test matched to the cluster version,
  --ginkgo.focus=NetworkPolicy --ginkgo.skip=SCTP): 45/45 passed, run against both the
  previous ApplyAll configuration and the new on-demand default.
• Cyclonus (test/cyclonus/test-cyclonus.sh): 112/112 passed.

Known limitations

These are stated so a reviewer does not have to discover them, and so the ICMs are not closed on
a stronger claim than the evidence supports.

• Change 6 still leaves a per-label cost in the agent. At the reported 34,000 labels the agent
  holds 14.6 MB for them, down from 88.8 MB, because each label still allocates a userspace IPSet
  so that a set is already populated when a policy references it. Holding unreferenced label
  membership in a compact index is the remaining follow-up. A creation cap is not — three variants
  were tried and each produced an enforcement escape, either blocking a policy from installing or
  leaving it referencing an empty set.
• The ipset_counts bound is first-come. Slots are claimed as sets appear, so a workload that
  floods labels before a policy is created can occupy them and leave later, legitimate sets without
  a per-set series. The aggregate counters are unaffected. Following the existing
  TODO kernel-based prometheus metrics markers, so that the breakdown tracks kernel
  materialization instead, would bound it by policy-referenced sets rather than by arrival order;
  that changes what current dashboards report and is left for a change that can be signed off on
  its telemetry impact. For the same reason ipsetInventoryMap is left unbounded: it gates the
  aggregate entry-count decrement, and bounding it without care makes that counter drift.
• Change 4 retries terminal failures indefinitely. Errors like ErrUnsupportedIPAddress and
  the selector validation errors are permanent functions of the policy object, but they now go
  through AddRateLimited and recur on every node. Classifying them as terminal (log once,
  Forget, emit an Event on the object) is the follow-up.
• Change 5's connection ceiling is a global pool. The debug routes no longer contribute, since
  a pod is refused immediately, but the Prometheus routes still share the ceiling and a pod can
  still hold connections against it. A shorter idle timeout, a per-source limit, or a separate
  listener for metrics would address it.
• The Prometheus routes remain unauthenticated on the configured address. They have to be
  reachable from off the node to be scraped, so they cannot be restricted to loopback the way the
  debug routes are. Their response is now bounded rather than scalable by workload labels, but
  authenticating them needs a token review and RBAC and is out of scope here.

Deployment notes for reviewers

• Change 6 needs a configuration change to take effect on managed clusters. NPM does not merge
  its config file with DefaultConfig — cmd/root.go notes "there is no config merging with
  default, if config is loaded, options must be set", and start.go unmarshals into a zero-value
  Config. Any cluster that mounts a configmap therefore takes Go zero values for absent keys, so
  ApplyIPSetsOnNeed must be set explicitly in the deployed configmap, not only in
  DefaultConfig. This PR sets it in npm/azure-npm.yaml; the AKS-managed configmap is generated
  elsewhere and needs the same key.
• For the same reason, the DefaultConfig changes in change 5 only cover the case where no config
  file loads at all. The server-side bounds, not the toggles, are what protect a cluster whose
  configmap enables the debug route.

Scope

Linux NPM v2. util.IsIPV4, deDuplicateExcept, exceptDirectDropRules,
directPeerAndPortAllowRule and the dataplane ipset member check are byte-identical to master,
so the Windows and NPM Lite specific paths are unchanged, and no NPM v1 file is touched. Note that
npm/pkg/controlplane/translation/ is a single compiler shared by v2 on both operating systems,
so changes 1-4 necessarily run on Windows v2 as well; no Windows-specific code path was added or
altered.

[azure-pipelines]

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

[Copilot]

🟡 Changes recommended

The current error/telemetry logging behavior can produce duplicated and potentially spammy error signals (especially for repeated translation failures and over-limit pod labels), and should be tightened before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Robustness and correctness hardening for Azure NPM v2 (Linux) policy translation and related resource-bound controls, aiming to preserve behavior for well-formed NetworkPolicies while closing edge-case allow-bypasses, silent translation failures, and potential resource-exhaustion paths.

Changes:

• Fix namespaceSelector semantics (NotIn conjunction, negation-only scoping to namespaces) and bound selector flattening expansion.
• Canonicalize ipBlock CIDRs and ensure translation failures are surfaced/requeued rather than being reported as successful no-ops.
• Add resource bounds for per-pod label ipset creation and harden the NPM HTTP API server (timeouts, header limit, concurrency limits), plus disable debug/pprof routes by default in manifests.

File summaries

File	Description
npm/util/util.go	Adds NormalizeCIDR and updates IPv4/CIDR validation to accept non-canonical spellings safely.
npm/util/util_test.go	Adds unit coverage for IsIPV4 and CIDR canonicalization behavior.
npm/pkg/dataplane/ipsets/ipsetmanager.go	Requires CIDR ipset members to be canonical before programming kernel sets.
npm/pkg/controlplane/translation/translatePolicy.go	Canonicalizes ipBlock CIDRs/Excepts and adds new translation errors for selector handling limits.
npm/pkg/controlplane/translation/translatePolicy_test.go	Adds regressions for NotIn semantics, negation-only scoping, and non-canonical all-addresses CIDRs.
npm/pkg/controlplane/translation/parseSelector.go	Fixes NotIn flattening semantics, adds expansion limit, and scopes negation-only namespace selectors.
npm/pkg/controlplane/translation/parseSelector_test.go	Adds unit tests for NotIn conjunction, unsupported operators, empty values, and expansion limit propagation.
npm/pkg/controlplane/controllers/v2/podController.go	Caps per-pod label processing to bound label-derived ipset creation.
npm/pkg/controlplane/controllers/v2/podController_test.go	Tests label cap behavior and cache/dataplane agreement.
npm/pkg/controlplane/controllers/v2/networkPolicyController.go	Surfaces translation failures (requeue) while keeping deliberate unsupported-feature suppression.
npm/pkg/controlplane/controllers/v2/networkPolicyController_test.go	Adds tests ensuring non-canonical CIDR policies apply and translation failures are surfaced.
npm/http/server/server.go	Adds HTTP server deadlines/header limit, connection ceiling, and cache-handler concurrency shedding.
npm/http/server/server_test.go	Adds tests for cache-handler concurrency limit and guards server timeout constants.
npm/deploy/npm/azure-npm.yaml	Disables pprof/debug API by default in deployment config.
npm/deploy/manifests/daemon/azure-npm.yaml	Disables pprof/debug API by default in daemon manifest config.
npm/deploy/manifests/controller/azure-npm.yaml	Disables pprof/debug API by default in controller manifest config.
npm/deploy/manifests/common/npm-configmap.yaml	Disables pprof/debug API by default in common configmap.
npm/deploy/kustomize/base/configmap.yaml	Disables pprof/debug API by default in kustomize base configmap.
npm/azure-npm.yaml	Disables pprof/debug API by default in top-level manifest.

Review details

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

---

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🔵 Needs a closer look

It makes several security- and correctness-sensitive changes across policy translation and host-network API behavior that warrant final human review despite strong test coverage.

Review details

• Files reviewed: 18/18 changed files
• Comments generated: 4
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The new ACL budget guard is intended as a hard resource bound, but the current checkACLBudget condition allows exceeding the declared limit by one ACL (and tests codify that), undermining the strictness/early-stop goal of the bound.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

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

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

• ErrEmptyMatchExpressionValues' text uses inconsistent casing ("notIn") and refers to operators by name but not in a consistent, log-friendly way. Since this string will surface in logs/events, prefer a fully-lowercase, consistently formatted message (Go convention is to start error strings with lowercase).

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

[Copilot]

🔵 Needs a closer look

It changes core policy translation/enforcement semantics and introduces multiple new resource-bounding behaviors, so a final human review is warranted despite strong unit test coverage.

Review details

Suppressed comments (1)

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

npm/http/server/server.go:90

• The PathPrefix("/debug/") route is registered before the specific /debug/pprof/* routes, so it will match first and the explicit pprof handlers below become unreachable in gorilla/mux. This is confusing to maintain (it looks like those handlers matter, but they never run). Register the more specific pprof routes first, then the /debug/ prefix fallback.

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

[Copilot]

🔵 Needs a closer look

It changes core NetworkPolicy translation semantics and host-network API hardening, which are security-sensitive and warrant final human review despite strong test coverage.

Review details

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

[Copilot]

🔵 Needs a closer look

One added unit test mutates the global http.DefaultServeMux without isolation, which can make the test suite order-dependent/flaky and should be made hermetic before approval.

Review details

Suppressed comments (2)

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

npm/http/server/server_test.go:218

• This test mutates the global http.DefaultServeMux via HandleFunc, which can leak routes into other tests in this package (and makes the test order-dependent). Prefer using a fresh *http.ServeMux instance for the test and mounting that, so the test stays hermetic.
  npm/pkg/controlplane/translation/translatePolicy.go:856
• checkACLBudget’s docstring claims the per-append checks ensure translation paths “never take the policy past the ceiling”, but ingressPolicy/egressPolicy still append the default drop ACL without a budget check. That means a policy can hit the ceiling during allow-rule emission and only exceed it when adding the mandatory drop, then fail only at the end via checkACLTotal. Consider either updating the comment, or adding a budget check immediately before appending the default drop so the translator fails at the right spot.

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

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate findings remain in selector expansion, ACL budgeting, IPSet metrics, and HTTP lifecycle handling.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (4)

npm/metrics/ipsets.go:166

• This implementation deliberately stops registering ipset_counts series after 20,000, but the PR's known-limitations text says every label still registers a metric series and that deferring per-set metric registration is follow-up work. Please reconcile the stated limitation and operator expectations with this new incomplete-metrics behavior.

	if len(inventorySeries) >= maxIPSetInventorySeries {
		return false

npm/metrics/ipsets.go:151

• Once inventorySeries reaches the cap, updateIPSetInventory skips this set, but GetNumEntriesForIPSet still calls getVecValue, whose GaugeVec.With creates a series on lookup. Querying any unreported live set therefore bypasses the cap and creates a series that inventorySeries does not track, so the cardinality bound is not enforced for this exported accessor. Return the userspace count for names that are not in inventorySeries (or use a non-creating lookup) instead.

func updateIPSetInventory(setName string) {
	if !canReportIPSetInventory(setName) {
		return

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

• Because this is only a post-build backstop, a policy with many empty Ingress/Egress rules still enters translateRule's zero-peer/zero-port branch, appends one ACL per rule, and is rejected only after the over-limit slice has been materialized. That leaves a valid Kubernetes object able to bypass the new allocation bound; check the budget before that append (or count this path up front) so oversized policies stop at the ceiling.

func checkACLTotal(npmNetPol *policies.NPMNetworkPolicy) error {
	if len(npmNetPol.ACLs) > maxACLsPerPolicy {
		return tooManyACLs(npmNetPol)

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

• This comment incorrectly says the ipset path is Linux-only. translateRule uses ipBlockRule for Windows v2 whenever NPM Lite is disabled, so the new CIDR normalization also affects that path; only the NPM Lite direct-rule path is unchanged. Please describe the actual ipset/direct-rule boundary so future platform changes are not based on the wrong scope.

	// 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.

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

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate findings remain in selector expansion, ACL budgeting, CIDR exception handling, and HTTP server lifecycle.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

npm/http/server/server.go:107

• The new listener is created with context.Background() even though NPMRestServerListenAndServe is launched as a long-lived goroutine from the startup path. That context can never be cancelled, and this function has no caller-owned shutdown path for the listener/server, so a shutdown cannot stop or gracefully close the HTTP service. Thread a lifecycle context into this function and shut the server down from its cancellation.

	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

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

• This check only counts ACLs emitted so far, but ingressPolicy and egressPolicy unconditionally append a default-drop ACL after translateRule returns. A port-only rule with exactly maxACLsPerPolicy ports therefore emits 2000 allow ACLs and then a 2001st drop before checkACLTotal rejects the policy. Reserve the mandatory drop or check that append as part of the bound so the early paths cannot exceed the stated ceiling.

// 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.

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

• Canonicalizing an exception can turn a non-canonical /0 such as 10.0.0.1/0 into 0.0.0.0/0. The /0 parent is later split into two /1 members, but splitCIDRSet has no entry for the full /0, so this value is emitted as 0.0.0.0/0 nomatch; the surrounding code explicitly notes that ipset cannot add the /0 block, so restoring the translated set fails instead of representing the empty result. Handle an exception that canonicalizes to the full parent before it reaches the member-building loop.

		canonical, ok := util.NormalizeCIDR(except)
		if !ok {
			return nil, fmt.Errorf("except %q: %w", except, ErrUnsupportedIPAddress)
		}
		if _, exist := exceptsSet[canonical]; !exist {

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

[Copilot]

🔵 Needs a closer look

Four moderate findings remain unresolved in selector bounds, ACL ceiling enforcement, CIDR exception preservation, and metric reporting.

Review details

Suppressed comments (4)

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

npm/metrics/ipsets.go:167

• Once 20,000 names have claimed a slot, this silently skips ipsetInventory.With even though ipsetInventoryMap and num_ipsets continue tracking the live set. GetNumEntriesForIPSet therefore returns zero and ipset_counts omits real sets, so dashboards or callers lose per-set data; this also contradicts the PR description's claim that Prometheus metrics are unchanged. If this cap is required, expose an explicit dropped-series signal and update the metric contract/consumers, or preserve the existing per-set behavior.

npm/pkg/controlplane/translation/parseSelector.go:102

• This check does not bound the total generated matches. A selector with 991 MatchLabels plus nine two-value In requirements passes with matches == 1000 but produces 512 branches, each with about 1000 set matches; the translation loop appends each branch's IP sets before the ACL check, so roughly 512,000 translated IPSet objects plus 512,000 SetInfo entries can be materialized under both advertised limits. It also undercounts the implicit all-namespaces anchor added by parseNSSelector for negation-only selectors, so 1000 NotIn values passes here and produces 1001 matches. Bound the aggregate branch×match count and include generated anchors before nameSpaceSelector allocation.

	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 {

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

• The final check runs after the required default-drop ACL is appended. If rule translation has already produced exactly maxACLsPerPolicy allow ACLs, the drop is materialized as ACL 2001 and only then rejected, so the hard ceiling is exceeded and an oversized policy still allocates one extra ACL. Reserve a slot for the required drop or guard those append sites before adding it.

func checkACLTotal(npmNetPol *policies.NPMNetworkPolicy) error {
	if len(npmNetPol.ACLs) > maxACLsPerPolicy {
		return tooManyACLs(npmNetPol)

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

• Canonicalizing exceptions can turn an ordinary entry followed by split-half entries into the special shrink path, but the existing packing loop indexes ordinary entries by the original position and then truncates members. For input [200.0.0.0/8, 250.0.0.0/1, 10.0.0.0/1], the latter two normalize to the split halves and the first exception is overwritten/lost, contrary to the new TestIPBlockExceptCanonicalizationKeepsEveryExcept expectation. Build the output without positional shrinking (or compact it after applying split replacements) so every canonical exception is retained.

	deDupExcepts, err := canonicalizeExcepts(ipBlockRule.Except)
	if err != nil {
		return nil, err

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

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate findings affect fail-closed policy behavior and HTTP lifecycle/access controls.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

npm/http/server/server.go:104

• This new listener is created with context.Background(), while all NPM start paths launch NPMRestServerListenAndServe as a detached goroutine and retain no server handle for shutdown. That leaves the HTTP listener and active connections disconnected from the process lifecycle during coordinated stop/restart. Pass a caller-owned cancellation signal into this function and shut down the server/listener from that lifecycle instead of creating a root context here.

	srv := newServer(rs.listeningAddress, rs.router)

	var lc net.ListenConfig
	listener, err := lc.Listen(context.Background(), "tcp", rs.listeningAddress)

npm/http/server/server.go:135

• IsLoopback only proves that the peer used the node's loopback address; it does not distinguish the node process from a hostNetwork: true pod, which shares the node network namespace and can connect to 127.0.0.1. Such a pod can therefore still reach the policy cache and pprof routes despite the stated pod isolation. Use a Unix-socket/permission boundary or an explicit authentication/authorization mechanism if these endpoints must be node-process-only.

		if ip := net.ParseIP(host); ip == nil || !ip.IsLoopback() {
			http.Error(w, "forbidden", http.StatusForbidden)
			return

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

• The pre-append guard reserves two drop ACLs on every translation path (len(ACLs) >= maxACLsPerPolicy-2), even when the policy has only one directional default drop left. A one-direction Linux policy with 1,999 allow ACLs plus its one drop is exactly 2,000 ACLs, but it is rejected while it has only 1,998 ACLs; consequently the claimed exact-bound behavior is never reachable through the guarded path (the boundary test only produces 1,999). Track the number of drops still pending, or otherwise make the reservation reflect the policy's actual directions.

	if len(npmNetPol.ACLs) >= maxACLsPerPolicy-reservedDropACLs {
		return tooManyACLs(npmNetPol)

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