Skip to content

feat(helm): Allow enabling user namespaces - #18661

Merged
Jayclifford345 merged 7 commits into
grafana:mainfrom
sebhoss:issue-18646
Aug 27, 2025
Merged

feat(helm): Allow enabling user namespaces#18661
Jayclifford345 merged 7 commits into
grafana:mainfrom
sebhoss:issue-18646

Conversation

@sebhoss

@sebhoss sebhoss commented Jul 30, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

What this PR does / why we need it:

This gates the .spec.hostUsers field behind a semver version check which verifies that a Kubernetes cluster is at least on version 1.33. If that is the case, it sets the field to true which disables user namespaces by default because cluster administrators need to prepare their cluster nodes first, before they are able to switch on user namespaces.

Which issue(s) this PR fixes:

Fixes #18646

Special notes for your reviewer:

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • Title matches the required conventional commits format, see here
    • Note that Promtail is considered to be feature complete, and future development for logs collection will be in Grafana Alloy. As such, feat PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
  • If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR
@sebhoss
sebhoss requested a review from a team as a code owner July 30, 2025 12:52
@sebhoss sebhoss mentioned this pull request Jul 30, 2025
Closed
jkroepke
jkroepke reviewed Jul 30, 2025
View reviewed changes

@jkroepke jkroepke left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, thanks for your PR.

Could you please

  • add feat(helm): as PR Title prefix
  • add a note to the CHANGELOG?
  • run make helm-docs?

And I miss tests/test-canary.yaml and ingester/statefulset-ingester.yaml
Comment thread production/helm/loki/Chart.yaml Outdated
Comment thread production/helm/loki/templates/admin-api/deployment-admin-api.yaml Outdated
Comment thread production/helm/loki/values.yaml Outdated
@JStickler JStickler changed the title Allow enabling user namespaces Jul 30, 2025
@JStickler JStickler changed the title fix(helm): Allow enabling user namespaces Jul 30, 2025
@sebhoss
sebhoss force-pushed the issue-18646 branch 3 times, most recently from 5e0a762 to 6cf5c92 Compare July 31, 2025 06:13
@sebhoss

sebhoss commented Jul 31, 2025

Copy link
Copy Markdown
Contributor Author

I think I've addresses all comments, rebased on top of latest main - please take another look & thanks for the review!
jkroepke
jkroepke reviewed Jul 31, 2025
View reviewed changes
Comment thread production/helm/loki/CHANGELOG.md Outdated
jkroepke
jkroepke reviewed Jul 31, 2025
View reviewed changes

@jkroepke jkroepke left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

thanks for your PR. I tested the PR locally and found some context issues.

Aside form that, my proposed ne .testUsers nil seems not working, but kindIs "bool" .testUsers does. Could you please change the logic? I tested the kindIs function and it works in my case.
Comment thread production/helm/loki/templates/loki-canary/daemonset.yaml Outdated
Comment thread production/helm/loki/templates/memcached/_memcached-statefulset.tpl Outdated
Comment thread production/helm/loki/templates/admin-api/deployment-admin-api.yaml Outdated
jkroepke
jkroepke reviewed Jul 31, 2025
View reviewed changes

@jkroepke jkroepke left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Lets wait for CI.
@jkroepke jkroepke mentioned this pull request Aug 7, 2025
Closed
@sebhoss

sebhoss commented Aug 11, 2025

Copy link
Copy Markdown
Contributor Author

rebased on latest main
@JStickler

JStickler commented Aug 18, 2025

Copy link
Copy Markdown
Contributor

@sebhoss could you fix the merge conflicts please?
@sebhoss
feat(helm): Allow enabling user namespaces
a8d95f7 
This gates the ``.spec.hostUsers` field behind a semver version check which verifies that a Kubernetes cluster is at least on version 1.33. If that is the case, it sets the field to `true` which disables user namespaces by default because cluster administrators need to prepare their cluster nodes first, before they are able to switch on user namespaces.

Fixes grafana#18646

Signed-off-by: Sebastian Hoß <seb@xn--ho-hia.de>
@sebhoss

sebhoss commented Aug 18, 2025

Copy link
Copy Markdown
Contributor Author

rebased on latest main & executed make helm-docs
Jayclifford345 and others added 3 commits August 19, 2025 10:47
@Jayclifford345
Merge branch 'main' into issue-18646
ab4975c 
Signed-off-by: Jay Clifford <45856600+Jayclifford345@users.noreply.github.com>
@JStickler
Merge branch 'main' into issue-18646
37d581e
@Jayclifford345
Merge branch 'main' into issue-18646
e411825 
Signed-off-by: Jay Clifford <45856600+Jayclifford345@users.noreply.github.com>
@Jayclifford345
Jayclifford345 enabled auto-merge (squash) August 27, 2025 08:31
@Jayclifford345

Jayclifford345 commented Aug 27, 2025

Copy link
Copy Markdown
Contributor

Set to auto merge when tests complete
@Jayclifford345
Merge branch 'main' into issue-18646
68f44b4
@Jayclifford345
Merge branch 'main' into issue-18646
554ec65 
Signed-off-by: Jay Clifford <45856600+Jayclifford345@users.noreply.github.com>
@Jayclifford345
Jayclifford345 merged commit d0b39e5 into grafana:main Aug 27, 2025
80 checks passed
@loki-gh-app loki-gh-app Bot mentioned this pull request Apr 2, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/L

4 participants

@sebhoss @JStickler @Jayclifford345 @jkroepke