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 all 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .../aws-cdk-nlb-attributes-integ.assets.json → .../aws-cdk-nlb-attributes-integ.assets.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 0 additions & 4 deletions ...ws-cdk-nlb-attributes-integ.template.json → ...ws-cdk-nlb-attributes-integ.template.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,10 +403,6 @@
"Key": "load_balancing.cross_zone.enabled",
"Value": "true"
},
{
"Key": "ipv6.deny_all_igw_traffic",
"Value": "true"
},
Comment on lines -406 to -409
Copy link
Contributor

Choose a reason for hiding this comment

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

That's a scary change, I'm going to assume it's an unintended side effect. Given the integration was being deployed before your change, I assume the integ stack was being deployed successfully

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hehe, well that's the thing, I could not deploy the integration test before my change. The deployment failed with

Resource handler returned message: "Load balancer attribute key 'ipv6.deny_all_igw_traffic' is not supported on load balancers with IP address type 'ipv4'.

That's why I renamed the test to, in effect, remove the old test and replace with a template you can deploy (the name I use also aligns with the existing alb test). This particular integration test was added very recently. I can only think that who ever created it only synthesized the test and didn't try deploying.

Copy link
Contributor

Choose a reason for hiding this comment

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

@badmintoncryer Could we get some clarification on this? See #29521

Copy link
Contributor

Choose a reason for hiding this comment

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

Surely I haven't created a snapshot without deploying it...
I'll try it later. Please wait for a while.

Copy link
Contributor

Choose a reason for hiding this comment

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

@nmussy @Michae1CC
I tried to deploy integ.nlb-attributes.ts but it cannot. I'm really sorry and I don't know how I could create snapshot files...

I have conducted deployment verification for internal ALBs and NLBs across several patterns.
The results are the same for both ALB and NLB.

ipAddressType denyAllIgwTraffic deployment result error message
IPV4 true failed Load balancer attribute key 'ipv6.deny_all_igw_traffic' is not supported on load balancers with IP address type 'ipv4'.
IPV4 false failed Load balancer attribute key 'ipv6.deny_all_igw_traffic' is not supported on load balancers with IP address type 'ipv4'.
DUAL_STACK true success
DUAL_STACK false failed Load balancer attribute key 'ipv6.deny_all_igw_traffic' cannot be modified for load balancers of type 'network'.

Based on these results, I propose the following amendments:

  1. If denyAllIgwTraffic is defined, return an error if ipAddressType is not DUAL_STACK.
  2. Revise the existing integ.nlb-attributes.ts to make it deployable:
  • Remove the denyAllIgwTraffic setting.
  • Instead, set denyAllIgwTraffic in integ.nlb.dualstack.internal.ts.

For item 2, it might be better to handle it as a separate issue.
If so, I will take responsibility for addressing it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Michae1CC My only suggestion is that the PR title should describe the content of the bug being resolved, rather than just mentioning the resolution of integration test errors.

Titles for feat and fix PRs end up in the change log. Think about what makes most sense for users reading the changelog while writing them.
- feat: describe the feature (not the action of creating the commit or PR, for example, avoid words like "added" or "changed")
- fix: describe the bug (not the solution)

Since I am unable to conduct community reviews, it would be best for the rest to be checked by @nmussy .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, maybe something like integ test for NLB attributes failing to deploy due to setting denyAllIgwTraffic?

Copy link
Contributor

Choose a reason for hiding this comment

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

I apologize, I finally realized that this was actually a PR intended to resolve an integration test error. My English skills are limited and I made a significant misunderstanding. My suggestion to split the PR also made no sense. I'm sorry.

Without specifically mentioning the integration test, how about using the title:
'fix(elbv2): unable to deploy an IPv4 load balancer with denyAllIgwTraffic enabled'?
or considering it as the addition of a verification feature,
'feat(elbv2): verification of denyAllIgwTraffic for IPv4 Load Balancers'

However, I would appreciate it if you could consider nmussy opinion on this matter as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

No need to apologize! And I agree, this PR's goal is not to fix an integration test, it's to prevent this pattern from being synthesized in the first place

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks both, updated the title

