charts
charts copied to clipboard
tinkerbell
Switch roles to clusterroles
Description
Accompanies pull request https://github.com/tinkerbell/hegel/pull/370
Why is this needed
If hegel is going to read from multiple namespaces it need accompanying rbac
How are existing users impacted? What migration steps/scripts do we need?
May break compatibility if a user is running in an environment where they are not allowed to roll out cluster-rbac resources ? Otherwise there should be no impact.
Checklist:
I have:
- [ ] updated the documentation and/or roadmap (if required)
- [ ] added unit or e2e tests
- [ ] provided instructions on how to upgrade
Like this? I named the option allNamespaces: false
I also took the liberty of adding the --kubernetes-namespace option when not opting in to clusterroles for all namespaces because I felt that was prudent.
What do we think about offering a .Values.rbac.enabled which, when disabled, assumes the user configures the RBAC however they want? You can already set --kubernetes-namespace with the .Values.args field.
I'm not fond of the RBAC customizations and would prefer this approach because it seems simpler and maintains the minimal set of CLI flags.
If I, as a user, have to configure my own RBAC that means I have to introduce additional configurations myself, outside of the chart, and can no longer install it off-the-shelf as it were. That kinda defeats the whole purpose of having a helm chart in the first place.
But if you're really not fond of the RBAC customizations, then I would suggest that most users will be fine with the default of ClusterRole and ClusterRoleBinding, which can then be disabled with a value. Note that would also be consistent with all the other components which have cluster-wide rbac. (hegel is the exception in this). That would be better anyway, because if a user really needs it to just look at a specific namespace, it's at least equally likely that that will not be the namespace hegel is installed in in the first place.
The values.args thing seems valid, I don't have any argument for or against.
Thanks for the feedback, @willemm. I'm open to making ClusterRole and ClusterRoleBinding the default, though it still needs to be opt-out so users can define a small scope should they want to.
... have to configure my own RBAC that means I have to introduce additional configurations myself, outside of the chart, and can no longer install it off-the-shelf as it were. That kinda defeats the whole purpose of having a helm chart in the first place.
The existing RBAC aligned with historical design decisions. I appreciate it doesn't suit your deployment model, but it is a valid deployment model that's been working for some time.
I don't think tweaking a chart to satisfy a different deployment model is abnormal.
Ok. I had a small look and I'm not sure .Values.rbac.enabled works "nicely" without some other changes, mainly because there are currewntly two values, .Values.roleName and .Values.roleBindingName which seem like they shoud also go under .Values.rbac.* ?
Ok. I had a small look and I'm not sure .Values.rbac.enabled works "nicely" without some other changes, mainly because there are currewntly two values, .Values.roleName and .Values.roleBindingName which seem like they shoud also go under .Values.rbac.* ?
That's fine. There isn't an expectation to remain backward compatible yet, though we do our best to minimize disruptions.
#120 says, "unrestricted access shouldn't be the default or only option". I agree with this. @willemm, i apologize. i think we've gone back and forth on this, but i think we're going to need to be able to select between both role and clusterRole.
For Rufio that makes sense, as it can read secrets. Which it can currently do, cluster-wide. However, hegel only reads 'hardware' resources from the tinkerbell.org apigroup.
So I'm afraid I don't understand why this is an issue for the hegel chart. Currently it is the only exception that does not have clusterrolebindings, and it's the least impactful one from a security standpoint. Negligible impact, in fact.
So while this PR could include a switch option, I feel it would be much better if all the charts had the same rbac structure/setup, whuich should IMO be part of how you fix the #120 issue. (i.e. don't just fix rufio there, but copy-paste the fix across all the charts)
Hey @willemm , Apologies for all the churn on this one. Thank you for putting this up. @chrisdoherty4 and I spoke today. I would like us to go back to what you originally had. Apologies for the flip-flopping. Also, as you've noted, we need a fix across all services. Not only that, but not all of our services handle single namespaces properly either. With all this said, I've created PR #129 and I'm working across the Tinkerbell service repos in order to make sure they all handle namespacing properly.
If you have some cycles (if not, totally fine), I'd really appreciate your review on #129. Thanks again for contributing!
No worries, I work in fintech so this is a breeze, comparatively ;)
I looked at it, all of it looks good. (Assuming hegel indeed does not need access to workflows, which I think it shouldn't, but I'm not sure)