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) diff --git a/npm/pkg/controlplane/controllers/v2/networkPolicyController.go b/npm/pkg/controlplane/controllers/v2/networkPolicyController.go index 31ed83b5067..84aac9bb9b2 100644 --- a/npm/pkg/controlplane/controllers/v2/networkPolicyController.go +++ b/npm/pkg/controlplane/controllers/v2/networkPolicyController.go @@ -291,8 +291,8 @@ func (c *NetworkPolicyController) syncAddAndUpdateNetPol(netPolObj *networkingv1 // install translated rules into kernel npmNetPolObj, err := translation.TranslatePolicy(netPolObj, c.npmLiteToggle) if err != nil { - if isUnsupportedWindowsTranslationErr(err) { - klog.Warningf("NetworkPolicy %s in namespace %s is not translated because it has unsupported translated features of Windows: %s", + if isUnsupportedTranslationErr(err) { + klog.Warningf("NetworkPolicy %s in namespace %s is not translated because it uses a feature this datapath does not support: %s", netPolObj.ObjectMeta.Name, netPolObj.ObjectMeta.Namespace, err.Error()) // We can safely suppress unsupported network policy because re-Queuing will result in same error. @@ -300,9 +300,19 @@ func (c *NetworkPolicyController) syncAddAndUpdateNetPol(netPolObj *networkingv1 return metrics.NoOp, nil } - klog.Errorf("Failed to translate podSelector in NetworkPolicy %s in namespace %s: %s", netPolObj.ObjectMeta.Name, netPolObj.ObjectMeta.Namespace, err.Error()) - // The exec time isn't relevant here, so consider a no-op. Returning nil to prevent re-queuing since this is not a transient error. - return metrics.NoOp, nil + // Do not report success here. Reporting success left the policy's selected pods with + // no rules at all - not even the default drop the policy implies - while the policy + // object appeared to be applied and nothing signalled the failure. Return the error so + // it is surfaced and the key is requeued (rate limited) instead. + // + // The error is deliberately not logged or counted here: processNextWorkItem already + // runs the returned error through utilruntime.HandleError and SendErrorLogAndMetric, + // so recording it here as well would emit the same failure three times. The wrapped + // message names the policy so that single record stays specific. + // + // The exec time isn't relevant here, so consider a no-op. + return metrics.NoOp, fmt.Errorf("translating network policy %s/%s: %w", + netPolObj.Namespace, netPolObj.Name, err) } _, policyExisted := c.rawNpSpecMap[netpolKey] @@ -358,3 +368,14 @@ func isUnsupportedWindowsTranslationErr(err error) bool { errors.Is(err, translation.ErrUnsupportedSCTP) || errors.Is(err, translation.ErrUnsupportedExceptCIDR) } + +// isUnsupportedTranslationErr reports whether err is a deliberate limitation of the datapath +// or mode NPM is running in, rather than a policy NPM failed to translate. Those limitations +// cannot resolve on retry, so they stay suppressed with a warning. Every other translation +// failure is surfaced and requeued, because reporting success would leave the policy's +// selected pods with no rules while nothing signalled that the policy was never applied. +func isUnsupportedTranslationErr(err error) bool { + return isUnsupportedWindowsTranslationErr(err) || + // NPM Lite only supports CIDR peers; a label-selector peer is out of scope there. + errors.Is(err, translation.ErrUnsupportedNonCIDR) +} diff --git a/npm/pkg/controlplane/controllers/v2/networkPolicyController_test.go b/npm/pkg/controlplane/controllers/v2/networkPolicyController_test.go index d14f6f67f12..29aa44f8cbd 100644 --- a/npm/pkg/controlplane/controllers/v2/networkPolicyController_test.go +++ b/npm/pkg/controlplane/controllers/v2/networkPolicyController_test.go @@ -9,6 +9,7 @@ import ( "github.com/Azure/azure-container-networking/npm/metrics" "github.com/Azure/azure-container-networking/npm/metrics/promutil" + "github.com/Azure/azure-container-networking/npm/pkg/controlplane/translation" "github.com/Azure/azure-container-networking/npm/pkg/dataplane" dpmocks "github.com/Azure/azure-container-networking/npm/pkg/dataplane/mocks" "github.com/Azure/azure-container-networking/npm/util" @@ -618,3 +619,101 @@ func TestLabelUpdateNetworkPolicy(t *testing.T) { checkNetPolTestResult("TestUpdateNetPol", f, testCases) } + +// netPolWithCIDR builds an ingress NetworkPolicy that selects all pods in its namespace and +// admits the given ipBlock CIDR. +func netPolWithCIDR(cidr string) *networkingv1.NetworkPolicy { + return &networkingv1.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: "allow-cidr", Namespace: "test-nwpolicy"}, + Spec: networkingv1.NetworkPolicySpec{ + PodSelector: metav1.LabelSelector{}, + PolicyTypes: []networkingv1.PolicyType{networkingv1.PolicyTypeIngress}, + Ingress: []networkingv1.NetworkPolicyIngressRule{ + {From: []networkingv1.NetworkPolicyPeer{{IPBlock: &networkingv1.IPBlock{CIDR: cidr}}}}, + }, + }, + } +} + +// TestAddNetworkPolicyNonCanonicalCIDRIsApplied verifies that a policy naming the +// all-addresses block with host bits set is programmed into the dataplane. It used to fail +// translation, and the controller turned that failure into a successful no-op, so the +// policy's selected pods were left with no rules at all. +func TestAddNetworkPolicyNonCanonicalCIDRIsApplied(t *testing.T) { + netPolObj := netPolWithCIDR("10.0.0.0/0") + + f := newNetPolFixture(t) + f.netPolLister = append(f.netPolLister, netPolObj) + f.kubeobjects = append(f.kubeobjects, netPolObj) + stopCh := make(chan struct{}) + defer close(stopCh) + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + dp := dpmocks.NewMockGenericDataplane(ctrl) + f.newNetPolController(stopCh, dp, false) + + // The policy must reach the dataplane instead of being dropped during translation. + dp.EXPECT().UpdatePolicy(gomock.Any()).Times(1) + + addNetPol(f, netPolObj) + checkNetPolTestResult("TestAddNetworkPolicyNonCanonicalCIDRIsApplied", f, []expectedNetPolValues{ + {1, 0, netPolPromVals{1, 1, 0, 0}}, + }) +} + +// TestSyncAddAndUpdateNetPolSurfacesTranslationFailure verifies that a policy NPM cannot +// translate is reported as an error rather than as a successful no-op. Reporting success +// left the policy's selected pods with no rules while nothing signalled that the policy had +// never been applied. +func TestSyncAddAndUpdateNetPolSurfacesTranslationFailure(t *testing.T) { + // An IPv6 ipBlock cannot be expressed by the IPv4 datapath, so translation fails. + netPolObj := netPolWithCIDR("2001:db8::/32") + + f := newNetPolFixture(t) + f.netPolLister = append(f.netPolLister, netPolObj) + f.kubeobjects = append(f.kubeobjects, netPolObj) + stopCh := make(chan struct{}) + defer close(stopCh) + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + dp := dpmocks.NewMockGenericDataplane(ctrl) + f.newNetPolController(stopCh, dp, false) + + // Nothing may be programmed for a policy that failed to translate. + dp.EXPECT().UpdatePolicy(gomock.Any()).Times(0) + + _, err := f.netPolController.syncAddAndUpdateNetPol(netPolObj) + require.Error(t, err, "a translation failure must be surfaced, not reported as success") + require.ErrorIs(t, err, translation.ErrUnsupportedIPAddress) + + // The policy must not be recorded as applied, so a later retry still reconciles it. + netpolKey, keyErr := cache.MetaNamespaceKeyFunc(netPolObj) + require.NoError(t, keyErr) + require.NotContains(t, f.netPolController.rawNpSpecMap, netpolKey) +} + +// TestSyncAddAndUpdateNetPolSuppressesUnsupportedFeature verifies that a deliberate datapath +// limitation stays suppressed. Those cannot resolve on retry, so requeuing them forever +// would be pure churn. +func TestSyncAddAndUpdateNetPolSuppressesUnsupportedFeature(t *testing.T) { + // NPM Lite only supports CIDR peers, so a label-selector peer is out of scope there. + netPolObj := createNetPol() + + f := newNetPolFixture(t) + f.netPolLister = append(f.netPolLister, netPolObj) + f.kubeobjects = append(f.kubeobjects, netPolObj) + stopCh := make(chan struct{}) + defer close(stopCh) + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + dp := dpmocks.NewMockGenericDataplane(ctrl) + f.newNetPolController(stopCh, dp, true) + + dp.EXPECT().UpdatePolicy(gomock.Any()).Times(0) + + _, err := f.netPolController.syncAddAndUpdateNetPol(netPolObj) + require.NoError(t, err, "an unsupported-feature limitation must stay suppressed") +} diff --git a/npm/pkg/controlplane/translation/parseSelector.go b/npm/pkg/controlplane/translation/parseSelector.go index 447d1283058..0e67c1e6a3e 100644 --- a/npm/pkg/controlplane/translation/parseSelector.go +++ b/npm/pkg/controlplane/translation/parseSelector.go @@ -2,10 +2,8 @@ package translation import ( "fmt" - "regexp" - "github.com/Azure/azure-container-networking/log" "github.com/Azure/azure-container-networking/npm/pkg/dataplane/ipsets" "github.com/Azure/azure-container-networking/npm/util" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -16,38 +14,73 @@ import ( // an alphanumeric character (e.g. 'MyValue', or 'my_value', or '12345', regex used for validation is '(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?' var validLabelRegex = regexp.MustCompile("(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?") +// maxTotalSelectorMatches bounds the set matches a namespaceSelector produces across every +// branch it fans out into. A multi-value In repeats the whole selector once per value, so the +// cost is the branch count multiplied by the matches in each branch; bounding either factor on +// its own leaves a wide selector repeated across many branches unbounded, and the translator +// materializes an IPSet and a SetInfo for each before the per-policy rule budget is consulted. +const maxTotalSelectorMatches = 10000 + +// maxSelectorMatches bounds how many set matches a single namespaceSelector may expand into. +// Each match becomes its own IPSet and its own condition on the rule the selector produces, and +// a multi-value NotIn contributes one per value while staying in a single selector, so it is +// counted by neither maxFlattenedNSSelectors nor the per-policy rule budget. The bound is the +// same ceiling used for the selector count, and is far above any workable selector. +const maxSelectorMatches = maxFlattenedNSSelectors + +// maxFlattenedNSSelectors caps how many labelSelectors a single namespaceSelector may be +// flattened into. Flattening multi-value In requirements produces the Cartesian product of +// their values, and each resulting selector is deep-copied and later turned into its own +// IPSet and ACL, so the cost grows exponentially with the number of such requirements. The +// cap is far above any workable policy (a selector fanning out this wide would already be +// unusable as iptables rules) while keeping a crafted selector from exhausting memory. +const maxFlattenedNSSelectors = 1000 + // flattenNameSpaceSelector will help flatten multiple nameSpace selector match Expressions values // into multiple label selectors helping with the OR condition. func flattenNameSpaceSelector(nsSelector *metav1.LabelSelector) ([]metav1.LabelSelector, error) { /* - This function helps to create multiple labelSelectors when given a single multivalue nsSelector - Take below example: this nsSelector has 2 values in a matchSelector. + This function helps to create multiple labelSelectors when given a single multivalue nsSelector. + + The two multi-value operators are handled differently because they carry different semantics: + + In: a multi-value In is a disjunction (OR) over its values, so it is fanned out into one + labelSelector per value. Take below example with 2 values in a matchExpression: - namespaceSelector: matchExpressions: - key: ns - operator: NotIn + operator: In values: - netpol-x - netpol-y - goal is to convert this single nsSelector into multiple nsSelectors to preserve OR condition - between multiple values of the matchExpr i.e. this function will return + becomes - namespaceSelector: matchExpressions: - key: ns - operator: NotIn + operator: In values: - netpol-x - namespaceSelector: matchExpressions: - key: ns - operator: NotIn + operator: In values: - netpol-y - then, translate policy will replicate each of these nsSelectors to add two different rules in iptables, - resulting in OR condition between the values. + then, translate policy will replicate each of these nsSelectors to add two different rules, + resulting in the OR condition between the values. + + NotIn: a multi-value NotIn is a single set-membership conjunction, i.e. + ns NotIn [x, y] means (ns != x AND ns != y). It must NOT be fanned out into separate + selectors, because each generated selector becomes an independent allow rule and allow + rules are additive (OR): a namespace carrying one excluded value would still match the + rule negating the other value and be admitted. Instead, every value is kept as its own + single-value NotIn requirement within the same selector, so all negated conditions land + in a single decision (AND) and the default drop stays effective for every excluded value. + When a selector mixes In and NotIn, each NotIn exclusion is carried conjunctively into + every In branch. Check TestFlattenNameSpaceSelector 2nd subcase for complex scenario */ @@ -58,6 +91,60 @@ func flattenNameSpaceSelector(nsSelector *metav1.LabelSelector) ([]metav1.LabelS return []metav1.LabelSelector{}, nil } + // Bound how many matches this selector produces, before anything is allocated and before + // the matchLabels-only shortcut below, since those labels each become a match too. A + // multi-value NotIn stays inside a single selector, so it is invisible to both the + // selector-count bound further down and the per-policy rule budget, yet every one of its + // values becomes its own IPSet and its own condition on one rule. + matches := len(nsSelector.MatchLabels) + branches := 1 + hasPositiveMatch := len(nsSelector.MatchLabels) > 0 + for _, req := range nsSelector.MatchExpressions { + switch req.Operator { + case metav1.LabelSelectorOpNotIn: + // each excluded value is carried as its own negated match + matches += len(req.Values) + case metav1.LabelSelectorOpIn: + // one match per branch, and a multi-value In fans out into branches + matches++ + hasPositiveMatch = true + if len(req.Values) > 1 { + // the branch count is bounded on its own terms first, so a selector that + // fans out too far still reports that rather than the total below. + // Divide rather than multiply so the product cannot overflow. + if len(req.Values) > maxFlattenedNSSelectors/branches { + return nil, fmt.Errorf("key %q with %d values expands past the %d selector limit: %w", + req.Key, len(req.Values), maxFlattenedNSSelectors, ErrTooManyFlattenedSelectors) + } + branches *= len(req.Values) + } + case metav1.LabelSelectorOpExists: + matches++ + hasPositiveMatch = true + case metav1.LabelSelectorOpDoesNotExist: + matches++ + default: + // an unknown operator, which the loop below rejects + matches++ + } + } + if !hasPositiveMatch { + // parseNSSelector anchors a selector that matches only negatively with the + // all-namespaces set, so that match counts too + matches++ + } + if matches > maxSelectorMatches { + return nil, fmt.Errorf("selector expands into %d matches, past the %d limit: %w", + matches, maxSelectorMatches, ErrTooManySelectorMatches) + } + // Each branch repeats every match, so the cost is the product rather than either factor. + // The branch count alone is bounded further down and the rule count by the policy budget, + // but neither sees a wide selector repeated across many branches. + if matches > maxTotalSelectorMatches/branches { + return nil, fmt.Errorf("selector expands into %d branches of %d matches, past the %d total match limit: %w", + branches, matches, maxTotalSelectorMatches, ErrTooManySelectorMatches) + } + if len(nsSelector.MatchExpressions) == 0 { return []metav1.LabelSelector{*nsSelector}, nil } @@ -70,14 +157,20 @@ func flattenNameSpaceSelector(nsSelector *metav1.LabelSelector) ([]metav1.LabelS } multiValuePresent := false + // notInExpanded records whether a multi-value NotIn was rewritten into several + // single-value NotIn requirements on baseSelector. When it is, baseSelector no + // longer equals the input, so the original selector must not be returned as-is. + notInExpanded := false multiValueMatchExprs := []metav1.LabelSelectorRequirement{} for _, req := range nsSelector.MatchExpressions { - // Only In and NotIn operators of matchExprs have multiple values - // NPM will ignore single value matchExprs of these operators. - // for multiple values, it will create a slice of them to be used for Zipping with baseSelector - // to create multiple nsSelectors to preserve OR condition across all labels and expressions + // In/NotIn requirements carry the values; single-value requirements are added to + // baseSelector as-is, while multi-value requirements are handled per operator below. + // Exists/DoesNotExist carry no values and are added to baseSelector directly. switch { - case (req.Operator == metav1.LabelSelectorOpIn) || (req.Operator == metav1.LabelSelectorOpNotIn): + case req.Operator == metav1.LabelSelectorOpIn: + if len(req.Values) == 0 { + return nil, ErrEmptyMatchExpressionValues + } for _, v := range req.Values { if !isValidLabelValue(v) { return nil, ErrInvalidMatchExpressionValues @@ -88,28 +181,87 @@ func flattenNameSpaceSelector(nsSelector *metav1.LabelSelector) ([]metav1.LabelS // for length 1, add the matchExpr to baseSelector baseSelector.MatchExpressions = append(baseSelector.MatchExpressions, req) } else { + // multi-value In is a disjunction: zip it with baseSelector to + // create one nsSelector per value and preserve the OR condition. multiValuePresent = true multiValueMatchExprs = append(multiValueMatchExprs, req) } + case req.Operator == metav1.LabelSelectorOpNotIn: + if len(req.Values) == 0 { + return nil, ErrEmptyMatchExpressionValues + } + for _, v := range req.Values { + if !isValidLabelValue(v) { + return nil, ErrInvalidMatchExpressionValues + } + } + + if len(req.Values) == 1 { + // for length 1, add the matchExpr to baseSelector + baseSelector.MatchExpressions = append(baseSelector.MatchExpressions, req) + } else { + // A multi-value NotIn is a single set-membership conjunction + // (key NotIn [a, b] == key != a AND key != b), NOT a disjunction. + // Fanning it out into separate selectors would emit independent + // additive allow rules and let each excluded value be admitted by + // the rule negating another value. Keep every value as its own + // single-value NotIn within the same selector so all negations + // stay in one decision (AND). + notInExpanded = true + for _, v := range req.Values { + baseSelector.MatchExpressions = append( + baseSelector.MatchExpressions, + metav1.LabelSelectorRequirement{ + Key: req.Key, + Operator: metav1.LabelSelectorOpNotIn, + Values: []string{v}, + }, + ) + } + } case (req.Operator == metav1.LabelSelectorOpExists) || (req.Operator == metav1.LabelSelectorOpDoesNotExist): // since Exists and NotExists do not contain any values, NPM can safely add them to the baseSelector baseSelector.MatchExpressions = append(baseSelector.MatchExpressions, req) default: - log.Errorf("Invalid operator [%s] for selector [%v] requirement", req.Operator, *nsSelector) + // Fail closed: an unknown operator must not silently drop the requirement + // and widen the selector. Kubernetes only admits In/NotIn/Exists/DoesNotExist. + // The operator and key identify the requirement without copying the whole + // selector into the message, which a hostile selector could make enormous. + return nil, fmt.Errorf("operator %q on key %q: %w", + req.Operator, req.Key, ErrUnsupportedMatchExpressionOperator) } } - // If there are no multiValue NS selector match expressions - // return the original NsSelector + // If there are no multiValue In match expressions to fan out, the baseSelector + // (which already carries any conjunctive NotIn expansions) is the only selector. if !multiValuePresent { - return []metav1.LabelSelector{*nsSelector}, nil + if !notInExpanded { + // Nothing was rewritten; return the original selector unchanged so callers + // that compare against the input see an identical selector. + return []metav1.LabelSelector{*nsSelector}, nil + } + return []metav1.LabelSelector{*baseSelector.DeepCopy()}, nil } // Now use the baseSelector and loop over multiValueMatchExprs to create all - // combinations of values - flatNsSelectors := []metav1.LabelSelector{ - *baseSelector.DeepCopy(), + // combinations of values. The number of combinations is the product of the value + // counts, so it grows exponentially with the number of multi-value In requirements + // (19 two-value requirements already yield 2^19 selectors). Bound the product before + // doing any allocation: every selector below is deep-copied and later becomes its own + // IPSet and ACL, so an unbounded product exhausts memory on every node running NPM. + combinations := 1 + for _, req := range multiValueMatchExprs { + if len(req.Values) > maxFlattenedNSSelectors/combinations { + // Summarize rather than print the selector: the message must stay bounded + // precisely because the selector that triggers it need not be. + return nil, fmt.Errorf("key %q with %d values expands past the %d selector limit: %w", + req.Key, len(req.Values), maxFlattenedNSSelectors, ErrTooManyFlattenedSelectors) + } + combinations *= len(req.Values) } + + flatNsSelectors := make([]metav1.LabelSelector, 0, combinations) + flatNsSelectors = append(flatNsSelectors, *baseSelector.DeepCopy()) for _, req := range multiValueMatchExprs { flatNsSelectors = zipMatchExprs(flatNsSelectors, req) } @@ -198,6 +350,17 @@ func (ps *parsedSelectors) addSelector(include bool, setType ipsets.SetType, set ps.labelSet[setNameWithOp] = struct{}{} } +// hasPositiveSelector reports whether any parsed selector is a positive (non-negated) match. +// Without one, the parsed selectors match purely by negation and constrain nothing. +func (ps *parsedSelectors) hasPositiveSelector() bool { + for _, ls := range ps.labelSelectors { + if ls.include { + return true + } + } + return false +} + // parseNSSelector parses namespaceSelector and returns slice of labelSelector object // which includes operator, setType, ipset name and always nil members slice. // Member slices is always nil since parseNSSelector function is called @@ -239,6 +402,17 @@ func parseNSSelector(selector *metav1.LabelSelector) []labelSelector { parsedSelectors.addSelector(noNegativeOp, setType, setName) } + // #4. A namespaceSelector only ever selects namespaces, so every match it produces + // must be a cluster address. A negative requirement (NotIn / DoesNotExist) renders as + // a negated set match, which is satisfied by every address that is not in that set, + // including addresses outside the cluster. When the selector produces no positive set + // to intersect with, the negations alone are the whole match and the rule would also + // admit non-cluster (e.g. internet) peers. Intersect with the all-namespaces set so + // the match stays scoped to namespaces, mirroring allowAllInternal. + if !parsedSelectors.hasPositiveSelector() { + parsedSelectors.addSelector(true, ipsets.KeyLabelOfNamespace, util.KubeAllNamespacesFlag) + } + return parsedSelectors.labelSelectors } diff --git a/npm/pkg/controlplane/translation/parseSelector_test.go b/npm/pkg/controlplane/translation/parseSelector_test.go index e93f99500ae..e1b03045d87 100644 --- a/npm/pkg/controlplane/translation/parseSelector_test.go +++ b/npm/pkg/controlplane/translation/parseSelector_test.go @@ -5,8 +5,11 @@ import ( "reflect" "testing" + "github.com/Azure/azure-container-networking/npm/pkg/dataplane/policies" "github.com/stretchr/testify/require" + networkingv1 "k8s.io/api/networking/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/intstr" ) func TestFlattenNameSpaceSelectorCases(t *testing.T) { @@ -599,6 +602,209 @@ func TestFlattenNamespaceSelectorError(t *testing.T) { } } +// TestFlattenNameSpaceSelectorMultiValueNotIn verifies that a multi-value NotIn +// requirement is preserved as a single conjunction rather than fanned out into +// separate selectors. Separate selectors would become independent additive allow +// rules, so a namespace carrying one excluded value could still match the rule +// negating a different value. +func TestFlattenNameSpaceSelectorMultiValueNotIn(t *testing.T) { + selector := &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: tenantLabelKey, + Operator: metav1.LabelSelectorOpNotIn, + Values: []string{"x", "y"}, + }, + }, + } + + testSelectors, err := flattenNameSpaceSelector(selector) + require.NoError(t, err) + + expected := []metav1.LabelSelector{ + { + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: tenantLabelKey, + Operator: metav1.LabelSelectorOpNotIn, + Values: []string{"x"}, + }, + { + Key: tenantLabelKey, + Operator: metav1.LabelSelectorOpNotIn, + Values: []string{"y"}, + }, + }, + }, + } + + require.Equal(t, expected, testSelectors) +} + +// TestFlattenNameSpaceSelectorMixedInAndNotIn verifies that multi-value In values +// fan out into disjunctive branches while every multi-value NotIn exclusion is +// carried conjunctively into each branch. +func TestFlattenNameSpaceSelectorMixedInAndNotIn(t *testing.T) { + selector := &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: tenantLabelKey, + Operator: metav1.LabelSelectorOpNotIn, + Values: []string{"x", "y"}, + }, + { + Key: "role", + Operator: metav1.LabelSelectorOpIn, + Values: []string{"a", "b"}, + }, + }, + } + + testSelectors, err := flattenNameSpaceSelector(selector) + require.NoError(t, err) + + // Two In branches, each carrying both NotIn exclusions conjunctively. + require.Len(t, testSelectors, 2) + for _, s := range testSelectors { + var notInValues []string + var inValues []string + for _, req := range s.MatchExpressions { + require.Len(t, req.Values, 1, "every requirement must be single-value after flatten") + switch req.Operator { + case metav1.LabelSelectorOpNotIn: + require.Equal(t, tenantLabelKey, req.Key) + notInValues = append(notInValues, req.Values[0]) + case metav1.LabelSelectorOpIn: + require.Equal(t, "role", req.Key) + inValues = append(inValues, req.Values[0]) + case metav1.LabelSelectorOpExists, metav1.LabelSelectorOpDoesNotExist: + t.Fatalf("unexpected valueless operator %s", req.Operator) + default: + t.Fatalf("unexpected operator %s", req.Operator) + } + } + require.ElementsMatch(t, []string{"x", "y"}, notInValues, "both exclusions must be present in every branch") + require.Len(t, inValues, 1) + } +} + +// TestFlattenNameSpaceSelectorUnsupportedOperator verifies that a matchExpression with +// an operator other than In/NotIn/Exists/DoesNotExist is rejected (fail closed) rather +// than silently dropped, which could otherwise widen the selector. +func TestFlattenNameSpaceSelectorUnsupportedOperator(t *testing.T) { + selector := &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: tenantLabelKey, + Operator: metav1.LabelSelectorOperator("Frobnicate"), + Values: []string{"x"}, + }, + }, + } + s, err := flattenNameSpaceSelector(selector) + require.ErrorIs(t, err, ErrUnsupportedMatchExpressionOperator) + require.Nil(t, s) +} + +// TestFlattenNameSpaceSelectorEmptyValues verifies that In/NotIn requirements with +// no values are rejected (fail closed) rather than silently dropped, which could +// otherwise widen a selector or produce no rules at all. +func TestFlattenNameSpaceSelectorEmptyValues(t *testing.T) { + for _, op := range []metav1.LabelSelectorOperator{metav1.LabelSelectorOpIn, metav1.LabelSelectorOpNotIn} { + selector := &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: tenantLabelKey, + Operator: op, + Values: []string{}, + }, + }, + } + s, err := flattenNameSpaceSelector(selector) + require.ErrorIs(t, err, ErrEmptyMatchExpressionValues, "operator %s", op) + require.Nil(t, s) + } +} + +// TestFlattenNameSpaceSelectorExpansionLimit verifies that a namespaceSelector whose +// multi-value In requirements would expand into more selectors than NPM is willing to +// translate is rejected before any allocation. Each flattened selector is deep-copied and +// later becomes its own IPSet and ACL, and the count is the product of the value counts, +// so an unbounded selector exhausts memory on every node running NPM. +func TestFlattenNameSpaceSelectorExpansionLimit(t *testing.T) { + twoValueReqs := func(n int) []metav1.LabelSelectorRequirement { + reqs := make([]metav1.LabelSelectorRequirement, 0, n) + for i := 0; i < n; i++ { + reqs = append(reqs, metav1.LabelSelectorRequirement{ + Key: fmt.Sprintf("key%d", i), + Operator: metav1.LabelSelectorOpIn, + Values: []string{"a", "b"}, + }) + } + return reqs + } + + // 2^9 = 512 selectors is under the limit and must still translate. + under := &metav1.LabelSelector{MatchExpressions: twoValueReqs(9)} + selectors, err := flattenNameSpaceSelector(under) + require.NoError(t, err) + require.Len(t, selectors, 512) + + // 2^19 = 524288 selectors is the reported exhaustion case and must be rejected. + over := &metav1.LabelSelector{MatchExpressions: twoValueReqs(19)} + selectors, err = flattenNameSpaceSelector(over) + require.ErrorIs(t, err, ErrTooManyFlattenedSelectors) + require.Nil(t, selectors) + + // A single requirement wider than the limit is rejected on the first iteration, + // so the guard cannot be sidestepped by using one very wide requirement. + values := make([]string, maxFlattenedNSSelectors+1) + for i := range values { + values[i] = fmt.Sprintf("v%d", i) + } + wide := &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + {Key: "key", Operator: metav1.LabelSelectorOpIn, Values: values}, + }, + } + selectors, err = flattenNameSpaceSelector(wide) + require.ErrorIs(t, err, ErrTooManyFlattenedSelectors) + require.Nil(t, selectors) +} + +// TestTranslatePolicyExpansionLimit verifies the expansion guard surfaces through the full +// translation path rather than being swallowed, so an oversized policy is rejected instead +// of being expanded. +func TestTranslatePolicyExpansionLimit(t *testing.T) { + reqs := make([]metav1.LabelSelectorRequirement, 0, 19) + for i := 0; i < 19; i++ { + reqs = append(reqs, metav1.LabelSelectorRequirement{ + Key: fmt.Sprintf("key%d", i), + Operator: metav1.LabelSelectorOpIn, + Values: []string{"a", "b"}, + }) + } + + pol := &networkingv1.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: "expand", Namespace: defaultNS}, + Spec: networkingv1.NetworkPolicySpec{ + PodSelector: metav1.LabelSelector{}, + PolicyTypes: []networkingv1.PolicyType{networkingv1.PolicyTypeIngress}, + Ingress: []networkingv1.NetworkPolicyIngressRule{ + { + From: []networkingv1.NetworkPolicyPeer{ + {NamespaceSelector: &metav1.LabelSelector{MatchExpressions: reqs}}, + }, + }, + }, + }, + } + + npmNetPol, err := TranslatePolicy(pol, false) + require.ErrorIs(t, err, ErrTooManyFlattenedSelectors) + require.Nil(t, npmNetPol) +} + func TestIsValidLabel(t *testing.T) { good := []string{ "", @@ -635,3 +841,276 @@ func TestIsValidLabel(t *testing.T) { require.False(t, isValidLabelValue(b), "string was [%s]", b) } } + +// TestTranslatePolicyACLBudget covers the multiplication the selector cap alone does not +// catch. Every flattened namespaceSelector branch is emitted once per port in the rule, so a +// policy whose selector expansion is comfortably under the selector limit can still generate +// an enormous number of ACLs by listing many ports. Each ACL becomes an iptables rule. +func TestTranslatePolicyACLBudget(t *testing.T) { + // 2^9 = 512 flattened selectors: under maxFlattenedNSSelectors. + reqs := make([]metav1.LabelSelectorRequirement, 0, 9) + for i := 0; i < 9; i++ { + reqs = append(reqs, metav1.LabelSelectorRequirement{ + Key: fmt.Sprintf("key%d", i), + Operator: metav1.LabelSelectorOpIn, + Values: []string{"a", "b"}, + }) + } + + // Sanity: the selector expansion on its own is accepted. + flattened, err := flattenNameSpaceSelector(&metav1.LabelSelector{MatchExpressions: reqs}) + require.NoError(t, err) + require.Len(t, flattened, 512) + + // 512 selectors x 512 ports would be 262144 ACLs. + ports := make([]networkingv1.NetworkPolicyPort, 0, 512) + for i := 0; i < 512; i++ { + p := intstr.FromInt(1000 + i) + ports = append(ports, networkingv1.NetworkPolicyPort{Port: &p}) + } + + pol := &networkingv1.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: "expand", Namespace: defaultNS}, + Spec: networkingv1.NetworkPolicySpec{ + PodSelector: metav1.LabelSelector{}, + PolicyTypes: []networkingv1.PolicyType{networkingv1.PolicyTypeIngress}, + Ingress: []networkingv1.NetworkPolicyIngressRule{{ + Ports: ports, + From: []networkingv1.NetworkPolicyPeer{ + {NamespaceSelector: &metav1.LabelSelector{MatchExpressions: reqs}}, + }, + }}, + }, + } + + npmNetPol, err := TranslatePolicy(pol, false) + require.ErrorIs(t, err, ErrTooManyACLs, + "a policy that multiplies selectors by ports must be rejected even when the selector count is under its own limit") + require.Nil(t, npmNetPol) +} + +// TestTranslatePolicyOrdinaryPolicyWithinACLBudget guards the budget against false positives: +// a normal policy with several peers and ports must translate unaffected. +func TestTranslatePolicyOrdinaryPolicyWithinACLBudget(t *testing.T) { + ports := make([]networkingv1.NetworkPolicyPort, 0, 8) + for i := 0; i < 8; i++ { + p := intstr.FromInt(8000 + i) + ports = append(ports, networkingv1.NetworkPolicyPort{Port: &p}) + } + + pol := &networkingv1.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: "normal", Namespace: defaultNS}, + Spec: networkingv1.NetworkPolicySpec{ + PodSelector: metav1.LabelSelector{MatchLabels: map[string]string{"app": "web"}}, + PolicyTypes: []networkingv1.PolicyType{networkingv1.PolicyTypeIngress}, + Ingress: []networkingv1.NetworkPolicyIngressRule{{ + Ports: ports, + From: []networkingv1.NetworkPolicyPeer{ + {NamespaceSelector: &metav1.LabelSelector{MatchLabels: map[string]string{teamLabelKey: teamBlueValue}}}, + {PodSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"role": "client"}}}, + {IPBlock: &networkingv1.IPBlock{CIDR: "10.0.0.0/8"}}, + }, + }}, + }, + } + + npmNetPol, err := TranslatePolicy(pol, false) + require.NoError(t, err) + require.NotNil(t, npmNetPol) + require.Less(t, len(npmNetPol.ACLs), maxACLsPerPolicy) +} + +// TestTranslatePolicyExactlyAtACLLimit guards the boundary. The per-append guard is asked +// whether there is room for one more ACL, so it must refuse at the ceiling; the check on the +// finished policy is asked whether the policy is past the ceiling, so it must admit a policy +// that lands exactly on it. Using the same comparison for both would reject a policy of +// exactly maxACLsPerPolicy rules. +func TestTranslatePolicyExactlyAtACLLimit(t *testing.T) { + // the budget holds back a slot for the default drop the policy implies, so this is the + // widest a policy can get: every port emits an ACL and the drop still fits under the ceiling + portCount := maxACLsPerPolicy - reservedDropACLs + ports := make([]networkingv1.NetworkPolicyPort, 0, portCount) + for i := 0; i < portCount; i++ { + p := intstr.FromInt(1 + i) + ports = append(ports, networkingv1.NetworkPolicyPort{Port: &p}) + } + + pol := &networkingv1.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: "at-limit", Namespace: defaultNS}, + Spec: networkingv1.NetworkPolicySpec{ + PodSelector: metav1.LabelSelector{}, + PolicyTypes: []networkingv1.PolicyType{networkingv1.PolicyTypeIngress}, + Ingress: []networkingv1.NetworkPolicyIngressRule{{Ports: ports}}, + }, + } + + npmNetPol, err := TranslatePolicy(pol, false) + require.NoError(t, err, "a policy at the widest the budget allows must translate") + require.NotNil(t, npmNetPol) + require.Len(t, npmNetPol.ACLs, portCount+1, "every port plus the default drop") + require.LessOrEqual(t, len(npmNetPol.ACLs), maxACLsPerPolicy, + "the drop must never take the policy past the ceiling") +} + +// TestPortOnlyRuleBudgetStopsWithinPortLoop covers a rule that lists ports and no peers. That +// path emits one ACL per port with no peer expansion to bound it, so the budget has to be +// checked inside its loop rather than only by the backstop at the end of translation. +func TestPortOnlyRuleBudgetStopsWithinPortLoop(t *testing.T) { + portCount := maxACLsPerPolicy * 2 + ports := make([]networkingv1.NetworkPolicyPort, 0, portCount) + for i := 0; i < portCount; i++ { + p := intstr.FromInt(1 + i) + ports = append(ports, networkingv1.NetworkPolicyPort{Port: &p}) + } + + npmNetPol := policies.NewNPMNetworkPolicy("port-only", defaultNS) + err := checkOnlyPortRuleExists(true, false, false, ports, false, policies.Ingress, npmNetPol) + require.ErrorIs(t, err, ErrTooManyACLs) + require.LessOrEqual(t, len(npmNetPol.ACLs), maxACLsPerPolicy, + "a rule with only ports must stop once the budget is spent") +} + +// TestPeerAndPortRuleBudgetStopsWithinPortLoop covers a single peer listing more ports than +// the budget allows. One peer emits one ACL per port, so a budget checked only on entry to +// peerAndPortRule would let that peer materialize every ACL before anything noticed. +func TestPeerAndPortRuleBudgetStopsWithinPortLoop(t *testing.T) { + portCount := maxACLsPerPolicy * 2 + ports := make([]networkingv1.NetworkPolicyPort, 0, portCount) + for i := 0; i < portCount; i++ { + p := intstr.FromInt(1 + i) + ports = append(ports, networkingv1.NetworkPolicyPort{Port: &p}) + } + + npmNetPol := policies.NewNPMNetworkPolicy("wide-ports", defaultNS) + err := peerAndPortRule(npmNetPol, policies.Ingress, ports, []policies.SetInfo{}, false) + require.ErrorIs(t, err, ErrTooManyACLs) + require.LessOrEqual(t, len(npmNetPol.ACLs), maxACLsPerPolicy, + "the port loop must stop once the budget is spent instead of emitting an ACL for every port") +} + +// TestNotInValuesAreBounded covers a long NotIn list. Compiling it as one conjunction keeps it +// out of the flattened-selector count and out of the rule budget, because it stays a single +// selector producing a single rule, but every value still becomes its own IPSet and its own +// condition on that rule. The match bound is what stops it. +func TestNotInValuesAreBounded(t *testing.T) { + values := make([]string, 0, maxSelectorMatches+1) + for i := 0; i <= maxSelectorMatches; i++ { + values = append(values, fmt.Sprintf("v%d", i)) + } + + selector := &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + {Key: tenantLabelKey, Operator: metav1.LabelSelectorOpNotIn, Values: values}, + }, + } + + flattened, err := flattenNameSpaceSelector(selector) + require.ErrorIs(t, err, ErrTooManySelectorMatches, + "a NotIn list past the match bound must be refused") + require.Nil(t, flattened) + + // the same policy is refused end to end, so no partial rules are installed + pol := &networkingv1.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: "wide-notin", Namespace: defaultNS}, + Spec: networkingv1.NetworkPolicySpec{ + PodSelector: metav1.LabelSelector{}, + PolicyTypes: []networkingv1.PolicyType{networkingv1.PolicyTypeIngress}, + Ingress: []networkingv1.NetworkPolicyIngressRule{{ + From: []networkingv1.NetworkPolicyPeer{{NamespaceSelector: selector}}, + }}, + }, + } + npmNetPol, err := TranslatePolicy(pol, false) + require.ErrorIs(t, err, ErrTooManySelectorMatches) + require.Nil(t, npmNetPol) +} + +// TestNotInValuesAtTheBoundAreAccepted keeps the bound from rejecting a selector that sits +// exactly on it, and guards the ordinary small NotIn that real policies use. +func TestNotInValuesAtTheBoundAreAccepted(t *testing.T) { + // one short of the bound: the selector matches only negatively, so parseNSSelector + // anchors it with the all-namespaces set and that match counts too + values := make([]string, 0, maxSelectorMatches-1) + for i := 0; i < maxSelectorMatches-1; i++ { + values = append(values, fmt.Sprintf("v%d", i)) + } + + flattened, err := flattenNameSpaceSelector(&metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + {Key: tenantLabelKey, Operator: metav1.LabelSelectorOpNotIn, Values: values}, + }, + }) + require.NoError(t, err, "a selector exactly on the bound must translate") + require.Len(t, flattened, 1, "a NotIn stays a single conjunction") + require.Len(t, flattened[0].MatchExpressions, maxSelectorMatches-1) +} + +// TestMatchLabelsOnlySelectorIsBounded covers a selector that carries only matchLabels. It +// takes a shortcut past the expression handling, but each label still becomes its own match, +// so the bound has to be applied before that shortcut. +func TestMatchLabelsOnlySelectorIsBounded(t *testing.T) { + labels := make(map[string]string, maxSelectorMatches+1) + for i := 0; i <= maxSelectorMatches; i++ { + labels[fmt.Sprintf("key%d", i)] = "v" + } + + flattened, err := flattenNameSpaceSelector(&metav1.LabelSelector{MatchLabels: labels}) + require.ErrorIs(t, err, ErrTooManySelectorMatches, + "a matchLabels-only selector past the bound must be refused") + require.Nil(t, flattened) + + // an ordinary selector is untouched + ok, err := flattenNameSpaceSelector(&metav1.LabelSelector{ + MatchLabels: map[string]string{"team": teamBlueValue}, + }) + require.NoError(t, err) + require.Len(t, ok, 1) +} + +// TestSelectorBranchesTimesMatchesIsBounded covers a selector that stays under both the match +// bound and the branch bound yet multiplies them together. Each branch repeats every match, and +// the translator materializes an IPSet and a SetInfo per match before the policy's rule budget +// is consulted, so the product is what has to be bounded. +func TestSelectorBranchesTimesMatchesIsBounded(t *testing.T) { + // 991 labels plus nine two-value In requirements: 1000 matches per branch, 512 branches + labels := make(map[string]string, 991) + for i := 0; i < 991; i++ { + labels[fmt.Sprintf("key%d", i)] = "v" + } + reqs := make([]metav1.LabelSelectorRequirement, 0, 9) + for i := 0; i < 9; i++ { + reqs = append(reqs, metav1.LabelSelectorRequirement{ + Key: fmt.Sprintf("in%d", i), + Operator: metav1.LabelSelectorOpIn, + Values: []string{"a", "b"}, + }) + } + + selector := &metav1.LabelSelector{MatchLabels: labels, MatchExpressions: reqs} + + // each factor on its own is within its bound + require.LessOrEqual(t, len(labels)+len(reqs), maxSelectorMatches) + require.LessOrEqual(t, 1< 0 { + // The Windows datapath refuses an except before any of it is canonicalized, exactly as + // it did before, so the validation below is reached on the Linux path only. + if util.IsWindowsDP() && len(ipBlockRule.Except) > 0 { return nil, ErrUnsupportedExceptCIDR } + // de-duplicated Except if there are redundance elements, in canonical form so they + // compare correctly against the all-addresses split entries below. + deDupExcepts, err := canonicalizeExcepts(ipBlockRule.Except) + if err != nil { + return nil, err + } + lenOfDeDupExcepts := len(deDupExcepts) + var members []string indexOfMembers := 0 // Ipset doesn't allow 0.0.0.0/0 to be added. @@ -190,7 +249,7 @@ func ipBlockIPSet(policyName, ns string, direction policies.Direction, ipBlockSe // splitCIDRSet has two entries ("0.0.0.0/1" and "128.0.0.0/1") as key. splitCIDRLen := 2 splitCIDRSet := make(map[string]int, splitCIDRLen) - if ipBlockRule.CIDR == "0.0.0.0/0" { + if cidr == "0.0.0.0/0" { // two cidrs (0.0.0.0/1 and 128.0.0.0/1) for 0.0.0.0/0 + except. members = make([]string, lenOfDeDupExcepts+splitCIDRLen) // in case of "0.0.0.0/0", "0.0.0.0/1" or "0.0.0.0/1 nomatch" comes eariler than "128.0.0.0/1" or "128.0.0.0/1 nomatch". @@ -203,7 +262,7 @@ func ipBlockIPSet(policyName, ns string, direction policies.Direction, ipBlockSe } else { // one cidr + except members = make([]string, lenOfDeDupExcepts+1) - members[indexOfMembers] = ipBlockRule.CIDR + members[indexOfMembers] = cidr indexOfMembers++ } @@ -233,7 +292,13 @@ func ipBlockRule(policyName, ns string, direction policies.Direction, matchType return nil, policies.SetInfo{}, nil } - if !util.IsIPV4(ipBlockRule.CIDR) { + // Validate the canonical form rather than the literal the user wrote. A block whose host + // bits are set, such as "10.0.0.0/0", denotes exactly the same addresses as its canonical + // form, but IsIPV4 refuses a /0 that is not spelled "0.0.0.0". Rejecting here aborts the + // translation of the whole policy, so neither the peer rule nor the default drop the policy + // 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. + if _, ok := util.NormalizeCIDR(ipBlockRule.CIDR); !ok { return nil, policies.SetInfo{}, ErrUnsupportedIPAddress } @@ -339,6 +404,10 @@ func ruleExists(ports []networkingv1.NetworkPolicyPort, peer []networkingv1.Netw // peerAndPortRule deals with composite rules including ports and peers // (e.g., IPBlock, podSelector, namespaceSelector, or both podSelector and namespaceSelector). func peerAndPortRule(npmNetPol *policies.NPMNetworkPolicy, direction policies.Direction, ports []networkingv1.NetworkPolicyPort, setInfo []policies.SetInfo, npmLiteToggle bool) error { + if err := checkACLBudget(npmNetPol); err != nil { + return err + } + if len(ports) == 0 { acl := policies.NewACLPolicy(policies.Allowed, direction) acl.AddSetInfo(setInfo) @@ -347,6 +416,12 @@ func peerAndPortRule(npmNetPol *policies.NPMNetworkPolicy, direction policies.Di } for i := range ports { + // Re-checked per port, not only on entry: this peer emits one ACL per port, so a + // check that ran once could not stop a single peer from expanding past the limit. + if err := checkACLBudget(npmNetPol); err != nil { + return err + } + portKind, err := portType(ports[i]) if err != nil { return err @@ -763,9 +838,57 @@ func TranslatePolicy(npObj *networkingv1.NetworkPolicy, npmLiteToggle bool) (*po } } } + + if err := checkACLTotal(npmNetPol); err != nil { + return nil, err + } + return npmNetPol, nil } +// maxACLsPerPolicy bounds how many ACLs a single NetworkPolicy may translate into. Each ACL +// becomes one iptables rule, and the count multiplies rather than adds: every flattened +// namespaceSelector branch is emitted once per port in the rule, and that product is summed +// across every peer and every rule in the policy. Bounding the flattened selector count on +// its own is therefore not enough, because a policy that stays under that bound can still +// multiply itself out by listing many ports. The ceiling is far above any workable policy, +// since a policy expanding this wide would already be unusable as iptables rules. +const maxACLsPerPolicy = 2000 + +// reservedDropACLs is what the per-append guard holds back for the default drop a policy still +// needs after its rules are translated, one per direction. Without the reservation a policy +// that filled the budget with allow rules would append its drop on top and land one or two ACLs +// past the ceiling before the check at the end of translation refused it. +const reservedDropACLs = 2 + +// 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, including the default drop still to come. +func checkACLBudget(npmNetPol *policies.NPMNetworkPolicy) error { + if len(npmNetPol.ACLs) >= maxACLsPerPolicy-reservedDropACLs { + return tooManyACLs(npmNetPol) + } + return nil +} + +// checkACLTotal reports whether the finished policy is past the ceiling. It is the backstop +// for the paths that append without asking for room first, including the direct-rule path +// this change leaves alone. It admits a policy that lands exactly on the ceiling, which +// checkACLBudget cannot do because it is asked before the ACL exists. +func checkACLTotal(npmNetPol *policies.NPMNetworkPolicy) error { + if len(npmNetPol.ACLs) > maxACLsPerPolicy { + return tooManyACLs(npmNetPol) + } + return nil +} + +// tooManyACLs builds the refusal. The error carries the policy context and is recorded once +// by the caller. +func tooManyACLs(npmNetPol *policies.NPMNetworkPolicy) error { + return fmt.Errorf("network policy %s expands past the %d rule limit: %w", + npmNetPol.PolicyKey, maxACLsPerPolicy, ErrTooManyACLs) +} + func checkForNamedPortType(npmNetPol *policies.NPMNetworkPolicy, portKind netpolPortType, npmLiteToggle bool, direction policies.Direction, port *networkingv1.NetworkPolicyPort, cidr string) error { if npmLiteToggle && portKind == namedPortType { return fmt.Errorf("named port not supported in policy %s (namespace: %s, direction: %s, cidr: %s, port: %v, protocol: %v): %w", @@ -786,6 +909,12 @@ func checkOnlyPortRuleExists( // #1. Only Ports fields exist in rule if portRuleExists && !peerRuleExists && !allowExternal { for i := range ports { + // This path emits one ACL per port with no peer to bound it, so the budget is + // checked here too rather than leaving it to the backstop at the end. + if err := checkACLBudget(npmNetPol); err != nil { + return err + } + portKind, err := portType(ports[i]) if err != nil { return err diff --git a/npm/pkg/controlplane/translation/translatePolicy_test.go b/npm/pkg/controlplane/translation/translatePolicy_test.go index 29fa77407ea..209b1cf62ba 100644 --- a/npm/pkg/controlplane/translation/translatePolicy_test.go +++ b/npm/pkg/controlplane/translation/translatePolicy_test.go @@ -24,6 +24,15 @@ const ( appLabelKey string = "app" enclosingCIDR string = "10.244.1.0/24" exceptedHostBits string = "10.244.1.106/32" + + tenantLabelKey string = "tenant" + teamLabelKey string = "team" + blockedLabelKey string = "blocked" + teamBlueValue string = "blue" + lowerHalfNomatch string = "0.0.0.0/1 nomatch" + exceptedClassA string = "200.0.0.0/8" + ingressName string = "ingress" + egressName string = "egress" ) var namedPortPolicyKey = fmt.Sprintf("%s/%s", defaultNS, namedPortStr) @@ -636,13 +645,18 @@ func TestIPBlockIPSet(t *testing.T) { translatedIPSet: ipsets.NewTranslatedIPSet("test:in-ns:default-0-0IN", ipsets.CIDRBlocks, []string{"0.0.0.0/1", "128.0.0.0/1"}...), }, { - name: "cidr: 0.0.0.0/0 and except: 10.0.0.0/1", + // "10.0.0.0/1" is a non-canonical spelling of the block "0.0.0.0/1", so this + // except names the lower half that the 0.0.0.0/0 split already emits. It must + // therefore collapse onto that entry as a nomatch, exactly as the canonical + // "0.0.0.0/1" case below does. Emitting "0.0.0.0/1" alongside a separate + // "10.0.0.0/1 nomatch" would name the same net twice with opposite meanings. + name: "cidr: 0.0.0.0/0 and except: 10.0.0.0/1 (non-canonical 0.0.0.0/1)", ipBlockInfo: createIPBlockInfo("test", defaultNS, policies.Ingress, policies.SrcMatch, 0, 0), ipBlockRule: &networkingv1.IPBlock{ CIDR: "0.0.0.0/0", Except: []string{"10.0.0.0/1"}, }, - translatedIPSet: ipsets.NewTranslatedIPSet("test:in-ns:default-0-0IN", ipsets.CIDRBlocks, []string{"0.0.0.0/1", "128.0.0.0/1", "10.0.0.0/1 nomatch"}...), + translatedIPSet: ipsets.NewTranslatedIPSet("test:in-ns:default-0-0IN", ipsets.CIDRBlocks, []string{lowerHalfNomatch, "128.0.0.0/1"}...), skipWindows: true, }, { @@ -652,7 +666,7 @@ func TestIPBlockIPSet(t *testing.T) { CIDR: "0.0.0.0/0", Except: []string{"0.0.0.0/1"}, }, - translatedIPSet: ipsets.NewTranslatedIPSet("test:in-ns:default-0-0IN", ipsets.CIDRBlocks, []string{"0.0.0.0/1 nomatch", "128.0.0.0/1"}...), + translatedIPSet: ipsets.NewTranslatedIPSet("test:in-ns:default-0-0IN", ipsets.CIDRBlocks, []string{lowerHalfNomatch, "128.0.0.0/1"}...), skipWindows: true, }, { @@ -672,7 +686,7 @@ func TestIPBlockIPSet(t *testing.T) { CIDR: "0.0.0.0/0", Except: []string{"0.0.0.0/1", "128.0.0.0/1"}, }, - translatedIPSet: ipsets.NewTranslatedIPSet("test:in-ns:default-0-0IN", ipsets.CIDRBlocks, []string{"0.0.0.0/1 nomatch", "128.0.0.0/1 nomatch"}...), + translatedIPSet: ipsets.NewTranslatedIPSet("test:in-ns:default-0-0IN", ipsets.CIDRBlocks, []string{lowerHalfNomatch, "128.0.0.0/1 nomatch"}...), skipWindows: true, }, { @@ -682,7 +696,7 @@ func TestIPBlockIPSet(t *testing.T) { CIDR: "0.0.0.0/0", Except: []string{"0.0.0.0/1", "128.0.0.0/1", "128.0.0.0/1"}, }, - translatedIPSet: ipsets.NewTranslatedIPSet("test:in-ns:default-0-0IN", ipsets.CIDRBlocks, []string{"0.0.0.0/1 nomatch", "128.0.0.0/1 nomatch"}...), + translatedIPSet: ipsets.NewTranslatedIPSet("test:in-ns:default-0-0IN", ipsets.CIDRBlocks, []string{lowerHalfNomatch, "128.0.0.0/1 nomatch"}...), skipWindows: true, }, } @@ -804,6 +818,25 @@ func TestIPBlockRule(t *testing.T) { } } +// TestIPBlockRuleRejectsInvalidExcept covers an ipBlock whose except is not an IPv4 CIDR. Such +// an exclusion cannot be programmed, so the translation fails rather than carrying the entry +// into the set, which would either widen the allow to the enclosing CIDR or take the whole set +// down when it is restored. The Windows datapath refuses any except before this check, so the +// case is exercised on Linux only. +func TestIPBlockRuleRejectsInvalidExcept(t *testing.T) { + if util.IsWindowsDP() { + t.Skip("the Windows datapath refuses any except on this path") + } + + for _, except := range []string{"2001:db8::/32", "not-a-cidr", "10.0.0.1", "10.0.0.0/33"} { + translatedIPSet, setInfo, err := ipBlockRule("test", defaultNS, policies.Ingress, policies.SrcMatch, 0, 0, + &networkingv1.IPBlock{CIDR: "172.17.0.0/16", Except: []string{except}}) + require.ErrorIs(t, err, ErrUnsupportedIPAddress, "except %q must be refused", except) + require.Nil(t, translatedIPSet) + require.Equal(t, policies.SetInfo{}, setInfo) + } +} + func TestPodSelector(t *testing.T) { matchType := policies.DstMatch policyKey := "test-ns/test-policy" @@ -1268,6 +1301,391 @@ func TestNameSpaceSelector(t *testing.T) { } } +// TestNameSpaceSelectorMultiValueNotIn verifies that a namespaceSelector with a +// single multi-value NotIn requirement is translated (after flatten, as translateRule +// does) into one decision carrying a negated match-set for every excluded value. +// Emitting these as separate allow rules would be additive (OR) and admit a namespace +// that carries any one of the excluded values. +func TestNameSpaceSelectorMultiValueNotIn(t *testing.T) { + matchType := policies.SrcMatch + selector := &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: tenantLabelKey, + Operator: metav1.LabelSelectorOpNotIn, + Values: []string{"x", "y"}, + }, + }, + } + + flattened, err := flattenNameSpaceSelector(selector) + require.NoError(t, err) + // The NotIn conjunction must stay in a single selector, not fan out. + require.Len(t, flattened, 1) + + _, nsSelectorList := nameSpaceSelector(matchType, &flattened[0]) + + expected := []policies.SetInfo{ + // The all-namespaces set keeps the negation-only match scoped to cluster namespaces. + policies.NewSetInfo(util.KubeAllNamespacesFlag, ipsets.KeyLabelOfNamespace, included, matchType), + policies.NewSetInfo("tenant:x", ipsets.KeyValueLabelOfNamespace, nonIncluded, matchType), + policies.NewSetInfo("tenant:y", ipsets.KeyValueLabelOfNamespace, nonIncluded, matchType), + } + require.ElementsMatch(t, expected, nsSelectorList) +} + +// TestNameSpaceSelectorMatchLabelsAndMultiValueNotIn covers a namespaceSelector that +// combines matchLabels with a multi-value NotIn matchExpression. The matchLabels set +// must be ANDed into the same decision as the two negated values (a positive match plus +// two negated matches in one ACL), matching Kubernetes' conjunction of all requirements. +func TestNameSpaceSelectorMatchLabelsAndMultiValueNotIn(t *testing.T) { + matchType := policies.SrcMatch + selector := &metav1.LabelSelector{ + MatchLabels: map[string]string{teamLabelKey: teamBlueValue}, + MatchExpressions: []metav1.LabelSelectorRequirement{ + {Key: tenantLabelKey, Operator: metav1.LabelSelectorOpNotIn, Values: []string{"x", "y"}}, + }, + } + + flattened, err := flattenNameSpaceSelector(selector) + require.NoError(t, err) + // matchLabels + a single conjunctive NotIn must stay in ONE selector, not fan out. + require.Len(t, flattened, 1) + + _, nsSelectorList := nameSpaceSelector(matchType, &flattened[0]) + + expected := []policies.SetInfo{ + policies.NewSetInfo("team:blue", ipsets.KeyValueLabelOfNamespace, included, matchType), + policies.NewSetInfo("tenant:x", ipsets.KeyValueLabelOfNamespace, nonIncluded, matchType), + policies.NewSetInfo("tenant:y", ipsets.KeyValueLabelOfNamespace, nonIncluded, matchType), + } + require.ElementsMatch(t, expected, nsSelectorList, + "matchLabels set must be ANDed with both negated tenant sets in one decision") +} + +// TestNameSpaceSelectorNegationOnlyIsScopedToNamespaces verifies that a namespaceSelector +// whose requirements are all negative (NotIn / DoesNotExist) is intersected with the +// all-namespaces set. A negated set match is satisfied by every address that is not in the +// set, so without a positive set to intersect with, the decision also matches addresses +// that are not cluster pods at all (e.g. the internet). +func TestNameSpaceSelectorNegationOnlyIsScopedToNamespaces(t *testing.T) { + matchType := policies.DstMatch + tests := []struct { + name string + selector *metav1.LabelSelector + expected []policies.SetInfo + }{ + { + name: "single-value NotIn", + selector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + {Key: tenantLabelKey, Operator: metav1.LabelSelectorOpNotIn, Values: []string{"x"}}, + }, + }, + expected: []policies.SetInfo{ + policies.NewSetInfo(util.KubeAllNamespacesFlag, ipsets.KeyLabelOfNamespace, included, matchType), + policies.NewSetInfo("tenant:x", ipsets.KeyValueLabelOfNamespace, nonIncluded, matchType), + }, + }, + { + name: "DoesNotExist", + selector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + {Key: tenantLabelKey, Operator: metav1.LabelSelectorOpDoesNotExist}, + }, + }, + expected: []policies.SetInfo{ + policies.NewSetInfo(util.KubeAllNamespacesFlag, ipsets.KeyLabelOfNamespace, included, matchType), + policies.NewSetInfo(tenantLabelKey, ipsets.KeyLabelOfNamespace, nonIncluded, matchType), + }, + }, + { + name: "NotIn and DoesNotExist together", + selector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + {Key: tenantLabelKey, Operator: metav1.LabelSelectorOpNotIn, Values: []string{"x"}}, + {Key: teamLabelKey, Operator: metav1.LabelSelectorOpDoesNotExist}, + }, + }, + expected: []policies.SetInfo{ + policies.NewSetInfo(util.KubeAllNamespacesFlag, ipsets.KeyLabelOfNamespace, included, matchType), + policies.NewSetInfo("tenant:x", ipsets.KeyValueLabelOfNamespace, nonIncluded, matchType), + policies.NewSetInfo(teamLabelKey, ipsets.KeyLabelOfNamespace, nonIncluded, matchType), + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + nsSelectorIPSets, nsSelectorList := nameSpaceSelector(matchType, tt.selector) + require.ElementsMatch(t, tt.expected, nsSelectorList) + // The all-namespaces set must also be translated so it exists in the dataplane. + require.Contains(t, nsSelectorIPSets, + ipsets.NewTranslatedIPSet(util.KubeAllNamespacesFlag, ipsets.KeyLabelOfNamespace)) + }) + } +} + +// TestNameSpaceSelectorWithPositiveMatchIsUnchanged verifies that the all-namespaces +// intersection is added only when it is needed. A selector that already carries a positive +// requirement is scoped to namespaces by that requirement, so it must be left as-is. +func TestNameSpaceSelectorWithPositiveMatchIsUnchanged(t *testing.T) { + matchType := policies.DstMatch + tests := []struct { + name string + selector *metav1.LabelSelector + expected []policies.SetInfo + }{ + { + name: "matchLabels only", + selector: &metav1.LabelSelector{MatchLabels: map[string]string{teamLabelKey: teamBlueValue}}, + expected: []policies.SetInfo{ + policies.NewSetInfo("team:blue", ipsets.KeyValueLabelOfNamespace, included, matchType), + }, + }, + { + name: "matchLabels with a negative expression", + selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{teamLabelKey: teamBlueValue}, + MatchExpressions: []metav1.LabelSelectorRequirement{ + {Key: tenantLabelKey, Operator: metav1.LabelSelectorOpNotIn, Values: []string{"x"}}, + }, + }, + expected: []policies.SetInfo{ + policies.NewSetInfo("team:blue", ipsets.KeyValueLabelOfNamespace, included, matchType), + policies.NewSetInfo("tenant:x", ipsets.KeyValueLabelOfNamespace, nonIncluded, matchType), + }, + }, + { + name: "Exists with a negative expression", + selector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + {Key: teamLabelKey, Operator: metav1.LabelSelectorOpExists}, + {Key: tenantLabelKey, Operator: metav1.LabelSelectorOpNotIn, Values: []string{"x"}}, + }, + }, + expected: []policies.SetInfo{ + policies.NewSetInfo(teamLabelKey, ipsets.KeyLabelOfNamespace, included, matchType), + policies.NewSetInfo("tenant:x", ipsets.KeyValueLabelOfNamespace, nonIncluded, matchType), + }, + }, + { + name: "empty selector still resolves to all namespaces once", + selector: &metav1.LabelSelector{}, + expected: []policies.SetInfo{ + policies.NewSetInfo(util.KubeAllNamespacesFlag, ipsets.KeyLabelOfNamespace, included, matchType), + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, nsSelectorList := nameSpaceSelector(matchType, tt.selector) + require.ElementsMatch(t, tt.expected, nsSelectorList) + }) + } +} + +// TestTranslatePolicyNegationOnlyNamespaceSelector is the end-to-end regression for a +// peer whose only requirement is a negative namespaceSelector. It asserts that the +// resulting allow decision carries the all-namespaces set, so the rule cannot be +// satisfied by an address outside the cluster. Egress is the impactful direction (an +// unscoped negation lets a selected pod reach arbitrary external hosts), but ingress is +// covered too since the compiler is direction-agnostic. +func TestTranslatePolicyNegationOnlyNamespaceSelector(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + direction networkingv1.PolicyType + matchType policies.MatchType + peerList func(*policies.ACLPolicy) []policies.SetInfo + }{ + { + name: egressName, + direction: networkingv1.PolicyTypeEgress, + matchType: policies.DstMatch, + peerList: func(acl *policies.ACLPolicy) []policies.SetInfo { return acl.DstList }, + }, + { + name: ingressName, + direction: networkingv1.PolicyTypeIngress, + matchType: policies.SrcMatch, + peerList: func(acl *policies.ACLPolicy) []policies.SetInfo { return acl.SrcList }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + pol := nsNotInPolicy("victim", "default", tenantLabelKey, tt.direction, nil, "x") + npmNetPol, err := TranslatePolicy(pol, false) + require.NoError(t, err) + + var theAllow *policies.ACLPolicy + for i := range npmNetPol.ACLs { + if npmNetPol.ACLs[i].Target == policies.Allowed { + require.Nil(t, theAllow, "there must be exactly one allow ACL") + theAllow = npmNetPol.ACLs[i] + } + } + require.NotNil(t, theAllow) + + peers := tt.peerList(theAllow) + require.ElementsMatch(t, []policies.SetInfo{ + policies.NewSetInfo(util.KubeAllNamespacesFlag, ipsets.KeyLabelOfNamespace, included, tt.matchType), + policies.NewSetInfo("tenant:x", ipsets.KeyValueLabelOfNamespace, nonIncluded, tt.matchType), + }, peers, "a negation-only namespaceSelector must be intersected with the all-namespaces set") + + var sawAllNamespaces bool + for _, si := range peers { + if si.Included && si.IPSet.Name == util.KubeAllNamespacesFlag { + sawAllNamespaces = true + } + } + require.True(t, sawAllNamespaces, + "without the all-namespaces set the negated match also admits non-cluster addresses") + }) + } +} + +// nsNotInPolicy builds a NetworkPolicy that selects all local pods and, for the given +// direction, admits peers whose namespace matches `key NotIn values`. When ports is +// non-empty, the peer rule also carries those ports. +func nsNotInPolicy(name, ns, key string, direction networkingv1.PolicyType, ports []networkingv1.NetworkPolicyPort, values ...string) *networkingv1.NetworkPolicy { + peer := networkingv1.NetworkPolicyPeer{ + NamespaceSelector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + {Key: key, Operator: metav1.LabelSelectorOpNotIn, Values: values}, + }, + }, + } + pol := &networkingv1.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: ns}, + Spec: networkingv1.NetworkPolicySpec{ + PodSelector: metav1.LabelSelector{}, + PolicyTypes: []networkingv1.PolicyType{direction}, + }, + } + if direction == networkingv1.PolicyTypeIngress { + pol.Spec.Ingress = []networkingv1.NetworkPolicyIngressRule{{Ports: ports, From: []networkingv1.NetworkPolicyPeer{peer}}} + } else { + pol.Spec.Egress = []networkingv1.NetworkPolicyEgressRule{{Ports: ports, To: []networkingv1.NetworkPolicyPeer{peer}}} + } + return pol +} + +// TestTranslatePolicyMultiValueNotInConjunction is the end-to-end regression for a +// multi-value namespaceSelector NotIn. It drives the full TranslatePolicy path (both +// directions, with and without a port) and asserts the complete enforcement invariant: +// exactly ONE allow ACL exists, it negates every excluded value within that single +// decision (a conjunction / AND) and references no positive tenant set, and a default +// drop is still present. The pre-fix behavior emitted one additive allow ACL per value, +// so a namespace carrying any one excluded value matched the ACL negating another value +// and was admitted before the default drop. +func TestTranslatePolicyMultiValueNotInConjunction(t *testing.T) { + t.Parallel() + + tcpPort := networkingv1.NetworkPolicyPort{Port: &intstr.IntOrString{Type: intstr.Int, IntVal: 80}} + + tests := []struct { + name string + direction networkingv1.PolicyType + ports []networkingv1.NetworkPolicyPort + peerList func(*policies.ACLPolicy) []policies.SetInfo + }{ + { + name: ingressName, + direction: networkingv1.PolicyTypeIngress, + peerList: func(acl *policies.ACLPolicy) []policies.SetInfo { return acl.SrcList }, + }, + { + name: egressName, + direction: networkingv1.PolicyTypeEgress, + peerList: func(acl *policies.ACLPolicy) []policies.SetInfo { return acl.DstList }, + }, + { + name: "ingress-with-port", + direction: networkingv1.PolicyTypeIngress, + ports: []networkingv1.NetworkPolicyPort{tcpPort}, + peerList: func(acl *policies.ACLPolicy) []policies.SetInfo { return acl.SrcList }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + pol := nsNotInPolicy("victim", "default", tenantLabelKey, tt.direction, tt.ports, "attacker", "quarantine") + npmNetPol, err := TranslatePolicy(pol, false) + require.NoError(t, err) + + excluded := map[string]bool{"tenant:attacker": true, "tenant:quarantine": true} + var allowACLs, dropACLs int + var theAllow, theDrop *policies.ACLPolicy + for i := range npmNetPol.ACLs { + acl := npmNetPol.ACLs[i] + switch acl.Target { + case policies.Allowed: + allowACLs++ + theAllow = npmNetPol.ACLs[i] + case policies.Dropped: + dropACLs++ + theDrop = npmNetPol.ACLs[i] + default: + t.Fatalf("unexpected ACL target %v", acl.Target) + } + } + + // Full enforcement invariant: exactly one allow decision and exactly one + // default drop. An additive-OR bypass would yield two allow ACLs; a missing + // drop or an allow-all leaking in would also be caught here. + require.Equal(t, 1, allowACLs, "there must be exactly one allow ACL, not additive allow ACLs") + require.Equal(t, 1, dropACLs, "there must be exactly one default drop ACL") + require.NotNil(t, theAllow) + require.NotNil(t, theDrop) + + // The single allow ACL's peer list must be the two excluded values, each a + // negated match (Included == false), intersected with the all-namespaces set. + // The all-namespaces set is what keeps a negation-only match scoped to cluster + // namespaces; without it the negations alone also match non-cluster addresses. + allowPeers := tt.peerList(theAllow) + require.Len(t, allowPeers, 3, "allow ACL must reference the two excluded sets plus the all-namespaces set") + var negated []string + var positive []string + for _, si := range allowPeers { + if si.Included { + require.Equal(t, util.KubeAllNamespacesFlag, si.IPSet.Name, + "the only positive set may be the all-namespaces set") + require.Equal(t, ipsets.KeyLabelOfNamespace, si.IPSet.Type) + positive = append(positive, si.IPSet.Name) + continue + } + require.True(t, excluded[si.IPSet.Name], "unexpected set %s in allow ACL", si.IPSet.Name) + require.Equal(t, ipsets.KeyValueLabelOfNamespace, si.IPSet.Type) + negated = append(negated, si.IPSet.Name) + } + require.ElementsMatch(t, []string{"tenant:attacker", "tenant:quarantine"}, negated, + "the single allow ACL must negate every excluded value") + require.Equal(t, []string{util.KubeAllNamespacesFlag}, positive, + "the negation-only match must be intersected with the all-namespaces set") + + // The default drop must be same-direction and unconditional (no peer match), + // so the excluded namespaces have no allow path and fall through to it. + require.Equal(t, theAllow.Direction, theDrop.Direction, "drop must be the same direction as the allow") + require.Empty(t, tt.peerList(theDrop), "the default drop must be unconditional") + + // When a port is present it must be carried in the same allow decision, + // conjunctively with the negated tenant sets. + if len(tt.ports) > 0 { + require.EqualValues(t, 80, theAllow.DstPorts.Port, + "the port must render in the same allow ACL as the negated tenant sets") + } + }) + } +} + func TestAllowAllInternal(t *testing.T) { matchType := policies.SrcMatch tests := []struct { @@ -3474,3 +3892,249 @@ func TestTranslatePolicyNodeEgressPorts(t *testing.T) { require.NoError(t, err) require.Equal(t, []int32{5005, 2500}, npmNetPol.NodeEgressPorts) } + +// ipBlockPolicy builds an ingress NetworkPolicy that selects all pods in ns and admits the +// given ipBlock CIDR. +func ipBlockPolicy(name, ns, cidr string) *networkingv1.NetworkPolicy { + return &networkingv1.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: ns}, + Spec: networkingv1.NetworkPolicySpec{ + PodSelector: metav1.LabelSelector{}, + PolicyTypes: []networkingv1.PolicyType{networkingv1.PolicyTypeIngress}, + Ingress: []networkingv1.NetworkPolicyIngressRule{ + { + From: []networkingv1.NetworkPolicyPeer{ + {IPBlock: &networkingv1.IPBlock{CIDR: cidr}}, + }, + }, + }, + }, + } +} + +// TestTranslatePolicyNonCanonicalAllAddressesCIDR verifies that an ipBlock naming the +// all-addresses block with host bits set (e.g. "10.0.0.0/0") translates identically to the +// canonical "0.0.0.0/0". Rejecting it failed the whole policy, so neither the allow nor the +// default drop the policy implies was installed and the selected pods stayed unisolated. +func TestTranslatePolicyNonCanonicalAllAddressesCIDR(t *testing.T) { + t.Parallel() + + canonical, err := TranslatePolicy(ipBlockPolicy("victim", "default", "0.0.0.0/0"), false) + require.NoError(t, err) + + for _, cidr := range []string{"10.0.0.0/0", "255.255.255.255/0"} { + t.Run(cidr, func(t *testing.T) { + t.Parallel() + + npmNetPol, err := TranslatePolicy(ipBlockPolicy("victim", "default", cidr), false) + require.NoError(t, err, "a non-canonical all-addresses block must not fail translation") + require.NotNil(t, npmNetPol) + + // The policy must be indistinguishable from the canonical spelling: same + // ipset members (the 0.0.0.0/0 split) and the same ACLs. + require.Equal(t, canonical.RuleIPSets, npmNetPol.RuleIPSets) + require.Len(t, npmNetPol.ACLs, len(canonical.ACLs)) + + // Most importantly the default drop must exist, since its absence is what + // left the selected pods unisolated. + var dropACLs int + for i := range npmNetPol.ACLs { + if npmNetPol.ACLs[i].Target == policies.Dropped { + dropACLs++ + } + } + require.Equal(t, 1, dropACLs, "the policy's default drop must be installed") + }) + } +} + +// TestTranslatePolicyInvalidCIDRStillFails verifies the canonicalization did not weaken +// validation: a CIDR that is not IPv4 at all must still be rejected. +func TestTranslatePolicyInvalidCIDRStillFails(t *testing.T) { + t.Parallel() + + for _, cidr := range []string{"2001:db8::/32", "10.0.0.0/33", "not-a-cidr/0"} { + t.Run(cidr, func(t *testing.T) { + t.Parallel() + npmNetPol, err := TranslatePolicy(ipBlockPolicy("victim", "default", cidr), false) + require.ErrorIs(t, err, ErrUnsupportedIPAddress) + require.Nil(t, npmNetPol) + }) + } +} + +// nsExprPolicy builds a NetworkPolicy that selects all local pods and, for the given +// direction, admits peers whose namespace satisfies the single given matchExpression. +func nsExprPolicy(name, ns string, direction networkingv1.PolicyType, req metav1.LabelSelectorRequirement) *networkingv1.NetworkPolicy { + peer := networkingv1.NetworkPolicyPeer{ + NamespaceSelector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{req}, + }, + } + pol := &networkingv1.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: ns}, + Spec: networkingv1.NetworkPolicySpec{ + PodSelector: metav1.LabelSelector{}, + PolicyTypes: []networkingv1.PolicyType{direction}, + }, + } + if direction == networkingv1.PolicyTypeIngress { + pol.Spec.Ingress = []networkingv1.NetworkPolicyIngressRule{{From: []networkingv1.NetworkPolicyPeer{peer}}} + } else { + pol.Spec.Egress = []networkingv1.NetworkPolicyEgressRule{{To: []networkingv1.NetworkPolicyPeer{peer}}} + } + return pol +} + +// TestTranslatePolicyNegationOnlyOperators covers every operator that can produce a +// negation-only namespaceSelector, in both directions. +// +// A namespaceSelector selects pods in matching namespaces, and NPM's namespace sets hold +// pod IPs. A negated set match is satisfied by any address absent from that set, so an +// address that is not a pod in any namespace satisfies it too. Without a positive set to +// intersect with, the negation alone is the whole match and the rule admits non-pod +// addresses: on ingress a routable non-pod host reaching the pod directly, on egress the +// selected pod reaching arbitrary external hosts. +// +// Each case asserts the allow decision carries the all-namespaces anchor, which is the +// positive match that keeps the decision inside the pod domain. +func TestTranslatePolicyNegationOnlyOperators(t *testing.T) { + t.Parallel() + + operators := []struct { + name string + req metav1.LabelSelectorRequirement + excluded string + setType ipsets.SetType + }{ + { + name: "DoesNotExist", + req: metav1.LabelSelectorRequirement{Key: blockedLabelKey, Operator: metav1.LabelSelectorOpDoesNotExist}, + excluded: blockedLabelKey, + setType: ipsets.KeyLabelOfNamespace, + }, + { + name: "single-value NotIn", + req: metav1.LabelSelectorRequirement{Key: blockedLabelKey, Operator: metav1.LabelSelectorOpNotIn, Values: []string{"yes"}}, + excluded: "blocked:yes", + setType: ipsets.KeyValueLabelOfNamespace, + }, + { + name: "multi-value NotIn", + req: metav1.LabelSelectorRequirement{Key: blockedLabelKey, Operator: metav1.LabelSelectorOpNotIn, Values: []string{"yes", "maybe"}}, + excluded: "blocked:yes", + setType: ipsets.KeyValueLabelOfNamespace, + }, + } + + directions := []struct { + name string + direction networkingv1.PolicyType + matchType policies.MatchType + peerList func(*policies.ACLPolicy) []policies.SetInfo + }{ + {ingressName, networkingv1.PolicyTypeIngress, policies.SrcMatch, func(a *policies.ACLPolicy) []policies.SetInfo { return a.SrcList }}, + {egressName, networkingv1.PolicyTypeEgress, policies.DstMatch, func(a *policies.ACLPolicy) []policies.SetInfo { return a.DstList }}, + } + + for _, op := range operators { + for _, dir := range directions { + t.Run(op.name+"/"+dir.name, func(t *testing.T) { + t.Parallel() + + npmNetPol, err := TranslatePolicy(nsExprPolicy("victim", "default", dir.direction, op.req), false) + require.NoError(t, err) + + var allowACLs int + var theAllow *policies.ACLPolicy + for i := range npmNetPol.ACLs { + if npmNetPol.ACLs[i].Target == policies.Allowed { + allowACLs++ + theAllow = npmNetPol.ACLs[i] + } + } + // One decision only: every negation must be ANDed into it, never split + // into additive allow decisions. + require.Equal(t, 1, allowACLs, "there must be exactly one allow ACL") + require.NotNil(t, theAllow) + + peers := dir.peerList(theAllow) + anchor := policies.NewSetInfo(util.KubeAllNamespacesFlag, ipsets.KeyLabelOfNamespace, included, dir.matchType) + require.Contains(t, peers, anchor, + "a negation-only namespaceSelector must carry the all-namespaces anchor, "+ + "otherwise the negation alone also matches addresses that are not pods") + + // The exclusion itself must still be present and still negated. + require.Contains(t, peers, + policies.NewSetInfo(op.excluded, op.setType, nonIncluded, dir.matchType)) + + // Exactly one positive set: the anchor. Anything else positive would + // widen the decision beyond what the selector asked for. + var positives []string + for _, si := range peers { + if si.Included { + positives = append(positives, si.IPSet.Name) + } + } + require.Equal(t, []string{util.KubeAllNamespacesFlag}, positives) + }) + } + } +} + +// TestIPBlockExceptCanonicalizationKeepsEveryExcept locks the member packing now that except +// CIDRs are canonicalized first. Canonicalizing can turn an except into one of the two halves +// that 0.0.0.0/0 is split into, which takes a different branch of the packing loop and shortens +// the member list, so every other except must still survive that, whatever order they arrive in. +func TestIPBlockExceptCanonicalizationKeepsEveryExcept(t *testing.T) { + if util.IsWindowsDP() { + t.Skip("the Windows datapath refuses any except on this path") + } + + tests := []struct { + name string + cidr string + except []string + want []string + }{ + { + name: "except canonicalizes onto the lower half, listed last", + cidr: "0.0.0.0/0", + except: []string{exceptedClassA, "10.0.0.0/1"}, + want: []string{lowerHalfNomatch, "128.0.0.0/1", exceptedClassA + " nomatch"}, + }, + { + name: "same excepts in the other order", + cidr: "0.0.0.0/0", + except: []string{"10.0.0.0/1", exceptedClassA}, + want: []string{lowerHalfNomatch, "128.0.0.0/1", exceptedClassA + " nomatch"}, + }, + { + name: "a split-half except between two ordinary ones", + cidr: "0.0.0.0/0", + except: []string{exceptedClassA, "10.0.0.0/1", "9.0.0.0/8"}, + want: []string{lowerHalfNomatch, "128.0.0.0/1", exceptedClassA + " nomatch", "9.0.0.0/8 nomatch"}, + }, + { + name: "both halves reached by canonicalization", + cidr: "0.0.0.0/0", + except: []string{exceptedClassA, "250.0.0.0/1", "10.0.0.0/1"}, + want: []string{lowerHalfNomatch, "128.0.0.0/1 nomatch", exceptedClassA + " nomatch"}, + }, + { + name: "a non-canonical all-addresses block behaves the same", + cidr: "10.0.0.0/0", + except: []string{exceptedClassA, "10.0.0.0/1"}, + want: []string{lowerHalfNomatch, "128.0.0.0/1", exceptedClassA + " nomatch"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + set, err := ipBlockIPSet("p", defaultNS, policies.Ingress, 0, 0, + &networkingv1.IPBlock{CIDR: tt.cidr, Except: tt.except}) + require.NoError(t, err) + require.Equal(t, tt.want, set.Members) + }) + } +} diff --git a/npm/pkg/dataplane/policies/policymanager_linux_test.go b/npm/pkg/dataplane/policies/policymanager_linux_test.go index 8fa044e373a..06bd69ce90b 100644 --- a/npm/pkg/dataplane/policies/policymanager_linux_test.go +++ b/npm/pkg/dataplane/policies/policymanager_linux_test.go @@ -517,3 +517,60 @@ func TestUpdatingStaleChains(t *testing.T) { require.NoError(t, pMgr.AddPolicies([]*NPMNetworkPolicy{bothDirectionsNetPol}, nil)) assertStaleChainsContain(t, pMgr.staleChains, egressNetPolChain) } + +// TestNegationOnlyPeerRendersAnchor asserts how a negation-only namespace peer reaches the +// kernel. A negated set match (`! --match-set`) is satisfied by every address absent from that +// set, including addresses that are not pods at all, so an ACL whose peer list is only negations +// matches non-pod traffic. The all-namespaces anchor is what confines the decision to the pod +// domain, and this test pins that it renders as a positive `--match-set` in the same rule as the +// negation, in both directions. +func TestNegationOnlyPeerRendersAnchor(t *testing.T) { + anchor := ipsets.NewIPSetMetadata(util.KubeAllNamespacesFlag, ipsets.KeyLabelOfNamespace) + excluded := ipsets.NewIPSetMetadata("blocked", ipsets.KeyLabelOfNamespace) + + tests := []struct { + name string + direction Direction + matchType MatchType + matchArg string + }{ + {"ingress", Ingress, SrcMatch, "src"}, + {"egress", Egress, DstMatch, "dst"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + acl := &ACLPolicy{ + Target: Allowed, + Direction: tt.direction, + } + peers := []SetInfo{ + NewSetInfo(util.KubeAllNamespacesFlag, ipsets.KeyLabelOfNamespace, true, tt.matchType), + NewSetInfo("blocked", ipsets.KeyLabelOfNamespace, false, tt.matchType), + } + if tt.matchType == SrcMatch { + acl.SrcList = peers + } else { + acl.DstList = peers + } + + specs := strings.Join(iptablesRuleSpecs(acl), " ") + + // The anchor must render as a positive match, so only pod addresses satisfy it. + // Asserted by exact count: a bare NotEqual would also pass if it were absent. + positive := strings.Join([]string{util.IptablesMatchSetFlag, anchor.GetHashedName(), tt.matchArg}, " ") + require.Equal(t, 1, strings.Count(specs, positive), + "the all-namespaces anchor must render exactly once as a positive match-set") + + // The exclusion must remain negated. + negated := strings.Join([]string{util.IptablesNotFlag, util.IptablesMatchSetFlag, excluded.GetHashedName(), tt.matchArg}, " ") + require.Equal(t, 1, strings.Count(specs, negated), + "the excluded namespace label must render exactly once as a negated match-set") + + // Exactly two match-sets: the anchor and the exclusion. A negation-only rule, which + // is the shape that admits non-pod addresses, would have only one. + require.Equal(t, 2, strings.Count(specs, util.IptablesMatchSetFlag), + "a namespace peer must never render as a lone negated match") + }) + } +} diff --git a/npm/util/util.go b/npm/util/util.go index daefd4d1b4c..bf208ee5b19 100644 --- a/npm/util/util.go +++ b/npm/util/util.go @@ -363,6 +363,26 @@ func SliceToString(list []string) string { return strings.Join(list, SetPolicyDelimiter) } +// NormalizeCIDR returns the canonical form of an IPv4 CIDR, i.e. the block with its host +// bits cleared, so "10.0.0.0/0" becomes "0.0.0.0/0" and "10.1.2.3/24" becomes "10.1.2.0/24". +// It reports false when s is not an IPv4 CIDR. Callers must normalize before comparing a +// CIDR against a well-known block or handing it to the kernel, because a non-canonical +// spelling denotes the same block but does not compare equal and is not accepted by ipset. +func NormalizeCIDR(s string) (string, bool) { + _, network, err := net.ParseCIDR(s) + if err != nil || network.IP.To4() == nil || len(network.Mask) != net.IPv4len { + return "", false + } + return network.String(), true +} + +// IsIPV4 returns true when ip is an IPv4 address or an IPv4 CIDR block. +// +// Note this rejects a /0 block whose address text is not literally "0.0.0.0", even though such +// a block is valid and denotes the same addresses. Callers on the Linux ipBlock path must +// therefore canonicalize with NormalizeCIDR before validating, so a valid block is not refused +// on spelling alone. This function's behavior is deliberately left unchanged because it is also +// consumed by the Windows and NPM Lite paths, which are not in scope for these changes. func IsIPV4(ip string) bool { isIPBlock := strings.Contains(ip, "/") ipOnly := strings.Split(ip, "/") diff --git a/npm/util/util_test.go b/npm/util/util_test.go index af671eabd10..f16035b27be 100644 --- a/npm/util/util_test.go +++ b/npm/util/util_test.go @@ -514,3 +514,72 @@ func TestHashedNameGoldenVectors(t *testing.T) { require.Equal(t, want, GetHashedChainName(in), "GetHashedChainName(%q) golden vector", in) } } + +// Test CIDRs shared by the IsIPV4 and NormalizeCIDR cases below. +const ( + allIPv4CIDR = "0.0.0.0/0" + singleHostCIDR = "10.0.0.1/32" + canonicalNet24 = "10.1.2.0/24" +) + +// TestIsIPV4 pins the existing behavior of the shared classifier. It is deliberately left +// unchanged by these fixes because the Windows and NPM Lite paths also consume it, and those +// are out of scope. Note it refuses a /0 block that is not spelled "0.0.0.0" even though such a +// block is valid; the Linux ipBlock path therefore validates via NormalizeCIDR instead. +func TestIsIPV4(t *testing.T) { + valid := []string{ + "10.0.0.1", + "0.0.0.0", + "10.0.0.0/24", + allIPv4CIDR, + "10.1.2.3/24", + singleHostCIDR, + } + for _, ip := range valid { + require.True(t, IsIPV4(ip), "IsIPV4(%q) must be true", ip) + } + + invalid := []string{ + "", + "not-an-ip", + "10.0.0.256", + "10.0.0.0/33", + "10.0.0.0/", + "2001:db8::1", + "2001:db8::/32", + "::/0", + // a valid but non-canonical /0: refused on spelling, which is why the Linux + // ipBlock path canonicalizes before validating. + "10.0.0.0/0", + "255.255.255.255/0", + } + for _, ip := range invalid { + require.False(t, IsIPV4(ip), "IsIPV4(%q) must be false", ip) + } +} + +// TestNormalizeCIDR verifies that host bits are cleared, so callers can compare a CIDR +// against a well-known block and hand the canonical form to the kernel. +func TestNormalizeCIDR(t *testing.T) { + canonical := map[string]string{ + allIPv4CIDR: allIPv4CIDR, + "10.0.0.0/0": allIPv4CIDR, + "255.255.255.255/0": allIPv4CIDR, + "10.0.0.0/1": "0.0.0.0/1", + "200.0.0.0/1": "128.0.0.0/1", + "10.1.2.3/24": canonicalNet24, + canonicalNet24: canonicalNet24, + singleHostCIDR: singleHostCIDR, + } + for in, want := range canonical { + got, ok := NormalizeCIDR(in) + require.True(t, ok, "NormalizeCIDR(%q) must succeed", in) + require.Equal(t, want, got, "NormalizeCIDR(%q)", in) + } + + for _, in := range []string{"", "10.0.0.1", "not-a-cidr", "10.0.0.0/33", "2001:db8::/32", "::/0"} { + got, ok := NormalizeCIDR(in) + require.False(t, ok, "NormalizeCIDR(%q) must fail", in) + require.Empty(t, got) + } +}