Add ACNP copy span for multi-cluster #3363

Dyanngg · 2022-02-25T23:23:53Z

With this change, Antrea Multi-cluster admins can specify certain ClusterNetworkPolicies to be replicated
across the entire ClusterSet. This is especially useful for ClusterSet admins who want all clusters in the
ClusterSet to be applied with a consistent security posture (for example, all namespaces in all clusters can
only communicate with Pods in their own namespaces).

To achieve such ACNP copy-span, admins can, in the acting leader cluster of a Multi-cluster deployment,
create a ResourceExport of kind AntreaClusterNetworkPolicy which contains the ClusterNetworkPolicy spec
they wish to be replicated. The ResourceExport should be created in the Namespace which implements the
Common Area of the ClusterSet. In future releases, some additional tooling may become available to
automate the creation of such ResourceExport and make ACNP replication across clusters easier.

apiVersion: multicluster.crd.antrea.io/v1alpha1
kind: ResourceExport
metadata:
  name: strict-namespace-isolation-for-test-clusterset
  namespace: antrea-mcs-ns          # Namespace that implements Common Area of test-clusterset
spec:
  kind: AntreaClusterNetworkPolicy  
  name: strict-namespace-isolation  # In each importing cluster, an ACNP of name antrea-mc-strict-namespace-isolation will be created with the spec below
  clusternetworkpolicy:
    priority: 1
    tier: securityops
    appliedTo:
      - namespaceSelector: {}       # Selects all Namespaces in the member cluster
    ingress:
      - action: Pass
        from:
        - namespaces:
            match: Self            # Skip drop rule for traffic from Pods in the same Namespace
        - podSelector:
            matchLabels:
              k8s-app: kube-dns    # Skip drop rule for traffic from the core-dns components
      - action: Drop
        from:
        - namespaceSelector: {}    # Drop from Pods from all other Namespaces

The above sample spec will create an ACNP in each member cluster which implements strict namespace
isolation for that cluster.

codecov-commenter · 2022-02-26T00:22:26Z

Codecov Report

Merging #3363 (2982920) into main (f7e980e) will decrease coverage by 10.57%.
The diff coverage is 64.24%.

@@             Coverage Diff             @@
##             main    #3363       +/-   ##
===========================================
- Coverage   65.56%   54.99%   -10.58%     
===========================================
  Files         268      375      +107     
  Lines       26909    51719    +24810     
===========================================
+ Hits        17643    28443    +10800     
- Misses       7354    20793    +13439     
- Partials     1912     2483      +571

Flag	Coverage Δ
e2e-tests	`53.48% <ø> (?)`
integration-tests	`35.83% <ø> (?)`
kind-e2e-tests	`55.55% <ø> (-0.22%)`	⬇️
unit-tests	`42.84% <64.24%> (+0.15%)`	⬆️

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

Impacted Files	Coverage Δ
...ticluster/commonarea/remote_common_area_manager.go	`56.16% <ø> (ø)`
...trollers/multicluster/resourceexport_controller.go	`69.39% <61.36%> (-0.53%)`	⬇️
...uster/controllers/multicluster/stale_controller.go	`51.69% <62.06%> (+2.21%)`	⬆️
...uster/commonarea/acnp_resourceimport_controller.go	`65.26% <65.26%> (ø)`
...lticluster/commonarea/resourceimport_controller.go	`70.24% <66.66%> (+1.76%)`	⬆️
...icluster/cmd/multicluster-controller/controller.go	`6.15% <100.00%> (+1.46%)`	⬆️
multicluster/controllers/multicluster/test_data.go	`100.00% <100.00%> (ø)`
pkg/agent/cniserver/pod_configuration_linux.go	`26.31% <0.00%> (-40.36%)`	⬇️
pkg/controller/ipam/antrea_ipam_controller.go	`48.71% <0.00%> (-31.57%)`	⬇️
pkg/controller/networkpolicy/endpoint_querier.go	`61.46% <0.00%> (-29.97%)`	⬇️
... and 344 more

luolanzone · 2022-02-27T07:29:53Z

