From fac1140927b05c1dd71eb2ce88a80c7d36be5df0 Mon Sep 17 00:00:00 2001 From: Wenying Dong Date: Mon, 12 Jun 2023 06:41:39 -0700 Subject: [PATCH] Bugfix: TCP src port is unset on the TCP DNS response flow (#5106) 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 --- pkg/agent/openflow/network_policy.go | 54 ++++++++++-------- pkg/agent/openflow/network_policy_test.go | 68 +++++++++++++++++++++++ 2 files changed, 100 insertions(+), 22 deletions(-) diff --git a/pkg/agent/openflow/network_policy.go b/pkg/agent/openflow/network_policy.go index f8d43974d97..23e3723cb5a 100644 --- 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 index 08a79354fa3..4a95f43f587 100644 --- 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) + }) + } +}