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

pccowboy · 2017-11-21T00:52:53Z

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         }

We are still verifying that this is working for our installation, hence the WIP tag. We are able to create the same-region replica reliably (after push c84c16d), but we are getting a destroy-add on the next plan for the replica. We are tracking that down currently.

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. I have not yet run the tests, however.

the replica

Add test for same-region replicas, verify that source_region is not set.

aws client bits instead.

pccowboy · 2017-11-27T17:54:59Z

@Ninir Can you take a look at this latest, and tell me if it looks like a valid fix for #2399 ? If it looks like a fix y'all might take, we'll test it out, since testing it costs some money.

Otherwise, any pointers on producing a fix? This is not blocking us, but it did make us create a module specific to same-region replicas, which seems less than ideal.

pccowboy · 2017-12-11T18:22:13Z

@Ninir ping? We are using this fix currently, and it seems to be working. Any feedback y'all have on the current PR would be appreciated.

pccowboy · 2017-12-15T22:30:45Z

@radeksimko @Ninir Hello, can we get your feedback on this PR, please?

the replica

Add test for same-region replicas, verify that source_region is not set.

aws client bits instead.

jen20 · 2017-12-22T18:50:39Z

aws/resource_aws_db_instance.go

@@ -407,11 +437,11 @@ func resourceAwsDbInstanceCreate(d *schema.ResourceData, meta interface{}) error
 			opts.DBSubnetGroupName = aws.String(attr.(string))
 		}

+		// TODO: Only allow this param if the master is not encrypted or


Is this still an outstanding concern?

@jen20 I wrote that based on the ambiguity in the AWS documentation I describe above. When creating a replica, they seem to have two situations where they allow an encryption key to be specified for a replica. When creating an encrypted replica in the same region as the master, you cannot specify the key.

I had thought that maybe we should show an error if a key was specified for a same-region replica.

However, maybe it is best to let the AWS API throw an error if this param is used when it is not allowed.

Unless you have advice on which way to go, I can take those lines out. The PR is working as-is for us.

jen20

I have a couple of questions and haven't tried running the tests yet, but overall this looks fairly solid. Thanks for the PR!

jen20 · 2017-12-22T18:51:04Z

aws/resource_aws_db_instance.go

@@ -399,6 +401,34 @@ func resourceAwsDbInstanceCreate(d *schema.ResourceData, meta interface{}) error
 			opts.AvailabilityZone = aws.String(attr.(string))
 		}

+		//
+		// If we are called with a Source DB ARN, and the ARN is a different region


👍 for the block comment explaining the rationale here.

pccowboy · 2018-02-26T21:05:16Z

@radeksimko Is there anything about this PR that is blocking it from being merged in? I am happy to make whatever adjustments are needed.

pccowboy · 2018-03-08T18:27:59Z

@radeksimko bump -anything I can do to move this through?

pccowboy · 2018-03-08T18:34:45Z

@radeksimko @jen20 Actually, I have a cleaner branch, where I removed all the duplicate commits. I think I am going to close this PR, and open a new one from that cleaner branch.

ghost · 2020-04-07T17:41:31Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

David Swift added 7 commits November 20, 2017 11:32

source-region should be set from the source DB ARN, not the replica ARN

fc9b9eb

Use the AWS ARN object, instead of string manipulation

90ab3b8

Add acceptance test for source_region on replicas.

bb7aea7

Correction for using an source DB ARN that is in the same region as

25c3f37

the replica

Make sure that source_region is not set in the prior use cases.

a2e822f

Add test for same-region replicas, verify that source_region is not set.

Remember that 'make test' still has value, even wlo the acceptance tests

55fa93e

availability_zone is an optional param, get the replica's region from

c84c16d

aws client bits instead.

pccowboy mentioned this pull request Nov 22, 2017

ReadReplicaSourceDBInstanceIdentifier gets changed when an encrypted replica is created in the same zone as the master #2399

Closed

Ninir added the bug Addresses a defect in current functionality. label Nov 22, 2017

David Swift added 3 commits November 27, 2017 09:44

Possible fix for terraform-provider-aws/issues/2399

f4ad1c6

ugh, correct var/package naming conflict

20c3f8d

Correct unused var from CNP

e0a390f

gah! remove the line I intended to replace - I blame monday.

691a18b

pccowboy changed the title ~~[WIP] Allow ARN for the replicate_source_db when in the same region~~ Allow ARN for the replicate_source_db when in the same region Dec 15, 2017

David Swift and others added 13 commits December 18, 2017 11:22

source-region should be set from the source DB ARN, not the replica ARN

0cd9a52

Use the AWS ARN object, instead of string manipulation

bcb2e50

Add acceptance test for source_region on replicas.

c558651

Correction for using an source DB ARN that is in the same region as

bafedcd

the replica

Make sure that source_region is not set in the prior use cases.

86e6321

Add test for same-region replicas, verify that source_region is not set.

Remember that 'make test' still has value, even wlo the acceptance tests

ccc6f0b

availability_zone is an optional param, get the replica's region from

4b34d4e

aws client bits instead.

Possible fix for terraform-provider-aws/issues/2399

bd9d42c

ugh, correct var/package naming conflict

9919c30

Correct unused var from CNP

0dd23da

gah! remove the line I intended to replace - I blame monday.

65737f1

Correct typo

06a8353

Merge branch 'master' into bug-correct-source-region

5b0b146

Resolve merge conflict from typo fix - ????

038f0f3

jen20 reviewed Dec 22, 2017

View reviewed changes

radeksimko added the service/rds Issues and PRs that pertain to the rds service. label Jan 16, 2018

radeksimko changed the title ~~Allow ARN for the replicate_source_db when in the same region~~ resource/aws_db_instance: Allow ARN for the replicate_source_db when in the same region Jan 16, 2018

pccowboy mentioned this pull request Feb 9, 2018

Integration/1.8.0 ValiMail/terraform-provider-aws#1

Closed

pccowboy closed this Mar 9, 2018

pccowboy mentioned this pull request Mar 9, 2018

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

Open

ghost locked and limited conversation to collaborators Apr 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

pccowboy commented Nov 21, 2017 •

edited

Loading

pccowboy commented Nov 27, 2017

pccowboy commented Dec 11, 2017

pccowboy commented Dec 15, 2017

jen20 Dec 22, 2017

pccowboy Dec 26, 2017

jen20 left a comment

jen20 Dec 22, 2017

pccowboy commented Feb 26, 2018

pccowboy commented Mar 8, 2018

pccowboy commented Mar 8, 2018

ghost commented Apr 7, 2020

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

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

Conversation

pccowboy commented Nov 21, 2017 • edited Loading

pccowboy commented Nov 27, 2017

pccowboy commented Dec 11, 2017

pccowboy commented Dec 15, 2017

jen20 Dec 22, 2017

Choose a reason for hiding this comment

pccowboy Dec 26, 2017

Choose a reason for hiding this comment

jen20 left a comment

Choose a reason for hiding this comment

jen20 Dec 22, 2017

Choose a reason for hiding this comment

pccowboy commented Feb 26, 2018

pccowboy commented Mar 8, 2018

pccowboy commented Mar 8, 2018

ghost commented Apr 7, 2020

pccowboy commented Nov 21, 2017 •

edited

Loading