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 13, 2023 · 45c091a · 45c091a
1 parent 66973b9
commit 45c091a
Show file tree

Hide file tree

Showing 10 changed files with 106 additions and 46 deletions.
diff --git a/pkg/agent/controller/trafficcontrol/controller.go b/pkg/agent/controller/trafficcontrol/controller.go
@@ -920,7 +920,12 @@ 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,
+			types.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,19 @@ import (
 
 const maxRetryForOFSwitch = 5
 
+func tcPriorityToOFPriority(p types.TrafficControlFlowPriority) uint16 {
+	switch p {
+	case types.TrafficControlFlowPriorityHigh:
+		return priorityHigh
+	case types.TrafficControlFlowPriorityMedium:
+		return priorityNormal
+	case types.TrafficControlFlowPriorityLow:
+		return priorityLow
+	default:
+		return 0
+	}
+}
+
 // 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 +336,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 types.TrafficControlFlowPriority) error
 
 	// UninstallTrafficControlMarkFlows removes the flows for a traffic control rule.
 	UninstallTrafficControlMarkFlows(name string) error
@@ -1449,8 +1467,13 @@ 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 types.TrafficControlFlowPriority) error {
+	flows := c.featurePodConnectivity.trafficControlMarkFlows(sourceOFPorts, targetOFPort, direction, action, tcPriorityToOFPriority(priority))
 	cacheKey := fmt.Sprintf("tc_%s", name)
 	c.replayMutex.RLock()
 	defer c.replayMutex.RUnlock()

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, types.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).

diff --git a/pkg/agent/openflow/network_policy_test.go b/pkg/agent/openflow/network_policy_test.go
@@ -1404,7 +1404,7 @@ func networkPolicyInitFlows(ovsMeterSupported, externalNodeEnabled, l7NetworkPol
 		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=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/pkg/agent/types/trafficcontrol.go b/pkg/agent/types/trafficcontrol.go
@@ -0,0 +1,28 @@
+// Copyright 2023 Antrea Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package types
+
+// TrafficControlFlowPriority sets the priority for flows installed by OpenFlow client using InstallTrafficControlMarkFlows
+// method.
+type TrafficControlFlowPriority string
+
+const (
+	// TrafficControlFlowPriorityHigh is not used yet.
+	TrafficControlFlowPriorityHigh TrafficControlFlowPriority = "high"
+	// TrafficControlFlowPriorityMedium is for user-defined TrafficControl CRs.
+	TrafficControlFlowPriorityMedium TrafficControlFlowPriority = "medium"
+	// TrafficControlFlowPriorityLow is not used yet.
+	TrafficControlFlowPriorityLow TrafficControlFlowPriority = "low"
+)
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, types.TrafficControlFlowPriorityMedium)
 	for _, tableFlow := range expectedFlows {
 		ofTestUtils.CheckFlowExists(t, ovsCtlClient, tableFlow.tableName, 0, true, tableFlow.flows)
 	}