influxdata / tick-charts

A repository for Helm Charts for the full TICK Stack
Apache License 2.0
90 stars 74 forks source link

Cannot override configuration parameters (example doesn't work) #76

Open elpadrinoIV opened 5 years ago

elpadrinoIV commented 5 years ago
$ helm version
Client: &version.Version{SemVer:"v2.12.1", GitCommit:"02a47c7249b1fc6d8fd3b94e6b4babf9d818144e", GitTreeState:"clean"}
Server: &version.Version{SemVer:"v2.12.0", GitCommit:"d325d2a9c179b33af1a024cdb5a4472b6288016a", GitTreeState:"clean"}

Running with no parameters:

$ helm install --debug --dry-run    influx/telegraf-ds
...
USER-SUPPLIED VALUES:
{}

COMPUTED VALUES:
config:
  agent:
    collection_jitter: 0s
    debug: false
    flush_interval: 10s
    flush_jitter: 0s
    hostname: $HOSTNAME
    interval: 10s
    logfile: ""
    metric_batch_size: 1000
    metric_buffer_limit: 10000
    omit_hostname: false
    precision: ""
    quiet: false
    round_interval: true
  inputs:
  - kubernetes:
      bearer_token: /var/run/secrets/kubernetes.io/serviceaccount/token
      insecure_skip_verify: true
      url: https://$HOSTIP:10250
  - docker:
      docker_label_exclude:
      - annotation.kubernetes.io/*
      endpoint: unix:///var/run/docker.sock
      perdevice: true
      timeout: 5s
      total: false
  outputs:
  - influxdb:
      database: telegraf
      insecure_skip_verify: false
      password: ""
      retention_policy: ""
      timeout: 5s
      url: http://data-influxdb.tick:8086
      user_agent: telegraf
      username: ""
...
# Source: telegraf-ds/templates/configmap.yaml
apiVersion: v1
kind: ConfigMap
metadata:
  name: whimsical-cardinal-teleg
  labels:
    app: whimsical-cardinal-teleg
    chart: "telegraf-ds-1.9.1"
    release: "whimsical-cardinal"
    heritage: "Tiller"
data:
  telegraf.conf: |+

    [agent]
      collection_jitter = "0s"
      debug = false
      flush_interval = "10s"
      flush_jitter = "0s"
      hostname = "$HOSTNAME"
      interval = "10s"
      logfile = ""
      metric_batch_size = 1000
      metric_buffer_limit = 10000
      omit_hostname = false
      precision = ""
      quiet = false
      round_interval = true

    [[outputs.influxdb]]
      database = "telegraf"
      insecure_skip_verify = false
      password = ""
      retention_policy = ""
      timeout = "5s"
      url = "http://data-influxdb.tick:8086"
      user_agent = "telegraf"
      username = ""
    [[inputs.kubernetes]]
      bearer_token = "/var/run/secrets/kubernetes.io/serviceaccount/token"
      insecure_skip_verify = true
      url = "https://$HOSTIP:10250"
    [[inputs.docker]]
      docker_label_exclude = [
        "annotation.kubernetes.io/*"
      ]
      endpoint = "unix:///var/run/docker.sock"
      perdevice = true
      timeout = "5s"
      total = false

However, if I want to set the influxdb as in the example:

helm install --debug --dry-run  --set config.outputs.influxdb.url=http://foo.bar:8086   influx/telegraf-ds
...
2019/01/16 10:44:41 Warning: Merging destination map for chart 'telegraf-ds'. The destination item 'outputs' is a table and ignoring the source 'outputs' as it has a non-table value of: [map[influxdb:map[user_agent:telegraf username: database:telegraf insecure_skip_verify:false password: retention_policy: timeout:5s url:http://data-influxdb.tick:8086]]]
...
USER-SUPPLIED VALUES:
config:
  outputs:
    influxdb:
      url: http://foo.bar:8086

COMPUTED VALUES:
config:
  agent:
    collection_jitter: 0s
    debug: false
    flush_interval: 10s
    flush_jitter: 0s
    hostname: $HOSTNAME
    interval: 10s
    logfile: ""
    metric_batch_size: 1000
    metric_buffer_limit: 10000
    omit_hostname: false
    precision: ""
    quiet: false
    round_interval: true
  inputs:
  - kubernetes:
      bearer_token: /var/run/secrets/kubernetes.io/serviceaccount/token
      insecure_skip_verify: true
      url: https://$HOSTIP:10250
  - docker:
      docker_label_exclude:
      - annotation.kubernetes.io/*
      endpoint: unix:///var/run/docker.sock
      perdevice: true
      timeout: 5s
      total: false
  outputs:
    influxdb:
      url: http://foo.bar:8086
...
# Source: telegraf-ds/templates/configmap.yaml
apiVersion: v1
kind: ConfigMap
metadata:
  name: dandy-ladybug-telegraf-d
  labels:
    app: dandy-ladybug-telegraf-d
    chart: "telegraf-ds-1.9.1"
    release: "dandy-ladybug"
    heritage: "Tiller"
data:
  telegraf.conf: |+

    [agent]
      collection_jitter = "0s"
      debug = false
      flush_interval = "10s"
      flush_jitter = "0s"
      hostname = "$HOSTNAME"
      interval = "10s"
      logfile = ""
      metric_batch_size = 1000
      metric_buffer_limit = 10000
      omit_hostname = false
      precision = ""
      quiet = false
      round_interval = true

    [[outputs.url]]
    [[inputs.kubernetes]]
      bearer_token = "/var/run/secrets/kubernetes.io/serviceaccount/token"
      insecure_skip_verify = true
      url = "https://$HOSTIP:10250"
    [[inputs.docker]]
      docker_label_exclude = [
        "annotation.kubernetes.io/*"
      ]
      endpoint = "unix:///var/run/docker.sock"
      perdevice = true
      timeout = "5s"
      total = false

I think it's a helm issue, but the things is that the example doesn't work and it's key to get that working

elpadrinoIV commented 5 years ago

I think the issue has to do that helm changes the array to a hash.

I also think that using a map in the values.yml should avoid the issue.

jackzampolin commented 5 years ago

@elpadrinoIV would love to see a PR for this! Also as a note these charts haven't been substantially updated since around the 2.5 release of helm.

rawkode commented 5 years ago

@elpadrinoIV While less than ideal, you can mitigate this with:

helm install --debug --dry-run --set config.outputs.0.influxdb.url=http://foo.bar:8086 ./telegraf-ds

This would require all values set in the same fashion, which is pretty awkward.

The best approach would be to change the way we generate the config; but this would be quite the backwards compatibility break and wouldn't be possible until #74 is fixed.

elpadrinoIV commented 5 years ago

@rawkode Yes, that's what I had to do and that was the reason to open this issue :)

@jackzampolin I'll try to make a map version and submit a PR