[Windows] Kube-proxy replacement for NodePort #2863

lzhecheng · 2021-10-04T11:19:19Z

No description provided.

codecov-commenter · 2021-10-04T12:01:27Z

Codecov Report

Merging #2863 (5622c95) into main (e81f444) will increase coverage by 0.17%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2863      +/-   ##
==========================================
+ Coverage   60.32%   60.50%   +0.17%     
==========================================
  Files         283      283              
  Lines       23603    23644      +41     
==========================================
+ Hits        14239    14305      +66     
+ Misses       7845     7808      -37     
- Partials     1519     1531      +12

Flag	Coverage Δ
kind-e2e-tests	`47.59% <ø> (+0.16%)`	⬆️
unit-tests	`40.90% <ø> (-0.05%)`	⬇️

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

Impacted Files	Coverage Δ
pkg/agent/config/node_config.go	`100.00% <ø> (ø)`
...ver/registry/controlplane/nodestatssummary/rest.go	`50.00% <0.00%> (-50.00%)`	⬇️
pkg/antctl/antctl.go	`66.66% <0.00%> (-33.34%)`	⬇️
pkg/controller/networkpolicy/status_controller.go	`73.20% <0.00%> (-3.93%)`	⬇️
pkg/agent/controller/traceflow/packetin.go	`64.60% <0.00%> (-0.37%)`	⬇️
pkg/agent/flowexporter/exporter/exporter.go	`80.98% <0.00%> (-0.33%)`	⬇️
pkg/agent/route/route_linux.go	`49.60% <0.00%> (-0.33%)`	⬇️
pkg/agent/proxy/proxier.go	`60.20% <0.00%> (ø)`
pkg/ovs/ovsctl/interface.go	`0.00% <0.00%> (ø)`
pkg/agent/querier/querier.go	`83.75% <0.00%> (ø)`
... and 11 more

lzhecheng · 2021-10-14T00:56:37Z

For the reviewers, please notice that this PR is base on ClusterIP one which still changes from time to time. So just review the last commit is enough. Thank you.
6c65f47

wenyingd · 2021-10-14T01:05:42Z

pkg/agent/openflow/pipeline.go

+	// it cannot be 169.254.1.0 through 169.254.255.255.
+	// So 169.254.0.253 is a valid one.
+	ServiceNodePortVirtualIP    = net.ParseIP("169.254.0.253").To4()
+	ServiceNodePortVirtualIPNet = util.NewIPNet(ServiceNodePortVirtualIP)


Please provide the referrence link why 169.254.0.253 is valid.

wenyingd · 2021-10-14T01:09:23Z

pkg/agent/openflow/pipeline.go

+			c.pipeline[serviceClassifierTable].BuildFlow(priorityNormal).
+				Cookie(c.cookieAllocator.Request(cookie.Service).Raw()).
+				MatchProtocol(ipProtocol).
+				MatchDstIP(ServiceNodePortVirtualIP).


Is it possible to use ServiceNodePortVirtualIP on Linux, so that we could have a unique flow for both Linux and Window? @hongliangl @lzhecheng

pkg/agent/route/route_windows.go

jianjuns · 2021-10-15T01:09:31Z

pkg/agent/config/node_config.go

-	VirtualServiceIPv4 = net.ParseIP("169.254.169.253")
-	VirtualServiceIPv6 = net.ParseIP("fc01::aabb:ccdd:eeff")
+	VirtualServiceIPv4    = net.ParseIP("169.254.169.253")
+	VirtualServiceIPv4Net = util.NewIPNet(VirtualServiceIPv4)


If it is used in route_windows.go only, probably define a local variable there.

Updated in the cluster ip pr.

forgot to update?

Forgot to update after rebasing. It is correct now.

jianjuns · 2021-10-15T01:18:57Z

pkg/agent/controller/noderoute/node_route_controller.go

@@ -190,8 +197,30 @@ func (c *Controller) removeStaleGatewayRoutes() error {
 		desiredPodCIDRs = append(desiredPodCIDRs, podCIDRs...)
 	}

