feat(core): generalization of dependencies #1583

rix0rrr · 2019-01-20T21:10:31Z

CONSTRUCTS NOW TAKE DEPENDENCIES ON OTHER CONSTRUCTS.

Before, only Resources could take dependencies, and they would depend
on IDependable. Instead, we generalize the concept of dependencies
from construct trees to other construct trees. The result will be that
any resource in the depending tree will depend on all resources in the
depended tree.

Lazy dependency objects have been removed; they are are now calculated
in the prepare() phase.

DEPENDENCIES ARE CROSS-STACK AWARE

If you take a dependency on a construct in another stack, the dependency
does not get rendered in the template, but is instead added as a
dependency between stacks.

BREAKING CHANGE: resource.addDependency() has been moved onto
ConstructNode. You now write resource.node.addDependency().

Fixes #1568, fixes #95.

Pull Request Checklist

Testing
- Unit test added
- CLI change?: manually run integration tests and paste output as a PR comment
- cdk-init template change?: coordinated update of integration tests with team
Docs
- jsdocs: All public APIs documented
- README: README and/or documentation topic updated
Title and Description
- Change type: title prefixed with fix, feat will appear in changelog
- Title: use lower-case and doesn't end with a period
- Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
- Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
Sensitive Modules (requires 2 PR approvers)
- IAM Policy Document (in @aws-cdk/aws-iam)
- EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
- Grant APIs (only if not based on official documentation with a reference)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

CONSTRUCTS NOW TAKE DEPENDENCIES ON OTHER CONSTRUCTS. Before, only `Resource`s could take dependencies, and they would depend on `IDependable`. Instead, we generalize the concept of dependencies from construct trees to other construct trees. The result will be that any resource in the depending tree will depend on all resources in the depended tree. Lazy dependency objects have been removed; they are are now calculated in the prepare() phase. DEPENDENCIES ARE CROSS-STACK AWARE If you take a dependency on a construct in another stack, the dependency does not get rendered in the template, but is instead added as a dependency between stacks. BREAKING CHANGE: `resource.addDependency()` has been moved onto `ConstructNode`. You now write `resource.node.addDependency()`.

RomainMuller

Looks nice :) Discussed some concerns about possible impact on change set execution performance (due to inability to parallelize stuff that could have been, only because DependsOn was over-eager), but I feel it's an okay tradeoff (don't expect this mechanism to be used "often" between large trees). Would nee ncie if we could find a strategy to make this "customizable" by the depended-on construct (so it could scope down to omit the resources that are not interesting to depend on, or where the dependencies are already implied transitively).

RomainMuller · 2019-01-22T02:20:42Z

packages/@aws-cdk/aws-elasticloadbalancingv2/lib/shared/base-target-group.ts

   */
-  protected readonly defaultPort: string;
+  public readonly loadBalancerConstructs = new Array<cdk.IConstruct>();


Probably should prefix that name with an _ since the user won't need to interact with this directly. Also, probably should be called something like loadBalancerAttachments or something.

rix0rrr · 2019-01-22T02:36:06Z

Note: for APIGW, the set of dependencies has increased; we can make those explicit again.

Also, we should probably find a good way of naming sets of constructs that are only exposed in order to take dependencies on them. OR find a way to expose those via types. The alternative is we reintroduce IDependable and make IConstruct implement that.

sam-goodwin · 2019-01-22T22:04:05Z

packages/@aws-cdk/cdk/lib/core/construct.ts

