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

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

Issue # (if applicable)

Closes #30247 .

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.

Michae1CC · 2024-05-13T04:19:00Z

Just curious as to what the new behavior will be post-PR?

This PR isn't changing much. The biggest change (in terms of functionality) will be that the deploy error you got will now occur during synthesis with a more descriptive message.

Michae1CC · 2024-05-13T04:19:52Z

@moelasmar , let me know if anything else needs to change - thanks.

moelasmar · 2024-05-15T19:34:09Z

thanks @Michae1CC for raising this PR and fix this issue. It looks good to me, I will create a github issue for this PR, and link it to this PR (just for tracking purposes). I need also to verify the integration test cases on my side before approving it.

mergify · 2024-05-17T01:42:59Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

mergify · 2024-05-17T01:46:54Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Pull request has been modified.

aws-cdk-automation · 2024-05-17T02:17:18Z

AWS CodeBuild CI Report

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

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

mergify · 2024-05-17T02:17:43Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

…nyAllIgwTraffic set (aws#29956) ### Issue # (if applicable) Closes aws#30247 . ### Reason for this change Integ test for NLB attributes ([integ.nlb-attributes.ts](https://github.com/aws/aws-cdk/blob/4f1c94b27ef7f4ceccea0ff39625c0e8add31c9f/packages/%40aws-cdk-testing/framework-integ/test/aws-elasticloadbalancingv2/test/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 - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

aws-cdk-automation · 2024-07-25T21:13:25Z

Comments on closed issues and PRs are hard for our team to see. If you need help, please open a new issue that references this one.

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

moelasmar added the @aws-cdk/aws-elasticloadbalancingv2 Related to Amazon Elastic Load Balancing V2 label May 17, 2024

github-actions bot added the bug This issue is a bug. label May 17, 2024

moelasmar previously approved these changes May 17, 2024

View reviewed changes

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

Merge branch 'main' into denyAllIgwTraffic-ipv4-lb-fix

dc77a5c

moelasmar approved these changes May 17, 2024

View reviewed changes

mergify bot merged commit 42d424e into aws:main May 17, 2024
11 checks passed

NOUIY mentioned this pull request Jun 14, 2024

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#103

Open

snyk-io bot mentioned this pull request Jun 20, 2024

[Snyk] Upgrade aws-cdk-lib from 2.138.0 to 2.143.1 Opetushallitus/heratepalvelu#280

Open

NOUIY mentioned this pull request Jun 20, 2024

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#104

Open

snyk-io bot mentioned this pull request Jun 21, 2024

[Snyk] Upgrade aws-cdk-lib from 2.139.0 to 2.143.1 Opetushallitus/ovara#36

Open

rwlxxvii mentioned this pull request Jun 22, 2024

[Snyk] Upgrade aws-cdk-lib from 2.139.0 to 2.144.0 rwlxxvii/containers#140

Merged

This was referenced Jun 22, 2024

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#105

Open

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#106

Open

This was referenced Jul 5, 2024

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#107

Open

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#108

Open

This was referenced Jul 16, 2024

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#109

Open

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#110

Open

NOUIY mentioned this pull request Jul 23, 2024

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#111

Open

aws locked as resolved and limited conversation to collaborators Jul 25, 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 by moelasmar

Loading

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

Loading

Michae1CC commented May 13, 2024

Michae1CC commented May 13, 2024

moelasmar commented May 15, 2024

mergify bot commented May 17, 2024

mergify bot commented May 17, 2024

aws-cdk-automation commented May 17, 2024

mergify bot commented May 17, 2024

aws-cdk-automation commented Jul 25, 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

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

Conversation

Michae1CC commented Apr 25, 2024 • edited by moelasmar Loading

Issue # (if applicable)

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 Loading

Michae1CC commented May 13, 2024

Michae1CC commented May 13, 2024

moelasmar commented May 15, 2024

mergify bot commented May 17, 2024

mergify bot commented May 17, 2024

aws-cdk-automation commented May 17, 2024

AWS CodeBuild CI Report

mergify bot commented May 17, 2024

aws-cdk-automation commented Jul 25, 2024

Michae1CC commented Apr 25, 2024 •

edited by moelasmar

Loading

Michae1CC commented Apr 27, 2024 •

edited

Loading