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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 25 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
fc9b9eb
source-region should be set from the source DB ARN, not the replica ARN
Nov 20, 2017
90ab3b8
Use the AWS ARN object, instead of string manipulation
Nov 20, 2017
bb7aea7
Add acceptance test for source_region on replicas.
Nov 20, 2017
25c3f37
Correction for using an source DB ARN that is in the same region as
Nov 21, 2017
a2e822f
Make sure that source_region is not set in the prior use cases.
Nov 21, 2017
55fa93e
Remember that 'make test' still has value, even wlo the acceptance tests
Nov 21, 2017
c84c16d
availability_zone is an optional param, get the replica's region from
Nov 21, 2017
f4ad1c6
Possible fix for terraform-provider-aws/issues/2399
Nov 27, 2017
20c3f8d
ugh, correct var/package naming conflict
Nov 27, 2017
e0a390f
Correct unused var from CNP
Nov 27, 2017
691a18b
gah! remove the line I intended to replace - I blame monday.
Nov 27, 2017
0cd9a52
source-region should be set from the source DB ARN, not the replica ARN
Nov 20, 2017
bcb2e50
Use the AWS ARN object, instead of string manipulation
Nov 20, 2017
c558651
Add acceptance test for source_region on replicas.
Nov 20, 2017
bafedcd
Correction for using an source DB ARN that is in the same region as
Nov 21, 2017
86e6321
Make sure that source_region is not set in the prior use cases.
Nov 21, 2017
ccc6f0b
Remember that 'make test' still has value, even wlo the acceptance tests
Nov 21, 2017
4b34d4e
availability_zone is an optional param, get the replica's region from
Nov 21, 2017
bd9d42c
Possible fix for terraform-provider-aws/issues/2399
Nov 27, 2017
9919c30
ugh, correct var/package naming conflict
Nov 27, 2017
0dd23da
Correct unused var from CNP
Nov 27, 2017
65737f1
gah! remove the line I intended to replace - I blame monday.
Nov 27, 2017
06a8353
Correct typo
Nov 30, 2017
5b0b146
Merge branch 'master' into bug-correct-source-region
pccowboy Dec 19, 2017
038f0f3
Resolve merge conflict from typo fix - ????
Dec 19, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 44 additions & 8 deletions aws/resource_aws_db_instance.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"time"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/aws/arn"
"github.com/aws/aws-sdk-go/aws/awserr"
"github.com/aws/aws-sdk-go/service/rds"

Expand Down Expand Up @@ -387,6 +388,7 @@ func resourceAwsDbInstanceCreate(d *schema.ResourceData, meta interface{}) error
PubliclyAccessible: aws.Bool(d.Get("publicly_accessible").(bool)),
Tags: tags,
}

if attr, ok := d.GetOk("iops"); ok {
opts.Iops = aws.Int64(int64(attr.(int)))
}
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 for the block comment explaining the rationale here.

// than the replica to be created, set SourceRegion.
//
// The correct way to do this would be to query the master, and see if it
// is encrypted and in the same region. If it is encrypted and in the
// same region, drop the source region and the kms_key_id. If the master is not
// encrypted, behavior is kinda undefined.
//
// The CLI docs for kms_key_id state:
// "If you specify this parameter when you create a Read Replica from an
// unencrypted DB instance, the Read Replica is encrypted.""
//
// The RDS userguide states:
// "You cannot have an encrypted Read Replica of an unencrypted DB instance
// or an unencrypted Read Replica of an encrypted DB instance."
//
// go figure, eh?
//
replicaRegion := meta.(*AWSClient).region

arnParts, arnErr := arn.Parse(d.Get("replicate_source_db").(string))
if arnErr == nil {
if arnParts.Region != replicaRegion {
opts.SourceRegion = aws.String(arnParts.Region)
}
}

