[Windows] Bugfix: SNAT is not performed on the +est packets. #2702

wenyingd · 2021-09-02T03:32:55Z

Load the SNAT mark in snatTable on the established connection packets which is sent out from local Pods.
Fixes #2701

Signed-off-by: wenyingd wenyingd@vmware.com

wenyingd · 2021-09-02T03:36:52Z

/test-all

wenyingd · 2021-09-02T05:59:38Z

/test-windows-all
/test-conformance
/test-windows-e2e

wenyingd · 2021-09-02T06:00:48Z

/test-windows-conformance
/test-windows-networkpolicy
/test-ipv6-all
/test-ipv6-only-all

wenyingd · 2021-09-02T08:01:39Z

pkg/agent/openflow/pipeline_windows.go

-	l3FwdTable := c.pipeline[l3ForwardingTable]
-	nextTable := l3FwdTable.GetNext()
+	snatTable := c.pipeline[snatTable]
+	nextTable := snatTable.GetNext()


@jianjuns @tnqn I am thinking shall we resubmit the packet to the l3DecTTLTable after SNAT on Windows? In current implementation (<= Antrea 1.2), we output the SNATed packet to the uplink interface, and we didn't decrement TTL because we define l2ForwardingCalcTable is the one next to snatTable. I think it is possibly because we output the packet to antrea-gw0 on Linux, and TTL is decremented on the Linux host.

That makes sense to me.

@wenyingd @jianjuns It looked to me that you are outputting SNATted packet to antrea-gw0 interface (as destination IP is in a different subnet) and not to the uplink interface directly on Windows too. Once it goes to antrea-gw0, it comes back to OVS pipeline via br-int (as that is the one that has the nodeIP). And then you o/p it via uplink interface (e.g Ethernet).

You probably meant the same thing. But, I wanted to make sure that I understand the converstion.

If you are submitting it directly to uplink interface, how would you know the mac address of this out of k8s cluster IP?

@shettyg Thanks for reminding me. You are correct, we actually output the packet th antrea-gw0 even if we perform SNAT inside OVS. The current code for TTL is correct, and please ignore my previous suggestion. I would add comments here for the next table so that we could avoid similar confusion in future.

jianjuns · 2021-09-02T18:16:01Z

build/yamls/antrea-windows.yml

@@ -95,7 +95,7 @@ data:
    #                    supporting Pod traffic across IP subnets.
    # hybrid:            noEncap if source and destination Nodes are on the same subnet, otherwise encap.
    #
-    #trafficEncapMode: encap


You will revert the change before merge?

Yes, this change is to trigger CI test only.

jianjuns · 2021-09-02T18:19:05Z

pkg/agent/openflow/pipeline_windows.go

-	l3FwdTable := c.pipeline[l3ForwardingTable]
-	nextTable := l3FwdTable.GetNext()
+	snatTable := c.pipeline[snatTable]
+	nextTable := snatTable.GetNext()


That makes sense to me.

codecov-commenter · 2021-09-03T02:16:33Z

Codecov Report

Merging #2702 (930d41a) into release-1.2 (191c7b7) will increase coverage by 4.85%.
The diff coverage is n/a.

@@               Coverage Diff               @@
##           release-1.2    #2702      +/-   ##
===============================================
+ Coverage        60.19%   65.04%   +4.85%     
===============================================
  Files              284      284              
  Lines            22385    25672    +3287     
===============================================
+ Hits             13474    16699    +3225     
+ Misses            7474     7413      -61     
- Partials          1437     1560     +123

Flag	Coverage Δ
e2e-tests	`55.94% <ø> (?)`
kind-e2e-tests	`47.18% <ø> (+0.02%)`	⬆️
unit-tests	`42.23% <ø> (-0.01%)`	⬇️

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

Impacted Files	Coverage Δ
pkg/controller/egress/ipallocator/allocator.go	`67.82% <0.00%> (-15.16%)`	⬇️
pkg/controller/networkpolicy/endpoint_querier.go	`77.64% <0.00%> (-13.79%)`	⬇️
pkg/apis/controlplane/v1beta1/conversion.go	`72.44% <0.00%> (-11.89%)`	⬇️
pkg/legacyapis/core/v1alpha2/register.go	`69.23% <0.00%> (-10.77%)`	⬇️
pkg/controller/egress/controller.go	`76.76% <0.00%> (-10.44%)`	⬇️
pkg/apis/stats/register.go	`71.42% <0.00%> (-10.39%)`	⬇️
pkg/legacyapis/stats/register.go	`71.42% <0.00%> (-10.39%)`	⬇️
pkg/ovs/openflow/ofctrl_meter.go	`33.84% <0.00%> (-10.16%)`	⬇️
pkg/legacyapis/security/v1alpha1/register.go	`73.33% <0.00%> (-10.00%)`	⬇️
pkg/agent/config/traffic_encap_mode.go	`91.30% <0.00%> (-8.70%)`	⬇️
... and 268 more

wenyingd · 2021-09-03T08:30:49Z

/test-all
/test-ipv6-all
/test-ipv6-only-all

jianjuns

LGTM.

wenyingd · 2021-09-06T01:17:50Z

/test-all-features-conformance
/test-conformance
/test-windows-conformance
/test-windows-e2e
/test-windows-networkpolicy

lzhecheng · 2021-09-06T03:21:28Z

/test-e2e

wenyingd · 2021-09-06T04:05:23Z

/test-all
/test-ipv6-all
/test-ipv6-only-all

wenyingd · 2021-09-06T06:03:21Z

/test-ipv6-conformance
/test-ipv6-e2e
/test-windows-networkpolicy

wenyingd · 2021-09-06T09:59:01Z

/test-e2e
/test-ipv6-e2e
/test-ipv6-conformance
/test-ipv6-networkpolicy
/test-windows-networkpolicy

wenyingd · 2021-09-07T05:45:06Z

/skip-e2e
The failed test cases in e2e include: TestIPSecTunnelConnectivity and TestIPSecDeleteStaleTunnelPorts. These two cases are not for noEncap mode.

wenyingd · 2021-09-07T05:45:38Z

/test-windows-networkpolicy

wenyingd · 2021-09-07T08:05:16Z

/skip-windows-networkpolicy
All windows-networkpolicy tests are passed on my local setup. I failed to trigger CI tests on windows-networkpolicy on Antrea v1.2 in noEncap mode.

tnqn · 2021-09-07T08:22:15Z

pkg/agent/openflow/pipeline_windows.go

+	// Do not decrement TTL on the SNATed packet on Windows. This is because the SNATed packet is output to antrea-gw0
+	// first to learn the destination MAC using the host ARP cache. And antrea-gw0 will decrement the TTL when it
+	// forwards the packet to the bridge interface on which the host IP address is configured.
+	nextTable := c.pipeline[l3DecTTLTable].GetNext()


The snatTable's default next table is l2ForwardingCalcTable, why we don't use its next table directly here but uses a non-related table's?
I think the meaning of default table is packets will go to it by default unless we explicitly change it for some reason. So I don't quite understand what's the indication here.

I want to explicitly skip "l3DecTTLTable" for the reason I commented here.

I understand the comment but what's the point of the default next table then? I think this would confuse readers that snatTable's next table is l3DecTTLTable so we explicitly skip it. Overriding nextTable only makes sense when we use a non-default value. If the normal path should go snatTable -> l3DecTTLTable, it would make sense to skip it like this. When the pipeline is designed as snatTable-> l2ForwardingCalcTable, doesn't it imply we only delTTL for particular traffic explicitly? I'm fine with the comment but want to avoid this confusing nextTable declaration.

Sure, updated.

Load the SNAT mark in snatTable on the established connection packets which is sent out from local Pods. Signed-off-by: wenyingd <wenyingd@vmware.com>

wenyingd · 2021-09-07T10:24:16Z

/test-all
/skip-ipv6-all
/skip-ipv6-only-all

I have passed Windows conformance test, networkpolicy test, and windows-e2e-test in noEncap mode on my local setup. Since the change is only on Windows Node and IPv6 cluster is not impacted, I skip IPv6 related tests.

tnqn

LGTM

wenyingd · 2021-09-08T01:07:13Z

/test-networkpolicy

wenyingd · 2021-09-08T05:08:56Z

/test-all

antoninbas · 2021-09-08T23:51:40Z

@wenyingd could you take care of back-porting to release-1.1?

wenyingd · 2021-09-09T01:08:35Z

@wenyingd could you take care of back-porting to release-1.1?

Yes, I would check if it is needed in release-1.1

wenyingd changed the base branch from main to release-1.2 September 2, 2021 03:33

wenyingd requested a review from jianjuns September 2, 2021 07:40

wenyingd commented Sep 2, 2021

View reviewed changes

jianjuns reviewed Sep 2, 2021

View reviewed changes

wenyingd force-pushed the bugfix_2701 branch from 450938f to eee3588 Compare September 3, 2021 01:51

wenyingd force-pushed the bugfix_2701 branch from eee3588 to 549c6c9 Compare September 3, 2021 02:55

jianjuns reviewed Sep 3, 2021

View reviewed changes

wenyingd force-pushed the bugfix_2701 branch from 549c6c9 to 15f4b63 Compare September 6, 2021 04:04

luolanzone mentioned this pull request Sep 7, 2021

IPsec e2e failure #2726

Closed

tnqn reviewed Sep 7, 2021

View reviewed changes

wenyingd force-pushed the bugfix_2701 branch from 15f4b63 to ad76105 Compare September 7, 2021 10:14

[Windows] Bugfix: SNAT is not performed on the +est packets.

930d41a

Load the SNAT mark in snatTable on the established connection packets which is sent out from local Pods. Signed-off-by: wenyingd <wenyingd@vmware.com>

wenyingd force-pushed the bugfix_2701 branch from ad76105 to 930d41a Compare September 7, 2021 10:23

tnqn approved these changes Sep 7, 2021

View reviewed changes

tnqn merged commit 6d7478f into antrea-io:release-1.2 Sep 8, 2021

This was referenced Sep 9, 2021

Automated cherry pick of #2702: Bugfix: SNAT is not performed on the +est packets. #2733

Merged

Automated cherry pick of #2702: Bugfix: SNAT is not performed on the +est packets. #2734

Closed

tnqn mentioned this pull request Sep 16, 2021

Windows bug: SNAT is not performed on the Service request packet if it is not new in Antrea v1.2 #2701

Closed

wenyingd deleted the bugfix_2701 branch October 8, 2021 14:02

[Windows] Bugfix: SNAT is not performed on the +est packets. #2702

[Windows] Bugfix: SNAT is not performed on the +est packets. #2702

wenyingd commented Sep 2, 2021 •

edited

wenyingd commented Sep 2, 2021

wenyingd commented Sep 2, 2021

wenyingd commented Sep 2, 2021

wenyingd Sep 2, 2021 •

edited

jianjuns Sep 2, 2021

shettyg Sep 2, 2021 •

edited

shettyg Sep 2, 2021 •

edited

wenyingd Sep 3, 2021

jianjuns Sep 2, 2021

wenyingd Sep 3, 2021

jianjuns Sep 2, 2021

codecov-commenter commented Sep 3, 2021 •

edited

wenyingd commented Sep 3, 2021

jianjuns left a comment

wenyingd commented Sep 6, 2021

lzhecheng commented Sep 6, 2021

wenyingd commented Sep 6, 2021

wenyingd commented Sep 6, 2021

wenyingd commented Sep 6, 2021

wenyingd commented Sep 7, 2021

wenyingd commented Sep 7, 2021

wenyingd commented Sep 7, 2021

tnqn Sep 7, 2021

wenyingd Sep 7, 2021

tnqn Sep 7, 2021

wenyingd Sep 7, 2021

wenyingd commented Sep 7, 2021

tnqn left a comment

wenyingd commented Sep 8, 2021

wenyingd commented Sep 8, 2021

antoninbas commented Sep 8, 2021 •

edited

wenyingd commented Sep 9, 2021

[Windows] Bugfix: SNAT is not performed on the +est packets. #2702

[Windows] Bugfix: SNAT is not performed on the +est packets. #2702

Conversation

wenyingd commented Sep 2, 2021 • edited

wenyingd commented Sep 2, 2021

wenyingd commented Sep 2, 2021

wenyingd commented Sep 2, 2021

wenyingd Sep 2, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shettyg Sep 2, 2021 • edited

Choose a reason for hiding this comment

shettyg Sep 2, 2021 • edited

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

codecov-commenter commented Sep 3, 2021 • edited

Codecov Report

wenyingd commented Sep 3, 2021

jianjuns left a comment

Choose a reason for hiding this comment

wenyingd commented Sep 6, 2021

lzhecheng commented Sep 6, 2021

wenyingd commented Sep 6, 2021

wenyingd commented Sep 6, 2021

wenyingd commented Sep 6, 2021

wenyingd commented Sep 7, 2021

wenyingd commented Sep 7, 2021

wenyingd commented Sep 7, 2021

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 commented Sep 7, 2021

tnqn left a comment

Choose a reason for hiding this comment

wenyingd commented Sep 8, 2021

wenyingd commented Sep 8, 2021

antoninbas commented Sep 8, 2021 • edited

wenyingd commented Sep 9, 2021

wenyingd commented Sep 2, 2021 •

edited

wenyingd Sep 2, 2021 •

edited

shettyg Sep 2, 2021 •

edited

shettyg Sep 2, 2021 •

edited

codecov-commenter commented Sep 3, 2021 •

edited

antoninbas commented Sep 8, 2021 •

edited