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

fix(aws-elasticloadbalancingv2): Set stickiness.enabled unless target type is lambda #17271

Merged
merged 6 commits into from Nov 29, 2021

Conversation

topecongiro
Copy link
Contributor

Avoid setting stickiness.enabled to false when the target group type is lambda as it breaks on deployment.

Fixes #17261.


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

@gitpod-io
Copy link

gitpod-io bot commented Nov 2, 2021

@github-actions github-actions bot added the @aws-cdk/aws-elasticloadbalancing Related to Amazon Elastic Load Balancing label Nov 2, 2021
@@ -147,7 +147,7 @@ export class ApplicationTargetGroup extends TargetGroupBase implements IApplicat

if (props.stickinessCookieDuration) {
this.enableCookieStickiness(props.stickinessCookieDuration, props.stickinessCookieName);
} else {
} else if (this.targetType !== TargetType.LAMBDA) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this would be effective with your use case. Line 157 would call the function that sets the target type based on the target

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to clarify what @peterwoodworth is saying here -- this fixes the functionality when the targetType is explicitly defined as an input prop, but not when the type is not defined at creation, and is instead set by the target itself. Here's an example test that breaks with this implementation, but should work:

  test('Lambda target should not have stickiness.enabled set', () => {
    const app = new cdk.App();
    const stack = new cdk.Stack(app, 'Stack');
    const tg = new elbv2.ApplicationTargetGroup(stack, 'TG');
    tg.addTarget({
      attachToApplicationTargetGroup(_targetGroup: elbv2.IApplicationTargetGroup): elbv2.LoadBalancerTargetProps {
        return {
          targetType: elbv2.TargetType.LAMBDA,
          targetJson: { id: 'arn:aws:lambda:eu-west-1:123456789012:function:myFn' },
        };
      },
    });

    expect(stack).not.toHaveResourceLike('AWS::ElasticLoadBalancingV2::TargetGroup', {
      TargetGroupAttributes: [
        {
          Key: 'stickiness.enabled',
          Value: 'false',
        },
      ],
    });
  });

I think unfortunately we're going to have to do something kind of ugly, like detect when a Lambda targetType is added, and then unset the attribute if so (in addition to what you've done here).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@peterwoodworth @njlynch Sorry for the late reply, and thank you for your reviews! Will take a look.

targetType: elbv2.TargetType.LAMBDA,
});

expect(stack).not.toHaveResource('AWS::ElasticLoadBalancingV2::TargetGroup', {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll want toHaveResourceLike here. Otherwise, this just checks to make sure there's not a TargetGroup with only the attributes you specify

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@peterwoodworth Hmm, this doesn't seem to work anymore; maybe related to #17344? What can I use to test that a certain field does not exist in the resource?

    Not None of 1 resources matches resource 'AWS::ElasticLoadBalancingV2::TargetGroup' with {
      "$deepObjectLike": {
        "TargetGroupAttributes": [
          {
            "Key": "stickiness.enabled"
          }
        ]
      }
    }.
    - Field TargetGroupAttributes missing in:
        {
          "Type": "AWS::ElasticLoadBalancingV2::TargetGroup",
          "Properties": {
            "TargetType": "lambda"
          }
        }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind, it was just that I forgot to run yarn bulid before running yarn test 🤦‍♂️

TargetGroupAttributes: [
{
Key: 'stickiness.enabled',
Value: 'false',
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can remove this line also. That way the test will fail even if this is set to 'true'
i.e. use this:

expect(stack).not.toHaveResourceLike('AWS::ElasticLoadBalancingV2::TargetGroup', {
  TargetGroupAttributes: [
    {
      Key: 'stickiness.enabled',
    },
  ],
});

@mergify
Copy link
Contributor

mergify bot commented Nov 29, 2021

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
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 2b9f1f1
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify mergify bot merged commit 168a98f into aws:master Nov 29, 2021
@mergify
Copy link
Contributor

mergify bot commented Nov 29, 2021

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).

pedrosola pushed a commit to pedrosola/aws-cdk that referenced this pull request Dec 1, 2021
… type is lambda (aws#17271)

Avoid setting `stickiness.enabled` to `false` when the target group type is lambda as it breaks on deployment.

Fixes aws#17261.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
… type is lambda (aws#17271)

Avoid setting `stickiness.enabled` to `false` when the target group type is lambda as it breaks on deployment.

Fixes aws#17261.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-elasticloadbalancing Related to Amazon Elastic Load Balancing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aws-elasticloadbalancingv2: cannot create lambda TargetGroup
5 participants