Support NodePortLocal on Antrea Windows Agent and Update NPL annotation #3453

XinShuYang · 2022-03-16T05:54:56Z

Support NodePortLocal rules on by using NetNatStaticMapping on windows
Support NPL agent on Windows platform
Require the same Antrea NPL configuration as Linux

Signed-off-by: Shuyang Xin gavinx@vmware.com

codecov-commenter · 2022-03-16T06:58:49Z

Codecov Report

Merging #3453 (a377b57) into main (58f17de) will decrease coverage by 4.83%.
The diff coverage is 60.00%.

@@            Coverage Diff             @@
##             main    #3453      +/-   ##
==========================================
- Coverage   62.20%   57.37%   -4.84%     
==========================================
  Files         281      398     +117     
  Lines       40096    55954   +15858     
==========================================
+ Hits        24941    32102    +7161     
- Misses      13190    21351    +8161     
- Partials     1965     2501     +536

Flag	Coverage Δ
e2e-tests	`46.16% <60.00%> (?)`
integration-tests	`38.20% <ø> (?)`
kind-e2e-tests	`53.11% <60.00%> (+5.12%)`	⬆️
unit-tests	`43.92% <20.63%> (+0.01%)`	⬆️

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

Impacted Files	Coverage Δ
pkg/agent/nodeportlocal/k8s/annotations.go	`100.00% <ø> (+15.55%)`	⬆️
pkg/agent/nodeportlocal/npl_agent_init.go	`57.14% <ø> (ø)`
pkg/features/antrea_features.go	`11.11% <ø> (ø)`
.../agent/nodeportlocal/portcache/port_table_linux.go	`57.69% <57.69%> (ø)`
pkg/agent/util/net.go	`54.97% <66.66%> (+3.42%)`	⬆️
pkg/agent/nodeportlocal/portcache/port_table.go	`67.50% <75.00%> (+7.23%)`	⬆️
pkg/agent/nodeportlocal/k8s/npl_controller.go	`61.12% <100.00%> (-0.10%)`	⬇️
pkg/agent/nodeportlocal/rules/iptable_rule.go	`55.78% <100.00%> (+1.44%)`	⬆️
pkg/agent/openflow/client.go	`70.28% <100.00%> (+0.90%)`	⬆️
pkg/agent/openflow/pipeline.go	`74.16% <100.00%> (+0.54%)`	⬆️
... and 170 more

XinShuYang · 2022-03-23T15:55:17Z

/test-e2e

XinShuYang · 2022-04-01T05:33:26Z

/test-windows-all

XinShuYang · 2022-04-05T10:00:25Z

/test-windows-e2e

antoninbas · 2022-04-05T18:47:17Z

pkg/agent/nodeportlocal/npl_agent_init_windows.go

@@ -18,28 +18,31 @@
 package nodeportlocal


do we still need this file? it seems to be pretty much the same as npl_agent_init.go now. I think you just need to remove the //go:build !windows tag in npl_agent_init.go.

Updated, thanks.

antoninbas · 2022-04-05T18:59:16Z

pkg/agent/nodeportlocal/portcache/port_table_windows.go

+// PrepareAddRule closes socket on Windows before inserting NetNatStaticMapping rules.
+func PrepareAddRule(protocolSocketData *ProtocolSocketData) error {
+	if err := protocolSocketData.socket.Close(); err != nil {
+		return fmt.Errorf("Windows socket close failed %v", err)
+	}
+	return nil
+}


