Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Defaults bug when nested type is map(string) #31177

Closed
theherk opened this issue Jun 1, 2022 · 3 comments
Closed

Defaults bug when nested type is map(string) #31177

theherk opened this issue Jun 1, 2022 · 3 comments
Labels
bug new new issue not yet triaged

Comments

@theherk
Copy link
Contributor

theherk commented Jun 1, 2022

Terraform Version

Terraform v1.2.1
on darwin_amd64

Terraform Configuration Files

terraform {
  experiments = [module_variable_optional_attrs]
}

variable "things" {
  type = list(object({
    inner = optional(object({
      a = optional(string)
      b = optional(map(string))
    }))
  }))
}

locals {
  things_with_defaults = defaults(var.things, {
    inner = {
      # If this is uncommented: error:
      # Invalid value for "defaults" parameter: .inner.b: invalid default value for string: string required.
      # But the type is a map(string), so {} should be fine, right?
      # b = {}
    }
  })

  inners = [for v in local.things_with_defaults : v.inner]
}

output "inners" {
  value = local.inners
}

output "things" {
  value = [for v in local.inners : v.b != {} ? v.b : { "override" = "map value" }]
}

terraform.tfvars

things = [
  {
    inner = {
      a = "something"
    }
  },
  {
    inner = {
      b = { "want" = "bar" }
    }
  },
]

Debug Output

Expected Behavior

Should have given:

Changes to Outputs:
  + inners = [
      + {
          + a = "something"
          + b = {}
        },
      + {
          + a = null
          + b = {
              + "want" = "bar"
            }
        },
    ]
  + things = [
      + {
          + "override" = "map value"
        },
      + {
          + "want" = "bar"
        },
    ]

Actual Behavior

Actually yields:

Changes to Outputs:
  + inners = [
      + {
          + a = "something"
          + b = {}
        },
      + {
          + a = null
          + b = {
              + "want" = "bar"
            }
        },
    ]
  + things = [
      + {}, # this is the issue
      + {
          + "want" = "bar"
        },
    ]

Steps to Reproduce

terraform init
terraform plan

Additional Context

This example is contrived, and therefore may not seem practical, but the actual use case is more complex. This is just the minimum reproducible example.

I understand this is still being evaluated as noted recently in #19898, but I use it currently and this case I'm not sure the workaround.

You can see in the output inners that b is clearly {}, so why can't we seem to match that?

I even tried setting the inner.b default to {}, but then I get the error Invalid value for "defaults" parameter: .inner.b: invalid default value for string: string. I find this even more baffling. This isn't a string. It is map(string), so {} should be a valid value, right?

References

@theherk theherk added bug new new issue not yet triaged labels Jun 1, 2022
@apparentlymart
Copy link
Member

Hi @theherk! Thanks for reporting this.

It looks like you've encountered one of the common rough edges in the design of defaults which has caused it (and the associated optional attributes syntax) to remain experimental so far: the defaults function is intentionally designed so you can provide common defaults for all elements in a collection together, rather than defaults for the collection as a whole.

The defaults documentation tries (and, evidently, fails) to explain this behavior with the following text and a subsequent example:

Collection types (list, map, and set types): Terraform will visit each of the collection elements in turn and apply defaults to them. In this case the default value is only a single value to be applied to all elements of the collection, so it must have a type compatible with the collection's element type rather than with the collection type itself.

Since the element type of map(string) is string, the default value for an attribute of that type must be the string to use whenever any element of the map is null. That interpretation is pretty useless for a map of strings in particular, but it's intended for maps of an object type where the attribute values are themselves optional, as in the documents map in the example declaration in the documentation:

variable "storage" {
  type = object({
    name    = string
    enabled = optional(bool)
    website = object({
      index_document = optional(string)
      error_document = optional(string)
    })
    documents = map(
      object({
        source_file  = string
        content_type = optional(string)
      })
    )
  })
}

In the subsequent call to defaults, the caller provides only a single object value for documents, which specifies a default value to use for any element of documents that has a null value for content_type:

  storage = defaults(var.storage, {
    # ...

    # The "documents" attribute has a map type,
    # so the default value represents defaults
    # to be applied to all of the elements in
    # the map, not for the map itself. Therefore
    # it's a single object matching the map
    # element type, not a map itself.
    documents = {
      # If _any_ of the map elements omit
      # content_type then this default will be
      # used instead.
      content_type = "application/octet-stream"
    }
  })

Thankfully, this confusing situation will shortly become moot because we are intending to remove the defaults function entirely and instead integrate the idea of default values right into the type constraint. When we initially implemented this experiment it wasn't yet clear how we could internally represent a type constraint effectively having values embedded in it, but we do now have a design that seems to work which is under discussion over in #31154.

Assuming we don't find an unexpected significant problem with that design in the meantime, it should hopefully be available for testing in an alpha release soon, in case you'd like to try it with your use-case. If we hear positive feedback about it during the alpha period then it will hopefully be stabilized in the next minor release, though it's too early to say for certain yet before there's been any external feedback at all.

I think your example would look like this under the design implemented in that PR:

variable "things" {
  type = list(object({
    inner = optional(object({
      a = optional(string)
      b = optional(map(string), {})
    }))
  }))
}

Terraform will then handle the substitution of {} instead of null for attribute b automatically as part of preparing the final value for var.things, removing the need to declare a separate local value with the default values filled in. In effect, the behavior of defaults is now built in to Terraform Core's handling of input variables, and so a separate function is no longer needed.

Including the default value as part of the optional "pseudo-function" means it's unambiguous whether you intend the default value to apply to the collection as a whole (as in this case) or to individual elements of the collection (as the defaults function assumed), and so this design should allow us to address both of those needs at once.

I think given that the defaults function is likely to be removed imminently, I'm going to close this issue now just because it'll avoid me forgetting to come back here and close it once #31154 is merged. Once there's something available for easy testing we'll post a note about it over in #19898 so it'll be more visible to all of the subscribers of that one.

Thanks again!

@apparentlymart apparentlymart closed this as not planned Won't fix, can't repro, duplicate, stale Jun 1, 2022
@theherk
Copy link
Contributor Author

theherk commented Jun 2, 2022

Okay. Thank you for the detailed explanation as always, @apparentlymart. The map handling is a bit counterintuitive for the reasons you specify, but the improvements proposed here and in the main thread really look good, and I'm excited to test it out. I'm still glad I went through this exercise because it was dizzying when trying to sort it out in a bigger implementation, so boiling it down helped to understand what was happening, even if not why.

@github-actions
Copy link

github-actions bot commented Jul 3, 2022

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug new new issue not yet triaged
Projects
None yet
Development

No branches or pull requests

2 participants