+	// TODO: This is not the best place to keep the ClusterIP Service routes.


Yes, it is strange we reconcile Service routes here. Should we reconcile Service routes from proxier? @hongliangl @tnqn

I think we shouldn't use more than one reconcile functions. If so, they may delete the routes added by another function.

I think the current conclusion is that these code is needed for function before a refactor is conducted. Such refactor will be in another PR.

docs/design/windows-design.md

tnqn · 2021-10-18T16:33:14Z

pkg/agent/route/route_windows.go

 	vRoute := &util.Route{
 		LinkIndex:         linkIndex,
 		DestinationSubnet: virtualServiceIPv4Net,
 		GatewayAddress:    net.IPv4zero,
-		RouteMetric:       util.MetricHigh,
+		RouteMetric:       metric,


This change seems pointless.

Updated. Originally I'd like to have 2 routes here so I defined a variable.

tnqn · 2021-10-18T16:36:33Z

pkg/agent/config/node_config.go

-	VirtualServiceIPv4 = net.ParseIP("169.254.169.253")
-	VirtualServiceIPv6 = net.ParseIP("fc01::aabb:ccdd:eeff")
+	VirtualServiceIPv4    = net.ParseIP("169.254.169.253")
+	VirtualServiceIPv4Net = util.NewIPNet(VirtualServiceIPv4)


forgot to update?

tnqn · 2021-10-18T16:39:03Z

pkg/agent/openflow/pipeline.go

+				MatchProtocol(ipProtocol).
+				MatchDstIP(config.VirtualServiceIPv4).
+				Action().LoadRegMark(ToNodePortAddressRegMark).
+				Done())


Isn't this flow duplicated with the above one?

tnqn · 2021-10-18T16:39:35Z

pkg/agent/route/route_windows.go

