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

Objects with optional(list(string)) attributes cannot have useful defaults applied with defaults function #28406

Closed
ibacalu opened this issue Apr 16, 2021 · 17 comments

Comments

@ibacalu
Copy link

ibacalu commented Apr 16, 2021

As you will see in the example below, defaults doesn't work as expected.
I've been struggling with this for too much time already.

Terraform Version

➜ terraform --version
Terraform v0.15.0
on darwin_amd64
+ provider registry.terraform.io/hashicorp/aws v3.36.0
+ provider registry.terraform.io/hashicorp/null v3.1.0

Terraform Configuration Files

variable "config" {
  description = "(Required) Module configuration"
  type = object({
    cluster_id                   = string,
    replication_group_id         = optional(string),
    engine                       = optional(string),
    engine_version               = optional(string),
    maintenance_window           = optional(string),
    node_type                    = optional(string),
    num_cache_nodes              = optional(number),
    parameter_group_name         = optional(string),
    port                         = optional(number),
    subnet_group_name            = optional(string),
    security_group_names         = optional(list(string)),
    security_group_ids           = optional(list(string)),
    apply_immediately            = optional(bool),
    snapshot_arns                = optional(list(string)),
    snapshot_name                = optional(string),
    snapshot_window              = optional(string),
    snapshot_retention_limit     = optional(string),
    notification_topic_arn       = optional(string),
    az_mode                      = optional(string),
    availability_zone            = optional(string),
    preferred_availability_zones = optional(list(string)),
    final_snapshot_identifier    = optional(string),
    tags                         = optional(map(string)),
  })
}

locals {
  config = defaults(var.config, {
    maintenance_window  = "sun:05:00-sun:09:00"
    engine              = "redis"
    engine_version      = "3.2.10"
    node_type           = "cache.t2.medium"
    num_cache_nodes     = 1
    port                = 6379
    apply_immediately   = false
    az_mode             = "single-az"
    security_group_ids = ["sg-000000"]
  })
}

Debug Output

╷
│ Error: Invalid function argument
│ 
│   on .terraform/modules/aws_security_group/aws_elasticache_cluster/locals.tf line 2, in locals:
│    2:   config = defaults(var.config, {
│    3:     maintenance_window  = "sun:05:00-sun:09:00"
│    4:     engine              = "redis"
│    5:     engine_version      = "3.2.10"
│    6:     node_type           = "cache.t2.medium"
│    7:     num_cache_nodes     = 1
│    8:     port                = 6379
│    9:     apply_immediately   = false
│   10:     az_mode             = "single-az"
│   11:     security_group_ids = ["sg-000000"]
│   12:   })
│ 
│ Invalid value for "defaults" parameter: .security_group_ids: invalid default value for string: string required.

Crash Output

Expected Behavior

Set default value of local.config.security_group_ids to ["sg-000000"]

Actual Behavior

Error

Steps to Reproduce

  1. terraform init
  2. terraform plan

Additional Context

Worthwhile mentioning also that for_each loops based on values from defaults do not work and complain that its value cannot be determined until apply.

References

@ibacalu ibacalu added bug new new issue not yet triaged labels Apr 16, 2021
@ibacalu
Copy link
Author

ibacalu commented Apr 16, 2021

Hey @alisdair! Tagged you because you were involved in this one as well: #27385
I was hoping you have better insights on the issue.

@alisdair
Copy link
Contributor

Hi @ibacalu, thanks for reporting this. This is a duplicate of #28372.

It looks like you are trying to assign a list value as a default for a list attribute, which is the cause of this error. While I understand that is an intuitive thing to want to do, this is not how the defaults function was designed to work. From the documentation:

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.

That means that there is no way (that I can think of) to use defaults to express something like "for the security_group_ids list attribute, if it is unset, use this other list of strings instead". Depending on exactly how you use this local value, the coalescelist function might be applicable here.

While I don't think this is a bug in Terraform, the defaults function and the optional object attributes syntax are still experimental, so we're working on the design. If you could share your use case in some detail in a new topic on the discussion forum, that would help us to plan our follow-up work here—and maybe someone will have ideas for how to work around it in the interim. Thanks in advance!

@alisdair alisdair added experiment/module_variable_optional_attrs functions working as designed confirmed as reported and closed because the behavior is intended and removed new new issue not yet triaged labels Apr 16, 2021
@vladimirtiukhtin
Copy link

