-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(route53): vpc region in template overridden by stack region #20530
Conversation
@peterwoodworth Since you commented on the original issue - could you review this PR please? I would appreciate the feedback! ❤️ |
Sure thing @daschaa, I'll try to get to this today 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution @daschaa!
This is going to be a slightly larger PR than I had anticipated. We need to add some additional functionality elsewhere for this to work.
First, let's take a look at how our S3 module handles this:
Bucket.fromBucketAttributes()
has a region
prop the user can pass in for when the region of the stack and bucket differ. This ends up being represented to the user through IBucket.env.region
aws-cdk/packages/@aws-cdk/aws-s3/lib/bucket.ts
Lines 1588 to 1591 in ab28878
return new Import(scope, id, { | |
account: attrs.account, | |
region: attrs.region, | |
}); |
Vpc.fromVpcAttributes()
and Vpc.fromLookup()
should both support this. However, neither of these methods currently support this. The former doesn't allow you to set the region at all, and the latter does, but it isn't set when the construct is created. Both of these functionalities will need to be implemented 🙂
As for tests, let's create tests in the EC2 library to verify that the region is properly set with these methods. Additionally, for the hostedzone test, could we use one of these import methods instead of a mock construct?
This is required for now because of a CDK bug: aws/aws-cdk#20496 The PR for the above bug is: aws/aws-cdk#20530 Fix use of existing hosted zone.
@peterwoodworth I'm working on this since some time now and I don't really have an idea how I can properly test this.
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.. |
No worries @daschaa
Well, that is all we need to test here! Right? All we want to know is that the Vpc you create with Let me know if you need any more help! |
This is required for now because of a CDK bug: aws/aws-cdk#20496 The PR for the above bug is: aws/aws-cdk#20530 Fix use of existing hosted zone.
This is required for now because of a CDK bug: aws/aws-cdk#20496 The PR for the above bug is: aws/aws-cdk#20530 Fix use of existing hosted zone.
Add Regions and AZs to the InstanceConfig The code has been updated support multiple regions. The instance types that are available and the pricing varies by region so all instance type info must be maintained by region. Spot pricing additionally varies by instance type and by AZ and this commit adds an updated EC2InstanceTypeInfoPkg package that looks up the spot pricing for each instance type in each AZ and region. The Region/AZ configuration is added to the InstanceConfig section of the config file. The region requires the VpcId, CIDR, and SshKeyPair. The AZ requires the subnet ID and priority. The slurm node configuration has been updated to add the AZ id to all compute nodes and add the AZ name to all partitions. Users can specify multiple partitions with sbatch if they want jobs to be spread across multiple AZs. The modulefile has been updated to set the partition to the list of all regional/az partitions so that all nodes are available to the jobs in the priority configured in the config file. Create compute node security groups for other regions using a custom resource. Save regional security group ids in ssm parameter store. Update multi-region route53 hosted zone Fix IAM permissions to handle multiple regions Decode iam permissions messsages Update security groups with remote region cidrs Create slurmfs ARecord for use in other regions. This required adding a lambda to do DNS lookups. Add custom resource to add regional VPCs to the Route53 hosted zone. This is required for now because of a CDK bug: aws/aws-cdk#20496 The PR for the above bug is: aws/aws-cdk#20530 Update github-pages to use mkdocs Add github-docs target to Makefile Update to cdk@2.28.1 Create AZ and interactive partitions, set default partitions Resolves [FEATURE #22: Support mutiple availability zones and regions](#2)
* chore: replace master with main wherever possible * update amplify tests * fix iconurl * monocdk main Co-authored-by: Kendra Neil <53584728+TheRealAmazonKendra@users.noreply.github.com>
Fixes aws#20546 Refreshes the links in the AWS Glue docs pointing to the Apache Hive website (for more information see the issue aws#20546 ) ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)? * [ ] 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*
…s#20581) Now that v1 is in maintenance mode, we need some updates to the contributing guide. The biggest change to the developer workflow now that v1 is in maintenance mode, is that the main development branch, `main`, has the v2 source code. This does not have many practical impacts on developer workflow, because in most cases we will continue to just iterate on a single service module in its `packages/@aws-cdk/aws-<service>` directory. This PR adds instructions for building and testing `aws-cdk-lib` and the individual `-alpha` packages, which developers might need to do occasionally. closes https://github.com/cdklabs/cdk-ops/issues/1933 closes aws#20041 ---- ### All Submissions: * [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)? * [ ] 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*
…20592) This is an automatic backport of pull request aws#20587 done by [Mergify](https://mergify.com). --- <details> <summary>Mergify commands and options</summary> <br /> More conditions and actions can be found in the [documentation](https://docs.mergify.com/). You can also trigger Mergify actions by commenting on this pull request: - `@Mergifyio refresh` will re-evaluate the rules - `@Mergifyio rebase` will rebase this PR on its base branch - `@Mergifyio update` will merge the base branch into this PR - `@Mergifyio backport <destination>` will backport this PR on `<destination>` branch Additionally, on Mergify [dashboard](https://dashboard.mergify.com/) you can: - look at your merge queues - generate the Mergify configuration with the config editor. Finally, you can contact us on https://mergify.com </details>
…ws#20608) When you add a property override, we merge it with any properties set when the construct is initialized. The merge happens _after_ tokens are resolved. For example, if we created a resource: ```ts const resource = new CfnResource(this, 'Resource', { type: 'MyResourceType', properties: { prop1: Fn.ref('Param'), }, }); ``` And the added an override for `prop1`: ```ts resource.addPropertyOverride('prop1', Fn.ref('OtherParam')); ``` We would then perform a deep merge on the properties with a priority on the overrides. The above example would work fine, eventually the merge would get to the point where it was comparing two objects: ``` target = { prop1: { Ref: 'Param' } } source = { prop1: { Ref: 'OtherParam' } } result = { prop1: { Ref: 'OtherParam' } } ``` If we instead added an override using a different intrinsic: ```ts resource.addPropertyOverride('prop1', Fn.join('-', ['hello', Fn.ref('Param')])); ``` Then we would get to the point in the merge where we were comparing two different objects: ``` target = { prop1: { Ref: 'Param' } } source = { prop1: { 'Fn::Join': ['-', ['hello', { Ref: 'Param' } ]] } } result = { prop1: { Ref: 'Param', 'Fn::Join': ['-', ['hello', { Ref: 'Param' } ]] }, }} ``` This PR solves this issue by filtering out any CloudFormation intrinsics. If the merge finds an intrinsic key in the target it "drops" the intrinsic and takes whatever value is in the source (override). fix aws#19971, aws#19447 ---- ### All Submissions: * [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] 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*
…_14_X (aws#20595) Spiritual successor of aws#20531 <img width="487" alt="Screen Shot 2022-06-01 at 9 08 18 PM" src="https://user-images.githubusercontent.com/31543/171552028-7153063a-1c19-4972-ab89-6f280a4e9326.png"> - Migrated away (tests, docs, internal use) from NODEJS_10_X to NODEJS_14_X - Migrated away (tests, docs, internal use) from NODEJS_12_X to NODEJS_14_X - Added NODEJS_16_X to CustomResources - Update CustomResources to use nodejs14.x by default - Updated integration tests across the board. Some integ tests introduced destructive changes (expected with change of nodejs runtime) Closes aws#20568 Closes aws#19992 Closes aws#20474 ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [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*
fixes aws#14279. Introduces optional property to the ReportGroup to define the type of the report group (either 'TEST' or 'CODE_COVERAGE'). It just passes down the property to the underlying CfnReportGroup. ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [x] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)? * [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*
…lambda-layer-awscli (aws#20629) Bumps [awscli](https://github.com/aws/aws-cli) from 1.25.1 to 1.25.2. <details> <summary>Changelog</summary> <p><em>Sourced from <a href="https://github.com/aws/aws-cli/blob/develop/CHANGELOG.rst">awscli's changelog</a>.</em></p> <blockquote> <h1>1.25.2</h1> <ul> <li>api-change:<code>connect</code>: This release adds the following features: 1) New APIs to manage (create, list, update) task template resources, 2) Updates to startTaskContact API to support task templates, and 3) new TransferContact API to programmatically transfer in-progress tasks via a contact flow.</li> <li>api-change:<code>proton</code>: Add new "Components" API to enable users to Create, Delete and Update AWS Proton components.</li> <li>api-change:<code>codeartifact</code>: Documentation updates for CodeArtifact</li> <li>api-change:<code>application-insights</code>: Provide Account Level onboarding support through CFN/CLI</li> <li>api-change:<code>kendra</code>: Amazon Kendra now provides a data source connector for GitHub. For more information, see <a href="https://docs.aws.amazon.com/kendra/latest/dg/data-source-github.html">https://docs.aws.amazon.com/kendra/latest/dg/data-source-github.html</a></li> <li>api-change:<code>voice-id</code>: Added a new attribute ServerSideEncryptionUpdateDetails to Domain and DomainSummary.</li> </ul> </blockquote> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/aws/aws-cli/commit/e25332ebfec94ca597ba0705ce815b197f78e2c8"><code>e25332e</code></a> Merge branch 'release-1.25.2'</li> <li><a href="https://github.com/aws/aws-cli/commit/71fa2b7b96d3234928c00727f1b2c8469915c7e9"><code>71fa2b7</code></a> Bumping version to 1.25.2</li> <li><a href="https://github.com/aws/aws-cli/commit/3c98094fbaf1ffd909441bf034acba3610795618"><code>3c98094</code></a> Update changelog based on model updates</li> <li><a href="https://github.com/aws/aws-cli/commit/dc7c26849d3f518229716fef8a211aad793a73c6"><code>dc7c268</code></a> Merge pull request <a href="https://github-redirect.dependabot.com/aws/aws-cli/issues/6045">#6045</a> from pawanakhil/feature/cloudsearch/define-index-fie...</li> <li><a href="https://github.com/aws/aws-cli/commit/ccbe2a477671585dcb62f75d1b99cfbdc4ac8747"><code>ccbe2a4</code></a> Updated test with Source field field option</li> <li><a href="https://github.com/aws/aws-cli/commit/e69993188e111fa3c9c5551657a67dbaeaaa2990"><code>e699931</code></a> Added source field option for CloudSearch define-index-field</li> <li><a href="https://github.com/aws/aws-cli/commit/c71d1dfccb5f340d8a117929b428f4b092cad934"><code>c71d1df</code></a> Merge branch 'release-1.25.1' into develop</li> <li>See full diff in <a href="https://github.com/aws/aws-cli/compare/1.25.1...1.25.2">compare view</a></li> </ul> </details> <br /> [![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=awscli&package-manager=pip&previous-version=1.25.1&new-version=1.25.2)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details>
With this change, the `Vpc` construct gains a new constructor prop, `availabilityZones`, which gives more control over AZs than the existing `maxAzs` prop. closes aws#5847 ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)? * [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*
@TheRealAmazonKendra I have added your suggested changes. Thanks for the feedback! |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought I commented this earlier but I can't find it anywhere. I tend to think that region should be added to VpcContextResponse
and added to attributes
more organically that way, instead of through the additional prop.
I just realized that I only kind of answered this question. I'd want to see integ tests on Does that make sense? |
Pull request has been modified.
@TheRealAmazonKendra Sorry for not working on this consistently - I have a lot on my plate right now. |
No need to be sorry at all, you're contributing your time and resources to this and we're super grateful for it! |
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. |
@TheRealAmazonKendra Any updates on this one? :) |
@Mergifyio update |
✅ Branch has been successfully updated |
@Mergifyio update |
Pull request has been modified.
✅ Branch has been successfully updated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all your work on this and patience in the long delay between feedback. This looks great!
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 CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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). |
…20530) Fixes aws#20496 This PR implements the proposed change in aws#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?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)? * [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*
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:
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....
|
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:
Adding new Unconventional Dependencies:
New Features
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