if attr, ok := d.GetOk("storage_type"); ok {
opts.StorageType = aws.String(attr.(string))
}
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still an outstanding concern?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// is in a different region than the replica

if attr, ok := d.GetOk("kms_key_id"); ok {
opts.KmsKeyId = aws.String(attr.(string))
if arnParts := strings.Split(v.(string), ":"); len(arnParts) >= 4 {
opts.SourceRegion = aws.String(arnParts[3])
}
}

if attr, ok := d.GetOk("monitoring_role_arn"); ok {
Expand Down Expand Up @@ -777,21 +807,21 @@ func resourceAwsDbInstanceRead(d *schema.ResourceData, meta interface{}) error {
// list tags for resource
// set tags
conn := meta.(*AWSClient).rdsconn
arn, err := buildRDSARN(d.Id(), meta.(*AWSClient).partition, meta.(*AWSClient).accountid, meta.(*AWSClient).region)
builtArn, err := buildRDSARN(d.Id(), meta.(*AWSClient).partition, meta.(*AWSClient).accountid, meta.(*AWSClient).region)
if err != nil {
name := "<empty>"
if v.DBName != nil && *v.DBName != "" {
name = *v.DBName
}
log.Printf("[DEBUG] Error building ARN for DB Instance, not setting Tags for DB %s", name)
} else {
d.Set("arn", arn)
d.Set("arn", builtArn)
resp, err := conn.ListTagsForResource(&rds.ListTagsForResourceInput{
ResourceName: aws.String(arn),
ResourceName: aws.String(builtArn),
})

if err != nil {
log.Printf("[DEBUG] Error retrieving tags for ARN: %s", arn)
log.Printf("[DEBUG] Error retrieving tags for ARN: %s", builtArn)
}

var dt []*rds.Tag
Expand Down Expand Up @@ -828,7 +858,13 @@ func resourceAwsDbInstanceRead(d *schema.ResourceData, meta interface{}) error {
return fmt.Errorf("[DEBUG] Error setting replicas attribute: %#v, error: %#v", replicas, err)
}

d.Set("replicate_source_db", v.ReadReplicaSourceDBInstanceIdentifier)
// If an ARN was passed in, do NOT use what AWS passes back for replicate_source_id,
// as it passes back the master's ID-
// see https://github.com/terraform-providers/terraform-provider-aws/issues/2399
_, arnErr := arn.Parse(d.Get("replicate_source_db").(string))
if arnErr != nil {
d.Set("replicate_source_db", v.ReadReplicaSourceDBInstanceIdentifier)
}

d.Set("ca_cert_identifier", v.CACertificateIdentifier)

Expand Down
146 changes: 145 additions & 1 deletion aws/resource_aws_db_instance_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,14 +266,80 @@ func TestAccAWSDBInstance_replica(t *testing.T) {
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSDBInstanceExists("aws_db_instance.bar", &s),
testAccCheckAWSDBInstanceExists("aws_db_instance.replica", &r),
resource.TestCheckNoResourceAttr("aws_db_instance.replica", "source_region"),
testAccCheckAWSDBInstanceReplicaAttributes(&s, &r),
),
},
},
})
}

func TestAccAWSDBInstance_noSnapshot(t *testing.T) {
func TestAccAWSDBInstanceReplicaSameRegionWithArn(t *testing.T) {
var s, r rds.DBInstance
var id int

region := "us-east-1"
id = rand.New(rand.NewSource(time.Now().UnixNano())).Int()

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckAWSDBInstanceDestroy,
Steps: []resource.TestStep{
{
Config: testAccReplicaInstanceSameRegionWithArnConfig(
id,
fmt.Sprintf("arn:aws:rds:%s:%s:db:foobarbaz-test-terraform-%v",
region,
"123456789012",
id,
),
),
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSDBInstanceExists("aws_db_instance.bar", &s),
testAccCheckAWSDBInstanceExists("aws_db_instance.replica", &r),
resource.TestCheckNoResourceAttr("aws_db_instance.replica", "source_region"),
testAccCheckAWSDBInstanceReplicaAttributes(&s, &r),
),
},
},
})
}

func TestAccAWSDBInstanceReplicaCrossRegion(t *testing.T) {
var s, r rds.DBInstance
var id int

region := "us-east-1"
id = rand.New(rand.NewSource(time.Now().UnixNano())).Int()

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckAWSDBInstanceDestroy,
Steps: []resource.TestStep{
{
Config: testAccReplicaInstanceCrossRegionConfig(
id,
fmt.Sprintf("arn:aws:rds:%s:%s:db:foobarbaz-test-terraform-%v",
region,
"123456789012",
id,
),
),
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSDBInstanceExists("aws_db_instance.bar", &s),
testAccCheckAWSDBInstanceExists("aws_db_instance.replica", &r),
testAccCheckAWSDBInstanceReplicaAttributes(&s, &r),
resource.TestCheckResourceAttr(
"aws_db_instance.replica", "source_region", region),
),
},
},
})
}

func TestAccAWSDBInstanceNoSnapshot(t *testing.T) {
var snap rds.DBInstance

resource.Test(t, resource.TestCase{
Expand Down Expand Up @@ -931,6 +997,84 @@ func testAccReplicaInstanceConfig(val int) string {
`, val, val)
}

func testAccReplicaInstanceSameRegionWithArnConfig(val int, arn string) string {
return fmt.Sprintf(`
resource "aws_db_instance" "bar" {
identifier = "%s"

allocated_storage = 5
engine = "mysql"
engine_version = "5.6.35"
instance_class = "db.t2.micro"
name = "baz"
password = "barbarbarbar"
username = "foo"
availability_zone = "us-east-1a"

backup_retention_period = 1
skip_final_snapshot = true

parameter_group_name = "default.mysql5.6"
}

resource "aws_db_instance" "replica" {
identifier = "tf-replica-db-%d"
backup_retention_period = 0
availability_zone = "us-east-1a"
replicate_source_db = "${aws_db_instance.bar.identifier}"
allocated_storage = "${aws_db_instance.bar.allocated_storage}"
engine = "${aws_db_instance.bar.engine}"
engine_version = "${aws_db_instance.bar.engine_version}"
instance_class = "${aws_db_instance.bar.instance_class}"
password = "${aws_db_instance.bar.password}"
username = "${aws_db_instance.bar.username}"
skip_final_snapshot = true
tags {
Name = "tf-replica-db"
}
}
`, arn, val)
}

func testAccReplicaInstanceCrossRegionConfig(val int, arn string) string {
return fmt.Sprintf(`
resource "aws_db_instance" "bar" {
identifier = "%s"

allocated_storage = 5
engine = "mysql"
engine_version = "5.6.35"
instance_class = "db.t2.micro"
name = "baz"
password = "barbarbarbar"
username = "foo"
availability_zone = "us-east-1a"

backup_retention_period = 1
skip_final_snapshot = true

parameter_group_name = "default.mysql5.6"
}

resource "aws_db_instance" "replica" {
identifier = "tf-replica-db-%d"
backup_retention_period = 0
availability_zone = "us-east-2a"
replicate_source_db = "${aws_db_instance.bar.identifier}"
allocated_storage = "${aws_db_instance.bar.allocated_storage}"
engine = "${aws_db_instance.bar.engine}"
engine_version = "${aws_db_instance.bar.engine_version}"
instance_class = "${aws_db_instance.bar.instance_class}"
password = "${aws_db_instance.bar.password}"
username = "${aws_db_instance.bar.username}"
skip_final_snapshot = true
tags {
Name = "tf-replica-db"
}
}
`, arn, val)
}

func testAccSnapshotInstanceConfig() string {
return fmt.Sprintf(`
resource "aws_db_instance" "snapshot" {
Expand Down