-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Conversation
@@ -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) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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', { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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"
}
}
There was a problem hiding this comment.
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', |
There was a problem hiding this comment.
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',
},
],
});
9cea6b5
to
9c216bc
Compare
Pull request has been modified.
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 CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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). |
Avoid setting
stickiness.enabled
tofalse
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