fix(route53): vpc region in template overridden by stack region

[daschaa]

Fixes #20496

This PR implements the proposed change in #20496 - When a region is set in the vpc it is used in the CloudFormation template. Otherwise the region from the respective stack is used.

---

All Submissions:

• [x] Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

• [ ] This PR adds new unconventional dependencies following the process described here

New Features

• [x] Have you added the new feature to an integration test?
  • [x] Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

[gitpod-io[bot]]

[daschaa]

@peterwoodworth Since you commented on the original issue - could you review this PR please? I would appreciate the feedback! ❤️

[peterwoodworth]

Sure thing @daschaa, I'll try to get to this today 🙂

[daschaa]

@peterwoodworth I'm working on this since some time now and I don't really have an idea how I can properly test this.

1. Integ-Snapshot testing is not really good because Vpc.fromLookup can't find a VPC then
2. Jest tests with a mocked construct is not really good imho because it doesn't really test the functionality - just if it is passed correctly
3. I don't have an idea how to do it without mocking the ContextProvider (in a jest test)

Do you have an idea how I could do it properly? I think I need at least a "spark" that I can continue making progress on this one.

Sorry that I does take such a long time..

[peterwoodworth]

No worries @daschaa

it doesn't really test the functionality - just if it is passed correctly

Well, that is all we need to test here! Right? All we want to know is that the Vpc you create with fromLookup sets its .env.region prop to the one you pass in. Check out this test file - this is the file where you'll want to add the test and it has plenty of examples 🙂

Let me know if you need any more help!

[daschaa]

@peterwoodworth I can now continue working on this feature again. Sorry for the delay!

I have added the region prop to the fromLookup and fromVpcAttributes methods and used fromVpcAttributes in the test for the hosted zone.

However I'm not really happy about the typing of the LookedUpVpc. I extended the props-property with region and decided to not introduce an own type. I thought it would be overkill to just implement it for the region prop. Is it ok like that? I would be happy to get feedback from your side and maybe some better ideas about the typing. Thanks in advance!

[daschaa]

@peterwoodworth I just saw that I did not rebase this branch very well, but I think everything is fine. At least the changes still look good :) So please excuse me for this huge list of commits in this PR 😄

[TheRealAmazonKendra]

Rebasing gets weird sometimes ¯\_(ツ)_/¯

[TheRealAmazonKendra]

@merigifyio update

[daschaa]

@mergifyio update

[mergify[bot]]

update

✅ Branch has been successfully updated

[daschaa]

@TheRealAmazonKendra Is this mergify command new? Never saw this before and wondered if we should use it instead of the GitHub Update Branch command :)

[TheRealAmazonKendra]

@TheRealAmazonKendra Is this mergify command new? Never saw this before and wondered if we should use it instead of the GitHub Update Branch command :)

I use it because it doesn't dismiss previous reviews and will rerun workflows. I think I'm the only one who really uses the Mergify commands, though. Either way is fine.

[daschaa]

@TheRealAmazonKendra I have added your suggested changes. Thanks for the feedback! Regarding the integration tests: Do you mean Integration Tests for the fromLookup and fromVpcAttributes methods? I wonder if this would be easily possible since the snapshot would not do a real lookup, right? 🤔 Or do you mean for the addVpc method?

[TheRealAmazonKendra]

New/updated integration tests actually create the resources and deploy them. They just don't run in a public account so you never see it happen. We also will see from the synthesized template that the region assigned to the VPC will be different from the one you assign for the stack. If you create the test using the IntegTest construct and then run yarn integ --update-on-failed <filename>.js then it will setup the resources, tear them back down, and produce the expected snapshot files to compare against later. Check out our new documentation on integration tests for more info.

[TheRealAmazonKendra]

@TheRealAmazonKendra I have added your suggested changes. Thanks for the feedback! Regarding the integration tests: Do you mean Integration Tests for the fromLookup and fromVpcAttributes methods? I wonder if this would be easily possible since the snapshot would not do a real lookup, right? 🤔 Or do you mean for the addVpc method?

I just realized that I only kind of answered this question. I'd want to see integ tests on fromLookup and fromVpcAttributes. How I see that working is to create two stacks in your test. One in region-a that creates the VPC and one in region-b that uses fromLookup and fromVpcAttributes to import it. Only list the stack that uses these functions in your testCases array.

Does that make sense?

[daschaa]

@TheRealAmazonKendra Sorry for not working on this consistently - I have a lot on my plate right now. I have now added the integration tests for fromLookup and fromVpcAttributes as you suggested and I moved the region prop to the VpcContextResponse. I look forward to your feedback :)

[TheRealAmazonKendra]

@TheRealAmazonKendra Sorry for not working on this consistently - I have a lot on my plate right now. I have now added the integration tests for fromLookup and fromVpcAttributes as you suggested and I moved the region prop to the VpcContextResponse. I look forward to your feedback :)

No need to be sorry at all, you're contributing your time and resources to this and we're super grateful for it!

[TheRealAmazonKendra]

FYI - I haven't forgot this PR but I do think there's more refactoring that needs to be done so I'm taking a deeper dive into the code to make sure my suggestions aren't off base.

[daschaa]

@TheRealAmazonKendra Any updates on this one? :)

[TheRealAmazonKendra]

@Mergifyio update

[mergify[bot]]

update

✅ Branch has been successfully updated

[daschaa]

@Mergifyio update

[mergify[bot]]

update

✅ Branch has been successfully updated

[mergify[bot]]

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

[aws-cdk-automation]

AWS CodeBuild CI Report

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

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

[mergify[bot]]

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

[michaelciccarelli]

Can anyone follow up on what I'm seeing? I'm running 2.44.0 and this doesn't seem like it's working yet:

   if env.region == "us-east-1" and not self.bcp_env:
        useast2vpcid = os.getenv("VPC_ID_US_EAST_2", "MISSING__VPC_ID_US_EAST_2_FOR_HIGHAVAILABILITY")
        useast2vpc = Vpc.from_lookup(self, "vpcLookup-useast2", vpc_id=useast2vpcid, region="us-east-2")
        my_hosted_zone = route53.HostedZone(self, APP_NAME + "-route53",
            zone_name = subdomain + ".mydomain.us",
            vpcs = [ self.vpc,  useast2vpc  ]
        )

When I run it, the synth is fine but apply shows that it CDKformation doesn't have permission to add this vpc.. the issue is the vpc is showing up as in us-east-1 even though we're specifying us-east-2.. The following code fixes the issue with calling Cfn directly....

   if env.region == "us-east-1" and not self.bcp_env:
        useast2vpcid = os.getenv("VPC_ID_US_EAST_2", "MISSING__VPC_ID_US_EAST_2_FOR_HIGHAVAILABILITY")

        my_hosted_zone = route53.CfnHostedZone(self, APP_NAME + "-route53",
            name = subdomain + ".mydomain.us",
            vpcs = [
                route53.CfnHostedZone.VPCProperty(vpc_id=self.vpc_id, vpc_region = env.region),
                route53.CfnHostedZone.VPCProperty(vpc_id=useast2vpcid, vpc_region = "us-east-2")
            ]
        )