fix: [NPM] runtime bounds and configuration follow-up - #4858
Draft
Isaiah Raya (rayaisaiah) wants to merge 3 commits into
Draft
fix: [NPM] runtime bounds and configuration follow-up#4858Isaiah Raya (rayaisaiah) wants to merge 3 commits into
Isaiah Raya (rayaisaiah) wants to merge 3 commits into
Conversation
…e node 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. A graceful close is no longer reported as a failure. The debug and profiling routes are also served only to requests that originate on the node itself. Any pod on the node could otherwise reach them through the node address it reads from the downward API, and the debug route returns the whole policy cache. A pod has its own network namespace and cannot reach the node's loopback, while the tooling that consumes these routes already connects over localhost, so its only caller is unaffected and a refused request is answered before the cache is encoded. The Prometheus routes are deliberately left reachable, because they are scraped from off the node. The profiling handlers move to the pprof prefix rather than /debug/. The router matched a /debug/ prefix ahead of the individually named pprof routes, so those 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, and exposes nothing else registered on that mux. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ipsets on demand Two default changes, both of which only take effect where the deployed configuration carries them. The debug and profiling routes are no longer on by default. They are served unauthenticated on the host network, so they are opt-in rather than the fallback when a config file is missing or unreadable. ApplyIPSetsOnNeed now defaults to true, so an ipset reaches the kernel only once a network policy references it. NPM creates two ipsets per distinct pod label on every node and applied every set unconditionally, and 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. Sets are still tracked and pods still join them, so a set is already populated by the time a policy references it; only kernel materialization is deferred, and enforcement is unchanged. Note that NPM does not merge its config file with DefaultConfig, so a cluster that mounts a configmap takes Go zero values for absent keys. Both keys have to be set in the deployed configmap, not only here. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Deferring kernel materialization does not on its own bound what a pod's 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. One pod carrying tens of thousands of labels added that many series on every node, which both retained them in the agent and inflated the response built for each scrape of an endpoint served on the host network. 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. Measured with one pod carrying 34,000 labels: 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. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
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
1 participant
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
The two runtime changes extracted from #4831. The four policy-translation fixes are reviewed and validated independently in #4859.
This PR remains draft because HTTP access/lifecycle, metric semantics and managed-configuration decisions are not settled. It does not claim complete runtime resource bounding.
Problems addressed and proposed changes
1. Bound HTTP work and make debug/profiling access explicit
Problem: The HTTP server previously had no explicit read/write/idle deadlines, header bound or connection ceiling. The cache endpoint constructs a complete serialized cache response per request, so concurrent slow requests can retain multiple responses and compete with normal serving work.
Debug and profiling routes also shared the configured listener with metrics. Their exposure and lifecycle need to be intentional rather than incidental to enabling the server.
Proposed change:
/debug/subtree of the default mux.Prometheus routes remain reachable off-node for scraping. Loopback filtering is an access boundary, not authentication: host-network processes remain inside it. The connection pool is still shared, and the existing startup paths do not yet provide graceful HTTP shutdown.
2. Materialize IPSets on demand and bound optional inventory detail
Problem: NPM tracks label-derived sets on each node. Applying every tracked set to the kernel makes kernel work grow with label inventory even when no policy references those sets. Tracking every set as a separate metric series adds another source of retained state and scrape work.
Proposed change:
ApplyIPSetsOnNeedoption by default. Only policy-referenced sets need kernel materialization; membership tracking is retained so later references remain correct.ApplyIPSetsOnNeedalready exists; this PR changes its default rather than introducing a new property. A loaded configuration is not merged withDefaultConfig, so configuration generated elsewhere must explicitly include the intended value.The metric limit is not a total heap or response-byte bound. Per-set detail is omitted beyond the cap, while the aggregate inventory map and userspace label bookkeeping still grow with input.
Decisions needed before leaving draft
ApplyIPSetsOnNeedactivation in the managed configuration generator and its rollout. Changing the compiled default alone does not update an existing mounted ConfigMap.Tests and evidence scope
The branch contains tests for HTTP limits, loopback filtering, profile routing, and the inventory cap/accounting. Earlier combined-branch runs are historical evidence only, not a standalone certification of this draft.
The full standalone validation in #4859 covers the translation branch, not these runtime changes. Runtime acceptance testing should be finalized after the decisions above, using the actual managed configuration.
Issue Fixed:
The runtime portion of #4831, pending the decisions above.
Requirements: