cdktf: construct name should be unique per resource type [same as terraform]

[shinebayar-g]

Description

Currently Construct naming experience is not consistent with Terraform. Consider the following example:

class DevStack extends TerraformStack {
    constructor(scope: Construct, id: string) {
        super(scope, id);

        new GoogleProvider(this, "dev", {
            project: "foo",
        });

        const vpc = new ComputeNetwork(this, "main", {
            name: "main",
            routingMode: "REGIONAL",
            autoCreateSubnetworks: false,
        });

        new ComputeRoute(this, "main", {
            name: "egress-internet",
            description: "route through IGW to access internet",
            destRange: "0.0.0.0/0",
            network: vpc.id,
            nextHopGateway: "default-internet-gateway",
        });
    }
}

Above code will give an error:

throw new Error(`There is already a Construct with name '${childName}' in ${typeName}${name.length > 0 ? ' [' + name + ']' : ''}`);
Error: There is already a Construct with name 'main' in DevStack [dev]

Ideally resource names shouldn't include resource types. In Terraform, we just name it as google_compute_network.main and google_compute_route.main as described in https://developer.hashicorp.com/terraform/language/resources/syntax#resource-syntax

Because of this we're forced to duplicate the resource type in the name to get around the error.

class DevStack extends TerraformStack {
    constructor(scope: Construct, id: string) {
        super(scope, id);

        new GoogleProvider(this, "dev", {
            project: "foo",
        });

        const vpc = new ComputeNetwork(this, "main-vpc", {
            name: "main",
            routingMode: "REGIONAL",
            autoCreateSubnetworks: false,
        });

        new ComputeRoute(this, "main-route", {
            name: "egress-internet",
            description: "route through IGW to access internet",
            destRange: "0.0.0.0/0",
            network: vpc.id,
            nextHopGateway: "default-internet-gateway",
        });
    }
}

which ultimately leaks to the terraform resource names:

     Terraform will perform the following actions:
dev    # google_compute_network.main-vpc (main-vpc) will be created
       + resource "google_compute_network" "main-vpc" {
           + auto_create_subnetworks                   = false
           + delete_default_routes_on_create           = false
           + gateway_ipv4                              = (known after apply)
           + id                                        = (known after apply)
           ...

       # google_compute_route.main-route (main-route) will be created
       + resource "google_compute_route" "main-route" {
           + description            = "route through IGW to access internet"
           + dest_range             = "0.0.0.0/0"
           + id                     = (known after apply)
           + name                   = "egress-internet"
           + network                = (known after apply)
           ...

Resources declared inside Constructs even have random suffix attached to them which makes this constraint even more meaningless.

References

https://developer.hashicorp.com/terraform/language/resources/syntax#resource-syntax

Help Wanted

• [ ] I'm interested in contributing a fix myself

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

[ansgarm]

Hi @shinebayar-g 👋

Thank you for raising this. I agree from a Terraform point of view – unfortunately, we can't control this, as the underlying construct package (which is used by all CDKs), makes no distinction between types of constructs and requires a unique name within each scope.

However, you can work around this by using .overrideLocalId():

new RandomProvider(this, "random");

new Password(this, "password", { length: 16 });
const pet = new Pet(this, "pet-password", {});
pet.overrideLogicalId("password");

produces:

"resource": {
    "random_password": {
      "password": {
        "//": {
          "metadata": {
            "path": "test-override/password",
            "uniqueId": "password"
          }
        },
        "length": 16
      }
    },
    "random_pet": {
      "password": {
        "//": {
          "metadata": {
            "path": "test-override/pet-password",
            "uniqueId": "password"
          }
        }
      }
    }
  },

[DanielMSchmidt]

If you feel strongly about keeping the terraform naming conventions in place in your CDKTF application you can extend the existing resources:

import { S3Bucket, S3BucketConfig } from "./.gen/providers/aws/s3-bucket"

class BetterS3Bucket extends S3Bucket {
  constructor(scope: Construct, name: string, config: S3BucketConfig) {
    super(scope, `${S3Bucket.tfResourceType}-${name}`, config);
    this.overrideLogicalId(name)
  }
}

For this class the same constraints as in Terraform would apply.

[shinebayar-g]

Thanks for the explanation and workarounds.

as the underlying construct package (which is used by all CDKs), makes no distinction between types of constructs and requires a unique name within each scope.

It looks like every resource is extended from cdktf.TerraformResource class and has tfResourceType property. Maybe using this information it might be doable. I hope this limitation will be improved in the future.

[StefanTheWiz]

If you feel strongly about keeping the terraform naming conventions in place in your CDKTF application

Err... we do feel strongly about this. CDKTF will be a tough sell in any organisation unless it saves us time writing code.

Thank you for raising this. I agree from a Terraform point of view – unfortunately, we can't control this, as the underlying construct package (which is used by all CDKs), makes no distinction between types of constructs and requires a unique name within each scope.

I don't fully agree with the "we can't control this" part.

Child classes of Construct that are defined in the cdktf package have access to the element type so one of these classes that's closest to the Construct class in terms of hierarchy could prefix the id with the element/resource type before passing it to the parent class, no?

In terms of practical solutions for people who might have wasted time looking into a nice hack for this, I just gave up and used uuid for now 😄

import { S3BucketServerSideEncryptionConfigurationA } from "@cdktf/provider-aws/lib/s3-bucket-server-side-encryption-configuration";
import { v4 as uuid } from "uuid";

new S3BucketServerSideEncryptionConfigurationA(this, uuid(), {
      bucket: "my-bucket",
      rule: [{ applyServerSideEncryptionByDefault: { sseAlgorithm: "aws:kms" } }],
    }).overrideLogicalId(id);

Code brevity is really appreciated, especially for long resource names like S3BucketServerSideEncryptionConfiguration...

[MiguelAraCo]

The following patches seem to be working on my side (haven't done extensive testing):

constructs

diff --git a/lib/construct.js b/lib/construct.js
index 4bb61d6c0c27e4fcd5a3abcd4fbd762827804890..cbe28b328b94660a8c58be1cbc8a0d339d1dd2aa 100644
--- a/lib/construct.js
+++ b/lib/construct.js
@@ -20,7 +20,9 @@ class Node {
     static of(construct) {
         return construct.node;
     }
-    constructor(host, scope, id) {
+    // Store originalId to allow constructs with different types to have the same id
+    // See: https://github.com/hashicorp/terraform-cdk/issues/3290
+    constructor(host, scope, id, originalId) {
         this.host = host;
         this._locked = false; // if this is "true", addChild will fail
         this._children = {};
@@ -30,6 +32,7 @@ class Node {
         this._validations = new Array();
         id = id ?? ''; // if undefined, convert to empty string
         this.id = sanitizeId(id);
+        this.originalId = originalId !== undefined ? sanitizeId(originalId) : undefined;
         this.scope = scope;
         if (scope && !this.id) {
             throw new Error('Only root constructs may have an empty ID');
@@ -422,8 +425,8 @@ class Construct {
      * the ID includes a path separator (`/`), then it will be replaced by double
      * dash `--`.
      */
-    constructor(scope, id) {
-        this.node = new Node(this, scope, id);
+    constructor(scope, id, originalId) {
+        this.node = new Node(this, scope, id, originalId);
         // implement IDependable privately
         dependency_1.Dependable.implement(this, {
             dependencyRoots: [this],

cdktf

diff --git a/lib/terraform-element.js b/lib/terraform-element.js
index 7c142f61c501b2072959219daaafa3b744c7d3fe..bf61f1b5f24e17e66f90938b0216613244117962 100644
--- a/lib/terraform-element.js
+++ b/lib/terraform-element.js
@@ -12,10 +12,20 @@ const terraform_stack_1 = require("./terraform-stack");
 const tfExpression_1 = require("./tfExpression");
 const errors_1 = require("./errors");
 const TERRAFORM_ELEMENT_SYMBOL = Symbol.for("cdktf/TerraformElement");
+
+function sanitize(prefix) {
+  return prefix.replaceAll(/[^a-z0-9-_]/ig, "_");
+}
+
 // eslint-disable-next-line jsdoc/require-jsdoc
 class TerraformElement extends constructs_1.Construct {
     constructor(scope, id, elementType) {
-        super(scope, id);
+        // Store originalId to allow constructs with different types to have the same id
+        // See: https://github.com/hashicorp/terraform-cdk/issues/3290
+        const originalId = id;
+        id = sanitize(`${elementType ?? new.target.name}_${id}`);
+
+        super(scope, id, originalId);
         this.rawOverrides = {};
         Object.defineProperty(this, TERRAFORM_ELEMENT_SYMBOL, { value: true });
         this._elementType = elementType;
diff --git a/lib/terraform-stack.js b/lib/terraform-stack.js
index 4fa973fda6420cb134dc14650cd5e7cdc4bd4e7f..9eceee623995a5fd247a47b009adfe6e2958c49e 100644
--- a/lib/terraform-stack.js
+++ b/lib/terraform-stack.js
@@ -136,7 +136,9 @@ class TerraformStack extends constructs_1.Construct {
             ? tfElement.cdktfStack
             : this;
         const stackIndex = node.scopes.indexOf(stack);
-        const components = node.scopes.slice(stackIndex + 1).map((c) => c.node.id);
+        // Store originalId to allow constructs with different types to have the same id
+        // See: https://github.com/hashicorp/terraform-cdk/issues/3290
+        const components = node.scopes.slice(stackIndex + 1).map((c) => c.node.originalId ?? c.node.id);
         return components.length > 0 ? (0, unique_1.makeUniqueId)(components) : "";
     }
     allProviders() {

After applying those patches, internally cdktf should use an id that has the element type baked into it, and use the defined id when generating the terraform state, so this should work as expected:

class DevStack extends TerraformStack {
    constructor(scope: Construct, id: string) {
        super(scope, id);

        new GoogleProvider(this, "dev", {
            project: "foo",
        });

        // cdktf id = ComputeNetwork_main
        // state id = google_compute_network.main
        const vpc = new ComputeNetwork(this, "main", {
            name: "main",
            routingMode: "REGIONAL",
            autoCreateSubnetworks: false,
        });

        // cdktf id = ComputeRoute_main
        // state id = google_compute_route.main
        new ComputeRoute(this, "main", {
            name: "egress-internet",
            description: "route through IGW to access internet",
            destRange: "0.0.0.0/0",
            network: vpc.id,
            nextHopGateway: "default-internet-gateway",
        });
    }
}