fix: Catch panic when null value defined in string variable

[demophoon]

This commit allows for us to emit a more helpful error message when a null value is passed into a variable where we were expecting a string.

Fixes #4051

[briancain]

This looks pretty good! Let's get a unit test that exercises this over in variables_test.go! Otherwise I approve 😎

[briancain]

Looks amazing! ✨

[ddaws]

Hey, thanks for getting this in so quick!

Should null not be allowed as a valid default value? The lack of support for null feels unexpected coming from other Hashicorp tools that have the same syntax for declaring variables but allow null.

Should this also be extended to cover types other than strings?

[briancain]

Hey there @ddaws - I see, do you have a link to the TF docs that describes that behavior? We can make it work that way, this PR was mainly to fix up the panic at the very least.

I noticed on your originally reported issue that your goal was to source API keys from vault, in that case, you'd probably want to set your variable to use the dynamic config sourcer anyways?

variable "my_api_key" {
  default = dynamic("vault", {
    path = "secret/data/keys"
    key  = "/data/my_api_key"
  })
  type        = string
  sensitive   = true
  description = "my api key from vault"
}

[ddaws]

Sorry for the late reply @briancain. I went on a trip but I'm back now and up for discussing this :)

---

Re where it's described in the TF docs, the input variable docs talks about default and nullable where nullable is true by default. This means that any variable can be set to null by default, and setting the default value for a variable to null effectively makes the value optional.

https://developer.hashicorp.com/terraform/language/values/variables#default-values

For example, for some variable my_api_key

variable "my_api_key" {
  type = string
}

Valid values for this var are a string or null because nullable is true by default, but the value must be specified. If you also set the default value to null then the variable is nullable and defaults to null, so it is effectively optional. For example

variable "my_api_key" {
  type    = string
  default = null
}

---

Re sourcing API keys from Vault, my goal is to allow the user to either pass API keys as vars or source shared keys from Vault. This way the caller could pass a different set of keys.

Here's an example of what that may look like

project = "busybox"

variable "my_api_key" {
  type    = string
  default = null # optional
}

app "busybox" {
  config {
    env = {
      MY_API_KEY = coalesce(var.my_api_key, dynamic("vault", {
        path = "secrets/data/cool-new-service"
        key  = "my_api_key"
      }))
    }
  }
  build {
    use "docker-pull" {
      image = "busybox"
      tag   = "latest"
    }
  }
  deploy {
    use "docker" {}
  }
  url {
    # Disable generating a tunneled hostname
    auto_hostname = false
  }
}

I am using coalesce to take the first non-null value, checking var.my_api_key first and then falling back to `dynamic('vault', ...). Is there a better way to do this?

[ddaws]

Lmk if we should move this into a new issue. Maybe we can get aligned on the right way to do this here and then I can create an issue 😃

[briancain]

Thank you @ddaws for all of the extra information! Really appreciate it ❤️

I am using coalesce to take the first non-null value, checking var.my_api_key first and then falling back to `dynamic('vault', ...). Is there a better way to do this?

Yes...I think so. I think it would make more sense to have your variable {} stanza be configured with your vault dynamic config rather than placing it in the env:

variable "my_api_key" {
  default = dynamic("vault", {
    path = "secret/data/keys"
    key  = "/data/my_api_key"
  })
  type        = string
  sensitive   = true
  description = "my api key from vault"
}

app "busybox" {
  config {
    env = {
      MY_API_KEY = var.my_api_key
...
...

Even though the default value is looked up from vault, if you set this input variable via a CLI or UI variable, it should be overridden and use that over the dynamic vault config.

As far as null being supported in variables, I recommend opening a feature request issue for that! Personally, if the ask is to allow variables to be optional, I think it makes more sense and is more discoverable to have a required field on variables rather than this nullable behavior, but that's my opinion! It also makes sense to align with TF given they're both HashiCorp products and should behave similarly. We can open the issue and have a discussion there.

[ddaws]

Thanks @briancain, I'll give that a shot. I'm concerned that dynamic('vault', ...) may be evaluated regardless of whether the a value is passed (if a value is passed the default should not be needed) but I'll test it out.

In the meantime, I've created an issue here that talks about support optional variables #4149

[briancain]

@ddaws - You are correct, if you don't pass a value, the config sourcer will attempt to resolve from Vault to get a value in. We'll discuss this as a team and report back on the issue you created. Appreciate you taking the time to be so detailed on that one! Thank you ❤️