[kube-prometheus-stack] fixes thanos configs

[thameezb]

What this PR does / why we need it

Updates thanos configs to allow the usage of existing secrets or the option of creating a new secret object
Full design discussion can be found in the issue

Which issue this PR fixes

fixes #3869

Special notes for your reviewer

Checklist

• DCO signed
• Chart Version bumped
• Title of the PR starts with chart name (e.g. [prometheus-couchdb-exporter])

[GMartinez-Sisti]

Thanks @thameezb! Being a breaking change, we also need to include updating steps on the README.md file. Check this example to see what's expected.

[thameezb]

Thanks @thameezb! Being a breaking change, we also need to include updating steps on the README.md file. Check this example to see what's expected.

Added!

[GMartinez-Sisti]

@QuentinBisson,other than the last suggestion above, for me this is good to go. Can you please take a look and let us know what you think? Also if we are not forgetting anything regarding the breaking change process. Thanks 🙏

[rouke-broersma]

@GMartinez-Sisti @QuentinBisson this does not include the same change for thanos sidecar. This makes the configuration of the objectStorageConfig inconsistent between different products.

helm-charts/charts/kube-prometheus-stack/values.yaml

Line 3434 in 6fb47fb

thanos: {}

[QuentinBisson]

@rouke-broersma this is a good finding yes :) @thameezb could you address this ?

[QuentinBisson]

@GMartinez-Sisti Other than the issue brought up by @rouke-broersma I'm fine with the rest :)

[rouke-broersma]

I'm getting an error on thanos sidecar with this config:

  prometheusSpec:
    thanos:
      objectStorageConfig:
        existingSecret:
          key: config-file.yaml
          name: thanos-secret

Error: YAML parse error on kube-prometheus-stack/templates/prometheus/prometheus.yaml: error converting YAML to JSON: yaml: line 91: did not find expected key
helm.go:84: [debug] error converting YAML to JSON: yaml: line 91: did not find expected key

[thameezb]

I'm getting an error on thanos sidecar with this config:

  prometheusSpec:
    thanos:
      objectStorageConfig:
        existingSecret:
          key: config-file.yaml
          name: thanos-secret

Error: YAML parse error on kube-prometheus-stack/templates/prometheus/prometheus.yaml: error converting YAML to JSON: yaml: line 91: did not find expected key
helm.go:84: [debug] error converting YAML to JSON: yaml: line 91: did not find expected key

fixed and tested for all 3 permutations

[thameezb]

@thameezb I think we also need to remove thanosRuler.thanosRulerSpec.objectStorageConfigFile as it doesn't make sense before and even more - now. It just adds "confusion", but doesn't help in any way. What do you think? @GMartinez-Sisti @QuentinBisson

removed

[rouke-broersma]

I can confirm that the config now looks correct

[GMartinez-Sisti]

Thank you everyone for the help! 🚀 I'll let it simmer for a couple of hours and do some local testing in the meantime. If no one finds anything we can merge it.

[dragoangel]

@thameezb @GMartinez-Sisti one thing was missed:

  # Source: kube-prometheus-stack/templates/prometheus/prometheus.yaml
  apiVersion: monitoring.coreos.com/v1
  kind: Prometheus
  ...
    thanos:
-     objectStorageConfig:
-       key: objstore.yml
-       name: thanos-monitoring-config

here is a patch for a fix:

diff --git "a/charts/kube-prometheus-stack/templates/prometheus/prometheus.yaml" "b/charts/kube-prometheus-stack/templates/prometheus/prometheus.yaml"
index 238bc6e3..eaa4b900
--- "a/charts/kube-prometheus-stack/templates/prometheus/prometheus.yaml"
+++ "b/charts/kube-prometheus-stack/templates/prometheus/prometheus.yaml"
@@ -356,14 +356,11 @@ spec:
 {{- if not .Values.prometheus.agentMode }}
 {{- if .Values.prometheus.prometheusSpec.thanos }}
   thanos:
-{{- with (omit .Values.prometheus.prometheusSpec.thanos "objectStorageConfig")}}
-{{ toYaml . | indent 4 }}
-{{- end }}
-{{- if .Values.prometheus.prometheusSpec.thanos.objectStorageConfig.existingSecret }}
+{{- if .Values.thanosRuler.thanosRulerSpec.objectStorageConfig.existingSecret }}
     objectStorageConfig:
-      key: "{{.Values.prometheus.prometheusSpec.thanos.objectStorageConfig.existingSecret.key }}"
-      name: "{{.Values.prometheus.prometheusSpec.thanos.objectStorageConfig.existingSecret.name }}"
-{{- else if .Values.prometheus.prometheusSpec.thanos.objectStorageConfig.secret}}
+      key: "{{.Values.thanosRuler.thanosRulerSpec.objectStorageConfig.existingSecret.key }}"
+      name: "{{.Values.thanosRuler.thanosRulerSpec.objectStorageConfig.existingSecret.name }}"
+{{- else if .Values.thanosRuler.thanosRulerSpec.objectStorageConfig.secret }}
     objectStorageConfig:
       key: object-storage-configs.yaml
       name: {{ template "kube-prometheus-stack.fullname" . }}-prometheus

[dragoangel]

@thameezb @GMartinez-Sisti also I think we don't need charts\kube-prometheus-stack\templates\prometheus\secret.yaml file, I don't see a point in having a separate secret with the same data as for thanosRuler but in prometheus. It will just create a blank secret at this point, as .Values.prometheus.prometheusSpec.thanos.objectStorageConfig wrong in the same way as above ^^

[thameezb]

@thameezb @GMartinez-Sisti also I think we don't need charts\kube-prometheus-stack\templates\prometheus\secret.yaml file, I don't see a point in having a separate secret with the same data as for thanosRuler but in prometheus. It will just create a blank secret at this point, as .Values.prometheus.prometheusSpec.thanos.objectStorageConfig wrong in the same way as above ^^

Not sure if I agree. The above patch supposes that one has thanosruler configured/deployed if one wishes to add thanos side car info to prometheus. Is this always true?

Could I be using the side car (therefore configuring prometheus.prometheusSpec.thanos without configuring thanosRuler.thanosRulerSpec? I think so

Additionally there is other config being passed in .Values.prometheus.prometheusSpec.thanos beyond objectStorageConfig

[dragoangel]

@thameezb @GMartinez-Sisti also I think we don't need charts\kube-prometheus-stack\templates\prometheus\secret.yaml file, I don't see a point in having a separate secret with the same data as for thanosRuler but in prometheus. It will just create a blank secret at this point, as .Values.prometheus.prometheusSpec.thanos.objectStorageConfig wrong in the same way as above ^^

Not sure if I agree. The above patch supposes that one has thanosruler configured/deployed if one wishes to add thanos side car info to prometheus. Is this always true?

Could I be using the side car (therefore configuring prometheus.prometheusSpec.thanos without configuring thanosRuler.thanosRulerSpec? I think so

Additionally there is other config being passed in .Values.prometheus.prometheusSpec.thanos beyond objectStorageConfig

Oh, sorry, I missed that there was also section. My fail, all okay, thanks :)

[dragoangel]

Another thing that kube-prometheus used not lucky version of Thanos Ruler v0.32.3 (while Thanos Sidecar is up to date): https://github.com/thanos-io/thanos/releases/tag/v0.32.4 which fix critical issue with object storage, created PR: https://github.com/prometheus-community/helm-charts/pull/3935/files

[rouke-broersma]

Another thing that kube-prometheus used not lucky version of Thanos Ruler v0.32.3 (while Thanos Sidecar is up to date): https://github.com/thanos-io/thanos/releases/tag/v0.32.4 which fix critical issue with object storage, created PR: https://github.com/prometheus-community/helm-charts/pull/3935/files

Afaik the critical fixes relate to uploading blocks so thanos ruler shouldn't be affected.

[thameezb]

Another thing that kube-prometheus used not lucky version of Thanos Ruler v0.32.3 (while Thanos Sidecar is up to date): https://github.com/thanos-io/thanos/releases/tag/v0.32.4 which fix critical issue with object storage, created PR: https://github.com/prometheus-community/helm-charts/pull/3935/files

Welcome to create another PR to address this (chart owners can speak to the process of an application update - as I'm not sure)

[dragoangel]

Another thing that kube-prometheus used not lucky version of Thanos Ruler v0.32.3 (while Thanos Sidecar is up to date): https://github.com/thanos-io/thanos/releases/tag/v0.32.4 which fix critical issue with object storage, created PR: https://github.com/prometheus-community/helm-charts/pull/3935/files

Afaik the critical fixes relate to uploading blocks so thanos ruler shouldn't be affected.

Actually, Thanos Ruler is affected as it can "record" data, and when you use sharded Prometheus you will always use Thanos Ruler for all "recordings" and not Prometheus, as in sharded setup Prometheus doesn't see "all view" and he doesn't look up for global data via Thanos Query as Thanos Ruler does. Most alerts also would be placed in Thanos Ruler, in my case only Thanos-related alerts are in Prometheus, so I get triggered right after updating to Thanos Ruler v0.32.3 as it fails to upload data. PR already merged anyway.