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

dynamic block "iterator" argument is not well documented and gives poor feedback when used incorrectly #32432

Open
soudaburger opened this issue Dec 22, 2022 · 4 comments

Comments

@soudaburger
Copy link

Terraform Version

Terraform v1.3.6
on darwin_arm64
+ provider registry.terraform.io/hashicorp/google v4.44.1
+ provider registry.terraform.io/hashicorp/google-beta v4.44.1
+ provider registry.terraform.io/hashicorp/tls v4.0.4

Affected Pages

https://developer.hashicorp.com/terraform/language/expressions/dynamic-blocks

What is the docs issue?

After finding this issue, it's apparent that nothing will be fixed for the "iterator" "bug". I'm not here to suggest work be done other than show an actual example in the docs to support the use of iterator properly, without quotes. This example would have saved me a ton of trial and error and googling. If it wasn't for the one comment to suggest not commenting the iterator value, I would have been stuck. I believe the behavior is counterintuitive, and as such, should definitely have some explicit example in the documentation.

Proposal

Just add a formal example of using an iterator in a dynamic block.

References

#22340

@soudaburger soudaburger added documentation new new issue not yet triaged labels Dec 22, 2022
@apparentlymart
Copy link
Member

Hi @soudaburger,

Given which issue you've linked to I assume what you're discussing is the fact that dynamic blocks don't work for situations where providers are using the legacy attributes as blocks mode to make Terraform pretend that a particular attribute is a nested block type.

The way the docs for this are supposed to work is in two parts:

  • Any provider that is using this legacy mode should mention it for each attribute it's relevant to, and link to the "Attributes as Blocks" page as part of that mention.
  • The "Attributes as Blocks" page is then the place to discuss all of the oddities that result from that strange mode.

The goal then is to avoid bringing up complexity that is only relevant in some very specific legacy cases in the general docs — thereby making the general docs more accessible to the general reader — while still allowing folks who are affected by it to find the relevant documentation.

With that principle in mind, I think what we ought to do here is be much more explicit on the Attributes as Blocks page that dynamic blocks do not work for any attribute which uses this mode. Currently that page does describe what you ought to do instead (the examples in Arbitrary Expressions with Argument Syntax) but it only mentions dynamic blocks in passing as if they are a viable alternative that just isn't recommended.

Instead, I think we should mention much more prominently that dynamic blocks must never be used for these legacy attributes, and then present the existing examples as the only way to dynamically populate those.

Once we have the attributes as blocks page being clear about this, the existing links from provider docs to that page should therefore connect the two so that folks using the affected providers can find the relevant documentation. If there are existing examples of providers using this mode but not documenting it then I would suggest opening a documentation bug in the provider's own repository, since that is where the teams maintain each provider's own documentation.

With that said: the issue you linked to is a long one with lots of discussion, and so I may well have totally misunderstood what part of it you were referring to here. If so, it would help if you could link to a specific comment in that discussion or otherwise add some more context so I can understand what part of that discussion you'd like to see reflected in the main documentation. Thanks!

