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.

Sign up for GitHub

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

Jump to bottom

fix(elbv2): unable to deploy template with IPv4 load balancer when denyAllIgwTraffic set #29956

Merged
merged 6 commits into from May 17, 2024
Merged

fix(elbv2): unable to deploy template with IPv4 load balancer when denyAllIgwTraffic set #29956

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
89cc140
Handle denyAllIgwTraffic for Ipv4 LB
Michae1CC Apr 25, 2024
291c665
Added testing
Michae1CC Apr 25, 2024
3b9ab47
revert integration test changes
Michae1CC Apr 26, 2024
abd1ff2
logic and unit test changes
Michae1CC Apr 26, 2024
8542f60
Add back integration testing
Michae1CC Apr 27, 2024
dc77a5c
Merge branch 'main' into denyAllIgwTraffic-ipv4-lb-fix
moelasmar May 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/shared/base-load-balancer.ts
View file
Open in desktop
@@ -1,4 +1,5 @@
import { Construct } from 'constructs';
import { IpAddressType } from './enums';
import { Attributes, ifUndefined, mapTagMapToCxschema, renderAttributes } from './util';
import * as ec2 from '../../../aws-ec2';
import * as iam from '../../../aws-iam';
Expand Down Expand Up @@ -250,7 +251,9 @@ export abstract class BaseLoadBalancer extends Resource {
this.setAttribute('load_balancing.cross_zone.enabled', baseProps.crossZoneEnabled === true ? 'true' : 'false');
}

if (baseProps.denyAllIgwTraffic !== undefined) {
if (additionalProps.ipAddressType === IpAddressType.IPV4 && baseProps.denyAllIgwTraffic === false) {
Copy link
Contributor

Choose a reason for hiding this comment

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

ipAddressType is meant to be IpAddressType.IPV4 by default, and denyAllIgwTraffic is supposed to be false, I assume this condition is incorrect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Appreciate the feedback! Apologies for the spiel

Yes, ipAddressType is meant to be IpAddressType.IPV4. As for denyAllIgwTraffic the docs mention "optional, default: false for internet-facing load balancers and true for internal load balancers". Looking at code though, denyAllIgwTraffic is never actually given a default value which is why we have this check for undefined here.

The denyAllIgwTraffic was created from this issue: #29520. Basically
denyAllIgwTraffic is supposed to set ipv6.deny_all_igw_traffic. However, having ipv6.deny_all_igw_traffic set (to either true or false) for any Ipv4 load balancer will cause a failed deployment. I'm starting to think the condition should actually be this:

Suggested change
if (additionalProps.ipAddressType === IpAddressType.IPV4 && baseProps.denyAllIgwTraffic === false) {
if (additionalProps.ipAddressType === IpAddressType.IPV4 && baseProps.denyAllIgwTraffic !== undefined) {

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at code though, denyAllIgwTraffic is never actually given a default value which is why we have this check for undefined here.

The CDK doesn't always set the default values, in fact most of the time CloudFormation does. This is the case here, although it's more complicated than just defaulted to false, see docs. The @default value needs to be updated to reflect this

throw new Error('\'denyAllIgwTraffic\' cannot be false on load balancers with IPv4 addressing.');
} else if (additionalProps.ipAddressType === IpAddressType.DUAL_STACK && baseProps.denyAllIgwTraffic !== undefined) {
this.setAttribute('ipv6.deny_all_igw_traffic', baseProps.denyAllIgwTraffic.toString());
}

Expand Down