{
"Key": "dns_record.client_routing_policy",
"Value": "partial_availability_zone_affinity"
Expand Down
0 .../integ.nlb-attributes.js.snapshot/cdk.out → .../integ.nlb.attributes.js.snapshot/cdk.out
View file
Open in desktop
File renamed without changes.
0 ...teg.nlb-attributes.js.snapshot/integ.json → ...teg.nlb.attributes.js.snapshot/integ.json
View file
Open in desktop
File renamed without changes.
2 changes: 1 addition & 1 deletion ....nlb-attributes.js.snapshot/manifest.json → ....nlb.attributes.js.snapshot/manifest.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 ...faultTestDeployAssert2D727654.assets.json → ...faultTestDeployAssert2D727654.assets.json
View file
Open in desktop
File renamed without changes.
0 ...ultTestDeployAssert2D727654.template.json → ...ultTestDeployAssert2D727654.template.json
View file
Open in desktop
File renamed without changes.
4 changes: 0 additions & 4 deletions ...nteg.nlb-attributes.js.snapshot/tree.json → ...nteg.nlb.attributes.js.snapshot/tree.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion ...dbalancingv2/test/integ.nlb-attributes.ts → ...dbalancingv2/test/integ.nlb.attributes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ new elbv2.NetworkLoadBalancer(stack, 'NLB', {
vpc,
crossZoneEnabled: true,
deletionProtection: false,
denyAllIgwTraffic: true,
clientRoutingPolicy: elbv2.ClientRoutingPolicy.PARTIAL_AVAILABILITY_ZONE_AFFINITY,
});

Expand Down
2 changes: 1 addition & 1 deletion ...internal.js.snapshot/NlbDualstackInternalIntegDefaultTestDeployAssertEEBE69CB.assets.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions .../test/integ.nlb.dualstack.internal.js.snapshot/aws-cdk-nlb-dualstack-internal.assets.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions ...est/integ.nlb.dualstack.internal.js.snapshot/aws-cdk-nlb-dualstack-internal.template.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,10 @@
{
"Key": "deletion_protection.enabled",
"Value": "false"
},
{
"Key": "ipv6.deny_all_igw_traffic",
"Value": "true"
}
],
"Scheme": "internal",
Expand Down
2 changes: 1 addition & 1 deletion ...teg/test/aws-elasticloadbalancingv2/test/integ.nlb.dualstack.internal.js.snapshot/cdk.out
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion .../test/aws-elasticloadbalancingv2/test/integ.nlb.dualstack.internal.js.snapshot/integ.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions ...st/aws-elasticloadbalancingv2/test/integ.nlb.dualstack.internal.js.snapshot/manifest.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 6 additions & 2 deletions ...g/test/aws-elasticloadbalancingv2/test/integ.nlb.dualstack.internal.js.snapshot/tree.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions ...ting/framework-integ/test/aws-elasticloadbalancingv2/test/integ.nlb.dualstack.internal.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ const subnetIpv6CidrBlocks = cdk.Fn.cidr(vpcIpv6CidrBlock, 256, '64');

const lb = new elbv2.NetworkLoadBalancer(stack, 'LB', {
vpc,
denyAllIgwTraffic: true,
ipAddressType: elbv2.IpAddressType.DUAL_STACK,
});

Expand Down
7 changes: 6 additions & 1 deletion packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/shared/base-load-balancer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 @@ -251,7 +252,11 @@ export abstract class BaseLoadBalancer extends Resource {
}

if (baseProps.denyAllIgwTraffic !== undefined) {
this.setAttribute('ipv6.deny_all_igw_traffic', baseProps.denyAllIgwTraffic.toString());
if (additionalProps.ipAddressType === IpAddressType.DUAL_STACK) {
this.setAttribute('ipv6.deny_all_igw_traffic', baseProps.denyAllIgwTraffic.toString());
} else {
throw new Error(`'denyAllIgwTraffic' may only be set on load balancers with ${IpAddressType.DUAL_STACK} addressing.`);
}
}

this.loadBalancerCanonicalHostedZoneId = resource.attrCanonicalHostedZoneId;
Expand Down
67 changes: 62 additions & 5 deletions packages/aws-cdk-lib/aws-elasticloadbalancingv2/test/alb/load-balancer.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,6 @@ describe('tests', () => {
idleTimeout: cdk.Duration.seconds(1000),
dropInvalidHeaderFields: true,
clientKeepAlive: cdk.Duration.seconds(200),
denyAllIgwTraffic: true,
preserveHostHeader: true,
xAmznTlsVersionAndCipherSuiteHeaders: true,
preserveXffClientPort: true,
Expand All @@ -99,10 +98,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 +166,26 @@ describe('tests', () => {
}).toThrow('\'clientKeepAlive\' must be between 60 and 604800 seconds. Got: 100 milliseconds');
});

test.each([
[false, undefined],
[true, undefined],
[false, elbv2.IpAddressType.IPV4],
[true, elbv2.IpAddressType.IPV4],
])('throw error for denyAllIgwTraffic set to %s for Ipv4 (default) addressing.', (denyAllIgwTraffic, ipAddressType) => {
// GIVEN
const stack = new cdk.Stack();
const vpc = new ec2.Vpc(stack, 'Stack');

// THEN
expect(() => {
new elbv2.ApplicationLoadBalancer(stack, 'LB', {
vpc,
denyAllIgwTraffic: denyAllIgwTraffic,
ipAddressType: ipAddressType,
});
}).toThrow(`'denyAllIgwTraffic' may only be set on load balancers with ${elbv2.IpAddressType.DUAL_STACK} addressing.`);
});

