Conditionally pass complex object

[Kellen275]

I’m importing the terraform-aws-autoscaling module, and am trying to configure a simple boolean toggle on whether the var.instance_refresh value is set (used in the module here).

In my implementation, I have the following

  instance_refresh = local.refresh ? local.default_instance_refresh : null

Where

  default_instance_refresh = {
    strategy = "Rolling"
    preferences = {
      min_healthy_percentage = 100
      max_healthy_percentage = 110
    }
  }

If I set local.refresh = false I get an error because that causes null to be passed to the public module which blindly runs length(var.instance_refresh).

Alternatively, replacing my null with {} gives a type mismatch with my default_instance_refresh.

What would the recommended approach to have a boolean value toggle whether instance_refresh is used?

[apparentlymart]

Hi @Kellen275,

Unfortunately what you’ve described seems to be a design problem with the module you are calling, which is pushing a bunch of complexity up into your calling module as a result.

The way I’d expect this module to be defined is to declare the variable like this:

variable "instance_refresh" {
  description = "If this block is configured, start an Instance Refresh when this Auto Scaling Group is updated"

  type = object({
    strategy = string
    triggers = optional(set(string))
    preferences = optional(object({
      checkpoint_delay       = optional(number)
      checkpoint_percentages = optional(list(number))
      instance_warmup        = optional(number)
      max_healthy_percentage = optional(number)
      min_healthy_percentage = optional(number)
      skip_matching          = optional(bool)
      auto_rollback          = optional(bool)
      alarm_specification = optional(object({
        alarms = set(string)
      }))
      scale_in_protected_instances = optional(string)
      standby_instances            = optional(string)
    }))
  })
  default = null
}

…and then define the dynamic block based on the presence (non-nullness) of the value:

  dynamic "instance_refresh" {
    for_each = var.instance_refresh[*]
    content {
      strategy = instance_refresh.value.strategy
      triggers = instance_refresh.value.triggers

      dynamic "preferences" {
        for_each = instance_refresh.value.preferences[*]
        content {
          checkpoint_delay             = preferences.value.checkpoint_delay
          checkpoint_percentages       = preferences.value.checkpoint_percentages
          instance_warmup              = preferences.value.instance_warmup
          min_healthy_percentage       = preferences.value.min_healthy_percentage
          max_healthy_percentage       = preferences.value.max_healthy_percentage
          auto_rollback                = preferences.value.auto_rollback
          scale_in_protected_instances = preferences.value.scale_in_protected_instances
          skip_matching                = preferences.value.skip_matching
          standby_instances            = preferences.value.standby_instances
        }
      }
    }
  }

Your call to the module would then work as you wrote it, because null would be the correct representation of “not set”. Currently the module seems to treat an empty object as meaning “not set”, which is a non-idiomatic representation that is only possible because the module does not declare what type of value this variable is supposed to accept.

I assume this module is written this way because it was originally written for an old version of Terraform whose language didn’t yet have the facility for optional attributes in object type constraints. The author used some reasonable workarounds for that situation, although using {} as the default instead of null was unfortunate and is the main cause of the problem you’ve encountered here.

If possible I think the best solution would be to change the shared module to either fully specify its type constraint as I showed above, or at least to change the default to null and use the nullness to represent “not set” so that it agrees with the design assumptions of the Terraform language.

If the module author is not willing to make that change then there isn’t really any great option – you’re effectively working around some non-idiomatic module design – but one shorthand that I think of is the following:

  instance_refresh = {
    for k, v in local.default_instance_refresh : k => v
    if local.refresh
  }

This is a slightly-questionable use of a for expression, but it does at least match the slightly-questionable way that the module you are using decides whether you intended to set this argument: if local.refresh is not true then the for expression will filter out all of the attributes of local.default_instance_refresh, leaving you with a value of type object({}) (an object with no attributes) as the module is expecting.

(Using length on an object type to find out how many attributes it has seems to me essentially symmetrical to using for expression to dynamically remove attributes from an object value, even though both are quite a strange thing to do.)

[Kellen275]

This is great @apparentlymart , thank you for all the detail and potential solutions! I’ll see if I can get this change pushed into the module.

[apparentlymart]

One further option I just thought of, if the upstream author is willing to make changes here, would be to set this variable to be non-nullable:

variable "instance_refresh" {
  description = "If this block is configured, start an Instance Refresh when this Auto Scaling Group is updated"
  type        = any
  default     = {}
  nullable    = false
}

