fix(helm): only default bucket names when using minio

[trevorwhitney]

What this PR does / why we need it:

Only default bucket names in helm when using minio.

[slim-bean]

instead of deleting this, maybe comment it out with FIXME for the names?

[slim-bean]

LGTM

[luong-komorebi]

@trevorwhitney upgrading from 6.0 to 6.1 and the error below happens.

There is a high chance this PR breaks current installations where the defaults for storage are not defined
We should have some notes in the changelog to warn operators about this

│ Error: template: loki/templates/ruler/statefulset-ruler.yaml:24:12: executing "loki/templates/ruler/statefulset-ruler.yaml" at <include "loki.config.checksum" .>: error calling include: template: loki/templates/_helpers.tpl:978:20: executing "loki.config.checksum" at <include (print .Template.BasePath "/config.yaml") .>: error calling include: template: loki/templates/config.yaml:19:7: executing "loki/templates/config.yaml" at <include "loki.calculatedConfig" .>: error calling include: template: loki/templates/_helpers.tpl:461:24: executing "loki.calculatedConfig" at <tpl .Values.loki.config .>: error calling tpl: error during tpl function execution for "{{- if .Values.enterprise.enabled}}\n{{- tpl .Values.enterprise.config . }}\n{{- else }}\nauth_enabled: {{ .Values.loki.auth_enabled }}\n{{- end }}\n\n{{- with .Values.loki.server }}\nserver:\n  {{- toYaml . | nindent 2}}\n{{- end}}\n\nmemberlist:\n{{- if .Values.loki.memberlistConfig }}\n  {{- toYaml .Values.loki.memberlistConfig | nindent 2 }}\n{{- else }}\n{{- if .Values.loki.extraMemberlistConfig}}\n{{- toYaml .Values.loki.extraMemberlistConfig | nindent 2}}\n{{- end }}\n  join_members:\n    - {{ include \"loki.memberlist\" . }}\n    {{- with .Values.migrate.fromDistributed }}\n    {{- if .enabled }}\n    - {{ .memberlistService }}\n    {{- end }}\n    {{- end }}\n{{- end }}\n\n{{- with .Values.loki.ingester }}\ningester:\n  {{- tpl (. | toYaml) $ | nindent 4 }}\n{{- end }}\n\n{{- if .Values.loki.commonConfig}}\ncommon:\n{{- toYaml .Values.loki.commonConfig | nindent 2}}\n  storage:\n  {{- include \"loki.commonStorageConfig\" . | nindent 4}}\n{{- end}}\n\n{{- with .Values.loki.limits_config }}\nlimits_config:\n  {{- tpl (. | toYaml) $ | nindent 4 }}\n{{- end }}\n\nruntime_config:\n  file: /etc/loki/runtime-config/runtime-config.yaml\n\n{{- with .Values.chunksCache }}\n{{- if .enabled }}\nchunk_store_config:\n  chunk_cache_config:\n    default_validity: {{ .defaultValidity }}\n    background:\n      writeback_goroutines: {{ .writebackParallelism }}\n      writeback_buffer: {{ .writebackBuffer }}\n      writeback_size_limit: {{ .writebackSizeLimit }}\n    memcached:\n      batch_size: {{ .batchSize }}\n      parallelism: {{ .parallelism }}\n    memcached_client:\n      addresses: dnssrvnoa+_memcached-client._tcp.{{ template \"loki.fullname\" $ }}-chunks-cache.{{ $.Release.Namespace }}.svc\n      consistent_hash: true\n      timeout: {{ .timeout }}\n      max_idle_conns: 72\n{{- end }}\n{{- end }}\n\n{{- if .Values.loki.schemaConfig }}\nschema_config:\n{{- toYaml .Values.loki.schemaConfig | nindent 2}}\n{{- end }}\n\n{{- if .Values.loki.useTestSchema }}\nschema_config:\n{{- toYaml .Values.loki.testSchemaConfig | nindent 2}}\n{{- end }}\n\n{{ include \"loki.rulerConfig\" . }}\n\n{{- if or .Values.tableManager.retention_deletes_enabled .Values.tableManager.retention_period }}\ntable_manager:\n  retention_deletes_enabled: {{ .Values.tableManager.retention_deletes_enabled }}\n  retention_period: {{ .Values.tableManager.retention_period }}\n{{- end }}\n\nquery_range:\n  align_queries_with_step: true\n  {{- with .Values.loki.query_range }}\n  {{- tpl (. | toYaml) $ | nindent 4 }}\n  {{- end }}\n  {{- if .Values.resultsCache.enabled }}\n  {{- with .Values.resultsCache }}\n  cache_results: true\n  results_cache:\n    cache:\n      default_validity: {{ .defaultValidity }}\n      background:\n        writeback_goroutines: {{ .writebackParallelism }}\n        writeback_buffer: {{ .writebackBuffer }}\n        writeback_size_limit: {{ .writebackSizeLimit }}\n      memcached_client:\n        consistent_hash: true\n        addresses: dnssrvnoa+_memcached-client._tcp.{{ template \"loki.fullname\" $ }}-results-cache.{{ $.Release.Namespace }}.svc\n        timeout: {{ .timeout }}\n        update_interval: 1m\n  {{- end }}\n  {{- end }}\n\n{{- with .Values.loki.storage_config }}\nstorage_config:\n  {{- tpl (. | toYaml) $ | nindent 4 }}\n{{- end }}\n\n{{- with .Values.loki.query_scheduler }}\nquery_scheduler:\n  {{- tpl (. | toYaml) $ | nindent 4 }}\n{{- end }}\n\n{{- with .Values.loki.compactor }}\ncompactor:\n  {{- tpl (. | toYaml) $ | nindent 4 }}\n{{- end }}\n\n{{- with .Values.loki.analytics }}\nanalytics:\n  {{- tpl (. | toYaml) $ | nindent 4 }}\n{{- end }}\n\n{{- with .Values.loki.querier }}\nquerier:\n  {{- tpl (. | toYaml) $ | nindent 4 }}\n{{- end }}\n\n{{- with .Values.loki.index_gateway }}\nindex_gateway:\n  {{- tpl (. | toYaml) $ | nindent 4 }}\n{{- end }}\n\n{{- with .Values.loki.frontend }}\nfrontend:\n  {{- tpl (. | toYaml) $ | nindent 4 }}\n{{- end }}\n\n{{- with .Values.loki.frontend_worker }}\nfrontend_worker:\n  {{- tpl (. | toYaml) $ | nindent 4 }}\n{{- end }}\n\n{{- with .Values.loki.distributor }}\ndistributor:\n  {{- tpl (. | toYaml) $ | nindent 4 }}\n{{- end }}\n\ntracing:\n  enabled: {{ .Values.loki.tracing.enabled }}\n": template: loki/templates/ruler/statefulset-ruler.yaml:37:6: executing "loki/templates/ruler/statefulset-ruler.yaml" at <include "loki.commonStorageConfig" .>: error calling include: template: loki/templates/_helpers.tpl:228:19: executing "loki.commonStorageConfig" at <$.Values.loki.storage.bucketNames.chunks>: nil pointer evaluating interface {}.chunks

[grafanabot]

The backport to helm-5.47.3 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new branch
git switch --create backport-12548-to-helm-5.47.3 origin/helm-5.47.3
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x 2e32ec52d8766c0a5a75be30585402f1dce52cc5

When the conflicts are resolved, stage and commit the changes:

git add . && git cherry-pick --continue

If you have the GitHub CLI installed:

# Push the branch to GitHub:
git push --set-upstream origin backport-12548-to-helm-5.47.3
# Create the PR body template
PR_BODY=$(gh pr view 12548 --json body --template 'Backport 2e32ec52d8766c0a5a75be30585402f1dce52cc5 from #12548{{ "\n\n---\n\n" }}{{ index . "body" }}')
# Create the PR on GitHub
echo "${PR_BODY}" | gh pr create --title "chore: [helm-5.47.3] fix(helm): only default bucket names when using minio" --body-file - --label "size/M" --label "area/helm" --label "type/docs" --label "type/bug" --label "product-approved" --label "backport" --base helm-5.47.3 --milestone helm-5.47.3 --web

Or, if you don't have the GitHub CLI installed (we recommend you install it!):

# Push the branch to GitHub:
git push --set-upstream origin backport-12548-to-helm-5.47.3

# Create a pull request where the `base` branch is `helm-5.47.3` and the `compare`/`head` branch is `backport-12548-to-helm-5.47.3`.

# Remove the local backport branch
git switch main
git branch -D backport-12548-to-helm-5.47.3

[trevorwhitney]

@luong-komorebi would you mind sharing a bit of information about your install? I think the goal here is to intentionally break any installation using the default buckets with a cloud provider as you likely don't own those buckets and don't mean to send data there? Was your installation using the defaults, and if so, were you only using it to query recent data?

[Farfaday]

Hi @trevorwhitney

Commenting out loki.storage.bucketNames in values.yaml currently breaks helm lint, as the keys are not defined anymore but are called 9 times in the _helpers.tpl file.

Helm lint create errors of type nil pointer evaluating interface.

I would propose adding a default value, something like

-  bucket_name: {{ $.Values.loki.storage.bucketNames.ruler }}
+  bucket_name: {{ $.Values.loki.storage.bucketNames.ruler | default "ruler" }}

What do you think? I can open an MR if you think that makes sense.

Thank you!

[luong-komorebi]

@trevorwhitney sorry Trevor, I was late to this due to being busy.

Yes my installation was pretty much default. However, I did have storage config inside which I define an s3 bucket name

# something like this
  storage_config:
    aws:
      bucketnames: abc-prod-logs
      region: us-west-2
      s3forcepathstyle: true

and I dont configure other things like storage.type or storage.bucketNames

this breaks after I upgraded

Later I do have to configure other things so the app can work again ( cant recall what I configured, but it seems like I needed to add :

        storage = {
          type = "s3"
          bucketNames = {
            chunks = "chunks"
            ruler  = "ruler"
            admin  = "admin"
          }
        }
# and 

      minio = { enabled = false }

) because these are what caused the app to break. I think there are some defaults that the upgrade fails to handle or notice maintainers

[thiagowfx]

@Farfaday Do you plan to open a PR? :)