describe('Desync mitigation mode', () => {
test('Defensive', () => {
// GIVEN
Expand Down Expand Up @@ -971,6 +986,27 @@ describe('tests', () => {
});
});

test('Can create internet-facing dualstack ApplicationLoadBalancer with denyAllIgwTraffic set to false', () => {
// GIVEN
const stack = new cdk.Stack();
const vpc = new ec2.Vpc(stack, 'Stack');

// WHEN
new elbv2.ApplicationLoadBalancer(stack, 'LB', {
vpc,
denyAllIgwTraffic: false,
internetFacing: true,
ipAddressType: elbv2.IpAddressType.DUAL_STACK,
});

// THEN
Template.fromStack(stack).hasResourceProperties('AWS::ElasticLoadBalancingV2::LoadBalancer', {
Scheme: 'internet-facing',
Type: 'application',
IpAddressType: 'dualstack',
});
});

test('Can create internal dualstack ApplicationLoadBalancer', () => {
// GIVEN
const stack = new cdk.Stack();
Expand All @@ -989,5 +1025,26 @@ describe('tests', () => {
IpAddressType: 'dualstack',
});
});

test.each([undefined, false])('Can create internal dualstack ApplicationLoadBalancer with denyAllIgwTraffic set to true', (internetFacing) => {
// GIVEN
const stack = new cdk.Stack();
const vpc = new ec2.Vpc(stack, 'Stack');

// WHEN
new elbv2.ApplicationLoadBalancer(stack, 'LB', {
vpc,
denyAllIgwTraffic: true,
internetFacing: internetFacing,
ipAddressType: elbv2.IpAddressType.DUAL_STACK,
});

// THEN
Template.fromStack(stack).hasResourceProperties('AWS::ElasticLoadBalancingV2::LoadBalancer', {
Scheme: 'internal',
Type: 'application',
IpAddressType: 'dualstack',
});
});
});
});
50 changes: 44 additions & 6 deletions packages/aws-cdk-lib/aws-elasticloadbalancingv2/test/nlb/load-balancer.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,6 @@ describe('tests', () => {
new elbv2.NetworkLoadBalancer(stack, 'LB', {
vpc,
crossZoneEnabled: true,
denyAllIgwTraffic: true,
clientRoutingPolicy: elbv2.ClientRoutingPolicy.PARTIAL_AVAILABILITY_ZONE_AFFINITY,
});

Expand All @@ -91,10 +90,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 +483,26 @@ describe('tests', () => {
}).toThrow('Load balancer name: "my load balancer" must contain only alphanumeric characters or hyphens.');
});

test.each([
[false, undefined],
[true, undefined],
[false, elbv2.IpAddressType.IPV4],
[true, elbv2.IpAddressType.IPV4],
])('throw error for denyAllIgwTraffic set to %s for Ipv4 (default) addressing.', (denyAllIgwTraffic, ipAddressType) => {
// GIVEN
const stack = new cdk.Stack();
const vpc = new ec2.Vpc(stack, 'Stack');

// THEN
expect(() => {
new elbv2.NetworkLoadBalancer(stack, 'NLB', {
vpc,
denyAllIgwTraffic: denyAllIgwTraffic,
ipAddressType: ipAddressType,
});
}).toThrow(`'denyAllIgwTraffic' may only be set on load balancers with ${elbv2.IpAddressType.DUAL_STACK} addressing.`);
});

test('imported network load balancer with no vpc specified throws error when calling addTargets', () => {
// GIVEN
const stack = new cdk.Stack();
Expand Down Expand Up @@ -1074,14 +1089,37 @@ describe('tests', () => {
});
});

test('Can create internal dualstack NetworkLoadBalancer', () => {
test('Can create internet-facing dualstack NetworkLoadBalancer with denyAllIgwTraffic set to false', () => {
// GIVEN
const stack = new cdk.Stack();
const vpc = new ec2.Vpc(stack, 'Stack');

// WHEN
new elbv2.NetworkLoadBalancer(stack, 'LB', {
vpc,
denyAllIgwTraffic: false,
internetFacing: true,
ipAddressType: elbv2.IpAddressType.DUAL_STACK,
});

// THEN
Template.fromStack(stack).hasResourceProperties('AWS::ElasticLoadBalancingV2::LoadBalancer', {
Scheme: 'internet-facing',
Type: 'network',
IpAddressType: 'dualstack',
});
});

test.each([undefined, false])('Can create internal dualstack NetworkLoadBalancer with denyAllIgwTraffic set to true', (internetFacing) => {
// GIVEN
const stack = new cdk.Stack();
const vpc = new ec2.Vpc(stack, 'Stack');

// WHEN
new elbv2.NetworkLoadBalancer(stack, 'LB', {
vpc,
denyAllIgwTraffic: true,
internetFacing: internetFacing,
ipAddressType: elbv2.IpAddressType.DUAL_STACK,
});

Expand Down