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

Michae1CC · 2024-04-25T01:55:58Z

Reason for this change

Integ test for NLB attributes (integ.nlb-attributes.ts) fails to deploy due to an error. The error occurs when denyAllIgwTraffic is explicitly set for load balancers with Ipv4 addressing, the ipv6.deny_all_igw_traffic attribute is set.

Description of changes

Remove the denyAllIgwTraffic setting from integ.nlb-attribute.ts
Instead, set denyAllIgwTraffic in integ.nlb.dualstack.internal.ts.
Raise an error during synthesis if denyAllIgwTraffic is set on a load balancer that does not use dual stack addressing.

Description of how you validated changes

Added new unit tests for different combinations of denyAllIgwTraffic and ipAddressType
Updated existing integration test

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

nmussy

I think your change is affecting unrelated configurations, I would double check your condition and what you're expecting it do accomplish

I would also like to see additional unit tests, a combination of the following possibilities. I'm not sure what's the expected behavior for some of them, and coverage is always good:

denyAllIgwTraffic: undefined/denyAllIgwTraffic: false/denyAllIgwTraffic: true
ipAddressType: undefined/ipAddressType: IpAddressType.IPV4/ipAddressType: IpAddressType.DUALSTACK

Would you also be able to add additional integration tests to make sure that denyAllIgwTraffic: false and ipAddressType: IpAddressType.DUALSTACK is being deployed correctly?

nmussy · 2024-04-25T12:43:10Z

packages/aws-cdk-lib/aws-elasticloadbalancingv2/test/nlb/load-balancer.test.ts

@@ -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', () => {


Suggested change

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

test('throw error for denyAllIgwTraffic set to false for Ipv4 adressing', () => {

nmussy · 2024-04-25T12:47:45Z

packages/aws-cdk-lib/aws-elasticloadbalancingv2/lib/shared/base-load-balancer.ts

@@ -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) {


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

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

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

nmussy · 2024-04-25T12:52:37Z

...balancingv2/test/integ.nlb.attributes.js.snapshot/aws-cdk-nlb-attributes-integ.template.json

-     {
-      "Key": "ipv6.deny_all_igw_traffic",
-      "Value": "true"
-     },


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

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.

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

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

@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:

If denyAllIgwTraffic is defined, return an error if ipAddressType is not DUAL_STACK.

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.

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

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

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.

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

Thanks both, updated the title

nmussy · 2024-04-25T14:37:31Z

@Michae1CC I also left you a general comment in the review, in case you didn't see it: #29956 (review)

Michae1CC · 2024-04-25T14:39:44Z

@Michae1CC I also left you a general comment in the review, in case you didn't see it: #29956 (review)

Yes, thanks again! I'll add more tests once we've aligned on the expected behaviour.

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

Michae1CC · 2024-04-27T00:34:30Z

Hey both, updated the integration tests and PR description. Let me know what you think.

aws-cdk-automation · 2024-04-27T00:57:44Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 8542f60
Result: SUCCEEDED
Build Logs (available for 30 days)

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

Michae1CC · 2024-05-07T06:24:50Z

@nmussy just floating this back up, are there any other changes you wanted? @badmintoncryer seems happy with it, but mentioned they would leave the review to you since they're not part of the cdk team.

nmussy · 2024-05-07T07:20:18Z

I'm not a maintainer either 😅 But yes, LGTM 👍

jrobbins-LiveData · 2024-05-08T01:00:37Z

I don't know if the problem I am having trying to deploy an ELBv2 with an IPv4 VPC is related to this issue or not (sorry if irrelevant), but I am getting this error

AlbCdk-test | 14/22 | 8:28:08 PM | CREATE_FAILED        | AWS::ElasticLoadBalancingV2::LoadBalancer | ALB (ALBAEE750D2) Resource handler returned message: "Load balancer attribute key 'ipv6.deny_all_igw_traffic' is not supported on load balancers with IP address type 'ipv4'. (Service: ElasticLoadBalancingV2, Status Code: 400, Request ID: cfbc6c78-d50c-4760-a163-020bceffe4d2)" (RequestToken: c487d145-cdbc-6431-112b-daa1a9bc652f, HandlerErrorCode: InvalidRequest)

The Python CDK code is

        alb = elbv2.ApplicationLoadBalancer(
            self, 'ALB',
            internet_facing=True,
            http2_enabled=True,
            ip_address_type=elbv2.IpAddressType.IPV4,
            drop_invalid_header_fields=True,
            desync_mitigation_mode=elbv2.DesyncMitigationMode.DEFENSIVE,
            client_keep_alive=cdk.Duration.seconds(500),
            deny_all_igw_traffic=False,
            preserve_host_header=True,
            x_amzn_tls_version_and_cipher_suite_headers=True,
            preserve_xff_client_port=True,
            xff_header_processing_mode=elbv2.XffHeaderProcessingMode.APPEND,
            waf_fail_open=True,
            vpc=vpc,
            vpc_subnets=vpc_subnets
            # security_group=security_group
        )

The VPC is IPv4 only.

The subnets are the 3 public subnets of that VPC.

Michae1CC · 2024-05-08T01:06:36Z

I don't know if the problem I am having trying to deploy an ELBv2 with an IPv4 VPC is related to this issue or not (sorry if irrelevant), but I am getting this error

AlbCdk-test | 14/22 | 8:28:08 PM | CREATE_FAILED        | AWS::ElasticLoadBalancingV2::LoadBalancer | ALB (ALBAEE750D2) Resource handler returned message: "Load balancer attribute key 'ipv6.deny_all_igw_traffic' is not supported on load balancers with IP address type 'ipv4'. (Service: ElasticLoadBalancingV2, Status Code: 400, Request ID: cfbc6c78-d50c-4760-a163-020bceffe4d2)" (RequestToken: c487d145-cdbc-6431-112b-daa1a9bc652f, HandlerErrorCode: InvalidRequest)

The Python CDK code is

        alb = elbv2.ApplicationLoadBalancer(
            self, 'ALB',
            internet_facing=True,
            http2_enabled=True,
            ip_address_type=elbv2.IpAddressType.IPV4,
            drop_invalid_header_fields=True,
            desync_mitigation_mode=elbv2.DesyncMitigationMode.DEFENSIVE,
            client_keep_alive=cdk.Duration.seconds(500),
            deny_all_igw_traffic=False,
            preserve_host_header=True,
            x_amzn_tls_version_and_cipher_suite_headers=True,
            preserve_xff_client_port=True,
            xff_header_processing_mode=elbv2.XffHeaderProcessingMode.APPEND,
            waf_fail_open=True,
            vpc=vpc,
            vpc_subnets=vpc_subnets
            # security_group=security_group
        )

The VPC is IPv4 only.

The subnets are the 3 public subnets of that VPC.

@jrobbins-LiveData yes, this problem is directly related to these changes. Currently, having deny_all_igw_traffic set when using elbv2.IpAddressType.IPV4 will cause the template to fail during deploy. Part of these changes will catch this by throwing an error during synthesis.

Michae1CC · 2024-05-08T04:30:54Z

@paulhcsun , @GavinZZ would either of you mind reviewing this one - thanks.

jrobbins-LiveData · 2024-05-08T11:55:10Z

@Michae1CC, I commented out deny_all_igw_traffic=False and the deployment worked.

        alb = elbv2.ApplicationLoadBalancer(
            self, 'ALB',
            internet_facing=True,
            http2_enabled=True,
            ip_address_type=elbv2.IpAddressType.IPV4,
            drop_invalid_header_fields=True,
            desync_mitigation_mode=elbv2.DesyncMitigationMode.DEFENSIVE,
            client_keep_alive=cdk.Duration.seconds(500),
            # deny_all_igw_traffic=False,
            preserve_host_header=True,
            x_amzn_tls_version_and_cipher_suite_headers=True,
            preserve_xff_client_port=True,
            xff_header_processing_mode=elbv2.XffHeaderProcessingMode.APPEND,
            waf_fail_open=True,
            vpc=vpc,
            vpc_subnets=vpc_subnets
            # security_group=security_group
        )

The CDK help indeed does not list a default for this param as it is marked as Optional.

deny_all_igw_traffic (Optional[bool]) – Indicates whether the load balancer blocks traffic through the Internet Gateway (IGW). Default: - false for internet-facing load balancers and true for internal load balancers

Does that mean that deny_all_igw_traffic is a tri-state param, or is that what you are changing? Just curious as to what the new behavior will be post-PR?

github-actions bot added p2 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Apr 25, 2024

aws-cdk-automation requested a review from a team April 25, 2024 01:56

Handle denyAllIgwTraffic for Ipv4 LB

89cc140

Michae1CC force-pushed the denyAllIgwTraffic-ipv4-lb-fix branch from 5e4292f to 89cc140 Compare April 25, 2024 01:57

aws-cdk-automation previously requested changes Apr 25, 2024

View reviewed changes

Michae1CC changed the title ~~Handle denyAllIgwTraffic for Ipv4 Load Balancers~~ fix: handle denyAllIgwTraffic for Ipv4 Load Balancers Apr 25, 2024

Michae1CC marked this pull request as draft April 25, 2024 02:07

Added testing

291c665

Michae1CC force-pushed the denyAllIgwTraffic-ipv4-lb-fix branch from 14467eb to 291c665 Compare April 25, 2024 11:20

Michae1CC marked this pull request as ready for review April 25, 2024 11:31

nmussy suggested changes Apr 25, 2024

View reviewed changes

revert integration test changes

3b9ab47

aws-cdk-automation previously requested changes Apr 26, 2024

View reviewed changes

badmintoncryer mentioned this pull request Apr 26, 2024

elasticloadbalancingv2: fail to deploy integ test for NLB attributes #29970

Closed

logic and unit test changes

abd1ff2

This was referenced Apr 26, 2024

Weekly PR metrics report #29980

Closed

Weekly PR metrics report #29986

Closed

Add back integration testing

8542f60

Michae1CC requested review from nmussy and badmintoncryer April 27, 2024 00:33

Michae1CC changed the title ~~fix: handle denyAllIgwTraffic for Ipv4 Load Balancers~~ fix: fail to deploy integ test for NLB attributes Apr 27, 2024

This was referenced Apr 29, 2024

Weekly PR metrics report #29994

Closed

Monthly PR metrics report #30026

Closed

Michae1CC changed the title ~~fix: fail to deploy integ test for NLB attributes~~ fix(elbv2): unable to deploy template with IPv4 load balancer when denyAllIgwTraffic enabled May 1, 2024

Michae1CC changed the title ~~fix(elbv2): unable to deploy template with IPv4 load balancer when denyAllIgwTraffic enabled~~ fix(elbv2): unable to deploy template with IPv4 load balancer when denyAllIgwTraffic set May 1, 2024

nmussy approved these changes May 7, 2024

View reviewed changes

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label May 7, 2024

moelasmar self-assigned this May 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Michae1CC commented Apr 25, 2024 •

edited

aws-cdk-automation left a comment

nmussy left a comment

nmussy Apr 25, 2024

nmussy Apr 25, 2024

Michae1CC Apr 25, 2024

nmussy Apr 25, 2024

nmussy Apr 25, 2024

Michae1CC Apr 25, 2024

nmussy Apr 25, 2024

badmintoncryer Apr 25, 2024

badmintoncryer Apr 26, 2024

badmintoncryer May 1, 2024

Michae1CC May 1, 2024

badmintoncryer May 1, 2024

nmussy May 1, 2024

Michae1CC May 1, 2024

nmussy commented Apr 25, 2024

Michae1CC commented Apr 25, 2024

aws-cdk-automation left a comment

Michae1CC commented Apr 27, 2024 •

edited

aws-cdk-automation commented Apr 27, 2024

Michae1CC commented May 7, 2024

nmussy commented May 7, 2024

jrobbins-LiveData commented May 8, 2024 •

edited

Michae1CC commented May 8, 2024

Michae1CC commented May 8, 2024

jrobbins-LiveData commented May 8, 2024

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

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

`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'.

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

Are you sure you want to change the base?

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

Conversation

Michae1CC commented Apr 25, 2024 • edited

Reason for this change

Description of changes

Description of how you validated changes

Checklist

aws-cdk-automation left a comment

Choose a reason for hiding this comment

nmussy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmussy commented Apr 25, 2024

Michae1CC commented Apr 25, 2024

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Michae1CC commented Apr 27, 2024 • edited

aws-cdk-automation commented Apr 27, 2024

AWS CodeBuild CI Report

Michae1CC commented May 7, 2024

nmussy commented May 7, 2024

jrobbins-LiveData commented May 8, 2024 • edited

Michae1CC commented May 8, 2024

Michae1CC commented May 8, 2024

jrobbins-LiveData commented May 8, 2024

Michae1CC commented Apr 25, 2024 •

edited

Michae1CC commented Apr 27, 2024 •

edited

jrobbins-LiveData commented May 8, 2024 •

edited