(Even if you did have something else in mind here, I think we are missing some clear guidance in the docs about how dynamic blocks interact with the Attributes as Blocks mode, and so if this issue does end up being about something else then I'll open a new issue about that.)

@soudaburger
Copy link
Author

I appreciate the attempt at a reply, but I definitely think you misunderstood what I was intending. Let me try this a different way.

The documentation I linked to has references to using iterator when you need to specify the iterator object name to be used in a dynamic block. This is actually really helpful in certain circumstances. The problem is two fold:

  1. The documentation references the iterator usage, but never actually SHOWS you how to use it. It's sort of just implied.
  2. My assumption is that iterator = value where value is a string and, thus, value should really be "value".

This is where the link to the issue I posted becomes important. Someone (thankfully) at the bottom of that issue references that it should be iterator = value... UNQUOTED. If you quote it, you run into the variable issue that's mentioned by several people. When you do NOT quote value, terraform behaves as expected.

Example code that throws an error:

  private_visibility_config {
    dynamic "networks" {
      for_each = each.value.networks
      iterator = "network"
      content {
        network_url = google_compute_network.network[network.value].name
      }
    }
  }

Example code that does NOT throw an error:

  private_visibility_config {
    dynamic "networks" {
      for_each = each.value.networks
      iterator = network
      content {
        network_url = google_compute_network.network[network.value].name
      }
    }
  }

The ONLY difference is quoting of the iterator value. It's this piece that I'd like to simply have added to the example documentation so that people (like me) can better understand how to implement the iterator and not rely on googling for examples and to stumble upon the issue I linked.

Does that help better explain what I'm looking for here? Thanks!

@apparentlymart
Copy link
Member

Thanks for that extra context, @soudaburger!

Based on that, I'm guessing you are talking about this comment: #22340 (comment)

I'll copy its contents in here just so it's easier to read this issue in future:

Same issue on helm provider using set

  dynamic "set" {
    for_each = [local.tag_keys]
    iterator = "tag"
    content {
      name  = "podLabels[${index(local.tag_keys, tag.key)}]"
      value = "${tag.key}=${lookup(var.tags, tag.key)}"
    }
  }

Error

Error: Unknown variable                                                                                                                                                                       
                                                                                               
  on ../modules/external_dns/main.tf line 54, in resource "helm_release" "external_dns":                                                                                                      
  54:     for_each = [local.tag_keys]                                                                                                                                                         
                                                                                               
There is no variable named "local".


Error: Invalid expression

  on ../modules/external_dns/main.tf line 55, in resource "helm_release" "external_dns":
  55:     iterator = "tag"
A single static variable reference is required: only attribute access and
indexing with constant keys. No calculations, function calls, template
expressions, etc are allowed here.

It works if you remove the quotes like this:

iterator = tag

It seems like that other issue got confused partway through somehow, since it started off talking about a quirk of "Attributes as Blocks" mode but then in the middle started talking about using dynamic blocks in general. Oh well... it happens!

Based on this I think there are two things to fix here, the first of which is what I believe you are proposing:

  1. The documentation for dynamic blocks acknowledges that there is an argument called iterator but doesn't actually explain how to use it or show an example of using it.

    I think to give a realistic example we should look for a real resource type belonging to a real provider that has an example of two levels of nested blocks where both have the same block type name, since that is the main situation where this would be needed and so I think it's best to show it in that context to help readers decide between using that argument or not using it. (I think most dynamic blocks should not use it, and I suspect that's why the docs on it are currently only rudimentary.)

  2. The error message Terraform returns here is a very generic one that doesn't give any feedback about what might be wrong.

    I think this is a generic error message that the underlying HCL library would produce any time it's asked to interpret an expression as a static symbol. For example, I think Terraform would produce the same message if someone were to write a quoted string inside resource depends_on or ignore_changes.

    In this case it seems clear that some authors will attempt to use a quoted name and so it seems worth adding an extra check to see if the expression looks like a quoted string template and return a specialized error message which specifically says not to write the name in quotes.

    Along with that special case, I think we could also improve the error message in the general case. "A static variable reference" isn't really what Terraform wants here anyway, but rather the name of a new variable to be added to the scope when evaluating the content block. It would be clearer to say "a variable name to use to represent the current element", or something along those lines. If we already had the special case for detecting a quoted string template then this modified error message would appear only if the author wrote something really bizarre as the value of that argument, like iterator = [], so this would be seen much less often than the specialized message about removing quotes.

Thanks again! (and as I said in my previous comment, I'm going to open another issue about documenting the fact that dynamic blocks don't work for "Attributes as Blocks" mode, since that also seems missing even though that wasn't what you were discussing here.)

@apparentlymart apparentlymart changed the title Add iterator example to avoid confusion dynamic block "iterator" argument is not well documented and gives poor feedback when used incorrectly Dec 23, 2022
@apparentlymart apparentlymart added enhancement config and removed new new issue not yet triaged labels Dec 23, 2022
@soudaburger
Copy link
Author

Much appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants