From 568e626b6ce4a9f57774c6db26cc1b10d574f27d Mon Sep 17 00:00:00 2001 From: Isaiah Raya Date: Fri, 11 Sep 2026 20:31:12 +0000 Subject: [PATCH 1/3] fix: [NPM] bound the NPM HTTP API and restrict its debug routes to the 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> --- npm/http/server/server.go | 117 +++++++++++++++++--- npm/http/server/server_test.go | 197 +++++++++++++++++++++++++++++++++ 2 files changed, 301 insertions(+), 13 deletions(-) diff --git a/npm/http/server/server.go b/npm/http/server/server.go index d20db191038..29088706235 100644 --- a/npm/http/server/server.go +++ b/npm/http/server/server.go @@ -1,26 +1,71 @@ package server import ( + "context" "encoding/json" + "errors" "fmt" + "net" "net/http" - "net/http/pprof" + // registers the pprof handlers on the default mux, which is mounted at the pprof + // prefix when profiling is enabled. _ "net/http/pprof" + "time" "github.com/Azure/azure-container-networking/log" npmconfig "github.com/Azure/azure-container-networking/npm/config" "github.com/Azure/azure-container-networking/npm/http/api" "github.com/Azure/azure-container-networking/npm/metrics" + "golang.org/x/net/netutil" "k8s.io/klog" "github.com/gorilla/mux" ) +const ( + // The NPM API listens on the host network of a privileged process, so any pod on the node + // can reach it. Without deadlines a client that opens connections and then reads its + // response one byte at a time keeps a request, and the response buffer built for it, alive + // indefinitely. These deadlines bound how long any single client can hold those resources. + // They are generous enough for a Prometheus scrape of this endpoint. + readHeaderTimeout = 10 * time.Second + readTimeout = 30 * time.Second + writeTimeout = 60 * time.Second + idleTimeout = 120 * time.Second + maxHeaderBytes = 1 << 16 // 64 KiB + + // maxConcurrentConns bounds how many connections the API serves at once. Each in-flight + // request to the cache handler buffers a full copy of the policy cache, so without a + // ceiling the number of concurrent clients alone decides how much memory NPM allocates. + maxConcurrentConns = 32 + + // maxConcurrentCacheRequests bounds how many cache encodings run at once. The encoding + // holds the cache lock and buffers the whole payload, so it is the most expensive thing + // the API does. One at a time keeps peak memory to a single copy of the cache; excess + // requests are shed rather than queued. + maxConcurrentCacheRequests = 1 +) + type NPMRestServer struct { listeningAddress string router *mux.Router } +// newServer builds the API server with the deadlines and bounds that keep a slow or unfinished +// request from holding resources indefinitely. It is a separate constructor so tests can assert +// the server that is actually served, rather than the constants it is built from. +func newServer(addr string, handler http.Handler) *http.Server { + return &http.Server{ + Handler: handler, + Addr: addr, + ReadHeaderTimeout: readHeaderTimeout, + ReadTimeout: readTimeout, + WriteTimeout: writeTimeout, + IdleTimeout: idleTimeout, + MaxHeaderBytes: maxHeaderBytes, + } +} + func NPMRestServerListenAndServe(config npmconfig.Config, npmEncoder json.Marshaler) { rs := NPMRestServer{} @@ -35,16 +80,17 @@ func NPMRestServerListenAndServe(config npmconfig.Config, npmEncoder json.Marsha // the nil check is for fan-out npm if config.Toggles.EnableHTTPDebugAPI && npmEncoder != nil { // ACN CLI debug handlers - rs.router.Handle(api.NPMMgrPath, rs.npmCacheHandler(npmEncoder)).Methods(http.MethodGet) + rs.router.Handle(api.NPMMgrPath, loopbackOnly(rs.npmCacheHandler(npmEncoder))).Methods(http.MethodGet) } if config.Toggles.EnablePprof { - rs.router.PathPrefix("/debug/").Handler(http.DefaultServeMux) - rs.router.HandleFunc("/debug/pprof/", pprof.Index) - rs.router.HandleFunc("/debug/pprof/cmdline", pprof.Cmdline) - rs.router.HandleFunc("/debug/pprof/profile", pprof.Profile) - rs.router.HandleFunc("/debug/pprof/symbol", pprof.Symbol) - rs.router.HandleFunc("/debug/pprof/trace", pprof.Trace) + // net/http/pprof registers every profile handler on the default mux under this + // prefix, including subpaths such as /debug/pprof/goroutine that naming the + // handlers individually used to miss. The prefix has no trailing slash so that + // /debug/pprof still reaches the mux, which redirects it to the index. Mounting at + // the pprof prefix rather than at /debug/ also keeps anything else later registered + // on the default mux from being served here. + rs.router.PathPrefix("/debug/pprof").Handler(loopbackOnly(http.DefaultServeMux)) } // use default listening address if none is specified @@ -52,20 +98,65 @@ func NPMRestServerListenAndServe(config npmconfig.Config, npmEncoder json.Marsha rs.listeningAddress = fmt.Sprintf("%s:%d", config.ListeningAddress, config.ListeningPort) } - srv := &http.Server{ - Handler: rs.router, - Addr: rs.listeningAddress, + srv := newServer(rs.listeningAddress, rs.router) + + 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 } klog.Infof("Starting NPM HTTP API on %s... ", rs.listeningAddress) - klog.Errorf("Failed to start NPM HTTP Server with error: %+v", srv.ListenAndServe()) + // A graceful close is not a failure, so it must not be reported as one. + if err := srv.Serve(netutil.LimitListener(listener, maxConcurrentConns)); err != nil && !errors.Is(err, http.ErrServerClosed) { + klog.Errorf("NPM HTTP Server stopped with error: %+v", err) + } +} + +// loopbackOnly serves a request only when it originated on the node itself. The debug route +// returns NPM's whole policy cache and the pprof routes expose the process, and both are +// served on the host network of a privileged process, so every pod on the node can otherwise +// reach them by reading its own node address. A pod has its own network namespace and cannot +// reach the node's loopback, while the on-node tooling that consumes these routes connects +// over localhost, so this keeps the routes available to their only caller and out of reach of +// a tenant workload. The Prometheus routes are deliberately not wrapped: they are scraped +// from off the node. +func loopbackOnly(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + host, _, err := net.SplitHostPort(r.RemoteAddr) + if err != nil { + http.Error(w, "forbidden", http.StatusForbidden) + return + } + + if ip := net.ParseIP(host); ip == nil || !ip.IsLoopback() { + http.Error(w, "forbidden", http.StatusForbidden) + return + } + + next.ServeHTTP(w, r) + }) } func (n *NPMRestServer) npmCacheHandler(npmCacheEncoder json.Marshaler) http.Handler { + // Admit only a few encodings at a time. Each one takes the cache lock and buffers the + // entire policy cache, so concurrent requests multiply both the lock hold time and the + // memory in flight. + inFlight := make(chan struct{}, maxConcurrentCacheRequests) + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + select { + case inFlight <- struct{}{}: + defer func() { <-inFlight }() + default: + http.Error(w, "too many concurrent cache requests", http.StatusServiceUnavailable) + return + } + b, err := json.Marshal(npmCacheEncoder) if err != nil { - http.Error(w, err.Error(), 500) + http.Error(w, err.Error(), http.StatusInternalServerError) return } _, err = w.Write(b) diff --git a/npm/http/server/server_test.go b/npm/http/server/server_test.go index 0cfe2333e63..64fb79d4725 100644 --- a/npm/http/server/server_test.go +++ b/npm/http/server/server_test.go @@ -5,12 +5,15 @@ import ( "io" "net/http" "net/http/httptest" + "sync" "testing" "github.com/Azure/azure-container-networking/npm" "github.com/Azure/azure-container-networking/npm/http/api" "github.com/Azure/azure-container-networking/npm/pkg/controlplane/controllers/common" + "github.com/gorilla/mux" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestGetNPMCacheHandler(t *testing.T) { @@ -55,3 +58,197 @@ func TestGetNPMCacheHandler(t *testing.T) { assert.Exactly(expected, actual) } + +// blockingMarshaler blocks inside MarshalJSON until released, so a test can hold cache +// encodings in flight and observe what happens to further requests. +type blockingMarshaler struct { + entered chan struct{} + release chan struct{} +} + +func (b *blockingMarshaler) MarshalJSON() ([]byte, error) { + b.entered <- struct{}{} + <-b.release + return []byte("{}"), nil +} + +// TestNPMCacheHandlerLimitsConcurrency verifies that the cache handler admits only a bounded +// number of encodings at once. Each encoding holds the cache lock and buffers the whole +// policy cache, so without a ceiling the number of concurrent clients alone decides how much +// memory NPM allocates and how long the cache stays locked. +func TestNPMCacheHandlerLimitsConcurrency(t *testing.T) { + encoder := &blockingMarshaler{ + entered: make(chan struct{}, maxConcurrentCacheRequests), + release: make(chan struct{}), + } + n := &NPMRestServer{} + handler := n.npmCacheHandler(encoder) + + // Fill every slot and wait until each request is actually inside MarshalJSON. + var wg sync.WaitGroup + for i := 0; i < maxConcurrentCacheRequests; i++ { + wg.Add(1) + go func() { + defer wg.Done() + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, api.NPMMgrPath, http.NoBody) + handler.ServeHTTP(httptest.NewRecorder(), req) + }() + } + for i := 0; i < maxConcurrentCacheRequests; i++ { + <-encoder.entered + } + + // With every slot busy, a further request must be shed instead of queueing another + // full copy of the cache. + rr := httptest.NewRecorder() + handler.ServeHTTP(rr, httptest.NewRequestWithContext(t.Context(), http.MethodGet, api.NPMMgrPath, http.NoBody)) + require.Equal(t, http.StatusServiceUnavailable, rr.Code, + "a request beyond the in-flight limit must be shed") + + close(encoder.release) + wg.Wait() + + // Once the in-flight requests drain, the handler must serve again. + rr = httptest.NewRecorder() + handler.ServeHTTP(rr, httptest.NewRequestWithContext(t.Context(), http.MethodGet, api.NPMMgrPath, http.NoBody)) + require.Equal(t, http.StatusOK, rr.Code, "the handler must recover once slots free up") +} + +// TestServerTimeoutsAreSet guards the deadlines and bounds on the server that is actually +// constructed. The API listens on the host network of a privileged process, so a client that +// never finishes a request must not be able to hold it, and the response buffered for it, open +// indefinitely. Asserting the constructed server rather than the constants means removing an +// assignment in newServer fails this test. +func TestServerTimeoutsAreSet(t *testing.T) { + srv := newServer("127.0.0.1:0", mux.NewRouter()) + + require.Equal(t, readHeaderTimeout, srv.ReadHeaderTimeout) + require.Equal(t, readTimeout, srv.ReadTimeout) + require.Equal(t, writeTimeout, srv.WriteTimeout) + require.Equal(t, idleTimeout, srv.IdleTimeout) + require.Equal(t, maxHeaderBytes, srv.MaxHeaderBytes) + + require.NotZero(t, srv.ReadHeaderTimeout) + require.NotZero(t, srv.ReadTimeout) + require.NotZero(t, srv.WriteTimeout) + require.NotZero(t, srv.IdleTimeout) + require.NotZero(t, srv.MaxHeaderBytes) + require.NotZero(t, maxConcurrentConns) + require.LessOrEqual(t, maxConcurrentCacheRequests, maxConcurrentConns, + "cache encodings must be bounded at or below the connection ceiling") +} + +// TestLoopbackOnly covers the guard on the debug and pprof routes. NPM runs on the host +// network of a privileged process, so before this guard any pod on the node could reach +// those routes through its own node address; a pod cannot reach the node's loopback, and +// the on-node tooling that consumes them connects over localhost. +func TestLoopbackOnly(t *testing.T) { + served := false + handler := loopbackOnly(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + served = true + w.WriteHeader(http.StatusOK) + })) + + tests := []struct { + name string + remoteAddr string + wantCode int + wantServed bool + }{ + {"IPv4 loopback", "127.0.0.1:54321", http.StatusOK, true}, + {"IPv4 loopback range", "127.9.9.9:54321", http.StatusOK, true}, + {"IPv6 loopback", "[::1]:54321", http.StatusOK, true}, + // the address a pod on the node would come from + {"pod address", "10.244.1.7:54321", http.StatusForbidden, false}, + // the node's own routable address, which a pod reads from the downward API + {"node address", "10.240.0.4:54321", http.StatusForbidden, false}, + {"malformed remote address", "not-an-address", http.StatusForbidden, false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + served = false + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, api.NPMMgrPath, http.NoBody) + req.RemoteAddr = tt.remoteAddr + + rr := httptest.NewRecorder() + handler.ServeHTTP(rr, req) + + require.Equal(t, tt.wantCode, rr.Code) + require.Equal(t, tt.wantServed, served, "whether the wrapped handler ran") + }) + } +} + +// TestLoopbackOnlyGuardsBeforeHandler makes sure a rejected request never reaches the cache +// encoder. The encoding is the expensive part of the route, so the guard has to run first. +func TestLoopbackOnlyGuardsBeforeHandler(t *testing.T) { + encoder := &blockingMarshaler{ + entered: make(chan struct{}, 1), + release: make(chan struct{}), + } + n := &NPMRestServer{} + handler := loopbackOnly(n.npmCacheHandler(encoder)) + + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, api.NPMMgrPath, http.NoBody) + req.RemoteAddr = "10.244.1.7:54321" + rr := httptest.NewRecorder() + handler.ServeHTTP(rr, req) + + require.Equal(t, http.StatusForbidden, rr.Code) + require.Empty(t, encoder.entered, "the cache must not be encoded for a rejected request") +} + +// Remote addresses used by the routing and guard cases below. +const ( + nodeLoopbackAddr = "127.0.0.1:1" + podAddr = "10.244.1.7:1" + // anyRedirect asks for a redirect of any code rather than a specific status. + anyRedirect = -1 +) + +// TestPprofRoutesAreMountedAtTheProfilePrefix covers the routing for the profiling handlers: +// every pprof subpath must be served, the routes must stay behind the loopback guard, and +// nothing else on the default mux may be reachable through /debug/. +func TestPprofRoutesAreMountedAtTheProfilePrefix(t *testing.T) { + http.DefaultServeMux.HandleFunc("/debug/unrelated", func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + }) + + router := mux.NewRouter() + router.PathPrefix("/debug/pprof").Handler(loopbackOnly(http.DefaultServeMux)) + + tests := []struct { + name string + path string + remoteAddr string + wantCode int + }{ + {"pprof index from the node", "/debug/pprof/", nodeLoopbackAddr, http.StatusOK}, + {"pprof cmdline from the node", "/debug/pprof/cmdline", nodeLoopbackAddr, http.StatusOK}, + // a subpath that naming each handler individually did not cover + {"pprof goroutine from the node", "/debug/pprof/goroutine", nodeLoopbackAddr, http.StatusOK}, + // without the trailing slash the mux redirects to the index rather than 404ing. + // The exact redirect code is the mux's choice, so only the class is asserted. + {"pprof index without a trailing slash", "/debug/pprof", nodeLoopbackAddr, anyRedirect}, + {"pprof index from a pod", "/debug/pprof/", podAddr, http.StatusForbidden}, + {"pprof goroutine from a pod", "/debug/pprof/goroutine", podAddr, http.StatusForbidden}, + // anything else on the default mux must not be reachable through this router + {"unrelated default mux route", "/debug/unrelated", nodeLoopbackAddr, http.StatusNotFound}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, tt.path, http.NoBody) + req.RemoteAddr = tt.remoteAddr + rr := httptest.NewRecorder() + router.ServeHTTP(rr, req) + if tt.wantCode == anyRedirect { + require.GreaterOrEqual(t, rr.Code, http.StatusMultipleChoices) + require.Less(t, rr.Code, http.StatusBadRequest) + return + } + require.Equal(t, tt.wantCode, rr.Code) + }) + } +} From 25e44d3cd52f2456a359edfe2e55528a7906e949 Mon Sep 17 00:00:00 2001 From: Isaiah Raya Date: Fri, 11 Sep 2026 20:31:12 +0000 Subject: [PATCH 2/3] fix: [NPM] stop enabling the debug routes by default and materialize 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> --- npm/azure-npm.yaml | 5 +++-- npm/config/config.go | 22 ++++++++++++++----- npm/deploy/kustomize/base/configmap.yaml | 4 ++-- .../manifests/common/npm-configmap.yaml | 4 ++-- .../manifests/controller/azure-npm.yaml | 4 ++-- npm/deploy/manifests/daemon/azure-npm.yaml | 4 ++-- npm/deploy/npm/azure-npm.yaml | 4 ++-- 7 files changed, 30 insertions(+), 17 deletions(-) diff --git a/npm/azure-npm.yaml b/npm/azure-npm.yaml index aa701ab4289..fe1d76de260 100644 --- a/npm/azure-npm.yaml +++ b/npm/azure-npm.yaml @@ -166,9 +166,10 @@ data: "MaxPendingNetPols": 100, "Toggles": { "EnablePrometheusMetrics": true, - "EnablePprof": true, - "EnableHTTPDebugAPI": true, + "EnablePprof": false, + "EnableHTTPDebugAPI": false, "EnableV2NPM": true, + "ApplyIPSetsOnNeed": true, "PlaceAzureChainFirst": false, "ApplyInBackground": true, "NetPolInBackground": true diff --git a/npm/config/config.go b/npm/config/config.go index c0a592c969f..929ef6339f6 100644 --- a/npm/config/config.go +++ b/npm/config/config.go @@ -42,11 +42,23 @@ var DefaultConfig = Config{ Toggles: Toggles{ EnablePrometheusMetrics: true, - EnablePprof: true, - EnableHTTPDebugAPI: true, - EnableV2NPM: true, - PlaceAzureChainFirst: util.PlaceAzureChainAfterKubeServices, - ApplyIPSetsOnNeed: false, + // The debug and profiling routes are served unauthenticated on the host network, so + // they are opt-in rather than on by default. This matters most when the config file is + // missing or unreadable, since that falls back to this struct: the fallback must not be + // the configuration that exposes them. + EnablePprof: false, + EnableHTTPDebugAPI: false, + EnableV2NPM: true, + PlaceAzureChainFirst: util.PlaceAzureChainAfterKubeServices, + // Materialize an ipset in the kernel only once a network policy references it. NPM + // creates two sets per distinct pod label and label count is attacker-controlled, so + // applying every set unconditionally lets one namespace push tens of thousands of sets + // into the kernel on every node, exhaust the agent, and stall policy programming + // cluster-wide. On-demand keeps incidental labels out of kernel state entirely. + // + // This does not weaken enforcement: the sets are still tracked and pods still join + // them, so a set is already populated by the time a policy references it. + ApplyIPSetsOnNeed: true, // ApplyInBackground is currently used in Windows to apply the following in background: IPSets and NetPols for new/updated Pods ApplyInBackground: true, // NetPolInBackground is currently used in Linux to apply NetPol controller Add events in the background diff --git a/npm/deploy/kustomize/base/configmap.yaml b/npm/deploy/kustomize/base/configmap.yaml index d9f549f2a37..3badcf6d54c 100644 --- a/npm/deploy/kustomize/base/configmap.yaml +++ b/npm/deploy/kustomize/base/configmap.yaml @@ -12,8 +12,8 @@ data: "ListeningAddress": "0.0.0.0", "Toggles": { "EnablePrometheusMetrics": true, - "EnablePprof": true, - "EnableHTTPDebugAPI": true, + "EnablePprof": false, + "EnableHTTPDebugAPI": false, "EnableV2NPM": false, "PlaceAzureChainFirst": false }, diff --git a/npm/deploy/manifests/common/npm-configmap.yaml b/npm/deploy/manifests/common/npm-configmap.yaml index 4d8bd0d3895..2d489dc37e7 100644 --- a/npm/deploy/manifests/common/npm-configmap.yaml +++ b/npm/deploy/manifests/common/npm-configmap.yaml @@ -12,8 +12,8 @@ data: "ListeningAddress": "0.0.0.0", "Toggles": { "EnablePrometheusMetrics": true, - "EnablePprof": true, - "EnableHTTPDebugAPI": true, + "EnablePprof": false, + "EnableHTTPDebugAPI": false, "EnableV2NPM": false, "PlaceAzureChainFirst": false }, diff --git a/npm/deploy/manifests/controller/azure-npm.yaml b/npm/deploy/manifests/controller/azure-npm.yaml index 9ff4d883746..311bc87932f 100644 --- a/npm/deploy/manifests/controller/azure-npm.yaml +++ b/npm/deploy/manifests/controller/azure-npm.yaml @@ -58,8 +58,8 @@ data: "ListeningAddress": "0.0.0.0", "Toggles": { "EnablePrometheusMetrics": true, - "EnablePprof": true, - "EnableHTTPDebugAPI": true, + "EnablePprof": false, + "EnableHTTPDebugAPI": false, "EnableV2NPM": false, "PlaceAzureChainFirst": false }, diff --git a/npm/deploy/manifests/daemon/azure-npm.yaml b/npm/deploy/manifests/daemon/azure-npm.yaml index 0e69605581b..3d4ee690c2c 100644 --- a/npm/deploy/manifests/daemon/azure-npm.yaml +++ b/npm/deploy/manifests/daemon/azure-npm.yaml @@ -58,8 +58,8 @@ data: "ListeningAddress": "0.0.0.0", "Toggles": { "EnablePrometheusMetrics": true, - "EnablePprof": true, - "EnableHTTPDebugAPI": true, + "EnablePprof": false, + "EnableHTTPDebugAPI": false, "EnableV2NPM": false, "PlaceAzureChainFirst": false }, diff --git a/npm/deploy/npm/azure-npm.yaml b/npm/deploy/npm/azure-npm.yaml index 3a833c2d943..2a267c2fd86 100644 --- a/npm/deploy/npm/azure-npm.yaml +++ b/npm/deploy/npm/azure-npm.yaml @@ -151,8 +151,8 @@ data: "ListeningAddress": "0.0.0.0", "Toggles": { "EnablePrometheusMetrics": true, - "EnablePprof": true, - "EnableHTTPDebugAPI": true, + "EnablePprof": false, + "EnableHTTPDebugAPI": false, "EnableV2NPM": false, "PlaceAzureChainFirst": false }, From 2643866c2bb75ddd94c466ab6ce7bf01c75cad01 Mon Sep 17 00:00:00 2001 From: Isaiah Raya Date: Fri, 11 Sep 2026 20:31:12 +0000 Subject: [PATCH 3/3] fix: [NPM] bound how many ipsets the inventory metric reports 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> --- npm/metrics/ipsets.go | 34 ++++++++++++++++++++ npm/metrics/ipsets_test.go | 52 +++++++++++++++++++++++++++++++ npm/metrics/prometheus-metrics.go | 1 + 3 files changed, 87 insertions(+) diff --git a/npm/metrics/ipsets.go b/npm/metrics/ipsets.go index a20b711b658..4dd52283811 100644 --- a/npm/metrics/ipsets.go +++ b/npm/metrics/ipsets.go @@ -7,6 +7,21 @@ import ( var ipsetInventoryMap map[string]int +// inventorySeries holds the set names that ipset_counts currently reports, so the number of +// series stays bounded and a set that is already reported keeps reporting. +var inventorySeries map[string]struct{} + +// maxIPSetInventorySeries bounds how many individual ipsets the ipset_counts metric reports. +// That series is labelled by set name, and NPM creates a set per distinct pod label, so its +// cardinality follows workload labels rather than anything an operator controls: one pod +// carrying tens of thousands of labels otherwise adds that many series on every node, which +// both retains them in the agent and inflates the response built for each scrape of an +// endpoint served on the host network. The aggregate counters stay exact past the bound and +// nothing NPM does reads the breakdown, so only the per-set detail stops growing; an operator +// can tell it is incomplete by comparing the reported series against num_ipsets. The bound is +// far above the number of sets a cluster's namespaces, policies and workloads produce. +const maxIPSetInventorySeries = 20000 + // AddPod increments the number of Pod IPs. func AddPod() { podsWatched.Inc() @@ -101,6 +116,7 @@ func ResetIPSetEntries() { removeFromIPSetInventory(setName) } ipsetInventoryMap = make(map[string]int) + inventorySeries = make(map[string]struct{}) } // GetNumIPSets returns the number of IPSets. @@ -131,12 +147,30 @@ func GetIPSetExecCount() (int, error) { } func updateIPSetInventory(setName string) { + if !canReportIPSetInventory(setName) { + return + } labels := getIPSetInventoryLabels(setName) val := getEntryCountForIPSet(setName) ipsetInventory.With(labels).Set(val) } +// canReportIPSetInventory reports whether setName may hold a per-set series, claiming a slot +// for it the first time. A set that already has a series keeps it, so an ipset's reported +// count does not flap once established. +func canReportIPSetInventory(setName string) bool { + if _, reported := inventorySeries[setName]; reported { + return true + } + if len(inventorySeries) >= maxIPSetInventorySeries { + return false + } + inventorySeries[setName] = struct{}{} + return true +} + func removeFromIPSetInventory(setName string) { + delete(inventorySeries, setName) labels := getIPSetInventoryLabels(setName) ipsetInventory.Delete(labels) } diff --git a/npm/metrics/ipsets_test.go b/npm/metrics/ipsets_test.go index 99a20305a2a..a057aff1649 100644 --- a/npm/metrics/ipsets_test.go +++ b/npm/metrics/ipsets_test.go @@ -1,6 +1,7 @@ package metrics import ( + "fmt" "testing" "github.com/Azure/azure-container-networking/npm/metrics/promutil" @@ -204,3 +205,54 @@ func TestResetIPSetEntries(t *testing.T) { assertNumEntriesAndCounts(t, &testSet{testName1, 0}, &testSet{testName2, 0}) assertMapIsGood(t) } + +// TestIPSetInventorySeriesAreBounded covers the cardinality bound on ipset_counts. NPM makes +// a set per distinct pod label, so without a bound a single pod's labels decide how many +// series every node holds and how large the metrics response is. The aggregate counters must +// stay exact regardless, since they are what NPM and its operators actually count on. +func TestIPSetInventorySeriesAreBounded(t *testing.T) { + ResetIPSetEntries() + defer ResetIPSetEntries() + + const over = maxIPSetInventorySeries + 500 + for i := 0; i < over; i++ { + AddEntryToIPSet(fmt.Sprintf("podlabel-key%d:v%d", i, i)) + } + + require.Len(t, inventorySeries, maxIPSetInventorySeries, + "the number of reported series must stop at the bound") + + // the aggregate is unaffected by the bound + entries, err := GetNumIPSetEntries() + promutil.NotifyIfErrors(t, err) + require.Equal(t, over, entries, "the total entry count must still be exact") + + // a set that got a series still reports its own count + first, err := GetNumEntriesForIPSet("podlabel-key0:v0") + promutil.NotifyIfErrors(t, err) + require.Equal(t, 1, first) + + // removing a reported set frees its slot for a new one + RemoveAllEntriesFromIPSet("podlabel-key0:v0") + require.Len(t, inventorySeries, maxIPSetInventorySeries-1) + AddEntryToIPSet("podlabel-fresh:v") + require.Contains(t, inventorySeries, "podlabel-fresh:v") +} + +// TestIPSetInventoryUnboundedBelowLimit guards against the bound changing behaviour for a +// cluster that stays under it, which is every real one. +func TestIPSetInventoryUnboundedBelowLimit(t *testing.T) { + ResetIPSetEntries() + defer ResetIPSetEntries() + + for i := 0; i < 500; i++ { + AddEntryToIPSet(fmt.Sprintf("podlabel-key%d:v%d", i, i)) + } + + require.Len(t, inventorySeries, 500) + for i := 0; i < 500; i++ { + count, err := GetNumEntriesForIPSet(fmt.Sprintf("podlabel-key%d:v%d", i, i)) + promutil.NotifyIfErrors(t, err) + require.Equal(t, 1, count, "every set under the bound reports its own count") + } +} diff --git a/npm/metrics/prometheus-metrics.go b/npm/metrics/prometheus-metrics.go index 4dcc07653f6..59d2f5f8101 100644 --- a/npm/metrics/prometheus-metrics.go +++ b/npm/metrics/prometheus-metrics.go @@ -374,6 +374,7 @@ func initializeDaemonMetrics() { numIPSetEntries = createClusterGauge(numIPSetEntriesName, numIPSetEntriesHelp) ipsetInventory = createClusterGaugeVec(ipsetInventoryName, ipsetInventoryHelp, ipsetInventoryLabels) ipsetInventoryMap = make(map[string]int) + inventorySeries = make(map[string]struct{}) // NODE METRICS addACLRuleExecTime = createNodeSummary(addACLRuleExecTimeName, addACLRuleExecTimeHelp)