Make externalIP proxying configurable for AntreaProxy #3130

antoninbas · 2021-12-13T21:59:21Z

A new proxyExternalIPs configuration parameter is added for
AntreaProxy. When set to false, ntreaProxy no longer load-balances
traffic destined to the ExternalIPs of LoadBalancer Services. This is
useful when the external LoadBalancer provides additional capabilities
(e.g. TLS termination) and it is desirable for Pod-to-ExternalIP traffic
to be sent to the external LoadBalancer instead of being load-balanced
to an Endpoint directly by AntreaProxy.

The default behavior is unchanged, i.e. proxyExternalIPs is true by
default.

Note that setting ProxyExternalIPs to false usually only makes sense
when ProxyAll is set to true and kube-proxy is removed from the cluser,
otherwise kube-proxy will still load-balance this traffic.

This feature cannot be leveraged by an external Load Balancer that
depends on the local proxy (AntreaProxy) being able to load balance
traffic destined to the ExternalIP. This is the case for example with
MetalLB. However Load Balancers that rely on NodePort / NodePortLocal
can leverage this feature.

Fixes #3121

Signed-off-by: Antonin Bas abas@vmware.com

codecov-commenter · 2021-12-13T22:16:38Z

Codecov Report

Merging #3130 (93cf9bd) into main (43d2ee9) will decrease coverage by 6.62%.
The diff coverage is 68.96%.

@@            Coverage Diff             @@
##             main    #3130      +/-   ##
==========================================
- Coverage   58.28%   51.65%   -6.63%     
==========================================
  Files         297      297              
  Lines       25308    25311       +3     
==========================================
- Hits        14750    13075    -1675     
- Misses       8965    10757    +1792     
+ Partials     1593     1479     -114

Flag	Coverage Δ
kind-e2e-tests	`30.82% <55.17%> (-13.46%)`	⬇️
unit-tests	`40.32% <62.06%> (+<0.01%)`	⬆️

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

Impacted Files	Coverage Δ
pkg/agent/proxy/proxier.go	`56.75% <68.96%> (-0.69%)`	⬇️
pkg/controller/types/networkpolicy.go	`0.00% <0.00%> (-100.00%)`	⬇️
pkg/agent/controller/networkpolicy/reject.go	`0.00% <0.00%> (-84.68%)`	⬇️
pkg/apis/controlplane/v1beta2/helper.go	`33.33% <0.00%> (-66.67%)`	⬇️
...formers/externalversions/security/v1alpha1/tier.go	`0.00% <0.00%> (-64.29%)`	⬇️
...ers/externalversions/core/v1alpha2/clustergroup.go	`0.00% <0.00%> (-64.29%)`	⬇️
...s/externalversions/core/v1alpha2/externalentity.go	`0.00% <0.00%> (-64.29%)`	⬇️
...xternalversions/security/v1alpha1/networkpolicy.go	`0.00% <0.00%> (-64.29%)`	⬇️
...versions/security/v1alpha1/clusternetworkpolicy.go	`0.00% <0.00%> (-64.29%)`	⬇️
pkg/controller/networkpolicy/mutate.go	`0.00% <0.00%> (-62.07%)`	⬇️
... and 85 more

jianjuns

LGTM.

In commit message, ntreaProxy -> AntreaProxy.

tnqn · 2021-12-14T02:51:57Z

pkg/agent/proxy/proxier.go

@@ -159,7 +160,7 @@ func (p *proxier) removeStaleServices() {
 			}
 		}
 		// Remove LoadBalancer flows and configurations.
-		if len(svcInfo.LoadBalancerIPStrings()) > 0 {
+		if p.proxyExternalIPs && len(svcInfo.LoadBalancerIPStrings()) > 0 {


I just found when we talked about externalIP, we actually referred to loadbalancerIPs, while there is another field in Service's Spec called ExternalIPs, which Antrea doesn't proxy at the moment but kube-proxy does. We may need to support it in the future. I don't think we need to have a toggle for it as the spec says "externalIPs is a list of IP addresses for which nodes in the cluster will also accept traffic for this service". But to avoid confusion, should we call the configuration for loadbalancer IPs proxyLoadBalancerIPs?

I actually opened an issue a while back about supporting ExternalIPs for ClusterIP Services: #1627

Originally I thought that the proxyExternalIPs configuration would eventually apply to both (LoadBalancerIPs and ExternalIPs) but thinking about it some more I don't think it makes sense. I'll work on renaming the configuration parameter as you suggested.

pkg/agent/proxy/proxier_test.go

hongliangl · 2021-12-14T03:24:49Z

pkg/config/agent/config.go

@@ -192,6 +192,14 @@ type AntreaProxyConfig struct {
 	// Services will not be load-balanced). Values can be a valid ClusterIP (e.g. 10.11.1.2) or a Service name
 	// with Namespace (e.g. kube-system/kube-dns)
 	SkipServices []string `yaml:"skipServices,omitempty"`
+	// When ProxyExternalIPs is set to false, AntreaProxy no longer load-balances traffic destined to the
+	// ExternalIPs of LoadBalancer Services. This is useful when the external LoadBalancer provides additional
+	// capabilities (e.g. TLS termination) and it is desirable for Pod-to-ExternalIP traffic to be sent to the


My concern is that: for externalTrafficPolicy of a LoadBalancer Service is Cluster, which means the source IP address of client should be reserved, for the connection whose source is a Pod, and destination is a ExternalIP(Pod-to-ExternalIP traffic mode). I'm a little confused about how the reply packets go back to external LB to guarantee the symmetry of for a connection.

Request packet path: Pod -> Antrea gateway -> external LB -> Antrea gateway -> Endpoint

Reply packet path: Endpoint -> Antrea gateway(without SNAT, I think the reply packet will be routed to the Pod since the source IP address of the client is reserved).

for externalTrafficPolicy of a LoadBalancer Service is Cluster, which means the source IP address of client should be reserved

Do you mean when externalTrafficPolicy is Local, in which case the IP is preserved?

The external Load Balancer must SNAT the traffic, which is typically the case or can be configured (for AVI for example it is the default), in order for the reply traffic to go back through the external LB. That applies regardless of the externalTrafficPolicy.

Probably @hongliangl means the case LB does not SNAT, e.g. LB is inline and embedded in the gateway? That is quite uncommon though.

The external Load Balancer must SNAT the traffic, which is typically the case or can be configured (for AVI for example it is the default), in order for the reply traffic to go back through the external LB. That applies regardless of the externalTrafficPolicy.

If SNAT is performed regardless of the externalTrafficPolicy, it makes sense.

tnqn

LGTM

A new proxyLoadBalancerIPs configuration parameter is added for AntreaProxy. When set to false, AntreaProxy no longer load-balances traffic destined to the External IPs of LoadBalancer Services. This is useful when the external LoadBalancer provides additional capabilities (e.g. TLS termination) and it is desirable for Pod-to-ExternalIP traffic to be sent to the external LoadBalancer instead of being load-balanced to an Endpoint directly by AntreaProxy. The default behavior is unchanged, i.e. proxyLoadBalancerIPs is true by default. Note that setting ProxyLoadBalancerIPs to false usually only makes sense when ProxyAll is set to true and kube-proxy is removed from the cluser, otherwise kube-proxy will still load-balance this traffic. This feature cannot be leveraged by an external Load Balancer that depends on the local proxy (AntreaProxy) being able to load balance traffic destined to the External IP. This is the case for example with MetalLB. However Load Balancers that rely on NodePort / NodePortLocal can leverage this feature. Fixes antrea-io#3121 Signed-off-by: Antonin Bas <abas@vmware.com>

tnqn

LGTM

tnqn · 2022-01-07T07:37:03Z

/test-all

tnqn · 2022-01-10T05:05:23Z

/skip-all
I have ran all required checks manually, "ACNPFQDNPolicy" and "TestFlowAggregator' failed but not related to this PR.

tnqn · 2022-01-10T05:13:55Z

/skip-all

antoninbas requested review from hongliangl, tnqn and jianjuns December 13, 2021 21:59

antoninbas added the area/proxy Issues or PRs related to proxy functions in Antrea label Dec 13, 2021

jianjuns reviewed Dec 14, 2021

View reviewed changes

tnqn reviewed Dec 14, 2021

View reviewed changes

hongliangl reviewed Dec 14, 2021

View reviewed changes

antoninbas force-pushed the make-externalIP-proxying-configurable-for-AntreaProxy branch from 476c37b to 596c088 Compare December 14, 2021 19:15

tnqn previously approved these changes Dec 16, 2021

View reviewed changes

antoninbas added this to the Antrea v1.5 release milestone Jan 6, 2022

antoninbas dismissed tnqn’s stale review via 93cf9bd January 6, 2022 18:46

antoninbas force-pushed the make-externalIP-proxying-configurable-for-AntreaProxy branch from 596c088 to 93cf9bd Compare January 6, 2022 18:46

tnqn approved these changes Jan 7, 2022

View reviewed changes

tnqn merged commit a59ed75 into antrea-io:main Jan 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make externalIP proxying configurable for AntreaProxy #3130

Make externalIP proxying configurable for AntreaProxy #3130

antoninbas commented Dec 13, 2021

codecov-commenter commented Dec 13, 2021 •

edited

Loading

jianjuns left a comment

tnqn Dec 14, 2021

antoninbas Dec 14, 2021

hongliangl Dec 14, 2021

antoninbas Dec 14, 2021

jianjuns Dec 14, 2021

hongliangl Dec 16, 2021

tnqn left a comment

tnqn left a comment

tnqn commented Jan 7, 2022

tnqn commented Jan 10, 2022

tnqn commented Jan 10, 2022

Make externalIP proxying configurable for AntreaProxy #3130

Make externalIP proxying configurable for AntreaProxy #3130

Conversation

antoninbas commented Dec 13, 2021

codecov-commenter commented Dec 13, 2021 • edited Loading

Codecov Report

jianjuns left a comment

Choose a reason for hiding this comment

tnqn Dec 14, 2021

Choose a reason for hiding this comment

antoninbas Dec 14, 2021

Choose a reason for hiding this comment

hongliangl Dec 14, 2021

Choose a reason for hiding this comment

antoninbas Dec 14, 2021

Choose a reason for hiding this comment

jianjuns Dec 14, 2021

Choose a reason for hiding this comment

hongliangl Dec 16, 2021

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Jan 7, 2022

tnqn commented Jan 10, 2022

tnqn commented Jan 10, 2022

codecov-commenter commented Dec 13, 2021 •

edited

Loading