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

clusterrole.yaml: add if condition for .Values.rbac.enabled #242

Merged
merged 3 commits into from
Jan 16, 2025

Conversation

kastl-ars
Copy link
Contributor

clusterrole.yaml: add if condition for .Values.rbac.enabled, like on the clusterrolebinding

fix #241

@mittal-ishaan
Copy link
Contributor

Hi @kastl-ars
Thank you for this. LGTM
Due to my recent merge, we did add rbac like this which we would want to consider upon. My bad here, I should have seen it under my PR.
Should we also have them under this if condition too, we are already gating them under a helm config?

Question is do we also want to have rbac.enabled flag gate the cluster role needed in openshift environment too (that looks exactly what this flag is intended to do) or we can have scenarios where a user wants to disable the rbac.enabled flag but want the required cluster role, etc for openshift environment.

Otherwise the Issue raised and PR is really helpful

…ues.rbac.enabled, like on the clusterrolebinding

fix opencost#241
@kastl-ars kastl-ars force-pushed the 20241220_rbac_if_condition branch from 1918c7c to 2228329 Compare January 7, 2025 07:20
@kastl-ars
Copy link
Contributor Author

Hi @kastl-ars Thank you for this. LGTM Due to my recent merge, we did add rbac like this which we would want to consider upon. My bad here, I should have seen it under my PR. Should we also have them under this if condition too, we are already gating them under a helm config?

Sorry, I am not quite sure what you mean?

Question is do we also want to have rbac.enabled flag gate the cluster role needed in openshift environment too (that looks exactly what this flag is intended to do) or we can have scenarios where a user wants to disable the rbac.enabled flag but want the required cluster role, etc for openshift environment.

In my opinion, as the OpenShift stuff needs to be enabled explicitly and there is already a value to enable/disable the creation of the clusterRoleBinding, I would say they can be left independent of each other.

https://github.com/opencost/opencost-helm-chart/blob/main/charts/opencost/values.yaml#L340
https://github.com/opencost/opencost-helm-chart/blob/main/charts/opencost/values.yaml#L342

The cluster I want to run OpenCost in does not allow me to create roles and rolebindings, so I will keep these settings disabled and have the cluster owner create them for me. But I will enable the general OpenShift integration.

@kastl-ars
Copy link
Contributor Author

Is there anything from blocking this from being merged and released? This prevents me from running OpenCost in one of my clusters, where my permissions do not allow creating roles and rolebindings. And the fix is simple...

@kastl-ars
Copy link
Contributor Author

Could someone have a look and merge this, please? It's really simple...

@mittal-ishaan
Copy link
Contributor

This will need a version bump. Otherwise all fine

@cliffcolvin
Copy link
Member

@mittal-ishaan you want to bump that rq and I'll approve and merge?

@mittal-ishaan
Copy link
Contributor

I am not sure if I might be able to add a commit in the PR as it is a fork. Should I go ahead creating a seperate PR with this change and version bump to get this in quickly?

@kastl-ars
Copy link
Contributor Author

Hi all, I just pushed a commit that changes the chart version to 1.43.1 (or should this be 1.44.0?).

Anything else missing?

Not sure about the --- markers in charts/opencost/templates/clusterrolebinding.yaml, shouldn't they be inside the if condition, too?

@mittal-ishaan
Copy link
Contributor

version bump is fine. I think the last --- maker outside the if condition in that file is not even necessary there and can be removed. I will remove it in another PR. or you can include that change in this PR too.

@kastl-ars
Copy link
Contributor Author

version bump is fine. I think the last --- maker outside the if condition in that file is not even necessary there and can be removed. I will remove it in another PR. or you can include that change in this PR too.

I just pushed a commit that moves it into the if condition. Or should I remove it completely?

@cliffcolvin cliffcolvin merged commit 131f9b3 into opencost:main Jan 16, 2025
1 check passed
@kastl-ars kastl-ars deleted the 20241220_rbac_if_condition branch January 16, 2025 12:21
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.

Creation of ClusterRole and Clusterrolebindings cannot be disabled
3 participants