fix(ecs): fargate on ALB without cyclic dependencies

[parisholley]

Ran into the bug on #3689, I believe this is the correct solution.

When registering target groups to a listener > alb, the default behavior is to update the LB security group with outbound permissions to the item being targeted. If I understand the code correctly, this seems backwards as most LB will not likely have restrictive outbound rules. Instead, I believe most people would expect instead the security groups assigned to target (IP, instance, Lambda, etc), would have an INBOUND rule that allows access from the LB.

Not sure the implications of this change, but allowed me to create a shared LB/listener in Stack A, and then later launch Fargate services and register them with path patterns on that listener in Stack B.

cc @jgondron @rix0rrr

[mergify]

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

• PR title type(scope): text
  • type: fix, feat, refactor go into CHANGELOG, chore is hidden
  • scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
  • text: use all lower-case, do not end with a period, do not include issue refs
• PR Description
  • Rationale: describe rationale of change and approach taken
  • Issues: indicate issues fixed via: fixes #xxx or closes #xxx
  • Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
• Testing
  • Unit test added. Prefer to add a new test rather than modify existing tests
  • CLI or init templates change? Re-run/add CLI integration tests
• Documentation
  • README: update module README to describe new features
  • API docs: public APIs must be documented. Copy from official AWS docs when possible
  • Design: for significant features, follow design process

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: FAILED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[rix0rrr]

This fixes your case, where you have a Shared ALB and you want to attach multiple services to it in downstream stacks.

It would break the other case, where you have an existing service in a shared stack and you want to route to it from an ALB in a downstream stack.

We have to do some thinking about this :x.

[parisholley]

@rix0rrr Ah I see, it will end up going cyclical in the other direction. what if we delayed the processing of Connectables until a later phase, then evaluate the dependency graph and choose which direction to draw them?

[parisholley]

@rix0rrr What if we updated the methods in Connections like these:

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

Lines 125 to 143 in 3d693e6

	public allowTo(other: IConnectable, portRange: Port, description?: string) {
	if (this.skip) { return; }
	const remoteRule = this.remoteRule; // Capture current value into local for callback to close over
	this._securityGroups.forEachAndForever(securityGroup => {
	other.connections._securityGroupRules.forEachAndForever(rule => {
	securityGroup.addEgressRule(rule, portRange, description, remoteRule);
	});
	});
	this.skip = true;
	other.connections.remoteRule = true;
	try {
	other.connections.allowFrom(this, portRange, description);
	} finally {
	this.skip = false;
	other.connections.remoteRule = false;
	}
	}

So that instead of modifying the SG's right away, we just store them locally temporarily. Then in the following method:

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

Lines 497 to 522 in 3d693e6

	protected prepare() {
	// References
	for (const ref of this.node.references) {
	if (CfnReference.isCfnReference(ref.reference)) {
	ref.reference.consumeFromStack(this, ref.source);
	}
	}
	// Resource dependencies
	for (const dependency of this.node.dependencies) {
	const theirStack = Stack.of(dependency.target);
	if (theirStack !== undefined && theirStack !== this) {
	this.addDependency(theirStack);
	} else {
	for (const target of findResources([dependency.target])) {
	for (const source of findResources([dependency.source])) {
	source.addDependsOn(target);
	}
	}
	}
	}
	if (this.tags.hasTags()) {
	this.node.addMetadata(cxapi.STACK_TAGS_METADATA_KEY, this.tags.renderTags());
	}
	}

We loop through all node children (recursively), and ask for those egress/ingress mappings, and depending on which way the dependency graph is built:

• Same stack, existing logic
• If different stacks, order based on child/parent relationship (from/to)

[rix0rrr]

Thinking about it some more, in the case of Fargate the other case can't actually happen, because it'll always be the case that the Service points to the load balancer.

And that'll be the case for most reasonable services we care about.

[rix0rrr]

I am inclined to accept this patch. If we need to support control of SGs in both directions, we will extend registerConnectable() to give control to the user or the construct, which knows in which direction the dependency will lie.

Could you add a test to make sure the new behavior works, so we can codify it?

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[parisholley]

@rix0rrr done :)

[rix0rrr]

👍

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository