Prioritize L7NP flows over TrafficControl

When applying an L7NP to a Pod, there's a potential issue where creating a TrafficControl CR with a redirect action to the same Pod could bypass the L7 engine. This is due to the fact that both the ct mark `L7NPRedirectCTMark` for identifying L7NP packets and the reg mark `TrafficControlRedirectRegMark` for identifying TrafficControl redirect packets can be set together. In `OutputTable`, the priorities of flows to match the ct mark and the reg mark are the same. Without an additional condition to distinguish between them, packets with both the reg mark and ct mark may be matched by either flow with an equal chance. To rectify this and ensure proper L7NP enforcement, it is crucial to assign a higher priority to the flow that matches the ct mark L7NPRedirectCTMark. This patch also adds an extra parameter `priority` to InstallTrafficControlMarkFlows, which is used to set the priority of the related flows. Signed-off-by: Hongliang Liu <lhongliang@vmware.com>
antrea-io · Dec 5, 2023 · 13d9482 · 13d9482
1 parent 66973b9
commit 13d9482
Show file tree

Hide file tree

Showing 9 changed files with 89 additions and 48 deletions.
diff --git a/pkg/agent/controller/trafficcontrol/controller.go b/pkg/agent/controller/trafficcontrol/controller.go
@@ -920,7 +920,7 @@ func (c *Controller) syncTrafficControl(tcName string) error {
 		for _, port := range sets.List(newOfPorts) {
 			ofPorts = append(ofPorts, uint32(port))
 		}
-		if err = c.ofClient.InstallTrafficControlMarkFlows(tc.Name, ofPorts, targetOFPort, tc.Spec.Direction, tc.Spec.Action); err != nil {
+		if err = c.ofClient.InstallTrafficControlMarkFlows(tc.Name, ofPorts, targetOFPort, tc.Spec.Direction, tc.Spec.Action, openflow.TrafficControlFlowPriorityMedium); err != nil {
 			return err
 		}
 	}

diff --git a/pkg/agent/controller/trafficcontrol/controller_test.go b/pkg/agent/controller/trafficcontrol/controller_test.go
diff --git a/pkg/agent/openflow/client.go b/pkg/agent/openflow/client.go
@@ -39,6 +39,12 @@ import (
 
 const maxRetryForOFSwitch = 5
 
+const (
+	TrafficControlFlowPriorityHigh   = "high"
+	TrafficControlFlowPriorityMedium = "medium"
+	TrafficControlFlowPriorityLow    = "low"
+)
+
 // Client is the interface to program OVS flows for entity connectivity of Antrea.
 type Client interface {
 	// Initialize sets up all basic flows on the specific OVS bridge. It returns a channel which
@@ -323,7 +329,12 @@ type Client interface {
 		igmp ofutil.Message) error
 
 	// InstallTrafficControlMarkFlows installs the flows to mark the packets for a traffic control rule.
-	InstallTrafficControlMarkFlows(name string, sourceOFPorts []uint32, targetOFPort uint32, direction crdv1alpha2.Direction, action crdv1alpha2.TrafficControlAction) error
+	InstallTrafficControlMarkFlows(name string,
+		sourceOFPorts []uint32,
+		targetOFPort uint32,
+		direction crdv1alpha2.Direction,
+		action crdv1alpha2.TrafficControlAction,
+		priority string) error
 
 	// UninstallTrafficControlMarkFlows removes the flows for a traffic control rule.
 	UninstallTrafficControlMarkFlows(name string) error
@@ -1449,8 +1460,17 @@ func (c *client) SendIGMPQueryPacketOut(
 	return c.bridge.SendPacketOut(packetOutObj)
 }
 
-func (c *client) InstallTrafficControlMarkFlows(name string, sourceOFPorts []uint32, targetOFPort uint32, direction crdv1alpha2.Direction, action crdv1alpha2.TrafficControlAction) error {
-	flows := c.featurePodConnectivity.trafficControlMarkFlows(sourceOFPorts, targetOFPort, direction, action)
+func (c *client) InstallTrafficControlMarkFlows(name string,
+	sourceOFPorts []uint32,
+	targetOFPort uint32,
+	direction crdv1alpha2.Direction,
+	action crdv1alpha2.TrafficControlAction,
+	priority string) error {
+	ofPriority, err := getOFPriority(priority)
+	if err != nil {
+		return err
+	}
+	flows := c.featurePodConnectivity.trafficControlMarkFlows(sourceOFPorts, targetOFPort, direction, action, ofPriority)
 	cacheKey := fmt.Sprintf("tc_%s", name)
 	c.replayMutex.RLock()
 	defer c.replayMutex.RUnlock()
@@ -1660,3 +1680,19 @@ func (c *client) getMeterStats() {
 		klog.ErrorS(err, "Failed to get OVS meter stats")
 	}
 }
+
+func getOFPriority(tcFlowPriority string) (uint16, error) {
+	var ofPriority uint16
+	var err error
+	switch tcFlowPriority {
+	case TrafficControlFlowPriorityHigh:
+		ofPriority = priorityHigh
+	case TrafficControlFlowPriorityMedium:
+		ofPriority = priorityNormal
+	case TrafficControlFlowPriorityLow:
+		ofPriority = priorityLow
+	default:
+		err = fmt.Errorf("unknown traffic control flow priority: %s", tcFlowPriority)
+	}
+	return ofPriority, err
+}
diff --git a/pkg/agent/openflow/client_test.go b/pkg/agent/openflow/client_test.go
@@ -2425,7 +2425,7 @@ func Test_client_InstallTrafficControlMarkFlows(t *testing.T) {
 
 			cacheKey := fmt.Sprintf("tc_%s", tcName)
 
-			assert.NoError(t, fc.InstallTrafficControlMarkFlows(tcName, sourceOFPorts, targetOFPort, tc.direction, tc.action))
+			assert.NoError(t, fc.InstallTrafficControlMarkFlows(tcName, sourceOFPorts, targetOFPort, tc.direction, tc.action, TrafficControlFlowPriorityMedium))
 			fCacheI, ok := fc.featurePodConnectivity.tcCachedFlows.Load(cacheKey)
 			require.True(t, ok)
 			assert.ElementsMatch(t, tc.expectedFlows, getFlowStrings(fCacheI))
@@ -2768,7 +2768,7 @@ func Test_client_ReplayFlows(t *testing.T) {
 	)
 	sourceOFPorts := []uint32{50, 100}
 	targetOFPort := uint32(200)
-	addFlowInCache(fc.featurePodConnectivity.tcCachedFlows, "tcFlows", fc.featurePodConnectivity.trafficControlMarkFlows(sourceOFPorts, targetOFPort, v1alpha2.DirectionEgress, v1alpha2.ActionMirror))
+	addFlowInCache(fc.featurePodConnectivity.tcCachedFlows, "tcFlows", fc.featurePodConnectivity.trafficControlMarkFlows(sourceOFPorts, targetOFPort, v1alpha2.DirectionEgress, v1alpha2.ActionMirror, priorityNormal))
 	replayedFlows = append(replayedFlows,
 		"cookie=0x1010000000000, table=TrafficControl, priority=200,in_port=50 actions=set_field:0xc8->reg9,set_field:0x400000/0xc00000->reg4,goto_table:IngressSecurityClassifier",
 		"cookie=0x1010000000000, table=TrafficControl, priority=200,in_port=100 actions=set_field:0xc8->reg9,set_field:0x400000/0xc00000->reg4,goto_table:IngressSecurityClassifier",

diff --git a/pkg/agent/openflow/network_policy.go b/pkg/agent/openflow/network_policy.go
@@ -2218,7 +2218,7 @@ func (f *featureNetworkPolicy) l7NPTrafficControlFlows() []binding.Flow {
 		// This generates the flow to output the packets marked with L7NPRedirectCTMark to an application-aware engine
 		// via the target ofPort. Note that, before outputting the packets, VLAN ID stored on field L7NPRuleVlanIDCTMarkField
 		// will be copied to VLAN ID register (OXM_OF_VLAN_VID) to set VLAN ID of the packets.
-		OutputTable.ofTable.BuildFlow(priorityHigh+1).
+		OutputTable.ofTable.BuildFlow(priorityHigh+2).
 			Cookie(cookieID).
 			MatchRegMark(OutputToOFPortRegMark).
 			MatchCTMark(L7NPRedirectCTMark).
@@ -2239,7 +2239,7 @@ func (f *featureNetworkPolicy) l7NPTrafficControlFlows() []binding.Flow {
 			Done(),
 		// This generates the flow to forward the returned packets (with FromTCReturnRegMark) to stageOutput directly
 		// after loading output port number to reg1 in L2ForwardingCalcTable.
-		TrafficControlTable.ofTable.BuildFlow(priorityHigh).
+		TrafficControlTable.ofTable.BuildFlow(priorityHigh+1).
 			Cookie(cookieID).
 			MatchRegMark(OutputToOFPortRegMark, FromTCReturnRegMark).
 			Action().GotoStage(stageOutput).

diff --git a/pkg/agent/openflow/network_policy_test.go b/pkg/agent/openflow/network_policy_test.go
@@ -1403,8 +1403,8 @@ func networkPolicyInitFlows(ovsMeterSupported, externalNodeEnabled, l7NetworkPol
 	if l7NetworkPolicyEnabled {
 		initFlows = append(initFlows,
 			"cookie=0x1020000000000, table=Classifier, priority=200,in_port=11,vlan_tci=0x1000/0x1000 actions=pop_vlan,set_field:0x6/0xf->reg0,goto_table:L3Forwarding",
-			"cookie=0x1020000000000, table=TrafficControl, priority=210,reg0=0x200006/0x60000f actions=goto_table:Output",
-			"cookie=0x1020000000000, table=Output, priority=211,ct_mark=0x80/0x80,reg0=0x200000/0x600000 actions=push_vlan:0x8100,move:NXM_NX_CT_LABEL[64..75]->OXM_OF_VLAN_VID[0..11],output:10",
+			"cookie=0x1020000000000, table=TrafficControl, priority=211,reg0=0x200006/0x60000f actions=goto_table:Output",
+			"cookie=0x1020000000000, table=Output, priority=212,ct_mark=0x80/0x80,reg0=0x200000/0x600000 actions=push_vlan:0x8100,move:NXM_NX_CT_LABEL[64..75]->OXM_OF_VLAN_VID[0..11],output:10",
 		)
 	}
 	return initFlows

diff --git a/pkg/agent/openflow/pod_connectivity.go b/pkg/agent/openflow/pod_connectivity.go
@@ -200,7 +200,11 @@ func (f *featurePodConnectivity) replayFlows() []*openflow15.FlowMod {
 }
 
 // trafficControlMarkFlows generates the flows to mark the packets that need to be redirected or mirrored.
-func (f *featurePodConnectivity) trafficControlMarkFlows(sourceOFPorts []uint32, targetOFPort uint32, direction v1alpha2.Direction, action v1alpha2.TrafficControlAction) []binding.Flow {
+func (f *featurePodConnectivity) trafficControlMarkFlows(sourceOFPorts []uint32,
+	targetOFPort uint32,
+	direction v1alpha2.Direction,
+	action v1alpha2.TrafficControlAction,
+	priority uint16) []binding.Flow {
 	cookieID := f.cookieAllocator.Request(f.category).Raw()
 	var actionRegMark *binding.RegMark
 	if action == v1alpha2.ActionRedirect {
@@ -212,7 +216,7 @@ func (f *featurePodConnectivity) trafficControlMarkFlows(sourceOFPorts []uint32,
 	for _, port := range sourceOFPorts {
 		if direction == v1alpha2.DirectionIngress || direction == v1alpha2.DirectionBoth {
 			// This generates the flow to mark the packets destined for a provided port.
-			flows = append(flows, TrafficControlTable.ofTable.BuildFlow(priorityNormal).
+			flows = append(flows, TrafficControlTable.ofTable.BuildFlow(priority).
 				Cookie(cookieID).
 				MatchRegFieldWithValue(TargetOFPortField, port).
 				Action().LoadToRegField(TrafficControlTargetOFPortField, targetOFPort).
@@ -222,7 +226,7 @@ func (f *featurePodConnectivity) trafficControlMarkFlows(sourceOFPorts []uint32,
 		}
 		// This generates the flow to mark the packets sourced from a provided port.
 		if direction == v1alpha2.DirectionEgress || direction == v1alpha2.DirectionBoth {
-			flows = append(flows, TrafficControlTable.ofTable.BuildFlow(priorityNormal).
+			flows = append(flows, TrafficControlTable.ofTable.BuildFlow(priority).
 				Cookie(cookieID).
 				MatchInPort(port).
 				Action().LoadToRegField(TrafficControlTargetOFPortField, targetOFPort).

diff --git a/pkg/agent/openflow/testing/mock_openflow.go b/pkg/agent/openflow/testing/mock_openflow.go
diff --git a/test/integration/agent/openflow_test.go b/test/integration/agent/openflow_test.go
@@ -1874,7 +1874,7 @@ func TestTrafficControlFlows(t *testing.T) {
 	returnOFPort := uint32(201)
 	expectedFlows := prepareTrafficControlFlows(sourceOFPorts, targetOFPort, returnOFPort)
 	c.InstallTrafficControlReturnPortFlow(returnOFPort)
-	c.InstallTrafficControlMarkFlows("tc", sourceOFPorts, targetOFPort, v1alpha2.DirectionBoth, v1alpha2.ActionRedirect)
+	c.InstallTrafficControlMarkFlows("tc", sourceOFPorts, targetOFPort, v1alpha2.DirectionBoth, v1alpha2.ActionRedirect, ofClient.TrafficControlFlowPriorityMedium)
 	for _, tableFlow := range expectedFlows {
 		ofTestUtils.CheckFlowExists(t, ovsCtlClient, tableFlow.tableName, 0, true, tableFlow.flows)
 	}