Bugfix: TCP src port is unset on the TCP DNS response flow #5078
Conversation
| tcpServiceMatch := &conjunctiveMatch{ | ||
| tableID: conj.serviceClause.ruleTable.GetID(), | ||
| matchPairs: []matchPair{ |
There was a problem hiding this comment.
Thanks for finding the root cause of this issue.
I think you can simply fix it by changing matchPairs here to matchPairs: append(dnsTCPMatchPair, tcpFlagsMatchPair),
Before adding source port support, dnsTCPMatchPair only has one item inside. Now, for a service with a source port match, getServiceMatchPairs will generate two pairs. One is for the destination port, matching port 0(match all) and the other is for the source port, matching the port we provided.
cc @Dyanngg since he is the owner of source port support.
There was a problem hiding this comment.
The truth is we only care about TCP source port and TCP flags, why it is a must to add a match for dst port value 0 although it does not work on OVS?
There was a problem hiding this comment.
Yes, from my understanding, that part could be improved. Let's see @Dyanngg 's opinion.
There was a problem hiding this comment.
Yes we could refine the logic for matchPairs. We do not have to add the match for dst port if src port matching is the only "filter" we have on the traffic. The code is written in the current way simply because dst port matching is a much common case and I wanted to simplify the implementation (instead of explicitly covering cases for src port matching only, dst port matching only and src dst port matching).
luolanzone
added
action/backport
pkg/agent/openflow/network_policy.go
Outdated
| } else { | ||
| klog.InfoS("Invalid protocol for TCP DNS", "protocol", proto) | ||
| continue | ||
| } |
There was a problem hiding this comment.
We have assumed there will only be IP and IPv6 in other places anyway, no need to add an unreachable branch
antrea/pkg/agent/openflow/pipeline.go
Lines 1747 to 1751 in c003098
There was a problem hiding this comment.
sure, removed "else" branch.
| @@ -1416,3 +1417,70 @@ func Test_featureNetworkPolicy_initFlows(t *testing.T) { | |||
| }) | |||
| } | |||
| } | |||
|
|
|||
| func Test_NewDNSPacketInConjunction(t *testing.T) { | |||
There was a problem hiding this comment.
Great test. I assume it will fail without the patch, right?
There was a problem hiding this comment.
Yes, it is failed before the change.
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 <[email protected]>
@wenyingd the desription mentioned it resolves bug in function |
No, I removed that part out from this change, but forgot to change the commit message. |
|
/test-ipv6-e2e |
|
@Dyanngg @GraysonWu please let us know if you have other comments. |
|
/skip-conformance |
|
/test-vm-e2e |
|
@wenyingd please backport it to release-1.12 |
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
getServiceMatchPairsto make the logic simply.