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

[Bug] kube-rbac-proxy missing resource quota #1017

Closed
elysweyr opened this issue Apr 26, 2023 · 4 comments · Fixed by #1030
Closed

[Bug] kube-rbac-proxy missing resource quota #1017

elysweyr opened this issue Apr 26, 2023 · 4 comments · Fixed by #1030
Labels
feature-request requests a new feature that currently isn't implemented in the project v4 Major version 4 wontfix This will not be worked on

Comments

@elysweyr
Copy link

Describe the bug
kube-rbac-proxy is missing a resources quota.

Version
.4.10.0

To Reproduce

  1. Deploy a generic Grafana CRD
  2. Inspect operator-controller-manager deployment
  3. kube-rbac-proxy has an empty resources object

Expected behavior
kube-rbac-proxy has a specified resources quota

Suspect component/Location where the bug might be occurring
https://github.com/grafana-operator/grafana-operator/blob/e31c0250f91cfb0cff71d74976780440218d3b75/deploy/kustomize/base/deployment.yaml#L32

Runtime (please complete the following information):

  • Grafana Operator Version [e.g. v5.0.0]
  • Environment: OpenShift 4.X
  • Deployment type: deployed
@elysweyr elysweyr added bug Something isn't working needs triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Apr 26, 2023
elysweyr added a commit to elsysweyr/grafana-operator that referenced this issue Apr 26, 2023
elysweyr added a commit to elsysweyr/grafana-operator that referenced this issue Apr 26, 2023
elysweyr added a commit to elysweyr/grafana-operator that referenced this issue Apr 26, 2023
@NissesSenap
Copy link
Collaborator

I don't see the need for this feature. You can overwrite the value trough kustomize.

You mention version 4 in your issue. But the pr that you sent in is performing a change in the master branch. I know this isn't easy to know but any v4 changes should be done in the v4 branch as documented in the readme https://github.com/grafana-operator/grafana-operator/blob/e31c0250f91cfb0cff71d74976780440218d3b75/README.md?plain=1#L9

In version v5, we are leaning towards removing the metrics proxy since it add more complexity then it's worth.

@NissesSenap NissesSenap added wontfix This will not be worked on feature-request requests a new feature that currently isn't implemented in the project v4 Major version 4 and removed bug Something isn't working needs triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Apr 26, 2023
@NissesSenap NissesSenap closed this as not planned Won't fix, can't repro, duplicate, stale Apr 26, 2023
@elysweyr
Copy link
Author

elysweyr commented Apr 26, 2023

I don't see the need for this feature. You can overwrite the value trough kustomize.

In my opinion, it doesn't make sense for the following reason: many commercial clusters enforce setting resource quotas.

Why is no well-formed default value set for this one pod? I am aware that I can change all this via kustomize. But why should I manually, for all my environments via an additional kustomize configuration set values for a container of the operator-controller-manager, that doesn't change and should actually be supplied as default value.
Also, I find it very inconsistent that a quota is set for the manager container but not for the kube-rbac-proxy, there is no understandable sense for this in my opinion.

But the pr that you sent in is performing a change in the master branch. I know this isn't easy to know but any v4 changes should be done in the v4 branch as documented in the readme

My bad.

@NissesSenap
Copy link
Collaborator

You bring a fair point. But you could argue that the change you are doing is a breaking change. Especially if you add limits to the request. If the current users of kustomize are using a limit range that is smaller than your settings, they will get issues.

In general, we are also trying to do limited amount of changes to v4 right now due to the coming release of v5.
Since it is so easy for you to either fork or update the kustomize files, I would argue that this change isn't worth the potential hassle it can create even though I agree with you, it is an inconsistency.

We have also decided to remove this feature in v5 all together, which makes me want to change it in v4 even less.
Sorry that we can't accommodate you.

Btw if you have multiple grafana-operator instance to be able to support multiple grafana instance I would really suggest you look in to v5 straight away. It might be RC but it's rather stable, we might do a few minor API changes before v5.0 release but nothing strange.

@NissesSenap
Copy link
Collaborator

FYI #1020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request requests a new feature that currently isn't implemented in the project v4 Major version 4 wontfix This will not be worked on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants