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

Show file tree
Hide file tree
Changes from 3 commits
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
19 changes: 15 additions & 4 deletions packages/aws-cdk-lib/aws-elasticloadbalancingv2/test/alb/load-balancer.test.ts
View file
Open in desktop
Expand Up @@ -99,10 +99,6 @@ describe('tests', () => {
Key: 'deletion_protection.enabled',
Value: 'true',
},
{
Key: 'ipv6.deny_all_igw_traffic',
Value: 'true',
},
{
Key: 'routing.http2.enabled',
Value: 'false',
Expand Down Expand Up @@ -171,6 +167,21 @@ describe('tests', () => {
}).toThrow('\'clientKeepAlive\' must be between 60 and 604800 seconds. Got: 100 milliseconds');
});

test('throw error for denyAllIgwTraffic set to false for Ipv4 adressing.', () => {
// GIVEN
const stack = new cdk.Stack();
const vpc = new ec2.Vpc(stack, 'Stack');

// THEN
expect(() => {
new elbv2.ApplicationLoadBalancer(stack, 'LB', {
vpc,
denyAllIgwTraffic: false,
ipAddressType: elbv2.IpAddressType.IPV4,
});
}).toThrow('\'denyAllIgwTraffic\' cannot be false on load balancers with IPv4 addressing.');
});

describe('Desync mitigation mode', () => {
test('Defensive', () => {
// GIVEN
Expand Down
19 changes: 15 additions & 4 deletions packages/aws-cdk-lib/aws-elasticloadbalancingv2/test/nlb/load-balancer.test.ts
View file
Open in desktop
Expand Up @@ -91,10 +91,6 @@ describe('tests', () => {
Key: 'load_balancing.cross_zone.enabled',
Value: 'true',
},
{
Key: 'ipv6.deny_all_igw_traffic',
Value: 'true',
},
{
Key: 'dns_record.client_routing_policy',
Value: 'partial_availability_zone_affinity',
Expand Down Expand Up @@ -488,6 +484,21 @@ describe('tests', () => {
}).toThrow('Load balancer name: "my load balancer" must contain only alphanumeric characters or hyphens.');
});

test('loadBalancerName unallowed: denyAllIgwTraffic set to false for Ipv4 adressing', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
test('loadBalancerName unallowed: denyAllIgwTraffic set to false for Ipv4 adressing', () => {
test('throw error for denyAllIgwTraffic set to false for Ipv4 adressing', () => {

// GIVEN
const stack = new cdk.Stack();
const vpc = new ec2.Vpc(stack, 'Stack');

// THEN
expect(() => {
new elbv2.NetworkLoadBalancer(stack, 'NLB', {
vpc,
denyAllIgwTraffic: false,
ipAddressType: elbv2.IpAddressType.IPV4,
});
}).toThrow('\'denyAllIgwTraffic\' cannot be false on load balancers with IPv4 addressing.');
});

test('imported network load balancer with no vpc specified throws error when calling addTargets', () => {
// GIVEN
const stack = new cdk.Stack();
Expand Down