Add NodePort support for Antrea Proxy on Linux #1471

weiqiangt · 2020-11-02T03:20:37Z

Note: To verify this PR, the NodePort support is enabled. The feature gate should be disabled before merging this PR.
Resolves #1463.
Signed-off-by: Weiqiang Tang [email protected]

antrea-bot · 2020-11-02T03:20:51Z

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-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-02T03:56:34Z

Codecov Report

❗ No coverage uploaded for pull request base (main@1f7775e). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1471   +/-   ##
=======================================
  Coverage        ?   41.09%           
=======================================
  Files           ?      116           
  Lines           ?    14717           
  Branches        ?        0           
=======================================
  Hits            ?     6048           
  Misses          ?     8148           
  Partials        ?      521

Flag	Coverage Δ
unit-tests	`41.09% <0.00%> (?)`

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

weiqiangt · 2020-11-02T11:42:05Z

/test-all

tnqn

I haven't finished review. But I did some test based on the patch and found an issue:
When scaling down backend pods, the members in openflow group doesn't change, leading to connectivity issue.

tnqn · 2021-01-21T14:13:06Z

build/yamls/base/conf/antrea-agent.conf

@@ -116,3 +119,13 @@ featureGates:
 # whenever a Pod's container defines a specific port to be exposed (each container can define a list of ports as pod.spec.containers[].ports),
 # and all Node traffic directed to that port will be forwarded to the Pod.
 #nplPortRange: 40000-41000
+
+# The virtual IP for NodePort Service support. It must be a link-local IP otherwise the Agents will report error.
+#nodePortVirtualIP: 169.254.169.110


Have you tried policy route approach? I did an experiment and it worked as we discussed.
If NAT is not likely to be the final solution, better not to add this argument in case compatbility issue in future. We can just use a reserved IP in alpha phase I think.

Why we care it must be link-local?

@tnqn, I tried the policy route way.
If the destination is a loopback address we still need to do a DNAT. Considering this is an ALPHA feature for now, we can discuss it more in the future.

@jianjuns, do you think we should use any forwardable address here?

I want to understand the current design first. Why IPv4 must be loopback, and IPv6 must not be?

Got more understanding after reading the design proposal. I think no need to restrict the IP must be link-local. We can recommend link-local though.

Thanks @jianjuns, updated.

I mean there are possibilities we don't need to do DNAT in future like the two approaches we have discussed. Even if we still need it for loopback address ( maybe we don't need it for this case as well), I think hardcoding a reserved IP is enough at this stage.
I just want to avoid introducing implementation specific configurations when it's not stable so we don't have the complexity of upgrade (If user set it and we delete it in future, the process will crash).
And it doesn't seem that user can get what the configuration means and how to use it with the current comments.

@tnqn, thanks for the explanation. I removed these two options and made them constant for now.

jianjuns

Do you have a design doc for the reviewers to understand the design?

jianjuns · 2021-01-26T21:47:15Z

build/yamls/base/conf/antrea-agent.conf

@@ -116,3 +119,13 @@ featureGates:
 # whenever a Pod's container defines a specific port to be exposed (each container can define a list of ports as pod.spec.containers[].ports),
 # and all Node traffic directed to that port will be forwarded to the Pod.
 #nplPortRange: 40000-41000
+
+# The virtual IP for NodePort Service support. It must be a link-local IP otherwise the Agents will report error.
+#nodePortVirtualIP: 169.254.169.110


Why we care it must be link-local?

build/yamls/base/conf/antrea-agent.conf

jianjuns · 2021-01-26T21:49:04Z

cmd/antrea-agent/agent.go

@@ -180,17 +184,27 @@ func run(o *Options) error {

 	var proxier k8sproxy.Provider
 	if features.DefaultFeatureGate.Enabled(features.AntreaProxy) {
+		var nodePortAddresses []*net.IPNet


I feel we should have a separate feature gate to control NodePort by AntreaProxy or not.

Yes, I added a new feature gate AntreaProxyNodePort to control it. Also, I updated the workflow here. The parsing of NodeAddress will go only if both AntreaProxyNodePort and AntreaProxy are enabled.

weiqiangt · 2021-01-27T01:37:55Z

Do you have a design doc for the reviewers to understand the design?

I attached the issue which includes the design.

weiqiangt · 2021-01-27T02:34:40Z

I haven't finished review. But I did some test based on the patch and found an issue:
When scaling down backend pods, the members in openflow group doesn't change, leading to connectivity issue.

Thanks @tnqn. I have fixed this issue and added checks in e2e.

weiqiangt · 2021-02-02T02:58:29Z

/test-all

weiqiangt · 2021-02-02T06:58:33Z

/test-all

weiqiangt · 2021-02-03T02:02:55Z

Windows conformance failed due to infrastructure failure, re-trigger it.
/test-windows-conformance

weiqiangt · 2021-02-03T17:39:29Z

/test-all

weiqiangt · 2021-02-03T17:39:55Z

/jenkins-ipv6-ds-conformance
/jenkins-ipv6-only-conformance

jianjuns · 2021-02-03T22:21:27Z

I do not mean to ask for a design change for this PR, but have we considered using TC to redirect traffic (https://man7.org/linux/man-pages/man8/tc-mirred.8.html)? Could it be faster or slower than iptables? @tnqn

weiqiangt · 2021-02-04T02:54:36Z

/test-all

Jenkins tests failed on some unrelated cases, re-trigger tests to have a double-check.

[k8s.io] InitContainer [NodeConformance] should not start app containers and fail the pod if init containers fail on a RestartNever pod [Conformance]
[k8s.io] InitContainer [NodeConformance] should invoke init containers on a RestartAlways pod [Conformance]
[k8s.io] Security Context When creating a pod with readOnlyRootFilesystem should run the container with writable rootfs when readOnlyRootFilesystem=false [NodeConformance] [Conformance]
[k8s.io] Probing container should be restarted with a exec "cat /tmp/health" liveness probe [NodeConformance] [Conformance]
[k8s.io] Kubelet when scheduling a busybox Pod with hostAliases should write entries to /etc/hosts [LinuxOnly] [NodeConformance] [Conformance]

weiqiangt · 2021-02-04T04:59:41Z

/test-ipv6-ds-conformance
/test-ipv6-only-conformance

weiqiangt · 2021-02-04T13:54:17Z

/test-windows-conformance

jianjuns · 2021-02-04T19:03:35Z

build/yamls/antrea.yml

@@ -1283,6 +1283,9 @@ data:
    # Service traffic.
    #  AntreaProxy: true

+    # Enable NodePort Service support in AntreaProxy in antrea-agent.
+      AntreaProxyNodePort: true


I would suggest not to enable it by default. Even we enable it, it does not provide any value, until we have a solution to remove kube-proxy.
@tnqn @antoninbas

This is the plan. I think Weiqiang only enables it for testing. See PR description "To verify this PR, the NodePort support is enabled. The feature gate should be disabled before merging this PR."

Yes, it is enabled only for testing. I will disable it when we're ready to merge.

Resolves antrea-io#1463. Signed-off-by: Weiqiang Tang <[email protected]>

jianjuns · 2021-03-15T22:51:01Z

cmd/antrea-agent/options.go

 }

 func newOptions() *Options {
 	return &Options{
+		nodePortVirtualIP:   net.ParseIP(defaultNodePortVirtualIP),


Probably better to move these to setDefaults()? I am fine to keep these two options hardcoded for now, but maybe later we should still make them configurable.

jianjuns · 2021-03-15T22:56:37Z

pkg/agent/nodeportlocal/rules/iptable_rule.go

@@ -29,50 +29,54 @@ import (
 const NodePortLocalChain = "ANTREA-NODE-PORT-LOCAL"

 // IPTableRules provides a client to perform IPTABLES operations
-type iptablesRules struct {
+type IPTableRules struct {


Do you remove "s" from "iptables" to IPTable" on purpose? I feel it should be "IPTablesRules".

jianjuns · 2021-03-15T22:57:35Z

pkg/agent/openflow/client.go

@@ -532,6 +532,15 @@ func (c *client) InstallGatewayFlows() error {
 	// Add flow to ensure the liveness check packet could be forwarded correctly.
 	flows = append(flows, c.localProbeFlow(gatewayIPs, cookie.Default)...)
 	flows = append(flows, c.ctRewriteDstMACFlows(gatewayConfig.MAC, cookie.Default)...)
+	if c.enableProxy {


Should we add the flows only when NodePort is enabled?

jianjuns · 2021-03-15T22:59:06Z

pkg/agent/openflow/pipeline.go

+func (c *client) arpNodePortVirtualResponderFlow() binding.Flow {
+	return c.pipeline[arpResponderTable].BuildFlow(priorityNormal).MatchProtocol(binding.ProtocolARP).
+		MatchARPOp(1).
+		MatchARPTpa(c.nodePortVirtualIPv4).


When Node IP is v6, no need to add this flow.

jianjuns · 2021-03-15T23:09:53Z

pkg/agent/route/route_linux.go

@@ -657,3 +794,145 @@ func (c *Client) UnMigrateRoutesFromGw(route *net.IPNet, linkName string) error
 	}
 	return nil
 }
+
+func (c *Client) ReconcileNodePort(nodeIPs []net.IP, svcEntries []*proxytypes.ServiceInfo) error {


I did not see this func is called, but might be my mistake?

jianjuns · 2021-03-15T23:48:49Z

pkg/agent/route/interfaces.go

@@ -38,6 +39,20 @@ type Interface interface {
 	// It should do nothing if the routes don't exist, without error.
 	DeleteRoutes(podCIDR *net.IPNet) error

+	// AddRoutes should add the route to the NodePort virtual IP.
+	AddNodePortRoute(isIPv6 bool) error


What you think adding a separate interface for AddNodePortRoute, AddNodePort, and DeleteNodePort, which can have different implementations in Linux (by Route Client) and Windows (by Openflow)? Or maybe on Windows they are not needed at all, as the existing Openflow Client interfaces can already cover these functions?

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

weiqiangt force-pushed the antrea-proxy-nodeport-dup branch from 9108527 to 1b3b6dc Compare November 2, 2020 08:29

weiqiangt linked an issue Nov 2, 2020 that may be closed by this pull request

Antrea Proxy NodePort Service Support #1463

Closed

weiqiangt added area/component/agent Issues or PRs related to the agent component kind/design Categorizes issue or PR as related to design. labels Nov 2, 2020

antoninbas added this to the Antrea v0.12.0 release milestone Nov 13, 2020

antoninbas modified the milestones: Antrea v0.12.0 release, Antrea v0.13.0 release Dec 16, 2020

weiqiangt force-pushed the antrea-proxy-nodeport-dup branch 9 times, most recently from 294c717 to d734e2b Compare January 15, 2021 07:51

weiqiangt force-pushed the antrea-proxy-nodeport-dup branch from d734e2b to c0b7207 Compare January 18, 2021 17:02

tnqn reviewed Jan 21, 2021

View reviewed changes

weiqiangt force-pushed the antrea-proxy-nodeport-dup branch from c0b7207 to 6d0ad1c Compare January 25, 2021 07:43

Base automatically changed from master to main January 26, 2021 00:00

weiqiangt force-pushed the antrea-proxy-nodeport-dup branch 3 times, most recently from 6d21ce5 to 550f9d2 Compare January 26, 2021 17:10

jianjuns reviewed Jan 26, 2021

View reviewed changes

weiqiangt force-pushed the antrea-proxy-nodeport-dup branch from b2656b1 to 401cbb7 Compare February 1, 2021 10:17

weiqiangt force-pushed the antrea-proxy-nodeport-dup branch from 401cbb7 to 97aacdf Compare February 2, 2021 06:42

weiqiangt force-pushed the antrea-proxy-nodeport-dup branch 2 times, most recently from e8ff683 to 41cbccb Compare February 3, 2021 16:26

jianjuns reviewed Feb 4, 2021

View reviewed changes

weiqiangt force-pushed the antrea-proxy-nodeport-dup branch from 41cbccb to f0837fa Compare February 10, 2021 06:47

antoninbas modified the milestones: Antrea v0.13.0 release, Antrea v0.14.0 release Feb 11, 2021

weiqiangt force-pushed the antrea-proxy-nodeport-dup branch 2 times, most recently from df06836 to 533826a Compare February 18, 2021 08:20

weiqiangt changed the title ~~Add NodePort support for Antrea Proxy~~ Add NodePort support for Antrea Proxy on Linux Feb 18, 2021

weiqiangt force-pushed the antrea-proxy-nodeport-dup branch 3 times, most recently from 5b3b1fd to 2342515 Compare February 24, 2021 07:58

Add NodePort support for Antrea Proxy on Linux

0b6dcd4

Resolves antrea-io#1463. Signed-off-by: Weiqiang Tang <[email protected]>

weiqiangt force-pushed the antrea-proxy-nodeport-dup branch from 2342515 to 0b6dcd4 Compare February 24, 2021 07:59

jianjuns reviewed Mar 15, 2021

View reviewed changes

antoninbas modified the milestones: Antrea v1.0 release, Antrea v1.1 release Apr 7, 2021

antoninbas removed this from the Antrea v1.1 release milestone May 21, 2021

hongliangl closed this May 26, 2021

Add NodePort support for Antrea Proxy on Linux #1471

Add NodePort support for Antrea Proxy on Linux #1471

Conversation

weiqiangt commented Nov 2, 2020 • edited Loading

antrea-bot commented Nov 2, 2020

codecov-io commented Nov 2, 2020 • edited Loading

Codecov Report

weiqiangt commented Nov 2, 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

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

weiqiangt commented Jan 27, 2021

weiqiangt commented Jan 27, 2021

weiqiangt commented Feb 2, 2021

weiqiangt commented Feb 2, 2021

weiqiangt commented Feb 3, 2021

weiqiangt commented Feb 3, 2021

weiqiangt commented Feb 3, 2021 • edited Loading

jianjuns commented Feb 3, 2021 • edited Loading

weiqiangt commented Feb 4, 2021

weiqiangt commented Feb 4, 2021

weiqiangt commented Feb 4, 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

weiqiangt commented Nov 2, 2020 •

edited

Loading

codecov-io commented Nov 2, 2020 •

edited

Loading

weiqiangt commented Feb 3, 2021 •

edited

Loading

jianjuns commented Feb 3, 2021 •

edited

Loading