feat(codedeploy): loadbalancer support for imported Target Groups by eastNine · Pull Request #17848 · aws/aws-cdk

eastNine · 2021-12-05T10:07:38Z

This PR fixes that imported alb and nlb target group be able to configure to loadBalancer property.

example:

import * as cdk from '@aws-cdk/core';
import * as codedeploy from '@aws-cdk/aws-codedeploy';
import * as elbv2 from '@aws-cdk/aws-elasticloadbalancingv2';

const deploymentGroup = new codedeploy.ServerDeploymentGroup(this, 'deploymentGroup', {
    ...

    // configurable imported application loadbalancer targetgroup
    loadBalancer: codedeploy.LoadBalancer.application(
        elbv2.ApplicationTargetGroup.fromTargetGroupAttributes(this, 'importedAlbTg', {
            targetGroupArn: 'arn:aws:elasticloadbalancing:ap-northeast-2:111111111111:targetgroup/myAlbTargetgroup/abcd12345678efgf'
        })
    ),

    // also network loadbalancer targetgroup
    loadBalancer: codedeploy.LoadBalancer.network(
        elbv2.NetworkTargetGroup.fromTargetGroupAttributes(this, 'importedNlbTg', {
            targetGroupArn: 'arn:aws:elasticloadbalancing:ap-northeast-2:111111111111:targetgroup/myNlbTargetgroup/wxyz09876543opqr'
        })
    ),
});

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

…b and nlb

gitpod-io · 2021-12-05T10:07:41Z

mergify · 2021-12-05T10:08:12Z

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

…ported alb and nlb

Move `parameterGroup` from `DatabaseInstanceSourceProps` to `DatabaseInstanceNewProps` since the parameter group of a replica instance can be customized. Closes aws#17580 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…Nine/aws-cdk into fix-codedeploy-loadbalancer

skinny85

Thanks for the contribution @eastNine! I think we should go a slightly different route than what you proposed here.

Let me know if you agree with my proposal!

Thanks,
Adam

skinny85 · 2021-12-06T19:22:11Z

packages/@aws-cdk/aws-codedeploy/test/server/deployment-group.test.ts

+                },
              ],
            },
          },


We're missing a unit test for using LoadBalancer with imported Target Groups here 🙂.

skinny85 · 2021-12-06T19:24:43Z

