Change L2 forwarding flows for traffic to gateway and tunnel #1594

jianjuns · 2020-11-24T05:56:26Z

Let the packets to the tunnel port go through L2ForwardingCalcTable
as well, to be consistent with other types of traffic.
Redirect packets to gateway and tunnel, as well as BUM packets to
conntrackCommitTable and bypass ingress NetworkPolicy enforcement.

antrea-bot · 2020-11-24T05:56:42Z

Thanks for your PR.
Unit tests and code linters are run automatically every time the PR is updated.
E2e, conformance and network policy tests can only be triggered by a member of the vmware-tanzu organization. Regular contributors to the project should join the org.

The following commands are available:

/test-e2e: to trigger e2e tests.
/skip-e2e: to skip e2e tests.
/test-conformance: to trigger conformance tests.
/skip-conformance: to skip conformance tests.
/test-all-features-conformance: to trigger conformance tests with all alpha features enabled.
/skip-all-features-conformance: to skip conformance tests with all alpha features enabled.
/test-whole-conformance: to trigger all conformance tests on linux.
/skip-whole-conformance: to skip all conformance tests on linux.
/test-networkpolicy: to trigger networkpolicy tests.
/skip-networkpolicy: to skip networkpolicy tests.
/test-windows-conformance: to trigger windows conformance tests.
/skip-windows-conformance: to skip windows conformance tests.
/test-windows-networkpolicy: to trigger windows networkpolicy tests.
/skip-windows-networkpolicy: to skip windows networkpolicy tests.
/test-hw-offload: to trigger ovs hardware offload test.
/skip-hw-offload: to skip ovs hardware offload test.
/test-all: to trigger all tests (except whole conformance).
/skip-all: to skip all tests (except whole conformance).

codecov-io · 2020-11-24T06:00:24Z

Codecov Report

Merging #1594 (22ffe91) into master (9d3d10b) will decrease coverage by 2.78%.
The diff coverage is 37.72%.

@@            Coverage Diff             @@
##           master    #1594      +/-   ##
==========================================
- Coverage   63.31%   60.53%   -2.79%     
==========================================
  Files         170      181      +11     
  Lines       14250    15562    +1312     
==========================================
+ Hits         9023     9421     +398     
- Misses       4292     5155     +863     
- Partials      935      986      +51

Flag	Coverage Δ
e2e-tests	`47.88% <35.63%> (?)`
kind-e2e-tests	`45.78% <30.93%> (-9.61%)`	⬇️
unit-tests	`40.46% <14.85%> (-0.81%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
cmd/antrea-agent/agent.go	`0.00% <0.00%> (ø)`
cmd/antrea-agent/options.go	`21.69% <0.00%> (+0.97%)`	⬆️
...gent/controller/noderoute/node_route_controller.go	`61.04% <ø> (+14.58%)`	⬆️
pkg/agent/stats/collector.go	`97.72% <ø> (ø)`
pkg/apiserver/apiserver.go	`73.95% <0.00%> (-12.07%)`	⬇️
pkg/apiserver/certificate/cacert_controller.go	`38.73% <0.00%> (-17.65%)`	⬇️
pkg/apiserver/handlers/webhook/mutation_crd.go	`0.00% <0.00%> (ø)`
pkg/controller/networkpolicy/mutate.go	`0.00% <0.00%> (ø)`
pkg/controller/networkpolicy/tier.go	`0.00% <ø> (-90.00%)`	⬇️
pkg/controller/networkpolicy/validate.go	`0.00% <0.00%> (-44.83%)`	⬇️
... and 99 more

jianjuns · 2020-11-24T06:01:00Z

@wenyingd and @tnqn : let me know what you think about the change. Want to make flows more consistent for all types of traffic.

tnqn

Put L2ForwardCalculation logic in one table looks good to me. Just a few comments about the pipeline and the global variables.

tnqn · 2020-11-24T15:50:33Z

pkg/agent/openflow/pipeline.go

-			l3DecTTLTable:         bridge.CreateTable(l3DecTTLTable, conntrackCommitTable, binding.TableMissActionNext),
-			l2ForwardingCalcTable: bridge.CreateTable(l2ForwardingCalcTable, IngressEntryTable, binding.TableMissActionNext),
+			l3DecTTLTable:         bridge.CreateTable(l3DecTTLTable, l2ForwardingCalcTable, binding.TableMissActionNext),
+			l2ForwardingCalcTable: bridge.CreateTable(l2ForwardingCalcTable, conntrackCommitTable, binding.TableMissActionNext),


why the next table of l2ForwardingCalcTable is changed to conntrackCommitTable? I feel from the pipeline's perspective its next table is still ingressEntryTable, just some particular traffic can skip it.

Right. But I hope to change the default flow (which is for BUM traffic) to goto conntrackCommitTable, and be dropped there.
What you think?

Sounds good to me, but would prefer we don't re-declare that the next table is conntrackCommitTable in l2ForwardCalcFlow.

tnqn · 2020-11-24T15:59:58Z

pkg/agent/openflow/pipeline.go

@@ -87,6 +87,8 @@ const (
 )

 var (
+	ingressEntryTable, egressEntryTable binding.TableIDType


if we still make ingressEntryTable as the next table of l2ForwardCalcTable, the global variable won't be needed.

jianjuns · 2020-11-24T22:34:44Z

pkg/agent/openflow/pipeline.go

@@ -785,8 +794,7 @@ func (c *client) l3FwdFlowToPod(localGatewayMAC net.HardwareAddr, podInterfaceIP
 		flows = append(flows, flowBuilder.MatchDstIP(ip).
 			Action().SetSrcMAC(localGatewayMAC).
 			Action().SetDstMAC(podInterfaceMAC).
-			Action().DecTTL().
-			Action().GotoTable(l3FwdTable.GetNext()).
+			Action().GotoTable(l3DecTTLTable).


@wenyingd : I made this change, because later with SNAT policy, we can have packets from gw0 which will hit this flow as well after un-SNAT'd (such packets' dst MAC will be rewritten to vMAC in conntrackStateTable), but we want not to decrement TTL for them.
Also we can keep all routed packets (handled by l3Fwd funcs) consistent.

Let me know what you think.

I note that you have changed flows in L3DecTTLTable: 1) not dec TTL if the packet is from gw0; 2) dec TTL for other cases.And I guess you want to use L3DecTTLTable in both source and destination Node (originally I only want to use it on source Node). I am not sure if a packet is sending from local Pod to gateway port would also go into L3DecTTLTable (e.g. in NoEncap mode), if yes, then the logics in L3DecTTLTable might be not as expected.

Yes, I hope to use L3DecTTLTable on both source and destination port, for the reasons I mentioned above.

if a packet is sending from local Pod to gateway port would also go into L3DecTTLTable (e.g. in NoEncap mode)
I think in that case the host stack should decrement the TTL? So, in l3FwdFlowRouteToGW() I skip L3DecTTLTable().

jianjuns · 2020-11-24T22:36:33Z

pkg/agent/openflow/pipeline.go

 func (c *client) l3FwdFlowRouteToGW(gwMAC net.HardwareAddr, category cookie.Category) []binding.Flow {
 	l3FwdTable := c.pipeline[l3ForwardingTable]
 	var flows []binding.Flow
 	for _, ipProto := range c.ipProtocols {
 		flows = append(flows, l3FwdTable.BuildFlow(priorityLow).MatchProtocol(ipProto).
 			Action().SetDstMAC(gwMAC).
-			Action().DecTTL().


@wenyingd : do you think this case should bypass DecTTL()?

Yes, I think the packet should be forwarded to the gateway interface, and the host should dec TTL, so we don't need TTL decrement in OpenFlow. @tnqn what do you think?

Yes, I think so.

jianjuns · 2020-11-24T22:37:57Z

pkg/agent/openflow/pipeline.go

-			Done())
+		flows = append(flows,
+			// Skip packets from the gateway interface.
+			decTTLTable.BuildFlow(priorityHigh).MatchPriority(priorityNormal).


@wenyingd : I assume kube-proxy packets (even they come from tunnel/remote Node) should bypass DecTTL(). What you think?

jianjuns · 2020-11-24T22:44:35Z

pkg/agent/openflow/pipeline.go

@@ -1096,7 +1094,6 @@ func (c *client) allowRulesMetricFlows(conjunctionID uint32, ingress bool) []bin
 	metricFlow := func(isCTNew bool, protocol binding.Protocol) binding.Flow {
 		return c.pipeline[metricTableID].BuildFlow(priorityNormal).
 			MatchProtocol(protocol).
-			MatchPriority(priorityNormal).


@wenyingd : I assume MatchPriority() is not needed?

I think we could remove the call for MatchPriority() in the functions in this file (pipeline.go), but please note that we should keep it in file (network_policy.go), for CNP might call this API to reset flow priority. An original discussion is #803 (comment)

wenyingd

Maybe you also need to change the svg file if changing the pipeline.

wenyingd · 2020-11-25T02:34:15Z

pkg/agent/openflow/client.go

-	if err := c.ofEntryOperations.Add(flow); err != nil {
+	flows := []binding.Flow{
+		c.tunnelClassifierFlow(config.DefaultTunOFPort, cookie.Default),
+		c.l2ForwardCalcFlow(globalVirtualMAC, config.DefaultTunOFPort, true, cookie.Default),


globalVirtualMAC is also used in some other cases like Windows SNAT as the mark to replace the dst MAC. Do you think we could unify the usage, e.g., using this MAC for tunnel traffic, and using macRewriteMark to rewrite dstMAC in any Node?

That (using macRewriteMark to indicate MAC rewrite) makes sense to me. But I am not familiar with all other cases. Would you make the change?

OK, I would make that change.

jianjuns · 2020-11-25T03:38:16Z

Sure. I will update ovs-pipeline.md after you agree on the design.

jianjuns · 2020-11-25T04:10:46Z

If you are fine, I hope to change ovs-pipeline.md after this change - need @antoninbas to review the SVG change who will come back next week. @wenyingd

wenyingd · 2020-11-25T05:42:46Z

If you are fine, I hope to change ovs-pipeline.md after this change - need @antoninbas to review the SVG change who will come back next week. @wenyingd

I am fine with it.

wenyingd · 2020-11-26T03:57:03Z

pkg/agent/openflow/pipeline.go

 	l2FwdCalcTable := c.pipeline[l2ForwardingCalcTable]
+	nextTable := ingressEntryTable
+	if skipIngressRules {


I am thinking is it possible to leverage the "ofport" value to decide next table? e.g., if the value is tunnel port or gateway port (I assume we have no ingress rules applied on the gw0), then skipIngressRules, otherwise go to ingressEntryTable.
Then we could not use the parameter "skipIngressRule"

Yes, that is possible too, but do you think better to put the logic in l2FwdCalcTable and pipeline.go or the callers?

Personnally, I would prefer to put it inside the l2FwdCalcTable's logic, that would make the call simple, and easier to understand. If in the callers, reader should learn the pipeline case by case.

Ok. Changed as your suggested.

jianjuns · 2020-12-01T01:40:54Z

@wenyingd @tnqn : do you have further comments on this one?

tnqn · 2020-11-25T02:59:10Z

pkg/agent/openflow/pipeline.go

 func (c *client) l3FwdFlowRouteToGW(gwMAC net.HardwareAddr, category cookie.Category) []binding.Flow {
 	l3FwdTable := c.pipeline[l3ForwardingTable]
 	var flows []binding.Flow
 	for _, ipProto := range c.ipProtocols {
 		flows = append(flows, l3FwdTable.BuildFlow(priorityLow).MatchProtocol(ipProto).
 			Action().SetDstMAC(gwMAC).
-			Action().DecTTL().


Yes, I think so.

pkg/agent/openflow/pipeline.go

tnqn · 2020-12-01T10:42:52Z

pkg/agent/openflow/pipeline.go

-			l3DecTTLTable:         bridge.CreateTable(l3DecTTLTable, conntrackCommitTable, binding.TableMissActionNext),
-			l2ForwardingCalcTable: bridge.CreateTable(l2ForwardingCalcTable, IngressEntryTable, binding.TableMissActionNext),
+			l3DecTTLTable:         bridge.CreateTable(l3DecTTLTable, l2ForwardingCalcTable, binding.TableMissActionNext),
+			l2ForwardingCalcTable: bridge.CreateTable(l2ForwardingCalcTable, conntrackCommitTable, binding.TableMissActionNext),


Sounds good to me, but would prefer we don't re-declare that the next table is conntrackCommitTable in l2ForwardCalcFlow.

pkg/agent/openflow/pipeline.go

Let the packets to the tunnel port go through L2ForwardingCalcTable as well, to be consistent with other types of traffic. Redirect packets to gateway and tunnel, as well as BUM packets to conntrackCommitTable and bypass ingress NetworkPolicy enforcement. Let l3DecTTLTable decrement TTL for packets from tunnel to local Pods.

tnqn

LGTM, thanks for addressing my comments!

jianjuns · 2020-12-04T05:15:51Z

/test-all

srikartati · 2020-12-07T19:01:09Z

Hi @jianjuns,
e2e tests on kind cluster with noEncap mode seems to be failing after this PR is merged, in particular TestNetworkPolicyStats.
I checked a few PRs with this commit, they have the same failure. Could you confirm if this is true and is there any issue?

One of the current PRs (#1617): https://github.com/vmware-tanzu/antrea/pull/1617/checks?check_run_id=1501453349
I see failure on my PR too.. #1549

jianjuns · 2020-12-07T19:40:25Z

@srikartati Sure. Let me check if it is relevant to my PR. Add @tnqn for his notice too.

jianjuns · 2020-12-07T20:01:33Z

@tnqn : from the test log, seems egress NP stats cannot be retrieved. However, I did not see how my PR is relevant to that. The only change seems relevant is that I removed "MatchPriority" in allow/dropRulesMetricFlows, as it seems not needed.
Do you have any idea?

antoninbas · 2020-12-07T20:58:33Z

@jianjuns looks like it's ingress NP stats, not egress

could it be because of this: https://github.com/vmware-tanzu/antrea/blob/da021fa75c7eceb1082fbef8345826c28fbe39a3/pkg/agent/openflow/pipeline.go#L697-L699

I'm not 100% sure, but is it possible that in noEncap mode we do not create DefaultTunOFPort, and thus that port number can be used for a regular Pod (therefore causing ingress NP enforcement to be bypassed for this Pod). Not sure why this would be the only test to fail. But I notice that we typically always use networkConfig.TrafficEncapMode.SupportsEncap() in the rest of the code before comparing to DefaultTunOFPort.

jianjuns · 2020-12-07T21:16:36Z

@antoninbas I thought ingress NP stats check passed but egress stats was not found. But you are right it might be ingress stats was not correctly updated.

And your theory about DefaultTunOFPort makes sense! At least we should not decide the next table based on the ofPort value. Let me change it.

jianjuns · 2020-12-07T22:19:37Z

Seems the failure is gone after changing the code: #1626.
@antoninbas @srikartati @tnqn

Let the packets to the tunnel port go through L2ForwardingCalcTable as well, to be consistent with other types of traffic. Redirect packets to gateway and tunnel, as well as BUM packets to conntrackCommitTable and bypass ingress NetworkPolicy enforcement. Let l3DecTTLTable decrement TTL for packets from tunnel to local Pods.

jianjuns requested review from wenyingd and tnqn November 24, 2020 05:56

vmwclabot added the cla-not-required label Nov 24, 2020

tnqn reviewed Nov 24, 2020

View reviewed changes

jianjuns force-pushed the l2fwd-flow branch 2 times, most recently from 90fc5c7 to d74d739 Compare November 24, 2020 22:28

jianjuns commented Nov 24, 2020

View reviewed changes

jianjuns force-pushed the l2fwd-flow branch from d74d739 to 494412a Compare November 24, 2020 22:43

jianjuns commented Nov 24, 2020

View reviewed changes

wenyingd reviewed Nov 25, 2020

View reviewed changes

wenyingd reviewed Nov 26, 2020

View reviewed changes

jianjuns force-pushed the l2fwd-flow branch from 494412a to baf13f1 Compare November 26, 2020 18:28

tnqn reviewed Dec 1, 2020

View reviewed changes

jianjuns force-pushed the l2fwd-flow branch from baf13f1 to fb1721c Compare December 1, 2020 20:11

tnqn previously approved these changes Dec 3, 2020

View reviewed changes

pkg/agent/openflow/pipeline.go Show resolved Hide resolved

jianjuns dismissed tnqn’s stale review via 22ffe91 December 3, 2020 17:01

jianjuns force-pushed the l2fwd-flow branch from fb1721c to 22ffe91 Compare December 3, 2020 17:01

tnqn approved these changes Dec 4, 2020

View reviewed changes

jianjuns merged commit de3bb94 into antrea-io:master Dec 4, 2020

jianjuns mentioned this pull request Dec 5, 2020

Update OVS pipeline doc for the L2 forwarding flow changes #1619

Merged

Change L2 forwarding flows for traffic to gateway and tunnel #1594

Change L2 forwarding flows for traffic to gateway and tunnel #1594

Conversation

jianjuns commented Nov 24, 2020

antrea-bot commented Nov 24, 2020

codecov-io commented Nov 24, 2020 • edited Loading

Codecov Report

jianjuns commented Nov 24, 2020

tnqn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenyingd Nov 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenyingd Nov 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenyingd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jianjuns commented Nov 25, 2020

jianjuns commented Nov 25, 2020

wenyingd commented Nov 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jianjuns commented Dec 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

jianjuns commented Dec 4, 2020

srikartati commented Dec 7, 2020 • edited Loading

jianjuns commented Dec 7, 2020

jianjuns commented Dec 7, 2020

antoninbas commented Dec 7, 2020

jianjuns commented Dec 7, 2020

jianjuns commented Dec 7, 2020

codecov-io commented Nov 24, 2020 •

edited

Loading

wenyingd Nov 25, 2020 •

edited

Loading

wenyingd Nov 25, 2020 •

edited

Loading

srikartati commented Dec 7, 2020 •

edited

Loading