This seems quite hacky to me.
On Linux, we open a socket to ensure that the port is reserved for this specific mapping, as otherwise it would be possible for another process to use the port (the DNAT iptables rule doesn't "reserve" the port).
If on Windows, inserting a NetNatStaticMapping rule means that the port cannot be used by any other process, then there is no need to open a socket at all.
I don't know if it is exactly the same thing, but the inspiration from this comes from the kube-proxy iptables & IPVS implementations, which open a socket to reserve ports. However, there is no equivalent for the kube-proxy winkernel implementation.

@antoninbas NPL will try to listen to a port during getting free port stage.The reason to reserve the port is to make sure the free port is avaliable until inserting NetNatStaticMapping rules. Otherwise other processes are more likely to occupy the allocated port in advance, which causes the rule insertion to fail. I also had an discussion about this implementation with Wenying, @wenyingd do you have other comments here?

I don't understand. I would say that if the NetNatStaticMapping rule insertion fails, you just need to try with the next port, and so on. This is not so different from trying to open a socket first: if the port is not available, it will fail, and we try with the next port.

If NetNatStaticMapping insertion fails, the reason is not necessarily that the port is invalid. But if the port can be listened and NetNatStaticMapping insertion still fails we can make troubleshooting easier.

So, we cannot know the reason after NetNatStaticMapping fails? I also feel creating a socket is not worthwhile.

I agree to create NetNatStaticMapping directly to for a free port chosen.

Thanks for all comments. After discussion I agree it is not necessary to open socket in Windows. The result of rule insertion/deletion can be used by port reservation. PR updated.

XinShuYang · 2022-04-06T10:14:13Z

/test-windows-e2e

XinShuYang · 2022-04-08T00:55:49Z

/test-integration

XinShuYang · 2022-04-08T01:03:24Z

/test-integration

lgtm-com · 2022-05-24T14:43:46Z

This pull request introduces 1 alert and fixes 1 when merging 4ead74e into 7487453 - view on LGTM.com

new alerts:

1 for Incorrect conversion between integer types

fixed alerts:

1 for Incorrect conversion between integer types

antoninbas · 2022-05-24T18:09:17Z

Hi Antonin, thanks for the new comments. Wenying and I also agreed that we should set the unified implementation as our ultimate goal. But we’d like to only introduce windows function and preserve Linux legacy behavior in this PR for Antrea1.7 because we may not have enough time for development and full verification. If you agree I can build another PR to implement final unified code in next release. What do you think? @antoninbas Other comments has been addressed.

It feels to me that we are not preserving exactly the Linux legacy behavior though: while we are still allocating the same nodePort value for UDP & TCP when the podPort is the same, we are now generating 2 distinct Pod annotations instead of 1. Given this change, I think it would have been ok to unify the implementations from the get-go. That being said, if you prefer to do it in a follow-up PR, that's fine by me.

XinShuYang · 2022-05-25T15:05:54Z

It feels to me that we are not preserving exactly the Linux legacy behavior though: while we are still allocating the same nodePort value for UDP & TCP when the podPort is the same, we are now generating 2 distinct Pod annotations instead of 1. Given this change, I think it would have been ok to unify the implementations from the get-go. That being said, if you prefer to do it in a follow-up PR, that's fine by me.

@antoninbas Thanks, the "legacy behavior" I mentioned does mean that allocating the same nodePort value when the podPort is the same, and annotation format is the same for both Linux and Windows.
I have created an issue to follow all related features, and will build another pr for unified implementation.
#3826
Do you have more comments?

lgtm-com · 2022-05-25T15:18:36Z

This pull request introduces 1 alert and fixes 1 when merging b699ba2 into 9425c61 - view on LGTM.com

new alerts:

1 for Incorrect conversion between integer types

fixed alerts:

1 for Incorrect conversion between integer types

tnqn · 2022-05-26T15:37:32Z

@XinShuYang could you resolve conflict so we can merge?

lgtm-com · 2022-05-29T05:08:14Z

This pull request introduces 1 alert and fixes 1 when merging e35ac0c into 58f17de - view on LGTM.com

new alerts:

1 for Incorrect conversion between integer types

fixed alerts:

1 for Incorrect conversion between integer types

XinShuYang · 2022-05-29T12:18:16Z

/test-all
/test-windows-all

XinShuYang · 2022-05-29T15:28:04Z

@tnqn Updated, thanks.

tnqn · 2022-05-30T04:06:18Z

@wenyingd could you confirm code is expected after rebasing? I will merge it with your approval.

tnqn · 2022-05-30T04:06:47Z

/test-integration

wenyingd

overall LGTM, one minor comment.

wenyingd · 2022-05-30T09:05:31Z

pkg/agent/util/net.go

@@ -36,6 +38,8 @@ const (

 	FamilyIPv4 uint8 = 4
 	FamilyIPv6 uint8 = 6
+
+	AntreaNatName = "antrea-nat"


Move it to net_windows.go

Updated, thanks.

* Support NodePortLocal rules on by using NetNatStaticMapping on windows * Support NPL agent on Windows platform * Require the same Antrea NPL configuration as Linux issue antrea-io#3826 Signed-off-by: Shuyang Xin <gavinx@vmware.com>

Currently mutateAntreaConfigMap can only support linux agent configmap. This patch will support windows configmap processing. issue antrea-io#3826 Signed-off-by: Shuyang Xin <gavinx@vmware.com>

Add related image to support more e2e tests on windows. Signed-off-by: Shuyang Xin <gavinx@vmware.com>

* Add NPL Netnat rules check on windows * Replace busybox with agnhost as the client pod * Still skip windows test by default due to the ovs HNSCall issue issue antrea-io#3826 Signed-off-by: Shuyang Xin <gavinx@vmware.com>

* Add protocol string to new NPLAnnotation set the protocols map as a deprecated value. * Support Node port for UDP and TCP using the different number for a single Pod on Windows. issue antrea-io#3826 Signed-off-by: Shuyang Xin <gavinx@vmware.com>

* Update annotation example with new protocol field. * Update NPL windows support feature. Signed-off-by: Shuyang Xin <gavinx@vmware.com>

XinShuYang · 2022-05-30T10:02:55Z

/test-all
/test-windows-all

wenyingd

LGTM

lgtm-com · 2022-05-30T10:27:04Z

This pull request introduces 1 alert and fixes 1 when merging a377b57 into f8559ec - view on LGTM.com

new alerts:

1 for Incorrect conversion between integer types

fixed alerts:

1 for Incorrect conversion between integer types

XinShuYang · 2022-05-30T15:00:33Z

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

XinShuYang · 2022-05-30T16:28:03Z

/test-conformance

XinShuYang · 2022-05-30T23:33:11Z

/test-conformance

XinShuYang requested a review from wenyingd March 16, 2022 05:55

XinShuYang force-pushed the npl branch 12 times, most recently from c302ab1 to 75c12d5 Compare March 22, 2022 07:52

XinShuYang force-pushed the npl branch 3 times, most recently from 2764a37 to 720288a Compare April 1, 2022 05:02

XinShuYang force-pushed the npl branch from 720288a to 294dfee Compare April 1, 2022 07:47

XinShuYang force-pushed the npl branch from 294dfee to dd07bb1 Compare April 5, 2022 15:14

antoninbas reviewed Apr 5, 2022

View reviewed changes

XinShuYang force-pushed the npl branch 2 times, most recently from 66ce016 to edeb9bd Compare April 6, 2022 02:26

XinShuYang force-pushed the npl branch from edeb9bd to 4bd3e2c Compare April 8, 2022 16:29

XinShuYang force-pushed the npl branch from 4bd3e2c to 8028fdc Compare April 16, 2022 16:43

XinShuYang force-pushed the npl branch from 4ead74e to b699ba2 Compare May 25, 2022 14:52

antoninbas previously approved these changes May 25, 2022

View reviewed changes

XinShuYang dismissed antoninbas’s stale review via e35ac0c May 29, 2022 04:37

XinShuYang force-pushed the npl branch from b699ba2 to e35ac0c Compare May 29, 2022 04:37

wenyingd reviewed May 30, 2022

View reviewed changes

XinShuYang added 6 commits May 30, 2022 18:01

Support windows agent configmap for mutateAntreaConfigMap

67c1ce6

Currently mutateAntreaConfigMap can only support linux agent configmap. This patch will support windows configmap processing. issue antrea-io#3826 Signed-off-by: Shuyang Xin <gavinx@vmware.com>

Support creating nginx pods on windows

ed9dad0

Add related image to support more e2e tests on windows. Signed-off-by: Shuyang Xin <gavinx@vmware.com>

Support NPL e2e test for windows nodes

c0eb7b9

* Add NPL Netnat rules check on windows * Replace busybox with agnhost as the client pod * Still skip windows test by default due to the ovs HNSCall issue issue antrea-io#3826 Signed-off-by: Shuyang Xin <gavinx@vmware.com>

Update docs for NodePortLocal

a377b57

* Update annotation example with new protocol field. * Update NPL windows support feature. Signed-off-by: Shuyang Xin <gavinx@vmware.com>

XinShuYang force-pushed the npl branch from e35ac0c to a377b57 Compare May 30, 2022 10:01

wenyingd approved these changes May 30, 2022

View reviewed changes

tnqn merged commit 87918f6 into antrea-io:main May 31, 2022

antoninbas mentioned this pull request Jun 1, 2022

Support NodePortLocal on Windows agent and update NPL annotation #3826

Closed

6 tasks

Support NodePortLocal on Antrea Windows Agent and Update NPL annotation #3453

Support NodePortLocal on Antrea Windows Agent and Update NPL annotation #3453

Conversation

XinShuYang commented Mar 16, 2022

codecov-commenter commented Mar 16, 2022 • edited Loading

Codecov Report

XinShuYang commented Mar 23, 2022

XinShuYang commented Apr 1, 2022

XinShuYang commented Apr 5, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XinShuYang commented Apr 6, 2022

XinShuYang commented Apr 8, 2022

XinShuYang commented Apr 8, 2022

lgtm-com bot commented May 24, 2022

antoninbas commented May 24, 2022

XinShuYang commented May 25, 2022 • edited Loading

lgtm-com bot commented May 25, 2022

tnqn commented May 26, 2022

lgtm-com bot commented May 29, 2022

XinShuYang commented May 29, 2022

XinShuYang commented May 29, 2022

tnqn commented May 30, 2022

tnqn commented May 30, 2022

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

XinShuYang commented May 30, 2022

wenyingd left a comment

Choose a reason for hiding this comment

lgtm-com bot commented May 30, 2022

XinShuYang commented May 30, 2022

XinShuYang commented May 30, 2022

XinShuYang commented May 30, 2022

codecov-commenter commented Mar 16, 2022 •

edited

Loading

XinShuYang commented May 25, 2022 •

edited

Loading