-
Notifications
You must be signed in to change notification settings - Fork 106
Make sidecar injector clusterrole more restrictive by specifying resourcenames #231
Conversation
verbs: ["get", "list", "watch"] | ||
- apiGroups: ["admissionregistration.k8s.io"] | ||
resources: ["mutatingwebhookconfigurations"] | ||
resourceNames: [{{ print "\"istio-sidecar-injector-" .Release.Namespace "\"" }}] |
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.
minor point: I think this can just be ["istio-sidecar-injector-{{.Release.Namespace}"]
or does that not work?
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.
good point - worth simplifying this if posible.
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.
Thanks @howardjohn. I updated the PR with the suggested change.
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.
Nice! a good part of istio/istio#14158
@sdake I've heard you have looked into the cluster roles a lot if you know of any reason why this isn't safe? Otherwise lgtm
@howardjohn looks good to me - @neeleshkorade contacted me prior to the submission. As a check is failing, we need to sort that out. Cheers |
gate failure
|
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
waiting on experiments to determine if the simplification produces desired results.
verbs: ["get", "list", "watch"] | ||
- apiGroups: ["admissionregistration.k8s.io"] | ||
resources: ["mutatingwebhookconfigurations"] | ||
resourceNames: [{{ print "\"istio-sidecar-injector-" .Release.Namespace "\"" }}] |
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.
good point - worth simplifying this if posible.
I think this is a flake, should be investigated |
@sdake
Tested this manually for-