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

Update RBAC permissions for controller and webhook #1035

Merged
merged 1 commit into from
Jan 14, 2022

Conversation

Nuatu
Copy link
Contributor

@Nuatu Nuatu commented Dec 22, 2021

1. Issue, if available:
#973

2. Description of changes:
Update RBAC permissions for controller and webhook managed/deployed by helm.

3. Does this change impact docs?

  • Yes, PR includes docs updates
  • Yes, issue opened: link to issue
  • No

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.


configmaps

removed "update" from configmaps list within karpenter-controller [ClusterRole]

  • scope: cluster-wide (all namespaces)
  • previous verbs: ["get", "list", "watch", update]
  • current verbs: ["get", "list", "watch"]
  • effect: remove permission to "update"

provisioners

removed "provisioners" from resources list within karpenter-controller [ClusterRole]

  • scope: cluster-wide (all namespaces)
  • previous resources: ["provisioners", "provisioners/status"]
  • current resources: ["provisioners/status"]
  • effect: removed access to "provisioners"

added "provisioners" rule within karpenter-controller [ClusterRole]

  • scope: cluster-wide (all namespaces)
  • effect: provide read permission to "karpenter.sh" API group provisioners

leases

moved "leases" from [ClusterRole] to [Role] within karpenter-controller

  • scope: karpenter namespace
  • previous leases: scoped to ClusterRole
  • current leases: scoped to Role
  • effect: restrict to karpenter namespace

secrets

added resourceName to existing rule within karpenter-webhook [Role]

  • scope: karpenter namespace
  • previous resourceNames: this feld was not present
  • current resourceNames: ["karpenter-webhook-cert"]
  • effect: restrict modify permission to only "karpenter-webhook-cert" within karpenter namespace

added new rule to karpenter-webhook [Role]

  • scope: karpenter namespace
  • effect: provide read permission to all secrets within karpenter namespace

@netlify
Copy link

netlify bot commented Dec 22, 2021

✔️ Deploy Preview for karpenter-docs-prod ready!

🔨 Explore the source changes: 6ceca84

🔍 Inspect the deploy log: https://app.netlify.com/sites/karpenter-docs-prod/deploys/61e0ba58cf5961000858034f

😎 Browse the preview: https://deploy-preview-1035--karpenter-docs-prod.netlify.app

@Nuatu Nuatu mentioned this pull request Dec 22, 2021
- apiGroups: [""]
resources: ["nodes", "pods"]
verbs: ["get", "list", "watch", "patch", "delete"]
- apiGroups: [""]
resources: ["configmaps"]
verbs: ["get", "list", "watch", "update"]
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 that knative/pkg's configmap controller might need this to update configmaps. We should be able to restrict it to the karpenter namespace.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see, this is the controller's rbac.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you have in mind as it relates to approving the running workflow?

No rush, just curious to see the workflow run to completion and getting acquainted to the contribution process for this project.

Either way - this should be the first contribution of many since I plan to consistently contribute to this project going forward.

Copy link
Contributor

Choose a reason for hiding this comment

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

Either way - this should be the first contribution of many since I plan to consistently contribute to this project going forward.

Awesome!

Unfortunately, RBAC is not included in the current testing workflow. We run make battletest as part of CI, which stands up a local APIServer/ETCD using controller-runtime's testenv package. In order to test this, you should run make apply (see https://karpenter.sh/docs/development-guide/) and do some basic scaling and provisioner CRUD.

We definitely need to improve this -- I'll cut a ticket.

Copy link
Contributor

Choose a reason for hiding this comment

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

Were you able to test this manually?

@ellistarn
Copy link
Contributor

Hey @Nuatu, just checking in if you've had a chance to test this.

@Nuatu
Copy link
Contributor Author

Nuatu commented Jan 7, 2022

Hey @ellistarn, haven't finished yet but will have it wrapped up soon. I'll follow up really soon with the results.

@Nuatu
Copy link
Contributor Author

Nuatu commented Jan 13, 2022

@ellistarn Manual testing has been completed. Nodes get provisioned and de-provisioned as expected when I scale various deployments up and down.

@ellistarn ellistarn merged commit cecf431 into aws:main Jan 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants