resource/aws_db_instance: Adds support for domain joining RDS instances #5378

mburtless · 2018-07-30T05:00:41Z

Adds domain and ~~domain-iam-role-name~~ domain_iam_role_name parameters to resource aws_db_instance. Based on work from hashicorp/terraform#14425 and #1367.

As #1367 has been closed, I've merged commits from branch by @mikewalker125 and added acceptance tests and updated documentation.

Output from acceptance testing:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSDBInstance_MSSQL_Domain'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSDBInstance_MSSQL_Domain -timeout 120m
=== RUN   TestAccAWSDBInstance_MSSQL_Domain

--- PASS: TestAccAWSDBInstance_MSSQL_Domain (2516.52s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       2517.593s

…stance.

…stance. This commit adds the domain related parameters to allow aws_db_instances to be joined to a Directory Services Active Directory domain. The original work to add the parameters was merged from mwalkera125 in hashicorp#5226. Subsequent changes added acceptance tests and documentation for the new arguments and attributes. Co-authored-by: Mike Walker <mike.walker@ticketmaster.com> Co-authored-by: Matthew Burtless <mburtless@threepnet.com>

bflad

This is off to a great start, @mburtless! Left some initial comments below. Can you please let us know if you have any questions or are not able to address them? Thanks!

Also, its probably important to note that RestoreDBInstanceFromDBSnapshotInput also supports the Domain and DomainIAMRoleName parameters, so we should support that on creation there as well (preferably with a matching acceptance test). As for CreateDBInstanceReadReplicaInput/RestoreDBInstanceFromS3Input they will require some additional handling to perform the ModifyDBInstance call on creation so they do not require two terraform apply calls.

bflad · 2018-08-09T13:13:41Z

aws/resource_aws_db_instance.go

+			"domain": {
+				Type:     schema.TypeString,
+				Optional: true,
+				Computed: true,


Is there a reason why this attribute is marked Computed: true? We likely want Terraform to report when an instance is part of a domain, but that is not configured in Terraform.

Not sure what the original reason was as this was merged from the previous code for this feature committed by @mikewalker125 in #1367. That said, your feedback makes sense to me. I'll remove Computed: true

bflad · 2018-08-09T13:14:11Z

aws/resource_aws_db_instance.go

+			"domain_iam_role_name": {
+				Type:     schema.TypeString,
+				Optional: true,
+				Computed: true,


Same here. Is there a reason why this attribute is marked Computed: true? We likely want Terraform to report when an instance has a domain IAM role assigned, but that is not configured in Terraform.

Same as above, this was merged from the previous code for this feature committed by @mikewalker125 in #1367. Will remove here as well.

bflad · 2018-08-09T13:15:13Z

aws/resource_aws_db_instance.go

@@ -1012,6 +1032,11 @@ func resourceAwsDbInstanceRead(d *schema.ResourceData, meta interface{}) error {
 		return fmt.Errorf("error setting enabled_cloudwatch_logs_exports: %s", err)
 	}

+	if v.DomainMemberships != nil {


It seems like we should be performing the following check here to prevent potential panics: if len(v.DomainMemberships) > 0 && v.DomainMemberships[0] != nil {

bflad · 2018-08-09T13:17:21Z

aws/resource_aws_db_instance_test.go

+						"aws_db_instance.mssql", "engine", "sqlserver-ex"),
+				),
+			},
+		},


It might be good to add a TestStep that updates/removes the domain settings here.

Makes sense, will add a step to test removing the instance from the domain.

mburtless · 2018-08-09T14:59:04Z

@bflad - Thanks for the feedback. All of those comments make sense and I should be able to get the branch updated in the next few days.

Regarding add support for Domain and DomainIAMRole in Terraform's use of RestoreDBInstanceFromDBSnapshotInput, CreateDBInstanceReadReplicaInput and RestoreDBInstanceFromS3Input; is your preference that we include that in this PR or a future one?

bflad · 2018-08-09T15:02:33Z

We should either include it now or document that it currently takes two terraform applys in the attribute documentation in those situations. 👍 Inevitably without at least documenting it, there will be bug report(s) submitted for the unexpected behavior.

…s further checking to DomainMemberships

@bflad

…in of db instance works as expected. This commit integrates suggesting made by @bflad in review. It also merges any updates to domain or domain_iam_role_name into a single update request, as both arguments must be present in order to update either property of a DB instance. Finally, it adds an accpetance test step to ensure that after a db instance is joined to a domain, it can be succesfully moved to another by updating the domain argument.X

…rm-provider-aws into rds-add-domain

mburtless · 2018-08-20T03:19:44Z

As an update here, at this point I've integrated the suggestions above, except for adding support or documentation for Domain and DomainIAMRole in Terraform's use of RestoreDBInstanceFromDBSnapshotInput, CreateDBInstanceReadReplicaInput and RestoreDBInstanceFromS3Input.

Ended up adding a step to check for updating the domain, instead of deleting it. This led me to discover and undocumented AWS requirement that domain_iam_role_name needs to be provided anytime an update to domain is made and vice versa; hence the merging of the updating of those two arguments into a single if block.

Will try to dig into the remaining work shortly.

…rm-provider-aws into rds-add-domain

mburtless · 2018-08-21T02:51:21Z

Noting that after doing some digging, it doesn't appear as if CreateDBInstanceReadReplicaInput supports the domain or domain_iam_role_name arguments, as RDS instances using the SQL Server engine do not currently support read replicas.

Source: AWS mentions "Read replicas are available in Amazon RDS for MySQL, MariaDB, and PostgreSQL as well as Amazon Aurora."

Additionally the CreateDBInstanceReadReplicaInput struct does not have fields or setters for Domain or DomainIAMRoleName.

Will continue to explore implementation of these arguments for RestoreDBInstanceFromDBSnapshotInput and RestoreDBInstanceFromS3Input

@bflad - Was there another use case you had in mind when you mentioned CreateDBInstanceReadReplicaInput above?

…FromDBSnapshotInput

mburtless · 2018-09-04T02:10:52Z

@bflad - According to AWS Go API Reference RestoreDBInstanceFromS3Input also does not support Domain or DomainIAMRoleName fields.

With that in mind, I think all the features we need to support have been committed to this PR and are ready for another review (support for RestoreDBInstanceFromDBSnapshotInput added in last commit), whenever you get a chance.

* Ensure d.Set() is always called for domain and domain_iam_role_name * Remove extraneous d.IsNewResource() check in Update function * Randomize IAM role naming for new domain acceptance tests

bflad

Great work, @mburtless! After the few minor adjustments noted below in a followup commit, we are all set. 🚀

53 tests passed (all tests)
--- PASS: TestAccAWSDBInstance_IsAlreadyBeingDeleted (501.55s)
--- PASS: TestAccAWSDBInstance_generatedName (502.27s)
--- PASS: TestAccAWSDBInstance_importBasic (504.88s)
--- PASS: TestAccAWSDBInstance_iamAuth (515.09s)
--- PASS: TestAccAWSDBInstance_basic (522.42s)
--- PASS: TestAccAWSDBInstance_namePrefix (523.04s)
--- PASS: TestAccAWSDBInstance_optionGroup (572.84s)
--- PASS: TestAccAWSDBInstance_kmsKey (601.98s)
--- PASS: TestAccAWSDBInstance_FinalSnapshotIdentifier_SkipFinalSnapshot (835.06s)
--- PASS: TestAccAWSDBInstance_FinalSnapshotIdentifier (866.54s)
--- PASS: TestAccAWSDBInstance_subnetGroup (888.59s)
--- PASS: TestAccAWSDBInstance_S3Import (795.53s)
--- PASS: TestAccAWSDBInstance_ReplicateSourceDb_Monitoring (1445.14s)
--- PASS: TestAccAWSDBInstance_ReplicateSourceDb_BackupWindow (1525.62s)
--- PASS: TestAccAWSDBInstance_ReplicateSourceDb_AvailabilityZone (1547.95s)
--- PASS: TestAccAWSDBInstance_ReplicateSourceDb_IamDatabaseAuthenticationEnabled (1550.03s)
--- PASS: TestAccAWSDBInstance_ReplicateSourceDb (1716.81s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier (1257.94s)
--- PASS: TestAccAWSDBInstance_ReplicateSourceDb_AutoMinorVersionUpgrade (1786.90s)
--- PASS: TestAccAWSDBInstance_ReplicateSourceDb_BackupRetentionPeriod (1847.59s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_AutoMinorVersionUpgrade (1268.90s)
--- PASS: TestAccAWSDBInstance_ReplicateSourceDb_MaintenanceWindow (1918.71s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_AvailabilityZone (1172.26s)
--- PASS: TestAccAWSDBInstance_ReplicateSourceDb_ParameterGroupName (1592.86s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_BackupWindow (1208.86s)
--- PASS: TestAccAWSDBInstance_ReplicateSourceDb_AllocatedStorage (2129.93s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_AllocatedStorage (1672.45s)
--- PASS: TestAccAWSDBInstance_ReplicateSourceDb_Port (1745.71s)
--- PASS: TestAccAWSDBInstance_ReplicateSourceDb_VpcSecurityGroupIds (1748.25s)
--- PASS: TestAccAWSDBInstance_ReplicateSourceDb_MultiAZ (1946.01s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_BackupRetentionPeriod (1642.11s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_IamDatabaseAuthenticationEnabled (1219.17s)
--- PASS: TestAccAWSDBInstance_enhancedMonitoring (677.63s)
--- PASS: TestAccAWSDBInstance_separate_iops_update (694.58s)
--- PASS: TestAccAWSDBInstance_diffSuppressInitialState (449.14s)
--- PASS: TestAccAWSDBInstance_MinorVersion (531.46s)
--- PASS: TestAccAWSDBInstance_portUpdate (694.73s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_MaintenanceWindow (1350.10s)
--- PASS: TestAccAWSDBInstance_ec2Classic (450.75s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_Monitoring (1412.59s)
--- PASS: TestAccAWSDBInstance_cloudwatchLogsExportConfiguration (524.51s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_VpcSecurityGroupIds (1199.90s)
--- PASS: TestAccAWSDBInstance_cloudwatchLogsExportConfigurationUpdate (556.47s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_ParameterGroupName (1389.54s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_Port (1373.55s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_VpcSecurityGroupIds_Tags (1320.82s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_Tags (1428.65s)
--- PASS: TestAccAWSDBInstance_EnabledCloudwatchLogsExports_Oracle (823.45s)
--- PASS: TestAccAWSDBInstance_MSSQL_TZ (1398.21s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_MultiAZ (1994.06s)
--- PASS: TestAccAWSDBInstance_MSSQL_DomainSnapshotRestore (2877.79s)
--- PASS: TestAccAWSDBInstance_MSSQL_Domain (3217.20s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_MultiAZ_SQLServer (4178.93s)

bflad · 2018-09-06T13:17:53Z

aws/resource_aws_db_instance.go

@@ -1153,6 +1179,11 @@ func resourceAwsDbInstanceRead(d *schema.ResourceData, meta interface{}) error {
 		return fmt.Errorf("error setting enabled_cloudwatch_logs_exports: %s", err)
 	}

+	if len(v.DomainMemberships) > 0 && v.DomainMemberships[0] != nil {


To detect drift when the domain membership is removed out of band of Terraform, we should ensure the Terraform state always starts with "", e.g.

d.Set("domain", "") d.Set("domain_iam_role_name", "") if len(v.DomainMemberships) > 0 && v.DomainMemberships[0] != nil { d.Set("domain", v.DomainMemberships[0].Domain) d.Set("domain_iam_role_name", v.DomainMemberships[0].IAMRoleName) }

👍

bflad · 2018-09-06T13:19:19Z

aws/resource_aws_db_instance.go

@@ -1412,6 +1443,14 @@ func resourceAwsDbInstanceUpdate(d *schema.ResourceData, meta interface{}) error
 		requestUpdate = true
 	}

+	if (d.HasChange("domain") || d.HasChange("domain_iam_role_name")) && !d.IsNewResource() {


We very recently fixed the resource so it does not call the Update function after the Create function, so the !d.IsNewResource() check here is now extraneous. 😄

bflad · 2018-09-06T13:23:22Z

aws/resource_aws_db_instance_test.go

+}
+
+resource "aws_iam_role" "role" {
+  name = "tf-acc-db-instance-mssql-domain-role"


Our CI environment (and soon locally 😉 ) runs acceptance testing in parallel so we need to randomize resource naming or we'll receive errors, e.g.

--- FAIL: TestAccAWSDBInstance_MSSQL_DomainSnapshotRestore (1891.97s) testing.go:527: Step 0 error: Error applying: 1 error occurred: * aws_iam_role.role: 1 error occurred: * aws_iam_role.role: Error creating IAM Role tf-acc-db-instance-mssql-domain-role: EntityAlreadyExists: Role with name tf-acc-db-instance-mssql-domain-role already exists.

Since we're already passing rInt throughout the test configuration here to achieve this with other resources, we can just do the same here. 👍

mburtless · 2018-09-06T15:59:39Z

Great news, glad I could help out! Thanks for all the help and feedback here Brian, and to @mikewalker125 for his work on the previous PR.

bflad · 2018-09-13T02:49:00Z

This has been released in version 1.36.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

RaviKumar1209 · 2019-12-12T11:31:35Z

can we now use terraform to add RDS instances to domain? If yes, what attribute we should be using for that?

ghost · 2019-12-12T14:50:15Z

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!

Adds domain and domain-iam-role-name parameters to resource aws_db_in…

e7806b1

…stance.

ghost added the size/L Managed by automation to categorize the size of a PR. label Jul 30, 2018

mburtless force-pushed the rds-add-domain branch from c386751 to 2ad8d53 Compare July 30, 2018 05:02

ghost added the size/L Managed by automation to categorize the size of a PR. label Jul 30, 2018

bflad added enhancement Requests to existing resources that expand the functionality or scope. service/rds Issues and PRs that pertain to the rds service. labels Jul 30, 2018

Merge branch 'master' into rds-add-domain

24d72f2

ghost added the size/L Managed by automation to categorize the size of a PR. label Aug 2, 2018

bflad reviewed Aug 9, 2018

View reviewed changes

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Aug 9, 2018

mburtless added 4 commits August 11, 2018 14:24

Removes Computed attributes from domain and domain_iam_role_name, add…

9a2b871

…s further checking to DomainMemberships

Updated name of config for domain acceptance test to match standard

6d7c4f5

Merge branch 'rds-add-domain' of https://github.com/mburtless/terrafo…

a0effd4

…rm-provider-aws into rds-add-domain

ghost added the size/L Managed by automation to categorize the size of a PR. label Aug 20, 2018

Merge branch 'master' into rds-add-domain

c86a557

mburtless added 2 commits August 19, 2018 23:25

Removed commented test code

82e6057

Merge branch 'rds-add-domain' of https://github.com/mburtless/terrafo…

6f8e687

…rm-provider-aws into rds-add-domain

Adds support for domain and domain_iam_role_name to RestoreDBInstance…

5e2da4a

…FromDBSnapshotInput

Merge branch 'master' into rds-add-domain

f8ed378

bflad removed the waiting-response Maintainers are waiting on response from community or contributor. label Sep 4, 2018

bflad approved these changes Sep 6, 2018

View reviewed changes

bflad added this to the v1.36.0 milestone Sep 6, 2018

bflad merged commit f8ed378 into hashicorp:master Sep 6, 2018

bflad added a commit that referenced this pull request Sep 6, 2018

Update CHANGELOG for #5378

2bcb244

ghost locked and limited conversation to collaborators Dec 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resource/aws_db_instance: Adds support for domain joining RDS instances #5378

resource/aws_db_instance: Adds support for domain joining RDS instances #5378

mburtless commented Jul 30, 2018 •

edited

Loading

bflad left a comment

bflad Aug 9, 2018

mburtless Aug 9, 2018

bflad Aug 9, 2018

mburtless Aug 9, 2018

bflad Aug 9, 2018

bflad Aug 9, 2018

mburtless Aug 9, 2018

mburtless commented Aug 9, 2018

bflad commented Aug 9, 2018

mburtless commented Aug 20, 2018 •

edited

Loading

mburtless commented Aug 21, 2018

mburtless commented Sep 4, 2018

bflad left a comment

bflad Sep 6, 2018

bflad Sep 6, 2018

bflad Sep 6, 2018

mburtless commented Sep 6, 2018

bflad commented Sep 13, 2018

RaviKumar1209 commented Dec 12, 2019 •

edited

Loading

ghost commented Dec 12, 2019

resource/aws_db_instance: Adds support for domain joining RDS instances #5378

resource/aws_db_instance: Adds support for domain joining RDS instances #5378

Conversation

mburtless commented Jul 30, 2018 • edited Loading

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mburtless commented Aug 9, 2018

bflad commented Aug 9, 2018

mburtless commented Aug 20, 2018 • edited Loading

mburtless commented Aug 21, 2018

mburtless commented Sep 4, 2018

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mburtless commented Sep 6, 2018

bflad commented Sep 13, 2018

RaviKumar1209 commented Dec 12, 2019 • edited Loading

ghost commented Dec 12, 2019

mburtless commented Jul 30, 2018 •

edited

Loading

mburtless commented Aug 20, 2018 •

edited

Loading

RaviKumar1209 commented Dec 12, 2019 •

edited

Loading