Skip to content

lib: reify construct-level node.addDependency into resource depends_on #354

Description

@so0k

Description

CDKTN inherits the constructs library's node.addDependency() API, but nothing in the framework ever reads node.dependencies. As a result, a construct-level dependency declared on an L2/L3 construct is silently dropped at synth time — it never becomes depends_on in the generated Terraform.

Today the only way to express ordering is to pass dependsOn: ITerraformDependable[] on each individual L1 TerraformResource/TerraformDataSource. That works, but it forces every abstraction author to manually plumb dependencies through their construct API down to every leaf resource, and it makes constructs' IDependable/DependencyGroup (the standard way to model "logical readiness" that doesn't map to a single resource) unusable.

This is a silent failure. myConstruct.node.addDependency(otherConstruct) compiles, runs, and produces plan output with no warning — it just has no effect on the generated configuration. Users reasonably expect it to work, because it does in AWS CDK, from which the construct programming model, Aspects, and much of the surrounding API were ported.

What's missing

packages/cdktn/src/terraform-dependable.ts is still the empty marker interface, and TerraformResource/TerraformDataSource only consume an explicitly-passed dependsOn array. There's no pass anywhere in TerraformStack or StackSynthesizer that walks node.dependencies and reifies it into resource-level depends_on. (TerraformStack.addDependency() is a separate, stack-to-stack deploy-ordering mechanism and doesn't help here.)

Prior art: AWS CDK

AWS CDK solves this generically during the prepare/synth phase, in core/lib/private/deps.ts:

/**
 * Turn all dependencies added via `source.node.addDependency(target)` into concrete dependencies.
 *
 * The semantics are that every construct underneath the source construct
 * depends on every construct underneath the target construct, ...
 */
export function reifyConstructDependencies(root: IConstruct) { ... }

Mechanically it:

  1. Walks the tree DFS-preorder collecting every construct with a non-empty node.dependencies.
  2. Resolves each entry via Dependable.of(dependable).dependencyRoots — this is what makes DependencyGroup fan out to multiple targets.
  3. Finds all L1s under source and under target (findCfnResources(), a DFS filtered by CfnResource.isCfnResource) and applies sourceResource._addResourceDependency(targetResource) for every pair.

That cross-product is exactly the design question @jsteinich raised in #785 — "Would that add depends_on to every resource within the custom construct?" — and yes, N×M is the semantic AWS CDK actually shipped and has lived with for years. CDKTF ported AWS CDK's Aspects (aspect.ts even says so in its header) but never ported the accompanying reification pass.

Upstream history

  • hashicorp/terraform-cdk#785 — "Add Dependency on Custom Construct", opened 2021-06-18 by @skorfmann, labelled enhancement + priority/important-longterm. Still open at archival. In 2023 @skorfmann asked whether contributions would be accepted and @DanielMSchmidt replied "I think we would accept contributions 👍". Nobody ever built it.
  • hashicorp/terraform-cdk#2727 — closed 2 days after filing with the guidance that this is user-land's problem:

    "The addDependency method you are calling is part of the Construct library and is used for ordering the constructs resolution. It has nothing to do with terraform dependencies. That being said, I am sure one could write an Aspect that adds the Construct level cdktfConstruct.node.dependencies to the CDKTF level cdktfConstruct.dependsOn array."

So this was never rejected on design grounds — it was deferred to user-land, tagged "important-longterm", and then ran out of runway when CDKTF was sunset. CDKTN inherits the gap unchanged, and now has the opportunity to close it.

Why user-land Aspects aren't good enough

We maintain TerraConstructs, an AWS-CDK-style L2/L3 library on CDKTN, and we took exactly the Aspect route @DanielMSchmidt suggested. It's applied to every stack, and the abstraction depends on it completely — our VPC constructs never set dependsOn directly at all; they build DependencyGroups (internetConnectivityEstablished, mirroring aws-ec2/lib/vpc.ts) and hand them to node.addDependency. Our base construct even throws from fqn to force users onto node.addDependency.

Two problems have made it clear this belongs in the framework:

  1. The Aspect can only approximate the real semantics. Because an Aspect visits top-down, ours propagates each construct's accumulated dependency set to its children through the scope chain rather than expanding the source subtree the way reifyConstructDependencies does. That's close, but not the same, and it interacts badly with lazily-created stack-scoped singletons.

  2. We needed an escape hatch. Stack-level singleton data sources (aws_region, aws_caller_identity, aws_partition, aws_availability_zones, aws_service_principal) are referenced by nearly every resource in the stack, so any depends_on landing on them creates a cycle. We carry a SKIP_DEPENDENCY_PROPAGATION symbol and mark each one with a // HACK to avoid cycles comment. A framework-level implementation operating on the source subtree wouldn't need this.

Every serious abstraction library on CDKTN will hit the same wall and write the same Aspect, slightly differently and slightly wrong. It's ~100 lines that belong in core, where it can be tested once and have well-defined semantics.

Proposal

Port the reification pass into CDKTN core:

  • Run a reifyConstructDependencies-equivalent during synth — StackSynthesizer.synthesize() after invokeAspects(), so resources created by Aspects and by prepareStack() are included.
  • Resolve dependables via Dependable.of(d).dependencyRoots so DependencyGroup and custom IDependables work.
  • Collect L1s under source and target via DFS filtered on TerraformResource.isTerraformResource / TerraformDataSource.isTerraformDataSource, and cross-apply to each source's dependsOn (skipping self-pairs and de-duplicating).
  • Decide and document the cross-stack case — AWS CDK dispatches those up to a stack-level dependency; CDKTN already has TerraformStack.addDependency for that, so the natural mapping exists.

Open questions worth settling in this issue before a PR:

  • Feature flag? This changes generated output for anyone whose node.addDependency calls are currently no-ops. It's arguably a bug fix, but it will produce plan diffs, so it may warrant a flag in features.ts for a release.
  • TerraformModule also takes dependsOn — should modules participate as both source and target?
  • Should depends_on be pruned when an explicit reference already implies the edge, or is redundancy acceptable (AWS CDK accepts it)?
  • Cycle safety — is a detection/annotation pass wanted, or is Terraform's own cycle error sufficient?

References

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions