Incorrectly generated iterator for ComplexList

[ignaloidas]

Community Note

• Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
• Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request
• If you are interested in working on this issue or have submitted a pull request, please leave a comment

cdktf & Language Versions

cdktf 0.12.0 Python 3.9.12

Debug Output

Debug output

Expected Behavior

A for loop generated to create a mapping to access properties of the objects in the list

Actual Behavior

The list converted into a set of objects, which is an invalid argument for for_each

Steps to Reproduce

#!/usr/bin/env python
from constructs import Construct
from cdktf import App, TerraformStack, TerraformIterator
from cdktf_cdktf_provider_aws import AwsProvider, acm, route53


class MyStack(TerraformStack):
    def __init__(self, scope: Construct, id_: str) -> None:
        super().__init__(scope, id_)
        self.root_domain_name = "example.com"
        self.zone_id = "EXAMPLE_ID"
        self.aws_provider = AwsProvider(self, "AwsProvider")

        acm_certificate = acm.AcmCertificate(
            self,
            "AcmCertificate",
            domain_name=f"*.{self.root_domain_name}",
            subject_alternative_names=[self.root_domain_name],
            validation_method="DNS",
        )

        iterator = TerraformIterator.from_list(
            acm_certificate.domain_validation_options
        )

        validation_records = route53.Route53Record(
            self,
            "ValidationRecords",
            for_each=iterator,
            name=iterator.get_string("resource_record_name"),
            records=[iterator.get_string("resource_record_value")],
            type=iterator.get_string("resource_record_type"),
            allow_overwrite=True,
            ttl=300,
            zone_id=self.zone_id,
        )


app = App()
MyStack(app, "cdktf_problem")
app.synth()

References

Prior references on how to work around this: https://discuss.hashicorp.com/t/issue-with-acm-certs-and-route53-in-cdktf/22270 https://github.com/hashicorp/terraform-cdk/issues/430#issuecomment-831511019

[DanielMSchmidt]

It looks like we synthesize "for_each": "${toset(tolist(aws_acm_certificate.AcmCertificate.domain_validation_options))}", where domain_validation_options is already a Set of objects. The Error terraform throws indicates we should use a map with static keys instead. We should be able to get the values by registering what is referenced from the iterator (even though that might be too much if the same iterator i used in multiple cases) and generate such a map statement.

[jsteinich]

There are actually a couple issues here:

1. TerraformIterator should be able to use a set directly without converting to a list and back to a set (not really related to this issue).
2. Need support for combining a for expression with an iterator.

Expanding on point 2:

• This comes from https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/acm_certificate_validation
  • An L2 construct could make this so much easier to use regardless of any other changes
• We already have ForExpression and TerraformIterator; combining the two (in a way that is jsii compatible) would make this possible

Possible method on TerraformIterator:

public for(values: { [key: string]: any }, key?: any): TerraformIterator {
  return new ForTerraformIterator(forExpression(this, values, key);
}

This would result in something like:

opts_iterator = TerraformIterator.from_list(acm_certificate.domain_validation_options)
iterator = opts_iterator.for({
  "name": opts_iterator.get_string("resource_record_name"),
  "record": opts_iterator.get_string("resource_record_value"),
  "type": opts_iterator.get_string("resource_record_type")
},
opts_iterator.get_string("domain_name"))

validation_records = route53.Route53Record(
    self,
    "ValidationRecords",
    for_each=iterator,
    name=iterator.get_string("name"),
    records=[iterator.get_string("record")],
    type=iterator.get_string("type"),
    allow_overwrite=True,
    ttl=300,
    zone_id=self.zone_id,
)

Ultimately this seems worse to me than just using an escape hatch; hence having an L2 construct for this specific (common) use case that hides all the complexity from the user.

[ignaloidas]

I would still be for having support in the iterator, if only for making the L2 construct, for the simple reason that it would give the correct output reliably. I encountered this while making such L2 construct myself, and I'd really like to remove the escape hatches I used there. I don't think this is the only construct where such case happens, and having at least a way to do it without needing an escape hatch would be good.

Also, seems like ForExpression isn't exported into generated Python packages, I assume this is intended?

Edit: I'm pretty certain that this method would be needed to iterate through every ComplexList, so maybe even worth giving that case a different constructor? Something like this

iterator = TerraformIterator.from_complex_list(
  acm_certificate.domain_validation_options),
  "domain_name",
  {
    "name": "resource_record_name",
    "record": "resource_record_value",
    "type": "resource_record_type",
  },
)

validation_records = route53.Route53Record(
    self,
    "ValidationRecords",
    for_each=iterator,
    name=iterator.get_string("name"),
    records=[iterator.get_string("record")],
    type=iterator.get_string("type"),
    allow_overwrite=True,
    ttl=300,
    zone_id=self.zone_id,
)

[jsteinich]

I would still be for having support in the iterator, if only for making the L2 construct, for the simple reason that it would give the correct output reliably. I encountered this while making such L2 construct myself, and I'd really like to remove the escape hatches I used there. I don't think this is the only construct where such case happens, and having at least a way to do it without needing an escape hatch would be good.

Ordinarily I would agree; however, currently JSII limitations make it so there may not be an implementation that is actually easier/safer to use. I'm poking the JSII team to see if there might be a path to eliminate some of the limitations so that we can come up with something that is truly better. That said we are still bound by #435.

Also, seems like ForExpression isn't exported into generated Python packages, I assume this is intended?

It is intentional since it's not a very friendly api, though there may be some other cases where it would be helpful.

Edit: I'm pretty certain that this method would be needed to iterate through every ComplexList, so maybe even worth giving that case a different constructor? Something like this

#2013 is roughly meant to cover that.

One thing that makes this case more complicated is that there are actually 2 iterations happening. Another way to think of it is that we are doing a series of transformations (pipeline). Iterators provide a primitivize version of a single step pipeline. We could probably provide a way to chain them, but that's pretty clunky.

[cyrfer]

Would it be possible to make Terraform.fromList return an ES6 style iterator (MDN, blog), so that we could do things like this?

import * as aws from "@cdktf/provider-aws"

const zone = new aws.route53Zone.Route53Zone(this, "zone", {...blah})
const cert = new aws.acmCertificate.AcmCertificate(this, "cert", {...blah})
const iterator = TerraformIterator.fromList(cert.domainValidationOptions)
const validationRecords = [...iterator].map((each, i) => {
  return new aws.route53Record.Route53Record(this, `validation-record-${i}`, {...each,blah})
})

I don't think the CDK needs to expose a forEach property in higher level languages like Typescript.

[DanielMSchmidt]

Would it be possible to make Terraform.fromList return an ES6 style iterator (MDN, blog), so that we could do things like this?

We discussed this in depth, but the short answer is no, it's not possible as of right now. JSII has an issue tracking support for language-native iterators, if that would be done we could hook into their solution and solve this in a nicer way. As this is not done yet we would need to resort to user-land solutions. The main problem when implementing this is that our iterator is a Token and not the real list. Normal tokens are easy to resolve, we see them in a property, look into our table what reference or terraform expression they contain and replace that when building the terraform json for the stack.

If your code example from above runs as is we would have a single Route53Record instance at hand. We could detect the usage of the each and derive someone is iterating over it, propagating the for_each property in HCL. But what if you are new, don't have this context and do [...iterator].slice(1).map(? We have no way of detecting this and warning you that you are doing something unintentional. We can't be sure that the terraform code we generate is exactly matching what you intended to express when we need to reverse engineer your usage of iterators. This is why we went with the uglier option that is very direct, it's clear what is happening, no magic, nothing can go wrong.

Now if JSII would add support for iterators, they would solve this tracking problem at a more foundational level I would imagine which could open up the path you mentioned for us.

[ansgarm]

Closing this as #3273 adds iterator support for complex lists such as the domain_validation_options attribute is.

[github-actions[bot]]

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've 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.