+    const ret = new Set<IConstruct>();
+    let node: ConstructNode | undefined = this;
+    while (node !== undefined) {
+      addSet(ret, node.dependencies);


I think it would have been easier to read if you had just in-lined the loop here.

sam-goodwin · 2019-01-22T22:05:43Z

packages/@aws-cdk/cdk/test/cloudformation/test.resource.ts

-  }
+  },
+
+  'cross-stack construct dependencies are not rendered but turned into stack dependencies'(test: Test) {


…deps

eladb · 2019-01-23T15:15:26Z

packages/@aws-cdk/aws-apigateway/lib/deployment.ts

@@ -182,5 +168,7 @@ class LatestDeploymentResource extends CfnDeployment {

      this.lazyLogicalId = this.originalLogicalId + md5.digest("hex");
    }
+
+    super.prepare();


See my note about maybe moving resource dependency calculation to the stack. If you decide to leave Rhiannon here, then add a wee comment on the super call to hint why it Ian needed

eladb · 2019-01-23T15:18:41Z

packages/@aws-cdk/aws-autoscaling/lib/target-tracking-scaling-policy.ts

+   * so we add an ordering dependency on the Target Group being associated with
+   * a Load Balancer.
+   */
+  public dependOnLoadBalancerAttachment(targetGroup: elbv2.ITargetGroup) {


“AddTargetGroup” maybe?

eladb · 2019-01-23T15:21:00Z

packages/@aws-cdk/aws-ec2/lib/vpc.ts

-      this.natGatewayByAZ[sub.availabilityZone] = sub.addNatGateway();
+      const gateway = sub.addNatGateway();
+      this.natGatewayByAZ[sub.availabilityZone] = gateway.natGatewayId;
+      this.natDependencies.push(gateway);


Wouldn’t it make more sense to just expose this as “natGateways” ?

Maybe. But I don't want to overpromise; they were there to be able to take a dependency on, nothing more.

Sounds like a legitimate API to expose the NAT gateways and internet gateways within the VPC, no?

eladb · 2019-01-23T15:30:52Z

packages/@aws-cdk/aws-ec2/lib/vpc-ref.ts

   */
-  public readonly dependencyElements: IDependable[] = [];


Just expose the internet gateway?

It's actually not the gateway, it's the attachment of the gateway to the VPC.

eladb · 2019-01-23T15:37:06Z

packages/@aws-cdk/cdk/lib/cloudformation/stack.ts

@@ -33,14 +33,24 @@ export class Stack extends Construct {
   * @returns The Stack object (throws if the node is not part of a Stack-rooted tree)
   */
  public static find(scope: IConstruct): Stack {
+    const curr = Stack.tryFind(scope);


That’s an unsafe breaking change... maybe we can’t safely
Make it without checking all call sites...

Why is this a breaking change? find() used to throw and still throws, tryFind() does not throw.

eladb · 2019-01-23T15:38:02Z

packages/@aws-cdk/cdk/lib/core/construct.ts

+  /**
+   * Return all dependencies registered on this node and its ancestors.
+   */
+  public myDependencies(): IConstruct[] {


I wonder the api can be more explicit and/or rich to indicate that it returns both explicit and implicit dependencies (maybe “findDependencies(includeImpicit = true)”

We can, but there's no use for it (at least today). Future proofing?

I just don't like the fact that it returns implicit dependencies without that reflecting in the API. It could be findImplicitAndExplicitDependencies() but findDependencies({ implicit: true }) feels a bit less ugly.

...I am not sure about the "implicit" terminology. Maybe "inherited" or "derived" or something like that...

eladb · 2019-01-23T15:44:15Z

packages/@aws-cdk/cdk/lib/cloudformation/resource.ts

+  protected prepare() {
+    super.prepare();
+
+    // As an optimization, do the stack dependencies first on the parents


Would it make more sense perhaps to do this globally at the stack instead of distribute to each resource?

rix0rrr · 2019-01-23T18:25:13Z

I'm having one more issue with this PR at the moment.

Representing a "bundle of dependencies"

Relevant in the following cases:

Any instance that needs an EIP is dependent on the
AWS::VPC2::VPCGatewayAttachment object having been created. This
is basically any instance in a Public Subnet, such as ASGs, ELBs,
etc.
RDS instances have a requirement on network connectivity, which translates
into either the IGW attachment (public subnets) or a Route to a NAT gateway
having been created (private subnets). Note: right now we're modeling this
as a dependency on the NAT gateway itself, but I'm fairly sure that's not
correct; it probably just happens that the race condition window is so small
that we haven't seen it cause problems yet.
Some resources that only get a TargetGroup as argument (in EBLv2) need a
LoadBalancer associated with that particular TargetGroup. This includes ECS
Services and TargetTrackingScalingPolicys that scale on ALB request count
(both in EC2 AutoScaling and in Application AutoScaling). Again, in this
case it's not the LoadBalancer itself that's interesting or needs to be
depended upon, but the association object, which in this case is either
a Listener or a ListenerRule object.

Right now, I'm representing this as an IConstruct[] (so you have to do
construct.node.addDependency(...targetGroup.loadBalancerDependencies)), but I actually
don't like that very much. It overpromises on the API; the only ability you
should get is being able to take a dependency on these things.

I'm thinking of reintroducing IDependable and implementing it like this
instead:

interface IDependable {
  readonly dependencyElements: IConstruct[];
}

interface Construct extends IDependable {
  public readonly dependencyElements = [this];
}

class DependableSet implements IDependable {
  public add(element: IConstruct) { ... }
}

This will allow naive use in most cases:
construct.node.addDependency(otherConstruct), but also allow explicit
control over dependency sets when it makes sense. For completeness

// OLD (IConstruct[])
construct.node.addDependency(otherConstruct);
construct.node.addDependency(...targetGroup.loadBalancerDependencies);

// NEW (IDependable)
construct.node.addDependency(otherConstruct);
construct.node.addDependency(targetGroup.loadBalancerAttached);

For the VPC case, instead of exposing both the IGW dependency objects and the NAT
gateway dependency objects on the VPC object itself, and having consumers
grub around those, I think it makes more sense to do the following:

interface IVpcSubnet {
  /**
   * Returns the right set of dependencies depending on the subnet type
   */
  readonly internetConnectivityDependency: IDependable;
}

eladb · 2019-01-23T20:39:09Z

@rix0rrr I like the IDependable approach. Looks clean and I love the fact that by default people don't have to think about it but it gives them the ability to customize!

eladb · 2019-01-23T20:39:34Z

Just one question: why do we need DependableSet?

rix0rrr · 2019-01-23T21:06:59Z

It would be an IDependable for the cases where you don't want just all constructs rooted under this one, so you just customize a literal list.

RomainMuller · 2019-01-30T09:32:16Z

This blocks #1633

…deps

Constructs can now take a dependency on any other construct. Before, only `Resource`s could take dependencies, and they would depend on `IDependable` which had to be implemented explicitly. In this change we generalize the concept of dependencies from construct trees to other construct trees; all constructs now take dependencies and also implement `IDependable`. The semantics are that any resource in the depending tree will depend on all resources in the depended tree. Dependencies are cross-stack aware If you take a dependency on a construct in another stack, the dependency does not get rendered in the template, but is instead added as a dependency between stacks. Fixes aws#1568, fixes aws#95. BREAKING CHANGE: `resource.addDependency()` has been moved onto `ConstructNode`. You now write `resource.node.addDependency()`. VPC's `internetDependency` has been moved to the subnets as `internetConnectivityEstablished`. Target Group's `loadBalancerAssociationDependencies` has been renamed to `loadBalancerAttached`.

rix0rrr requested review from RomainMuller, skinny85, SoManyHs and a team as code owners January 20, 2019 21:10

Rico Huijbers added 3 commits January 21, 2019 11:32

Don't use Set in public API

aa100a0

Interface should have readonly properties

4df8045

Fix dependency handling in APIGW

3bda9c3

RomainMuller approved these changes Jan 22, 2019

View reviewed changes

RomainMuller mentioned this pull request Jan 22, 2019

Apply conditions on constructs #1591

Closed

sam-goodwin approved these changes Jan 22, 2019

View reviewed changes

sam-goodwin added the @aws-cdk/core Related to core CDK functionality label Jan 22, 2019

Rico Huijbers added 2 commits January 22, 2019 15:58

Merge remote-tracking branch 'origin/master' into huijbers/construct-…

642a136

…deps

Rename loadBalancerConstructs => loadBalancerDependencies

b054224

eladb approved these changes Jan 23, 2019

View reviewed changes

Simplify dependencies by not making them lazy

e296d3c

Move dependency resolution to Stack

ec74b29

Update some more expectations

0a27398

RomainMuller mentioned this pull request Jan 30, 2019

Depend on bucket and policy before configuring ELB logging #1633

Closed

rix0rrr added 5 commits February 4, 2019 11:32

Merge remote-tracking branch 'origin/master' into huijbers/construct-…

8787b06

…deps

Introduce IDependable

3874942

Use IDependable in case of bundled dependencies

f2de176

Updating tests

9e1873e

Merge remote-tracking branch 'origin/master' into huijbers/construct-…

5926bf6

…deps

rix0rrr merged commit 53e6825 into master Feb 4, 2019

rix0rrr deleted the huijbers/construct-deps branch February 4, 2019 13:46

NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): generalization of dependencies #1583

feat(core): generalization of dependencies #1583

rix0rrr commented Jan 20, 2019

RomainMuller left a comment

RomainMuller Jan 22, 2019

rix0rrr commented Jan 22, 2019 •

edited

Loading

sam-goodwin Jan 22, 2019

sam-goodwin Jan 22, 2019

eladb Jan 23, 2019

eladb Jan 23, 2019

eladb Jan 23, 2019

rix0rrr Jan 23, 2019

eladb Jan 23, 2019

eladb Jan 23, 2019

rix0rrr Jan 23, 2019

eladb Jan 23, 2019

rix0rrr Jan 23, 2019

eladb Jan 23, 2019

rix0rrr Jan 23, 2019

eladb Jan 23, 2019

eladb Jan 23, 2019

rix0rrr commented Jan 23, 2019

eladb commented Jan 23, 2019

eladb commented Jan 23, 2019

rix0rrr commented Jan 23, 2019

RomainMuller commented Jan 30, 2019

feat(core): generalization of dependencies #1583

feat(core): generalization of dependencies #1583

Conversation

rix0rrr commented Jan 20, 2019

Pull Request Checklist

RomainMuller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr commented Jan 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr commented Jan 23, 2019

Representing a "bundle of dependencies"

eladb commented Jan 23, 2019

eladb commented Jan 23, 2019

rix0rrr commented Jan 23, 2019

RomainMuller commented Jan 30, 2019

rix0rrr commented Jan 22, 2019 •

edited

Loading