search

coredns / helm

Helm Charts for CoreDNS
Apache License 2.0
108 stars 111 forks source link

add support for additional clusterRole rules #143

Closed danlaramay closed 1 year ago

danlaramay commented 1 year ago

Why is this pull request needed and what does it do?

This allows users to specify additional clusterRole rules for CoreDNS via the additionalClusterRoleRules parameter, and for the autoscaler via autoscaler.additionalClusterRoleRules. This is necessary for Datadog metrics in EKS Fargate because the sidecar requires some additional permissions not in the provided clusterRole. I'm sure there are other use cases, but this is the one I'm currently facing.

Which issues (if any) are related?

Checklist:

Changes are automatically published when merged to main. They are not published on branches.

Note on DCO If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the *Details* link next to the DCO action for instructions on how to resolve this.
hagaibarel commented 1 year ago

I'm not sure this is something we should support. The required RBAC for coredns and the autoscaler is known and finite, opening it up to additional unlimited rules poses a security concern.

Why not defining a separate clusterrole/role outside of the chart and assigning it to the coredns service account past install?

danlaramay commented 1 year ago

Hi @hagaibarel. Thanks for the quick response. I was inspired by how karpenter supports this and was hoping to add similar support here.

Why not defining a separate clusterrole/role outside of the chart and assigning it to the coredns service account past install?

I was hoping to use this chart without any additional custom scripting. I'm able to use .Values.extraContainers to configure sidecars and was hoping to also be able to configure the permissions I need for the sidecar in the same way.

hagaibarel commented 1 year ago

Extra container are meant to solve a side car issue that you can't really solve without changes in the chart (one can argue that the proper solution would be a mutating webhook). Adding extra rules is something that can be done outside of the chart, and as such I'm not sure we should include it

danlaramay commented 1 year ago

@hagaibarel Thanks so much for your time and contributions to this helm chart. I appreciate you taking a look. I'll handle this outside of the helm chart.