Fix K8s NetworkPolicy Deny All Audit Logging (#6855)

EnableLogging was not set for deny-all K8s NetworkPolicies even with Namespace properly annotated. This PR fixes the issue by configuring enableLogging field. Fixes #6844 Signed-off-by: Qiyue Yao <[email protected]>
antrea-io · Dec 13, 2024 · 831b83e · 831b83e
1 parent 11e176f
commit 831b83e
Show file tree

Hide file tree

Showing 3 changed files with 83 additions and 20 deletions.
diff --git a/pkg/controller/networkpolicy/networkpolicy_controller.go b/pkg/controller/networkpolicy/networkpolicy_controller.go
@@ -119,10 +119,6 @@ var (
 	matchAllPodsPeer = networkingv1.NetworkPolicyPeer{
 		NamespaceSelector: &metav1.LabelSelector{},
 	}
-	// denyAllIngressRule is a NetworkPolicyRule which denies all ingress traffic.
-	denyAllIngressRule = controlplane.NetworkPolicyRule{Direction: controlplane.DirectionIn}
-	// denyAllEgressRule is a NetworkPolicyRule which denies all egress traffic.
-	denyAllEgressRule = controlplane.NetworkPolicyRule{Direction: controlplane.DirectionOut}
 	// defaultAction is a RuleAction which sets the default Action for the NetworkPolicy rule.
 	defaultAction = secv1beta1.RuleActionAllow
 )
@@ -772,12 +768,12 @@ func (n *NetworkPolicyController) processNetworkPolicy(np *networkingv1.NetworkP
 	// If ingress isolation is specified explicitly and there's no ingress rule, append a deny-all ingress rule.
 	// See https://kubernetes.io/docs/concepts/services-networking/network-policies/#default-deny-all-ingress-traffic
 	if ingressIsolated && !ingressRuleExists {
-		rules = append(rules, denyAllIngressRule)
+		rules = append(rules, denyAllRule(controlplane.DirectionIn, enableLogging))
 	}
 	// If egress isolation is specified explicitly and there's no egress rule, append a deny-all egress rule.
 	// See https://kubernetes.io/docs/concepts/services-networking/network-policies/#default-deny-all-egress-traffic
 	if egressIsolated && !egressRuleExists {
-		rules = append(rules, denyAllEgressRule)
+		rules = append(rules, denyAllRule(controlplane.DirectionOut, enableLogging))
 	}
 
 	internalNetworkPolicy := &antreatypes.NetworkPolicy{
@@ -1726,6 +1722,14 @@ func (n *NetworkPolicyController) cleanupOrphanGroups(internalNetworkPolicy *ant
 	}
 }
 
+// denyAllRule returns a NetworkPolicyRule which denies all traffic in the given direction.
+func denyAllRule(direction controlplane.Direction, enableLogging bool) controlplane.NetworkPolicyRule {
+	return controlplane.NetworkPolicyRule{
+		Direction:     direction,
+		EnableLogging: enableLogging,
+	}
+}
+
 // ipStrToIPAddress converts an IP string to a controlplane.IPAddress.
 // nil will returned if the IP string is not valid.
 func ipStrToIPAddress(ip string) controlplane.IPAddress {

diff --git a/pkg/controller/networkpolicy/networkpolicy_controller_test.go b/pkg/controller/networkpolicy/networkpolicy_controller_test.go
@@ -2000,7 +2000,41 @@ func TestProcessNetworkPolicy(t *testing.T) {
 					UID:       "uidC",
 				},
 				Rules: []controlplane.NetworkPolicyRule{
-					denyAllIngressRule,
+					denyAllRule(controlplane.DirectionIn, false),
+				},
+				AppliedToGroups: []string{getNormalizedUID(antreatypes.NewGroupSelector("nsA", &metav1.LabelSelector{}, nil, nil, nil).NormalizedName)},
+			},
+			expectedAppliedToGroups: 1,
+			expectedAddressGroups:   0,
+		},
+		{
+			name: "default-deny-ingress-enable-logging",
+			existingObjects: []runtime.Object{
+				&corev1.Namespace{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:        "nsA",
+						Annotations: map[string]string{"networkpolicy.antrea.io/enable-logging": "true"},
+					},
+				},
+			},
+			inputPolicy: &networkingv1.NetworkPolicy{
+				ObjectMeta: metav1.ObjectMeta{Namespace: "nsA", Name: "npC", UID: "uidC"},
+				Spec: networkingv1.NetworkPolicySpec{
+					PodSelector: metav1.LabelSelector{},
+					PolicyTypes: []networkingv1.PolicyType{networkingv1.PolicyTypeIngress},
+				},
+			},
+			expectedPolicy: &antreatypes.NetworkPolicy{
+				UID:  "uidC",
+				Name: "uidC",
+				SourceRef: &controlplane.NetworkPolicyReference{
+					Type:      controlplane.K8sNetworkPolicy,
+					Namespace: "nsA",
+					Name:      "npC",
+					UID:       "uidC",
+				},
+				Rules: []controlplane.NetworkPolicyRule{
+					denyAllRule(controlplane.DirectionIn, true),
 				},
 				AppliedToGroups: []string{getNormalizedUID(antreatypes.NewGroupSelector("nsA", &metav1.LabelSelector{}, nil, nil, nil).NormalizedName)},
 			},
@@ -2025,7 +2059,9 @@ func TestProcessNetworkPolicy(t *testing.T) {
 					Name:      "npA",
 					UID:       "uidA",
 				},
-				Rules:           []controlplane.NetworkPolicyRule{denyAllEgressRule},
+				Rules: []controlplane.NetworkPolicyRule{
+					denyAllRule(controlplane.DirectionOut, false),
+				},
 				AppliedToGroups: []string{getNormalizedUID(antreatypes.NewGroupSelector("nsA", &metav1.LabelSelector{}, nil, nil, nil).NormalizedName)},
 			},
 			expectedAppliedToGroups: 1,

