Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot add AutoScalingGroup to more than one Target Group #5667

Closed
2 tasks
KingOfPoptart opened this issue Jan 6, 2020 · 17 comments · Fixed by #23044
Closed
2 tasks

Cannot add AutoScalingGroup to more than one Target Group #5667

KingOfPoptart opened this issue Jan 6, 2020 · 17 comments · Fixed by #23044
Labels
@aws-cdk/aws-autoscaling Related to Amazon EC2 Auto Scaling effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1

Comments

@KingOfPoptart
Copy link
Contributor

KingOfPoptart commented Jan 6, 2020

When attempting to assign an ASG to multiple target groups, CDK errors out with Cannot add AutoScalingGroup to 2nd Target Group

CFN supports passing in a list of Target Group ARNs to an autoscaling group - https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-as-group.html#cfn-as-group-targetgrouparns

Use Case

This feature is necessary if anybody would like to assign multiple load balancers to an ASG

Proposed Solution

Remove the check here - https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts#L559

Figure out any other functions that are effected and come up with workarounds. Rico thinks there could be issues with scaleOnRequestCount()

Other

  • 👋 I may be able to implement this feature request
  • ⚠️ This feature might incur a breaking change

This is a 🚀 Feature Request

@KingOfPoptart KingOfPoptart added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Jan 6, 2020
@SomayaB SomayaB added the @aws-cdk/aws-autoscaling Related to Amazon EC2 Auto Scaling label Jan 7, 2020
@rix0rrr rix0rrr assigned NetaNir and unassigned rix0rrr Jan 23, 2020
@icecold21
Copy link

+1 on this, my use case is I would like to add listener for both http and https, but point them to the same autoscaling group.

@hikeeba
Copy link

hikeeba commented Mar 3, 2020

This issue is causing us problems as well. We need an ASG to be attached multiple times to a target group because of multiple applications running on our instances with different ports and domains.

@NetaNir NetaNir added the effort/medium Medium work item – several days of effort label Mar 9, 2020
@rix0rrr rix0rrr self-assigned this Mar 9, 2020
@manikandanmsairam
Copy link

manikandanmsairam commented Mar 18, 2020

+1 We have a usecase where the application has to be exposed both internally and to Internet. Internet facing load balancer will be restricted to CloudFront IPs and internal load balancer will be accessed by another application for flushing. Hence we need to attach the same target group into internal load balancer and internet facing load balancer.

@kevinslin
Copy link

current workaround we are using

...
const cfnAsg = asg.node.defaultChild as CfnAutoScalingGroup;
cfnAsg.targetGroupArns = [tgHTTP.targetGroupArn, tgHTTPS.targetGroupArn];

@SomayaB SomayaB removed the needs-triage This issue or PR still needs to be triaged. label May 19, 2020
@trevordilley
Copy link

Definitely would love to have this resolved unless their is a more idiomatic way to handle HTTP -> HTTPS redirects, thanks!

@rbobrowicz
Copy link
Contributor

Ran into this issue today as well. As far as I can tell the only reason this limitation is there is because scaleOnRequestCount() is hard coded to only work with one target group. Which is an unnecessary limitation as well, since you can have a scaling policy per target group.

Unfortunately, I'm not sure how to fix this. The real fix would be a change to scaleOnRequestCount() to take a target group as an argument, but that will cause an API breakage.

The only other path I see would be to have scaleOnRequestCount() only work with the first target group attached, as it does now, but still allow other target groups to be attached. This is rather crummy API, and still limits scaling policies, but it would at least be a step forward.

I'm willing to work on a patch for this, since this a pretty big issue for us, but I would need some guidance from the maintainers on how to proceed and what they think an acceptable solution looks like.

@trevordilley You shouldn't need multiple target groups to handle an HTTP to HTTPS redirect, that can be handled with a listener on the ALB:

const alb = new ApplicationLoadBalancer(this, 'alb', { ...your props...});
const listener = new ApplicationListener(this, 'httplistener', { port: 80, loadBalancer: alb });
listener.addRedirectResponse("redirect", { protocol: "HTTPS", port: "443", statusCode: "HTTP_301" });

@thedevopsguyblog
Copy link

+1 I have run into this issue.
I need to point multiple target groups to a single fleet running some containers on multiple ports.

@antonfelich
Copy link

+1 we use Octopus to push multiple applications to a single box running on different ports. Would be nice to attach multiple target groups to a single asg without resorting to a "hack".

Gold star for @kevinslin his workaround has sorted me out for now.

@wheresalice
Copy link

@kevinslin are you able to expand upon your workaround please?

What is tgHTTP and how is it created? What associates any of this with a listener?

@wheresalice
Copy link

In our case we have a service which exposes two ports and we want both attached to an ALB listener, one with a listenCondition based on path.

Our workaround looks something like the following:

const asg = new autoscaling.AutoScalingGroup()
const lb = new elbv2.ApplicationLoadBalancer()
const listener = lb.addListener()
const tgREST = new elbv2.ApplicationTargetGroup()
listener.addTargetGroups("RestTarget", {
            targetGroups: [tgREST],
            conditions: [
                elbv2.ListenerCondition.pathPatterns(["/api/*"]),
            ],
            priority: 100,
        });
const tgHTTP = new elbv2.ApplicationTargetGroup()
const ServiceAsg = asg.node.defaultChild as autoscaling.CfnAutoScalingGroup
ServiceAsg.targetGroupArns = [tgREST.targetGroupArn, tgHTTP.targetGroupArn]

@robertofd1995
Copy link

Any update about this? We are also facing the same problem

@markdollan243
Copy link

Hello, any news about this issue?

@njlynch njlynch removed the p2 label Aug 24, 2021
@realugbun
Copy link

Bump.

We are having the same issue. Any update?

@klang
Copy link

klang commented May 20, 2022

The workaround works, but finding the workaround here, takes work :-)

@azatoth
Copy link
Contributor

azatoth commented Nov 12, 2022

So I have been thinking, and I wonder if following diff would be an acceptable/reasonable/valid solution to the problem?

I might have totally missed the underlying problem but it seems to me that in the short term (no API changes) we only need to prevent multiple target groups if setScaleOnRequest() has been used; And I believe that the validate functionality might work here.

diff --git a/packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts b/packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts
index 0c5006c29f..d27863b642 100644
--- a/packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts
+++ b/packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts
@@ -961,6 +961,7 @@ abstract class AutoScalingGroupBase extends Resource implements IAutoScalingGrou
   public abstract readonly osType: ec2.OperatingSystemType;
   protected albTargetGroup?: elbv2.ApplicationTargetGroup;
   public readonly grantPrincipal: iam.IPrincipal = new iam.UnknownPrincipal({ resource: this });
+  protected hasSetScaleOnRequest: boolean = false;
 
   /**
    * Send a message to either an SQS queue or SNS topic when instances launch or terminate
@@ -1064,6 +1065,7 @@ abstract class AutoScalingGroupBase extends Resource implements IAutoScalingGrou
     });
 
     policy.node.addDependency(this.albTargetGroup.loadBalancerAttached);
+    this.hasSetScaleOnRequest = true;
     return policy;
   }
 
@@ -1363,6 +1365,8 @@ export class AutoScalingGroup extends AutoScalingGroupBase implements
     if (props.requireImdsv2) {
       Aspects.of(this).add(new AutoScalingGroupRequireImdsv2Aspect());
     }
+
+    this.node.addValidation({ validate: () => this.validateTargetGroup() });
   }
 
   /**
@@ -1390,10 +1394,6 @@ export class AutoScalingGroup extends AutoScalingGroupBase implements
    * Attach to ELBv2 Application Target Group
    */
   public attachToApplicationTargetGroup(targetGroup: elbv2.IApplicationTargetGroup): elbv2.LoadBalancerTargetProps {
-    if (this.albTargetGroup !== undefined) {
-      throw new Error('Cannot add AutoScalingGroup to 2nd Target Group');
-    }
-
     this.targetGroupArns.push(targetGroup.targetGroupArn);
     if (targetGroup instanceof elbv2.ApplicationTargetGroup) {
       // Copy onto self if it's a concrete type. We need this for autoscaling
@@ -1740,6 +1740,16 @@ export class AutoScalingGroup extends AutoScalingGroupBase implements
       };
     }
   }
+
+
+  private validateTargetGroup(): string[] {
+    const errors = new Array<string>();
+    if (this.hasSetScaleOnRequest && this.targetGroupArns.length > 1) {
+      errors.push('Cannon use multiple target groups if `setScaleOnRequest()` is being used.');
+    }
+
+    return errors;
+  }
 }
 
 /**
diff --git a/packages/@aws-cdk/aws-autoscaling/test/auto-scaling-group.test.ts b/packages/@aws-cdk/aws-autoscaling/test/auto-scaling-group.test.ts
index c0a979408a..23e576599d 100644
--- a/packages/@aws-cdk/aws-autoscaling/test/auto-scaling-group.test.ts
+++ b/packages/@aws-cdk/aws-autoscaling/test/auto-scaling-group.test.ts
@@ -2,6 +2,7 @@ import { Annotations, Match, Template } from '@aws-cdk/assertions';
 import * as cloudwatch from '@aws-cdk/aws-cloudwatch';
 import * as ec2 from '@aws-cdk/aws-ec2';
 import { AmazonLinuxCpuType, AmazonLinuxGeneration, AmazonLinuxImage, InstanceType, LaunchTemplate } from '@aws-cdk/aws-ec2';
+import { ApplicationLoadBalancer, ApplicationTargetGroup } from '@aws-cdk/aws-elasticloadbalancingv2';
 import * as iam from '@aws-cdk/aws-iam';
 import * as sns from '@aws-cdk/aws-sns';
 import { testDeprecated } from '@aws-cdk/cdk-build-tools';
@@ -1766,6 +1767,43 @@ describe('auto scaling group', () => {
     })).not.toThrow();
 
   });
+
+  test('Should validate multiple target groups i conjunction with `setScaleOnRequest()`', () => {
+    const stack = new cdk.Stack(undefined, 'MyStack', { env: { region: 'us-east-1', account: '1234' } });
+
+
+    const vpc = mockVpc(stack);
+
+    const alb = new ApplicationLoadBalancer(stack, 'alb', {
+      vpc,
+      internetFacing: true,
+    });
+
+    const listener = alb.addListener('Listener', {
+      port: 80,
+      open: true,
+    });
+    const asg = new autoscaling.AutoScalingGroup(stack, 'MyFleet', {
+      instanceType: ec2.InstanceType.of(ec2.InstanceClass.M4, ec2.InstanceSize.MICRO),
+      machineImage: new ec2.AmazonLinuxImage(),
+      vpc,
+    });
+
+    const atg1 = new ApplicationTargetGroup(stack, 'ATG1', { port: 443 });
+    const atg2 = new ApplicationTargetGroup(stack, 'ATG2', { port: 443 });
+
+    listener.addTargetGroups('tgs', { targetGroups: [atg1, atg2] });
+
+    asg.attachToApplicationTargetGroup(atg1);
+    asg.attachToApplicationTargetGroup(atg2);
+
+    expect(asg.node.validate()).toEqual([]);
+
+    asg.scaleOnRequestCount('requests-per-minute', { targetRequestsPerMinute: 60 });
+
+    expect(asg.node.validate()).toContainEqual('Cannon use multiple target groups if `setScaleOnRequest()` is being used.');
+
+  });
 });
 
 function mockVpc(stack: cdk.Stack) {

azatoth added a commit to azatoth/aws-cdk that referenced this issue Nov 29, 2022
…oups

While this doesn't solve the underlying problem, which will require an
API breakage in `scaleOnRequestCount()` (see
aws#5667 (comment)).

As `scaleOnRequestCount()` seems to be the only part that is affected by
this, we'll only issue a validation error if we have multiple
`targetGroupArns` and `scaleOnRequestCount()` has been called.

closes aws#5667
azatoth added a commit to azatoth/aws-cdk that referenced this issue Dec 6, 2022
…oups

While this doesn't solve the underlying problem, which will require an
API breakage in `scaleOnRequestCount()` (see
aws#5667 (comment)).

As `scaleOnRequestCount()` seems to be the only part that is affected by
this, we'll only issue a validation error if we have multiple
`targetGroupArns` and `scaleOnRequestCount()` has been called.

closes aws#5667
azatoth added a commit to azatoth/aws-cdk that referenced this issue Dec 14, 2022
…oups

While this doesn't solve the underlying problem, which will require an
API breakage in `scaleOnRequestCount()` (see
aws#5667 (comment)).

As `scaleOnRequestCount()` seems to be the only part that is affected by
this, we'll only issue a validation error if we have multiple
`targetGroupArns` and `scaleOnRequestCount()` has been called.

closes aws#5667
azatoth added a commit to azatoth/aws-cdk that referenced this issue Dec 15, 2022
…oups

While this doesn't solve the underlying problem, which will require an
API breakage in `scaleOnRequestCount()` (see
aws#5667 (comment)).

As `scaleOnRequestCount()` seems to be the only part that is affected by
this, we'll only issue a validation error if we have multiple
`targetGroupArns` and `scaleOnRequestCount()` has been called.

closes aws#5667
azatoth added a commit to azatoth/aws-cdk that referenced this issue Dec 17, 2022
…oups

While this doesn't solve the underlying problem, which will require an
API breakage in `scaleOnRequestCount()` (see
aws#5667 (comment)).

As `scaleOnRequestCount()` seems to be the only part that is affected by
this, we'll only issue a validation error if we have multiple
`targetGroupArns` and `scaleOnRequestCount()` has been called.

closes aws#5667
azatoth added a commit to azatoth/aws-cdk that referenced this issue Dec 19, 2022
…oups

While this doesn't solve the underlying problem, which will require an
API breakage in `scaleOnRequestCount()` (see
aws#5667 (comment)).

As `scaleOnRequestCount()` seems to be the only part that is affected by
this, we'll only issue a validation error if we have multiple
`targetGroupArns` and `scaleOnRequestCount()` has been called.

closes aws#5667
@mergify mergify bot closed this as completed in #23044 Dec 21, 2022
mergify bot pushed a commit that referenced this issue Dec 21, 2022
…oups (#23044)

While this doesn't solve the underlying problem, which will require an API breakage in `scaleOnRequestCount()` (see
#5667 (comment)).

As `scaleOnRequestCount()` seems to be the only part that is affected by this, we'll only issue a validation error if we have multiple `targetGroupArns` and `scaleOnRequestCount()` has been called.

closes #5667


----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️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.

brennanho pushed a commit to brennanho/aws-cdk that referenced this issue Jan 20, 2023
…oups (aws#23044)

While this doesn't solve the underlying problem, which will require an API breakage in `scaleOnRequestCount()` (see
aws#5667 (comment)).

As `scaleOnRequestCount()` seems to be the only part that is affected by this, we'll only issue a validation error if we have multiple `targetGroupArns` and `scaleOnRequestCount()` has been called.

closes aws#5667


----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
brennanho pushed a commit to brennanho/aws-cdk that referenced this issue Feb 22, 2023
…oups (aws#23044)

While this doesn't solve the underlying problem, which will require an API breakage in `scaleOnRequestCount()` (see
aws#5667 (comment)).

As `scaleOnRequestCount()` seems to be the only part that is affected by this, we'll only issue a validation error if we have multiple `targetGroupArns` and `scaleOnRequestCount()` has been called.

closes aws#5667


----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@Lee4396
Copy link

Lee4396 commented Sep 19, 2023

3 years later and this is still a issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-autoscaling Related to Amazon EC2 Auto Scaling effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.