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 }, 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) + }) + } +} 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)