Terraform-provider-helm: helm_release "set" regression when setting a list

[mwconceicao]

Terraform Version

Terraform v0.11.13
+ provider.aws v2.8.0
+ provider.helm v0.9.1
+ provider.kubernetes v1.6.2
+ provider.random v2.1.1

Affected Resource(s)

• helm_release

Terraform Configuration Files

provider "helm" {}

locals {
  mykeys = ["aaa", "bbb", "ccc"]
}

data "helm_repository" "stable" {
  name = "stable"
  url  = "https://kubernetes-charts.storage.googleapis.com"
}

resource "helm_release" "app" {
  repository = "${data.helm_repository.stable.name}"
  name       = "myapp"
  chart      = "stable/unbound"

  reuse_values = true

  set {
    name  = "myinfo.keys"
    value = "{${join(",", local.mykeys)}}"
  }
}

Panic Output

_No panic._

Expected Behavior

1. Apply runs successfully;
2. Release has myinfo.keys set as a list:

$ helm get values myapp
myinfo:
  keys:
  - aaa
  - bbb
  - ccc

1. My chart iterates successfully on myinfo.keys to add some environment variables on my app.

Actual Behavior

Commas get escaped and treated as one big item making my chart to try to add an invalid config key on Kubernetes, because of the commas, causing the apply to fail.

$ helm get values myapp
myinfo:
  keys:
  - aaa,bbb,ccc

Apply error: https://gist.github.com/mwconceicao/1f4c5275893da5b82b8fecfd292102f6

Steps to Reproduce

1. terraform apply

Important Factoids

Problem started after PR #227.

References

• Issue #226
• PR #227

---

Basically the problem started after PR #227 which adds the comma escaping code to set, set_sensitive and set_string.

Not sure if this was intended to be added in those 3 sets. I think this comma escaping makes sense for set_string and perhaps to set_sensitive as well, but I think we should keep set without comma escaping so we don't brake list definitions (syntax {aaa,bbb,ccc}) which is something important to support.

I'm willing to open a PR for this.

What do you folks think?

Thanks!

[rporres]

Thx for your report @mwconceicao. A few things:

• Can you post a fully working example? helm_repository xpto is not defined
• Is this working as you expect in previous versions on the chart?
• The only difference between set, set_string and set_sensitive is how they treat/display data, so there shouldn't be any difference between them

[mwconceicao]

@rporres

Thx for your report @mwconceicao. A few things:

• Can you post a fully working example? helm_repository xpto is not defined

I've updated the example with a fully working one.

• Is this working as you expect in previous versions on the chart?

Yes, version 0.9.0 works, version 0.9.1 doesn't work anymore.

• The only difference between set, set_string and set_sensitive is how they treat/display data, so there shouldn't be any difference between them

Well, my proposal is to decrease the difference created from Helm cmd tool, keeping set without the auto escape code.

What do you suggest?

Please let me know if more clarification is needed.

[sephriot]

I've also encountered this issue today.
It breaks functionality parity with command line Helm tool, as well as backward compatibility.
Good example of such issue is setting "controller.service.loadBalancerSourceRanges" for nginx-ingress Helm Chart.

data "helm_repository" "stable" {
    name = "stable"
    url  = "https://kubernetes-charts.storage.googleapis.com"
}

resource "helm_release" "ingress" {
  name       = "ingress"
  repository = "${data.helm_repository.stable.metadata.0.name}"
  chart      = "nginx-ingress"
  version    = "1.6.0"

  set {
    name  = "controller.service.loadBalancerSourceRanges"
    value = "{127.0.0.1/32,0.0.0.0/32}"
  }
}

After trying to apply such resource you should see following error:
* helm_release.ingress: rpc error: code = Unknown desc = Service "ingress-nginx-ingress-controller" is invalid: spec.LoadBalancerSourceRanges: Invalid value: "[127.0.0.1/32,0.0.0.0/32]": must be a list of IP ranges. For example, 10.240.0.0/24,10.250.0.0/24

[okgolove]

I have the same problem.

[selslack]

This needs to be fixed ASAP. Setting spec.LoadBalancerSourceRanges is unusable in set block if you generate range from a list variable.

I'll also leave a reference to https://github.com/terraform-providers/terraform-provider-helm/issues/132 so others coming from Google may find this ticket easier.

[ejschoen]

Would it make any sense for set to encode array values automatically into Helm's { xxx,yyy,zzz } syntax for setting values? IMHO, it would make for cleaner modules not having to "{${join(",", ...)}}" to try to force that encoding. (Or add a set_array for that purpose?)

[oasalonen]

This is starting to be a painful regression for us as well. We worked around it by downgrading to 0.9.0 but now we can't upgrade to terraform 0.12 because terraform 0.12checklist has the following to say:

After analyzing this configuration and working directory, we have identified some necessary steps that we recommend you take before upgrading to Terraform v0.12:

- [ ] Upgrade provider "helm" to version 0.9.1 or newer.

  No currently-installed version is compatible with Terraform 0.12. To upgrade, set the version constraint for this provider as follows and then run `terraform init`:

      version = "~> 0.9.1"

Has anyone tried running Terraform 0.12 with Helm provider 0.9.0?

[sephriot]

Setting array can be do with a "for" loop in terraform 0.12.
https://www.terraform.io/docs/configuration/expressions.html

To be specific here is an example that I've used with ingress controller:

  dynamic "set" {
    for_each = var.whitelisted_ips
    content {
      name = join("", ["controller.service.loadBalancerSourceRanges[", set.key, "]"])
      value = set.value
    }
  }

[mwconceicao]

Can we make a decision on this? Either adding a set_list (#302) or reverting the culprit PR is OK for me.

Let me know if you need anything.

/cc @rporres