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

Allow better lookup default value for map of objects with optional attributes #35027

Open
bholzer opened this issue Apr 19, 2024 · 4 comments
Open
Labels
enhancement new new issue not yet triaged

Comments

@bholzer
Copy link

bholzer commented Apr 19, 2024

Terraform Version

1.7.3

Use Cases

I would like to provide a sane default lookup value for a map of objects with optional values.

My current specific use case:

I have a module that creates some AWS eventbridge schedules. I have a variable that I am using to allow consumers to override values of default schedules while at the same time providing custom schedules of their own.

Here's what it looks like right now:

variable "schedules" {
  description = <<-DESC
    EventBridge schedule overrides for the application.
    These values will be merged with the default schedules or can create new schedules if no matching key is found.
  DESC
  type = map(object({
    schedule_expression = optional(string)
    description = optional(string)
    enabled = optional(bool, true)
    command = optional(list(string))
    cpu = optional(number)
    memory = optional(number)
  }))
  default = {}
}

locals {
  default_schedules = {
    schedule1 = {
      schedule_expression = "cron(0 8 ? * 3 *)" # Every Tuesday at 8:00 AM
      description = "Schedule 1"
      enabled = true
      command = ["command1"]
    }
    schedule2 = {
      schedule_expression = "cron(0/15 * ? * * *)" # Every 15 minutes
      description = "Schedule 2"
      enabled = true
      command = ["command2"]
    }
  }

  # Merge default schedules with var.schedules
  # If matching default schedule does not exist, create the provided schedule
  schedules = {
    for name in setunion(keys(var.schedules), keys(local.default_schedules)) :
      name => merge(
        try(local.default_schedules[name], {}),
        { for k, v in try(var.schedules[name], {}): k => v if v != null } # Remove null values that result from optional object values
      )
  }
}

I'm doing this to provide a flexible and easily configurable abstraction

Attempted Solutions

In my use case, note my usage of try(local.default_schedules[name], {})

I'm doing this because lookup(local.default_schedules, name, {}) does not work. I get an error: the default value must have the same type as the map elements

try seems like a suboptimal solution here because I don't want to swallow other unspecified errors unexpectedly or anything like that.

The solution suggested in the referenced issue also feels really unsatisfying. Fully specifying the entire object with null values is not something I'm going to do in this case.

Proposal

No response

References

This has been discussed a bit before, and it's said that this currently behaves as designed. That may be the case, but if so, I think the design should be changed to support a reasonable alternative.

#32417

@bholzer bholzer added enhancement new new issue not yet triaged labels Apr 19, 2024
@apparentlymart
Copy link
Member

apparentlymart commented Apr 19, 2024

Hi @bholzer,

When I consider your whole writeup here, I understand the underlying need as something like: I'm trying to generate a value for a remote system that requires optional properties to be omitted when unset, rather than explicitly set to null, and it's hard to get from Terraform's idea of optional attributes to what the remote system expects.

Do you think that's a valid interpretation of the problem you've encountered?

I ask because when thinking about that topmost goal a more direct solution comes to my mind: a function that takes an arbitrary value of any type, visits all of the nested values if any, and for any object type or map type it removes any attribute/element whose value is null.

Do you think that would be a viable replacement for the complicated expression you are trying to write in that local value?


If we did want to offer such a function there are some tricky details to handle, but I think all solvable. Some initial thoughts:

  • The data structure might contain unknown values. Unless they are refined as "definitely not null" we cannot know whether the final value will be null or not, so will need to leave the unknown value in place.
  • Removing an attribute from an object value produces a value of a different object type, because an object type is defined by its attributes. Therefore if we encounter an unknown value that might be null, as described in the previous point, we'll need to conservatively replace that entire object with an unknown value of unknown type, because we can't know yet whether that attribute would be present once the unknown value becomes known.
  • We will need to think about how to treat marks such as "sensitive" in the previous mentioned case. I think the right behavior is to discard the markings unless they are on the overall object being replaced, because that best matches what would happen if we were making a dynamic decision between a value containing nested sensitive values and one that does not and needed to produce an unknown result for that case.
  • Maps are at least easier to deal with, because removing an element from a map does not change the map's element type.
  • If we use the nullness of a sensitive value as any part of the decision process then any part of the value decided based on that result should also be marked as sensitive. (This generalizes for all value marks, but sensitive happens to be the only one we use broadly today.)

