From 19a24adf04c816bef2bc0e2dec3a05157638a9fd Mon Sep 17 00:00:00 2001 From: wenyingd Date: Mon, 5 Jun 2023 11:46:55 +0800 Subject: [PATCH] Bugfix: TCP src port is unset on the TCP DNS response flow 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) + }) + } +}