Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 1 commit into from
Jun 9, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 32 additions & 22 deletions pkg/agent/openflow/network_policy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, from my understanding, that part could be improved. Let's see @Dyanngg 's opinion.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

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)
}
Expand Down
68 changes: 68 additions & 0 deletions pkg/agent/openflow/network_policy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1381,6 +1381,7 @@ func networkPolicyInitFlows(externalNodeEnabled, l7NetworkPolicyEnabled bool) []
}
return initFlows
}

func Test_featureNetworkPolicy_initFlows(t *testing.T) {
testCases := []struct {
name string
Expand Down Expand Up @@ -1416,3 +1417,70 @@ func Test_featureNetworkPolicy_initFlows(t *testing.T) {
})
}
}

func Test_NewDNSPacketInConjunction(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great test. I assume it will fail without the patch, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is failed before the change.

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)
})
}
}