vectordotdev / helm-charts

Helm charts for Vector.
https://vector.dev
Mozilla Public License 2.0
114 stars 89 forks source link

fix: rendering custom config tpl #346

Open kimxogus opened 1 year ago

kimxogus commented 1 year ago

There's {{ tpl (toYaml .Values.customConfig) . | indent 4 }}, but we can't actually use tpl becuase there's several points accessing and iterating child objects of .Values.customConfig. So I made them not to access or iterate child objects if customConfig is string.

kimxogus commented 1 year ago

Can anyone review this?

dsmith3197 commented 11 months ago

Hi @kimxogus, thank you for opening the pull request. To help me better understand and review this change, could you please provide an example?

kimxogus commented 11 months ago

Hi @dsmith3197 , this is a simple example of usage of tpl config. In various environments, you can use only necessary resources in each environments with this config. Without tpl, you have to add every resources because you cannot use conditional statements.

global:
  feature:
    a: true
    b: false

vector:
  customConfig: |
    sources:
      kafka:
        type: kafka
        bootstrap_servers: KAFKA:9092
        decoding:
          codec: json
        group_id: MY_GROUP
        topics:
          - common-topic
          {{- if .Values.global.feature.a }}
          - feature-a-topic
          {{- end }}
          {{- if .Values.global.feature.b }}
          - feature-b-topic
          {{- end }}
kimxogus commented 10 months ago

@dsmith3197 Is there any progress reviewing this pr?

kimxogus commented 10 months ago

@dsmith3197 Yes, this could break those container port related templates, but I think adding all those additional values for container ports will make this pr too large

kimxogus commented 7 months ago

@dsmith3197 Is this ok?

jszwedko commented 7 months ago

Hi @kimxogus ,

Apologies in the delay of review of this. I'm admittedly having trouble with the whitespace trying to test this out but templating like this should be fine for templating the list value rather than a string, no? I'm looking at https://helm.sh/docs/chart_template_guide/control_structures/ which shows an example of doing it to add a key/value to a map.

kimxogus commented 7 months ago

@jszwedko I just wanted to fix the broken tpl config. Also, templating in string config let you handle different configs in single template. as I mentioned above. Using list or key-value config, you have to copy and paste your values.yaml everywhere or write very complicated templates and conditions in your helm chart.

jszwedko commented 7 months ago

@jszwedko I just wanted to fix the broken tpl config.

Ah I see, Focusing in on this bit, can you explain the issue a bit more clearly? I'm still not seeing it. It seems to imply the customConfig option isn't usable, but that isn't the case and so it sounds like there is some specific issue it is causing?

kimxogus commented 7 months ago

@jszwedko Sorry, I was confused the context as it's 4months old. The original intention was to support string type customConfig because it's the best way to utilize tpl. In current helm templates, is there any way to implement conditional features like below? We are using our own helm chart wrapping vector's helm chart with those types of helm template, and deployed in various environments. I believe this kind of pattern is the most reusable and convenient way to manage various environments with a single template. If not, we have to write different values.yaml and edit in every helm releases(like adding every single topic names and additional configs in each values.yaml), which is not reusable and make us too hard to manage our data pipelines with vector.

global:
  feature:
    a: true
    b: false

vector:
  customConfig: |
    sources:
      kafka:
        type: kafka
        bootstrap_servers: KAFKA:9092
        decoding:
          codec: json
        group_id: MY_GROUP
        topics:
          - common-topic
          {{- if .Values.global.feature.a }}
          - feature-a-topic
          {{- end }}
          {{- if .Values.global.feature.b }}
          - feature-b-topic
          {{- end }}
jszwedko commented 7 months ago

Thanks @kimxogus ! My understanding is that it should be possible to template the YAML as-is like I mentioned in https://github.com/vectordotdev/helm-charts/pull/346#issuecomment-2071065355 (though admittedly I was still struggling with the formatting). Would that work for you?

jszwedko commented 7 months ago

E.g. like they show here:

data:
  myvalue: "Hello World"
  drink: {{ .Values.favorite.drink | default "tea" | quote }}
  food: {{ .Values.favorite.food | upper | quote }}
  {{ if eq .Values.favorite.drink "coffee" }}mug: "true"{{ end }}
kimxogus commented 7 months ago

The code {{ tpl (toYaml .Values.customConfig) . | indent 4 }} converts the yaml to string first, and then runs the tpl function. That means {{ if eq .Values.favorite.drink "coffee" }}mug: "true"{{ end }} won't work in toYaml function before tpl because it's go template format, not valid yaml format.

Helm raised this error with the template. Error: cannot load values.yaml: error converting YAML to JSON: yaml: line ___: did not find expected key

jszwedko commented 7 months ago

@kimxogus Ah I see. Would it also work to run the tpl function on the string before doing toYaml then?

kimxogus commented 7 months ago

Yes

jszwedko commented 7 months ago

Yes

👍 do you mind updating this PR to do that instead, then?

kimxogus commented 6 months ago

@jszwedko I've just read helm documents. tpl function generates string output, and toYaml function converts key-value map to string in yaml format. tpl can't be run before toYaml. toYaml should be before tpl as here.

DingGGu commented 4 months ago

Hi! Any updates for this PR? @jszwedko

kimxogus commented 4 months ago

@jszwedko Hi, toYaml's input is object and tpl consume and produces string, so it's not possible to run tpl before toYaml. Can we merge this?

jszwedko commented 4 months ago

Hey! Apologies, i still have to circle back to this PR. I'm still not really sure I understand the why change is necessary here but that is likely due to my lack of familiarity with Helm. I'll try to spend more time with this soon.