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

minimize the rbac permissions for karmada-operator #5586

Merged
merged 1 commit into from
Oct 22, 2024

Conversation

B1F030
Copy link
Contributor

@B1F030 B1F030 commented Sep 22, 2024

What type of PR is this?
/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:
Part of #5182

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

@karmada-bot karmada-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Sep 22, 2024
@karmada-bot karmada-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 22, 2024
@codecov-commenter
Copy link

codecov-commenter commented Sep 22, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 40.53%. Comparing base (be571fb) to head (8b6d4c4).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5586      +/-   ##
==========================================
- Coverage   40.53%   40.53%   -0.01%     
==========================================
  Files         650      650              
  Lines       55184    55184              
==========================================
- Hits        22369    22367       -2     
- Misses      31383    31385       +2     
  Partials     1432     1432              
Flag Coverage Δ
unittests 40.53% <ø> (-0.01%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@B1F030
Copy link
Contributor Author

B1F030 commented Sep 22, 2024

/cc @zhzhuang-zju

@zhzhuang-zju
Copy link
Contributor

/assign

@zhzhuang-zju
Copy link
Contributor

@B1F030 I see that you have broken down the RBAC permissions for component karmada-operator, which is a good idea. Have you validated this locally? If so, could you tell me how you conducted the validation?

@zhzhuang-zju
Copy link
Contributor

@B1F030 The karmada-operator has two installation methods: installation via Helm and installation from source code. What you are modifying now is the RBAC under the Helm installation method. When the RBAC for the Helm installation method is ready, it needs to be synchronized and modified in the source code as well. I will break down the work items in the issue #5182.

@zhzhuang-zju
Copy link
Contributor

During my review, I found a method for local verification; you might also want to give it a try.

  1. Set up the KUBECONFIG and CONTEXT.
$ export KUBECONFIG=~/.kube/xxxx.config
$ kubectl config use-context xxx
  1. Modify the manifest of the karmada-operator to increase the log level.
# charts/karmada-operator/templates/karmada-operator-deployment.yaml
- --v=4
  1. Install the karmada-operator by Helm.
$ helm install karmada-operator -n karmada-system --create-namespace --dependency-update ./charts/karmada-operator --set operator.image.tag=latest --debug
  1. Deploy a Karmada instance through the karmada-operator.
  2. The environment will have a ServiceAccount (sa), a ClusterRole, and a ClusterRoleBinding. The ClusterRole controls the RBAC permissions for the karmada-operator, and we need to validate by modifying the content of the ClusterRole.
$ kubectl get clusterrole
NAME                                                                   
karmada-operator  
$ kubectl get sa
karmada-operator 
$ kubectl get clusterrolebinding
karmada-operator 
  1. modify clusterrole
$ kubectl get clusterrole karmada-operator --namespace karmada-system -oyaml > karmada-operator.yaml
$ vi karmada-operator.yaml
$ kubectl apply -f karmada-operator.yaml
# karmada-operator.yaml
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
  annotations:
    meta.helm.sh/release-name: karmada-operator
    meta.helm.sh/release-namespace: karmada-system
  labels:
    app.kubernetes.io/instance: karmada-operator
    app.kubernetes.io/managed-by: Helm
    app.kubernetes.io/name: karmada-operator
    helm.sh/chart: karmada-operator-0.0.1
  name: karmada-operator
rules:
- apiGroups:
  - coordination.k8s.io
  resources:
  - leases # karmada-operator needs to use the Leases resource for leader election.
  verbs:
  - get
  - create
  - update
  1. restart karmada-operator
$ kubectl rollout restart deployment/karmada-operator -nkarmada-system
  1. Check the functionality of the karmada-operator through logs. If the logs indicate insufficient permissions, supplement the permissions for the relevant resources.
  2. Repeat steps 6, 7, and 8 until the functionality is fully operational.

@karmada-bot karmada-bot added the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Oct 13, 2024
@B1F030 B1F030 force-pushed the karmada-operator-rbac branch from 82828f1 to d631fd9 Compare October 13, 2024 10:19
@karmada-bot karmada-bot removed the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Oct 13, 2024
@zhzhuang-zju
Copy link
Contributor

@B1F030 I am using the RBAC you set up to install the karmada-operator and to deploy karmada instances through the karmada-operator. The installation process gets stuck when installing karmada-etcd. Refer to

if err := waiter.WaitForSomePods(etcdLabels.String(), data.GetNamespace(), 1); err != nil {

The karmada-operator still requires RBAC permissions for the pod resource to check if the pods are ready.

@B1F030 B1F030 force-pushed the karmada-operator-rbac branch 2 times, most recently from f8057dd to 31ae4cf Compare October 16, 2024 09:12
@karmada-bot karmada-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 16, 2024
@RainbowMango
Copy link
Member

@B1F030 Could you please squash the commits?

@B1F030 B1F030 force-pushed the karmada-operator-rbac branch from e0ed0b1 to c763bce Compare October 21, 2024 03:39
@B1F030
Copy link
Contributor Author

B1F030 commented Oct 21, 2024

@B1F030 Could you please squash the commits?

@RainbowMango Done.

@RainbowMango
Copy link
Member

/lgtm
@zhzhuang-zju Please take another look.

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 21, 2024
@RainbowMango RainbowMango added this to the v1.12 milestone Oct 21, 2024
@zhzhuang-zju
Copy link
Contributor

@B1F030 others LGTM

Using the one-click installation script provided by #5519 to install Karmada, the result is as follows:

$ kh get pods -ntest
NAME                                                    READY   STATUS    RESTARTS   AGE
karmada-demo-aggregated-apiserver-7f88d95ddb-w4csq      1/1     Running   0          4m45s
karmada-demo-apiserver-58b4c865bb-7fh9w                 1/1     Running   0          5m27s
karmada-demo-controller-manager-8ccff6859-qjfk9         1/1     Running   0          4m42s
karmada-demo-descheduler-c64fbb854-c52wn                1/1     Running   0          4m41s
karmada-demo-etcd-0                                     1/1     Running   0          6m14s
karmada-demo-kube-controller-manager-8585fb7bc4-7pqth   1/1     Running   0          4m42s
karmada-demo-metrics-adapter-66d696d574-t8nqs           1/1     Running   0          4m41s
karmada-demo-metrics-adapter-66d696d574-tsvlp           1/1     Running   0          4m41s
karmada-demo-scheduler-5479b4c9fc-qjb5w                 1/1     Running   0          4m42s
karmada-demo-search-78b6dd56c9-97nfv                    1/1     Running   0          4m38s
karmada-demo-webhook-7dd55448cb-4pjq4                   1/1     Running   0          4m41s

Passed the basic e2e tests

$ ./run-e2e-local.sh "[BasicPropagation] propagation testing deployment propagation testing"
Ran 3 of 239 Specs in 36.530 seconds
SUCCESS! -- 3 Passed | 0 Failed | 0 Pending | 236 Skipped

$ ./run-e2e-local.sh "priorityMatchName/priorityMatchLabel/priorityMatchAll propagation testing"
Ran 1 of 239 Specs in 177.398 seconds
SUCCESS! -- 1 Passed | 0 Failed | 0 Pending | 238 Skipped


Ginkgo ran 1 suite in 3m32.078400677s
Test Suite Passed

@B1F030 B1F030 force-pushed the karmada-operator-rbac branch from c763bce to 42ba368 Compare October 22, 2024 02:45
@karmada-bot karmada-bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 22, 2024
@B1F030 B1F030 force-pushed the karmada-operator-rbac branch from 42ba368 to 8b6d4c4 Compare October 22, 2024 03:10
@zhzhuang-zju
Copy link
Contributor

/lgtm
cc @RainbowMango

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 22, 2024
Copy link
Member

@RainbowMango RainbowMango left a comment

Choose a reason for hiding this comment

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

/approve

@karmada-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RainbowMango

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@karmada-bot karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 22, 2024
@karmada-bot karmada-bot merged commit 13df63f into karmada-io:master Oct 22, 2024
13 checks passed
@B1F030 B1F030 deleted the karmada-operator-rbac branch October 22, 2024 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants