fix(elasticloadbalancingv2): default to one subnet per AZ

[scubbo]

Both ALBs and NLBs require that the subnets chosen from the VPC are all in separate AZs - however, the default SubnetSelection does not enforce that requirement.

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

[gitpod-io]

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

[scubbo]

Note that I did have one test failure from yarn test after making this change (output copied below) - however, this test also failed on a clean commit, so I believe it's an issue with my setup rather than with the code.

=============================== Coverage summary ===============================
Statements   : 57.53% ( 59717/103792 )
Branches     : 41.07% ( 16817/40938 )
Functions    : 66.92% ( 11511/17200 )
Lines        : 58.97% ( 57763/97953 )
================================================================================

Summary of all failing tests
 FAIL  custom-resources/test/aws-custom-resource/runtime/aws-sdk-v2-handler.test.js (34.17 s)
  ● SDK global credentials are never set

    expect(received).toBeNull()

    Received: {"accessKeyId": "AKIARXRN7F6FW34GHS6E", "disableAssumeRole": true, "expireTime": null, "expired": false, "filename": undefined, "httpOptions": null, "preferStaticCredentials": false, "profile": "default", "refreshCallbacks": [], "sessionToken": undefined, "tokenCodeFn": null}

      67 |   // THEN
      68 |   expect(AWS.config).toBeInstanceOf(AWS.Config);
    > 69 |   expect(AWS.config.credentials).toBeNull();
         |                                  ^
      70 | });
      71 |
      72 | test('SDK credentials are not persisted across subsequent invocations', async () => {

      at Object.toBeNull (custom-resources/test/aws-custom-resource/runtime/aws-sdk-v2-handler.test.ts:69:34)


Test Suites: 1 failed, 1 skipped, 657 passed, 658 of 659 total
Tests:       1 failed, 40 skipped, 10153 passed, 10194 total
Snapshots:   20 passed, 20 total
Time:        836.062 s
Ran all test suites.
error Command failed with exit code 1.

---

Note also that it's possible (though I'm not certain) that the CONTRIBUTING instructions are out-of-date. They state a requirement for Node.js >= 14.15.0 , but when I initially tried npx lerna run build --scope=aws-cdk-lib with version 14.17.0 I got:

lerna notice cli v6.6.2
lerna notice filter including "aws-cdk-lib"
lerna info filter [ 'aws-cdk-lib' ]

   ✖    1/7 dependent project tasks failed (see below)
   ✔    6/7 dependent project tasks succeeded [0 read from cache]

 ———————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————


> @aws-cdk/cfnspec:build

yarn run v1.22.19
$ cdk-build && ts-node --preferTsExts build-tools/build.ts
internal/modules/cjs/loader.js:888
  throw err;
  ^

Error: Cannot find module 'node:fs'

StackOverflow suggests that this naming was introduced in Node v18, though it has apparently been backported to v14 and v16. This error was resolved by updating to v18.12.0.

If it's determined that the docs need to be updated, I'd be happy to do so - either in this PR or a separate one.

[aws-cdk-automation]

AWS CodeBuild CI Report

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

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

[scubbo]

Hmm - I tried adding a test for this in aws-elasticloadbalancingv2/test/nlb/load-balancer.test.ts, but it seems the .synth() doesn't throw an Exception. Maybe I've misunderstood the situation?

test('Load Balancer with default Subnet settings refuses to synth if AZ-unique Subnets cannot be found', () => {
  // GIVEN
  const app = new cdk.App();
  const stack = new cdk.Stack(app);
  const vpc = new ec2.Vpc(stack, 'VPC', {
    subnetConfiguration: [{
      cidrMask: 24,
      name: 'Public-1',
      subnetType: ec2.SubnetType.PUBLIC,
    }, {
      cidrMask: 24,
      name: 'Public-2',
      subnetType: ec2.SubnetType.PUBLIC,
    }],
    maxAzs: 1,
  });

  // WHEN
  new elbv2.NetworkLoadBalancer(stack, 'LB', {
    vpc,
  });

  // THEN
  expect(() => {
    app.synth();
  }).toThrow('Some error'); // <-- but it doesn't
});

[aws-cdk-automation]

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

[scubbo]

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned.

Still waiting on guidance on how I've written the tests incorrectly.

[aws-cdk-automation]

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

[aws-cdk-automation]

The pull request linter fails with the following errors:

❌ Fixes must contain a change to a test file.
❌ Fixes must contain a change to an integration test file and the resulting snapshot.

PRs must pass status checks before we can provide a meaningful review.

If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing Exemption Request and/or Clarification Request.

[mrgrain]

Hi @scubbo Apologies for the very late reply. Your test code should work in principal, but why would you expect your code to throw?
Also can you please open an issue so our issue triage team can have a look at it.
If you are stuck with code problems, you can leverage the community on cdk.dev to get help.

Feel free to open a new PR that references an issue.