Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Promtail] extraClientConfigs breaking change #1214

Closed
dungdm93 opened this issue Apr 11, 2022 · 11 comments
Closed

[Promtail] extraClientConfigs breaking change #1214

dungdm93 opened this issue Apr 11, 2022 · 11 comments

Comments

@dungdm93
Copy link
Contributor

Before, I have one lokiAddress and I use extraClientConfigs to add extra config/labels before send logs to Loki server.
image
image

After #1119, extraClientConfigs seem used to add extra clients, instead of extra config of existing client. So that will breaking my use-case:
image

@dungdm93
Copy link
Contributor Author

I can make a PR for this, but I don't known which one you prefer:

  1. Rollback [promtail] make extra client configs an array of configs #1119 and add another key for this like extraClients
  2. make lokiAddress become a map, instead of string
lokiAddress:
   url: http://loki-gateway/loki/api/v1/push
   // more config can be added here

then:

    clients:
      - {{ toYaml .Values.config.lokiAddress }}

@tobygale
Copy link

We've hit the same issue.
We use extraClientConfigs to set the tenant_id in places where we cannot inject the X-Scope-OrgID header.
Just had to rollback to the previous version.

@jimgus
Copy link

jimgus commented Apr 12, 2022

Hit this issue as well when trying to add tls_config in extraClientConfigs

@secustor
Copy link
Contributor

secustor commented Apr 12, 2022

We have ran into this today too. We use it to set the external labels and authentication methods .

I suggest the introduction of a new variable instead of replacing lokiAddress with a map. That would be a little misleading as what the variable contains.

lokiClientConfig:
  external_labels:
     aLabel: aValue
    clients:
      - url: {{ .Values.config.lokiAddress }}
        {{ toYaml .Values.config.lokiClientConfig | nindent 4 }}
      {{- with .Values.config.snippets.extraClientConfigs }}
      {{- toYaml . | nindent 2 }}
      {{- end }}

@ts-mini
Copy link

ts-mini commented Apr 12, 2022

hit trying to upgrade to 2.5.0, +1 to the first option

Rollback #1119 and add another key for this like extraClients

@fabienpelletier
Copy link

same, I'm using the extraClientConfigs to set basic_auth and this change breaks it.

@ambis
Copy link

ambis commented Apr 15, 2022

same, I'm using the extraClientConfigs to set basic_auth and this change breaks it.

Same here. I have to re-write the whole file, which is not update proof.

    extraEnv:
      - name: PROMTAIL_PASSWORD
        valueFrom:
          secretKeyRef:
            name: some-pw
            key: some-key
    extraArgs:
      - -config.expand-env=true
    config:
      lokiAddress: http://loki-loki-distributed-gateway/loki/api/v1/push
      file: |
        server:
          log_level: {{ .Values.config.logLevel }}
          http_listen_port: {{ .Values.config.serverPort }}

        clients:
          - url: {{ tpl .Values.config.lokiAddress . }}
            basic_auth:
              username: promtail
              password: ${PROMTAIL_PASSWORD}
          {{- with .Values.config.snippets.extraClientConfigs }}
          {{- toYaml . | nindent 2 }}
          {{- end }}

        positions:
          filename: /run/promtail/positions.yaml

        scrape_configs:
          {{- tpl .Values.config.snippets.scrapeConfigs . | nindent 2 }}
          {{- tpl .Values.config.snippets.extraScrapeConfigs . | nindent 2 }}

@ducnm0711
Copy link

Same issue, rollback to Chart version 3.11.0 and use image.tag for Promtail 2.5.0

fyi #1242 is working on a fix for #1119

@trevorwhitney
Copy link
Contributor

@dungdm93 sorry for the late response here, a PR would be welcome, and I think we want to go with something similar to option 2.

My vote would be to remove both lokiAddress and extraClientConfigs, and just expose a single array value like clientConfigs, that takes an array of objects containing client configs. The lokiAddress construct seems too strongly predicated on the fact you will only ever have one client.

WDYT?

@dungdm93
Copy link
Contributor Author

@trevorwhitney That great, I create a PR at #1425.
Could you please help me review it.

@trevorwhitney
Copy link
Contributor

Merged #1425

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants