Promote feature gate AntreaProxy to GA #5401

hongliangl · 2023-08-17T02:18:44Z

No description provided.

luolanzone

A few nits, you need to update the Introduction section in the docs/antrea-proxy.md.

cmd/antrea-agent/options.go

pkg/agent/controller/networkpolicy/networkpolicy_controller.go

test/e2e/fixtures.go

luolanzone · 2023-08-17T03:17:14Z

You also need to update the doc docs/feature-gates.md about AntreaProxy feature gate.

build/charts/antrea/values.yaml

pkg/config/agent/config.go

antoninbas · 2023-08-17T21:30:10Z

pkg/apiserver/handlers/featuregates/handler_test.go

@@ -52,7 +52,7 @@ func Test_getGatesResponse(t *testing.T) {
 			},
 			want: []Response{
 				{Component: "agent", Name: "AntreaPolicy", Status: "Disabled", Version: "BETA"},
-				{Component: "agent", Name: "AntreaProxy", Status: "Enabled", Version: "BETA"},
+				{Component: "agent", Name: "AntreaProxy", Status: "Enabled"},


why aren't we using Version: "GA" here?

In "k8s.io/component-base/featuregate", we can see that GA is empty string:

const ( // Values for PreRelease. Alpha = prerelease("ALPHA") Beta = prerelease("BETA") GA = prerelease("") // Deprecated Deprecated = prerelease("DEPRECATED") )

I have added an if condition to handle this. If we get an empty string, we use "GA" to replace it.

build/yamls/antrea-windows-containerd-with-ovs.yml

docs/feature-gates.md

pkg/agent/controller/networkpolicy/networkpolicy_controller.go

luolanzone · 2023-08-23T06:18:13Z

@hongliangl please resolve the code conflicts. Thanks.

hongliangl · 2023-08-23T07:50:10Z

@hongliangl please resolve the code conflicts. Thanks.

Done

luolanzone

A few nits, and please resolve the manifest conflicts.

docs/feature-gates.md

pkg/agent/controller/networkpolicy/networkpolicy_controller.go

cmd/antrea-agent/agent.go

cmd/antrea-agent/options.go

tnqn · 2023-08-29T04:35:33Z

docs/feature-gates.md

@@ -372,7 +367,7 @@ Refer to this [document](external-node.md) for more information.
 #### Requirements for this Feature

 Since Antrea Agent is running on an unmanaged VM/BM when this feature is enabled, features designed for K8s Pods are
-disabled. As of now, this feature requires that `AntreaProxy` and `NetworkPolicyStats` are also enabled.
+disabled. As of now, this feature requires that AntreaProxy and feature `NetworkPolicyStats` are also enabled.


Why would ExternalNode require AntreaProxy, I think this was a typo, which meant to say AntreaPolicy? @wenyingd

pkg/agent/controller/networkpolicy/networkpolicy_controller.go

pkg/agent/controller/traceflow/traceflow_controller.go

build/charts/antrea/conf/antrea-agent.conf

cmd/antrea-agent/agent.go

luolanzone · 2023-09-18T02:03:21Z

docs/feature-gates.md

@@ -89,8 +87,7 @@ for more information about EndpointSlice. If this feature is enabled but the End

 #### Requirements for this Feature

- EndpointSlice v1 API is available (Kubernetes version >=1.21).
- `AntreaProxy` is enabled.
+EndpointSlice v1 API is available (Kubernetes version >=1.21).


I suppose if use set the option antreaProxy.enable to false, this feature still won't work? If so, we should add another line like "antreaProxy.enable is true"?
@tnqn any suggestion here?

I think we can keep a mention to AntreaProxy being enabled.

luolanzone · 2023-09-18T02:03:28Z

docs/feature-gates.md

@@ -100,8 +97,7 @@ for more information about TopologyAwareHints.

 #### Requirements for this Feature

- `AntreaProxy` is enabled.
- `EndpointSlice` is enabled.
+Feature `EndpointSlice` is enabled.


Any context for this ditto?

I mean the removed line "- AntreaProxy is enabled" , if it still requires antreaProxy.enable to true, we may need to highlight this.

For TopologyAwareHints, it still requires requires antreaProxy.enable to be true. I have added related text.

luolanzone · 2023-09-18T02:03:37Z

docs/feature-gates.md

@@ -115,7 +111,7 @@ antrea-proxy.md#configuring-load-balancer-mode-for-external-traffic) for more in

 #### Requirements for this Feature

- `AntreaProxy` with `proxyAll` is enabled.
+- `proxyAll` in AntreaProxy is enabled.


docs/feature-gates.md

luolanzone · 2023-09-18T02:12:47Z

pkg/controller/networkpolicy/validate.go

@@ -680,9 +680,6 @@ func (v *antreaPolicyValidator) validatePeers(ingress, egress []crdv1beta1.Rule)
 	}
 	for _, rule := range egress {
 		if rule.ToServices != nil {
-			if !features.DefaultFeatureGate.Enabled(features.AntreaProxy) {


No need to check antreaProxy.enable's value here?

It will be checked on agent.

There seems no check added in agent. I think we should fail the realization of a rule if it contains toServices but does't have AntreaProxy enabled. Otherwise there would be no issue when creating the rule but it would just not work without any error in API and log.

#5401 (comment) I'll make another PR about this after merging this PR.

Please create an issue in case it's forgotten.

Added in #5591

tnqn

LGTM overall

tnqn · 2023-10-17T10:43:08Z

cmd/antrea-agent/options.go

@@ -218,7 +216,11 @@ func (o *Options) validateTLSOptions() error {

 func (o *Options) validateAntreaProxyConfig(encapMode config.TrafficEncapModeType) error {
 	if !features.DefaultFeatureGate.Enabled(features.AntreaProxy) {
-		// Validate service CIDR configuration if AntreaProxy is not enabled.
+		o.config.AntreaProxy.Enable = pointer.Bool(false)


I feel we should do the latter. Mutating the config data directly may cause confusion when it's later logged or when the whole config is dumpped. At least we haven’t mutated config data (except defaulting) so far AFAIK.

tnqn · 2023-10-17T10:52:45Z

pkg/controller/networkpolicy/validate.go

@@ -680,9 +680,6 @@ func (v *antreaPolicyValidator) validatePeers(ingress, egress []crdv1beta1.Rule)
 	}
 	for _, rule := range egress {
 		if rule.ToServices != nil {
-			if !features.DefaultFeatureGate.Enabled(features.AntreaProxy) {


Please create an issue in case it's forgotten.

tnqn · 2023-10-23T09:34:04Z

cmd/antrea-agent/options.go

+	if !features.DefaultFeatureGate.Enabled(features.AntreaProxy) {
+		o.enableAntreProxy = false
+		klog.InfoS("Feature gate `AntreaProxy` is deprecated, please use option `antreaProxy.enable` to disable AntreaProxy")
+	} else {
+		o.enableAntreProxy = true
+	}


I think things are getting more complicated with these code.
We should:

Let defaulting func only do defaulting

if o.config.AntreaProxy.Enable == nil { o.config.AntreaProxy.Enable = pointer.Bool(true) } if o.config.AntreaProxy.ProxyLoadBalancerIPs == nil { o.config.AntreaProxy.ProxyLoadBalancerIPs = pointer.Bool(true) } if o.config.AntreaProxy.DefaultLoadBalancerMode == "" { o.config.AntreaProxy.DefaultLoadBalancerMode = config.LoadBalancerModeNAT.String() }

Let validating func do validating, calculate o.enableAntreProxy

if !features.DefaultFeatureGate.Enabled(features.AntreaProxy) { klog.InfoS("Feature gate `AntreaProxy` is deprecated, please use option `antreaProxy.enable` to disable AntreaProxy") } o.enableAntreaProxy = *o.config.AntreaProxy.Enable && !features.DefaultFeatureGate.Enabled(features.AntreaProxy) if o.enableAntreaProxy { ... } else { ... }

Let run func consume only o.enableAntreProxy, do not calculate whether it's enabled again.

cmd/antrea-agent/options.go

antoninbas

one more question, otherwise LGTM

antoninbas · 2023-10-23T19:10:25Z

cmd/antrea-agent/options.go

+	if o.config.AntreaProxy.Enable == nil {
+		o.config.AntreaProxy.Enable = pointer.Bool(true)
+	}


Do we need these 3 lines in setExternalNodeDefaultOptions as well?
It may be needed to avoid a crash if someone uses an old version of the config. I think we did the same in the past with enablePrometheusMetrics.

It was because EnablePrometheusMetrics was accessed directly in run. In AntreaProxy case, we have enableAntreaProxy and use it in run, so it should be fine to leave it nil in ExternalNode case.

~~And we don't need to default it to true?~~

There's probably no AnteraProxy for ExternalNode :)

It won't be read in ExternalNode case as validateK8sNodeOptions won't even be executed in that case. This implies a question: whether defaulting should be done in a modeless way or on demand. I think currently it's the latter. However, I also wondered if the former is better as it could ensure every config gets a value, either from user config or the default one.

antoninbas · 2023-10-23T19:12:03Z

cmd/antrea-agent/options.go

+	if o.config.AntreaProxy.ProxyLoadBalancerIPs == nil {
+		o.config.AntreaProxy.ProxyLoadBalancerIPs = pointer.Bool(true)
+	}
+	if o.config.ServiceCIDR == "" { // It's okay to set the default value of this field when AntreaProxy is enabled.


Suggested change

if o.config.ServiceCIDR == "" { // It's okay to set the default value of this field when AntreaProxy is enabled.

if o.config.ServiceCIDR == "" { // It's okay to set the default value of this field even when AntreaProxy is enabled and the field is not used.

tnqn

LGTM, a nit

tnqn · 2023-10-24T02:09:46Z

cmd/antrea-agent/agent.go

@@ -142,6 +142,7 @@ func run(o *Options) error {
 	enableMulticlusterGW := features.DefaultFeatureGate.Enabled(features.Multicluster) && o.config.Multicluster.EnableGateway
 	enableMulticlusterNP := features.DefaultFeatureGate.Enabled(features.Multicluster) && o.config.Multicluster.EnableStretchedNetworkPolicy
 	enableFlowExporter := features.DefaultFeatureGate.Enabled(features.FlowExporter) && o.config.FlowExporter.Enable
+	enableAntreaProxy := o.enableAntreaProxy


this is a bit verbose, the code could use o.enableAntreaProxy directly.

tnqn · 2023-10-24T02:18:42Z

cmd/antrea-agent/options.go

+	if o.config.AntreaProxy.Enable == nil {
+		o.config.AntreaProxy.Enable = pointer.Bool(true)
+	}


It was because EnablePrometheusMetrics was accessed directly in run. In AntreaProxy case, we have enableAntreaProxy and use it in run, so it should be fine to leave it nil in ExternalNode case.

Signed-off-by: Hongliang Liu <lhongliang@vmware.com>

tnqn

LGTM

tnqn · 2023-10-24T09:37:30Z

/test-all

antoninbas · 2023-10-24T18:18:10Z

/test-conformance

hongliangl force-pushed the 20230816-upgrade-proxy-2 branch from 6749715 to 8ccee5f Compare August 17, 2023 02:24

hongliangl requested review from tnqn and luolanzone August 17, 2023 02:36

luolanzone reviewed Aug 17, 2023

View reviewed changes

cmd/antrea-agent/options.go Outdated Show resolved Hide resolved

pkg/agent/controller/networkpolicy/networkpolicy_controller.go Outdated Show resolved Hide resolved

test/e2e/fixtures.go Outdated Show resolved Hide resolved

hongliangl force-pushed the 20230816-upgrade-proxy-2 branch 2 times, most recently from f85650a to e49e22b Compare August 17, 2023 06:24

hongliangl requested a review from luolanzone August 17, 2023 06:25

antoninbas reviewed Aug 17, 2023

View reviewed changes

hongliangl force-pushed the 20230816-upgrade-proxy-2 branch 6 times, most recently from cb6e959 to dcef48e Compare August 18, 2023 03:22

hongliangl added the action/release-note Indicates a PR that should be included in release notes. label Aug 18, 2023

hongliangl force-pushed the 20230816-upgrade-proxy-2 branch from dcef48e to b2dc9aa Compare August 23, 2023 07:48

hongliangl requested a review from antoninbas August 23, 2023 07:49

luolanzone mentioned this pull request Aug 24, 2023

[Antrea 2.0] Promote APIs to appropriate versions and feature gates to appropriate stages #5068

Closed

27 tasks

luolanzone reviewed Aug 29, 2023

View reviewed changes

hongliangl force-pushed the 20230816-upgrade-proxy-2 branch from b2dc9aa to 00c7f01 Compare August 29, 2023 02:24

tnqn reviewed Aug 29, 2023

View reviewed changes

hongliangl force-pushed the 20230816-upgrade-proxy-2 branch 3 times, most recently from cc91eb2 to 574c22f Compare September 1, 2023 02:53

luolanzone reviewed Sep 18, 2023

View reviewed changes

hongliangl force-pushed the 20230816-upgrade-proxy-2 branch from 574c22f to f7d1013 Compare September 18, 2023 02:24

luolanzone added this to the Antrea v1.14 release milestone Sep 18, 2023

tnqn reviewed Oct 17, 2023

View reviewed changes

hongliangl dismissed luolanzone’s stale review via 5f5335b October 17, 2023 16:33

hongliangl force-pushed the 20230816-upgrade-proxy-2 branch from 07da21b to 5f5335b Compare October 17, 2023 16:33

hongliangl mentioned this pull request Oct 17, 2023

Report NetworkPolicy destined for Services as invalid when proxyAll is disabled #5591

Closed

hongliangl requested a review from tnqn October 18, 2023 06:30

antoninbas mentioned this pull request Oct 20, 2023

Promote feature gate NodePortLocal to GA #5491

Merged

hongliangl force-pushed the 20230816-upgrade-proxy-2 branch from 5f5335b to b7e0d9e Compare October 21, 2023 02:40

hongliangl requested a review from antoninbas October 21, 2023 02:41

hongliangl force-pushed the 20230816-upgrade-proxy-2 branch 3 times, most recently from d033c73 to 4bab10f Compare October 21, 2023 06:39

tnqn reviewed Oct 23, 2023

View reviewed changes

hongliangl force-pushed the 20230816-upgrade-proxy-2 branch 7 times, most recently from c53d9fa to d79e7b6 Compare October 23, 2023 13:40

antoninbas reviewed Oct 23, 2023

View reviewed changes

tnqn reviewed Oct 24, 2023

View reviewed changes

hongliangl force-pushed the 20230816-upgrade-proxy-2 branch from d79e7b6 to 88618b8 Compare October 24, 2023 04:51

Promote feature gate AntreaProxy to GA

1aa4b49

Signed-off-by: Hongliang Liu <lhongliang@vmware.com>

hongliangl force-pushed the 20230816-upgrade-proxy-2 branch from 88618b8 to 1aa4b49 Compare October 24, 2023 06:25

tnqn approved these changes Oct 24, 2023

View reviewed changes

antoninbas merged commit ebd391e into antrea-io:main Oct 24, 2023
46 of 56 checks passed

hongliangl deleted the 20230816-upgrade-proxy-2 branch October 25, 2023 00:01

tnqn mentioned this pull request Oct 26, 2023

Fix windows deployment manifest #5617

Merged

	if o.config.ServiceCIDR == "" { // It's okay to set the default value of this field when AntreaProxy is enabled.
	if o.config.ServiceCIDR == "" { // It's okay to set the default value of this field even when AntreaProxy is enabled and the field is not used.

Promote feature gate AntreaProxy to GA #5401

Promote feature gate AntreaProxy to GA #5401

Conversation

hongliangl commented Aug 17, 2023

luolanzone left a comment

Choose a reason for hiding this comment

luolanzone commented Aug 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luolanzone commented Aug 23, 2023

hongliangl commented Aug 23, 2023

luolanzone 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

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

antoninbas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoninbas Oct 24, 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 left a comment

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

tnqn commented Oct 24, 2023

antoninbas commented Oct 24, 2023

antoninbas Oct 24, 2023 •

edited

Loading