Support ProxyTerminatingEndpoints in AntreaProxy #4607

hongliangl · 2023-02-07T02:44:17Z

Signed-off-by: Hongliang Liu [email protected]

codecov · 2023-02-07T03:09:22Z

Codecov Report

Merging #4607 (8c2d9a3) into main (cf90cfa) will decrease coverage by 0.01%.
The diff coverage is 96.15%.

@@            Coverage Diff             @@
##             main    #4607      +/-   ##
==========================================
- Coverage   69.93%   69.92%   -0.01%     
==========================================
  Files         400      403       +3     
  Lines       59457    60259     +802     
==========================================
+ Hits        41582    42137     +555     
- Misses      15077    15303     +226     
- Partials     2798     2819      +21

Flag	Coverage Δ		*Carryforward flag
e2e-tests	`38.29% <ø> (-0.05%)`	⬇️	Carriedforward from 361b805
integration-tests	`34.38% <0.00%> (-0.20%)`	⬇️	Carriedforward from 361b805
kind-e2e-tests	`47.69% <80.12%> (+0.59%)`	⬆️
unit-tests	`59.84% <95.94%> (+0.06%)`	⬆️	Carriedforward from 361b805

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files	Coverage Δ
pkg/features/antrea_features.go	`100.00% <ø> (ø)`
pkg/agent/proxy/proxier.go	`76.48% <91.30%> (+0.40%)`	⬆️
pkg/agent/proxy/topology.go	`99.04% <100.00%> (+17.22%)`	⬆️
...nt/apiserver/handlers/serviceexternalip/handler.go	`29.62% <0.00%> (-22.23%)`	⬇️
pkg/agent/cniserver/ipam/antrea_ipam.go	`63.02% <0.00%> (-12.73%)`	⬇️
pkg/agent/route/route_linux.go	`66.19% <0.00%> (-5.47%)`	⬇️
pkg/controller/networkpolicy/tier.go	`53.84% <0.00%> (-4.62%)`	⬇️
pkg/controller/ipam/antrea_ipam_controller.go	`75.25% <0.00%> (-3.02%)`	⬇️
...gent/controller/networkpolicy/status_controller.go	`79.16% <0.00%> (-2.50%)`	⬇️
.../flowexporter/connections/conntrack_connections.go	`81.42% <0.00%> (-2.39%)`	⬇️
... and 32 more

docs/feature-gates.md

jianjuns

LGTM

jianjuns

A few nits.

pkg/agent/proxy/proxier.go

pkg/agent/proxy/topology.go

hongliangl · 2023-02-24T01:19:45Z

/test-all

hongliangl · 2023-02-24T02:06:17Z

/test-all

tnqn

LGTM overall

tnqn · 2023-02-24T09:47:03Z

pkg/agent/proxy/proxier.go

+					if internalPolicyLocal {
+						endpoints = localEndpoints
+					} else {
+						endpoints = clusterEndpoints
+					}


Isn't endpoints just allReachableEndpoints?

ClusterEndpoints may contain remote Endpoints that aren't in localEndpoints, while localEndpoints may contain
terminating or topologically-unavailable local endpoints that aren't in clusterEndpoints. So we have to merge
the two lists, that's allReachableEndpoints.

I don't get how it's related to this code, and

When the Service is ClusterIP, if the internal policy is local, why allReachableEndpoints ever contains non local endpoints? if the internal policy is Cluster, why allReachableEndpoints doesn't contain all cluster endpoints?

If allReachableEndpoints is not the parameter of the single InstallServiceGroup call, why we install use it as the parameter of InstallEndpointFlows? If there are different endpoints, what are they installed for?

tnqn · 2023-02-24T09:56:19Z

pkg/agent/proxy/proxier.go

+				if bothPolicyLocal {
+					endpoints = localEndpoints
+				} else {
+					endpoints = clusterEndpoints
+				}


tnqn · 2023-02-24T12:03:58Z

docs/feature-gates.md

+
+### ProxyTerminatingEndpoints
+
+`ProxyTerminatingEndpoints` enables ProxyTerminatingEndpoints support in AntreaProxy. When ProxyTerminatingEndpoints is


Question: if the support is controlled by feature gate and disabled by default, could Antrea pass latest K8s conformance test? I remember you mentioned latest conformance requires some EndpointSlice related feature to be supported.

I'll verify that.

I have verified that with Zhengsheng:

It is not conformance test, it is sig-network test.

For sig-network test in Kubernetes 1.26, EndpointSliceTerminatingCondition is GA, and terminating Endpoints will be included in EndpointSlice anyway, then we need to filter out terminating Endpoints when ProxyTerminatingEndpoints is not enabled. In current code, we don't have such code to filter out terminating Endpoints when EndpointSlice is enabled.

Then if we merge it as is, the test case won't pass unless the ProxyTerminatingEndpoints feature is enabled?
If so, I feel not really necessary to add this feature gate, the code isolated by the feature gate is very few and no real risk, and proxy terminating endpoints has been proved an acceptable behavior in kube-proxy.

cc @jianjuns for input

If we want to pass corresponding sig-network tests about ProxyTerminatingEndpoints, we need to enable both this feature and proxyAll.

Why? If proxyAll is not running, kube-proxy must be running, right? How could kube-proxy fail the test?

I think we might need a feature gate since like TopologyAwareHints in AntreaProxy has a feature gateway, ProxyTerminatingEndpoints should be consistent with that.

I don't feel such consistency is meaningful. We do have many small features that are added directly, like you did in #2792. The rule to add a feature gate should be related to the code/feature's risk and maturity, instead of whether there is one in K8s.

Why? If proxyAll is not running, kube-proxy must be running, right? How could kube-proxy fail the test?

Sorry, I missed something: as you said, the tests will always pass with kube-proxy. The code of ProxyTerminatingEndpoints in AntreaProxy takes effect when proxyAll is enabled and no kube-proxy.

I don't feel such consistency is meaningful. We do have many small features that are added directly, like you did in #2792. The rule to add a feature gate should be related to the code/feature's risk and maturity, instead of whether there is one in K8s.

If so, do you think we could also remove the feature gate TopologyAwareHints in Antrea?

Then there should be no problem if we just remove ProxyTerminatingEndpoints featuregate regardless of the configuration?

I didn't evaluate the risk of TopologyAwareHints but at least it's not worth to change what has been added, which would just cause more confusion. We should consider promoting it to Beta and GA instead according to it's maturity.

I don't think we have anymore problem if we just remove ProxyTerminatingEndpoints. Then with proxyAll enabled and without kube-proxy, AntreaProxy can pass the follow sig-network test cases:

[It] [sig-network] Services should fallback to terminating endpoints when there are no ready endpoints with internalTrafficPolicy=Cluster [Feature:ProxyTerminatingEndpoints] [It] [sig-network] Services should fallback to local terminating endpoints when there are no ready endpoints with internalTrafficPolicy=Local [Feature:ProxyTerminatingEndpoints] [It] [sig-network] Services should fallback to terminating endpoints when there are no ready endpoints with externallTrafficPolicy=Cluster [Feature:ProxyTerminatingEndpoints] [It] [sig-network] Services should fail health check node port if there are only terminating endpoints [Feature:ProxyTerminatingEndpoints]

The test that Antrea cannot pass is:

[It] [sig-network] Services should fallback to local terminating endpoints when there are no ready endpoints with externalTrafficPolicy=Local [Feature:ProxyTerminatingEndpoints]

The root cause is that AntreaProxy doesn't select any remote Endpoints as backend when externalTrafficPolicy is Local (in this test, no local Endpoint is available in a sub test), while kube-proxy could select remote Endpoints as backend when externalTrafficPolicy is Local and client is from in-cluster. I'll add another PR to support this later.

Removed the featuregate.

pkg/agent/proxy/topology.go

pkg/agent/proxy/topology_test.go

Signed-off-by: Hongliang Liu <[email protected]>

tnqn · 2023-03-02T10:31:51Z

/test-all

hongliangl · 2023-03-02T13:12:59Z

e2e failed on known flaky test.

tnqn · 2023-03-02T14:57:02Z

e2e failed on known flaky test.

No, that's a new error. I think it's introduced by #4654. The TrafficControl port was not deleted somehow, and antrea-agent failed to set no flood for it after restart.

I meant the one in https://github.com/antrea-io/antrea/actions/runs/4312092832/jobs/7522929072

tnqn · 2023-03-02T14:59:37Z

Anyway it's not related to this PR. /skip-e2e

Signed-off-by: Hongliang Liu <[email protected]>

hongliangl force-pushed the 20230118-endpointslice-enhance-v2 branch from 4d056d0 to 2182b34 Compare February 7, 2023 02:52

hongliangl marked this pull request as ready for review February 7, 2023 02:52

hongliangl requested review from tnqn and jianjuns February 7, 2023 02:52

hongliangl force-pushed the 20230118-endpointslice-enhance-v2 branch from 2182b34 to 9becf81 Compare February 8, 2023 03:11

hongliangl added area/proxy Issues or PRs related to proxy functions in Antrea action/release-note Indicates a PR that should be included in release notes. labels Feb 9, 2023

hongliangl added this to the Antrea v1.11 release milestone Feb 9, 2023

hongliangl force-pushed the 20230118-endpointslice-enhance-v2 branch from 9becf81 to a030f1c Compare February 17, 2023 00:24

jianjuns reviewed Feb 17, 2023

View reviewed changes

docs/feature-gates.md Outdated Show resolved Hide resolved

hongliangl force-pushed the 20230118-endpointslice-enhance-v2 branch from a030f1c to 0a54517 Compare February 20, 2023 07:38

jianjuns reviewed Feb 23, 2023

View reviewed changes

docs/feature-gates.md Outdated Show resolved Hide resolved

docs/feature-gates.md Outdated Show resolved Hide resolved

docs/feature-gates.md Outdated Show resolved Hide resolved

hongliangl force-pushed the 20230118-endpointslice-enhance-v2 branch 2 times, most recently from a565f69 to c9fe386 Compare February 23, 2023 03:04

hongliangl requested a review from jianjuns February 23, 2023 03:09

jianjuns reviewed Feb 23, 2023

View reviewed changes

pkg/agent/proxy/proxier.go Outdated Show resolved Hide resolved

pkg/agent/proxy/topology.go Outdated Show resolved Hide resolved

pkg/agent/proxy/topology.go Outdated Show resolved Hide resolved

pkg/agent/proxy/topology.go Outdated Show resolved Hide resolved

hongliangl force-pushed the 20230118-endpointslice-enhance-v2 branch 2 times, most recently from 9dca8c6 to 361b805 Compare February 24, 2023 00:28

hongliangl requested a review from jianjuns February 24, 2023 02:06

tnqn reviewed Feb 24, 2023

View reviewed changes

hongliangl mentioned this pull request Feb 27, 2023

Graduate feature EndpointSlice from Alpha to Beta #4634

Merged

hongliangl commented Feb 27, 2023

View reviewed changes

pkg/agent/proxy/topology.go Show resolved Hide resolved

hongliangl commented Feb 27, 2023

View reviewed changes

pkg/agent/proxy/topology_test.go Outdated Show resolved Hide resolved

Support ProxyTerminatingEndpoints in AntreaProxy

d4955ed

Signed-off-by: Hongliang Liu <[email protected]>

hongliangl force-pushed the 20230118-endpointslice-enhance-v2 branch from 8c2d9a3 to d4955ed Compare March 2, 2023 08:39

tnqn approved these changes Mar 2, 2023

View reviewed changes

tnqn merged commit 6339388 into antrea-io:main Mar 2, 2023

hongliangl deleted the 20230118-endpointslice-enhance-v2 branch March 3, 2023 01:54

hongliangl mentioned this pull request Mar 3, 2023

Fix that Node controller doesn't start for TopologyAwareHints in AntreaProxy #4660

Closed

jainpulkit22 pushed a commit to urharshitha/antrea that referenced this pull request Apr 28, 2023

Support ProxyTerminatingEndpoints in AntreaProxy (antrea-io#4607)

ec80001

Signed-off-by: Hongliang Liu <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support ProxyTerminatingEndpoints in AntreaProxy #4607

Support ProxyTerminatingEndpoints in AntreaProxy #4607

hongliangl commented Feb 7, 2023

codecov bot commented Feb 7, 2023 •

edited

Loading

jianjuns left a comment

jianjuns left a comment

hongliangl commented Feb 24, 2023

hongliangl commented Feb 24, 2023

tnqn left a comment

tnqn Feb 24, 2023

hongliangl Feb 24, 2023

tnqn Feb 27, 2023

tnqn Feb 24, 2023

tnqn Feb 24, 2023

hongliangl Feb 24, 2023

hongliangl Feb 27, 2023

tnqn Mar 2, 2023 •

edited

Loading

tnqn Mar 2, 2023

tnqn Mar 2, 2023

hongliangl Mar 2, 2023 •

edited

Loading

tnqn Mar 2, 2023 •

edited

Loading

hongliangl Mar 2, 2023

hongliangl Mar 2, 2023

tnqn commented Mar 2, 2023

hongliangl commented Mar 2, 2023

tnqn commented Mar 2, 2023 •

edited

Loading

tnqn commented Mar 2, 2023


		### ProxyTerminatingEndpoints

		`ProxyTerminatingEndpoints` enables ProxyTerminatingEndpoints support in AntreaProxy. When ProxyTerminatingEndpoints is

Support ProxyTerminatingEndpoints in AntreaProxy #4607

Support ProxyTerminatingEndpoints in AntreaProxy #4607

Conversation

hongliangl commented Feb 7, 2023

codecov bot commented Feb 7, 2023 • edited Loading

Codecov Report

jianjuns left a comment

Choose a reason for hiding this comment

jianjuns left a comment

Choose a reason for hiding this comment

hongliangl commented Feb 24, 2023

hongliangl commented Feb 24, 2023

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

tnqn Mar 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hongliangl Mar 2, 2023 • edited Loading

Choose a reason for hiding this comment

tnqn Mar 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn commented Mar 2, 2023

hongliangl commented Mar 2, 2023

tnqn commented Mar 2, 2023 • edited Loading

tnqn commented Mar 2, 2023

codecov bot commented Feb 7, 2023 •

edited

Loading

tnqn Mar 2, 2023 •

edited

Loading

hongliangl Mar 2, 2023 •

edited

Loading

tnqn Mar 2, 2023 •

edited

Loading

tnqn commented Mar 2, 2023 •

edited

Loading