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

Disambiguate between lists and sets in the CLI plan output #35170

Open
EugenKon opened this issue May 16, 2024 · 7 comments
Open

Disambiguate between lists and sets in the CLI plan output #35170

EugenKon opened this issue May 16, 2024 · 7 comments
Labels
enhancement new new issue not yet triaged

Comments

@EugenKon
Copy link

EugenKon commented May 16, 2024

Terraform Core Version

v1.8.2

AWS Provider Version

v4.67.0

Affected Resource(s)

  • aws_acm_certificate

Expected Behavior

It should be shown as set, and not as an array:

Actual Behavior

  # aws_acm_certificate.dns-url-redirects will be created
  + resource "aws_acm_certificate" "dns-url-redirects" {
      + arn                       = (known after apply)
      + domain_name               = "xxx.com"
      + domain_validation_options = [
          + {
              + domain_name           = "*.xxx.com"
              + resource_record_name  = (known after apply)
              + resource_record_type  = (known after apply)
              + resource_record_value = (known after apply)
            },
          + {
              + domain_name           = "xxx.gr"
              + resource_record_name  = (known after apply)
              + resource_record_type  = (known after apply)
              + resource_record_value = (known after apply)
            },
          + {
              + domain_name           = "xxx.uk"
              + resource_record_name  = (known after apply)
              + resource_record_type  = (known after apply)
              + resource_record_value = (known after apply)
            },
          + {
              + domain_name           = "xxx.com"
              + resource_record_name  = (known after apply)
              + resource_record_type  = (known after apply)
              + resource_record_value = (known after apply)
            },
        ]
      + id                        = (known after apply)
      + key_algorithm             = (known after apply)
      + not_after                 = (known after apply)
      + not_before                = (known after apply)
      + pending_renewal           = (known after apply)
...

If I try to access to it by index aws_acm_certificate.dns-url-redirects.domain_validation_options[0].resource_record_name I get the error:

│ Error: Invalid index
│
│   on company-redirects.tf line 248, in resource "aws_route53_record" "validation":
│  248:   records = ["${lookup(aws_acm_certificate.dns-url-redirects.domain_validation_options[count.index], "resource_record_value")}"]
│     ├────────────────
│     │ aws_acm_certificate.dns-url-redirects.domain_validation_options is set of object with 4 elements
│     │ count.index is 0
│
│ Elements of a set are identified only by their value and don't have any separate index or key
│ to select with, so it's only possible to perform operations across all elements of the set.

Documentation also says that it is the set: https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/acm_certificate#domain_validation_options
image

Relevant Error/Panic Output Snippet

No response

Terraform Configuration Files

I am trying to access it like described here:azavea/terraform-aws-acm-certificate#3 (comment)

resource "aws_route53_record" "validation" {
  provider = "aws.route53_account"
  count    = "${length(var.subject_alternative_names) + 1}"

  name    = "${lookup(aws_acm_certificate.default.domain_validation_options[count.index], "resource_record_name")}"
  type    = "${lookup(aws_acm_certificate.default.domain_validation_options[count.index], "resource_record_type")}"
  zone_id = "${data.aws_route53_zone.selected[count.index].id}"
  records = ["${lookup(aws_acm_certificate.default.domain_validation_options[count.index], "resource_record_value")}"]
  ttl     = "${var.validation_record_ttl}"
}

Steps to Reproduce

Run terraform plan for the configuration above.

Debug Output

No response

Panic Output

No response

Important Factoids

No response

References

hashicorp/terraform-provider-aws#37544

Would you like to implement a fix?

None

@EugenKon EugenKon added bug new new issue not yet triaged labels May 16, 2024
@jbardin
Copy link
Member

jbardin commented May 16, 2024

Hi @EugenKon,

The plan renderer here is operating as designed. The HCL language doesn't have a distinct syntax for a set, but rather you would use the same [...] syntax used for a list, but the type would be converted in the context of a set type. Since a user would normally write a literal set in the configuration using the [...] syntax (as opposed to wrapping the value in the toset() conversion function), this is how the values are displayed in the plan.

Thanks!

@jbardin jbardin added enhancement and removed bug labels May 16, 2024
@jbardin jbardin changed the title [Bug]: Data displayed in invalid format Disambiguate between lists and sets in the CLI plan output May 16, 2024
@apparentlymart
Copy link
Member

apparentlymart commented May 16, 2024

We have decided to show toset([ ... ]) in some other contexts where things are more ambiguous, such as in terraform console and in output values.

What makes this situation slightly different is that Terraform handles the conversion to set silently when a list or tuple is assigned to this argument, because Terraform knows the provider's schema and so hides that complexity from the user by performing the type conversion automatically.

I think the question to ponder here, then, is whether it's actually valid to hide this detail if the difference between the two is important to how Terraform understands references, as we can see with this error message.

I could appreciate the argument that showing toset([ instead of just [ in the plan output would help new Terraform users learn that this type conversion is happening and so set them up to better understand how the rest of the language behaves as they write more complicated expressions later. However, I could also appreciate the argument that it would add additional clutter to our already-somewhat-cluttered plan rendering, just to draw attention to something that most of the time isn't crucial information.

I expect that answering this will require some research to understand how often this confusion arises, and thus how many people would be helped by including this extra information in the plan output. 🤔

@EugenKon
Copy link
Author

EugenKon commented May 16, 2024

Thank you for for the detailed answer. It is a bit clear to me now. If you are on the way to change the Terraform output, then the best to display information ASIS, without additional guessing, which could make a confusion.

For example, from the output:

+ domain_validation_options = [
          + {
              + domain_name           = "xxx.gr"
              + resource_record_name  = (known after apply)     # For the purpose of example lets call it A
              + resource_record_type  = (known after apply)     # B
              + resource_record_value = (known after apply)     # C
            },

It is not clear how to access underlying elements. Eg.:

  1. domain_validation_options["xxx.gr"].domain_name
  2. domain_validation_options["A"].domain_name
  3. domain_validation_options["B"].domain_name
  4. domain_validation_options["C"].domain_name

hm... Ok. The value is not known, so we can not dump its value as the "key". From now, I believe, the most important thing missed in the dump above is the field name which should be used to access an underlied data. I propose to dump the name of a such field if the value still not known:

 + domain_validation_options = [
+        key: resource_record_name                              # Just for the purpose of the example, lets count this field as its key.
          + {
              + domain_name           = "xxx.gr"
              + resource_record_name  = (known after apply)     # For the purpose of example lets use "A" as its value
              + resource_record_type  = (known after apply)     # B
              + resource_record_value = (known after apply)     # C
            },

Or dump the key's value if it is already known:

 + domain_validation_options = [
-          + {
+          + "xxx.gr" => {
              + domain_name           = "xxx.gr"
              + resource_record_name  = (known after apply)     # For the purpose of example lets use "A" as its value
              + resource_record_type  = (known after apply)     # B
              + resource_record_value = (known after apply)     # C
            },

@apparentlymart @jbardin What do you think?

@jbardin
Copy link
Member

jbardin commented May 16, 2024

@EugenKon, a set data structure doesn't have a key, so I'm not sure what key: resource_record_name would be used for. A set is an unordered collection of values, so you could say each object is its own "key", but there is no way to directly access individual elements of a set.

@EugenKon
Copy link
Author

Ouch. I was thinking about this structure as a map. I see now, that map != set.

Ok. Terraform has a set https://developer.hashicorp.com/terraform/language/expressions/types#set
Is there some technical issue to iterate over set by index?

list          ->            set            
[ value1, value2, value1 ] -> [ value2, value1 ]

@apparentlymart
Copy link
Member

Set elements don't have indices, because there is no stored order for set elements. Sets represent the relatively common case in many remote APIs where there's just a collection of objects (or other values) that exist but aren't considered to be in any particular order.

When working with sets the only operations that typically make sense are:

  • Derive a new value systematically from each element of the set, such as using a splat or for expression.
  • Sort the elements in some way to produce a list and then access elements of the list. This currently only really makes sense for sets of string because Terraform has no generalized sort function.
  • Project the set into a map using some part of each element as a key, again using a for expression, and then access the map elements using those keys.
  • Assert that your set can only have zero or one elements using the one function, and thus get access to the single element without the wrapping set

I understood this as a request to show more clearly in the plan output when an attribute is a set, so that it's clearer to the reader that they need to use the value in one of the ways I described above.

We can't support indexing into a set because the whole purpose of sets is to deal with situations where the remote API does not provide any single identifier for the objects itself, and so Terraform pretending that it does would be fragile if the API happens to return the items in a different order on a future request, causing all of the derived values to be invalidated.

@apparentlymart
Copy link
Member

This seems related to #31102, which was caused by a similar ambiguity between tuple types and list types.

Considering that this has now caused at least two different variations of confusion, I personally feel more inclined to change the plan output to include tolist/toset/tomap annotations when relevant, to help folks learn about these distinctions sooner in their Terraform usage.

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