Skip to content
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

resource/aws_db_instance: Allow ARN for the replicate_source_db when in the same region #3701

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pccowboy
Copy link

@pccowboy pccowboy commented Mar 9, 2018

I believe this addresses a possible bug we ran into that was triggered by https://github.com/terraform-providers/terraform-provider-aws/pull/865/files and mentioned at https://groups.google.com/forum/#!topic/terraform-tool/zX-bWBZgViI

If you pass both an ARN and a kms_key_id to create a replica in the same region as the master, you get an error of the form "* aws_db_instance.read_replica: Error creating DB Instance: InvalidParameterCombination: Your request does not require the preSignedUrl parameter. Please remove the preSignedUrl parameter and try your request again."

I think this is because of AWS code - in aws/aws-sdk-go/service/rds/customizations.go, a preSignedUrl will get added to the CreateDBInstanceReadReplicaInput if the SourceRegion is set for the replica. If SourceRegion is nil, the presignedURL is skipped.

     58         if originParams.SourceRegion == nil || originParams.PreSignedUrl != nil || originParams.DestinationRegion != nil {
     59                 return
     60         }

I've modified the test for creating a replica, and added two more, one for the replica created with a source ARN in the same region, and one for creating a replica from a source ARN in a different region.

This PR is a replacement for #2386 - due to a bad merge, that branch had a bunch of duplicate commits. This branch has been cleaned up to make a cleaner merge @jen20 @radeksimko

@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Mar 9, 2018
@pccowboy pccowboy force-pushed the feature/correct_source_region branch from 2b9234f to 73f7ba8 Compare March 12, 2018 22:54
@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Mar 12, 2018
@pccowboy
Copy link
Author

@radeksimko Ping? Do you have any information on when this PR might be able to get a review?

@bflad bflad added bug Addresses a defect in current functionality. service/rds Issues and PRs that pertain to the rds service. labels Mar 28, 2018
@pccowboy
Copy link
Author

pccowboy commented Apr 2, 2018

@bflad Thanks for checking in. Do you have any time to give this a review? I'd really like feedback on whether the tests I added are sufficient.

I'd like to get this into master, as it fixes a blocker for using encrypted read replicas in the same region when using ARNs.

@bflad
Copy link
Contributor

bflad commented Apr 2, 2018

I would prefer someone more familiar with this code/functionality to look at it before I commit myself to looking at it. There is quite a lot in my backlog already.

@pccowboy
Copy link
Author

pccowboy commented Apr 2, 2018

OK, that makes sense. Thanks!

@pccowboy pccowboy force-pushed the feature/correct_source_region branch from 73f7ba8 to 3204200 Compare April 10, 2018 20:56
@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Apr 10, 2018
@pccowboy pccowboy force-pushed the feature/correct_source_region branch from 3204200 to da125a1 Compare April 10, 2018 21:58
@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Apr 10, 2018
@pccowboy pccowboy force-pushed the feature/correct_source_region branch from da125a1 to d15b255 Compare April 13, 2018 23:18
@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Apr 13, 2018
@pccowboy pccowboy force-pushed the feature/correct_source_region branch from d15b255 to f0b063c Compare April 14, 2018 17:42
@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Apr 14, 2018
@pccowboy pccowboy force-pushed the feature/correct_source_region branch from f0b063c to e266b48 Compare April 23, 2018 21:23
@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Apr 23, 2018
@pccowboy
Copy link
Author

@radeksimko ping? Wanted to see about getting a review on this, so I have a chance to rework anything I need to.

@pccowboy
Copy link
Author

pccowboy commented May 9, 2018

@radeksimko Hello, this PR just passed its 6-month anniversary - Is there someone I can chat with about getting it reviewed further? @jen20 Gave some feedback on the first iteration of this PR, hoping for more so we can move it along. Thanks

@pccowboy pccowboy force-pushed the feature/correct_source_region branch from e266b48 to 61d0bb4 Compare May 14, 2018 20:30
@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label May 14, 2018
@petergoldstein
Copy link

👍

@pccowboy
Copy link
Author

@radeksimko Checking in again, do you have any suggestions as to someone who could review this PR?

@aeschright aeschright requested a review from a team June 25, 2019 19:24
@ghost ghost added the tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. label Jun 26, 2019
@petergoldstein
Copy link

Doing an incremental rebase to eliminate conflicts and ensure code still runs as expected - should be done by Thursday 6/27.

@ghost ghost added size/S Managed by automation to categorize the size of a PR. and removed size/L Managed by automation to categorize the size of a PR. labels Jun 27, 2019
Use the AWS ARN object, instead of string manipulation
Add acceptance test for source_region on replicas.
Add test for same-region replicas, verify that source_region is not set.
Get the replica's region from aws client instead.
Possible fix for terraform-provider-aws/issues/2399
Don't overwrite the 'arn' object with a local string variable.

TODO: Readd tests post-rebase.  This is an old PR and the test structure has changed signficantly since original submission.
Base automatically changed from master to main January 23, 2021 00:55
@breathingdust breathingdust requested a review from a team as a code owner January 23, 2021 00:55
@zhelding
Copy link
Contributor

Pull request #21306 has significantly refactored the AWS Provider codebase. As a result, most PRs opened prior to the refactor now have merge conflicts that must be resolved before proceeding.

Specifically, PR #21306 relocated the code for all AWS resources and data sources from a single aws directory to a large number of separate directories in internal/service, each corresponding to a particular AWS service. This separation of code has also allowed for us to simplify the names of underlying functions -- while still avoiding namespace collisions.

We recognize that many pull requests have been open for some time without yet being addressed by our maintainers. Therefore, we want to make it clear that resolving these conflicts in no way affects the prioritization of a particular pull request. Once a pull request has been prioritized for review, the necessary changes will be made by a maintainer -- either directly or in collaboration with the pull request author.

For a more complete description of this refactor, including examples of how old filepaths and function names correspond to their new counterparts: please refer to issue #20000.

For a quick guide on how to amend your pull request to resolve the merge conflicts resulting from this refactor and bring it in line with our new code patterns: please refer to our Service Package Refactor Pull Request Guide.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Addresses a defect in current functionality. service/rds Issues and PRs that pertain to the rds service. size/S Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants