1Password / connect-helm-charts

Official 1Password Helm Charts
https://developer.1password.com
MIT License
90 stars 73 forks source link

passing connect.credentials not possible in terraform & helm #67

Closed villesau closed 2 years ago

villesau commented 3 years ago

Your environment

Chart Version: 1.4.0

Helm Version: 3.x

Kubernetes Version: latest

What happened?

Terraform supports applying helm charts: https://registry.terraform.io/providers/hashicorp/helm/latest/docs/resources/release However, when setting connect.credentials like:

  set_sensitive {
    name  = "connect.credentials"
    type  = "string"
    value = " ${file("${path.root}/path/to/1password-credentials.json")} "
  }

the apply fails to Error: failed parsing key "connect.credentials" with value ...

This is due to this bug: https://github.com/hashicorp/terraform-provider-helm/issues/618 that is apparently also reproduceable in the actual helm: https://github.com/hashicorp/terraform-provider-helm/issues/618#issuecomment-729978207

What did you expect to happen?

Above should not fail. Could the chart e.g take only the file path and manage the JSON internally instead? Or would there be another possible workaround?

Steps to reproduce

  1. try to deploy the chart via terraform
  2. you get above error

E: found a workaround:

  set_sensitive {
    name  = "connect.credentials"
    type  = "string"
    value = " ${replace(file("${path.root}/path/to/1password-credentials.json"), ",", "\\,")}"
  }
Gibstick commented 3 years ago

I just ran into the same issue, and I came up with a different workaround. I noticed you mentioned not being able to use values in the other issue, but with the sensitive function you actually can:

  values = [
    yamlencode({
      connect = {
        credentials = sensitive(replace(file("path/to/credentials.json"), "\n", ""))
      }
    })
  ]
kylekurz commented 2 years ago

I was able to get this working via @Gibstick's workaround, but it would be very beneficial to avoid the extra layers when putting this into Terraform and instead just pass the file reference (or ideally leverage an ENV variable containing the contents of the file as part of CI).

jpcoenen commented 2 years ago

Hi all,

Thank you all for chipping in on this issue and bringing up the workaround! 🙌

What if we added an option to set the credential base64-encoded? So:

 set_sensitive {
    name  = "connect.credentials_base64"
    type  = "string"
    value = " ${filebase64("${path.root}/path/to/1password-credentials.json"))}"
  }

That should be really easy to add.

kylekurz commented 2 years ago

@jpcoenen that sounds promising. I would add that when deploying the operator alongside, I'd like the operator.token.value to be b64 capable as well to again provide that via an environment var.

jpcoenen commented 2 years ago

I'd like the operator.token.value to be b64 capable as well

That sounds like a great addition :+1:

kylekurz commented 2 years ago

Actually, given that the token can already be provided as a value instead of a file reference, that may not be strictly necessary? Doesn't hurt to make it consistent though.

villesau commented 2 years ago

Yep base64 is not a bad idea :+1: As long as the rationale behind credentials_base64 is well documented :)