Store cluster's Service CIDR to Multi-cluster Gateway's fields #4737

hjiajing · 2023-03-22T07:24:55Z

Add a new field named serviceCIDR to store the local cluster Service CIDR in Multi-cluster Gateway. So that the antrea-agent could get the Service CIDR from a Gateway, which makes the Service CIDR in antrea-agent and antrea-mc-controller consistent.

multicluster/apis/multicluster/v1alpha1/gateway_types.go

luolanzone · 2023-03-22T08:11:15Z

multicluster/cmd/multicluster-controller/gateway_webhook.go

+		oldGateway := gatewayList.Items[0]
+		if oldGateway.ServiceCIDR != gateway.ServiceCIDR {


This is not correct, you should compare the req.OldObject and req.Object's ServiceCIDR instead of comparing gatewayList.Items[0]'s CIDR with current Gateway's.

@luolanzone : I do not understand why we need these checks if Gateway is created by MC Controller. The webhook is generally for validating user intents?

If the goal is to prevent users from modifying the object, we should just check that, and ideally override user changes at controller restart.

Yeah, I think the purpose is to prevent users to modify the field. @jianjuns do you mean not to add validation on webhook but check the field and override the change in node_controller?

Ideally MC Controller should check and override changes. But first it makes no sense to validate particular fields here. If we want to prevent modification from users, can we just check the identity?

Good point, @hjiajing could you refine this part? Thanks.

Sure, I will refine this part.

Now the controller will check the SA if the verb is "update"

multicluster/controllers/multicluster/member/node_controller.go

multicluster/controllers/multicluster/member/gateway_controller.go

luolanzone · 2023-03-22T09:03:33Z

@hjiajing please add commit message to your commit to clarify the change.

hjiajing · 2023-03-22T09:08:13Z

@luolanzone Added, thanks for the reminder.

jianjuns · 2023-03-27T18:02:49Z

multicluster/cmd/multicluster-controller/gateway_webhook.go

+			klog.ErrorS(err, "Error getting ServiceAccount name", "Gateway", req.Namespace+"/"+req.Name)
+			return admission.Errored(http.StatusBadRequest, err)
+		}
+		if saName != mcControllerSAName && isServiceCIDRChanged(oldGateway, gateway) {


Again, no need to check individual fields. Just check only MC Controller can update the Gateway spec.

Done. Removed the field check.

jianjuns

I probably approved the PR by mistake. Please ignore that.

jianjuns · 2023-03-27T18:16:09Z

multicluster/controllers/multicluster/member/node_controller.go

@@ -116,12 +119,16 @@ func (r *NodeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.
 	var isValidGateway bool

 	if stillGatewayNode {
+		gw.ServiceCIDR, err = r.getServiceCIDR()


I think we should discover ServiceCIDR in NewNodeReconciler() or maybe SetupWithManager(). @luolanzone?

A NodeReconciler without ServiceCIDR is invalid, it does not make sense to reconcile gateways, so I moved the getServiceCIDR step to SetupWithManager func.

multicluster/controllers/multicluster/member/gateway_controller.go

luolanzone · 2023-03-28T02:47:34Z

multicluster/cmd/multicluster-controller/gateway_webhook.go

+			return admission.Errored(http.StatusBadRequest, err)
+		}
+		if saName != mcControllerSAName {
+			return admission.Errored(http.StatusPreconditionFailed, fmt.Errorf("ServiceCIDR can only be updated by Antrea Multi-cluster Controller"))


You are not comparing ServiceCIDR, so better to say Gateway can only be updated by Antrea Multi-cluster Controller

Fixed. Removed the ServiceAccount to the above of the Operation check. Now only SA is "antrea-mc-controller" could create or update Gateway.

multicluster/controllers/multicluster/member/node_controller.go

jianjuns · 2023-03-29T00:21:19Z

multicluster/cmd/multicluster-controller/gateway_webhook.go

+		klog.ErrorS(err, "Error getting ServiceAccount name", "Gateway", req.Namespace+"/"+req.Name)
+		return admission.Errored(http.StatusBadRequest, err)
+	}
+	if saName != mcControllerSAName {


We should at least allow other users to get Gateway right?

We defined a Gateway webhook to validate the operation for CREATE/UPDATE, it has no impact to GET.

An operation check is added above the SA check. Now only "antrea-mc-controller" could update or create Gateways, but other users could also get Gateways. Two more unit test cases were added.

jianjuns · 2023-03-29T00:22:38Z

multicluster/cmd/multicluster-controller/gateway_webhook.go

+	}
+	if saName != mcControllerSAName {
+		return admission.Errored(http.StatusPreconditionFailed, fmt.Errorf("Gateway can only be created or updated by Antrea Multi-cluster Controller"))
+	}
 	// Check if there is any existing Gateway.
 	gatewayList := &mcv1alpha1.GatewayList{}


If we check only MC Controller can create Gateways, even this check is not necessary? @luolanzone

Yeah, I feel we can remove this.

The "unique Gateway" check is removed, as well as the unit test.

luolanzone

LGTM overall, one nit.

multicluster/cmd/multicluster-controller/gateway_webhook.go

In order to keep Service CIDR in antrea-mc-controller and antrea-agent consistent, the node controller will store the Service CIDR to Gateway's field. When the Service CIDR is used in antrea-agent, controllers could get the Service CIDR in Multi-cluster Gateway. Signed-off-by: hjiajing <hjiajing@vmware.com>

jianjuns · 2023-03-30T23:10:09Z

/test-multicluster-e2e
/skip-all

In order to keep Service CIDR in antrea-mc-controller and antrea-agent consistent, the node controller will store the Service CIDR to Gateway's field. When the Service CIDR is used in antrea-agent, controllers could get the Service CIDR in Multi-cluster Gateway. Signed-off-by: hjiajing <hjiajing@vmware.com>

hjiajing force-pushed the gateway-serviceCIDR branch from a30d513 to f19be84 Compare March 22, 2023 07:25

hjiajing requested review from luolanzone and jianjuns March 22, 2023 07:36

luolanzone added the area/multi-cluster Issues or PRs related to multi cluster. label Mar 22, 2023

luolanzone added this to the Antrea v1.12 release milestone Mar 22, 2023

hjiajing force-pushed the gateway-serviceCIDR branch from f19be84 to d6352b0 Compare March 22, 2023 08:06

luolanzone reviewed Mar 22, 2023

View reviewed changes

hjiajing force-pushed the gateway-serviceCIDR branch from d6352b0 to f343578 Compare March 22, 2023 09:06

hjiajing force-pushed the gateway-serviceCIDR branch 2 times, most recently from 9152047 to 999a16d Compare March 26, 2023 23:23

jianjuns previously approved these changes Mar 27, 2023

View reviewed changes

jianjuns reviewed Mar 27, 2023

View reviewed changes

hjiajing dismissed jianjuns’s stale review via bc9a4bf March 28, 2023 00:48

hjiajing force-pushed the gateway-serviceCIDR branch 4 times, most recently from 1d39bac to f55c326 Compare March 28, 2023 02:27

luolanzone reviewed Mar 28, 2023

View reviewed changes

hjiajing force-pushed the gateway-serviceCIDR branch 2 times, most recently from 77356b3 to 92359f2 Compare March 28, 2023 05:31

jianjuns reviewed Mar 29, 2023

View reviewed changes

hjiajing force-pushed the gateway-serviceCIDR branch 2 times, most recently from 533934c to 0a044ac Compare March 29, 2023 05:15

luolanzone reviewed Mar 29, 2023

View reviewed changes

multicluster/cmd/multicluster-controller/gateway_webhook.go Outdated Show resolved Hide resolved

hjiajing force-pushed the gateway-serviceCIDR branch from 0a044ac to 3ea1da6 Compare March 29, 2023 07:50

jianjuns approved these changes Mar 30, 2023

View reviewed changes

jianjuns merged commit d7ffc85 into antrea-io:main Mar 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store cluster's Service CIDR to Multi-cluster Gateway's fields #4737

Store cluster's Service CIDR to Multi-cluster Gateway's fields #4737

hjiajing commented Mar 22, 2023

luolanzone Mar 22, 2023

jianjuns Mar 23, 2023

luolanzone Mar 23, 2023

jianjuns Mar 23, 2023

luolanzone Mar 23, 2023

hjiajing Mar 24, 2023

hjiajing Mar 27, 2023

luolanzone commented Mar 22, 2023

hjiajing commented Mar 22, 2023

jianjuns Mar 27, 2023 •

edited

Loading

hjiajing Mar 28, 2023

jianjuns left a comment

jianjuns Mar 27, 2023

hjiajing Mar 28, 2023 •

edited

Loading

luolanzone Mar 28, 2023

hjiajing Mar 28, 2023

jianjuns Mar 29, 2023

luolanzone Mar 29, 2023

hjiajing Mar 29, 2023

jianjuns Mar 29, 2023

luolanzone Mar 29, 2023

hjiajing Mar 29, 2023

luolanzone left a comment

jianjuns commented Mar 30, 2023

		oldGateway := gatewayList.Items[0]
		if oldGateway.ServiceCIDR != gateway.ServiceCIDR {

Store cluster's Service CIDR to Multi-cluster Gateway's fields #4737

Store cluster's Service CIDR to Multi-cluster Gateway's fields #4737

Conversation

hjiajing commented Mar 22, 2023

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

luolanzone commented Mar 22, 2023

hjiajing commented Mar 22, 2023

jianjuns Mar 27, 2023 • edited Loading

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

hjiajing Mar 28, 2023 • edited Loading

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

luolanzone left a comment

Choose a reason for hiding this comment

jianjuns commented Mar 30, 2023

jianjuns Mar 27, 2023 •

edited

Loading

hjiajing Mar 28, 2023 •

edited

Loading