fix(helm): Add validation for loki.storage.bucketNames

[horihel]

What this PR does / why we need it:

Using root context in _helpers.tpl triggers problems in helm, during render and especially during lint (helm/helm#12798)

This change works around the problem by storing the objects in a variable before the chart changes scope. I've added "required" keywords to retain the original behavior (chart fails to render if bucketNames is unset).

Which issue(s) this PR fixes:
Fixes #13284
Fixes #13564
Fixes #14843

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
• For Helm chart changes bump the Helm chart version in production/helm/loki/Chart.yaml and update production/helm/loki/CHANGELOG.md and production/helm/loki/README.md. Example PR
• 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

[CLAassistant]

All committers have signed the CLA.

[horihel]

Dear review team. It's now a month in and there was no reaction to this PR. I'm still available to discuss it, but I will pause maintaining it until someone shows up willing to review.

[puchalski-darwin]

why proceed with this? A lot of people is getting this lint error and needs making adjustment for bypass this.
Just to know. Any maintainer?

[xDmitriev]

Dear maintainers, could you please review and merge it? We are all experiencing the same issue and have to keep a local copy with the fix, which brings additional complexity during each upgrade.

[trevorwhitney]

I'm fine with this change but it looks pretty out of date. Can you please update this branch against main. I'd like to see the helm diff CI job results to make sure this doesn't change anything (looks like it should be a no-op on the diff). Also the changelog entry doesn't need a version header, just put in in the unversioned top section, the correct version header will be applied automatically.

[horihel]

thanks for picking this up - i'll update the PR as soon as time allows

[horihel]

updated the PR - i've added the check for bucketNames to the use_thanos_objstorage path, though I can't test that locally.

[horihel]

@trevorwhitney i know you're busy, but if nobody even permits the checks to run, this PR will again fall behind main...

[jkroepke]

Hi @horihel

I'm part of new loki helm maintainer group. Once PR updated again, I can take a look into this.

Running the chart without an extra values files currently results into a error

error calling include: template: loki/templates/_helpers.tpl:233:19: executing "loki.commonStorageConfig" at <$.Values.loki.storage.bucketNames.chunks>: nil pointer evaluating interface {}.chunks

I hope this PR can fixed this issue as well.

[horihel]

Hi @jkroepke - Thanks for picking this up.
Updated the PR - I'm still not sure if that's the correct solution because the cause of the problem is still a bug in helm. But it should at least make it lint/work again.

[jkroepke]

I looked through the changes and I guess it might be better to use the validate.yaml.

Adding

{{- if not (hasKey .Values.loki.storage.bucketNames "chunks") }}
{{- fail "Please define loki.storage.bucketName.chunks" }}
{{- end }}

{{- if not (hasKey .Values.loki.storage.bucketNames "ruler") }}
{{- fail "Please define loki.storage.bucketName.ruler" }}
{{- end }}

to the validate.yaml would resolved this as well. WDYT?

And please change might also clear up: https://github.com/grafana/loki/issues/13564 (not sure) to fixes https://github.com/grafana/loki/issues/13564

[jkroepke]

LGTM - maybe change the PR Title as well.

[jkroepke]

LGTM

[jkroepke]

LGTM - Nice! Thanks a lot!

[jkroepke]

Hey @horihel,

sound like we have an bug here.

helm template . -f scenarios/simple-scalable-aws-kube-irsa-values.yaml 
Error: execution error at (loki/templates/validate.yaml:54:4): Please define loki.storage.bucketName.admin

but the admin bucket is set.

loki/production/helm/loki/scenarios/simple-scalable-aws-kube-irsa-values.yaml

Line 9 in 49d6d79

admin: aws-s3-admin-bucket

[jkroepke]

LGTM, lets check the CI.