@Dyanngg the unit test and DCO check are failed, you may take a look at them first.

Dyanngg · 2022-02-28T07:01:22Z

@Dyanngg the unit test and DCO check are failed, you may take a look at them first.

Fixed

multicluster/apis/multicluster/v1alpha1/resourceexport_webhook.go

multicluster/config/crd/bases/multicluster.crd.antrea.io_acnpimports.yaml

multicluster/controllers/multicluster/commonarea/resourceimport_controller.go

multicluster/controllers/multicluster/resourceexport_controller.go

multicluster/controllers/multicluster/commonarea/resourceimport_controller.go

luolanzone · 2022-03-01T01:06:01Z

/test-multicluster-e2e

luolanzone · 2022-03-01T01:08:48Z

@Dyanngg you probably need to rebase the codes,there is a github build error from build-ubi, I noticed there was a change revert for ubi related change.

multicluster/Makefile

multicluster/apis/multicluster/v1alpha1/acnpimport.go

docs/multicluster/architecture.md

multicluster/controllers/multicluster/commonarea/acnp_resourceimport_controller.go

luolanzone · 2022-03-08T07:48:19Z

/test-multicluster-e2e

luolanzone

LGTM overall, two minor comments.

multicluster/Makefile

multicluster/controllers/multicluster/resourceexport_controller.go

jianjuns

LGTM

Dyanngg · 2022-03-11T18:57:20Z

/test-all E2E test for this feature is passing according to #3435

luolanzone

LGTM overall, one nit

luolanzone · 2022-03-15T01:13:36Z

multicluster/controllers/multicluster/commonarea/acnp_resourceimport_controller.go

+func randSeq(n int) string {
+	b := make([]rune, n)
+	for i := range b {
+		// #nosec G404: random number generator not used for security purposes
+		randIdx := rand.Intn(len(lettersAndDigits))
+		b[i] = lettersAndDigits[randIdx]
+	}
+	return string(b)
+}
+
+func randName(prefix string) string {
+	return prefix + randSeq(nameSuffixLength)
+}


I noticed there are a few duplicate code in test framework, can we share them somewhere? maybe address it in a separate PR.

luolanzone · 2022-03-18T02:02:53Z

@jianjuns do you have any new comment? @tnqn Could you help to take a look at this PR? it's a core feature from MC in v1.6.0. a few other e2e PRs are based on this, thanks!

jianjuns · 2022-03-18T03:44:04Z

No further comment from me.

docs/multicluster/getting-started.md

tnqn · 2022-03-18T12:32:49Z

multicluster/apis/multicluster/v1alpha1/resourceexport_types.go

