-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
[tempo] Fix tempo permissions after update to v1.9.0 #3161
[tempo] Fix tempo permissions after update to v1.9.0 #3161
Conversation
08aa3e7
to
9e0af9d
Compare
Any news on the ETA of this fix? |
if it is blocking you, you can also just apply it to your own values.yaml. Not sure when someone will take a look at the PR / if there is something I can do to speed this up. |
Unfortunately tempo is still producing "permission denied" errors after applying this fix to values.yaml.
Is there anything else I have to change? Supporting the necessary chown -R on /var/tempo via initContainer in the Helm chart would be really helpful. |
Hmm in my case there was no issue with permissions after that, but my setup is as simple as it gets. Would you mind pasting your configuration here as well?
If that fixes the issue for you, good idea. Feel free to make a PR with that or post the diff here and I will add it here. |
The change looks good to me. We need to update the readme and bump the chart version to get the lint to pass. Additionally, I'd like to see some output @nrekretep. You may be running into the issue that made me question if this was enough of a change to resolve the issue. Please give the output from within the container of the following: |
3e91e53
to
aedc4c3
Compare
I bumped the versions, anything else to do in the readme? |
For the readme, I usually do |
The CI is failing to install the chart for a field incompatibility, but I've not the time to dig on this. Could you take a look and see if that can be reproduced locally? |
Hmm I expected that adding the values there would be equivalent to adding them locally in my values.yaml, weird that this fails in CI I found other people running into this error here: minio/minio#16521 |
Its possible the version of k8s that CI is running is too old, but that's a guess. |
I think the cause of the failing CI is not the k8s version used by kind. This PR suggests to set the This will modify the If you would use the top level |
i see, my bad. This is now changing the top level securityContext, please trigger CI again |
We are running into the same issue in the tempo-operator. Setting securityContext:
fsGroup: 10001
runAsGroup: 10001
runAsNonRoot: true
runAsUser: 10001 on ingester statefulset pod spec (nor container, and keeping fsGroup on pod) didn't work
Logs:
|
The |
I think it would be good to have an option to allow an init container to be added so that folks have the option to specify a chown if necessary. This would help accommodate the wide range of scenarios. Perhaps that could be added to this PR, but what do folks think? |
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.
@StefanLobbenmeierObjego Could you please bump chart version to fix CI lint.
6c98146
to
f196519
Compare
f196519
to
49be3d1
Compare
Signed-off-by: Stefan Lobbenmeier <stefan.lobbenmeier@objego.de>
49be3d1
to
08be426
Compare
bumped the chart version, also squashed all those formatting commits now to avoid having that noise on main |
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.
I'm somewhat inclined to only want the fsGroup
default change here, but I think what is here is still an improvement to the situation when persistence is enabled.
This PR contains the following updates: | Package | Update | Change | |---|---|---| | [tempo](https://grafana.net) ([source](https://togithub.com/grafana/helm-charts)) | patch | `1.10.2` -> `1.10.3` | --- ### Release Notes <details> <summary>grafana/helm-charts (tempo)</summary> ### [`v1.10.3`](https://togithub.com/grafana/helm-charts/releases/tag/tempo-1.10.3) [Compare Source](https://togithub.com/grafana/helm-charts/compare/tempo-1.10.2...tempo-1.10.3) Grafana Tempo Single Binary Mode #### What's Changed - \[tempo] Fix tempo permissions after update to v1.9.0 by [@​StefanLobbenmeierObjego](https://togithub.com/StefanLobbenmeierObjego) in [https://github.com/grafana/helm-charts/pull/3161](https://togithub.com/grafana/helm-charts/pull/3161) #### New Contributors - [@​StefanLobbenmeierObjego](https://togithub.com/StefanLobbenmeierObjego) made their first contribution in [https://github.com/grafana/helm-charts/pull/3161](https://togithub.com/grafana/helm-charts/pull/3161) **Full Changelog**: grafana/helm-charts@loki-distributed-0.79.3...tempo-1.10.3 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://togithub.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC4yNS4xIiwidXBkYXRlZEluVmVyIjoiMzguMjUuMSIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsicmVub3ZhdGUvaGVsbSIsInR5cGUvcGF0Y2giXX0=--> Co-authored-by: lumiere-bot[bot] <98047013+lumiere-bot[bot]@users.noreply.github.com>
Tempo v2.5.0-rc.0 is now being run as non-root, see grafana/tempo#2265. Change default values.yaml to avoid having every user apply this to their configuration.