Fix K8s NP Service Logging (antrea-io#4780)

Service for K8s NetworkPolicy is not logging as expected.
This solution checks the CnpDenyRegMark in the case of K8s
default drop before fetching the conjunction ID match.
And also renames Cnp to AP in CnpConjIDField and CnpDenyRegMark.

Fixes antrea-io#4765

Signed-off-by: Qiyue Yao <yaoq@vmware.com>

pkg/agent/controller/networkpolicy/audit_logging.go

@@ -206,26 +206,35 @@ func getNetworkPolicyInfo(pktIn *ofctrl.PacketIn, c *Controller, ob *logInfo) er
 		}
 	}
 
-	// Set match to corresponding ingress/egress reg according to disposition.
-	match = getMatch(matchers, tableID, disposition)
-
-	if match != nil {
-		// Get NetworkPolicy full name and OF priority of the conjunction.
-		conjID, err := getInfoInReg(match, nil)
+	// Get K8s default deny action, if traffic is default deny, no conjunction could be matched.
+	if match = getMatchRegField(matchers, openflow.APDenyRegMark.GetField()); match != nil {
+		cnpDenyRegVal, err := getInfoInReg(match, openflow.APDenyRegMark.GetField().GetRange().ToNXRange())
 		if err != nil {
-			return fmt.Errorf("received error while unloading conjunction id from reg: %v", err)
-		}
-		ob.npRef, ob.ofPriority, ob.ruleName = c.ofClient.GetPolicyInfoFromConjunction(conjID)
-		if ob.npRef == "" || ob.ofPriority == "" {
-			return fmt.Errorf("networkpolicy not found for conjunction id: %v", conjID)
+			return fmt.Errorf("received error while unloading deny mark from reg: %v", err)
 		}
-		// Placeholder for K8s NetworkPolicies without rule names.
-		if ob.ruleName == "" {
-			ob.ruleName = "<nil>"
+		isK8sDefaultDeny := (cnpDenyRegVal == 0) && (disposition == openflow.DispositionDrop || disposition == openflow.DispositionRej)
+		if isK8sDefaultDeny {
+			// For K8s NetworkPolicy implicit drop action, we cannot get Namespace/name.
+			ob.npRef, ob.ofPriority, ob.ruleName = string(v1beta2.K8sNetworkPolicy), "<nil>", "<nil>"
+			return nil
 		}
-	} else {
-		// For K8s NetworkPolicy implicit drop action, we cannot get Namespace/name.
-		ob.npRef, ob.ofPriority, ob.ruleName = string(v1beta2.K8sNetworkPolicy), "<nil>", "<nil>"
+	}
+
+	// Set match to corresponding conjunction ID field according to disposition.
+	match = getMatch(matchers, tableID, disposition)
+
+	// Get NetworkPolicy full name and OF priority of the conjunction.
+	conjID, err := getInfoInReg(match, nil)
+	if err != nil {
+		return fmt.Errorf("received error while unloading conjunction id from reg: %v", err)
+	}
+	ob.npRef, ob.ofPriority, ob.ruleName = c.ofClient.GetPolicyInfoFromConjunction(conjID)
+	if ob.npRef == "" || ob.ofPriority == "" {
+		return fmt.Errorf("networkpolicy not found for conjunction id: %v", conjID)
+	}
+	// Placeholder for K8s NetworkPolicies without rule names.
+	if ob.ruleName == "" {
+		ob.ruleName = "<nil>"
 	}
 	return nil
 }

pkg/agent/controller/networkpolicy/audit_logging_test.go

@@ -260,7 +260,8 @@ func TestGetNetworkPolicyInfo(t *testing.T) {
 	testK8sRef := "K8sNetworkPolicy:default/test-anp"
 	testPriority, testRule := "61800", "test-rule"
 	allowDispositionData := []byte{0x11, 0x00, 0x00, 0x11}
-	dropDispositionData := []byte{0x11, 0x00, 0x08, 0x11}
+	dropCNPDispositionData := []byte{0x11, 0x00, 0x0c, 0x11}
+	dropK8sDispositionData := []byte{0x11, 0x00, 0x08, 0x11}
 	redirectDispositionData := []byte{0x11, 0x10, 0x00, 0x11}
 	ingressData := []byte{0x11, 0x11, 0x11, 0x11, 0x11, 0x11, 0x11, 0x11}
 	tests := []struct {
@@ -311,7 +312,7 @@ func TestGetNetworkPolicyInfo(t *testing.T) {
 				mockClient.GetPolicyInfoFromConjunction(gomock.Any()).Return(
 					testANPRef, testPriority, testRule)
 			},
-			dispositionData: dropDispositionData,
+			dispositionData: dropCNPDispositionData,
 			wantOb: &logInfo{
 				tableName:   openflow.AntreaPolicyIngressRuleTable.GetName(),
 				disposition: actionDrop,
@@ -323,7 +324,7 @@ func TestGetNetworkPolicyInfo(t *testing.T) {
 		{
 			name:            "K8s Drop",
 			tableID:         openflow.IngressDefaultTable.GetID(),
-			dispositionData: dropDispositionData,
+			dispositionData: dropK8sDispositionData,
 			wantOb: &logInfo{
 				tableName:   openflow.IngressDefaultTable.GetName(),
 				disposition: actionDrop,
@@ -359,7 +360,7 @@ func TestGetNetworkPolicyInfo(t *testing.T) {
 			if tc.expectedCalls != nil {
 				regID := openflow.TFIngressConjIDField.GetRegID()
 				if tc.wantOb.disposition == actionDrop {
-					regID = openflow.CNPConjIDField.GetRegID()
+					regID = openflow.APConjIDField.GetRegID()
 				}
 				ingressMatch := generateMatch(regID, ingressData)
 				matchers = append(matchers, ingressMatch)

pkg/agent/controller/networkpolicy/packetin.go

@@ -78,7 +78,7 @@ func getMatchRegField(matchers *ofctrl.Matchers, field *binding.RegField) *ofctr
 func getMatch(matchers *ofctrl.Matchers, tableID uint8, disposition uint32) *ofctrl.MatchField {
 	// Get match from CNPDenyConjIDReg if disposition is Drop or Reject.
 	if disposition == openflow.DispositionDrop || disposition == openflow.DispositionRej {
-		return getMatchRegField(matchers, openflow.CNPConjIDField)
+		return getMatchRegField(matchers, openflow.APConjIDField)
 	}
 	// Get match from ingress/egress reg if disposition is Allow or Pass.
 	for _, table := range append(openflow.GetAntreaPolicyEgressTables(), openflow.EgressRuleTable) {

pkg/agent/controller/traceflow/packetin.go

@@ -228,7 +228,7 @@ func (c *Controller) parsePacketIn(pktIn *ofctrl.PacketIn) (*crdv1alpha1.Tracefl
 	// Get drop table.
 	if tableID == openflow.EgressMetricTable.GetID() || tableID == openflow.IngressMetricTable.GetID() {
 		ob := getNetworkPolicyObservation(tableID, tableID == openflow.IngressMetricTable.GetID())
-		if match := getMatchRegField(matchers, openflow.CNPConjIDField); match != nil {
+		if match := getMatchRegField(matchers, openflow.APConjIDField); match != nil {
 			notAllowConjInfo, err := getRegValue(match, nil)
 			if err != nil {
 				return nil, nil, nil, err

pkg/agent/openflow/fields.go

@@ -57,8 +57,9 @@ var (
 	// reg0[9]: Field to indicate whether the packet's source / destination MAC address needs to be rewritten.
 	RewriteMACRegMark    = binding.NewOneBitRegMark(0, 9)
 	NotRewriteMACRegMark = binding.NewOneBitZeroRegMark(0, 9)
-	// reg0[10]: Mark to indicate the packet is denied(Drop/Reject).
-	CnpDenyRegMark = binding.NewOneBitRegMark(0, 10)
+	// reg0[10]: Mark to indicate the packet is denied(Drop/Reject) for Antrea Policy.
+	// K8s default drop will not be recorded in this reg.
+	APDenyRegMark = binding.NewOneBitRegMark(0, 10)
 	// reg0[11..12]: Field to indicate disposition of Antrea Policy. It could have more bits to support more dispositions
 	// that Antrea Policy support in the future. Marks in this field include:
 	//   - 0b00: allow
@@ -100,9 +101,9 @@ var (
 	// reg3(NXM_NX_REG3)
 	// Field to store the selected Service Endpoint IP
 	EndpointIPField = binding.NewRegField(3, 0, 31)
-	// Field to store the conjunction ID which is for rule in CNP. It shares the same register with EndpointIPField,
+	// Field to store the conjunction ID which is for rule in Antrea Policy. It shares the same register with EndpointIPField,
 	// since the service selection will finish when a packet hitting NetworkPolicy related rules.
-	CNPConjIDField = binding.NewRegField(3, 0, 31)
+	APConjIDField = binding.NewRegField(3, 0, 31)
 
 	// reg4(NXM_NX_REG4)
 	// reg4[0..15]: Field to store the selected Service Endpoint port.

pkg/agent/openflow/network_policy_test.go

@@ -479,18 +479,18 @@ func TestBatchInstallPolicyRuleFlows(t *testing.T) {
 						Action().CT(true, IngressMetricTable.GetID(), CtZone, nil).LoadToLabelField(10, IngressRuleCTLabel).CTDone().Done(),
 					AntreaPolicyIngressRuleTable.ofTable.BuildFlow(priority100).Cookie(cookiePolicy).
 						MatchConjID(11).
-						Action().LoadToRegField(CNPConjIDField, 11).
-						Action().LoadRegMark(CnpDenyRegMark).
+						Action().LoadToRegField(APConjIDField, 11).
+						Action().LoadRegMark(APDenyRegMark).
 						Action().GotoTable(IngressMetricTable.GetID()).Done(),
 					AntreaPolicyIngressRuleTable.ofTable.BuildFlow(priority200).Cookie(cookiePolicy).
 						MatchConjID(12).
-						Action().LoadToRegField(CNPConjIDField, 12).
-						Action().LoadRegMark(CnpDenyRegMark).
+						Action().LoadToRegField(APConjIDField, 12).
+						Action().LoadRegMark(APDenyRegMark).
 						Action().GotoTable(IngressMetricTable.GetID()).Done(),
 					AntreaPolicyIngressRuleTable.ofTable.BuildFlow(priority201).Cookie(cookiePolicy).
 						MatchConjID(13).
-						Action().LoadToRegField(CNPConjIDField, 13).
-						Action().LoadRegMark(CnpDenyRegMark).
+						Action().LoadToRegField(APConjIDField, 13).
+						Action().LoadRegMark(APDenyRegMark).
 						Action().GotoTable(IngressMetricTable.GetID()).Done(),
 					AntreaPolicyIngressRuleTable.ofTable.BuildFlow(priority100).Cookie(cookiePolicy).
 						MatchProtocol(binding.ProtocolIP).MatchSrcIP(net.ParseIP("192.168.1.40")).
@@ -546,13 +546,13 @@ func TestBatchInstallPolicyRuleFlows(t *testing.T) {
 						MatchProtocol(binding.ProtocolIP).MatchCTStateNew(false).MatchCTLabelField(0, 10, IngressRuleCTLabel).
 						Action().NextTable().Done(),
 					IngressMetricTable.ofTable.BuildFlow(priorityNormal).Cookie(cookiePolicy).
-						MatchRegMark(CnpDenyRegMark).MatchRegFieldWithValue(CNPConjIDField, 11).
+						MatchRegMark(APDenyRegMark).MatchRegFieldWithValue(APConjIDField, 11).
 						Action().Drop().Done(),
 					IngressMetricTable.ofTable.BuildFlow(priorityNormal).Cookie(cookiePolicy).
-						MatchRegMark(CnpDenyRegMark).MatchRegFieldWithValue(CNPConjIDField, 12).
+						MatchRegMark(APDenyRegMark).MatchRegFieldWithValue(APConjIDField, 12).
 						Action().Drop().Done(),
 					IngressMetricTable.ofTable.BuildFlow(priorityNormal).Cookie(cookiePolicy).
-						MatchRegMark(CnpDenyRegMark).MatchRegFieldWithValue(CNPConjIDField, 13).
+						MatchRegMark(APDenyRegMark).MatchRegFieldWithValue(APConjIDField, 13).
 						Action().Drop().Done(),
 					IngressDefaultTable.ofTable.BuildFlow(priority200).Cookie(cookiePolicy).
 						MatchTunnelID(uint64(UnknownLabelIdentity)).

pkg/agent/openflow/pipeline.go

@@ -1667,7 +1667,7 @@ func (f *featureNetworkPolicy) allowRulesMetricFlows(conjunctionID uint32, ingre
 	if metricTable == MulticastEgressMetricTable || metricTable == MulticastIngressMetricTable {
 		flow := metricTable.ofTable.BuildFlow(priorityNormal).
 			Cookie(f.cookieAllocator.Request(f.category).Raw()).
-			MatchRegFieldWithValue(CNPConjIDField, conjunctionID).
+			MatchRegFieldWithValue(APConjIDField, conjunctionID).
 			Action().GotoTable(metricTable.GetNext()).
 			Done()
 		flows = append(flows, flow)
@@ -1696,8 +1696,8 @@ func (f *featureNetworkPolicy) denyRuleMetricFlow(conjunctionID uint32, ingress
 	}
 	return metricTable.ofTable.BuildFlow(priorityNormal).
 		Cookie(f.cookieAllocator.Request(f.category).Raw()).
-		MatchRegMark(CnpDenyRegMark).
-		MatchRegFieldWithValue(CNPConjIDField, conjunctionID).
+		MatchRegMark(APDenyRegMark).
+		MatchRegFieldWithValue(APConjIDField, conjunctionID).
 		Action().Drop().
 		Done()
 }
@@ -1826,7 +1826,7 @@ func (f *featureNetworkPolicy) conjunctionActionFlow(conjunctionID uint32, table
 	// Any matched flow will be resubmitted to next table in corresponding metric tables.
 	if f.enableMulticast && (tableID == MulticastEgressRuleTable.GetID() || tableID == MulticastIngressRuleTable.GetID()) {
 		flow := table.BuildFlow(ofPriority).MatchConjID(conjunctionID).
-			Action().LoadToRegField(CNPConjIDField, conjunctionID).
+			Action().LoadToRegField(APConjIDField, conjunctionID).
 			Action().NextTable().
 			Cookie(f.cookieAllocator.Request(f.category).Raw()).
 			Done()
@@ -1858,8 +1858,8 @@ func (f *featureNetworkPolicy) conjunctionActionDenyFlow(conjunctionID uint32, t
 	}
 	flowBuilder := table.BuildFlow(ofPriority).
 		MatchConjID(conjunctionID).
-		Action().LoadToRegField(CNPConjIDField, conjunctionID).
-		Action().LoadRegMark(CnpDenyRegMark)
+		Action().LoadToRegField(APConjIDField, conjunctionID).
+		Action().LoadRegMark(APDenyRegMark)
 
 	var customReason int
 	if f.enableDenyTracking {