From f60e13efa80a1d6c198a82151784b94985de8b8f Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Mon, 20 Aug 2018 12:29:14 -0400 Subject: [PATCH] resource/aws_db_instance: Prevent error when using snapshot_identifier with multi_az enabled and sqlserver engine When calling `RestoreDBInstanceFromDBSnapshot` and attempting to enable `MultiAZ` with `Engine` set to SQL server, it first must be set to disabled since the API requires `BackupRetentionPeriod`, but that parameter is not available for the initial API call. * When using `multi_az` with `snapshot_identifier` and `engine` set to `sqlserver*`, remove parameter or catch specific error and require `ModifyDBInstance` API call afterwards * When `ModifyDBInstance` is being called during resource creation, always set `ApplyImmediately` to `true` to prevent need for double apply. Previously: ``` --- FAIL: TestAccAWSDBInstance_SnapshotIdentifier_MultiAZ_SQLServer (1101.33s) testing.go:527: Step 0 error: Error applying: 1 error occurred: * aws_db_instance.test: 1 error occurred: * aws_db_instance.test: Error creating DB Instance: InvalidParameterValue: Mirroring cannot be applied to instances with backup retention set to zero. ``` Now: ``` --- PASS: TestAccAWSDBInstance_SnapshotIdentifier_MultiAZ_SQLServer (4031.24s) ``` --- aws/resource_aws_db_instance.go | 82 ++++++++++++++------ aws/resource_aws_db_instance_test.go | 111 +++++++++++++++++++++++++++ 2 files changed, 169 insertions(+), 24 deletions(-) diff --git a/aws/resource_aws_db_instance.go b/aws/resource_aws_db_instance.go index fbf599b36d5..d82e82ad7ea 100644 --- a/aws/resource_aws_db_instance.go +++ b/aws/resource_aws_db_instance.go @@ -665,6 +665,12 @@ func resourceAwsDbInstanceCreate(d *schema.ResourceData, meta interface{}) error return resourceAwsDbInstanceRead(d, meta) } else if _, ok := d.GetOk("snapshot_identifier"); ok { + // RestoreDBInstanceFromDBSnapshot does not support all parameters + // correctly apply in one pass. For missing parameters or unsupported + // configurations, we need to call ModifyDBInstance afterwards to + // prevent Terraform operators from API errors or double apply. + var requiresModifyDbInstance bool + opts := rds.RestoreDBInstanceFromDBSnapshotInput{ DBInstanceClass: aws.String(d.Get("instance_class").(string)), DBInstanceIdentifier: aws.String(d.Get("identifier").(string)), @@ -711,7 +717,17 @@ func resourceAwsDbInstanceCreate(d *schema.ResourceData, meta interface{}) error } if attr, ok := d.GetOk("multi_az"); ok { - opts.MultiAZ = aws.Bool(attr.(bool)) + // When using SQL Server engine with MultiAZ enabled, its not + // possible to immediately enable mirroring since + // BackupRetentionPeriod is not available as a parameter to + // RestoreDBInstanceFromDBSnapshot and you receive an error. e.g. + // InvalidParameterValue: Mirroring cannot be applied to instances with backup retention set to zero. + // If we know the engine, prevent the error upfront. + if v, ok := d.GetOk("engine"); ok && strings.HasPrefix(strings.ToLower(v.(string)), "sqlserver") { + requiresModifyDbInstance = true + } else { + opts.MultiAZ = aws.Bool(attr.(bool)) + } } if attr, ok := d.GetOk("option_group_name"); ok { @@ -719,46 +735,56 @@ func resourceAwsDbInstanceCreate(d *schema.ResourceData, meta interface{}) error } + if _, ok := d.GetOk("password"); ok { + requiresModifyDbInstance = true + } + if attr, ok := d.GetOk("port"); ok { opts.Port = aws.Int64(int64(attr.(int))) } - if attr, ok := d.GetOk("tde_credential_arn"); ok { - opts.TdeCredentialArn = aws.String(attr.(string)) + + if attr := d.Get("security_group_names").(*schema.Set); attr.Len() > 0 { + requiresModifyDbInstance = true } if attr, ok := d.GetOk("storage_type"); ok { opts.StorageType = aws.String(attr.(string)) } - log.Printf("[DEBUG] DB Instance restore from snapshot configuration: %s", opts) - _, err := conn.RestoreDBInstanceFromDBSnapshot(&opts) - if err != nil { - return fmt.Errorf("Error creating DB Instance: %s", err) + if attr, ok := d.GetOk("tde_credential_arn"); ok { + opts.TdeCredentialArn = aws.String(attr.(string)) } - var sgUpdate bool - var passwordUpdate bool - - if _, ok := d.GetOk("password"); ok { - passwordUpdate = true + if attr := d.Get("vpc_security_group_ids").(*schema.Set); attr.Len() > 0 { + requiresModifyDbInstance = true } - if attr := d.Get("vpc_security_group_ids").(*schema.Set); attr.Len() > 0 { - sgUpdate = true + log.Printf("[DEBUG] DB Instance restore from snapshot configuration: %s", opts) + _, err := conn.RestoreDBInstanceFromDBSnapshot(&opts) + + // When using SQL Server engine with MultiAZ enabled, its not + // possible to immediately enable mirroring since + // BackupRetentionPeriod is not available as a parameter to + // RestoreDBInstanceFromDBSnapshot and you receive an error. e.g. + // InvalidParameterValue: Mirroring cannot be applied to instances with backup retention set to zero. + // Since engine is not a required argument when using snapshot_identifier + // and the RDS API determines this condition, we catch the error + // and remove the invalid configuration for it to be fixed afterwards. + if isAWSErr(err, "InvalidParameterValue", "Mirroring cannot be applied to instances with backup retention set to zero") { + opts.MultiAZ = aws.Bool(false) + requiresModifyDbInstance = true + _, err = conn.RestoreDBInstanceFromDBSnapshot(&opts) } - if attr := d.Get("security_group_names").(*schema.Set); attr.Len() > 0 { - sgUpdate = true + + if err != nil { + return fmt.Errorf("Error creating DB Instance: %s", err) } - if sgUpdate || passwordUpdate { - log.Printf("[INFO] DB is restoring from snapshot with default security, but custom security should be set, will now update after snapshot is restored!") - // wait for instance to get up and then modify security + if requiresModifyDbInstance { d.SetId(d.Get("identifier").(string)) - log.Printf("[INFO] DB Instance ID: %s", d.Id()) - - log.Println( - "[INFO] Waiting for DB Instance to be available") + log.Printf("[INFO] DB Instance %q configuration requires ModifyDBInstance after RestoreDBInstanceFromDBSnapshot", d.Id()) + log.Printf("[INFO] Waiting for DB Instance %q to be available", d.Id()) stateConf := &resource.StateChangeConf{ Pending: resourceAwsDbInstanceCreatePendingStates, @@ -1123,9 +1149,17 @@ func resourceAwsDbInstanceUpdate(d *schema.ResourceData, meta interface{}) error ApplyImmediately: aws.Bool(d.Get("apply_immediately").(bool)), DBInstanceIdentifier: aws.String(d.Id()), } + + // ModifyDBInstance might be called during resource creation + // to fix unsupported configurations, e.g. missing parameters + // from RestoreDBInstanceFromDBSnapshot. In this case, we should + // always apply immediately. + if d.IsNewResource() { + req.ApplyImmediately = aws.Bool(true) + } d.SetPartial("apply_immediately") - if !d.Get("apply_immediately").(bool) { + if !aws.BoolValue(req.ApplyImmediately) { log.Println("[INFO] Only settings updating, instance changes will be applied in next maintenance window") } diff --git a/aws/resource_aws_db_instance_test.go b/aws/resource_aws_db_instance_test.go index 2ef3a0c0773..6ee847a7996 100644 --- a/aws/resource_aws_db_instance_test.go +++ b/aws/resource_aws_db_instance_test.go @@ -365,6 +365,60 @@ func TestAccAWSDBInstance_SnapshotIdentifier(t *testing.T) { }) } +func TestAccAWSDBInstance_SnapshotIdentifier_MultiAZ(t *testing.T) { + var dbInstance, sourceDbInstance rds.DBInstance + var dbSnapshot rds.DBSnapshot + + rName := acctest.RandomWithPrefix("tf-acc-test") + sourceDbResourceName := "aws_db_instance.source" + snapshotResourceName := "aws_db_snapshot.test" + resourceName := "aws_db_instance.test" + + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckAWSDBInstanceDestroy, + Steps: []resource.TestStep{ + { + Config: testAccAWSDBInstanceConfig_SnapshotIdentifier_MultiAZ(rName, true), + Check: resource.ComposeTestCheckFunc( + testAccCheckAWSDBInstanceExists(sourceDbResourceName, &sourceDbInstance), + testAccCheckDbSnapshotExists(snapshotResourceName, &dbSnapshot), + testAccCheckAWSDBInstanceExists(resourceName, &dbInstance), + resource.TestCheckResourceAttr(resourceName, "multi_az", "true"), + ), + }, + }, + }) +} + +func TestAccAWSDBInstance_SnapshotIdentifier_MultiAZ_SQLServer(t *testing.T) { + var dbInstance, sourceDbInstance rds.DBInstance + var dbSnapshot rds.DBSnapshot + + rName := acctest.RandomWithPrefix("tf-acc-test") + sourceDbResourceName := "aws_db_instance.source" + snapshotResourceName := "aws_db_snapshot.test" + resourceName := "aws_db_instance.test" + + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckAWSDBInstanceDestroy, + Steps: []resource.TestStep{ + { + Config: testAccAWSDBInstanceConfig_SnapshotIdentifier_MultiAZ_SQLServer(rName, true), + Check: resource.ComposeTestCheckFunc( + testAccCheckAWSDBInstanceExists(sourceDbResourceName, &sourceDbInstance), + testAccCheckDbSnapshotExists(snapshotResourceName, &dbSnapshot), + testAccCheckAWSDBInstanceExists(resourceName, &dbInstance), + resource.TestCheckResourceAttr(resourceName, "multi_az", "true"), + ), + }, + }, + }) +} + func TestAccAWSDBInstance_SnapshotIdentifier_Tags(t *testing.T) { var dbInstance, sourceDbInstance rds.DBInstance var dbSnapshot rds.DBSnapshot @@ -2101,6 +2155,63 @@ resource "aws_db_instance" "test" { `, rName, rName, rName) } +func testAccAWSDBInstanceConfig_SnapshotIdentifier_MultiAZ(rName string, multiAz bool) string { + return fmt.Sprintf(` +resource "aws_db_instance" "source" { + allocated_storage = 5 + engine = "mariadb" + identifier = "%s-source" + instance_class = "db.t2.micro" + password = "avoid-plaintext-passwords" + username = "tfacctest" + skip_final_snapshot = true +} + +resource "aws_db_snapshot" "test" { + db_instance_identifier = "${aws_db_instance.source.id}" + db_snapshot_identifier = %q +} + +resource "aws_db_instance" "test" { + identifier = %q + instance_class = "${aws_db_instance.source.instance_class}" + multi_az = %t + snapshot_identifier = "${aws_db_snapshot.test.id}" + skip_final_snapshot = true +} +`, rName, rName, rName, multiAz) +} + +func testAccAWSDBInstanceConfig_SnapshotIdentifier_MultiAZ_SQLServer(rName string, multiAz bool) string { + return fmt.Sprintf(` +resource "aws_db_instance" "source" { + allocated_storage = 20 + engine = "sqlserver-se" + identifier = "%s-source" + instance_class = "db.m4.large" + license_model = "license-included" + password = "avoid-plaintext-passwords" + username = "tfacctest" + skip_final_snapshot = true +} + +resource "aws_db_snapshot" "test" { + db_instance_identifier = "${aws_db_instance.source.id}" + db_snapshot_identifier = %q +} + +resource "aws_db_instance" "test" { + # InvalidParameterValue: Mirroring cannot be applied to instances with backup retention set to zero. + backup_retention_period = 1 + identifier = %q + instance_class = "${aws_db_instance.source.instance_class}" + multi_az = %t + snapshot_identifier = "${aws_db_snapshot.test.id}" + skip_final_snapshot = true +} +`, rName, rName, rName, multiAz) +} + func testAccAWSDBInstanceConfig_SnapshotIdentifier_Tags(rName string) string { return fmt.Sprintf(` resource "aws_db_instance" "source" {