That additional option means that if a caller of the module sets the variable explicitly to null then Terraform will use the default value {} instead. That would then make your conditional expression work because your null would be replaced automatically by {} as the value enters the module.

This third option is effectively an alternative to setting default = null, which would get a similar effect but without needing to change how the dynamic block tests whether this variable was set.

[bryantbiggs]

thats a rather bold statement that I don’t think is correct. the module provides a variable for instance_refresh. If you wish to configure instance refresh, you provide the configuration values through that variable. If you do not, you don’t provide anything.

The issue is the user is wrapping this module and their implementation is introducing the challenges

[apparentlymart]

This module is doing two things that are not consistent with how the Terraform language is intended to be used:

• Using type = any for an input variable where the module doesn’t actually accept values of any type. The usage of this variable clearly expects it to be an object with certain attributes, so any is an insufficient type constraint for what this module requires.
• Using the number of attributes in an object type as part of a decision. This is only possible at all because of the previous point: if the module were properly declaring its type constraint then it wouldn’t be possible for the number of attributes to vary.

As I hinted above, I assume the first problem is a result of Terraform being late to introduce the possibility of optional object type attributes in a type constraint. Using any was a plausible workaround for that limitation in older Terraform versions. A variable declaration like the one I wrote above is the intended way to deal with this in modern Terraform.

This module is also, due to a historical design error in Terraform itself, treating absence of the argument differently from it being set to null. The nullable = false suggestion above opts into the new better behavior of treating those two situations as equivalent. It’s likely that nullable = false will become the default in a future language edition, because Terraform’s current default treatment is accidentally inconsistent with the treatment of null in every other case. It’s an opt-in only today to avoid breaking modules that depend on this design mistake, but your module does not depend on that mistake (null is not a valid value for this variable anyway) and so nullable = false would solve this problem without changing anything else.

I understand that you consider some parts of Terraform’s language design to be “limitations”, rather than being designed to be used in a different way than how this module is written, and you are entitled to that perspective. My intention here is only to describe the way the Terraform language is intended to be used, not to try to compel you to use it in that way.

With all of that said, given the discussion I see in the GitHub issue that I assume prompted this reaction, I hope we can agree that the original author of this topic has a need that the module isn’t designed to support – deciding dynamically whether to populate this argument – and it seems that you do not wish to change that module to support that need, and that is of course your right as a module maintainer. If that is your position then the original poster here will indeed either need to employ a workaround like the one you proposed on GitHub, maintain a fork of the module that incorporates one of my suggestions above, or solve this problem in some other way that does not involve this module.

[bryantbiggs]

This is rather unfortunate. We have a substantial number of users who rely on our modules, you’ll see them at the top of the registry. We simply cannot stay on the bleeding edge as the language introduces features. Terraform does not provide abstractions like traditional software any any adoption of these newer features is felt immediately by users and therefore fall under a breaking change on modules. To see the core maintainers label our work as lacking, only because the language has recently evolved, is disheartening.

Would we do things differently today given the current language feature set? Absolutely. But this is part of software and “legacy” usage. But our module design is not flawed

[apparentlymart]

Hi @bryantbiggs,

I was not intending to make broad statements about this module or any other modules in this collection. I am trying to directly address the question raised in this topic, which was about some friction that arises when a module treats setting an input variable to null explicitly in a different way than omitting it, and in this particular case where a module uses a value of an entirely different type to represent “unset”, making it harder to choose between the two possibilities using the conditional operator. This is not a judgement of module quality, but rather an explanation of why this particular module can’t be used in the typical pattern of using null to represent “not set”, and why a more complex expression is required for this module instead.

I tried to describe a few different ways this problem could potentially be addressed in the shared module rather than in the caller, with changes of different magnitudes because I anticipated that it would be hard to make a larger change (such as a new type constraint) to an existing module.

The smallest of the alternatives I suggested was to add nullable = false to this variable. This would cause Terraform to transform null into {} at the entry into the module, which would therefore match the expectations of the implementation of the module which uses an empty object to represent “not set”.

The nullable option for input variables was introduced in Terraform v1.1.0, which was released at the end of 2021. Adding this option would make v1.1.0 the earliest version of Terraform supported by the next release of your module. Of course I cannot decide on your behalf if that’s a reasonable minimum supported version for the users that you are intending to serve with new releases.

If you need to prioritize support for older releases of Terraform then indeed it won’t be possible to update this module to support the usage pattern described in this topic, and so the original requester will need to decide between the various alternatives we’ve already discussed here.