Sorry @alisdair, do we take it as a "wait for a change in the design" or "use coalescelist as there won't be any change"?

@alisdair
Copy link
Contributor

The former! We're still gathering feedback on both optional object attributes and the associated defaults function in order to determine what the stable design should be. For example, issue #28344 is a different example of similar feedback on this topic, and pull request #28376 prototypes one possible solution to this.

Providing detailed feedback and use cases at this stage is extremely helpful, and will help us to improve the design of these features for future versions. We appreciate your input!

@ibacalu
Copy link
Author

ibacalu commented Apr 17, 2021

so, @alisdair you are suggesting the following configuration would work? Because it doesn't
Similar somewhat to #28344 but with lists/sets
I think this ticket needs to be re-opened. At the moment defaults is only usable with plain strings/numbers.

Configuration

variable "config" {
  description = "(Required) Module configuration"
  type = object({
    cluster_id                   = string,
    replication_group_id         = optional(string),
    engine                       = optional(string),
    engine_version               = optional(string),
    maintenance_window           = optional(string),
    node_type                    = optional(string),
    num_cache_nodes              = optional(number),
    parameter_group_name         = optional(string),
    port                         = optional(number),
    subnet_group_name            = optional(string),
    security_group_names         = optional(list(string)),
    security_group_ids           = optional(list(string)),
    apply_immediately            = optional(bool),
    snapshot_arns                = optional(list(string)),
    snapshot_name                = optional(string),
    snapshot_window              = optional(string),
    snapshot_retention_limit     = optional(string),
    notification_topic_arn       = optional(string),
    az_mode                      = optional(string),
    availability_zone            = optional(string),
    preferred_availability_zones = optional(list(string)),
    final_snapshot_identifier    = optional(string),
    tags                         = optional(map(string)),
  })
}

locals {
  config = defaults(var.config, {
    maintenance_window  = "sun:05:00-sun:09:00"
    engine              = "redis"
    engine_version      = "3.2.10"
    node_type           = "cache.t2.medium"
    num_cache_nodes     = 1
    port                = 6379
    apply_immediately   = false
    az_mode             = "single-az"
    security_group_ids = "sg-000001"
  })
}

output "security_group_ids" {
  value = local.config.security_group_ids
}

Output

+ security_group_ids = []

@QuingKhaos
Copy link

@alisdair we have the same problems with maps. Some input objects should be able to supply tags, but optional. So the default would be either {} or referencing another variable.

variable "child_accounts" {
  description = "Map of child accounts to create. The map key is the name of the account and the value is an object containing account configuration variables. See the comments below for what keys and values this object should contain."

  # The key for the account are used to reference them in outputs.
  type = map(object({
    # Human readable name for the account.
    name = string

    # Email address for the AWS account.
    email = string

    # Organization Unit for the account, defined as key in `organizational_units`.
    ou_key = string

    //noinspection TFIncorrectVariableType
    tags = optional(map(string))
  }))
}

locals {
  child_accounts = defaults(var.child_accounts, {
    tags = {}
  })
}

Error:

│ Error: Invalid function argument
│ 
│   on variables.tf line 62, in locals:
│   62:   child_accounts = defaults(var.child_accounts, {
│   63:     tags = {}
│   64:   })
│ 
│ Invalid value for "defaults" parameter: .tags: invalid default value for
│ string: string required.

Or similar to it:

locals {
  child_accounts = defaults(var.child_accounts, {
    tags = var.default_tags
  })
}
│ Error: Invalid function argument
│ 
│   on variables.tf line 62, in locals:
│   62:   child_accounts = defaults(var.child_accounts, {
│   63:     tags = var.default_tags
│   64:   })
│     ├────────────────
│     │ var.default_tags is a map of string, known only after apply
│ 
│ Invalid value for "defaults" parameter: .tags: invalid default value for
│ string: string required.

@QuingKhaos
Copy link

QuingKhaos commented Apr 19, 2021

👀 I got it running for setting another map as default value with merge():

locals {
  child_accounts = {
    for key, child in var.child_accounts : key => merge(
      child,
      {
        tags = merge(child.tags, var.default_tags)
      }
    )
  }
}

@alisdair
Copy link
Contributor

I think this ticket needs to be re-opened. At the moment defaults is only usable with plain strings/numbers.

Thanks for the follow-up, and I agree. While this statement isn't completely correct (see below), I agree that it's worth retitling and reopening this ticket, which I'm doing now.

defaults works as designed with lists which have non-primitive element types. For example, here's a list of objects where defaults are applied correctly:

terraform {
  experiments = [module_variable_optional_attrs]
}

variable "website" {
  type = object({
    base_url = string
    ttl = optional(number)

    pages = list(
      object({
        path = string
        data = string
        draft = optional(bool)
      })
    )
  })
}

locals {
  website = defaults(var.website, {
    ttl = 86400
    pages = {
      draft = false
    }
  })
}

output "website" {
  value = local.website
}
website = {
  base_url = "https://example.com"
  pages = [
    {
      path = "index.txt"
      data = "Hello, world"
    },
    {
      path = "release.txt"
      data = "Our app shipped today!"
      draft = true
    },
  ]
}
Apply complete! Resources: 0 added, 0 changed, 0 destroyed.

Outputs:

website = {
  "base_url" = "https://example.com"
  "pages" = tolist([
    {
      "data" = "Hello, world"
      "draft" = false
      "path" = "index.txt"
    },
    {
      "data" = "Our app shipped today!"
      "draft" = true
      "path" = "release.txt"
    },
  ])
  "ttl" = 86400
}

As you have noticed however, optional lists with primitive element types (string, number, bool) have no useful way to be used with the defaults function. This is a problem with the design of the function, which is why we continue to review it and hope to improve the design in future Terraform versions.

@QuingKhaos Thanks for sharing that workaround! Until we find a way to redesign defaults, using functions like merge and coalescelist is likely the best way forward.

@alisdair alisdair reopened this Apr 19, 2021
@alisdair alisdair changed the title lists defined with optional not working as expected with defaults Objects with optional(list(string)) attributes cannot have useful defaults applied with defaults function Apr 19, 2021
@alisdair alisdair added enhancement and removed bug working as designed confirmed as reported and closed because the behavior is intended labels Apr 19, 2021
@adamday2
Copy link

Additional Context

Worthwhile mentioning also that for_each loops based on values from defaults do not work and complain that its value cannot be determined until apply.

Can anyone comment on if the information described in the additional context above is a bug or as designed?

@alisdair
Copy link
Contributor

alisdair commented Apr 19, 2021

Can anyone comment on if the information described in the additional context above is a bug or as designed?

This information is not correct. Values returned from defaults can be used in for_each, so long as the inputs to defaults are wholly-known.

If you find this not to be the case, please open a separate issue with reproduction steps.

@dzintars
Copy link

dzintars commented Apr 19, 2021

Pretty much same issue. Downgraded back to v0.14.7

Error: Invalid function argument
│
│   on locals.tf line 14, in locals:14:   network = defaults(var.network, {
│   15:     id   = random_uuid.instance.id
│   16:   })
│     ├────────────────
│     │ random_uuid.instance.id will be known only after apply
│
│ Invalid value for "defaults" parameter: .id: invalid default value for string: string required.

@Yunsang-Jeong
Copy link

Yunsang-Jeong commented Apr 26, 2021

Below my code works even though additional_tag, cidr_blocks are not define in defaults function.

Configuration

terraform {
  experiments = [module_variable_optional_attrs]
}

variable "security_groups" {
  description = "Security gorups"
  type = list(object({
    description      = string
    name_tag_postfix = string
    additional_tag   = optional(map(string))
    ingresses = list(object({
      description                      = string
      from_port                        = string
      to_port                          = string
      protocol                         = string
      cidr_blocks                      = optional(list(string))
      source_security_group_identifier = optional(string)
      source_security_group_id         = optional(string)
      self                             = optional(bool)
    }))
  }))
}


locals {
  security_groups = defaults(var.security_groups, {
    # additional_tag = {} ---> It applies without definition!
    ingresses = {
      # cidr_blocks = [] ---> It applies without definition!
      source_security_group_identifier = ""
      source_security_group_id         = ""
      self                             = false
    }
  })
}

output "out" {
  value = local.security_groups
}

Input values

security_groups = [
  {
    identifier       = "ec2-bastion"
    name_tag_postfix = "ec2-bastion"
    description      = "the security group for bastion host"
    ingresses = [{
      description = "SSH connection"
      from_port   = "22"
      to_port     = "22"
      protocol    = "tcp"
      cidr_blocks = ["1.2.3.4/32"]
      }
    ]
  },
  {
    identifier       = "elb-web"
    description      = "the security group for prod elb"
    name_tag_postfix = "elb-web"
    ingresses = [{
      description = "Web service"
      from_port   = "80"
      to_port     = "80"
      protocol    = "tcp"
      cidr_blocks = ["0.0.0.0/0"]
      }
    ]
  },
  {
    identifier       = "ec2-web"
    name_tag_postfix = "ec2-web"
    description      = "the security group for prod elb"
    ingresses = [{
      description                      = "Web service"
      from_port                        = "80"
      to_port                          = "80"
      protocol                         = "tcp"
      source_security_group_identifier = "elb-web"
      }, {
      description                      = "Connect from bastion host"
      from_port                        = "22"
      to_port                          = "22"
      protocol                         = "tcp"
      source_security_group_identifier = "ec2-bastion"
      }
    ]
  }
]

Outputs

[
    {
        additional_tag   = {}
        description      = "the security group for bastion host"
        ingresses        = [
            {
                cidr_blocks                      = [
                    "1.2.3.4/32",
                ]
                description                      = "SSH connection"
                from_port                        = "22"
                protocol                         = "tcp"
                self                             = false
                source_security_group_id         = ""
                source_security_group_identifier = ""
                to_port                          = "22"
            },
        ]
        name_tag_postfix = "ec2-bastion"
    },
    {
        additional_tag   = {}
        description      = "the security group for prod elb"
        ingresses        = [
            {
                cidr_blocks                      = [
                    "0.0.0.0/0",
                ]
                description                      = "Web service"
                from_port                        = "80"
                protocol                         = "tcp"
                self                             = false
                source_security_group_id         = ""
                source_security_group_identifier = ""
                to_port                          = "80"
            },
        ]
        name_tag_postfix = "elb-web"
    },
    {
        additional_tag   = {}
        description      = "the security group for prod elb"
        ingresses        = [
            {
                cidr_blocks                      = []
                description                      = "Web service"
                from_port                        = "80"
                protocol                         = "tcp"
                self                             = false
                source_security_group_id         = ""
                source_security_group_identifier = "elb-web"
                to_port                          = "80"
            },
            {
                cidr_blocks                      = []
                description                      = "Connect from bastion host"
                from_port                        = "22"
                protocol                         = "tcp"
                self                             = false
                source_security_group_id         = ""
                source_security_group_identifier = "ec2-bastion"
                to_port                          = "22"
            },
        ]
        name_tag_postfix = "ec2-web"
    },
]

@kurianoff
Copy link

Came across the same issue: defaults on complex types does not work. As a workaround, I used simple merge() of the input with the map of default values, but an additional clean up was needed before merge because unset optional attributes would come into the map as nulls thus overriding the provided default values.

Configuration of the Variable with optional attribute:

terraform {
  experiments = [module_variable_optional_attrs]
}

variable "function" {
  type = object({
    name    = string
    zip     = string
    hash    = optional(string)
    handler = string
    runtime = string
    memsize = optional(string)
    timeout = optional(string)
    role    = optional(map(any))

    vpc_config = optional(object({
      subnet_ids      = list(string)
      security_groups = list(string)
    }))

    env                = optional(map(any))
    policies           = optional(map(any))
    policy_attachments = optional(list(string))
    permissions        = optional(map(any))
    s3_permissions     = optional(map(any))
    track_versions     = optional(bool)
  })
  description = "(Required) All parameters needed to describe lambda function: name, zip archive with the code, VPC configuration, policies, invocation permissions, and such"
}

A-la "Defaults" block (Input Cleanup + Merge)

  • defaults is the defaults block
  • clean_input is the cleanup of null (unset) values from the input variable var.function
  • lambda is the final merged variable that has all defaults + everything that was in the input
locals {

  defaults = {
    policy_attachments = []
    permissions        = {}
    policies           = {}
    env                = []
    s3_permissions     = {}

    runtime        = "nodejs14.x"
    memsize        = 128
    timeout        = 900
    track_versions = false
  }

  clean_input = {
    for k, v in var.function : k => v if v != null
  }

  lambda = merge(local.defaults, local.clean_input)
}

