Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unnecessary CRDs and RBAC rules #3491

Merged
merged 1 commit into from
Mar 25, 2022
Merged

Conversation

luolanzone
Copy link
Contributor

@luolanzone luolanzone commented Mar 21, 2022

Fixes #3489

This PR is based on #3435, only the last commit is relevant to this PR.

Some CRDs and access control rules are required by member or leader
only, so remove unused CRDs and RBAC rules in separate leader and member
manifests.

Please note: since Kustomize didn't support deleteFromPrimitiveList,
it's unable to remove elements in all-in-one RBAC rules through overlay
setting, so I added separate rbac files for both leader and member. It means
if you use kubebuilder comment marker without adding them into right overlay role files, you will only
get the change in all-in-one file which it's the file in multicluster/config/rbac/role.yaml.

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

@luolanzone luolanzone added the area/multi-cluster Issues or PRs related to multi cluster. label Mar 21, 2022
@luolanzone
Copy link
Contributor Author

/test-multicluster-e2e

@codecov-commenter
Copy link

codecov-commenter commented Mar 21, 2022

Codecov Report

Merging #3491 (c2eebee) into main (7599ffb) will decrease coverage by 11.07%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #3491       +/-   ##
===========================================
- Coverage   65.49%   54.41%   -11.08%     
===========================================
  Files         278      392      +114     
  Lines       27750    42958    +15208     
===========================================
+ Hits        18174    23377     +5203     
- Misses       7657    17257     +9600     
- Partials     1919     2324      +405     
Flag Coverage Δ
integration-tests 38.30% <ø> (?)
kind-e2e-tests 51.27% <ø> (-3.89%) ⬇️
unit-tests 43.34% <ø> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
...g/agent/apiserver/handlers/featuregates/handler.go 0.00% <0.00%> (-82.36%) ⬇️
pkg/apis/controlplane/helper.go 33.33% <0.00%> (-66.67%) ⬇️
pkg/controller/egress/store/egressgroup.go 1.38% <0.00%> (-58.34%) ⬇️
...kg/apiserver/registry/system/supportbundle/rest.go 20.45% <0.00%> (-54.55%) ⬇️
pkg/support/dump.go 8.19% <0.00%> (-49.19%) ⬇️
...egator/apiserver/handlers/recordmetrics/handler.go 0.00% <0.00%> (-44.45%) ⬇️
pkg/support/dump_others.go 0.00% <0.00%> (-44.00%) ⬇️
pkg/controller/networkpolicy/tier.go 12.50% <0.00%> (-40.00%) ⬇️
...g/agent/apiserver/handlers/addressgroup/handler.go 0.00% <0.00%> (-40.00%) ⬇️
...agent/apiserver/handlers/appliedtogroup/handler.go 0.00% <0.00%> (-40.00%) ⬇️
... and 154 more

@@ -7,8 +7,10 @@ resources:
- service_account.yaml
- role.yaml
- role_binding.yaml
- leader_election_role.yaml
- leader_election_role_binding.yaml
# We disabled leader election in manager, so this role and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "manager" refer to here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean an option in a kubebuilder manager initialization which is used by leader and member controllers. let me change it to controller for easy understanding.

@Dyanngg
Copy link
Contributor

Dyanngg commented Mar 21, 2022

@tnqn @luolanzone can we merge #3488 first? Otherwise /test-multicluster-e2e is not reliable

@Dyanngg
Copy link
Contributor

Dyanngg commented Mar 21, 2022

Could you explain the following in more details?

It means if you use kubebuilder comment marker without adding them into right overlay role files, you will only
get the change in all-in-one file which it's the file in multicluster/config/rbac/role.yaml.

Does that indicate that in the future, rbac rules for member/leader needs to be manually added (in /config/overlays/member/roles.yml for example)? If so, is multicluster/config/rbac/role.yaml still used anywhere?

@luolanzone
Copy link
Contributor Author

Could you explain the following in more details?

It means if you use kubebuilder comment marker without adding them into right overlay role files, you will only
get the change in all-in-one file which it's the file in multicluster/config/rbac/role.yaml.

Does that indicate that in the future, rbac rules for member/leader needs to be manually added (in /config/overlays/member/roles.yml for example)? If so, is multicluster/config/rbac/role.yaml still used anywhere?

correct, we need manually add all required roles separately in the future, role.yaml is automatically generated when we run make manifests because controller-gen will translate any kubebuilder comment marker into correct role setting automatically. but it will not be used by leader and member manifests directly any more after this change. but you can refer to the auto-generated roles if you still like to use kubebuilder marker. and we may reuse the all-in-one role file again once deleteFromPrimitiveList directive is supported in Kustomize.

@@ -7,8 +7,11 @@ resources:
- service_account.yaml
- role.yaml
- role_binding.yaml
- leader_election_role.yaml
- leader_election_role_binding.yaml
# We disabled leader election in controller, so this role and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should make it clearer. Do you mean Multi-cluster Controller or antrea-mc-controller?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, let me change it to Multi-cluster Controller.

kind: ClusterRole
metadata:
creationTimestamp: null
name: controller-role
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could not use a name like "controller-role" which can easily conflicts with other controllers in the cluster. Add "antrea-mc-" prefix for all such K8s standard resources.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, this is a base file, we add a prefix antrea-mc- in kustomization.yml which will add this prefix in final manifest.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I recall this now!

@luolanzone
Copy link
Contributor Author

/test-multicluster-e2e

@luolanzone luolanzone added this to the Antrea v1.6 release milestone Mar 23, 2022
@luolanzone luolanzone changed the title Remove uncessary CRDs and RBAC rules Remove unnecessary CRDs and RBAC rules Mar 23, 2022
@luolanzone
Copy link
Contributor Author

/test-multicluster-e2e

@luolanzone
Copy link
Contributor Author

@Dyanngg multicluster e2e passed after I rebased my changes on top of your ACNP e2e. but I checked the ACNP e2e test, it only verify the replicated ACNP's realization, no failed case for event creation, I manually tried it, it works as expected. so I think this PR can work fine with latest ACNP changes.

@luolanzone
Copy link
Contributor Author

@jianjuns @tnqn do you have any new comment for this PR? thanks!

jianjuns
jianjuns previously approved these changes Mar 24, 2022
@tnqn
Copy link
Member

tnqn commented Mar 25, 2022

@luolanzone could you rebase?

Some CRDs and access control rules are required by member or leader
only, so remove unused CRDs and RBAC rules in separate leader and member
manifests.

Please note: since Kustomize didn't support deleteFromPrimitiveList,
it's unable to remove elements in all-in-one RBAC rules through overlay
setting, so I added separate rbac files for both leader and member. It means
if you use kubebuilder comment marker without adding them into right overlay role files, you will only
get the change in all-in-one file which it's the file `multicluster/config/rbac/role.yaml`.

Signed-off-by: Lan Luo <[email protected]>
@luolanzone
Copy link
Contributor Author

/test-multicluster-e2e

@luolanzone
Copy link
Contributor Author

@tnqn rebase is done, MC e2e is triggered. thanks.

@tnqn
Copy link
Member

tnqn commented Mar 25, 2022

/skip-all

@tnqn tnqn merged commit f0c3cbe into antrea-io:main Mar 25, 2022
@luolanzone luolanzone deleted the fix-rbac branch March 25, 2022 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/multi-cluster Issues or PRs related to multi cluster.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove uncessary access right for leader and member service account.
5 participants