diff --git a/test/e2e/antreapolicy_test.go b/test/e2e/antreapolicy_test.go
@@ -2752,6 +2752,7 @@ func testAuditLoggingBasic(t *testing.T, data *TestData) {
 // tests both Allow traffic by K8s NP and Drop traffic by implicit K8s policy drop
 func testAuditLoggingEnableK8s(t *testing.T, data *TestData) {
 	failOnError(data.updateNamespaceWithAnnotations(getNS("x"), map[string]string{networkpolicy.EnableNPLoggingAnnotationKey: "true"}), t)
+	failOnError(data.updateNamespaceWithAnnotations(getNS("y"), map[string]string{networkpolicy.EnableNPLoggingAnnotationKey: "true"}), t)
 	// Add a K8s namespaced NetworkPolicy in ns x that allow ingress traffic from
 	// Pod x/b to x/a which default denies other ingress including from Pod x/c to x/a
 	npName := "allow-x-b-to-x-a"
@@ -2767,40 +2768,62 @@ func testAuditLoggingEnableK8s(t *testing.T, data *TestData) {
 	failOnError(err, t)
 	failOnError(waitForResourceReady(t, timeout, knp), t)
 
+	// Add a K8s namespaced NetworkPolicy with no ingress rule that triggers the
+	// default deny all ingress traffic.
+	npName2 := "default-deny-all-y"
+	k8sNPBuilder2 := &NetworkPolicySpecBuilder{}
+	k8sNPBuilder2 = k8sNPBuilder2.SetName(getNS("y"), npName2).SetTypeIngress()
+
+	knp2, err := k8sUtils.CreateOrUpdateNetworkPolicy(k8sNPBuilder2.Get())
+	failOnError(err, t)
+	failOnError(waitForResourceReady(t, timeout, knp2), t)
+
 	podXA, err := k8sUtils.GetPodByLabel(getNS("x"), "a")
 	if err != nil {
 		t.Errorf("Failed to get Pod in Namespace x with label 'pod=a': %v", err)
 	}
+	podYA, err := k8sUtils.GetPodByLabel(getNS("y"), "a")
+	if err != nil {
+		t.Errorf("Failed to get Pod in Namespace y with label 'pod=a': %v", err)
+	}
 
-	// matcher1 is for connections allowed by the K8s NP
-	matcher1 := NewAuditLogMatcher(npRef, "<nil>", "Ingress", "Allow")
-	// matcher2 is for connections dropped by the isolated behavior of the K8s NP
-	matcher2 := NewAuditLogMatcher("K8sNetworkPolicy", "<nil>", "Ingress", "Drop")
+	// matcherX1 is for connections allowed by the K8s NP1
+	matcherX1 := NewAuditLogMatcher(npRef, "<nil>", "Ingress", "Allow")
+	// matcherX2 is for connections dropped by the isolated behavior of the K8s NP1
+	matcherX2 := NewAuditLogMatcher("K8sNetworkPolicy", "<nil>", "Ingress", "Drop")
+	// matcherY is for connections dropped by the default deny all behavior of the K8s NP2
+	matcherY := NewAuditLogMatcher("K8sNetworkPolicy", "<nil>", "Ingress", "Drop")
 
-	appliedToRef := fmt.Sprintf("%s/%s", podXA.Namespace, podXA.Name)
+	appliedToRefX := fmt.Sprintf("%s/%s", podXA.Namespace, podXA.Name)
+	appliedToRefY := fmt.Sprintf("%s/%s", podYA.Namespace, podYA.Name)
 
 	// generate some traffic that will be dropped by implicit K8s policy drop
 	var wg sync.WaitGroup
-	oneProbe := func(ns1, pod1, ns2, pod2 string, matcher *auditLogMatcher) {
+	oneProbe := func(appliedToRef, ns1, pod1, ns2, pod2 string, matcher *auditLogMatcher) {
 		matcher.AddProbe(appliedToRef, ns1, pod1, ns2, pod2, p80)
 		wg.Add(1)
 		go func() {
 			defer wg.Done()
 			k8sUtils.Probe(ns1, pod1, ns2, pod2, p80, ProtocolTCP, nil, nil)
 		}()
 	}
-	oneProbe(getNS("x"), "b", getNS("x"), "a", matcher1)
-	oneProbe(getNS("x"), "c", getNS("x"), "a", matcher2)
+	oneProbe(appliedToRefX, getNS("x"), "b", getNS("x"), "a", matcherX1)
+	oneProbe(appliedToRefX, getNS("x"), "c", getNS("x"), "a", matcherX2)
+	oneProbe(appliedToRefY, getNS("y"), "b", getNS("y"), "a", matcherY)
 	wg.Wait()
 
-	// nodeName is guaranteed to be set at this stage, since the framework waits for all Pods to be in Running phase
-	nodeName := podXA.Spec.NodeName
-	checkAuditLoggingResult(t, data, nodeName, "K8sNetworkPolicy", append(matcher1.Matchers(), matcher2.Matchers()...))
+	// pod NodeName is guaranteed to be set at this stage, since the framework waits for all Pods to be in Running phase
+	checkAuditLoggingResult(t, data, podXA.Spec.NodeName, "K8sNetworkPolicy", append(matcherX1.Matchers(), matcherX2.Matchers()...))
+	checkAuditLoggingResult(t, data, podYA.Spec.NodeName, "K8sNetworkPolicy", matcherY.Matchers())
 
-	failOnError(k8sUtils.DeleteNetworkPolicy(getNS("x"), "allow-x-b-to-x-a"), t)
+	failOnError(k8sUtils.DeleteNetworkPolicy(getNS("x"), npName), t)
+	failOnError(k8sUtils.DeleteNetworkPolicy(getNS("y"), npName2), t)
 	failOnError(data.UpdateNamespace(getNS("x"), func(namespace *v1.Namespace) {
 		delete(namespace.Annotations, networkpolicy.EnableNPLoggingAnnotationKey)
 	}), t)
+	failOnError(data.UpdateNamespace(getNS("y"), func(namespace *v1.Namespace) {
+		delete(namespace.Annotations, networkpolicy.EnableNPLoggingAnnotationKey)
+	}), t)
 }
 
 // testAuditLoggingK8sService tests that audit logs are generated for K8s Service access