@@ -36,6 +36,9 @@ import (
 const (
 	inboundFirewallRuleName  = "Antrea: accept packets from local Pods"
 	outboundFirewallRuleName = "Antrea: accept packets to local Pods"
+
+	antreaNat = "antrea-nat"
+	antreaNatStaticMapping = "antrea-natstaticmapping"


Is this nat specific for nodeport traffic? If yes, could we name it in a way that can be associated with NodePort?

Yes, updated.

tnqn · 2021-10-18T16:40:34Z

pkg/agent/route/route_windows.go

@@ -377,6 +389,7 @@ func (c *Client) DeleteSNATRule(mark uint32) error {
 }

 func (c *Client) AddNodePort(nodePortAddresses []net.IP, port uint16, protocol binding.Protocol) error {
+	util.AddNetNatStaticMapping(antreaNatStaticMapping, "0.0.0.0", port, config.VirtualServiceIPv4, port, string(protocol))


it should handle the error

tnqn · 2021-10-18T16:42:45Z

pkg/agent/route/route_windows.go

@@ -239,6 +244,13 @@ func (c *Client) addVirtualServiceIPRoute(isIPv6 bool) error {
 		return err
 	}
 	klog.InfoS("Added virtual Service IP neighbor", "neighbor", vNeighbor)
+
+	// For NodePort Service, a new NetNat for NetNatStaticMapping is needed.
+	err := util.NewNetNat(antreaNatStaticMapping, config.VirtualServiceIPv4Net)


VirtualServiceIPv4Net is the original IP or the translation IP?

virtualServiceIPv4Net is the translated: 169.254.0.253/32

tnqn

The code LGTM overall, why don't squash the commits and add the description to the PR description as well?
I think it doesn't respect nodePortAddresses? Is it possible to support it? If yes, we should have an issue to track it. Regardless of whether it's possible, the comment of nodePortAddresses should mention it doesn't work for Windows yet.

lzhecheng · 2021-10-20T08:30:37Z

The code LGTM overall, why don't squash the commits and add the description to the PR description as well? I think it doesn't respect nodePortAddresses? Is it possible to support it? If yes, we should have an issue to track it. Regardless of whether it's possible, the comment of nodePortAddresses should mention it doesn't work for Windows yet.

@tnqn I have squashed the commits and added a TODO for nodePortAddresses. I will create an issue for it later. Besides, I updated NetNatStaticMapping powershell usage.

tnqn

Found two minor problems in the final review

tnqn · 2021-10-20T10:54:26Z

pkg/agent/util/net_windows.go

@@ -658,6 +657,67 @@ func CreateNetNatOnHost(subnetCIDR *net.IPNet) error {
 	return nil
 }

+func ReplaceNetNatStaticMapping(netNatName string, externalIPAddr string, externalPort uint16, internalIPAddr net.IP, internalPort uint16, proto string) error {


It's strange that externalIPAddr is a string but internalIPAddr is net.IP, can we unify it to string?

The function doesn't really do "replace": replace means it searchs the mapping based on the natName, externalIPAddress, externalPort, protocol, and overrides the mapping if the internalIPAddr, internalPort don't match. In current code, even if there is a mapping that have same external IP and port but different internal IP and port, it's not able to replace it.

Updated.

Now GetNetNatStaticMapping doesn't filter with internal IP and Port. Instead in ReplaceNetNatStaticMapping, it checks if internal IP and Port match inputs.

I don't see it's updated.

It does not make sense to pass internalIPAddr, internalPort to GetNetNatStaticMapping and just ingore them in the body.

Sorry, some git issue. Now it is correct.

Removed these parameters.

tnqn · 2021-10-20T10:54:51Z

pkg/agent/util/net_windows.go

+}
+
+// GetNetNatStaticMapping checks if a NetNatStaticMapping exists.
+func GetNetNatStaticMapping(netNatName string, externalIPAddr string, externalPort uint16, internalIPAddr net.IP, internalPort uint16, proto string) (string, error) {


Updated this time.

tnqn · 2021-10-20T13:12:27Z

pkg/agent/util/net_windows.go

@@ -658,6 +657,67 @@ func CreateNetNatOnHost(subnetCIDR *net.IPNet) error {
 	return nil
 }

+func ReplaceNetNatStaticMapping(netNatName string, externalIPAddr string, externalPort uint16, internalIPAddr net.IP, internalPort uint16, proto string) error {


I don't see it's updated.

It does not make sense to pass internalIPAddr, internalPort to GetNetNatStaticMapping and just ingore them in the body.

tnqn · 2021-10-20T13:16:03Z

pkg/agent/util/net_windows.go

+	}
+	parsed := parseGetNetCmdResult(staticMappingStr, 6)
+	if len(parsed) > 1 {
+		return fmt.Errorf("when ReplaceNetNatStaticMapping, number of parsed result of Get-NetNatStaticMapping is larger than 1, %v", parsed)


I tested get-netnatstaticmapping, it's not possible to install two mapping with same externalIPAddr+externalPort and different internalIPAddr+internalPort, and it makes sense from the API's perspective. So let's don't add code to handle something that won't happen.
It could just handle found case:

if len(parsed) > 0 { items := parsed[0] ... }

tnqn · 2021-10-20T13:16:48Z

pkg/agent/util/net_windows.go

+		if err != nil {
+			return err
+		}
+		if err := RemoveNetNatStaticMappingDirect(netNatName, id); err != nil {


Why Direct? Maybe you mean RemoveNetNatStaticMappingByID?

Originally I'd like to emphasize that it runs Remove-NetNatStaticMapping command directly. But ByID is good. Updated.

tnqn · 2021-10-20T13:17:04Z

pkg/agent/util/net_windows.go

+}
+
+// GetNetNatStaticMapping checks if a NetNatStaticMapping exists.
+func GetNetNatStaticMapping(netNatName string, externalIPAddr string, externalPort uint16, internalIPAddr net.IP, internalPort uint16, proto string) (string, error) {


tnqn · 2021-10-20T14:15:45Z

pkg/agent/util/net_windows.go

+	return err
+}
+
+func RemoveNetNatStaticMapping(netNatName string, externalIPAddr string, externalPort uint16, internalIPAddr string, internalPort uint16, proto string) error {


Similarly, it doesn't make sense to pass internalIPAddr and internalPort but ignore them in the function, which could give people a pression that it will only remove the mapping when all parameters match.

I understand we want to share some code between ReplaceNetNatStaticMapping and RemoveNetNatStaticMapping, then maybe we don't need RemoveNetNatStaticMappingByID and RemoveNetNatStaticMapping could have all arguments. In ReplaceNetNatStaticMapping, it could call RemoveNetNatStaticMapping with the given externalIPAddr and externalPort and the got internalIPAddr and internalPort.

Another approach is to remove internalIPAddr and internalPort from the arguments.

Go with the second approach.

I think only if calling NewNetNatStaticMapping needs internalIPAddr and internalPort soRemoveNetNatStaticMapping just doesn't need it. I am not sure why

In ReplaceNetNatStaticMapping, it could call RemoveNetNatStaticMapping

Since ReplaceNetNatStaticMapping calls RemoveNetNatStaticMapping means calling GetNetNatStaticMapping twice.

I forgot to mention in the first approach RemoveNetNatStaticMapping should call the command directly without calling GetNetNatStaticMapping first. The function should check if the returned error is "mapping not exist" and ignore it if yes.

I am bit confused. Without GetNetNatStaticMapping, StaticMappingID is unknown for Remove-NetNatStaticMapping.

I thought Remove-NetNatStaticMapping supports removing it by other arguments like ExternalIPAddress. But it seems not, so 1st approach can't work

tnqn

LGTM

jianjuns

Just a minor comment.

jianjuns · 2021-10-20T18:42:14Z

pkg/agent/config/node_config.go

@@ -52,7 +52,10 @@ var (
 	// - Use the virtual IP as onlink routing entry gateway in host routing entry.
 	// - Use the virtual IP as destination IP in host routing entry. It is used to forward DNATed NodePort packets
 	//   or replied SNATed Service packets back to Antrea gateway.
-	VirtualServiceIPv4 = net.ParseIP("169.254.169.253")
+	// - Use the virtual IP for InternalIPAddress parameter of Add-NetNatStaticMapping.
+	//   For Antrea, it cannot be an IP used by users but for Add-NetNatStaticMapping it cannot be 169.254.1.0


"The IP cannot be one used in the network, and cannot to be within the 169.254.1.0 - 169.254.254.255 range according to IETF."?

Probably remove "So 169.254.0.253 is a valid one"

Create "antrea-nat-nodeport" NetNat and the NetNatStaticMapping. Then destination IP of a packet with NodePort will be DNATed to VirtualServiceIPv4 at the Windows host network. Signed-off-by: Zhecheng Li <[email protected]>

lzhecheng · 2021-10-21T07:49:04Z

/test-integration

lzhecheng · 2021-10-21T08:24:33Z

/test-all /test-windows-all

lzhecheng · 2021-10-21T09:38:55Z

/test-windows-networkpolicy

lzhecheng force-pushed the win-nodeport-lb branch 4 times, most recently from 0400651 to 6c65f47 Compare October 13, 2021 09:21

lzhecheng changed the title ~~[WIP][Windows] Kube-proxy replacement for NodePort~~ [Windows] Kube-proxy replacement for NodePort Oct 13, 2021

lzhecheng force-pushed the win-nodeport-lb branch from 6c65f47 to a31fdbe Compare October 14, 2021 00:54

lzhecheng requested review from hongliangl, wenyingd, tnqn, jianjuns and antoninbas and removed request for hongliangl and wenyingd October 14, 2021 00:55

wenyingd reviewed Oct 14, 2021

View reviewed changes

lzhecheng force-pushed the win-nodeport-lb branch 2 times, most recently from 5d8db89 to e428019 Compare October 14, 2021 06:44

jianjuns reviewed Oct 15, 2021

View reviewed changes

lzhecheng added area/proxy Issues or PRs related to proxy functions in Antrea review-manager-test labels Oct 18, 2021

lzhecheng force-pushed the win-nodeport-lb branch 4 times, most recently from 3bc578d to c8640e9 Compare October 18, 2021 12:38

tnqn reviewed Oct 18, 2021

View reviewed changes

lzhecheng added this to the Antrea v1.4 release milestone Oct 19, 2021

lzhecheng force-pushed the win-nodeport-lb branch from c8640e9 to 4a5b8b4 Compare October 19, 2021 01:51

tnqn reviewed Oct 19, 2021

View reviewed changes

lzhecheng force-pushed the win-nodeport-lb branch from 61e6258 to a046131 Compare October 20, 2021 06:49

github-actions bot requested a review from tnqn October 20, 2021 07:00

lzhecheng force-pushed the win-nodeport-lb branch from a046131 to 39e6412 Compare October 20, 2021 08:25

lzhecheng mentioned this pull request Oct 20, 2021

[Windows] Support nodePortAddresses in Windows AntreaProxy NodePort #2911

Closed

tnqn reviewed Oct 20, 2021

View reviewed changes

lzhecheng force-pushed the win-nodeport-lb branch from 39e6412 to 4856ad8 Compare October 20, 2021 12:14

github-actions bot requested a review from tnqn October 20, 2021 12:15

lzhecheng force-pushed the win-nodeport-lb branch from 4856ad8 to 64dfd92 Compare October 20, 2021 12:16

tnqn reviewed Oct 20, 2021

View reviewed changes

lzhecheng force-pushed the win-nodeport-lb branch from 64dfd92 to 789a7e7 Compare October 20, 2021 13:52

github-actions bot requested a review from tnqn October 20, 2021 13:54

tnqn reviewed Oct 20, 2021

View reviewed changes

lzhecheng force-pushed the win-nodeport-lb branch from 789a7e7 to 36f2e6b Compare October 20, 2021 14:33

github-actions bot requested a review from tnqn October 20, 2021 14:52

tnqn previously approved these changes Oct 20, 2021

View reviewed changes

jianjuns reviewed Oct 20, 2021

View reviewed changes

[Windows] Use AntreaProxy to support NodePort Service

5622c95

Create "antrea-nat-nodeport" NetNat and the NetNatStaticMapping. Then destination IP of a packet with NodePort will be DNATed to VirtualServiceIPv4 at the Windows host network. Signed-off-by: Zhecheng Li <[email protected]>

lzhecheng dismissed tnqn’s stale review via 5622c95 October 21, 2021 06:04

lzhecheng force-pushed the win-nodeport-lb branch from 36f2e6b to 5622c95 Compare October 21, 2021 06:04

github-actions bot requested a review from tnqn October 21, 2021 06:15

tnqn approved these changes Oct 21, 2021

View reviewed changes

jianjuns approved these changes Oct 21, 2021

View reviewed changes

tnqn merged commit 93170ab into antrea-io:main Oct 22, 2021

lzhecheng deleted the win-nodeport-lb branch October 22, 2021 02:26

lzhecheng mentioned this pull request Nov 10, 2021

[WIP][Windows]Antrea Proxy NodePort Service Support #1934

Closed

[Windows] Kube-proxy replacement for NodePort #2863

[Windows] Kube-proxy replacement for NodePort #2863

Conversation

lzhecheng commented Oct 4, 2021

codecov-commenter commented Oct 4, 2021 • edited Loading

Codecov Report

lzhecheng commented Oct 14, 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

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

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

lzhecheng Oct 19, 2021 • edited Loading

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

lzhecheng commented Oct 20, 2021

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn Oct 20, 2021 • edited Loading

Choose a reason for hiding this comment

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzhecheng commented Oct 21, 2021

lzhecheng commented Oct 21, 2021

lzhecheng commented Oct 21, 2021

codecov-commenter commented Oct 4, 2021 •

edited

Loading

lzhecheng Oct 19, 2021 •

edited

Loading

tnqn Oct 20, 2021 •

edited

Loading