packages/@aws-cdk/aws-codedeploy/lib/server/load-balancer.ts

    class AlbLoadBalancer extends LoadBalancer {
      public readonly generation = LoadBalancerGeneration.SECOND;
-      public readonly name = albTargetGroup.targetGroupName;
+      public readonly name = targetGruopNameFromArn(albTargetGroup.targetGroupArn);


I don't particularly like this solution. I actually think this is putting the logic of how to get the name of the Target Group from its ARN in the wrong place.

Let's do this a little bit differently:

Let's add a targetGroupName property to ITargetGroup.

In imported Target Groups, let's add the logic of parsing it from the ARN of the Target Group.

Let's keep using public readonly name = albTargetGroup.targetGroupName; here, but now, this will Just Work.

OK, I agree your proposal.👌
I will update soon with this approach.

Pull request has been modified.

skinny85

Looks good @eastNine, but we need a few changes before we merge this in.

skinny85 · 2021-12-14T23:48:09Z

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

   * ARN of the target group
   */
  readonly targetGroupArn: string;
-


Let's keep this empty line (there should be an empty line separating the properties).

skinny85 · 2021-12-14T23:48:24Z

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

+   *
+   * @default targetGroupName
+   */
+  readonly targetGroupName?: string;


Missing empty line after this line.

skinny85 · 2021-12-14T23:49:21Z

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

+  /**
+   * The name of the target group
+   *
+   * @default targetGroupName


Why are we adding this property here? targetGroupArn is already required, so what does providing an optional targetGroupName accomplish? I also don't see you using this property anywhere.

I think we should remove this.

This code added while my test.
I removed this.

skinny85 · 2021-12-14T23:49:53Z

packages/@aws-cdk/aws-elasticloadbalancingv2/lib/shared/imported.ts

   * ARN of the target group
   */
  public readonly targetGroupArn: string;
-


Same comment here (there should be an empty line separating the fields).

skinny85 · 2021-12-15T00:04:34Z

packages/@aws-cdk/aws-elasticloadbalancingv2/lib/shared/util.ts

+ * Return the targetGroupName from targetGroupArn
+ */
+export function targetGroupNameFromArn(targetGroupArn: string) {
+  const arnParts = Fn.split('/', targetGroupArn);


We have a function in the core Stack for this already: Stack.splitArn(). See how it's used here, for example.

skinny85 · 2021-12-15T00:08:32Z

packages/@aws-cdk/aws-codedeploy/test/server/deployment-group.test.ts

+      ),
+    });
+
+    expect(stack).toHaveResource('AWS::CodeDeploy::DeploymentGroup', {


Suggested change

expect(stack).toHaveResource('AWS::CodeDeploy::DeploymentGroup', {

expect(stack).toHaveResourceLike('AWS::CodeDeploy::DeploymentGroup', {

skinny85 · 2021-12-15T00:08:50Z

packages/@aws-cdk/aws-codedeploy/test/server/deployment-group.test.ts

+      'DeploymentStyle': {
+        'DeploymentOption': 'WITH_TRAFFIC_CONTROL',
+      },
+    });


I don't think this is relevant to this test, let's just remove this.

skinny85 · 2021-12-15T00:08:56Z

packages/@aws-cdk/aws-codedeploy/test/server/deployment-group.test.ts

+      'DeploymentStyle': {
+        'DeploymentOption': 'WITH_TRAFFIC_CONTROL',
+      },


I don't think this is relevant to this test, let's just remove this.

packages/@aws-cdk/aws-elasticloadbalancingv2/test/alb/target-group.test.ts

packages/@aws-cdk/aws-elasticloadbalancingv2/test/nlb/target-group.test.ts

skinny85 · 2021-12-15T00:11:57Z

@eastNine make sure to re-request my review (there's a button in the top-right of the PR window, next to my avatar) once you're ready for another round of reviews - this way, I won't miss another iteration when it's ready. Thanks!

Pull request has been modified.

eastNine · 2021-12-19T13:55:03Z

@skinny85
Thanks for review!
I updated PR.

this.targetGroupName = cdk.Stack.of(scope).splitArn(props.targetGroupArn, cdk.ArnFormat.SLASH_RESOURCE_SLASH_RESOURCE_NAME).resourceName!;

I tried above,
it returns myAlbTargetGroup/73e2d6bc24d8a067 as resourcename in test.

I think we can't use return value of Stack.splitArn().
Because, property targetGroupInfo.Name is only aceept targetgroup's name instead of resourcename.

Reference aws-properties-codedeploy-deploymentgroup-targetgroupinfo

So I added one more split.

https://github.com/eastNine/aws-cdk/blob/fix-codedeploy-loadbalancer/packages/%40aws-cdk/aws-elasticloadbalancingv2/lib/shared/imported.ts#L37

skinny85

Looks great @eastNine, thanks so much for the contribution!

mergify · 2021-12-20T19:22:14Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

mergify · 2021-12-20T19:24:18Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2021-12-20T20:08:02Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: 13fc16d
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2021-12-20T20:08:27Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

…s#17848) This PR fixes that imported alb and nlb target group be able to configure to loadBalancer property. Fixes aws#9677. example: ```TypeScript import * as cdk from '@aws-cdk/core'; import * as codedeploy from '@aws-cdk/aws-codedeploy'; import * as elbv2 from '@aws-cdk/aws-elasticloadbalancingv2'; const deploymentGroup = new codedeploy.ServerDeploymentGroup(this, 'deploymentGroup', { ... // configurable imported application loadbalancer targetgroup loadBalancer: codedeploy.LoadBalancer.application( elbv2.ApplicationTargetGroup.fromTargetGroupAttributes(this, 'importedAlbTg', { targetGroupArn: 'arn:aws:elasticloadbalancing:ap-northeast-2:111111111111:targetgroup/myAlbTargetgroup/abcd12345678efgf' }) ), // also network loadbalancer targetgroup loadBalancer: codedeploy.LoadBalancer.network( elbv2.NetworkTargetGroup.fromTargetGroupAttributes(this, 'importedNlbTg', { targetGroupArn: 'arn:aws:elasticloadbalancing:ap-northeast-2:111111111111:targetgroup/myNlbTargetgroup/wxyz09876543opqr' }) ), }); ``` ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

eastNine and others added 3 commits December 5, 2021 00:49

fix for codedeploy loadbalancer configuration can support imported al…

f9e012f

…b and nlb

Merge branch 'aws:master' into fix-codedeploy-loadbalancer

cd174cd

fix tests

4562daa

github-actions bot added the @aws-cdk/aws-codedeploy Related to AWS CodeDeploy label Dec 5, 2021

github-actions bot assigned skinny85 Dec 5, 2021

eastNine and others added 4 commits December 5, 2021 19:32

fix(codedeploy): codedeploy loadbalancer configuration can support im…

be0dff1

…ported alb and nlb

fix tests

26bcd06

Merge branch 'fix-codedeploy-loadbalancer' of https://github.com/east…

8187bfb

…Nine/aws-cdk into fix-codedeploy-loadbalancer

eastNine changed the title ~~fix(codedeploy) : loadbalancer support imported application and network loadbalancer targetgroup~~ fix(codedeploy): loadbalancer support imported application and network loadbalancer targetgroup Dec 5, 2021

skinny85 previously requested changes Dec 6, 2021

View reviewed changes

moved tgname logic to ImportedTargetGroupBase

a8c8e88

skinny85 previously requested changes Dec 15, 2021

View reviewed changes

Merge branch 'master' into fix-codedeploy-loadbalancer

bf4d07e

use Stack.splitArn()

80b9e37

eastNine requested a review from skinny85 December 19, 2021 13:59

eastNine added 2 commits December 19, 2021 23:22

add empty lines

f628071

fix arn in test

e3823b6

eastNine force-pushed the fix-codedeploy-loadbalancer branch from c15e59c to e3823b6 Compare December 19, 2021 14:22

skinny85 approved these changes Dec 20, 2021

View reviewed changes

skinny85 changed the title ~~fix(codedeploy): loadbalancer support imported application and network loadbalancer targetgroup~~ fix(codedeploy): loadbalancer support for imported Target Groups Dec 20, 2021

skinny85 added the pr-linter/exempt-readme The PR linter will not require README changes label Dec 20, 2021

skinny85 changed the title ~~fix(codedeploy): loadbalancer support for imported Target Groups~~ feat(codedeploy): loadbalancer support for imported Target Groups Dec 20, 2021

Merge branch 'master' into fix-codedeploy-loadbalancer

13fc16d

mergify bot merged commit 32f1c80 into aws:master Dec 20, 2021

	expect(stack).toHaveResource('AWS::CodeDeploy::DeploymentGroup', {
	expect(stack).toHaveResourceLike('AWS::CodeDeploy::DeploymentGroup', {

Conversation

eastNine commented Dec 5, 2021

Uh oh!

gitpod-io bot commented Dec 5, 2021

Uh oh!

mergify bot commented Dec 5, 2021

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

skinny85 commented Dec 15, 2021

Uh oh!

eastNine commented Dec 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Dec 20, 2021

Uh oh!

mergify bot commented Dec 20, 2021

Uh oh!

aws-cdk-automation commented Dec 20, 2021

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Dec 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

eastNine commented Dec 19, 2021 •

edited

Loading