aws-ecs: AsgCapacityProvider depends on AutoScalingGroup

[cheruvian]

Describe the bug

AsgCapacityProviderProps declares autoscalingGroup to be an IAutoScalingGroup but then coerces to an AutoScalingGroup.

Expected Behavior

Properly handles IAutoScalingGroup or does not allow IAutoScalingGroup to be passed in.

Current Behavior

TypeError: this.autoScalingGroup.protectNewInstancesFromScaleIn is not a function
    at new AsgCapacityProvider (/tmp/cdk-repro/node_modules/aws-cdk-lib/aws-ecs/lib/cluster.js:1:19024)
    at new CdkCodebuildReproStack (/tmp/cdk-repro/lib/cdk-repro-stack.ts:15:5)
    at Object.<anonymous> (/tmp/cdk-repro/bin/cdk-repro.ts:7:1)
    at Module._compile (node:internal/modules/cjs/loader:1376:14)
    at Module.m._compile (/tmp/cdk-repro/node_modules/ts-node/src/index.ts:1618:23)
    at Module._extensions..js (node:internal/modules/cjs/loader:1435:10)
    at Object.require.extensions.<computed> [as .ts] (/tmp/cdk-repro/node_modules/ts-node/src/index.ts:1621:12)
    at Module.load (node:internal/modules/cjs/loader:1207:32)
    at Function.Module._load (node:internal/modules/cjs/loader:1023:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:135:12)

Reproduction Steps

    const autoScalingGroup = AutoScalingGroup.fromAutoScalingGroupName(this, 'ASG', 'my-asg');
    new AsgCapacityProvider(this, 'AsgCapacityProvider', { autoScalingGroup });

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.126.0

Framework Version

No response

Node.js Version

v20.10.0

OS

OSX

Language

TypeScript

Language Version

No response

Other information

No response

[cheruvian]

As a hack (since I know that newInstancesProtectedFromScaleIn: true is set on my ASG), I tried:

    (this.autoScalingGroup as AutoScalingGroup).protectNewInstancesFromScaleIn = () => ({});

But actually adding the AsgCapacityProvider

cluster.addAsgCapacityProvider(capacityProvider);

Results in the same error but now for addToRolePolicy. If you know the role, you can work around this like so:

(this.autoScalingGroup as AutoScalingGroup).protectNewInstancesFromScaleIn = () => ({});
    (this.autoScalingGroup as AutoScalingGroup).addToRolePolicy = (policyStatement: PolicyStatement) => this.hostRole.addToPrincipalPolicy(policyStatement);

More and more it seems like this construct should just not allow IAutoScalingGroup and instead require AutoScalingGroup.

[pahud]

Properly handles IAutoScalingGroup or does not allow IAutoScalingGroup to be passed in.

Thanks for the report. Yes I can reproduce this and I agree with your suggestion.

[scorbiere]

Giving some visibility into this issue, here are the options that I have been considering/testing at the AsgCapacityProvider's level:

1. Change the props of AsgCapacityProvider to enforce the use of AutoScalingGroup instead of IAutoScalingGroup: this was considered as a bad practice since the AsgCapacityProvider construct can be used with the imported ASG (IAutoScalingGroup) if the props enableManagedTerminationProtection is set to false.
2. Throw an error with a clear description when we try to create a AsgCapacityProvider with IAutoScalingGroup and enableManagedTerminationProtection is set to true: the main issue is that it won't let anyone to use and ASG defined outside of the CDK stack, and so ti doesn't seem to be a good user experience.
3. Emit a warning with a clear description when we try to create a AsgCapacityProvider with IAutoScalingGroup and enableManagedTerminationProtection is set to true: which let the user being responsible of the correct ASG configuration but also make CFN stack execution failure a bit harder to fix (as userrs may not check the warnings from CDK)

The latest option seems to be the right solution. But regarding the other comment in this issue:

But actually adding the AsgCapacityProvider

cluster.addAsgCapacityProvider(capacityProvider);

Results in the same error but now for addToRolePolicy. If you know the role, you can work around this like so:

(this.autoScalingGroup as AutoScalingGroup).protectNewInstancesFromScaleIn = () => ({});
   (this.autoScalingGroup as AutoScalingGroup).addToRolePolicy = (policyStatement: PolicyStatement) => this.hostRole.addToPrincipalPolicy(policyStatement);

More and more it seems like this construct should just not allow IAutoScalingGroup and instead require AutoScalingGroup.

Make me think that we should probably go with the option 1 or a mix of the option 2 and apply the same logic in the Cluster construct as well.

IMO, the main point that needs to be clarified is how much useful would that be to let a user provide an imported ASG to an AsgCapacityProvider AND let it be used by a Cluster knowing how much trouble it would be to correctly define the permissions setting. Which gives me the impression that we would be losing most of the benefits of using these L2 constructs (AsgCapacityProvider and Cluster).

[scorbiere]

@cheruvian could you give us more details about the reason why you would like to bring an ASG defined outside of the CDK application? Like I mentioned, this seems to add a lot of constraints (painful configuration that you would have to manage yourself) which would remove most of the benefit of using the L2 constructs.

[github-actions]

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

[github-actions]

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.