Bugfix: TCP src port is unset on the TCP DNS response flow (#5078)

This change is to resolve an issue in ANP with FQDN rules which has sent all TCP packets marked with ack and psh flags to antrea-agent rather than only sent the DNS response packets. The root cause is the existing code would add a match pair with tp_dst=0 into the service match pairs even if no dst port is set in the ANP prtocols. Then the DNS logic has picked a wrong service match pair to generate the OpenFlow entries. This change directly generates the conjunctive match conditions for DNS response packets rather than calling `getServiceMatchPairs` to make the logic simply. Signed-off-by: wenyingd <wenyingd@vmware.com>
antrea-io · Jun 9, 2023 · 7f02a63 · 7f02a63
1 parent 55d1ddb
commit 7f02a63
Show file tree

Hide file tree

Showing 2 changed files with 100 additions and 22 deletions.
diff --git a/pkg/agent/openflow/network_policy.go b/pkg/agent/openflow/network_policy.go
@@ -710,37 +710,47 @@ func (c *client) NewDNSPacketInConjunction(id uint32) error {
 		Protocol: &protocolUDP,
 		SrcPort:  &dnsPort,
 	}
-	tcpService := v1beta2.Service{
-		Protocol: &protocolTCP,
-		SrcPort:  &dnsPort,
-	}
 	dnsPriority := priorityDNSIntercept
 	conj.serviceClause = conj.newClause(1, 2, getTableByID(conj.ruleTableID), nil)
 	conj.toClause = conj.newClause(2, 2, getTableByID(conj.ruleTableID), nil)
 	c.featureNetworkPolicy.conjMatchFlowLock.Lock()
 	defer c.featureNetworkPolicy.conjMatchFlowLock.Unlock()
 	ctxChanges := conj.serviceClause.addServiceFlows(c.featureNetworkPolicy, []v1beta2.Service{udpService}, &dnsPriority, false)
-	dnsTCPMatchPairs := getServiceMatchPairs(tcpService, c.featureNetworkPolicy.ipProtocols)
-	for _, dnsTCPMatchPair := range dnsTCPMatchPairs {
-		tcpFlagsMatchPair := matchPair{
-			matchKey: MatchTCPFlags,
-			matchValue: TCPFlags{
-				// URG|ACK|PSH|RST|SYN|FIN|
-				Flag: 0b011000,
-				Mask: 0b011000,
-			},
-		}
-		if dnsTCPMatchPair[0].matchKey.GetOFProtocol() == binding.ProtocolTCPv6 {
-			tcpFlagsMatchPair.matchKey = MatchTCPv6Flags
-		}
+
+	tcpFlags := TCPFlags{
+		// URG|ACK|PSH|RST|SYN|FIN|
+		Flag: 0b011000,
+		Mask: 0b011000,
+	}
+	tcpDNSPort := types.BitRange{Value: uint16(dnsPort)}
+	for _, proto := range c.featureNetworkPolicy.ipProtocols {
 		tcpServiceMatch := &conjunctiveMatch{
-			tableID: conj.serviceClause.ruleTable.GetID(),
-			matchPairs: []matchPair{
-				dnsTCPMatchPair[0],
-				tcpFlagsMatchPair,
-			},
+			tableID:  conj.serviceClause.ruleTable.GetID(),
 			priority: &dnsPriority,
 		}
+		if proto == binding.ProtocolIP {
+			tcpServiceMatch.matchPairs = []matchPair{
+				{
+					matchKey:   MatchTCPSrcPort,
+					matchValue: tcpDNSPort,
+				},
+				{
+					matchKey:   MatchTCPFlags,
+					matchValue: tcpFlags,
+				},
+			}
+		} else if proto == binding.ProtocolIPv6 {
+			tcpServiceMatch.matchPairs = []matchPair{
+				{
+					matchKey:   MatchTCPv6SrcPort,
+					matchValue: tcpDNSPort,
+				},
+				{
+					matchKey:   MatchTCPv6Flags,
+					matchValue: tcpFlags,
+				},
+			}
+		}
 		ctxChange := conj.serviceClause.addConjunctiveMatchFlow(c.featureNetworkPolicy, tcpServiceMatch, false, false)
 		ctxChanges = append(ctxChanges, ctxChange)
 	}

diff --git a/pkg/agent/openflow/network_policy_test.go b/pkg/agent/openflow/network_policy_test.go
@@ -1381,6 +1381,7 @@ func networkPolicyInitFlows(externalNodeEnabled, l7NetworkPolicyEnabled bool) []
 	}
 	return initFlows
 }
+
 func Test_featureNetworkPolicy_initFlows(t *testing.T) {
 	testCases := []struct {
 		name          string
@@ -1416,3 +1417,70 @@ func Test_featureNetworkPolicy_initFlows(t *testing.T) {
 		})
 	}
 }
+
+func Test_NewDNSPacketInConjunction(t *testing.T) {
+	for _, tc := range []struct {
+		name          string
+		enableIPv4    bool
+		enableIPv6    bool
+		conjID        uint32
+		expectedFlows []string
+	}{
+		{
+			name:       "IPv4 only",
+			enableIPv4: true,
+			enableIPv6: false,
+			conjID:     1,
+			expectedFlows: []string{
+				"cookie=0x1020000000000, table=AntreaPolicyIngressRule, priority=64991,conj_id=1 actions=controller(id=32776,reason=no_match,userdata=02,max_len=128),goto_table:IngressMetric",
+				"cookie=0x1020000000000, table=AntreaPolicyIngressRule, priority=64991,udp,tp_src=53 actions=conjunction(1,1/2)",
+				"cookie=0x1020000000000, table=AntreaPolicyIngressRule, priority=64991,tcp,tp_src=53,tcp_flags=+psh+ack actions=conjunction(1,1/2)",
+			},
+		},
+		{
+			name:       "IPv6 only",
+			enableIPv4: false,
+			enableIPv6: true,
+			conjID:     1,
+			expectedFlows: []string{
+				"cookie=0x1020000000000, table=AntreaPolicyIngressRule, priority=64991,conj_id=1 actions=controller(id=32776,reason=no_match,userdata=02,max_len=128),goto_table:IngressMetric",
+				"cookie=0x1020000000000, table=AntreaPolicyIngressRule, priority=64991,udp6,tp_src=53 actions=conjunction(1,1/2)",
+				"cookie=0x1020000000000, table=AntreaPolicyIngressRule, priority=64991,tcp6,tp_src=53,tcp_flags=+psh+ack actions=conjunction(1,1/2)",
+			},
+		},
+		{
+			name:       "dual stack",
+			enableIPv4: true,
+			enableIPv6: true,
+			conjID:     1,
+			expectedFlows: []string{
+				"cookie=0x1020000000000, table=AntreaPolicyIngressRule, priority=64991,conj_id=1 actions=controller(id=32776,reason=no_match,userdata=02,max_len=128),goto_table:IngressMetric",
+				"cookie=0x1020000000000, table=AntreaPolicyIngressRule, priority=64991,udp,tp_src=53 actions=conjunction(1,1/2)",
+				"cookie=0x1020000000000, table=AntreaPolicyIngressRule, priority=64991,tcp,tp_src=53,tcp_flags=+psh+ack actions=conjunction(1,1/2)",
+				"cookie=0x1020000000000, table=AntreaPolicyIngressRule, priority=64991,udp6,tp_src=53 actions=conjunction(1,1/2)",
+				"cookie=0x1020000000000, table=AntreaPolicyIngressRule, priority=64991,tcp6,tp_src=53,tcp_flags=+psh+ack actions=conjunction(1,1/2)",
+			},
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			ctrl := gomock.NewController(t)
+			m := oftest.NewMockOFEntryOperations(ctrl)
+			bridge := mocks.NewMockBridge(ctrl)
+			fc := newFakeClient(m, tc.enableIPv4, tc.enableIPv6, config.K8sNode, config.TrafficEncapModeEncap)
+			defer resetPipelines()
+			fc.featureNetworkPolicy.bridge = bridge
+			actualFlows := make([]string, 0)
+			m.EXPECT().AddAll(gomock.Any()).Do(func(flowMessages []*openflow15.FlowMod) {
+				flowStrings := getFlowStrings(flowMessages)
+				actualFlows = append(actualFlows, flowStrings...)
+			}).Return(nil).AnyTimes()
+			bridge.EXPECT().AddFlowsInBundle(gomock.Any(), gomock.Any(), gomock.Any()).Do(func(addflows, modFlows, delFlows []*openflow15.FlowMod) {
+				flowStrings := getFlowStrings(addflows)
+				actualFlows = append(actualFlows, flowStrings...)
+			}).Return(nil).Times(1)
+			err := fc.NewDNSPacketInConjunction(tc.conjID)
+			assert.NoError(t, err)
+			assert.ElementsMatch(t, tc.expectedFlows, actualFlows)
+		})
+	}
+}