This function therefore seems subtle enough that it deserves to be a built-in, and I've heard about this use-case of removing null values several times before so I think it's also common enough to justify being a built-in even if it were not so subtle.

@bholzer
Copy link
Author

bholzer commented Apr 19, 2024

Thanks for taking the the time to take a look at this @apparentlymart. I realize this is somewhat unclear, sorry about that.

I'm trying to generate a value for a remote system that requires optional properties to be omitted when unset, rather than explicitly set to null, and it's hard to get from Terraform's idea of optional attributes to what the remote system expects.
Do you think that's a valid interpretation of the problem you've encountered?

Not quite, although I think you've identified another thing that I encounter with some frequency:

I find myself doing { for k, v in some_object: k => v if v != null } often enough that I would appreciate having the function you suggest to deal with this. For this module, I'm doing this to remove null-valued properties for remote system compatibility and to ensure my own expressions are working properly. I think of it as compact for objects/maps

--

Trying to restate the problem that prompted this issue more concisely:

There is no way to provide an empty object as a default when looking up objects that only have optional properties.

A slimmed down example:

variable "example" {
  type = map(object({
    prop = optional(string)
  }))
  default = {}
}

output "example" {
  value = lookup(var.example, "missing", {})
}

I recognize that the empty object is of a different type than the object specified in the variable and that's what causes the problem here, but it also seems reasonable to me that an object type with only optional properties can be thought of as "compactable" to an empty object.

If I wanted to use lookup properly here to avoid overusing try, the solution feels excessive (and gets much worse with more object properties):

output "example" {
  value = {
    for k, v in lookup(var.example, "missing", { prop = null }):
      k => v if v != null
  }
}

@apparentlymart
Copy link
Member

Hi @bholzer,

Thanks for the additional context!

Unfortunately in your example the lookup function can only see the concrete type of the current value of var.example, and not the type constraint that it was converted to earlier by the variable block. That function only knows that the map's element type is object({ prop = string }), because the optional attribute was already dealt with upstream.

I think what this problem really seems to want is the general-purpose convert function we've wanted to add for a long time:

convert({}, object({
  prop = optional(string)
}))

This hypothetical function would allow performing the same type conversion behavior that is built into variable blocks at any location where a general expression is expected.

Unfortunately we've not been able to implement this function so far because it requires a breaking change to the language: it means that Terraform must treat identifiers like string, number, etc as type constraints when they appear in value expressions, whereas historically Terraform has treated them as the beginning of a reference to a resource like resource "string" "whatever", belonging to a hypothetical provider called "string". Although we know of no such provider, we cannot prove that one does not exist privately inside a company somewhere, and so must preserve it for as long as the Terraform v1.x Compatibility Promises remain in effect.

(This function broadly requires some kind of unification of the "value expression" and "type expression" concepts, so that type constraint expressions can be valid in locations where value expressions are expected. Today type constraint expressions are valid only in limited situations, such as in the type argument in a variable block, so there is no ambiguity between the meaning of string in a type constraint vs. the meaning of string in a value expression.)

In the meantime then I think try remains the best option, despite its disadvantages, because it's built for this sort of "I don't care about types, just do what I want" sort of situation.

I'm sorry I don't have a better answer. At least we can use this issue to represent the use-case.

@crw
Copy link
Collaborator

crw commented Apr 19, 2024

Note for future viewers: if you are viewing this issue and would like to indicate your interest, please use the 👍 reaction on the issue description to upvote this issue. We also welcome additional use case descriptions. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement new new issue not yet triaged
Projects
None yet
Development

No branches or pull requests

3 participants