diff --git a/npm/pkg/controlplane/controllers/common/cache.go b/npm/pkg/controlplane/controllers/common/cache.go index ad5cba85bd6..92bdd30aebf 100644 --- a/npm/pkg/controlplane/controllers/common/cache.go +++ b/npm/pkg/controlplane/controllers/common/cache.go @@ -86,6 +86,15 @@ func (c *Cache) GetNamespaceLabel(namespace, labelkey string) string { return "" } +// GetNamespaceLabels distinguishes a missing namespace from one with no labels. +func (c *Cache) GetNamespaceLabels(namespace string) (map[string]string, bool) { + ns, ok := c.NsMap[namespace] + if !ok || ns == nil { + return nil, false + } + return ns.LabelsMap, true +} + func (c *Cache) GetSetMap() map[string]string { return c.SetMap } diff --git a/npm/pkg/controlplane/controllers/v2/namespaceController.go b/npm/pkg/controlplane/controllers/v2/namespaceController.go index 654609ced2f..eceb9d76a65 100644 --- a/npm/pkg/controlplane/controllers/v2/namespaceController.go +++ b/npm/pkg/controlplane/controllers/v2/namespaceController.go @@ -465,12 +465,12 @@ func (nsc *NamespaceController) cleanDeletedNamespace(cachedNsKey string) error cachedNsObj.RemoveLabelsWithKey(nsLabelKey) } - allNamespacesSet := ipsets.NewIPSetMetadata(util.KubeAllNamespacesFlag, ipsets.KeyLabelOfNamespace) + allNamespacesSet := ipsets.NewIPSetMetadata(util.KubeAllNamespacesFlagV2, ipsets.KeyLabelOfNamespace) toBeDeletedCachedKey := []*ipsets.IPSetMetadata{ipsets.NewIPSetMetadata(cachedNsKey, ipsets.Namespace)} // Delete the namespace from all-namespace ipset list. if err = nsc.dp.RemoveFromList(allNamespacesSet, toBeDeletedCachedKey); err != nil { - metrics.SendErrorLogAndMetric(util.NSID, "[DeleteNamespace] Error: failed to delete namespace %s from ipset list %s with err: %v", cachedNsKey, util.KubeAllNamespacesFlag, err) + metrics.SendErrorLogAndMetric(util.NSID, "[DeleteNamespace] Error: failed to delete namespace %s from ipset list %s with err: %v", cachedNsKey, util.KubeAllNamespacesFlagV2, err) return fmt.Errorf("failed to remove from list during clean deleted namespace %w", err) } diff --git a/npm/pkg/controlplane/controllers/v2/namespace_selector_windows_test.go b/npm/pkg/controlplane/controllers/v2/namespace_selector_windows_test.go new file mode 100644 index 00000000000..9ae55c08b8f --- /dev/null +++ b/npm/pkg/controlplane/controllers/v2/namespace_selector_windows_test.go @@ -0,0 +1,74 @@ +package controllers + +import ( + "fmt" + "testing" + + "github.com/Azure/azure-container-networking/npm/pkg/controlplane/translation" + dpmocks "github.com/Azure/azure-container-networking/npm/pkg/dataplane/mocks" + "github.com/golang/mock/gomock" + "github.com/stretchr/testify/require" + networkingv1 "k8s.io/api/networking/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestWindowsFullNPMNamespaceNegationIsNotSubmitted(t *testing.T) { + const ( + podLabelKey = "app" + namespaceName = "test" + ) + for _, requirement := range []metav1.LabelSelectorRequirement{ + {Key: namespaceSelectorLabelKey, Operator: metav1.LabelSelectorOpNotIn, Values: []string{"a"}}, + {Key: namespaceSelectorLabelKey, Operator: metav1.LabelSelectorOpNotIn, Values: []string{"a", "b"}}, + {Key: namespaceSelectorLabelKey, Operator: metav1.LabelSelectorOpDoesNotExist}, + } { + for _, direction := range []networkingv1.PolicyType{networkingv1.PolicyTypeIngress, networkingv1.PolicyTypeEgress} { + for _, combined := range []bool{false, true} { + t.Run(fmt.Sprintf("%s/%s/%v/combined=%t", direction, requirement.Operator, requirement.Values, combined), func(t *testing.T) { + peer := networkingv1.NetworkPolicyPeer{ + NamespaceSelector: &metav1.LabelSelector{MatchExpressions: []metav1.LabelSelectorRequirement{requirement}}, + } + if combined { + peer.PodSelector = &metav1.LabelSelector{MatchLabels: map[string]string{podLabelKey: "client"}} + } + policy := &networkingv1.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: "namespace-selector", Namespace: namespaceName, ResourceVersion: "1"}, + Spec: networkingv1.NetworkPolicySpec{PolicyTypes: []networkingv1.PolicyType{direction}}, + } + if direction == networkingv1.PolicyTypeIngress { + policy.Spec.Ingress = []networkingv1.NetworkPolicyIngressRule{{From: []networkingv1.NetworkPolicyPeer{peer}}} + } else { + policy.Spec.Egress = []networkingv1.NetworkPolicyEgressRule{{To: []networkingv1.NetworkPolicyPeer{peer}}} + } + translated, err := translation.TranslatePolicy(policy, false) + require.ErrorIs(t, err, translation.ErrUnsupportedNegativeMatch) + require.Nil(t, translated) + + ctrl := gomock.NewController(t) + dp := dpmocks.NewMockGenericDataplane(ctrl) + f := newNetPolQueueFixture(t, policy, dp, false) + c := f.netPolController + c.addNetworkPolicy(policy) + require.True(t, c.processNextWorkItem()) + require.Empty(t, c.rawNpSpecMap) + require.Zero(t, c.workqueue.Len()) + require.Zero(t, c.workqueue.NumRequeues(getKey(policy, t))) + + corrected := policy.DeepCopy() + corrected.ResourceVersion = "2" + positive := metav1.LabelSelectorRequirement{Key: namespaceSelectorLabelKey, Operator: metav1.LabelSelectorOpExists} + if direction == networkingv1.PolicyTypeIngress { + corrected.Spec.Ingress[0].From[0].NamespaceSelector.MatchExpressions[0] = positive + } else { + corrected.Spec.Egress[0].To[0].NamespaceSelector.MatchExpressions[0] = positive + } + require.NoError(t, f.kubeInformer.Networking().V1().NetworkPolicies().Informer().GetIndexer().Update(corrected)) + dp.EXPECT().UpdatePolicy(gomock.Any()).Return(nil).Times(1) + c.updateNetworkPolicy(policy, corrected) + require.True(t, c.processNextWorkItem()) + require.Equal(t, &corrected.Spec, c.rawNpSpecMap[getKey(corrected, t)]) + }) + } + } + } +} diff --git a/npm/pkg/controlplane/controllers/v2/networkPolicyController.go b/npm/pkg/controlplane/controllers/v2/networkPolicyController.go index 31ed83b5067..520a1553c0b 100644 --- a/npm/pkg/controlplane/controllers/v2/networkPolicyController.go +++ b/npm/pkg/controlplane/controllers/v2/networkPolicyController.go @@ -186,6 +186,10 @@ func (c *NetworkPolicyController) processNextWorkItem() bool { // Run the syncNetPol, passing it the namespace/name string of the // network policy resource to be synced. if err := c.syncNetPol(key); err != nil { + if errors.Is(err, errNetPolTranslationFailure) { + c.workqueue.Forget(obj) + return fmt.Errorf("error syncing '%s': %w; waiting for a policy change", key, err) + } // Put the item back on the workqueue to handle any transient errors. c.workqueue.AddRateLimited(key) return fmt.Errorf("error syncing '%s': %w, requeuing", key, err) @@ -291,8 +295,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, c.npmLiteToggle) { + 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 +304,16 @@ 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 + // Translation depends only on the policy and fixed controller mode. Full NPM + // reports deterministic failures without retrying or caching an unapplied spec; + // the informer queues changed resource versions. Dataplane errors below remain + // retryable, and Lite keeps its existing error handling. The worker reports errors. + if !c.npmLiteToggle { + return metrics.NoOp, fmt.Errorf("%w %s/%s: %w", + errNetPolTranslationFailure, netPolObj.Namespace, netPolObj.Name, err) + } + return metrics.NoOp, fmt.Errorf("translating network policy %s/%s: %w", + netPolObj.Namespace, netPolObj.Name, err) } _, policyExisted := c.rawNpSpecMap[netpolKey] @@ -358,3 +369,19 @@ 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 +// stay suppressed with a warning; other failures must be reported without recording success. +func isUnsupportedTranslationErr(err error, npmLiteToggle bool) bool { + if errors.Is(err, util.ErrInvalidCIDR) || errors.Is(err, translation.ErrInvalidIPBlockExcept) { + return false + } + // Full NPM supplies a typed cause; only Lite retains unclassified address errors. + unsupportedAddress := errors.Is(err, util.ErrUnsupportedIPFamily) || + (npmLiteToggle && errors.Is(err, translation.ErrUnsupportedIPAddress)) + return isUnsupportedWindowsTranslationErr(err) || + (util.IsWindowsDP() && unsupportedAddress) || + // 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_retry_test.go b/npm/pkg/controlplane/controllers/v2/networkPolicyController_retry_test.go new file mode 100644 index 00000000000..488e60a0e49 --- /dev/null +++ b/npm/pkg/controlplane/controllers/v2/networkPolicyController_retry_test.go @@ -0,0 +1,213 @@ +package controllers + +import ( + "fmt" + "testing" + + "github.com/Azure/azure-container-networking/npm/pkg/controlplane/translation" + dpmocks "github.com/Azure/azure-container-networking/npm/pkg/dataplane/mocks" + "github.com/Azure/azure-container-networking/npm/util" + "github.com/golang/mock/gomock" + "github.com/stretchr/testify/require" + networkingv1 "k8s.io/api/networking/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/util/workqueue" +) + +const namespaceSelectorLabelKey = "tenant" + +func newNetPolQueueFixture(t *testing.T, policy *networkingv1.NetworkPolicy, dp *dpmocks.MockGenericDataplane, npmLite bool) *netPolFixture { + t.Helper() + f := newNetPolFixture(t) + f.netPolLister = append(f.netPolLister, policy) + f.kubeobjects = append(f.kubeobjects, policy) + f.newNetPolController(nil, dp, npmLite) + f.netPolController.workqueue.ShutDown() + // Zero-delay retries make incorrect requeueing observable without sleeps. + f.netPolController.workqueue = workqueue.NewTypedRateLimitingQueue[any](workqueue.NewTypedItemFastSlowRateLimiter[any](0, 0, 1)) + t.Cleanup(f.netPolController.workqueue.ShutDown) + return f +} + +func TestFullNPMTranslationFailureWaitsForPolicyChange(t *testing.T) { + oversized := netPolWithCIDR("192.0.2.0/24") + selector := &metav1.LabelSelector{} + for i := 0; i < 19; i++ { + selector.MatchExpressions = append(selector.MatchExpressions, metav1.LabelSelectorRequirement{ + Key: fmt.Sprintf("key%d", i), Operator: metav1.LabelSelectorOpIn, Values: []string{"a", "b"}, + }) + } + oversized.Spec.Ingress[0].From = []networkingv1.NetworkPolicyPeer{{NamespaceSelector: selector}} + invalidWithExcept := netPolWithCIDR("192.0.2.0/33") + invalidWithExcept.Spec.Ingress[0].From[0].IPBlock.Except = []string{"192.0.2.1/32"} + unknownOperator := netPolWithCIDR("192.0.2.0/24") + unknownOperator.Spec.Ingress[0].From = []networkingv1.NetworkPolicyPeer{{ + NamespaceSelector: &metav1.LabelSelector{MatchExpressions: []metav1.LabelSelectorRequirement{{ + Key: namespaceSelectorLabelKey, Operator: "Unknown", + }}}, + }} + + tests := []struct { + name string + policy *networkingv1.NetworkPolicy + cause error + }{ + {"malformed CIDR", netPolWithCIDR("192.0.2.0/33"), util.ErrInvalidCIDR}, + {"malformed CIDR with Except", invalidWithExcept, util.ErrInvalidCIDR}, + {"selector expansion", oversized, translation.ErrTooManyFlattenedSelectors}, + {"unsupported operator", unknownOperator, translation.ErrUnsupportedMatchExpressionOperator}, + } + if !util.IsWindowsDP() { + tests = append(tests, struct { + name string + policy *networkingv1.NetworkPolicy + cause error + }{"unsupported family", netPolWithCIDR("2001:db8::/32"), util.ErrUnsupportedIPFamily}) + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + test.policy.ResourceVersion = "1" + ctrl := gomock.NewController(t) + dp := dpmocks.NewMockGenericDataplane(ctrl) + f := newNetPolQueueFixture(t, test.policy, dp, false) + c := f.netPolController + key := getKey(test.policy, t) + + _, err := c.syncAddAndUpdateNetPol(test.policy) + require.ErrorIs(t, err, test.cause) + require.ErrorIs(t, err, errNetPolTranslationFailure) + require.ErrorContains(t, err, key) + require.Empty(t, c.rawNpSpecMap) + + c.addNetworkPolicy(test.policy) + require.Equal(t, 1, c.workqueue.Len()) + require.True(t, c.processNextWorkItem()) + require.Zero(t, c.workqueue.NumRequeues(key)) + require.Zero(t, c.workqueue.Len()) + require.Empty(t, c.rawNpSpecMap) + + c.updateNetworkPolicy(test.policy, test.policy.DeepCopy()) + require.Zero(t, c.workqueue.Len(), "an informer resync must not repeat the rejection") + + corrected := test.policy.DeepCopy() + corrected.ResourceVersion = "2" + corrected.Spec = netPolWithCIDR("10.0.0.0/0").Spec + require.NoError(t, f.kubeInformer.Networking().V1().NetworkPolicies().Informer().GetIndexer().Update(corrected)) + dp.EXPECT().UpdatePolicy(gomock.Any()).Return(nil).Times(1) + c.updateNetworkPolicy(test.policy, corrected) + require.Equal(t, 1, c.workqueue.Len()) + require.True(t, c.processNextWorkItem()) + require.Equal(t, &corrected.Spec, c.rawNpSpecMap[key]) + require.Zero(t, c.workqueue.NumRequeues(key)) + require.Zero(t, c.workqueue.Len()) + }) + } +} + +func TestFullNPMDataplaneFailureStillRetries(t *testing.T) { + policy := netPolWithCIDR("10.0.0.0/0") + ctrl := gomock.NewController(t) + dp := dpmocks.NewMockGenericDataplane(ctrl) + f := newNetPolQueueFixture(t, policy, dp, false) + c := f.netPolController + key := getKey(policy, t) + + gomock.InOrder( + dp.EXPECT().UpdatePolicy(gomock.Any()).Return(fmt.Errorf("programming policy: %w", translation.ErrUnsupportedIPAddress)), + dp.EXPECT().UpdatePolicy(gomock.Any()).Return(nil), + ) + c.addNetworkPolicy(policy) + require.True(t, c.processNextWorkItem()) + require.Equal(t, 1, c.workqueue.NumRequeues(key)) + require.Equal(t, 1, c.workqueue.Len()) + require.Empty(t, c.rawNpSpecMap) + + require.True(t, c.processNextWorkItem()) + require.Zero(t, c.workqueue.NumRequeues(key)) + require.Zero(t, c.workqueue.Len()) + require.Equal(t, &policy.Spec, c.rawNpSpecMap[key]) +} + +func TestFullNPMRejectedUpdateRetainsAppliedPolicyUntilDeletion(t *testing.T) { + policy := netPolWithCIDR("192.0.2.0/24") + policy.ResourceVersion = "1" + ctrl := gomock.NewController(t) + dp := dpmocks.NewMockGenericDataplane(ctrl) + f := newNetPolQueueFixture(t, policy, dp, false) + c := f.netPolController + key := getKey(policy, t) + + dp.EXPECT().UpdatePolicy(gomock.Any()).Return(nil).Times(1) + c.addNetworkPolicy(policy) + require.True(t, c.processNextWorkItem()) + require.Equal(t, &policy.Spec, c.rawNpSpecMap[key]) + + rejected := policy.DeepCopy() + rejected.ResourceVersion = "2" + rejected.Spec.Ingress[0].From[0].IPBlock.CIDR = "192.0.2.0/33" + indexer := f.kubeInformer.Networking().V1().NetworkPolicies().Informer().GetIndexer() + require.NoError(t, indexer.Update(rejected)) + c.updateNetworkPolicy(policy, rejected) + require.Equal(t, 1, c.workqueue.Len()) + require.True(t, c.processNextWorkItem()) + require.Equal(t, &policy.Spec, c.rawNpSpecMap[key]) + require.Zero(t, c.workqueue.NumRequeues(key)) + require.Zero(t, c.workqueue.Len()) + + require.NoError(t, indexer.Delete(rejected)) + dp.EXPECT().RemovePolicy(key).Return(nil).Times(1) + c.deleteNetworkPolicy(rejected) + require.Equal(t, 1, c.workqueue.Len()) + require.True(t, c.processNextWorkItem()) + require.Empty(t, c.rawNpSpecMap) + require.Zero(t, c.workqueue.Len()) +} + +func TestFullNPMRejectedCreateCanBeDeleted(t *testing.T) { + policy := netPolWithCIDR("invalid") + ctrl := gomock.NewController(t) + dp := dpmocks.NewMockGenericDataplane(ctrl) + f := newNetPolQueueFixture(t, policy, dp, false) + c := f.netPolController + + c.addNetworkPolicy(policy) + require.True(t, c.processNextWorkItem()) + require.Zero(t, c.workqueue.Len()) + require.Empty(t, c.rawNpSpecMap) + + require.NoError(t, f.kubeInformer.Networking().V1().NetworkPolicies().Informer().GetIndexer().Delete(policy)) + c.deleteNetworkPolicy(policy) + require.Equal(t, 1, c.workqueue.Len()) + require.True(t, c.processNextWorkItem()) + require.Empty(t, c.rawNpSpecMap) + require.Zero(t, c.workqueue.Len()) +} + +func TestLiteTranslationFailureHandlingIsUnchanged(t *testing.T) { + policy := netPolWithCIDR("invalid") + ctrl := gomock.NewController(t) + dp := dpmocks.NewMockGenericDataplane(ctrl) + f := newNetPolQueueFixture(t, policy, dp, true) + c := f.netPolController + key := getKey(policy, t) + + _, err := c.syncAddAndUpdateNetPol(policy) + if util.IsWindowsDP() { + require.NoError(t, err, "the legacy Lite direct-address limitation stays suppressed") + } else { + require.ErrorIs(t, err, util.ErrInvalidCIDR) + require.NotErrorIs(t, err, errNetPolTranslationFailure) + } + + c.addNetworkPolicy(policy) + require.True(t, c.processNextWorkItem()) + require.Empty(t, c.rawNpSpecMap) + if util.IsWindowsDP() { + require.Zero(t, c.workqueue.NumRequeues(key)) + require.Zero(t, c.workqueue.Len()) + } else { + require.Equal(t, 1, c.workqueue.NumRequeues(key)) + require.Equal(t, 1, c.workqueue.Len()) + } +} diff --git a/npm/pkg/controlplane/controllers/v2/networkPolicyController_test.go b/npm/pkg/controlplane/controllers/v2/networkPolicyController_test.go index d14f6f67f12..14760119170 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,128 @@ 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) + if util.IsWindowsDP() { + require.NoError(t, err, "an unsupported Windows address must stay suppressed") + } else { + require.ErrorIs(t, err, translation.ErrUnsupportedIPAddress) + require.ErrorIs(t, err, util.ErrUnsupportedIPFamily) + require.ErrorIs(t, err, errNetPolTranslationFailure) + } + + // The policy must not be recorded as applied, so a later policy change 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") +} + +func TestUnsupportedAddressClassificationIsPlatformSpecific(t *testing.T) { + for _, test := range []struct { + name string + err error + npmLite bool + want bool + }{ + {"full unclassified address", translation.ErrUnsupportedIPAddress, false, false}, + {"Lite unclassified address", translation.ErrUnsupportedIPAddress, true, util.IsWindowsDP()}, + {"full unsupported family", fmt.Errorf("%w: %w", translation.ErrUnsupportedIPAddress, util.ErrUnsupportedIPFamily), false, util.IsWindowsDP()}, + {"Lite unsupported family", fmt.Errorf("%w: %w", translation.ErrUnsupportedIPAddress, util.ErrUnsupportedIPFamily), true, util.IsWindowsDP()}, + {"full malformed CIDR", fmt.Errorf("%w: %w", translation.ErrUnsupportedIPAddress, util.ErrInvalidCIDR), false, false}, + {"Lite typed malformed CIDR", fmt.Errorf("%w: %w", translation.ErrUnsupportedIPAddress, util.ErrInvalidCIDR), true, false}, + {"full invalid exclusion", fmt.Errorf("%w: %w", translation.ErrUnsupportedIPAddress, translation.ErrInvalidIPBlockExcept), false, false}, + {"Lite typed invalid exclusion", fmt.Errorf("%w: %w", translation.ErrUnsupportedIPAddress, translation.ErrInvalidIPBlockExcept), true, false}, + } { + t.Run(test.name, func(t *testing.T) { + require.Equal(t, test.want, isUnsupportedTranslationErr(test.err, test.npmLite)) + }) + } +} diff --git a/npm/pkg/controlplane/controllers/v2/podController.go b/npm/pkg/controlplane/controllers/v2/podController.go index 3a3e193058a..568ac7525b8 100644 --- a/npm/pkg/controlplane/controllers/v2/podController.go +++ b/npm/pkg/controlplane/controllers/v2/podController.go @@ -37,7 +37,7 @@ const ( updateEvent string = "UPDATE" ) -var kubeAllNamespaces = &ipsets.IPSetMetadata{Name: util.KubeAllNamespacesFlag, Type: ipsets.KeyLabelOfNamespace} +var kubeAllNamespaces = &ipsets.IPSetMetadata{Name: util.KubeAllNamespacesFlagV2, Type: ipsets.KeyLabelOfNamespace} type PodController struct { podLister corelisters.PodLister diff --git a/npm/pkg/controlplane/translation/acl_budget_test.go b/npm/pkg/controlplane/translation/acl_budget_test.go new file mode 100644 index 00000000000..dc542a604f1 --- /dev/null +++ b/npm/pkg/controlplane/translation/acl_budget_test.go @@ -0,0 +1,91 @@ +package translation + +import ( + "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 TestPolicyACLBudgetDirections(t *testing.T) { + makePorts := func(count int) []networkingv1.NetworkPolicyPort { + ports := make([]networkingv1.NetworkPolicyPort, count) + for i := range ports { + port := intstr.FromInt(i + 1) + ports[i] = networkingv1.NetworkPolicyPort{Port: &port} + } + return ports + } + for _, dual := range []bool{false, true} { + for _, direction := range []networkingv1.PolicyType{networkingv1.PolicyTypeIngress, networkingv1.PolicyTypeEgress} { + for _, over := range []bool{false, true} { + name := string(direction) + if dual { + name += "-dual" + } + if over { + name += "-over" + } + t.Run(name, func(t *testing.T) { + portCount := maxACLsPerPolicy - 1 + policyTypes := []networkingv1.PolicyType{direction} + if dual { + portCount -= 2 // One allow and one drop for the other direction. + } + if over { + portCount++ + } + policy := &networkingv1.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: "boundary", Namespace: defaultNS}, + Spec: networkingv1.NetworkPolicySpec{ + PolicyTypes: policyTypes, + }, + } + if direction == networkingv1.PolicyTypeIngress { + policy.Spec.Ingress = []networkingv1.NetworkPolicyIngressRule{{Ports: makePorts(portCount)}} + if dual { + policy.Spec.PolicyTypes = append(policy.Spec.PolicyTypes, networkingv1.PolicyTypeEgress) + policy.Spec.Egress = []networkingv1.NetworkPolicyEgressRule{{Ports: makePorts(1)}} + } + } else { + policy.Spec.Egress = []networkingv1.NetworkPolicyEgressRule{{Ports: makePorts(portCount)}} + if dual { + policy.Spec.PolicyTypes = append(policy.Spec.PolicyTypes, networkingv1.PolicyTypeIngress) + policy.Spec.Ingress = []networkingv1.NetworkPolicyIngressRule{{Ports: makePorts(1)}} + } + } + got, err := TranslatePolicy(policy, false) + if over { + require.ErrorIs(t, err, ErrTooManyACLs) + require.Nil(t, got) + } else { + require.NoError(t, err) + require.Len(t, got.ACLs, maxACLsPerPolicy) + } + }) + } + } + } +} + +func TestDefaultDropDoesNotExceedACLBudget(t *testing.T) { + for _, direction := range []policies.Direction{policies.Ingress, policies.Egress} { + t.Run(string(direction), func(t *testing.T) { + policy := policies.NewNPMNetworkPolicy("full", defaultNS) + for i := 0; i < maxACLsPerPolicy; i++ { + policy.ACLs = append(policy.ACLs, policies.NewACLPolicy(policies.Allowed, direction)) + } + var err error + if direction == policies.Ingress { + err = ingressPolicy(policy, "full", nil, false) + } else { + err = egressPolicy(policy, "full", nil, false) + } + require.ErrorIs(t, err, ErrTooManyACLs) + require.Len(t, policy.ACLs, maxACLsPerPolicy) + }) + } +} diff --git a/npm/pkg/controlplane/translation/cidr_errors_test.go b/npm/pkg/controlplane/translation/cidr_errors_test.go new file mode 100644 index 00000000000..80c935e2eb0 --- /dev/null +++ b/npm/pkg/controlplane/translation/cidr_errors_test.go @@ -0,0 +1,84 @@ +package translation + +import ( + "testing" + + "github.com/Azure/azure-container-networking/npm/pkg/dataplane/policies" + "github.com/Azure/azure-container-networking/npm/util" + "github.com/stretchr/testify/require" + networkingv1 "k8s.io/api/networking/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestIPBlockNormalizationErrorCauses(t *testing.T) { + const ( + ipv6CIDR = "2001:db8:2::/48" + malformedCIDR = "invalid" + allAddresses = "0.0.0.0/0" + hostBitsZero = "10.0.0.0/0" + privateCIDR = "10.0.0.0/8" + outsideCIDR = "192.0.2.0/24" + ) + for _, test := range []struct { + name string + block networkingv1.IPBlock + cause error + windowsExceptFailure bool + }{ + {"invalid prefix", networkingv1.IPBlock{CIDR: "192.0.2.0/33"}, util.ErrInvalidCIDR, false}, + {"bare address", networkingv1.IPBlock{CIDR: "192.0.2.1"}, util.ErrInvalidCIDR, false}, + {"IPv6 prefix", networkingv1.IPBlock{CIDR: ipv6CIDR}, util.ErrUnsupportedIPFamily, false}, + {"mapped IPv6 prefix", networkingv1.IPBlock{CIDR: "::ffff:192.0.2.0/120"}, util.ErrUnsupportedIPFamily, false}, + {"invalid parent with exclusion", networkingv1.IPBlock{CIDR: "192.0.2.0/33", Except: []string{"192.0.2.1/32"}}, util.ErrInvalidCIDR, false}, + {"IPv6 parent with exclusion", networkingv1.IPBlock{CIDR: ipv6CIDR, Except: []string{"2001:db8:2::1/128"}}, util.ErrUnsupportedIPFamily, false}, + {"invalid parent and exclusion", networkingv1.IPBlock{CIDR: malformedCIDR, Except: []string{malformedCIDR}}, util.ErrInvalidCIDR, false}, + {"invalid exclusion", networkingv1.IPBlock{CIDR: enclosingCIDR, Except: []string{malformedCIDR}}, util.ErrInvalidCIDR, true}, + {"IPv6 exclusion", networkingv1.IPBlock{CIDR: enclosingCIDR, Except: []string{ipv6CIDR}}, util.ErrUnsupportedIPFamily, true}, + {"all-addresses exclusion", networkingv1.IPBlock{CIDR: allAddresses, Except: []string{allAddresses}}, ErrInvalidIPBlockExcept, true}, + {"noncanonical all-addresses exclusion", networkingv1.IPBlock{CIDR: hostBitsZero, Except: []string{hostBitsZero}}, ErrInvalidIPBlockExcept, true}, + {"equal exclusion", networkingv1.IPBlock{CIDR: privateCIDR, Except: []string{privateCIDR}}, ErrInvalidIPBlockExcept, true}, + {"noncanonical equal exclusion", networkingv1.IPBlock{CIDR: privateCIDR, Except: []string{"10.1.2.3/8"}}, ErrInvalidIPBlockExcept, true}, + {"broader exclusion", networkingv1.IPBlock{CIDR: "10.1.0.0/16", Except: []string{privateCIDR}}, ErrInvalidIPBlockExcept, true}, + {"outside exclusion", networkingv1.IPBlock{CIDR: privateCIDR, Except: []string{outsideCIDR}}, ErrInvalidIPBlockExcept, true}, + } { + t.Run(test.name, func(t *testing.T) { + unsupportedExcept := util.IsWindowsDP() && test.windowsExceptFailure + before := test.block.DeepCopy() + set, info, err := ipBlockRule("normalization", defaultNS, policies.Ingress, policies.SrcMatch, 0, 0, &test.block) + require.Nil(t, set) + require.Equal(t, policies.SetInfo{}, info) + require.Equal(t, before, &test.block) + + if unsupportedExcept { + require.ErrorIs(t, err, ErrUnsupportedExceptCIDR) + } else { + require.ErrorIs(t, err, ErrUnsupportedIPAddress) + require.ErrorIs(t, err, test.cause) + require.NotErrorIs(t, err, ErrUnsupportedExceptCIDR) + } + + for _, direction := range []networkingv1.PolicyType{networkingv1.PolicyTypeIngress, networkingv1.PolicyTypeEgress} { + policy := &networkingv1.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: "normalization", Namespace: defaultNS}, + Spec: networkingv1.NetworkPolicySpec{ + PolicyTypes: []networkingv1.PolicyType{direction}, + }, + } + peers := []networkingv1.NetworkPolicyPeer{{IPBlock: &test.block}} + if direction == networkingv1.PolicyTypeIngress { + policy.Spec.Ingress = []networkingv1.NetworkPolicyIngressRule{{From: peers}} + } else { + policy.Spec.Egress = []networkingv1.NetworkPolicyEgressRule{{To: peers}} + } + translated, policyErr := TranslatePolicy(policy, false) + require.Nil(t, translated) + if unsupportedExcept { + require.ErrorIs(t, policyErr, ErrUnsupportedExceptCIDR) + } else { + require.ErrorIs(t, policyErr, ErrUnsupportedIPAddress) + require.ErrorIs(t, policyErr, test.cause) + } + } + }) + } +} diff --git a/npm/pkg/controlplane/translation/namespace_anchor_test.go b/npm/pkg/controlplane/translation/namespace_anchor_test.go new file mode 100644 index 00000000000..09c01befa1d --- /dev/null +++ b/npm/pkg/controlplane/translation/namespace_anchor_test.go @@ -0,0 +1,39 @@ +package translation + +import ( + "testing" + + "github.com/Azure/azure-container-networking/npm/pkg/dataplane/ipsets" + "github.com/Azure/azure-container-networking/npm/pkg/dataplane/policies" + "github.com/Azure/azure-container-networking/npm/util" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestNamespaceAnchorDoesNotAliasLabelSets(t *testing.T) { + for _, requirement := range []metav1.LabelSelectorRequirement{ + {Key: "all-namespaces", Operator: metav1.LabelSelectorOpDoesNotExist}, + {Key: "all-namespaces", Operator: metav1.LabelSelectorOpNotIn, Values: []string{"flagged"}}, + {Key: "all", Operator: metav1.LabelSelectorOpNotIn, Values: []string{"namespaces"}}, + } { + t.Run(requirement.Key+"/"+string(requirement.Operator), func(t *testing.T) { + sets, matches := nameSpaceSelector(policies.SrcMatch, &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{requirement}, + }) + require.Len(t, sets, 2) + require.Len(t, matches, 2) + require.NotEqual(t, sets[0].Metadata.GetPrefixName(), sets[1].Metadata.GetPrefixName()) + require.NotEqual(t, sets[0].Metadata.GetHashedName(), sets[1].Metadata.GetHashedName()) + name := requirement.Key + setType := ipsets.KeyLabelOfNamespace + if requirement.Operator == metav1.LabelSelectorOpNotIn { + name = util.GetIpSetFromLabelKV(requirement.Key, requirement.Values[0]) + setType = ipsets.KeyValueLabelOfNamespace + } + require.ElementsMatch(t, []policies.SetInfo{ + policies.NewSetInfo(name, setType, false, policies.SrcMatch), + policies.NewSetInfo(util.KubeAllNamespacesFlagV2, ipsets.KeyLabelOfNamespace, true, policies.SrcMatch), + }, matches) + }) + } +} diff --git a/npm/pkg/controlplane/translation/parseSelector.go b/npm/pkg/controlplane/translation/parseSelector.go index 447d1283058..f723a962224 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,10 @@ func flattenNameSpaceSelector(nsSelector *metav1.LabelSelector) ([]metav1.LabelS return []metav1.LabelSelector{}, nil } + if _, _, err := namespaceSelectorWork(nsSelector); err != nil { + return nil, err + } + if len(nsSelector.MatchExpressions) == 0 { return []metav1.LabelSelector{*nsSelector}, nil } @@ -70,46 +107,93 @@ 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): - for _, v := range req.Values { - if !isValidLabelValue(v) { - return nil, ErrInvalidMatchExpressionValues - } - } - + case req.Operator == metav1.LabelSelectorOpIn: if len(req.Values) == 1 { // 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) == 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 + // Exists and DoesNotExist do not carry values. 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) } @@ -117,11 +201,73 @@ func flattenNameSpaceSelector(nsSelector *metav1.LabelSelector) ([]metav1.LabelS return flatNsSelectors, nil } -// zipMatchExprs helps with zipping a given matchExpr with given baseLabelSelectors -// this func will loop over each baseSelector in the slice, -// deepCopies each baseSelector, combines with given matchExpr by looping over each value -// and creating a new LabelSelector with given baseSelector and value matchExpr -// then returns a new slice of these zipped LabelSelectors +// namespaceSelectorWork counts expansion before allocating selectors, sets, or ACLs. +func namespaceSelectorWork(nsSelector *metav1.LabelSelector) (branches, matches int, err error) { + matches = len(nsSelector.MatchLabels) + branches = 1 + hasPositiveMatch := len(nsSelector.MatchLabels) > 0 + for _, req := range nsSelector.MatchExpressions { + switch req.Operator { + case metav1.LabelSelectorOpNotIn: + matches += len(req.Values) + case metav1.LabelSelectorOpIn: + matches++ + hasPositiveMatch = true + if len(req.Values) > 1 { + if len(req.Values) > maxFlattenedNSSelectors/branches { + return 0, 0, 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: + matches++ + } + } + if !hasPositiveMatch { + matches++ + } + if matches > maxSelectorMatches { + return 0, 0, fmt.Errorf("selector expands into %d matches, past the %d limit: %w", + matches, maxSelectorMatches, ErrTooManySelectorMatches) + } + if matches > maxTotalSelectorMatches/branches { + return 0, 0, fmt.Errorf("selector expands into %d branches of %d matches, past the %d total match limit: %w", + branches, matches, maxTotalSelectorMatches, ErrTooManySelectorMatches) + } + for _, requirement := range nsSelector.MatchExpressions { + if err := validateMatchExpression(requirement); err != nil { + return 0, 0, err + } + } + return branches, matches, nil +} + +func validateMatchExpression(requirement metav1.LabelSelectorRequirement) error { + switch requirement.Operator { + case metav1.LabelSelectorOpIn, metav1.LabelSelectorOpNotIn: + if len(requirement.Values) == 0 { + return ErrEmptyMatchExpressionValues + } + for _, value := range requirement.Values { + if !isValidLabelValue(value) { + return ErrInvalidMatchExpressionValues + } + } + case metav1.LabelSelectorOpExists, metav1.LabelSelectorOpDoesNotExist: + default: + return fmt.Errorf("operator %q on key %q: %w", + requirement.Operator, requirement.Key, ErrUnsupportedMatchExpressionOperator) + } + return nil +} + +// zipMatchExprs adds one alternative for each value to every existing branch. func zipMatchExprs(baseSelectors []metav1.LabelSelector, matchExpr metav1.LabelSelectorRequirement) []metav1.LabelSelector { zippedLabelSelectors := []metav1.LabelSelector{} for _, selector := range baseSelectors { @@ -198,6 +344,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 @@ -209,7 +366,7 @@ func parseNSSelector(selector *metav1.LabelSelector) []labelSelector { // #1. All namespaces case if len(selector.MatchLabels) == 0 && len(selector.MatchExpressions) == 0 { - parsedSelectors.addSelector(true, ipsets.KeyLabelOfNamespace, util.KubeAllNamespacesFlag) + parsedSelectors.addSelector(true, ipsets.KeyLabelOfNamespace, util.KubeAllNamespacesFlagV2) return parsedSelectors.labelSelectors } @@ -239,6 +396,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.KubeAllNamespacesFlagV2) + } + return parsedSelectors.labelSelectors } diff --git a/npm/pkg/controlplane/translation/parseSelector_test.go b/npm/pkg/controlplane/translation/parseSelector_test.go index e93f99500ae..e6986fdd581 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,274 @@ 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) { + // One ACL per port, plus the required default drop. + portCount := maxACLsPerPolicy - 1 + 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 exactly at the ceiling must translate") + require.NotNil(t, npmNetPol) + require.Len(t, npmNetPol.ACLs, portCount+1, "every port plus the default drop") + require.Len(t, npmNetPol.ACLs, maxACLsPerPolicy) +} + +// 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 { + // Parent validation takes precedence, as it did in ipBlockRule before normalization + // moved here. Windows rejects the unsupported Except feature without parsing its CIDRs. + if util.IsWindowsDP() && len(ipBlockRule.Except) > 0 { return nil, ErrUnsupportedExceptCIDR } + // Canonicalize and deduplicate exclusions before comparing with the split entries. + deDupExcepts, err := canonicalizeExcepts(cidr, 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 +263,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 +276,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,10 +306,7 @@ func ipBlockRule(policyName, ns string, direction policies.Direction, matchType return nil, policies.SetInfo{}, nil } - if !util.IsIPV4(ipBlockRule.CIDR) { - return nil, policies.SetInfo{}, ErrUnsupportedIPAddress - } - + // The set builder validates and normalizes the CIDR once, before creating any members. ipBlockIPSet, err := ipBlockIPSet(policyName, ns, direction, ipBlockSetIndex, ipBlockPeerIndex, ipBlockRule) if err != nil { return nil, policies.SetInfo{}, err @@ -306,8 +376,8 @@ func nameSpaceSelector(matchType policies.MatchType, selector *metav1.LabelSelec // allowAllInternal returns translatedIPSet and SetInfo in case of allowing all internal traffic excluding external. func allowAllInternal(matchType policies.MatchType) (*ipsets.TranslatedIPSet, policies.SetInfo) { - allowAllIPSets := ipsets.NewTranslatedIPSet(util.KubeAllNamespacesFlag, ipsets.KeyLabelOfNamespace) - setInfo := policies.NewSetInfo(util.KubeAllNamespacesFlag, ipsets.KeyLabelOfNamespace, included, matchType) + allowAllIPSets := ipsets.NewTranslatedIPSet(util.KubeAllNamespacesFlagV2, ipsets.KeyLabelOfNamespace) + setInfo := policies.NewSetInfo(util.KubeAllNamespacesFlagV2, ipsets.KeyLabelOfNamespace, included, matchType) return allowAllIPSets, setInfo } @@ -339,6 +409,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 +421,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 @@ -467,6 +547,9 @@ func translateRule(npmNetPol *policies.NPMNetworkPolicy, if npmLiteToggle { return ErrUnsupportedNonCIDR } + if err := checkACLBudget(npmNetPol); err != nil { + return err + } acl := policies.NewACLPolicy(policies.Allowed, direction) ruleIPSets, allowAllInternalSetInfo := allowAllInternal(matchType) npmNetPol.RuleIPSets = append(npmNetPol.RuleIPSets, ruleIPSets) @@ -613,12 +696,18 @@ func ingressPolicy(npmNetPol *policies.NPMNetworkPolicy, netPolName string, ingr // #1. Allow all traffic from both internal and external. // In yaml file, it is specified with '{}'. if isAllowAllToIngress(ingress) { + if err := checkACLBudget(npmNetPol); err != nil { + return err + } allowAllPolicy(npmNetPol, policies.Ingress) return nil } // #2. If ingress is nil (in yaml file, it is specified with '[]'), it means "Deny all" - it does not allow receiving any traffic from others. if ingress == nil { + if err := checkACLBudget(npmNetPol); err != nil { + return err + } // Except for allow all traffic case in #1, the rest of them should have default drop rules. dropACL := defaultDropACL(policies.Ingress) npmNetPol.ACLs = append(npmNetPol.ACLs, dropACL) @@ -633,6 +722,9 @@ func ingressPolicy(npmNetPol *policies.NPMNetworkPolicy, netPolName string, ingr } } // Except for allow all traffic case in #1, the rest of them should have default drop rules. + if err := checkACLBudget(npmNetPol); err != nil { + return err + } dropACL := defaultDropACL(policies.Ingress) npmNetPol.ACLs = append(npmNetPol.ACLs, dropACL) return nil @@ -656,12 +748,18 @@ func egressPolicy(npmNetPol *policies.NPMNetworkPolicy, netPolName string, egres // #1. Allow all traffic to both internal and external. // In yaml file, it is specified with '{}'. if isAllowAllToEgress(egress) { + if err := checkACLBudget(npmNetPol); err != nil { + return err + } allowAllPolicy(npmNetPol, policies.Egress) return nil } // #2. If egress is nil (in yaml file, it is specified with '[]'), it means "Deny all" - it does not allow sending traffic to others. if egress == nil { + if err := checkACLBudget(npmNetPol); err != nil { + return err + } // Except for allow all traffic case in #1, the rest of them should have default drop rules. dropACL := defaultDropACL(policies.Egress) npmNetPol.ACLs = append(npmNetPol.ACLs, dropACL) @@ -679,6 +777,9 @@ func egressPolicy(npmNetPol *policies.NPMNetworkPolicy, netPolName string, egres // #3. Except for allow all traffic case in #1, the rest of them should have default drop rules. // Add drop ACL to drop the rest of traffic which is not specified in Egress Spec. + if err := checkACLBudget(npmNetPol); err != nil { + return err + } dropACL := defaultDropACL(policies.Egress) npmNetPol.ACLs = append(npmNetPol.ACLs, dropACL) return nil @@ -721,6 +822,11 @@ func parseNodeEgressPorts(annotations map[string]string) []int32 { // TranslatePolicy translates networkpolicy object to NPMNetworkPolicy object // and returns the NPMNetworkPolicy object. func TranslatePolicy(npObj *networkingv1.NetworkPolicy, npmLiteToggle bool) (*policies.NPMNetworkPolicy, error) { + if !npmLiteToggle { + if err := validateFullPolicyWork(npObj); err != nil { + return nil, fmt.Errorf("network policy %s/%s: %w", npObj.Namespace, npObj.Name, err) + } + } netPolName := npObj.Name npmNetPol := policies.NewNPMNetworkPolicy(netPolName, npObj.Namespace) @@ -763,9 +869,53 @@ func TranslatePolicy(npObj *networkingv1.NetworkPolicy, npmLiteToggle bool) (*po } } } + + if err := checkACLTotal(npmNetPol); err != nil { + return nil, err + } + return npmNetPol, nil } +// maxACLsPerPolicy bounds generated ACL work on the full-NPM v2 ipset path. +// The Windows Lite direct-rule allocator is outside this work-budget guarantee; +// the final total check does not bound that allocator's intermediate work. 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 + +// checkACLBudget checks room before an ACL is added, including default drops. +// Unlike reserving a fixed number of slots, this admits an exact-limit policy +// regardless of which directions have already been translated. +func checkACLBudget(npmNetPol *policies.NPMNetworkPolicy) error { + if len(npmNetPol.ACLs) >= maxACLsPerPolicy { + 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 +936,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..3b6ca26579f 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" @@ -1111,10 +1144,10 @@ func TestNameSpaceSelector(t *testing.T) { MatchLabels: map[string]string{}, }, nsSelectorIPSets: []*ipsets.TranslatedIPSet{ - ipsets.NewTranslatedIPSet(util.KubeAllNamespacesFlag, ipsets.KeyLabelOfNamespace), + ipsets.NewTranslatedIPSet(util.KubeAllNamespacesFlagV2, ipsets.KeyLabelOfNamespace), }, nsSelectorList: []policies.SetInfo{ - policies.NewSetInfo(util.KubeAllNamespacesFlag, ipsets.KeyLabelOfNamespace, included, matchType), + policies.NewSetInfo(util.KubeAllNamespacesFlagV2, ipsets.KeyLabelOfNamespace, included, matchType), }, }, { @@ -1268,6 +1301,401 @@ 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.KubeAllNamespacesFlagV2, 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.KubeAllNamespacesFlagV2, 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.KubeAllNamespacesFlagV2, 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.KubeAllNamespacesFlagV2, 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.KubeAllNamespacesFlagV2, 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.KubeAllNamespacesFlagV2, 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) + if util.IsWindowsDP() { + require.ErrorIs(t, err, ErrUnsupportedNegativeMatch) + require.Nil(t, npmNetPol) + return + } + 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.KubeAllNamespacesFlagV2, 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.KubeAllNamespacesFlagV2 { + 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) + if util.IsWindowsDP() { + require.ErrorIs(t, err, ErrUnsupportedNegativeMatch) + require.Nil(t, npmNetPol) + return + } + 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.KubeAllNamespacesFlagV2, 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.KubeAllNamespacesFlagV2}, 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 { @@ -1279,8 +1707,8 @@ func TestAllowAllInternal(t *testing.T) { { name: "Allow all traffic from all namespaces in ingress", matchType: matchType, - nsSelectorIPSets: ipsets.NewTranslatedIPSet(util.KubeAllNamespacesFlag, ipsets.KeyLabelOfNamespace), - nsSelectorList: policies.NewSetInfo(util.KubeAllNamespacesFlag, ipsets.KeyLabelOfNamespace, included, matchType), + nsSelectorIPSets: ipsets.NewTranslatedIPSet(util.KubeAllNamespacesFlagV2, ipsets.KeyLabelOfNamespace), + nsSelectorList: policies.NewSetInfo(util.KubeAllNamespacesFlagV2, ipsets.KeyLabelOfNamespace, included, matchType), }, } @@ -3474,3 +3902,254 @@ 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) + if util.IsWindowsDP() { + require.ErrorIs(t, err, ErrUnsupportedNegativeMatch) + require.Nil(t, npmNetPol) + return + } + 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.KubeAllNamespacesFlagV2, 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.KubeAllNamespacesFlagV2}, 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/controlplane/translation/work_budget.go b/npm/pkg/controlplane/translation/work_budget.go new file mode 100644 index 00000000000..af80b49387e --- /dev/null +++ b/npm/pkg/controlplane/translation/work_budget.go @@ -0,0 +1,172 @@ +package translation + +import ( + "fmt" + + networkingv1 "k8s.io/api/networking/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/intstr" +) + +// Count copied ACL matches and generated members together before materialization. +const maxTotalPolicyMatches = maxTotalSelectorMatches + +type policyWorkBudget struct { + acls int + matches int +} + +func validateFullPolicyWork(policy *networkingv1.NetworkPolicy) error { + selectedMatches, selectedMembers, err := podSelectorWork(&policy.Spec.PodSelector) + if err != nil { + return err + } + if selectedMatches >= maxSelectorMatches { + return fmt.Errorf("selected pod selector expands into %d matches including its namespace anchor, past the %d limit: %w", + selectedMatches+1, maxSelectorMatches, ErrTooManySelectorMatches) + } + budget := policyWorkBudget{matches: selectedMatches + selectedMembers + 1} + if budget.matches > maxTotalPolicyMatches { + return fmt.Errorf("selected pod members exceed the %d policy match limit: %w", maxTotalPolicyMatches, ErrTooManyPolicyMatches) + } + for _, direction := range policy.Spec.PolicyTypes { + if direction == networkingv1.PolicyTypeIngress { + if isAllowAllToIngress(policy.Spec.Ingress) { + if err := budget.reserve(1, 0, nil, 0); err != nil { + return err + } + continue + } + for _, rule := range policy.Spec.Ingress { + if err := budget.rule(rule.Ports, rule.From); err != nil { + return err + } + } + } else { + if isAllowAllToEgress(policy.Spec.Egress) { + if err := budget.reserve(1, 0, nil, 0); err != nil { + return err + } + continue + } + for _, rule := range policy.Spec.Egress { + if err := budget.rule(rule.Ports, rule.To); err != nil { + return err + } + } + } + if err := budget.reserve(1, 0, nil, 0); err != nil { + return err + } + } + return nil +} + +func (budget *policyWorkBudget) rule(ports []networkingv1.NetworkPolicyPort, peers []networkingv1.NetworkPolicyPeer) error { + allowExternal, portRuleExists, peerRuleExists := ruleExists(ports, peers) + if portRuleExists && (!peerRuleExists || allowExternal) { + if err := budget.reserve(1, 0, ports, 0); err != nil { + return err + } + } + for _, peer := range peers { + if peer.IPBlock != nil { + if peer.IPBlock.CIDR != "" { + // A parent may split into two members; exclusions add at most one each. + if err := budget.reserve(1, 1, ports, len(peer.IPBlock.Except)+2); err != nil { + return err + } + } + continue + } + if peer.PodSelector == nil && peer.NamespaceSelector == nil { + continue + } + podMatches, podMembers, err := podSelectorWork(peer.PodSelector) + if err != nil { + return err + } + branches, namespaceMatches := 1, 1 + if peer.NamespaceSelector != nil { + branches, namespaceMatches, err = namespaceSelectorWork(peer.NamespaceSelector) + if err != nil { + return err + } + for _, requirement := range peer.NamespaceSelector.MatchExpressions { + if unsupportedOpsInWindows(requirement.Operator) { + return ErrUnsupportedNegativeMatch + } + } + } + if err := budget.reserve(branches, namespaceMatches+podMatches, ports, podMembers); err != nil { + return err + } + } + return nil +} + +func (budget *policyWorkBudget) reserve(branches, matches int, ports []networkingv1.NetworkPolicyPort, members int) error { + portCount := len(ports) + if portCount == 0 { + portCount = 1 + } + if portCount > (maxACLsPerPolicy-budget.acls)/branches { + return fmt.Errorf("policy exceeds the %d rule limit: %w", maxACLsPerPolicy, ErrTooManyACLs) + } + acls := branches * portCount + namedPorts := 0 + for _, port := range ports { + if port.Port != nil && port.Port.Type == intstr.String { + namedPorts++ + } + } + perACLMatches := matches + if namedPorts > 0 { + perACLMatches++ + } + if perACLMatches > maxSelectorMatches { + return fmt.Errorf("peer expands into %d matches per ACL, past the %d limit: %w", + perACLMatches, maxSelectorMatches, ErrTooManySelectorMatches) + } + remaining := maxTotalPolicyMatches - budget.matches + if members > remaining || matches > (remaining-members)/acls { + return fmt.Errorf("replicated matches exceed the %d policy match limit: %w", maxTotalPolicyMatches, ErrTooManyPolicyMatches) + } + work := members + matches*acls + if namedPorts > (remaining-work)/branches { + return fmt.Errorf("named-port matches exceed the %d policy match limit: %w", maxTotalPolicyMatches, ErrTooManyPolicyMatches) + } + budget.acls += acls + budget.matches += work + namedPorts*branches + return nil +} + +func podSelectorWork(selector *metav1.LabelSelector) (matches, members int, err error) { + if selector == nil { + return 0, 0, nil + } + matches = len(selector.MatchLabels) + len(selector.MatchExpressions) + if matches > maxSelectorMatches { + return 0, 0, fmt.Errorf("pod selector has %d matches, past the %d limit: %w", + matches, maxSelectorMatches, ErrTooManySelectorMatches) + } + for _, requirement := range selector.MatchExpressions { + if len(requirement.Values) > maxSelectorMatches { + return 0, 0, fmt.Errorf("pod requirement %q has %d values, past the %d limit: %w", + requirement.Key, len(requirement.Values), maxSelectorMatches, ErrTooManySelectorMatches) + } + if err := validateMatchExpression(requirement); err != nil { + return 0, 0, err + } + if unsupportedOpsInWindows(requirement.Operator) { + return 0, 0, ErrUnsupportedNegativeMatch + } + if len(requirement.Values) > 1 { + if len(requirement.Values) > maxTotalPolicyMatches-members { + return 0, 0, fmt.Errorf("pod selector members exceed the %d policy match limit: %w", maxTotalPolicyMatches, ErrTooManyPolicyMatches) + } + members += len(requirement.Values) + } + } + return matches, members, nil +} diff --git a/npm/pkg/controlplane/translation/work_budget_test.go b/npm/pkg/controlplane/translation/work_budget_test.go new file mode 100644 index 00000000000..ea92c4d9f99 --- /dev/null +++ b/npm/pkg/controlplane/translation/work_budget_test.go @@ -0,0 +1,138 @@ +package translation + +import ( + "fmt" + "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 combinedBudgetPolicy(direction networkingv1.PolicyType, labelCount, branchCount, portCount int) *networkingv1.NetworkPolicy { + labels := make(map[string]string, labelCount) + for index := 0; index < labelCount; index++ { + labels[fmt.Sprintf("key%d", index)] = "value" + } + values := make([]string, branchCount) + for index := range values { + values[index] = fmt.Sprintf("branch%d", index) + } + ports := make([]networkingv1.NetworkPolicyPort, portCount) + for index := range ports { + port := intstr.FromInt(1000 + index) + ports[index].Port = &port + } + peer := networkingv1.NetworkPolicyPeer{ + PodSelector: &metav1.LabelSelector{MatchLabels: labels}, + NamespaceSelector: &metav1.LabelSelector{MatchExpressions: []metav1.LabelSelectorRequirement{{ + Key: teamLabelKey, Operator: metav1.LabelSelectorOpIn, Values: values, + }}}, + } + policy := &networkingv1.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: "combined", Namespace: defaultNS}, + Spec: networkingv1.NetworkPolicySpec{PolicyTypes: []networkingv1.PolicyType{direction}}, + } + if direction == networkingv1.PolicyTypeIngress { + policy.Spec.Ingress = []networkingv1.NetworkPolicyIngressRule{{Ports: ports, From: []networkingv1.NetworkPolicyPeer{peer}}} + } else { + policy.Spec.Egress = []networkingv1.NetworkPolicyEgressRule{{Ports: ports, To: []networkingv1.NetworkPolicyPeer{peer}}} + } + return policy +} + +func TestCombinedPeerBudgetBeforeAllocation(t *testing.T) { + for _, direction := range []networkingv1.PolicyType{networkingv1.PolicyTypeIngress, networkingv1.PolicyTypeEgress} { + t.Run(string(direction), func(t *testing.T) { + policy := combinedBudgetPolicy(direction, 20000, 16, 124) + before := policy.DeepCopy() + var translated *policies.NPMNetworkPolicy + var err error + allocations := testing.AllocsPerRun(3, func() { + translated, err = TranslatePolicy(policy, false) + }) + require.ErrorIs(t, err, ErrTooManySelectorMatches) + require.Nil(t, translated) + require.Less(t, allocations, float64(64), "rejection must happen before per-label sets or branch ACLs are allocated") + require.Equal(t, before, policy) + }) + } +} + +func TestCombinedPeerPortReplicationBudget(t *testing.T) { + for _, direction := range []networkingv1.PolicyType{networkingv1.PolicyTypeIngress, networkingv1.PolicyTypeEgress} { + t.Run(string(direction), func(t *testing.T) { + policy := combinedBudgetPolicy(direction, 10, 16, 124) + translated, err := TranslatePolicy(policy, false) + require.ErrorIs(t, err, ErrTooManyPolicyMatches) + require.Nil(t, translated) + + control := combinedBudgetPolicy(direction, 4, 16, 124) + translated, err = TranslatePolicy(control, false) + require.NoError(t, err) + require.Len(t, translated.ACLs, 1985) + }) + } +} + +func TestPolicyMatchBudgetAccumulatesAcrossRulesAndDirections(t *testing.T) { + for _, dualDirection := range []bool{false, true} { + t.Run(fmt.Sprintf("dualDirection=%t", dualDirection), func(t *testing.T) { + policy := combinedBudgetPolicy(networkingv1.PolicyTypeIngress, 99, 1, 50) + if dualDirection { + policy.Spec.PolicyTypes = append(policy.Spec.PolicyTypes, networkingv1.PolicyTypeEgress) + policy.Spec.Egress = combinedBudgetPolicy(networkingv1.PolicyTypeEgress, 99, 1, 50).Spec.Egress + } else { + policy.Spec.Ingress = append(policy.Spec.Ingress, policy.Spec.Ingress[0]) + } + translated, err := TranslatePolicy(policy, false) + require.ErrorIs(t, err, ErrTooManyPolicyMatches) + require.Nil(t, translated) + }) + } +} + +func TestPolicyMatchBudgetExactBoundaryAndNamedPorts(t *testing.T) { + budget := policyWorkBudget{} + require.NoError(t, budget.reserve(10, maxSelectorMatches, nil, 0)) + require.Equal(t, maxTotalPolicyMatches, budget.matches) + require.NoError(t, budget.reserve(1, 0, nil, 0), "a zero-match default drop still fits") + before := budget + require.ErrorIs(t, budget.reserve(1, 1, nil, 0), ErrTooManyPolicyMatches) + require.Equal(t, before, budget, "rejection must not reserve partial work") + + namedPort := intstr.FromString("web") + ports := []networkingv1.NetworkPolicyPort{{Port: &namedPort}} + budget = policyWorkBudget{} + require.NoError(t, budget.reserve(1, maxSelectorMatches-1, ports, 0)) + require.Equal(t, maxSelectorMatches, budget.matches) + require.ErrorIs(t, budget.reserve(1, maxSelectorMatches, ports, 0), ErrTooManySelectorMatches) +} + +func TestSelectedPodAndNestedMemberBudgets(t *testing.T) { + policy := combinedBudgetPolicy(networkingv1.PolicyTypeIngress, 0, 1, 1) + policy.Spec.PodSelector.MatchLabels = make(map[string]string, maxSelectorMatches) + for index := 0; index < maxSelectorMatches; index++ { + policy.Spec.PodSelector.MatchLabels[fmt.Sprintf("selected%d", index)] = "value" + } + translated, err := TranslatePolicy(policy, false) + require.ErrorIs(t, err, ErrTooManySelectorMatches) + require.ErrorContains(t, err, fmt.Sprintf("selected pod selector expands into %d matches including its namespace anchor, past the %d limit", + maxSelectorMatches+1, maxSelectorMatches)) + require.NotContains(t, err.Error(), "namespaceSelector") + require.Nil(t, translated) + + policy.Spec.PodSelector = metav1.LabelSelector{} + values := make([]string, maxSelectorMatches+1) + for index := range values { + values[index] = fmt.Sprintf("value%d", index) + } + policy.Spec.Ingress[0].From[0].PodSelector = &metav1.LabelSelector{MatchExpressions: []metav1.LabelSelectorRequirement{{ + Key: "app", Operator: metav1.LabelSelectorOpIn, Values: values, + }}} + translated, err = TranslatePolicy(policy, false) + require.ErrorIs(t, err, ErrTooManySelectorMatches) + require.Nil(t, translated) +} diff --git a/npm/pkg/dataplane/debug/branch_matching_test.go b/npm/pkg/dataplane/debug/branch_matching_test.go new file mode 100644 index 00000000000..445ee4d3cd9 --- /dev/null +++ b/npm/pkg/dataplane/debug/branch_matching_test.go @@ -0,0 +1,170 @@ +package debug + +import ( + "fmt" + "testing" + + common "github.com/Azure/azure-container-networking/npm/pkg/controlplane/controllers/common" + "github.com/Azure/azure-container-networking/npm/pkg/dataplane/pb" + "github.com/Azure/azure-container-networking/npm/util" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" +) + +func TestV2MatchedSetsIncludeEveryCondition(t *testing.T) { + sets := []*pb.RuleResponse_SetInfo{ + {Name: util.NamespacePrefix + anchorPeerNamespace, HashedSetName: "namespace", Type: pb.SetType_NAMESPACE, Included: true}, + {Name: util.PodLabelPrefix + "app:shared", HashedSetName: "app-set", Type: pb.SetType_KEYLABELOFPOD, Included: true}, + } + rule := &pb.RuleResponse{Allowed: true, SrcList: sets} + hits, sourceSets, _, err := getHitRules( + &common.NpmPod{Namespace: anchorPeerNamespace, Labels: map[string]string{diagnosticAppLabelKey: diagnosticSharedValue}}, + &common.NpmPod{Namespace: anchorTargetNamespace}, + map[*pb.RuleResponse]struct{}{rule: {}}, &common.Cache{}, true, + ) + require.NoError(t, err) + require.ElementsMatch(t, []*pb.RuleResponse{rule}, hits) + require.Len(t, sourceSets, 2) + for _, set := range sets { + require.Equal(t, set, sourceSets[set.GetHashedSetName()]) + } +} + +func TestV2CIDRNamedPortDoesNotEnterSelectorPrecheck(t *testing.T) { + sets := []*pb.RuleResponse_SetInfo{ + {Name: util.CIDRPrefix + "peer", Type: pb.SetType_CIDRBLOCKS, Included: true}, + {Name: util.NamedPortIPSetPrefix + "web", Type: pb.SetType_NAMEDPORTS, Included: true}, + } + matched, err := matchNamespaceAnchorConditions( + "dst", &common.NpmPod{Namespace: anchorPeerNamespace}, + sets, &pb.RuleResponse{DstList: sets}, &common.Cache{}, true, + ) + require.NoError(t, err) + require.True(t, matched, "named ports must not turn an IPBlock peer into a selector peer") +} + +func TestCIDRNamedPortConditionsRespectVersion(t *testing.T) { + const ( + matchingIP = "10.0.0.10" + ruleProtocol = "tcp" + ) + webPort := []corev1.ContainerPort{{Name: "web", ContainerPort: 8080, Protocol: corev1.ProtocolTCP}} + for _, enableV2 := range []bool{false, true} { + for _, sourceSide := range []bool{false, true} { + for _, cidrFirst := range []bool{false, true} { + for _, test := range []struct { + name string + ip string + ports []corev1.ContainerPort + wantV1 bool + wantV2 bool + }{ + {"both match", matchingIP, webPort, true, true}, + {"CIDR only", matchingIP, nil, true, false}, + {"named port only", "10.1.0.10", webPort, true, false}, + {"neither matches", "10.1.0.10", nil, false, false}, + { + "wrong protocol", matchingIP, + []corev1.ContainerPort{{Name: "web", ContainerPort: 8080, Protocol: corev1.ProtocolUDP}}, + true, false, + }, + } { + t.Run(fmt.Sprintf("v2=%t/source=%t/cidrFirst=%t/%s", enableV2, sourceSide, cidrFirst, test.name), func(t *testing.T) { + cidr := &pb.RuleResponse_SetInfo{ + Name: util.CIDRPrefix + "peer", HashedSetName: "cidr", Type: pb.SetType_CIDRBLOCKS, + Included: true, Contents: []string{"10.0.0.0/24"}, + } + namedPort := &pb.RuleResponse_SetInfo{ + Name: util.NamedPortIPSetPrefix + "web", HashedSetName: "named-port", Type: pb.SetType_NAMEDPORTS, + Included: true, + } + peerSets := []*pb.RuleResponse_SetInfo{namedPort, cidr} + if cidrFirst { + peerSets = []*pb.RuleResponse_SetInfo{cidr, namedPort} + } + targetSets := []*pb.RuleResponse_SetInfo{{ + Name: util.NamespacePrefix + anchorTargetNamespace, HashedSetName: "target", + Type: pb.SetType_NAMESPACE, Included: true, + }} + peer := &common.NpmPod{Namespace: anchorPeerNamespace, PodIP: test.ip, ContainerPorts: test.ports} + target := &common.NpmPod{Namespace: anchorTargetNamespace} + allow := &pb.RuleResponse{ + Allowed: true, Protocol: ruleProtocol, Direction: pb.Direction_EGRESS, + SrcList: targetSets, DstList: peerSets, + } + deny := &pb.RuleResponse{Direction: pb.Direction_EGRESS, SrcList: targetSets} + src, dst := target, peer + if sourceSide { + src, dst = peer, target + allow.Direction, deny.Direction = pb.Direction_INGRESS, pb.Direction_INGRESS + allow.SrcList, allow.DstList = peerSets, targetSets + deny.SrcList, deny.DstList = nil, targetSets + } + hits, srcSets, dstSets, err := getHitRules( + src, dst, map[*pb.RuleResponse]struct{}{allow: {}, deny: {}}, &common.Cache{}, enableV2, + ) + require.NoError(t, err) + wantMatch := test.wantV1 + if enableV2 { + wantMatch = test.wantV2 + } + want := []*pb.RuleResponse{deny} + if wantMatch { + want = append(want, allow) + } + require.ElementsMatch(t, want, hits) + if enableV2 && wantMatch { + matchedSets, port := dstSets, allow.GetDPort() + if sourceSide { + matchedSets, port = srcSets, allow.GetSPort() + } + require.Len(t, matchedSets, 2) + require.Equal(t, cidr, matchedSets[cidr.GetHashedSetName()]) + require.Equal(t, namedPort, matchedSets[namedPort.GetHashedSetName()]) + require.Equal(t, int32(8080), port) + } + }) + } + } + } + } +} + +func TestV2ParentBranchesRemainAlternatives(t *testing.T) { + child := &pb.RuleResponse{ + Chain: EgressChainPrefix + "policy", Allowed: true, JumpTo: util.IptablesAzureAcceptChain, + DstList: []*pb.RuleResponse_SetInfo{{ + Name: util.NamespacePrefix + anchorTargetNamespace, Type: pb.SetType_NAMESPACE, Included: true, + }}, + } + first := &pb.RuleResponse{ + Chain: EgressChain, JumpTo: child.GetChain(), Comment: "first parent", + SrcList: []*pb.RuleResponse_SetInfo{{ + Name: util.NamespacePrefix + anchorPeerNamespace, Type: pb.SetType_NAMESPACE, Included: true, + }}, + } + second := &pb.RuleResponse{ + Chain: EgressChain, JumpTo: child.GetChain(), Comment: "second parent", + SrcList: []*pb.RuleResponse_SetInfo{{ + Name: util.NamespacePrefix + "another", Type: pb.SetType_NAMESPACE, Included: true, + }}, + } + merged := mergeV2ParentBranches(map[*pb.RuleResponse]struct{}{child: {}, first: {}, second: {}}) + require.Len(t, merged, 2) + require.Empty(t, child.GetSrcList(), "merging must not mutate the original child") + for branch := range merged { + require.Len(t, branch.GetSrcList(), 1) + require.Len(t, branch.GetDstList(), 1) + require.Equal(t, child.JumpTo, branch.JumpTo) + require.Contains(t, []string{first.Comment, second.Comment}, branch.Comment) + } + for _, namespace := range []string{anchorPeerNamespace, "another"} { + hits, _, _, err := getHitRules( + &common.NpmPod{Namespace: namespace}, &common.NpmPod{Namespace: anchorTargetNamespace}, + merged, &common.Cache{}, true, + ) + require.NoError(t, err) + require.Len(t, hits, 1) + require.Equal(t, child.GetChain(), hits[0].GetChain()) + } +} diff --git a/npm/pkg/dataplane/debug/cache_compatibility_test.go b/npm/pkg/dataplane/debug/cache_compatibility_test.go new file mode 100644 index 00000000000..4567a94eb6c --- /dev/null +++ b/npm/pkg/dataplane/debug/cache_compatibility_test.go @@ -0,0 +1,60 @@ +package debug + +import ( + "fmt" + "testing" + + common "github.com/Azure/azure-container-networking/npm/pkg/controlplane/controllers/common" + "github.com/Azure/azure-container-networking/npm/pkg/dataplane/pb" + "github.com/Azure/azure-container-networking/npm/util" + "github.com/stretchr/testify/require" +) + +// This implementation intentionally exposes only the original cache contract. +type legacyDiagnosticCache struct { + cache *common.Cache +} + +func (c *legacyDiagnosticCache) GetPod(input *common.Input) (*common.NpmPod, error) { + pod, err := c.cache.GetPod(input) + if err != nil { + return nil, fmt.Errorf("getting cached pod: %w", err) + } + return pod, nil +} + +func (c *legacyDiagnosticCache) GetNamespaceLabel(namespace, key string) string { + return c.cache.GetNamespaceLabel(namespace, key) +} + +func (c *legacyDiagnosticCache) GetListMap() map[string]string { + return c.cache.GetListMap() +} + +func (c *legacyDiagnosticCache) GetSetMap() map[string]string { + return c.cache.GetSetMap() +} + +func TestLegacyDiagnosticCacheCompatibility(t *testing.T) { + const labelKey = "team" + cache := &legacyDiagnosticCache{cache: &common.Cache{NsMap: map[string]*common.Namespace{ + util.NamespacePrefix + anchorPeerNamespace: {LabelsMap: map[string]string{labelKey: matchedTeamValue}}, + }}} + converter := &Converter{NPMCache: cache} + pod := &common.NpmPod{Namespace: anchorPeerNamespace} + set := &pb.RuleResponse_SetInfo{ + Name: util.NamespacePrefix + labelKey + ":" + matchedTeamValue, Type: pb.SetType_KEYVALUELABELOFNAMESPACE, Included: true, + } + rule := &pb.RuleResponse{Allowed: true, SrcList: []*pb.RuleResponse_SetInfo{set}} + hits, _, _, err := getHitRules(pod, &common.NpmPod{}, map[*pb.RuleResponse]struct{}{rule: {}}, converter.NPMCache, false) + require.NoError(t, err) + require.ElementsMatch(t, []*pb.RuleResponse{rule}, hits) + + set.Name = util.NamespaceLabelPrefix + labelKey + ":" + matchedTeamValue + matched, err := evaluateSetInfo("src", set, pod, rule, converter.NPMCache, true) + require.ErrorIs(t, err, errNamespaceLabelsUnavailable) + require.False(t, matched) + hits, _, _, err = getHitRules(pod, &common.NpmPod{}, map[*pb.RuleResponse]struct{}{rule: {}}, converter.NPMCache, true) + require.ErrorIs(t, err, errNamespaceLabelsUnavailable) + require.Nil(t, hits) +} diff --git a/npm/pkg/dataplane/debug/converter.go b/npm/pkg/dataplane/debug/converter.go index e8e27a961cd..570d841a4a5 100644 --- a/npm/pkg/dataplane/debug/converter.go +++ b/npm/pkg/dataplane/debug/converter.go @@ -23,6 +23,7 @@ import ( "github.com/Azure/azure-container-networking/npm/pkg/models" "github.com/Azure/azure-container-networking/npm/util" "github.com/pkg/errors" + "google.golang.org/protobuf/proto" ) var ( @@ -286,36 +287,48 @@ func (c *Converter) pbRuleList(ipTable *NPMIPtable.Table) (map[*pb.RuleResponse] } if c.EnableV2NPM { - parentRules := make([]*pb.RuleResponse, 0) - for childRule := range allRulesInNPMChains { - - // if rule is a string-int, we need to find the parent jump - // to add the src for egress and dst for ingress - if strings.HasPrefix(childRule.Chain, EgressChainPrefix) { - for parentRule := range allRulesInNPMChains { - if strings.HasPrefix(parentRule.Chain, EgressChain) && parentRule.JumpTo == childRule.Chain { - childRule.SrcList = append(childRule.SrcList, parentRule.SrcList...) - childRule.Comment = parentRule.Comment - parentRules = append(parentRules, parentRule) - } - } + return mergeV2ParentBranches(allRulesInNPMChains), nil + } + + return allRulesInNPMChains, nil +} + +func mergeV2ParentBranches(rules map[*pb.RuleResponse]struct{}) map[*pb.RuleResponse]struct{} { + result := make(map[*pb.RuleResponse]struct{}, len(rules)) + parents := make(map[*pb.RuleResponse]struct{}) + for child := range rules { + matchedParent := false + for parent := range rules { + if parent.JumpTo != child.GetChain() { + continue } - if strings.HasPrefix(childRule.Chain, IngressChainPrefix) { - for parentRule := range allRulesInNPMChains { - if strings.HasPrefix(parentRule.Chain, IngressChain) && parentRule.JumpTo == childRule.Chain { - childRule.DstList = append(childRule.DstList, parentRule.DstList...) - childRule.Comment = parentRule.Comment - parentRules = append(parentRules, parentRule) - } - } + egress := strings.HasPrefix(child.GetChain(), EgressChainPrefix) && strings.HasPrefix(parent.GetChain(), EgressChain) + ingress := strings.HasPrefix(child.GetChain(), IngressChainPrefix) && strings.HasPrefix(parent.GetChain(), IngressChain) + if !egress && !ingress { + continue + } + // Separate jumps are alternatives, not additional conditions on one path. + branch := proto.CloneOf(child) + branch.JumpTo = child.JumpTo + parentBranch := proto.CloneOf(parent) + if egress { + branch.SrcList = append(branch.GetSrcList(), parentBranch.GetSrcList()...) + } else { + branch.DstList = append(branch.GetDstList(), parentBranch.GetDstList()...) } + branch.Comment = parent.Comment + result[branch] = struct{}{} + parents[parent] = struct{}{} + matchedParent = true } - for _, parentRule := range parentRules { - delete(allRulesInNPMChains, parentRule) + if !matchedParent { + result[child] = struct{}{} } } - - return allRulesInNPMChains, nil + for parent := range parents { + delete(result, parent) + } + return result } func (c *Converter) getRulesFromChain(iptableChain *NPMIPtable.Chain) ([]*pb.RuleResponse, error) { diff --git a/npm/pkg/dataplane/debug/converter_test.go b/npm/pkg/dataplane/debug/converter_test.go index 56d332b2904..12b8f131100 100644 --- a/npm/pkg/dataplane/debug/converter_test.go +++ b/npm/pkg/dataplane/debug/converter_test.go @@ -99,7 +99,7 @@ func TestGetProtobufRulesFromIptableFileV2(t *testing.T) { }, } - hitrules, _, _, err := getHitRules(srcPod, dstPod, rules, c.NPMCache) + hitrules, _, _, err := getHitRules(srcPod, dstPod, rules, c.NPMCache, c.EnableV2NPM) require.NoError(t, err) log.Printf("hitrules %+v", hitrules) if err != nil { diff --git a/npm/pkg/dataplane/debug/fixture_consistency_test.go b/npm/pkg/dataplane/debug/fixture_consistency_test.go new file mode 100644 index 00000000000..5112af7d897 --- /dev/null +++ b/npm/pkg/dataplane/debug/fixture_consistency_test.go @@ -0,0 +1,41 @@ +package debug + +import ( + "os" + "strings" + "testing" + + "github.com/Azure/azure-container-networking/npm/pkg/dataplane/ipsets" + "github.com/Azure/azure-container-networking/npm/util" + "github.com/stretchr/testify/require" +) + +func TestV2NestedFixtureIdentitiesMatchKernelNames(t *testing.T) { + converter := &Converter{EnableV2NPM: true} + require.NoError(t, converter.NpmCacheFromFile(npmCacheFileV2)) + rules, err := os.ReadFile(iptableSaveFileV2) + require.NoError(t, err) + count := 0 + for hashedName, name := range converter.NPMCache.GetSetMap() { + unprefixedName, nested := strings.CutPrefix(name, util.NestedLabelPrefix) + if !nested { + continue + } + count++ + t.Run(name, func(t *testing.T) { + metadata := ipsets.NewIPSetMetadata(unprefixedName, ipsets.NestedLabelOfPod) + require.Equal(t, metadata.GetHashedName(), hashedName) + require.Contains(t, string(rules), hashedName) + require.Contains(t, string(rules), name) + }) + } + require.Equal(t, 2, count) +} + +func TestV2AggregateFixtureUsesCurrentIdentity(t *testing.T) { + converter := &Converter{EnableV2NPM: true} + require.NoError(t, converter.NpmCacheFromFile(npmCacheFileV2)) + metadata := ipsets.NewIPSetMetadata(util.KubeAllNamespacesFlagV2, ipsets.KeyLabelOfNamespace) + require.Contains(t, converter.NPMCache.GetSetMap(), metadata.GetHashedName()) + require.Equal(t, metadata.GetPrefixName(), converter.NPMCache.GetSetMap()[metadata.GetHashedName()]) +} diff --git a/npm/pkg/dataplane/debug/namespace_anchor_test.go b/npm/pkg/dataplane/debug/namespace_anchor_test.go new file mode 100644 index 00000000000..c5f7a54a3ac --- /dev/null +++ b/npm/pkg/dataplane/debug/namespace_anchor_test.go @@ -0,0 +1,273 @@ +package debug + +import ( + "fmt" + "testing" + + common "github.com/Azure/azure-container-networking/npm/pkg/controlplane/controllers/common" + "github.com/Azure/azure-container-networking/npm/pkg/dataplane/pb" + "github.com/Azure/azure-container-networking/npm/util" + "github.com/stretchr/testify/require" +) + +const ( + anchorPeerNamespace = "peer" + anchorTargetNamespace = "target" + matchedTeamValue = "blue" + otherLabelValue = "other" + diagnosticAppLabelKey = "app" + diagnosticSharedValue = "shared" +) + +func TestV2NamespaceAggregateMatch(t *testing.T) { + cache := &common.Cache{NsMap: map[string]*common.Namespace{anchorPeerNamespace: {}}} + for _, namespace := range []string{anchorPeerNamespace, "", "missing"} { + for _, included := range []bool{true, false} { + t.Run(fmt.Sprintf("namespace=%q/included=%t", namespace, included), func(t *testing.T) { + set := &pb.RuleResponse_SetInfo{ + Name: util.NamespaceLabelPrefix + util.KubeAllNamespacesFlagV2, + Type: pb.SetType_KEYLABELOFNAMESPACE, + Included: included, + } + matched, err := evaluateSetInfo("src", set, &common.NpmPod{Namespace: namespace}, &pb.RuleResponse{}, cache, true) + require.NoError(t, err) + require.Equal(t, included == (namespace == anchorPeerNamespace), matched) + }) + } + } +} + +func TestV2NamespaceKeyOnlyMatch(t *testing.T) { + for _, test := range []struct { + name string + labels map[string]string + present bool + }{ + {"absent", nil, false}, + {"empty value", map[string]string{"feature": ""}, true}, + {"nonempty value", map[string]string{"feature": "enabled"}, true}, + } { + for _, included := range []bool{false, true} { + t.Run(fmt.Sprintf("%s/included=%t", test.name, included), func(t *testing.T) { + cache := &common.Cache{NsMap: map[string]*common.Namespace{ + anchorPeerNamespace: {LabelsMap: test.labels}, + }} + set := &pb.RuleResponse_SetInfo{ + Name: util.NamespaceLabelPrefix + "feature", Type: pb.SetType_KEYLABELOFNAMESPACE, Included: included, + } + matched, err := evaluateSetInfo("src", set, &common.NpmPod{Namespace: anchorPeerNamespace}, &pb.RuleResponse{}, cache, true) + require.NoError(t, err) + require.Equal(t, included == test.present, matched) + }) + } + } +} + +func TestV1NamespaceMatchingIsUnchanged(t *testing.T) { + cache := &common.Cache{NsMap: map[string]*common.Namespace{ + util.NamespacePrefix + anchorPeerNamespace: {LabelsMap: map[string]string{"team": matchedTeamValue}}, + }} + pod := &common.NpmPod{ + Namespace: anchorPeerNamespace, + Labels: map[string]string{"nslabel-team": matchedTeamValue}, + } + for _, set := range []*pb.RuleResponse_SetInfo{ + {Name: util.NamespacePrefix + "team:blue", Type: pb.SetType_KEYVALUELABELOFNAMESPACE, Included: true}, + {Name: "nslabel-team:blue", Type: pb.SetType_KEYVALUELABELOFPOD, Included: true}, + } { + matched, err := matchNamespaceAnchorConditions("src", pod, []*pb.RuleResponse_SetInfo{set}, &pb.RuleResponse{}, cache, false) + require.NoError(t, err) + require.True(t, matched, "the v2 pre-check must leave v1 metadata alone") + matched, err = evaluateSetInfo("src", set, pod, &pb.RuleResponse{}, cache, false) + require.NoError(t, err) + require.True(t, matched) + } +} + +func TestV2MixedNamespaceConditionsAreConjunctive(t *testing.T) { + for _, tenant := range []string{"x", "y", otherLabelValue} { + for _, positiveFirst := range []bool{true, false} { + t.Run(fmt.Sprintf("tenant=%s/positiveFirst=%t", tenant, positiveFirst), func(t *testing.T) { + cache := &common.Cache{NsMap: map[string]*common.Namespace{ + anchorPeerNamespace: {LabelsMap: map[string]string{"team": matchedTeamValue, "tenant": tenant}}, + }} + positive := &pb.RuleResponse_SetInfo{Name: util.NamespaceLabelPrefix + "team:blue", Type: pb.SetType_KEYLABELOFNAMESPACE, Included: true} + excludeX := &pb.RuleResponse_SetInfo{Name: util.NamespaceLabelPrefix + "tenant:x", Type: pb.SetType_KEYLABELOFNAMESPACE} + excludeY := &pb.RuleResponse_SetInfo{Name: util.NamespaceLabelPrefix + "tenant:y", Type: pb.SetType_KEYLABELOFNAMESPACE} + sets := []*pb.RuleResponse_SetInfo{positive, excludeX, excludeY} + if !positiveFirst { + sets = []*pb.RuleResponse_SetInfo{excludeX, excludeY, positive} + } + allow := &pb.RuleResponse{Allowed: true, SrcList: sets} + deny := &pb.RuleResponse{DstList: []*pb.RuleResponse_SetInfo{{ + Name: util.NamespacePrefix + anchorTargetNamespace, Type: pb.SetType_NAMESPACE, Included: true, + }}} + hits, _, _, err := getHitRules( + &common.NpmPod{Namespace: anchorPeerNamespace}, &common.NpmPod{Namespace: anchorTargetNamespace}, + map[*pb.RuleResponse]struct{}{allow: {}, deny: {}}, cache, true, + ) + require.NoError(t, err) + want := []*pb.RuleResponse{deny} + if tenant == otherLabelValue { + want = append(want, allow) + } + require.ElementsMatch(t, want, hits) + }) + } + } +} + +func TestNamespaceAnchorRulesRequireEveryNamespaceMatch(t *testing.T) { + orders := [][3]int{{0, 1, 2}, {0, 2, 1}, {1, 0, 2}, {1, 2, 0}, {2, 0, 1}, {2, 1, 0}} + for _, direction := range []pb.Direction{pb.Direction_INGRESS, pb.Direction_EGRESS} { + for _, tenant := range []string{"a", "b", "good", ""} { + for _, order := range orders { + t.Run(fmt.Sprintf("%s/tenant=%q/order=%v", direction, tenant, order), func(t *testing.T) { + peer := &common.NpmPod{Namespace: anchorPeerNamespace} + target := &common.NpmPod{Namespace: anchorTargetNamespace} + cache := &common.Cache{ + NsMap: map[string]*common.Namespace{ + anchorPeerNamespace: {LabelsMap: map[string]string{"tenant": tenant}}, + }, + } + + allMatches := []*pb.RuleResponse_SetInfo{ + {Name: util.NamespaceLabelPrefix + "tenant:a", HashedSetName: "tenant-a", Type: pb.SetType_KEYVALUELABELOFNAMESPACE}, + {Name: util.NamespaceLabelPrefix + "tenant:b", HashedSetName: "tenant-b", Type: pb.SetType_KEYVALUELABELOFNAMESPACE}, + { + Name: util.NamespaceLabelPrefix + util.KubeAllNamespacesFlagV2, HashedSetName: "aggregate", + Type: pb.SetType_KEYLABELOFNAMESPACE, Included: true, + }, + } + converter := &Converter{EnableV2NPM: true} + for _, set := range allMatches { + set.Type, _ = converter.getSetTypeV2(set.GetName()) + } + peerMatches := []*pb.RuleResponse_SetInfo{allMatches[order[0]], allMatches[order[1]], allMatches[order[2]]} + targetMatches := []*pb.RuleResponse_SetInfo{{ + Name: util.NamespacePrefix + anchorTargetNamespace, HashedSetName: anchorTargetNamespace, Type: pb.SetType_NAMESPACE, Included: true, + }} + allow := &pb.RuleResponse{Allowed: true, Direction: direction, Chain: "allow"} + deny := &pb.RuleResponse{Direction: direction, Chain: "deny"} + src, dst := peer, target + if direction == pb.Direction_INGRESS { + allow.SrcList, allow.DstList = peerMatches, targetMatches + deny.DstList = targetMatches + } else { + src, dst = target, peer + allow.SrcList, allow.DstList = targetMatches, peerMatches + deny.SrcList = targetMatches + } + rules := map[*pb.RuleResponse]struct{}{allow: {}, deny: {}} + hits, _, _, err := getHitRules(src, dst, rules, cache, true) + require.NoError(t, err) + want := []*pb.RuleResponse{deny} + if tenant != "a" && tenant != "b" { + want = append(want, allow) + } + require.ElementsMatch(t, want, hits) + + peer.Namespace = "" + hits, _, _, err = getHitRules(src, dst, rules, cache, true) + require.NoError(t, err) + require.ElementsMatch(t, []*pb.RuleResponse{deny}, hits, "the aggregate must not match an external endpoint") + }) + } + } + } +} + +func TestNamespaceAnchorConditionsDistinguishLabelPresence(t *testing.T) { + for _, test := range []struct { + name string + labels map[string]string + want bool + }{ + {"missing label", map[string]string{}, true}, + {"empty value", map[string]string{util.KubeAllNamespacesFlag: ""}, false}, + {"nonempty value", map[string]string{util.KubeAllNamespacesFlag: "yes"}, false}, + } { + t.Run(test.name, func(t *testing.T) { + cache := &common.Cache{NsMap: map[string]*common.Namespace{ + anchorPeerNamespace: {LabelsMap: test.labels}, + util.NamespacePrefix + anchorPeerNamespace: {LabelsMap: map[string]string{util.KubeAllNamespacesFlag: otherLabelValue}}, + }} + sets := []*pb.RuleResponse_SetInfo{ + {Name: util.NamespaceLabelPrefix + util.KubeAllNamespacesFlag, Type: pb.SetType_KEYLABELOFNAMESPACE}, + {Name: util.NamespaceLabelPrefix + util.KubeAllNamespacesFlagV2, Type: pb.SetType_KEYLABELOFNAMESPACE, Included: true}, + } + matched, err := matchNamespaceAnchorConditions("src", &common.NpmPod{Namespace: anchorPeerNamespace}, sets, &pb.RuleResponse{}, cache, true) + require.NoError(t, err) + require.Equal(t, test.want, matched) + }) + } +} + +func TestNamespaceAnchorDoesNotOverridePodSelection(t *testing.T) { + cache := &common.Cache{NsMap: map[string]*common.Namespace{anchorPeerNamespace: {}}} + peer := &common.NpmPod{Namespace: anchorPeerNamespace, Labels: map[string]string{diagnosticAppLabelKey: otherLabelValue}} + target := &common.NpmPod{Namespace: anchorTargetNamespace} + converter := &Converter{EnableV2NPM: true} + podSet := &pb.RuleResponse_SetInfo{ + Name: util.PodLabelPrefix + "app:required", Included: true, + } + podSet.Type, _ = converter.getSetTypeV2(podSet.GetName()) + targetSet := &pb.RuleResponse_SetInfo{ + Name: util.NamespacePrefix + anchorTargetNamespace, Type: pb.SetType_NAMESPACE, Included: true, + } + allow := &pb.RuleResponse{ + Allowed: true, + SrcList: []*pb.RuleResponse_SetInfo{ + {Name: util.NamespaceLabelPrefix + util.KubeAllNamespacesFlagV2, Type: pb.SetType_KEYLABELOFNAMESPACE, Included: true}, + podSet, + }, + DstList: []*pb.RuleResponse_SetInfo{targetSet}, + } + deny := &pb.RuleResponse{DstList: []*pb.RuleResponse_SetInfo{targetSet}} + rules := map[*pb.RuleResponse]struct{}{allow: {}, deny: {}} + hits, _, _, err := getHitRules(peer, target, rules, cache, true) + require.NoError(t, err) + require.ElementsMatch(t, []*pb.RuleResponse{deny}, hits) + + peer.Labels[diagnosticAppLabelKey] = "required" + hits, _, _, err = getHitRules(peer, target, rules, cache, true) + require.NoError(t, err) + require.ElementsMatch(t, []*pb.RuleResponse{allow, deny}, hits) +} + +func TestNamespaceAnchorConditionsReportIncompleteSets(t *testing.T) { + cache := &common.Cache{NsMap: map[string]*common.Namespace{anchorPeerNamespace: {}}} + anchor := &pb.RuleResponse_SetInfo{ + Name: util.NamespaceLabelPrefix + util.KubeAllNamespacesFlagV2, Type: pb.SetType_KEYLABELOFNAMESPACE, Included: true, + } + for _, test := range []struct { + name string + set *pb.RuleResponse_SetInfo + cause error + }{ + { + "nested identity without values", + &pb.RuleResponse_SetInfo{Name: util.NestedLabelPrefix + "policy:key", Type: pb.SetType_NESTEDLABELOFPOD, Included: true}, + common.ErrInvalidInput, + }, + { + "unknown set type", + &pb.RuleResponse_SetInfo{Name: "unknown", Type: pb.SetType_UNKNOWN, Included: true}, + common.ErrSetType, + }, + { + "missing label key", + &pb.RuleResponse_SetInfo{Name: util.PodLabelPrefix + ":value", Type: pb.SetType_KEYLABELOFPOD, Included: true}, + common.ErrInvalidInput, + }, + } { + t.Run(test.name, func(t *testing.T) { + matched, err := matchNamespaceAnchorConditions( + "src", &common.NpmPod{Namespace: anchorPeerNamespace}, + []*pb.RuleResponse_SetInfo{anchor, test.set}, &pb.RuleResponse{}, cache, true, + ) + require.False(t, matched) + require.ErrorIs(t, err, test.cause) + }) + } +} diff --git a/npm/pkg/dataplane/debug/trafficanalyzer.go b/npm/pkg/dataplane/debug/trafficanalyzer.go index 2462989e12f..5daaaac3cdc 100644 --- a/npm/pkg/dataplane/debug/trafficanalyzer.go +++ b/npm/pkg/dataplane/debug/trafficanalyzer.go @@ -1,6 +1,7 @@ package debug import ( + "errors" "fmt" "log" "net" @@ -96,7 +97,7 @@ func (c *Converter) GetNetworkTuple(src, dst *common.Input, config *npmconfig.Co // after we have all rules from the AZURE-NPM chains in the filter table, get the network tuples of src and dst - return getNetworkTupleCommon(src, dst, c.NPMCache, allRules) + return getNetworkTupleCommon(src, dst, c.NPMCache, allRules, c.EnableV2NPM) } // GetNetworkTupleFile read from NPM cache and iptables-save files and @@ -112,7 +113,7 @@ func (c *Converter) GetNetworkTupleFile( //nolint:gocritic return nil, nil, nil, nil, fmt.Errorf("error occurred during get network tuple : %w", err) } - return getNetworkTupleCommon(src, dst, c.NPMCache, allRules) + return getNetworkTupleCommon(src, dst, c.NPMCache, allRules, c.EnableV2NPM) } // Common function. @@ -120,6 +121,7 @@ func getNetworkTupleCommon( src, dst *common.Input, npmCache common.GenericCache, allRules map[*pb.RuleResponse]struct{}, + enableV2NPM bool, ) ([][]byte, []*TupleAndRule, map[string]*pb.RuleResponse_SetInfo, map[string]*pb.RuleResponse_SetInfo, error) { srcPod, err := npmCache.GetPod(src) @@ -133,14 +135,14 @@ func getNetworkTupleCommon( } // find all rules where the source pod and dest pod exist - hitRules, srcSets, dstSets, err := getHitRules(srcPod, dstPod, allRules, npmCache) + hitRules, srcSets, dstSets, err := getHitRules(srcPod, dstPod, allRules, npmCache, enableV2NPM) if err != nil { return nil, nil, srcSets, dstSets, fmt.Errorf("%w", err) } ruleResListJSON := make([][]byte, 0) m := protojson.MarshalOptions{ - Indent: " ", + Indent: " ", EmitUnpopulated: true, } for _, rule := range hitRules { @@ -220,6 +222,7 @@ func getHitRules( src, dst *common.NpmPod, rules map[*pb.RuleResponse]struct{}, npmCache common.GenericCache, + enableV2NPM bool, ) ([]*pb.RuleResponse, map[string]*pb.RuleResponse_SetInfo, map[string]*pb.RuleResponse_SetInfo, error) { res := make([]*pb.RuleResponse, 0) @@ -227,6 +230,20 @@ func getHitRules( dstSets := make(map[string]*pb.RuleResponse_SetInfo, 0) for rule := range rules { + srcNamespaceMatch, err := matchNamespaceAnchorConditions("src", src, rule.GetSrcList(), rule, npmCache, enableV2NPM) + if err != nil { + return nil, nil, nil, fmt.Errorf("evaluating source namespace conditions: %w", err) + } + if !srcNamespaceMatch { + continue + } + dstNamespaceMatch, err := matchNamespaceAnchorConditions("dst", dst, rule.GetDstList(), rule, npmCache, enableV2NPM) + if err != nil { + return nil, nil, nil, fmt.Errorf("evaluating destination namespace conditions: %w", err) + } + if !dstNamespaceMatch { + continue + } matchedSrc := false matchedDst := false // evalute all match set in src @@ -236,13 +253,18 @@ func getHitRules( break } - matchedSource, err := evaluateSetInfo("src", setInfo, src, rule, npmCache) + matchedSource, err := evaluateSetInfo("src", setInfo, src, rule, npmCache, enableV2NPM) if err != nil { return nil, nil, nil, fmt.Errorf("error occurred during evaluating source's set info : %w", err) } if matchedSource { matchedSrc = true srcSets[setInfo.HashedSetName] = setInfo + if !enableV2NPM { + break + } + } else if enableV2NPM { + matchedSrc = false break } } @@ -254,14 +276,18 @@ func getHitRules( break } - matchedDestination, err := evaluateSetInfo("dst", setInfo, dst, rule, npmCache) + matchedDestination, err := evaluateSetInfo("dst", setInfo, dst, rule, npmCache, enableV2NPM) if err != nil { return nil, nil, nil, fmt.Errorf("error occurred during evaluating destination's set info : %w", err) } if matchedDestination { - dstSets[setInfo.HashedSetName] = setInfo matchedDst = true + if !enableV2NPM { + break + } + } else if enableV2NPM { + matchedDst = false break } } @@ -285,6 +311,117 @@ func getHitRules( return res, srcSets, dstSets, nil } +type namespaceLabelReader interface { + GetNamespaceLabels(namespace string) (map[string]string, bool) +} + +var errNamespaceLabelsUnavailable = errors.New("diagnostic cache does not support namespace label lookup") + +func namespaceLabels(npmCache common.GenericCache, namespace string) (labels map[string]string, exists bool, err error) { + reader, ok := npmCache.(namespaceLabelReader) + if !ok { + return nil, false, errNamespaceLabelsUnavailable + } + labels, exists = reader.GetNamespaceLabels(namespace) + return labels, exists, nil +} + +// V2 selector conditions are conjunctive, whether or not an aggregate was needed. +// The converter's explicit mode keeps user-controlled v1 names outside this path. +func matchNamespaceAnchorConditions(origin string, pod *common.NpmPod, sets []*pb.RuleResponse_SetInfo, rule *pb.RuleResponse, npmCache common.GenericCache, enableV2NPM bool) (bool, error) { + if !enableV2NPM { + return true, nil + } + hasSelectorCondition := false + for _, set := range sets { + if set.GetType() != pb.SetType_CIDRBLOCKS && set.GetType() != pb.SetType_UNKNOWN && + set.GetType() != pb.SetType_NAMEDPORTS { + hasSelectorCondition = true + break + } + } + if !hasSelectorCondition { + return true, nil + } + + labels, namespaceExists, err := namespaceLabels(npmCache, pod.Namespace) + if err != nil { + return false, err + } + for _, set := range sets { + var matches bool + switch set.GetType() { + case pb.SetType_KEYLABELOFNAMESPACE, pb.SetType_KEYVALUELABELOFNAMESPACE: + if set.GetName() == util.NamespaceLabelPrefix+util.KubeAllNamespacesFlagV2 { + matches = pod.Namespace != "" && namespaceExists + } else { + // The converter uses the same set type for key and key:value namespace sets. + var err error + matches, err = matchPrefixedLabelSet(labels, set.GetName(), util.NamespaceLabelPrefix) + if err != nil { + return false, err + } + } + case pb.SetType_NAMESPACE: + matches = set.GetName() == util.NamespacePrefix+pod.Namespace + case pb.SetType_KEYLABELOFPOD, pb.SetType_KEYVALUELABELOFPOD: + var err error + matches, err = matchPrefixedLabelSet(pod.Labels, set.GetName(), util.PodLabelPrefix) + if err != nil { + return false, err + } + case pb.SetType_NAMEDPORTS: + if !matchNAMEDPORTS(pod, set, rule, origin) { + return false, nil + } + continue + case pb.SetType_NESTEDLABELOFPOD: + var err error + matches, err = matchV2NestedLabelSet(pod.Labels, set.GetName()) + if err != nil { + return false, err + } + case pb.SetType_CIDRBLOCKS, pb.SetType_UNKNOWN: + return false, fmt.Errorf("unsupported anchored set %q: %w", set.GetName(), common.ErrSetType) + default: + return false, fmt.Errorf("namespace condition type %v: %w", set.GetType(), common.ErrSetType) + } + if matches != set.GetIncluded() { + return false, nil + } + } + return true, nil +} + +func matchPrefixedLabelSet(labels map[string]string, setName, prefix string) (bool, error) { + name, ok := strings.CutPrefix(setName, prefix) + key, value, hasValue := strings.Cut(name, ":") + if !ok || key == "" { + return false, fmt.Errorf("label set %q: %w", setName, common.ErrInvalidInput) + } + actual, exists := labels[key] + return exists && (!hasValue || actual == value), nil +} + +func matchV2NestedLabelSet(labels map[string]string, setName string) (bool, error) { + name, ok := strings.CutPrefix(setName, util.NestedLabelPrefix) + parts := strings.Split(name, util.IpsetLabelDelimter) + // V2 encodes policyKey:labelKey:value...; neither identity can contain ':'. + if !ok || len(parts) < 4 || parts[0] == "" || parts[1] == "" { + return false, fmt.Errorf("nested label set %q: %w", setName, common.ErrInvalidInput) + } + actual, exists := labels[parts[1]] + if !exists { + return false, nil + } + for _, expected := range parts[2:] { + if actual == expected { + return true, nil + } + } + return false, nil +} + // evalute an ipset to find out whether the pod's attributes match with the set func evaluateSetInfo( origin string, @@ -292,20 +429,45 @@ func evaluateSetInfo( pod *common.NpmPod, rule *pb.RuleResponse, npmCache common.GenericCache, + enableV2NPM bool, ) (bool, error) { switch setInfo.Type { case pb.SetType_KEYVALUELABELOFNAMESPACE: + if enableV2NPM { + return matchKEYLABELOFNAMESPACE(pod, npmCache, setInfo, true) + } return matchKEYVALUELABELOFNAMESPACE(pod, npmCache, setInfo), nil case pb.SetType_NESTEDLABELOFPOD: + if enableV2NPM { + matches, err := matchV2NestedLabelSet(pod.Labels, setInfo.GetName()) + if err != nil { + return false, err + } + return matches == setInfo.GetIncluded(), nil + } return matchNESTEDLABELOFPOD(pod, setInfo), nil case pb.SetType_KEYLABELOFNAMESPACE: - return matchKEYLABELOFNAMESPACE(pod, npmCache, setInfo), nil + return matchKEYLABELOFNAMESPACE(pod, npmCache, setInfo, enableV2NPM) case pb.SetType_NAMESPACE: return matchNAMESPACE(pod, setInfo), nil case pb.SetType_KEYVALUELABELOFPOD: + if enableV2NPM { + matches, err := matchPrefixedLabelSet(pod.Labels, setInfo.GetName(), util.PodLabelPrefix) + if err != nil { + return false, err + } + return matches == setInfo.GetIncluded(), nil + } return matchKEYVALUELABELOFPOD(pod, setInfo), nil case pb.SetType_KEYLABELOFPOD: + if enableV2NPM { + matches, err := matchPrefixedLabelSet(pod.Labels, setInfo.GetName(), util.PodLabelPrefix) + if err != nil { + return false, err + } + return matches == setInfo.GetIncluded(), nil + } return matchKEYLABELOFPOD(pod, setInfo), nil case pb.SetType_NAMEDPORTS: return matchNAMEDPORTS(pod, setInfo, rule, origin), nil @@ -357,18 +519,32 @@ func matchNESTEDLABELOFPOD(pod *common.NpmPod, setInfo *pb.RuleResponse_SetInfo) return true } -func matchKEYLABELOFNAMESPACE(pod *common.NpmPod, npmCache common.GenericCache, setInfo *pb.RuleResponse_SetInfo) bool { +func matchKEYLABELOFNAMESPACE(pod *common.NpmPod, npmCache common.GenericCache, setInfo *pb.RuleResponse_SetInfo, enableV2NPM bool) (bool, error) { + if enableV2NPM { + labels, namespaceExists, err := namespaceLabels(npmCache, pod.Namespace) + if err != nil { + return false, err + } + if setInfo.GetName() == util.NamespaceLabelPrefix+util.KubeAllNamespacesFlagV2 { + return setInfo.GetIncluded() == (pod.Namespace != "" && namespaceExists), nil + } + matches, err := matchPrefixedLabelSet(labels, setInfo.GetName(), util.NamespaceLabelPrefix) + if err != nil { + return false, err + } + return matches == setInfo.GetIncluded(), nil + } srcNamespace := pod.Namespace key := strings.Split(strings.TrimPrefix(setInfo.Name, util.NamespaceLabelPrefix), ":") included := npmCache.GetNamespaceLabel(srcNamespace, key[0]) if included != "" && included == key[1] { - return setInfo.Included + return setInfo.GetIncluded(), nil } if setInfo.Included { // if key does not exist but required in rule - return false + return false, nil } - return true + return true, nil } func matchNAMESPACE(pod *common.NpmPod, setInfo *pb.RuleResponse_SetInfo) bool { diff --git a/npm/pkg/dataplane/debug/trafficanalyzer_test.go b/npm/pkg/dataplane/debug/trafficanalyzer_test.go index 0e21bb172a7..e0d10ec41c6 100644 --- a/npm/pkg/dataplane/debug/trafficanalyzer_test.go +++ b/npm/pkg/dataplane/debug/trafficanalyzer_test.go @@ -50,6 +50,7 @@ func TestGetInputType(t *testing.T) { } func TestGetNetworkTuple(t *testing.T) { + const selectedPodIP = "10.224.0.70" type srcDstPair struct { src *common.Input dst *common.Input @@ -61,24 +62,17 @@ func TestGetNetworkTuple(t *testing.T) { } i0 := &srcDstPair{ - src: &common.Input{Content: "y/b", Type: common.NSPODNAME}, + src: &common.Input{Content: "y/a", Type: common.NSPODNAME}, dst: &common.Input{Content: "x/b", Type: common.NSPODNAME}, } + // The TCP/80 rules require destination namespace y or z as well as the pod label. + // Destination x/b must not be reported as a hit just because its pod label matches. expected0 := []*Tuple{ { RuleType: "ALLOWED", Direction: "EGRESS", - SrcIP: "10.224.0.17", - SrcPort: "ANY", - DstIP: "10.224.0.20", - DstPort: "80", - Protocol: "tcp", - }, - { - RuleType: "ALLOWED", - Direction: "EGRESS", - SrcIP: "10.224.0.17", + SrcIP: selectedPodIP, SrcPort: "ANY", DstIP: "ANY", DstPort: "53", @@ -87,25 +81,16 @@ func TestGetNetworkTuple(t *testing.T) { { RuleType: "ALLOWED", Direction: "EGRESS", - SrcIP: "10.224.0.17", + SrcIP: selectedPodIP, SrcPort: "ANY", DstIP: "ANY", DstPort: "53", Protocol: "tcp", }, - { - RuleType: "ALLOWED", - Direction: "EGRESS", - SrcIP: "10.224.0.17", - SrcPort: "ANY", - DstIP: "10.224.0.20", - DstPort: "80", - Protocol: "tcp", - }, { RuleType: "NOT ALLOWED", Direction: "EGRESS", - SrcIP: "10.224.0.17", + SrcIP: selectedPodIP, SrcPort: "ANY", DstIP: "ANY", DstPort: "ANY", diff --git a/npm/pkg/dataplane/debug/version_mode_test.go b/npm/pkg/dataplane/debug/version_mode_test.go new file mode 100644 index 00000000000..2bad785d609 --- /dev/null +++ b/npm/pkg/dataplane/debug/version_mode_test.go @@ -0,0 +1,116 @@ +package debug + +import ( + "testing" + + common "github.com/Azure/azure-container-networking/npm/pkg/controlplane/controllers/common" + "github.com/Azure/azure-container-networking/npm/pkg/controlplane/translation" + "github.com/Azure/azure-container-networking/npm/pkg/dataplane/ipsets" + "github.com/Azure/azure-container-networking/npm/pkg/dataplane/pb" + "github.com/Azure/azure-container-networking/npm/util" + "github.com/stretchr/testify/require" + networkingv1 "k8s.io/api/networking/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestV1NamespacePrefixDoesNotSelectV2(t *testing.T) { + const labelKey = "nslabel-team" + cache := &common.Cache{NsMap: map[string]*common.Namespace{ + anchorPeerNamespace: {LabelsMap: map[string]string{labelKey: matchedTeamValue}}, + }} + set := &pb.RuleResponse_SetInfo{ + Name: labelKey, Type: pb.SetType_KEYLABELOFNAMESPACE, Included: true, + } + matched, err := matchNamespaceAnchorConditions( + "src", &common.NpmPod{Namespace: anchorPeerNamespace}, + []*pb.RuleResponse_SetInfo{set}, &pb.RuleResponse{}, cache, false, + ) + require.NoError(t, err) + require.True(t, matched, "v1 user-controlled names must not enable the v2 pre-check") +} + +func TestV2PodOnlyPeerRequiresNamespace(t *testing.T) { + allow := &pb.RuleResponse{ + Allowed: true, + SrcList: []*pb.RuleResponse_SetInfo{ + {Name: util.PodLabelPrefix + "app:shared", Type: pb.SetType_KEYLABELOFPOD, Included: true}, + {Name: util.NamespacePrefix + anchorPeerNamespace, Type: pb.SetType_NAMESPACE, Included: true}, + }, + } + target := &common.NpmPod{Namespace: anchorTargetNamespace} + deny := &pb.RuleResponse{DstList: []*pb.RuleResponse_SetInfo{{ + Name: util.NamespacePrefix + anchorTargetNamespace, Type: pb.SetType_NAMESPACE, Included: true, + }}} + rules := map[*pb.RuleResponse]struct{}{allow: {}, deny: {}} + for _, namespace := range []string{anchorPeerNamespace, "different"} { + peer := &common.NpmPod{Namespace: namespace, Labels: map[string]string{diagnosticAppLabelKey: diagnosticSharedValue}} + hits, _, _, err := getHitRules(peer, target, rules, &common.Cache{}, true) + require.NoError(t, err) + want := []*pb.RuleResponse{deny} + if namespace == anchorPeerNamespace { + want = append(want, allow) + } + require.ElementsMatch(t, want, hits) + } +} + +func TestV2PodConditionsWithoutNamespaceAreConjunctive(t *testing.T) { + peer := &common.NpmPod{Namespace: anchorPeerNamespace, Labels: map[string]string{diagnosticAppLabelKey: diagnosticSharedValue, "role": otherLabelValue}} + allow := &pb.RuleResponse{Allowed: true, SrcList: []*pb.RuleResponse_SetInfo{ + {Name: util.PodLabelPrefix + "app:shared", Type: pb.SetType_KEYLABELOFPOD, Included: true}, + {Name: util.PodLabelPrefix + "role:required", Type: pb.SetType_KEYLABELOFPOD, Included: true}, + }} + deny := &pb.RuleResponse{DstList: []*pb.RuleResponse_SetInfo{{ + Name: util.NamespacePrefix + anchorTargetNamespace, Type: pb.SetType_NAMESPACE, Included: true, + }}} + target := &common.NpmPod{Namespace: anchorTargetNamespace} + rules := map[*pb.RuleResponse]struct{}{allow: {}, deny: {}} + hits, _, _, err := getHitRules(peer, target, rules, &common.Cache{}, true) + require.NoError(t, err) + require.ElementsMatch(t, []*pb.RuleResponse{deny}, hits) + peer.Labels["role"] = "required" + hits, _, _, err = getHitRules(peer, target, rules, &common.Cache{}, true) + require.NoError(t, err) + require.ElementsMatch(t, []*pb.RuleResponse{allow, deny}, hits) +} + +func TestV2NestedSelectorUsesTranslatedLabelKey(t *testing.T) { + const labelKey = diagnosticAppLabelKey + policy := &networkingv1.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: "nested", Namespace: anchorPeerNamespace}, + Spec: networkingv1.NetworkPolicySpec{ + PodSelector: metav1.LabelSelector{MatchExpressions: []metav1.LabelSelectorRequirement{{ + Key: labelKey, Operator: metav1.LabelSelectorOpIn, Values: []string{"one", "two"}, + }}}, + PolicyTypes: []networkingv1.PolicyType{networkingv1.PolicyTypeIngress}, + Ingress: []networkingv1.NetworkPolicyIngressRule{{}}, + }, + } + translated, err := translation.TranslatePolicy(policy, false) + require.NoError(t, err) + var nestedName string + for _, set := range translated.PodSelectorIPSets { + if set.Metadata.Type == ipsets.NestedLabelOfPod { + nestedName = util.NestedLabelPrefix + set.Metadata.Name + } + } + require.NotEmpty(t, nestedName) + for _, test := range []struct { + name string + labels map[string]string + want bool + }{ + {"first value", map[string]string{labelKey: "one"}, true}, + {"second value", map[string]string{labelKey: "two"}, true}, + {"different value", map[string]string{labelKey: otherLabelValue}, false}, + {"missing key", nil, false}, + {"policy identity is not a key", map[string]string{translated.PolicyKey: labelKey}, false}, + } { + t.Run(test.name, func(t *testing.T) { + set := &pb.RuleResponse_SetInfo{Name: nestedName, Type: pb.SetType_NESTEDLABELOFPOD, Included: true} + matched, err := evaluateSetInfo("src", set, &common.NpmPod{Labels: test.labels}, &pb.RuleResponse{}, &common.Cache{}, true) + require.NoError(t, err) + require.Equal(t, test.want, matched) + }) + } +} diff --git a/npm/pkg/dataplane/policies/negative_matches_windows_test.go b/npm/pkg/dataplane/policies/negative_matches_windows_test.go new file mode 100644 index 00000000000..03b7767636a --- /dev/null +++ b/npm/pkg/dataplane/policies/negative_matches_windows_test.go @@ -0,0 +1,28 @@ +package policies + +import ( + "fmt" + "testing" + + "github.com/Azure/azure-container-networking/npm/pkg/dataplane/ipsets" + "github.com/stretchr/testify/require" +) + +func TestWindowsACLRejectsNegativeSetsOnEitherSide(t *testing.T) { + for _, direction := range []Direction{Ingress, Egress} { + for _, destination := range []bool{false, true} { + t.Run(fmt.Sprintf("%s/destination=%t", direction, destination), func(t *testing.T) { + acl := NewACLPolicy(Allowed, direction) + acl.Protocol = TCP + negative := NewSetInfo("tenant", ipsets.KeyLabelOfNamespace, false, SrcMatch) + if destination { + acl.DstList = []SetInfo{negative} + } else { + acl.SrcList = []SetInfo{negative} + } + _, err := acl.convertToAclSettings("test-policy") + require.ErrorIs(t, err, ErrNegativeMatchsNotSupported) + }) + } + } +} diff --git a/npm/pkg/dataplane/policies/policy_windows.go b/npm/pkg/dataplane/policies/policy_windows.go index d09f6716386..49b28d87017 100644 --- a/npm/pkg/dataplane/policies/policy_windows.go +++ b/npm/pkg/dataplane/policies/policy_windows.go @@ -73,6 +73,11 @@ func (acl *ACLPolicy) convertToAclSettings(aclID string) (*NPMACLPolSettings, er return policySettings, ErrNegativeMatchsNotSupported } } + for _, setInfo := range acl.DstList { + if !setInfo.Included { + return policySettings, ErrNegativeMatchsNotSupported + } + } if !acl.checkIPSets() { return policySettings, ErrNamedPortsNotSupported diff --git a/npm/pkg/dataplane/policies/policymanager_linux_test.go b/npm/pkg/dataplane/policies/policymanager_linux_test.go index 8fa044e373a..2a95f24d5e0 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.KubeAllNamespacesFlagV2, 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.KubeAllNamespacesFlagV2, 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/pkg/dataplane/testdata/iptablesave-v2 b/npm/pkg/dataplane/testdata/iptablesave-v2 index a4e2768b735..19487ecfaee 100644 --- a/npm/pkg/dataplane/testdata/iptablesave-v2 +++ b/npm/pkg/dataplane/testdata/iptablesave-v2 @@ -375,8 +375,8 @@ COMMIT -A AZURE-NPM-EGRESS -m set --match-set azure-npm-4272224941 src -m set --match-set azure-npm-2064349730 src -m comment --comment "EGRESS-POLICY-kube-system/konnectivity-agent-FROM-podlabel-app:konnectivity-agent-AND-ns-kube-system-IN-ns-kube-system" -j AZURE-NPM-EGRESS-3618314628 -A AZURE-NPM-EGRESS -m mark --mark 0x5000 -m comment --comment DROP-ON-EGRESS-DROP-MARK-0x5000 -j DROP -A AZURE-NPM-EGRESS -m mark --mark 0x2000 -m comment --comment ACCEPT-ON-INGRESS-ALLOW-MARK-0x2000 -j AZURE-NPM-ACCEPT --A AZURE-NPM-EGRESS-2697641196 -p tcp -m tcp --dport 80 -m set --match-set azure-npm-2146053937 dst -m set --match-set azure-npm-2682470511 dst -m comment --comment "ALLOW-TO-nslabel-ns:y-AND-nestedlabel-pod:a:b-ON-TCP-TO-PORT-80" -j AZURE-NPM-ACCEPT --A AZURE-NPM-EGRESS-2697641196 -p tcp -m tcp --dport 80 -m set --match-set azure-npm-2095721080 dst -m set --match-set azure-npm-2682470511 dst -m comment --comment "ALLOW-TO-nslabel-ns:z-AND-nestedlabel-pod:a:b-ON-TCP-TO-PORT-80" -j AZURE-NPM-ACCEPT +-A AZURE-NPM-EGRESS-2697641196 -p tcp -m tcp --dport 80 -m set --match-set azure-npm-2146053937 dst -m set --match-set azure-npm-5p1pifvksz7fldlldq08 dst -m comment --comment "ALLOW-TO-nslabel-ns:y-AND-nestedlabel-y/test-policy:pod:a:b-ON-TCP-TO-PORT-80" -j AZURE-NPM-ACCEPT +-A AZURE-NPM-EGRESS-2697641196 -p tcp -m tcp --dport 80 -m set --match-set azure-npm-2095721080 dst -m set --match-set azure-npm-5p1pifvksz7fldlldq08 dst -m comment --comment "ALLOW-TO-nslabel-ns:z-AND-nestedlabel-y/test-policy:pod:a:b-ON-TCP-TO-PORT-80" -j AZURE-NPM-ACCEPT -A AZURE-NPM-EGRESS-2697641196 -p udp -m udp --dport 53 -m comment --comment ALLOW-ALL-ON-UDP-TO-PORT-53 -j AZURE-NPM-ACCEPT -A AZURE-NPM-EGRESS-2697641196 -p tcp -m tcp --dport 53 -m comment --comment ALLOW-ALL-ON-TCP-TO-PORT-53 -j AZURE-NPM-ACCEPT -A AZURE-NPM-EGRESS-2697641196 -m comment --comment DROP-ALL -j MARK --set-xmark 0x5000/0xffffffff @@ -384,8 +384,8 @@ COMMIT -A AZURE-NPM-INGRESS -m set --match-set azure-npm-2064349730 dst -m comment --comment "INGRESS-POLICY-kube-system/default-deny-ingress-TO-ns-kube-system-IN-ns-kube-system" -j AZURE-NPM-INGRESS-3750705395 -A AZURE-NPM-INGRESS -m set --match-set azure-npm-3922407721 dst -m set --match-set azure-npm-2837910840 dst -m comment --comment "INGRESS-POLICY-y/base-TO-podlabel-pod:a-AND-ns-y-IN-ns-y" -j AZURE-NPM-INGRESS-2697641196 -A AZURE-NPM-INGRESS -m mark --mark 0x4000 -m comment --comment DROP-ON-INGRESS-DROP-MARK-0x4000 -j DROP --A AZURE-NPM-INGRESS-2697641196 -p tcp -m tcp --dport 80 -m set --match-set azure-npm-2129276318 src -m set --match-set azure-npm-55798953 src -m comment --comment "ALLOW-FROM-nslabel-ns:x-AND-nestedlabel-pod:b:c-ON-TCP-TO-PORT-80" -j AZURE-NPM-INGRESS-ALLOW-MARK --A AZURE-NPM-INGRESS-2697641196 -p tcp -m tcp --dport 80 -m set --match-set azure-npm-2146053937 src -m set --match-set azure-npm-55798953 src -m comment --comment "ALLOW-FROM-nslabel-ns:y-AND-nestedlabel-pod:b:c-ON-TCP-TO-PORT-80" -j AZURE-NPM-INGRESS-ALLOW-MARK +-A AZURE-NPM-INGRESS-2697641196 -p tcp -m tcp --dport 80 -m set --match-set azure-npm-2129276318 src -m set --match-set azure-npm-67n0hp4c1th5oiesg5w0 src -m comment --comment "ALLOW-FROM-nslabel-ns:x-AND-nestedlabel-y/test-policy:pod:b:c-ON-TCP-TO-PORT-80" -j AZURE-NPM-INGRESS-ALLOW-MARK +-A AZURE-NPM-INGRESS-2697641196 -p tcp -m tcp --dport 80 -m set --match-set azure-npm-2146053937 src -m set --match-set azure-npm-67n0hp4c1th5oiesg5w0 src -m comment --comment "ALLOW-FROM-nslabel-ns:y-AND-nestedlabel-y/test-policy:pod:b:c-ON-TCP-TO-PORT-80" -j AZURE-NPM-INGRESS-ALLOW-MARK -A AZURE-NPM-INGRESS-2697641196 -m comment --comment DROP-ALL -j MARK --set-xmark 0x4000/0xffffffff -A AZURE-NPM-INGRESS-3750705395 -m comment --comment DROP-ALL -j MARK --set-xmark 0x4000/0xffffffff -A AZURE-NPM-INGRESS-ALLOW-MARK -m comment --comment SET-INGRESS-ALLOW-MARK-0x2000 -j MARK --set-xmark 0x2000/0xffffffff diff --git a/npm/pkg/dataplane/testdata/npmcachev2.json b/npm/pkg/dataplane/testdata/npmcachev2.json index f65b803020c..6102ac8620c 100644 --- a/npm/pkg/dataplane/testdata/npmcachev2.json +++ b/npm/pkg/dataplane/testdata/npmcachev2.json @@ -442,7 +442,7 @@ "azure-npm-1343132199": "podlabel-version", "azure-npm-1385180724": "podlabel-pod-template-hash", "azure-npm-1529935048": "podlabel-component:tunnel", - "azure-npm-1639206293": "nslabel-all-namespaces", + "azure-npm-51v3fhaia2ucu2kg03hl": "nslabel-:all-namespaces", "azure-npm-1802501696": "nslabel-kubernetes.io/cluster-service", "azure-npm-1883894896": "ns-kube-node-lease", "azure-npm-1889013859": "podlabel-pod-template-hash:69c47794", @@ -459,7 +459,7 @@ "azure-npm-2540899149": "podlabel-k8s-app", "azure-npm-2547206700": "podlabel-pod-template-hash:774f99dbf4", "azure-npm-2647803239": "nslabel-control-plane:true", - "azure-npm-2682470511": "nestedlabel-pod:a:b", + "azure-npm-5p1pifvksz7fldlldq08": "nestedlabel-y/test-policy:pod:a:b", "azure-npm-2714724634": "podlabel-k8s-app:kube-dns", "azure-npm-2764516068": "nslabel-addonmanager.kubernetes.io/mode", "azure-npm-2837910840": "ns-y", @@ -486,7 +486,7 @@ "azure-npm-4272224941": "podlabel-app:konnectivity-agent", "azure-npm-4284971813": "namedport:serve-80-tcp", "azure-npm-483924252": "nslabel-ns", - "azure-npm-55798953": "nestedlabel-pod:b:c", + "azure-npm-67n0hp4c1th5oiesg5w0": "nestedlabel-y/test-policy:pod:b:c", "azure-npm-708060905": "podlabel-version:v20", "azure-npm-71974944": "namedport:dns", "azure-npm-784554818": "ns-default", diff --git a/npm/util/const.go b/npm/util/const.go index e323d618b0e..fb926ab5422 100644 --- a/npm/util/const.go +++ b/npm/util/const.go @@ -6,10 +6,13 @@ import "k8s.io/klog" // kubernetes related constants. const ( - KubeSystemFlag string = "kube-system" - KubePodTemplateHashFlag string = "pod-template-hash" - KubeAllPodsFlag string = "all-pod" - KubeAllNamespacesFlag string = "all-namespaces" + KubeSystemFlag string = "kube-system" + KubePodTemplateHashFlag string = "pod-template-hash" + KubeAllPodsFlag string = "all-pod" + KubeAllNamespacesFlag string = "all-namespaces" + // A leading colon cannot occur in a label key or a key:value label identity. + // Keep the v1 name unchanged and separate v2 aggregate membership from labels. + KubeAllNamespacesFlagV2 string = ":all-namespaces" KubeAppFlag string = "k8s-app" KubeProxyFlag string = "kube-proxy" KubePodStatusFailedFlag string = "Failed" diff --git a/npm/util/util.go b/npm/util/util.go index daefd4d1b4c..7fd7d091659 100644 --- a/npm/util/util.go +++ b/npm/util/util.go @@ -34,6 +34,13 @@ const ( var ErrEmptyNodeIP = errors.New("error: node IP is empty") +var ( + // ErrInvalidCIDR identifies a CIDR that cannot be parsed. + ErrInvalidCIDR = errors.New("util: invalid CIDR") + // ErrUnsupportedIPFamily identifies a valid CIDR outside the supported IPv4 family. + ErrUnsupportedIPFamily = errors.New("util: unsupported IP family") +) + // regex to get minor version var re = regexp.MustCompile("[0-9]+") @@ -363,6 +370,30 @@ 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 distinguishes invalid CIDRs from unsupported IP families. 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, error) { + // Retain accepted spellings, including zero-padded prefix lengths. + _, network, err := net.ParseCIDR(s) + if err != nil { + return "", fmt.Errorf("%w: %w", ErrInvalidCIDR, err) + } + if network.IP.To4() == nil || len(network.Mask) != net.IPv4len { + return "", ErrUnsupportedIPFamily + } + return network.String(), nil +} + +// 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..acb896061be 100644 --- a/npm/util/util_test.go +++ b/npm/util/util_test.go @@ -1,6 +1,7 @@ package util import ( + "net" "reflect" "strings" "testing" @@ -514,3 +515,112 @@ 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, + "10.1.2.3/024": canonicalNet24, + canonicalNet24: canonicalNet24, + singleHostCIDR: singleHostCIDR, + } + for in, want := range canonical { + got, err := NormalizeCIDR(in) + require.NoError(t, err, "NormalizeCIDR(%q) must succeed", in) + require.Equal(t, want, got, "NormalizeCIDR(%q)", in) + } + + for _, test := range []struct { + cidr string + want error + }{ + {"", ErrInvalidCIDR}, + {"10.0.0.1", ErrInvalidCIDR}, + {"not-a-cidr", ErrInvalidCIDR}, + {"10.0.0.0/33", ErrInvalidCIDR}, + {"2001:db8::/32", ErrUnsupportedIPFamily}, + {"::/0", ErrUnsupportedIPFamily}, + {"::ffff:192.0.2.1/128", ErrUnsupportedIPFamily}, + } { + t.Run(test.cidr, func(t *testing.T) { + got, err := NormalizeCIDR(test.cidr) + require.ErrorIs(t, err, test.want) + require.Empty(t, got) + }) + } +} + +func FuzzNormalizeCIDRCompatibility(f *testing.F) { + for _, input := range []string{ + allIPv4CIDR, singleHostCIDR, "192.168.7.19/24", "192.168.7.19/0", + "2001:db8:1::/48", "::ffff:192.0.2.1/128", "0::/00", "broken", "", + } { + f.Add(input) + } + f.Fuzz(func(t *testing.T, input string) { + // Preserve the accepted values and canonical output of the previous classifier. + _, network, parseErr := net.ParseCIDR(input) + got, err := NormalizeCIDR(input) + switch { + case parseErr != nil: + require.ErrorIs(t, err, ErrInvalidCIDR) + case network.IP.To4() == nil || len(network.Mask) != net.IPv4len: + require.ErrorIs(t, err, ErrUnsupportedIPFamily) + default: + require.NoError(t, err) + require.Equal(t, network.String(), got) + } + if err != nil { + require.Empty(t, got) + } + }) +}