-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
fine tune rbac #1109
fine tune rbac #1109
Conversation
/assign @jonjohnsonjr |
/assign @evankanderson |
/retest |
I don't know enough about RBAC rules to review this correctly. @rootfs, how were these rules generated? The integration test failure seems legitimate BTW. I see errors like:
@rootfs are you able to see the integration test logs? |
@grantr no, i cannot (no google account). I generated the rules manually by reading the code and finding what resources are being used. It is highly probable I missed some (and thus DNM tag). I am still new to knative, so bear with me for the moment. |
@grantr added eventing and build crd api groups, let's how the tests go |
@rootfs It's not ideal, but there's a workaround for seeing the test logs without a google account by composing a URL to a GCS object. See https://github.com/knative/serving/blob/master/community/REVIEWING.md#viewing-test-logs. |
f4560a3
to
78378a7
Compare
6577498
to
c745281
Compare
Signed-off-by: Huamin Chen <[email protected]>
@grantr @mattmoor @jonjohnsonjr PTAL, thanks |
/assign @evankanderson |
config/200-clusterrole.yaml
Outdated
- apiGroups: ["build.dev"] | ||
resources: ["builds", "buildtemplates"] | ||
verbs: ["get", "list", "create", "update", "delete", "patch", "watch"] | ||
- apiGroups: ["feeds.knative.dev"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these be part of this repo? Probably eventing permissions should go in a separate service account for eventing controllers. WDYT @vaikas-google?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so, they should be part of eventing and I think builds above should also be part of the build repo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
build has to be here, because revision references it here
eventing can be separate.
Signed-off-by: Huamin Chen <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Thanks for getting to to pass @rootfs!
It's toil to have to create and update this when we need new permissions. I wish these rules could be generated. https://github.com/kubernetes-sigs/kubebuilder can generate roles based on comments in the controller definition, which seems preferable to having them here.
Since we don't have that, I think it would be a good idea to add a comment near the controllers (I'm not sure where since controllers are in flux right now) saying that these rules should be kept in sync as new resources are used.
yes, a rule generator is surely helpful. In the meantime, I'll dig into rbac in eventing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
I'd like to tighten these up further outside of serving. knative.dev
, but this seems like a good start.
Should we also have suggested/example roles for the developer and operator personas? Probablya separate file and PR, though.
rules: | ||
- apiGroups: [""] | ||
resources: ["pods", "namespaces", "secrets", "configmaps", "endpoints", "services", "events", "serviceaccounts"] | ||
verbs: ["get", "list", "create", "update", "delete", "patch", "watch"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to create service accounts?
This seems fine for a first pass, but it definitely shows how little we've been thinking about least privilege.
resources: ["mutatingwebhookconfigurations"] | ||
verbs: ["get", "list", "create", "update", "delete", "patch", "watch"] | ||
- apiGroups: ["apiextensions.k8s.io"] | ||
resources: ["customresourcedefinitions"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does the controller need create/update on custom resource definitions?
resources: ["customresourcedefinitions"] | ||
verbs: ["get", "list", "create", "update", "delete", "patch", "watch"] | ||
- apiGroups: ["serving.knative.dev"] | ||
resources: ["configurations", "configurationgenerations", "routes", "revisions", "revisionuids", "autoscalers", "services"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be "*"?
resources: ["ingresses","deployments"] | ||
verbs: ["get", "list", "update", "patch", "watch"] | ||
- apiGroups: ["apps"] | ||
resources: ["deployments", "statefulsets"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why statefulsets
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(and above)
verbs: ["get", "list", "update", "patch", "watch"] | ||
- apiGroups: ["admissionregistration.k8s.io"] | ||
resources: ["mutatingwebhookconfigurations"] | ||
verbs: ["get", "list", "update", "patch", "watch"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why update and patch for the autoscaler?
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: evankanderson, rootfs 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 |
Fixes #1095
Proposed Changes
add cluster role (admin, read, write) to replace cluster-admin
Release Note