@@ -63,6 +64,8 @@ type ResourceExportSpec struct {
 	Endpoints *EndpointsExport `json:"endpoints,omitempty"`
 	// If exported resource is ExternalEntity.
 	ExternalEntity *ExternalEntityExport `json:"externalentity,omitempty"`
+	// If exported resource is AntreaClusterNetworkPolicy.
+	ClusterNetworkPolicy *v1alpha1.ClusterNetworkPolicySpec `json:"clusternetworkpolicy,omitempty"`


Not starting from this PR, the json field name is not following the convention, which should be "clusterNetworkPolicy".

Let's address json field names in a separate PR then.

multicluster/build/yamls/antrea-multicluster-leader-namespaced.yml

multicluster/controllers/multicluster/commonarea/acnp_resourceimport_controller.go

tnqn · 2022-03-18T13:41:04Z

multicluster/controllers/multicluster/commonarea/acnp_resourceimport_controller.go

+		FirstTimestamp:      metav1.Now(),
+		LastTimestamp:       metav1.Now(),
+		ReportingController: acnpEventReportingController,
+		ReportingInstance:   acnpEventReportingInstance,


this could be the podName?

Yes, see my TODO on L41

tnqn · 2022-03-18T14:03:26Z

multicluster/build/yamls/antrea-multicluster-leader-namespaced.yml

+  - list
+  - patch
+  - update
+  - watch


does leader need any cnp permission?

No it does not. However this rbac manifest is generated and the only place I've added cnp permissions is in resourceimport_controller.go:
//+kubebuilder:rbac:groups=crd.antrea.io,resources=clusternetworkpolicies,verbs=get;list;watch;create;update;patch;delete
Not sure how to strip this permission from leader cluster specifically. @luolanzone any ideas?

When we split them into member and leader manifests, we didn't separate these accesses setting in fine-grained way, I think we may need a follow up PR to fix these access right. I will create an issue for this first.
@tnqn do you think it's OK to fix this kind of issue in a new PR?

Yes, but please address it in release 1.6. We had such issues before and should ensure all components only get permissions they require.

Signed-off-by: Yang Ding <[email protected]>

tnqn

LGTM

tnqn · 2022-03-21T17:30:56Z

multicluster/build/yamls/antrea-multicluster-leader-namespaced.yml

+  - list
+  - patch
+  - update
+  - watch


Yes, but please address it in release 1.6. We had such issues before and should ensure all components only get permissions they require.

luolanzone · 2022-03-22T01:06:17Z

sure, @tnqn I have made the PR #3491, I think we need to merge this first, then I will rebase my PR and update the role added by ACNP copy-span. thanks.

tnqn · 2022-03-22T02:24:42Z

/test-all
/test-multicluster-e2e

tnqn · 2022-03-22T13:19:56Z

/skip-conformance

tnqn · 2022-03-22T13:21:10Z

/skip-conformance

tnqn · 2022-03-22T15:37:37Z

/skip-conformance

tnqn · 2022-03-22T15:39:06Z

/skip-conformance

tnqn · 2022-03-22T15:39:25Z

/test-conformance

tnqn · 2022-03-22T15:40:21Z

Conformance succeeded but failed on cleanup

Dyanngg requested review from aravindakidambi, hjiajing, jianjuns and luolanzone February 25, 2022 23:23

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

Dyanngg force-pushed the mcs-copy-span-policy branch from 7d637a9 to c132869 Compare February 28, 2022 07:00

Dyanngg force-pushed the mcs-copy-span-policy branch from c132869 to 3bbd2f5 Compare February 28, 2022 07:04

luolanzone reviewed Feb 28, 2022

View reviewed changes

Dyanngg force-pushed the mcs-copy-span-policy branch 2 times, most recently from 2c34d57 to 43fcb0c Compare February 28, 2022 22:23

jianjuns reviewed Mar 1, 2022

View reviewed changes

multicluster/Makefile Show resolved Hide resolved

multicluster/apis/multicluster/v1alpha1/acnpimport.go Outdated Show resolved Hide resolved

Dyanngg force-pushed the mcs-copy-span-policy branch 2 times, most recently from 1229564 to b7f8cb9 Compare March 8, 2022 05:28

Dyanngg requested review from luolanzone and jianjuns March 8, 2022 05:31

Dyanngg force-pushed the mcs-copy-span-policy branch from b7f8cb9 to 405369e Compare March 8, 2022 05:33

Dyanngg requested a review from tnqn March 8, 2022 05:34

Dyanngg force-pushed the mcs-copy-span-policy branch 2 times, most recently from fbc8122 to a3bf01a Compare March 8, 2022 06:03

luolanzone reviewed Mar 8, 2022

View reviewed changes

Dyanngg force-pushed the mcs-copy-span-policy branch 2 times, most recently from 7c97846 to bc9b6c1 Compare March 9, 2022 00:01

Dyanngg requested a review from luolanzone March 9, 2022 01:03

luolanzone reviewed Mar 9, 2022

View reviewed changes

multicluster/Makefile Outdated Show resolved Hide resolved

multicluster/controllers/multicluster/resourceexport_controller.go Outdated Show resolved Hide resolved

jianjuns reviewed Mar 11, 2022

View reviewed changes

Dyanngg mentioned this pull request Mar 11, 2022

E2E test for MC ACNP copy-span #3435

Merged

Dyanngg requested a review from luolanzone March 11, 2022 18:57

luolanzone previously approved these changes Mar 15, 2022

View reviewed changes

tnqn reviewed Mar 18, 2022

View reviewed changes

Dyanngg added 7 commits March 18, 2022 11:38

Add ACNP copy span for multi-cluster

870d985

Signed-off-by: Yang Ding <[email protected]>

Cleanup test code

7b3d274

Signed-off-by: Yang Ding <[email protected]>

Rebase and resolve comments

83ca0c8

Signed-off-by: Yang Ding <[email protected]>

Add documentation

6ebe48c

Signed-off-by: Yang Ding <[email protected]>

Use events for acnp import status report

7407dc5

Signed-off-by: Yang Ding <[email protected]>

Address more comments and add logic in stale controller

66c6c10

Signed-off-by: Yang Ding <[email protected]>

Address final comments

2982920

Signed-off-by: Yang Ding <[email protected]>

Dyanngg dismissed luolanzone’s stale review via 2982920 March 18, 2022 19:04

Dyanngg force-pushed the mcs-copy-span-policy branch from f7e29a8 to 2982920 Compare March 18, 2022 19:04

luolanzone mentioned this pull request Mar 21, 2022

Remove uncessary access right for leader and member service account. #3489

Closed

Dyanngg requested a review from tnqn March 21, 2022 04:47

tnqn approved these changes Mar 21, 2022

View reviewed changes

tnqn merged commit 37291dd into antrea-io:main Mar 22, 2022

tnqn added the action/release-note Indicates a PR that should be included in release notes. label Mar 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ACNP copy span for multi-cluster #3363

Add ACNP copy span for multi-cluster #3363

Dyanngg commented Feb 25, 2022 •

edited

Loading

codecov-commenter commented Feb 26, 2022 •

edited

Loading

luolanzone commented Feb 27, 2022

Dyanngg commented Feb 28, 2022

luolanzone commented Mar 1, 2022

luolanzone commented Mar 1, 2022

luolanzone commented Mar 8, 2022

luolanzone left a comment

jianjuns left a comment

Dyanngg commented Mar 11, 2022

luolanzone left a comment

luolanzone Mar 15, 2022

Dyanngg Mar 15, 2022

luolanzone commented Mar 18, 2022

jianjuns commented Mar 18, 2022

tnqn Mar 18, 2022

Dyanngg Mar 18, 2022

tnqn Mar 18, 2022

Dyanngg Mar 18, 2022

tnqn Mar 18, 2022

Dyanngg Mar 18, 2022

luolanzone Mar 21, 2022

tnqn Mar 21, 2022

tnqn left a comment

tnqn Mar 21, 2022

luolanzone commented Mar 22, 2022

tnqn commented Mar 22, 2022

tnqn commented Mar 22, 2022

tnqn commented Mar 22, 2022

tnqn commented Mar 22, 2022

tnqn commented Mar 22, 2022

tnqn commented Mar 22, 2022

tnqn commented Mar 22, 2022

Add ACNP copy span for multi-cluster #3363

Add ACNP copy span for multi-cluster #3363

Conversation

Dyanngg commented Feb 25, 2022 • edited Loading

codecov-commenter commented Feb 26, 2022 • edited Loading

Codecov Report

luolanzone commented Feb 27, 2022

Dyanngg commented Feb 28, 2022

luolanzone commented Mar 1, 2022

luolanzone commented Mar 1, 2022

luolanzone commented Mar 8, 2022

luolanzone left a comment

Choose a reason for hiding this comment

jianjuns left a comment

Choose a reason for hiding this comment

Dyanngg commented Mar 11, 2022

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

luolanzone commented Mar 18, 2022

jianjuns commented Mar 18, 2022

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

luolanzone commented Mar 22, 2022

tnqn commented Mar 22, 2022

tnqn commented Mar 22, 2022

tnqn commented Mar 22, 2022

tnqn commented Mar 22, 2022

tnqn commented Mar 22, 2022

tnqn commented Mar 22, 2022

tnqn commented Mar 22, 2022

Dyanngg commented Feb 25, 2022 •

edited

Loading

codecov-commenter commented Feb 26, 2022 •

edited

Loading