Updates for review comments

Signed-off-by: Hongliang Liu <lhongliang@vmware.com>

pkg/agent/openflow/fields.go

@@ -26,6 +26,7 @@ var (
 	fromLocalVal   = uint32(2)
 	fromUplinkVal  = uint32(4)
 	fromBridgeVal  = uint32(5)
+
 	// reg0 (NXM_NX_REG0)
 	// reg0[0..3]: Field to mark the packet source. Marks in this field include,
 	//   - 0: from the tunnel port
@@ -147,14 +148,15 @@ var (
 	//TODO: There is a bug in libOpenflow when CT_MARK range is from 0 to 0, and a wrong mask will be got. As a result,
 	// don't just use bit 0 of CT_MARK.
 
-	// Mark to indicate the connection is initiated through the host gateway interface
-	// (i.e. for which the first packet of the connection was received through the gateway).
-	FromGatewayCTMark = binding.NewCTMark(fromGatewayVal, 0, 3)
-	// Mark to indicate the connection is initiated through the host bridge interface
-	// (i.e. for which the first packet of the connection was received through the bridge).
-	FromBridgeCTMark = binding.NewCTMark(fromBridgeVal, 0, 3)
-	// Mark to indicate DNAT is performed on the connection for Service.
-	ServiceCTMark = binding.NewCTMark(0b1, 4, 4)
+	// CTMark (NXM_NX_CT_MARK)
+	// CTMark[0..3]: Field to mark the source of the connection. This field has the same bits and positions as PktSourceField
+	// for persisting the value from reg0 to CTMark when committing the first packet of the connection with CT action.
+	PktSourceCTMarkField = binding.NewCTMarkField(0, 3, "PacketSourceCTMark")
+	FromGatewayCTMark    = binding.NewCTMark(PktSourceCTMarkField, fromGatewayVal)
+	FromBridgeCTMark     = binding.NewCTMark(PktSourceCTMarkField, fromBridgeVal)
+
+	// CTMark[4]: Mark to indicate DNAT is performed on the connection for Service.
+	ServiceCTMark = binding.NewOneBitCTMark(4, "ServiceCTMark")
 )
 
 // Fields using CT label.

pkg/agent/openflow/pipeline.go

@@ -594,7 +594,7 @@ func (c *client) connectionTrackFlows(category cookie.Category) []binding.Flow {
 					// This flow is used to match the Service traffic from Antrea gateway. The Service traffic from gateway
 					// should enter table serviceConntrackCommitTable, otherwise it will be matched by other flows in
 					// table connectionTrackCommit.
-					ConntrackCommitTable.BuildFlow(priorityHigh).MatchProtocol(proto).
+					ConntrackCommitTable.BuildFlow(priorityNormal).MatchProtocol(proto).
 						MatchCTMark(ServiceCTMark).
 						MatchRegMark(FromGatewayRegMark).
 						Action().GotoTable(ServiceConntrackCommitTable.GetID()).
@@ -685,31 +685,6 @@ func (c *client) connectionTrackFlows(category cookie.Category) []binding.Flow {
 		flows = append(flows, c.kubeProxyFlows(category)...)
 	}
 
-	// TODO: following flows should move to function "kubeProxyFlows". Since another PR(#1198) is trying
-	//  to polish the relevant logic, code refactoring is needed after that PR is merged.
-	for _, proto := range c.ipProtocols {
-		ctZone := CtZone
-		if proto == binding.ProtocolIPv6 {
-			ctZone = CtZoneV6
-		}
-		flows = append(flows,
-			// Connections initiated through the gateway are marked with FromGatewayCTMark.
-			ConntrackCommitTable.BuildFlow(priorityNormal).MatchProtocol(proto).
-				MatchRegMark(FromGatewayRegMark).
-				MatchCTStateNew(true).MatchCTStateTrk(true).
-				Action().CT(true, ConntrackCommitTable.GetNext(), ctZone).LoadToCtMark(FromGatewayCTMark).CTDone().
-				Cookie(c.cookieAllocator.Request(category).Raw()).
-				Done(),
-			// Connections initiated through the bridge port are marked with FromBridgeCTMark.
-			ConntrackCommitTable.BuildFlow(priorityNormal).MatchProtocol(proto).
-				MatchRegMark(FromBridgeRegMark).
-				MatchCTStateNew(true).MatchCTStateTrk(true).
-				Action().CT(true, ConntrackCommitTable.GetNext(), ctZone).LoadToCtMark(FromBridgeCTMark).CTDone().
-				Cookie(c.cookieAllocator.Request(category).Raw()).
-				Done(),
-			// Add reject response packet bypass flow.
-		)
-	}
 	return flows
 }
 
@@ -758,7 +733,9 @@ func (c *client) conntrackBasicFlows(category cookie.Category) []binding.Flow {
 				Done(),
 			ConntrackCommitTable.BuildFlow(priorityLow).MatchProtocol(proto).
 				MatchCTStateNew(true).MatchCTStateTrk(true).
-				Action().CT(true, ConntrackCommitTable.GetNext(), ctZone).CTDone().
+				Action().CT(true, ConntrackCommitTable.GetNext(), ctZone).
+				MoveToCtMark(PktSourceField, PktSourceCTMarkField).
+				CTDone().
 				Cookie(c.cookieAllocator.Request(category).Raw()).
 				Done(),
 		)
@@ -1344,9 +1321,8 @@ func (c *client) l3FwdServiceDefaultFlowsViaGW(ipProto binding.Protocol, categor
 		//    local Pod CIDR or any remote Pod CIDRs.
 		//  - When Egress is enabled:
 		//    - Service request packets sourced from Antrea gateway and destined for external Endpoint.
-		// Note that, when Egress is enabled, Service request packets sourced from local Pods and destined for external
-		// Endpoint should not be matched by the flow, and such packets should be matched by the flow with higher priority
-		// in L3ForwardingTable, which is installed by Egress.
+		// TODO: The priority of this flow is 190, and the priority of the flow to match Egress traffic from local Pods is
+		//  also 190. Egress traffic from local Pods is at risk of being matched by this flow.
 		// Skip traffic from AntreaFlexibleIPAM Pods.
 		L3ForwardingTable.BuildFlow(priorityLow).MatchProtocol(ipProto).
 			MatchCTMark(ServiceCTMark).
@@ -2433,7 +2409,7 @@ func (c *client) endpointDNATFlow(endpointIP net.IP, endpointPort uint16, protoc
 			&binding.PortRange{StartPort: endpointPort, EndPort: endpointPort},
 		).
 		LoadToCtMark(ServiceCTMark).
-		MoveToCtMark(PktSourceField.GetNXFieldName(), PktSourceField.GetRange(), &binding.Range{0, 3}).
+		MoveToCtMark(PktSourceField, PktSourceCTMarkField).
 		CTDone().
 		Done()
 }

pkg/ovs/openflow/interfaces.go

@@ -342,7 +342,7 @@ type CTAction interface {
 	LoadToCtMark(mark *CtMark) CTAction
 	LoadToLabelField(value uint64, labelField *CtLabel) CTAction
 	MoveToLabel(fromName string, fromRng, labelRng *Range) CTAction
-	MoveToCtMark(fromName string, fromRng, ctMarkRng *Range) CTAction
+	MoveToCtMark(fromRegField *RegField, ctMark *CtMarkField) CTAction
 	// NAT action translates the packet in the way that the connection was committed into the conntrack zone, e.g., if
 	// a connection was committed with SNAT, the later packets would be translated with the earlier SNAT configurations.
 	NAT() CTAction
@@ -443,9 +443,16 @@ type RegMark struct {
 // XXRegField specifies a xxreg with a required bit range.
 type XXRegField RegField
 
+// CtMarkField specifies a bit range of a CT mark. rng is the range of bits taken by the field. The OF client could use a
+// CtMarkField to cache or match varied value.
+type CtMarkField struct {
+	rng  *Range
+	name string
+}
+
 // CtMark is used to indicate the connection characteristics.
 type CtMark struct {
-	rng   *Range
+	field *CtMarkField
 	value uint32
 }
 

pkg/ovs/openflow/ofctrl_action.go

@@ -94,7 +94,7 @@ func (a *ofCTAction) LoadToMark(value uint32) CTAction {
 
 func (a *ofCTAction) LoadToCtMark(mark *CtMark) CTAction {
 	field, _, _ := getFieldRange(NxmFieldCtMark)
-	a.load(field, uint64(mark.value), mark.rng)
+	a.load(field, uint64(mark.value), mark.field.rng)
 	return a
 }
 
@@ -118,10 +118,10 @@ func (a *ofCTAction) MoveToLabel(fromName string, fromRng, labelRng *Range) CTAc
 }
 
 // MoveToCtMark is an action to move data into ct_mark.
-func (a *ofCTAction) MoveToCtMark(fromName string, fromRng, ctMarkRng *Range) CTAction {
-	fromField, _ := openflow13.FindFieldHeaderByName(fromName, false)
+func (a *ofCTAction) MoveToCtMark(fromRegField *RegField, ctMarkField *CtMarkField) CTAction {
+	fromField, _ := openflow13.FindFieldHeaderByName(fromRegField.GetNXFieldName(), false)
 	toField, _ := openflow13.FindFieldHeaderByName(NxmFieldCtMark, false)
-	a.move(fromField, toField, uint16(fromRng.Length()), uint16(fromRng[0]), uint16(ctMarkRng[0]))
+	a.move(fromField, toField, uint16(fromRegField.GetRange().Length()), uint16(fromRegField.GetRange()[0]), uint16(ctMarkField.rng[0]))
 	return a
 }
 

pkg/ovs/openflow/ofctrl_builder.go

@@ -237,7 +237,7 @@ func (b *ofFlowBuilder) MatchCTMark(mark *CtMark) FlowBuilder {
 		b.ofFlow.Match.CtMarkMask = nil
 		ctmarkKey = fmt.Sprintf("ct_mark=0x%x", mark.value)
 	} else {
-		mask := mark.rng.ToNXRange().ToUint32Mask()
+		mask := mark.field.rng.ToNXRange().ToUint32Mask()
 		ctmarkKey = fmt.Sprintf("ct_mark=0x%x/0x%x", mark.GetValue(), mask)
 		b.ofFlow.Match.CtMarkMask = &mask
 	}

pkg/ovs/openflow/ofctrl_flow_test.go

@@ -31,7 +31,8 @@ func TestCopyToBuilder(t *testing.T) {
 		id:   t0,
 		next: t1,
 	}
-	mark := NewCTMark(12345, 0, 31)
+
+	mark := NewCTMark(NewCTMarkField(0, 31, "mark"), 12345)
 	oriFlow := table.BuildFlow(uint16(100)).MatchProtocol(ProtocolIP).
 		Cookie(uint64(1004)).
 		MatchRegFieldWithValue(testField, 0x101).

pkg/ovs/openflow/ofctrl_nxfields.go

@@ -83,21 +83,35 @@ func NewXXRegField(id int, start, end uint32) *XXRegField {
 }
 
 func (m *CtMark) GetRange() *Range {
-	return m.rng
+	return m.field.rng
 }
 
 // GetValue gets CT mark value with offset since CT mark is used by bit. E.g, CT_MARK_REG[3]==1, the return
 // value of this function is 0b1000.
 func (m *CtMark) GetValue() uint32 {
-	return m.value << m.rng.Offset()
+	return m.value << m.field.rng.Offset()
 }
 
 func (m *CtMark) isFullRange() bool {
-	return m.rng.Length() == 32
+	return m.field.rng.Length() == 32
 }
 
-func NewCTMark(value uint32, start, end uint32) *CtMark {
-	return &CtMark{value: value, rng: &Range{start, end}}
+func NewCTMarkField(start, end uint32, name string) *CtMarkField {
+	return &CtMarkField{rng: &Range{start, end}, name: name}
+}
+
+func NewOneBitCTMark(bit uint32, name string) *CtMark {
+	field := NewCTMarkField(bit, bit, name)
+	return &CtMark{value: 1, field: field}
+}
+
+func NewOneBitZeroCTMark(bit uint32, name string) *CtMark {
+	field := NewCTMarkField(bit, bit, name)
+	return &CtMark{value: 0, field: field}
+}
+
+func NewCTMark(field *CtMarkField, value uint32) *CtMark {
+	return &CtMark{value: value, field: field}
 }
 
 func NewCTLabel(start, end uint32, name string) *CtLabel {

test/integration/agent/openflow_test.go

@@ -1346,8 +1346,7 @@ func prepareDefaultFlows(config *testConfig) []expectTableFlows {
 			&ofTestUtils.ExpectFlow{MatchStr: "priority=190,ct_state=+inv+trk,ip", ActStr: "drop"},
 		)
 		table105Flows.flows = append(table105Flows.flows,
-			&ofTestUtils.ExpectFlow{MatchStr: "priority=200,ct_state=+new+trk,ip,reg0=0x1/0xf", ActStr: "ct(commit,table=HairpinSNAT,zone=65520,exec(load:0x1->NXM_NX_CT_MARK[0..3])"},
-			&ofTestUtils.ExpectFlow{MatchStr: "priority=190,ct_state=+new+trk,ip", ActStr: "ct(commit,table=HairpinSNAT,zone=65520)"},
+			&ofTestUtils.ExpectFlow{MatchStr: "priority=190,ct_state=+new+trk,ip", ActStr: "ct(commit,table=HairpinSNAT,zone=65520,exec(move:NXM_NX_REG0[0..3]->NXM_NX_CT_MARK[0..3]))"},
 		)
 		table72Flows.flows = append(table72Flows.flows,
 			&ofTestUtils.ExpectFlow{MatchStr: "priority=210,ip,reg0=0x1/0xf", ActStr: "goto_table:L2Forwarding"},
@@ -1362,8 +1361,7 @@ func prepareDefaultFlows(config *testConfig) []expectTableFlows {
 			&ofTestUtils.ExpectFlow{MatchStr: "priority=190,ct_state=+inv+trk,ipv6", ActStr: "drop"},
 		)
 		table105Flows.flows = append(table105Flows.flows,
-			&ofTestUtils.ExpectFlow{MatchStr: "priority=200,ct_state=+new+trk,ipv6,reg0=0x1/0xf", ActStr: "ct(commit,table=HairpinSNAT,zone=65510,exec(load:0x1->NXM_NX_CT_MARK[0..3])"},
-			&ofTestUtils.ExpectFlow{MatchStr: "priority=190,ct_state=+new+trk,ipv6", ActStr: "ct(commit,table=HairpinSNAT,zone=65510)"},
+			&ofTestUtils.ExpectFlow{MatchStr: "priority=190,ct_state=+new+trk,ipv6", ActStr: "ct(commit,table=HairpinSNAT,zone=65510,exec(move:NXM_NX_REG0[0..3]->NXM_NX_CT_MARK[0..3]))"},
 		)
 		table72Flows.flows = append(table72Flows.flows,
 			&ofTestUtils.ExpectFlow{MatchStr: "priority=210,ipv6,reg0=0x1/0xf", ActStr: "goto_table:L2Forwarding"},

test/integration/ovs/ofctrl_test.go

@@ -49,7 +49,7 @@ var (
 	fromGatewayMark = binding.NewRegMark(sourceField, 1)
 
 	marksReg      = 0
-	gatewayCTMark = binding.NewCTMark(0x1, 1, 1)
+	gatewayCTMark = binding.NewOneBitCTMark(1, "gatewayCTMark")
 	ctZone        = 0xfff0
 
 	count uint64
@@ -1055,7 +1055,7 @@ func prepareNATflows(table binding.Table) ([]binding.Flow, []*ExpectFlow) {
 	natedIP2 := net.ParseIP("10.10.0.10")
 	natIPRange1 := &binding.IPRange{StartIP: natedIP1, EndIP: natedIP1}
 	natIPRange2 := &binding.IPRange{StartIP: natedIP1, EndIP: natedIP2}
-	snatCTMark := binding.NewCTMark(0x40, 0, 7)
+	snatCTMark := binding.NewCTMark(binding.NewCTMarkField(0, 7, "snatCTMark"), 0x40)
 	snatMark1 := binding.NewOneBitRegMark(marksReg, 17, "SNATMark1")
 	snatMark2 := binding.NewOneBitRegMark(marksReg, 18, "SNATMark2")
 	dnatMark1 := binding.NewOneBitRegMark(marksReg, 19, "DNATMark1")