output "lambda" {
   value = local.lambda
}

Input Value

function = {
    name        = random_pet.function_name.id
    description = "Example Hello World Lambda Function"

    zip     = "./lambda/lambda_handler.zip"
    handler = "lambda_handler.lambda_handler"
    runtime = "python3.8"
    memsize = "256"

    env = {
      STAGE_NAME    = random_pet.stage_name.id
      FUNCTION_NAME = random_pet.function_name.id
      REGION        = local.region
    }
}

Defaults

  defaults = {
    policy_attachments = []
    permissions        = {}
    policies           = {}
    env                = []
    s3_permissions     = {}

    runtime        = "nodejs14.x"
    memsize        = 128
    timeout        = 900
    track_versions = false
  }

Final Output

Without Cleanup (notice null values where it should be defaults)

  + lambda = {
      + env                = {
          + "FUNCTION_NAME" = (known after apply)
          + "REGION"        = "us-east-1"
          + "STAGE_NAME"    = (known after apply)
        }
      + handler            = "lambda_handler.lambda_handler"
      + hash               = null
      + memsize            = "256"
      + name               = (known after apply)
      + permissions        = null
      + policies           = null
      + policy_attachments = null
      + role               = null
      + runtime            = "python3.8"
      + s3_permissions     = null
      + timeout            = null
      + track_versions     = null
      + vpc_config         = null
      + zip                = "./lambda/lambda_handler.zip"
    }

With Cleanup (no nulls)

lambda = {
  "env" = tomap({
    "FUNCTION_NAME" = "knowing-sawfish"
    "REGION" = "us-east-1"
    "STAGE_NAME" = "pro-owl"
  })
  "handler" = "lambda_handler.lambda_handler"
  "memsize" = "256"
  "name" = "knowing-sawfish"
  "permissions" = {}
  "policies" = {}
  "policy_attachments" = []
  "runtime" = "python3.8"
  "s3_permissions" = {}
  "timeout" = 900
  "track_versions" = false
  "zip" = "./lambda/lambda_handler.zip"
}

@Justin-DynamicD
Copy link

@kurianoff has summarized my exact issue where if an entire object is optional it simply returns null and doesn't populate the minimal values. Using a merge solves the issue, but using both a merge AND the optional defaults function leads to a very difficult to read solution. I feel it's a behavioral issue when you set a default for an attribute and get a null back null instead of the object defined.

@korinne
Copy link

korinne commented Jun 8, 2022

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!

@apparentlymart
Copy link
Contributor

Hello again!

As a result of this and other feedback that the defaults function wasn't able to treat collection values in a way that was intuitive for all situations, we removed the function from the final incarnation of the feature and instead integrated support for default values directly into the type constraint syntax.

The example in the original issue comment could be adapted to the final design of the feature as follows:

variable "config" {
  description = "(Required) Module configuration"
  type = object({
    cluster_id                   = string,
    replication_group_id         = optional(string),
    engine                       = optional(string, "redis"),
    engine_version               = optional(string, "3.2.10"),
    maintenance_window           = optional(string, "sun:05:00-sun:09:00"),
    node_type                    = optional(string, "cache.t2.medium"),
    num_cache_nodes              = optional(number, 1),
    parameter_group_name         = optional(string),
    port                         = optional(number, 6379),
    subnet_group_name            = optional(string),
    security_group_names         = optional(list(string)),
    security_group_ids           = optional(list(string), ["sg-000000"]),
    apply_immediately            = optional(bool, false),
    snapshot_arns                = optional(list(string)),
    snapshot_name                = optional(string),
    snapshot_window              = optional(string),
    snapshot_retention_limit     = optional(string),
    notification_topic_arn       = optional(string),
    az_mode                      = optional(string, "single-az"),
    availability_zone            = optional(string),
    preferred_availability_zones = optional(list(string)),
    final_snapshot_identifier    = optional(string),
    tags                         = optional(map(string)),
  })
}

This new syntax, unlike the experimental defaults function, is able to distinguish between a default value for a collection as a whole and a default value for an individual attribute across all elements of a collection of objects, thereby resolving the ambiguity that cause the defaults function to handle the described situation poorly.

This functionality is already available in the release candidate of Terraform v1.3.0 and will be included in the forthcoming final release. Thanks!

@github-actions
Copy link
Contributor

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 Oct 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests