Validate ServiceAccount for ClusterSet member list changes #4090

luolanzone · 2022-08-09T05:18:12Z

Since ClusterSet's member list is maintained by MemberClusterAnnounce
controller, mc-controller should deny ClusterSet member list updates by users
manually.

Signed-off-by: Lan Luo [email protected]

luolanzone · 2022-08-09T06:03:47Z

/test-multicluster-e2e

codecov · 2022-08-09T06:11:44Z

Codecov Report

Merging #4090 (967ee6a) into main (454df5d) will increase coverage by 0.86%.
The diff coverage is 52.94%.

@@            Coverage Diff             @@
##             main    #4090      +/-   ##
==========================================
+ Coverage   64.25%   65.12%   +0.86%     
==========================================
  Files         294      310      +16     
  Lines       44805    45555     +750     
==========================================
+ Hits        28789    29666     +877     
+ Misses      13687    13512     -175     
- Partials     2329     2377      +48

Flag	Coverage Δ
e2e-tests	`58.34% <0.00%> (?)`
integration-tests	`35.47% <ø> (?)`
kind-e2e-tests	`43.57% <ø> (-6.72%)`	⬇️
unit-tests	`44.26% <52.94%> (-0.07%)`	⬇️

Impacted Files	Coverage Δ
...luster-controller/memberclusterannounce_webhook.go	`63.33% <0.00%> (+8.33%)`	⬆️
.../cmd/multicluster-controller/clusterset_webhook.go	`60.41% <56.25%> (+1.04%)`	⬆️
pkg/agent/proxy/endpointslicecache.go	`0.00% <0.00%> (-83.60%)`	⬇️
pkg/agent/secondarynetwork/cnipodcache/cache.go	`0.00% <0.00%> (-77.56%)`	⬇️
pkg/controller/egress/store/egressgroup.go	`1.72% <0.00%> (-54.32%)`	⬇️
...g/agent/controller/serviceexternalip/controller.go	`35.83% <0.00%> (-45.74%)`	⬇️
pkg/agent/secondarynetwork/podwatch/controller.go	`0.00% <0.00%> (-39.02%)`	⬇️
.../agent/flowexporter/connections/conntrack_linux.go	`58.87% <0.00%> (-26.62%)`	⬇️
pkg/ovs/ovsctl/ovsctl_others.go	`53.84% <0.00%> (-25.65%)`	⬇️
...nt/apiserver/handlers/serviceexternalip/handler.go	`29.62% <0.00%> (-22.23%)`	⬇️
... and 109 more

jianjuns

Nits.

multicluster/cmd/multicluster-controller/clusterset_webhook.go

jianjuns · 2022-08-09T18:51:55Z

multicluster/cmd/multicluster-controller/clusterset_webhook.go

@@ -57,6 +63,22 @@ func (v *clusterSetValidator) Handle(ctx context.Context, req admission.Request)
 			klog.ErrorS(err, "the field 'clusterID' is immutable", "ClusterSet", klog.KObj(clusterSet))
 			return admission.Denied("the field 'clusterID' is immutable")
 		}
+
+		if isMemberListChanged(oldClusterSet.Spec.Members, clusterSet.Spec.Members) {


Do you think more efficient to check ServiceAccount name matches or not, before checking member list changes? Not sure.

I think other users may still update other parts of ClusterSet, eg: add a label or an annotation etc.

I meant if checking ServiceAccount name is faster than checking member list, maybe we should check SA name first, also assuming more updates should be by MC controller.

You mean we allow user to create/delete ClusterSet, but for update, we assume it will only be done by MC controller?

No, I meant if the current code is more efficient or not:

ui := req.UserInfo _, saName, err := serviceaccount.SplitUsername(ui.Username) if err != nil { klog.ErrorS(err, "Error getting ServiceAccount name", "ClusterSet", req.Namespace+"/"+req.Name) return admission.Errored(http.StatusBadRequest, err) } if saName != mcControllerSAName && isMemberListChanged(oldClusterSet.Spec.Members, clusterSet.Spec.Members) { return admission.Errored(http.StatusPreconditionFailed, fmt.Errorf("member list can only be updated by Antrea Multi-cluster Controller")) }

assuming most updates are by controller.

Refined to check service account first.

luolanzone · 2022-08-10T02:39:38Z

/test-multicluster-e2e

jianjuns · 2022-08-10T20:15:06Z

multicluster/cmd/multicluster-controller/clusterset_webhook.go

@@ -57,6 +63,22 @@ func (v *clusterSetValidator) Handle(ctx context.Context, req admission.Request)
 			klog.ErrorS(err, "the field 'clusterID' is immutable", "ClusterSet", klog.KObj(clusterSet))
 			return admission.Denied("the field 'clusterID' is immutable")
 		}
+
+		if isMemberListChanged(oldClusterSet.Spec.Members, clusterSet.Spec.Members) {


No, I meant if the current code is more efficient or not:

ui := req.UserInfo _, saName, err := serviceaccount.SplitUsername(ui.Username) if err != nil { klog.ErrorS(err, "Error getting ServiceAccount name", "ClusterSet", req.Namespace+"/"+req.Name) return admission.Errored(http.StatusBadRequest, err) } if saName != mcControllerSAName && isMemberListChanged(oldClusterSet.Spec.Members, clusterSet.Spec.Members) { return admission.Errored(http.StatusPreconditionFailed, fmt.Errorf("member list can only be updated by Antrea Multi-cluster Controller")) }

assuming most updates are by controller.

multicluster/cmd/multicluster-controller/clusterset_webhook.go

jianjuns · 2022-08-11T01:59:25Z

multicluster/cmd/multicluster-controller/clusterset_webhook.go

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


This block is useless and can be removed.

You are right, removed it.

Since ClusterSet's member list will be updated by MemberClusterAnnounce controller, mc controller should deny ClusterSet member list update by user manually to avoid overwrites. Signed-off-by: Lan Luo <[email protected]>

jianjuns · 2022-08-11T03:02:43Z

/test-multicluster-e2e
/test-integration

jianjuns · 2022-08-11T03:53:28Z

/skip-all

luolanzone added the area/multi-cluster Issues or PRs related to multi cluster. label Aug 9, 2022

luolanzone force-pushed the validate-serviceaccount branch from 085096e to 0936a90 Compare August 9, 2022 09:59

luolanzone requested review from jianjuns and hjiajing August 9, 2022 09:59

jianjuns changed the title ~~Validate ServiceAccount if member list changed~~ Validate ServiceAccount for ClusterSet member list changes Aug 9, 2022

jianjuns reviewed Aug 9, 2022

View reviewed changes

luolanzone force-pushed the validate-serviceaccount branch from 0936a90 to 17b408a Compare August 10, 2022 02:36

jianjuns reviewed Aug 10, 2022

View reviewed changes

luolanzone force-pushed the validate-serviceaccount branch from 17b408a to 92b890f Compare August 11, 2022 01:24

jianjuns reviewed Aug 11, 2022

View reviewed changes

Validate ServiceAccount if member list changed

967ee6a

Since ClusterSet's member list will be updated by MemberClusterAnnounce controller, mc controller should deny ClusterSet member list update by user manually to avoid overwrites. Signed-off-by: Lan Luo <[email protected]>

luolanzone force-pushed the validate-serviceaccount branch from 92b890f to 967ee6a Compare August 11, 2022 02:02

jianjuns approved these changes Aug 11, 2022

View reviewed changes

jianjuns merged commit c54a715 into antrea-io:main Aug 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate ServiceAccount for ClusterSet member list changes #4090

Validate ServiceAccount for ClusterSet member list changes #4090

luolanzone commented Aug 9, 2022 •

edited by jianjuns

Loading

luolanzone commented Aug 9, 2022

codecov bot commented Aug 9, 2022 •

edited

Loading

jianjuns left a comment

jianjuns Aug 9, 2022

luolanzone Aug 10, 2022

jianjuns Aug 10, 2022

luolanzone Aug 10, 2022

jianjuns Aug 10, 2022

luolanzone Aug 11, 2022

luolanzone commented Aug 10, 2022

jianjuns Aug 10, 2022

jianjuns Aug 11, 2022

luolanzone Aug 11, 2022

jianjuns commented Aug 11, 2022

jianjuns commented Aug 11, 2022

Validate ServiceAccount for ClusterSet member list changes #4090

Validate ServiceAccount for ClusterSet member list changes #4090

Conversation

luolanzone commented Aug 9, 2022 • edited by jianjuns Loading

luolanzone commented Aug 9, 2022

codecov bot commented Aug 9, 2022 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luolanzone commented Aug 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jianjuns commented Aug 11, 2022

jianjuns commented Aug 11, 2022

luolanzone commented Aug 9, 2022 •

edited by jianjuns

Loading

codecov bot commented Aug 9, 2022 •

edited

Loading