fix(ec2): can't add non-default routes to subnets

[ialford]

Update the destinationCidrBlockIPV6 to use the proper options and not duplicate destinationCidrBlock

fixes #5321

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

[mergify]

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

• PR title type(scope): text
  • type: fix, feat, refactor go into CHANGELOG, chore is hidden
  • scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
  • text: use all lower-case, do not end with a period, do not include issue refs
• PR Description
  • Rationale: describe rationale of change and approach taken
  • Issues: indicate issues fixed via: fixes #xxx or closes #xxx
  • Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
• Testing
  • Unit test added. Prefer to add a new test rather than modify existing tests
  • CLI or init templates change? Re-run/add CLI integration tests
• Documentation
  • README: update module README to describe new features
  • API docs: public APIs must be documented. Copy from official AWS docs when possible
  • Design: for significant features, follow design process

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[rix0rrr]

Hi Ian,

Could you add a test? This might help you:

https://docs.aws.amazon.com/cdk/latest/guide/testing.html

You can also have a peek at the other tests in the same package to get inspiration.

[ialford]

Hi Ian,

Could you add a test? This might help you:

https://docs.aws.amazon.com/cdk/latest/guide/testing.html

You can also have a peek at the other tests in the same package to get inspiration.

Hi @rix0rrr - I was thinking of adding the unit test for this into the VPC unit tests, does that make sense to you as the right place?

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[ialford]

@rix0rrr - I've added a test to validate the removal of the DestinationIPv6CidrBlock which is what the bug was identifying. Please let me know if you need anything else.

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: FAILED
• Build Logs (available for 30 days)

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

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[rix0rrr]

Hi @ialford,

{} represents an object with no properties, but you seem to be using it as a kind of "wildcard" property, which it does not actually represent. The test is currently testing that a resource with invalid properties is NOT created. It succeeds, but not because of the code you changed.

You can check whether a test is a good test by running it WITHOUT your proposed code change (in which case it should fail) and again WITH your proposed code change (in which case it should succeed). By doing that, you know the test is actually testing the code change and not just passing by accident.

I would propose changing it to the following:

    'Can add an IPv6 route'(test: Test) {
      // GIVEN
      const stack = getTestStack();

      // WHEN
      const vpc = new Vpc(stack, 'VPC');
      (vpc.publicSubnets[0] as PublicSubnet).addRoute('SomeRoute', {
        destinationIpv6CidrBlock: '2001:4860:4860::8888/32',
        routerId: 'router-1',
        routerType: RouterType.NETWORK_INTERFACE
      });

      // THEN

      expect(stack).to(haveResourceLike("AWS::EC2::Route", {
        DestinationIpv6CidrBlock: '2001:4860:4860::8888/32',
        NetworkInterfaceId: 'router-1'
      }));

      test.done();
    },

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[ialford]

Hi @ialford,

{} represents an object with no properties, but you seem to be using it as a kind of "wildcard" property, which it does not actually represent. The test is currently testing that a resource with invalid properties is NOT created. It succeeds, but not because of the code you changed.

You can check whether a test is a good test by running it WITHOUT your proposed code change (in which case it should fail) and again WITH your proposed code change (in which case it should succeed). By doing that, you know the test is actually testing the code change and not just passing by accident.

I would propose changing it to the following:

    'Can add an IPv6 route'(test: Test) {
      // GIVEN
      const stack = getTestStack();

      // WHEN
      const vpc = new Vpc(stack, 'VPC');
      (vpc.publicSubnets[0] as PublicSubnet).addRoute('SomeRoute', {
        destinationIpv6CidrBlock: '2001:4860:4860::8888/32',
        routerId: 'router-1',
        routerType: RouterType.NETWORK_INTERFACE
      });

      // THEN

      expect(stack).to(haveResourceLike("AWS::EC2::Route", {
        DestinationIpv6CidrBlock: '2001:4860:4860::8888/32',
        NetworkInterfaceId: 'router-1'
      }));

      test.done();
    },

Hi @rix0rrr - I added the test that you suggested as well as created a test for explicitly adding an IPv4 Route. I also moved them to run under the VPC suite of tests, since that is where the rest of the subnet-related tests run.

I removed the test I originally submitted as well - I believe that the error handling here handles the logic that I was testing originally.

[rix0rrr]

Looks good, thanks for submitting!

[mergify]

Thank you for contributing! Your pull request is now being automatically merged.

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

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