hashicorp / terraform

Terraform enables you to safely and predictably create, change, and improve infrastructure. It is a source-available tool that codifies APIs into declarative configuration files that can be shared amongst team members, treated as code, edited, reviewed, and versioned.
https://www.terraform.io/
Other
42.74k stars 9.56k forks source link

Allow optional attribute values to have dynamic default values derived from objects elsewhere in the same module #30750

Open aidanmelen opened 2 years ago

aidanmelen commented 2 years ago
terraform {
  required_version = ">= 0.13.1"

  experiments = [module_variable_optional_attrs]
}

Use-cases

the optional() notation accepts a default value. The current default (null) can be tricky to work with and can lead to unexpected behavior.

Proposal

Similar to the lookup function (lookup(map, key, default), the second argument in option could take a default value.

variable "example" {
  type = object({
      foo = string
      bar = optional(string, "default") # proposed default argument
  })
}

output "foo" {
  value = var.example["foo"]
}

output "bar" {
  value = var.example["bar"] # "default"
}

This would elegantly solve the optional value without requiring ternaries to handle the null default.

Current Terraform Version

References

crw commented 2 years ago

Thanks for the report! It will be considered as we take the next steps with this experimental feature.

rawtaz commented 2 years ago

This is by far the most natural way to specify a default value for something that one explicitly marks as being optional, I hope it gets implemented :)

I would like to note that even if some other way to specify default values is implemented, this second parameter to optional() should still exist - there's no reason not to be able to do it this simple way, when one doesn't have more complex needs :) It also makes the code very readable.

korinne commented 2 years ago

Hi all, we recently released the Terraform v1.3 alpha, which includes the ability to mark object type attributes as optional and set default values.

I'm following up with issues that have provided feedback on the previous experiment, and wanted to invite you all to try the alpha and provide any feedback on this updated design. You can learn more/add comments here. Thank you so much in advance, and hope you like the feature!

rawtaz commented 2 years ago

I was sooo excited to finally use this new feature, but to my dismay I discovered that if you have e.g. flavor_name = optional(string, var.flavor_name), # Name of flavor to use you get the error "Variables may not be used here".

I'm not even sure what words to use to describe how impractical and unexpected this is, and foremost how much it basically nullifies the entire point of having this default parameter to optional(). Okay, sure, there are probably a bunch of situations where you'd set a static value as the default one in optional(), but using a variable as the default value is IMO an obvious use case. In the current state, I'd call this feature highly incomplete and honestly I can't imagine when I'd use it if it doesn't support variables and similar constructs.

To elaborate, in my example I have (or intended to have) this:

variable "docker_hosts" {
  description = <<-EOT
    Map of docker hosts, indexed by instance name.

    Example:
      {
        docker-sto1-srv1 = { az = "sto1" },
        docker-sto2-srv1 = { az = "sto2", flavor_name = "v1-micro-1", image_name = "debian-11-latest" },
      }
  EOT
  type = map(object({
    az          = string,                                            # Availability zone.
    flavor_name = optional(string, var.flavor_name),                 # Name of flavor to use
    image_name  = optional(string, var.image_name),                  # Name of image to use
    user_data   = optional(string, data.cloudinit_config.user_data), # Cloud-init user data
  }))
  default = {}
}

As you can see, the idea is to provide a default value for e.g. flavor and image when it's not specified explicitly for an instance, and the value for this should naturally not be hardcoded here in the object structure definition, but come from a separate variable so that it can be injected into the module. That said, of course the default parameter should be possible to define using a local variable, and if possible a data source as well.

Can we please have this fixed, so that this feature is actually usable? It would let us not have to use coalesce() in a ton of other places to simulate a default value at usage time, instead letting us specify the default in one place - much cleaner code.

Finally, sorry I didn't write this in the forum - I don't have an account there and don't believe in having to register accounts everywhere. Also, sorry if I should have pointed out the need for supporting variables et al earlier - it didn't even cross my mind that this default parameter would not support them :-)

aidanmelen commented 2 years ago

I was sooo excited to finally use this new feature, but to my dismay I discovered that if you have e.g. flavor_name = optional(string, var.flavor_name), # Name of flavor to use you get the error "Variables may not be used here".

I'm not even sure what words to use to describe how impractical and unexpected this is, and foremost how much it basically nullifies the entire point of having this default parameter to optional(). Okay, sure, there are probably a bunch of situations where you'd set a static value as the default one in optional(), but using a variable as the default value is IMO an obvious use case. In the current state, I'd call this feature highly incomplete and honestly I can't imagine when I'd use it if it doesn't support variables and similar constructs.

To elaborate, in my example I have (or intended to have) this:


variable "docker_hosts" {

  description = <<-EOT

    Map of docker hosts, indexed by instance name.

    Example:

      {

        docker-sto1-srv1 = { az = "sto1" },

        docker-sto2-srv1 = { az = "sto2", flavor_name = "v1-micro-1", image_name = "debian-11-latest" },

      }

  EOT

  type = map(object({

    az          = string,                                            # Availability zone.

    flavor_name = optional(string, var.flavor_name),                 # Name of flavor to use

    image_name  = optional(string, var.image_name),                  # Name of image to use

    user_data   = optional(string, data.cloudinit_config.user_data), # Cloud-init user data

  }))

  default = {}

}

As you can see, the idea is to provide a default value for e.g. flavor and image when it's not specified explicitly for an instance, and the value for this should naturally not be hardcoded here in the object structure definition, but come from a separate variable so that it can be injected into the module. That said, of course the default parameter should be possible to define using a local variable, and if possible a data source as well.

Can we please have this fixed, so that this feature is actually usable? It would let us not have to use coalesce() in a ton of other places to simulate a default value at usage time, instead letting us specify the default in one place - much cleaner code.

Finally, sorry I didn't write this in the forum - I don't have an account there and don't believe in having to register accounts everywhere. Also, sorry if I should have pointed out the need for supporting variables et al earlier - it didn't even cross my mind that this default parameter would not support them :-)

I would not expect defaults to allow dynamic values just like you can't use a dynamic value for variable.default.

You will need to solve by setting the default to null. Then use local to set a dynamic value if the lookup value is null.

rawtaz commented 2 years ago

You will need to solve by setting the default to null. Then use local to set a dynamic value if the lookup value is null.

Thanks, but it's not much different from what I'm doing now, which is flavor_id = lookup(var.flavor_ids, coalesce(each.value["flavor_name"], var.flavor_name)). This can indeed be turned into a local, but the point is that a default value should be possible to specify where there's a parameter for it :)

apparentlymart commented 2 years ago

Hello,

It seems like this issue has effectively now transformed into a request for allowing default values for input variables to be dynamic based on other values present in the same module, and so I'm going to re-title it that way so that we don't keep getting tempted to close this as complete with the new functionality in Terraform v1.3.0.

aidanmelen commented 2 years ago

Related to: https://github.com/hashicorp/terraform/pull/31154

robzr commented 2 years ago

The way that Hashicorp enabled the defaults() experiment from v0.14 all the way up to v1.2.9, then cut it and introduced, with zero experiment time, a far less powerful extended optional() syntax is highly frustrating.

Without defaults(), all dynamic default value setting needs to be rewritten with merge/coalesce and conditionals. It will be far less readable when traversing data structures.

pedroosorio commented 2 years ago

I second @robzr. optional() without allowing other variables as defaults like:

variable "foo" {
  type = string
  default = "123"
}

variable "bar" {
  type = object({
    baz = optional(string, "abc")
    another_foo = optional(string, var.foo)
  })
}

Becomes a bit limiting and requires the local + coalesce/lookup pattern, for big objects, isn't great. Nonetheless, the feature looks mature, just missing this cherry on top 👍

apparentlymart commented 2 years ago

I think this request requires essentially the same internal architecture change as https://github.com/hashicorp/terraform/issues/25609 and so it may be worthwhile to address both of them at the same time if it seems like "the rest is easy" once the graph shape changes are in.

These are also potentially related depending on whether we intend to allow referring to anything other than other input variables from the same module:

Input variable validation and type conversion are both happening in the same graph node, so essentially the capabilities between them are limited in the same way.

Default values do have the additional concern that allowing them to be dynamic will make it impossible to show the exact default values in statically-generated module documentation, such as in a module registry. That'll be a tradeoff to consider when it comes to designing this.

jalaziz commented 2 years ago

31823 is somewhat related as it would at least provide a simpler way to set dynamic defaults.

tmccombs commented 2 years ago

I think it is reasonable for the second argument to optional to have to be static (although it would be really nice if the default for both variables and the optional type could at least reference other variables).

However, there should really be an equivalent to the defaults function that was removed. Perhaps bring the old function back, or have a merge function that doesn't overwrite with nulls, or have a function for removing null values from an object or map.

apparentlymart commented 2 years ago

Hi all,

I want to be explicit that from the perspective of the stable Terraform language the defaults function never existed: it was only ever in an experimental version of the language and that part of the experiment failed due to the defaults function behavior being counter-intuitive. I understand that some folks ignored the experimental warning and used it in production successfully anyway, but that is not sufficient grounds for an experiment with a significant design problem to be shipped as stable and covered by the v1.x compatibility promises so that it then cannot evolve further.

The next step for this issue is therefore to design a suitable new language feature which aims to solve some of the same problems as the defaults experiment was aiming to solve, but without repeating the same usability hazard which defaults suffered from: it was unclear whether the default value given for a value of collection type is a default value for the entire collection or for an individual item in the collection. A significant amount of feedback on the experiment consisted of people re-asking the same question about why collection element defaulting didn't work in the way that the author expected or needed. The feature that shipped addressed that particular concern by including syntax for both situations, but that came at the expense of it only working for statically-defined values.

As I mentioned in an earlier comment above, it's also desirable for default values to be friendly to static documentation, and so that is another concern we'd want to keep in mind while evaluating possible new designs to address this request.

We don't yet have an improved design to discuss. This issue could be an appropriate place to share and discuss ideas to solve the problem, but we typically want to wait at least one release after an experiment becomes stable to learn from real experiences using it before trying to design new features that build on top of what just shipped, and so for the moment from my perspective this request is in an information-gathering phase but not an active design and development phase.

31823 is indeed one possible candidate, although since it already has its own issue I would suggest that we discuss the details of that one over there and then, if that looks promising, we can figure out how to best to consolidate these discussions together later.

Another possibility is that we first implement a facility for provider plugins to contribute additional functions to Terraform, and then the community can use provider plugins to experiment with a variety of different approaches that need not be constrained by the burden of Terraform's own backward compatibility promises. If one such answer becomes clearly prominent as a "winner" in the community then we might consider incorporating it as a builtin. (Note that it is already possible to try function ideas in providers using data sources which only perform local computation, though of course the syntax is far less convenient. But it could be a reasonable way to prototype in the meantime, without waiting for the ability for providers to contribute normal functions.)

Thanks!

tmccombs commented 2 years ago

I'm sorry, I probably didn't make my intent clear enough in my comment. I was fully aware that the optional feature was experimental, and am not mad that the finalized version wasn't exactly the same as the experiment. Nor do I consider it a regression, since the feature was experimental. I am, in fact, very happy that this feature has been stabilized, and think that the design works very well in a lot of cases. And I would rather have a stable optional feature now that could be improved in the future than an experimental feature that is a little more powerful.

The one case where I think it could be improved is if you need a dynamic default, which was served a little better with the defaults function in the experiment. It is certainly possible to work around this, just as it is with optional variables. Although it can be a little more complex and less readable in some cases. And have used the defaults function in the experiment, I do agree that it was kind of confusing, and probably shouldn't be brought back exactly as it was.

perlboy commented 1 year ago

As much as I acknowledge that defaults was always an experimental feature the removal of it is a bit bitter sweet without the ability to handle variables in optional. One of our use cases is that we output structure yaml/json configuration (Spring service) using HCL types to enforce and within that structure we define a value for secret loading which nomad interpolates later down the line on a per customer per environment basis (so the vault policy is locked down). While there are other ways, for example have the service hit vault directly to get DB secrets, from an example of a use case for this ticket it seems relevant.

Deliberately stripped config (squint to see the Spring Config):

variable "config_my_service_name" {
  description = "My Service"
  type = object({
    springy-config = object({
      datasource = object({
        driverClassName = optional(string, "org.postgresql.Driver")
        url             = optional(string, "{{ with secret \"secret/data/%CUSTOMER_CODE%/%ENVIRONMENT%/db/my-service\" }}{{.Data.data.url}}{{end}}")
        username        = optional(string, "{{ with secret \"secret/data/%CUSTOMER_CODE%/%ENVIRONMENT%/db/my-service\" }}{{.Data.data.username}}{{end}}")
        password        = optional(string, "{{ with secret \"secret/data/%CUSTOMER_CODE%/%ENVIRONMENT%/db/my-service\" }}{{.Data.data.password}}{{end}}")
      })
    })
  })
}

With defaults we could do ${var.CUSTOMER_CODE} and ship a map (from locals) for substitutions but now we can't. I had wanted to do a format("%s/%s", var.CUSTOMER_CODE, var.ENVIRONMENT) type thing. Nonetheless for those that find this thread and perhaps have limited variables to interpolate like we do, we've worked around this with a somewhat nasty jsonencode/jsondecode combo in locals{}:

  my_service_name                          = jsondecode(replace(replace(jsonencode(var.config_my_service_name), "%ENVIRONMENT%", var.environment), "%CUSTOMER_CODE%", var.customer_code))

Not pretty but still achieves the goal without merge/coalesce hell and optional is the only pathway (ie. forced to abandon defaults) while still netting relatively high benefit.

jeacott1 commented 3 months ago

interesting thread. I'd actually like to be able to reference other fields within a map/object variable directly too eg:

variable "bar" {
  type = object({
    baz = optional(string, "abc")
    another_foo = optional(string, "${var.bar.baz}-concatstr")
  })
}

the natural extension of this starts to look complex though as you begin to consider lists and maps and wanting to reference things with foreach like equivalents. still worth considering I think.