-
Notifications
You must be signed in to change notification settings - Fork 9.2k
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
Performance Insights configuration for aws_db_instance #6453
Conversation
Any update on when this will be merged/release? |
👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @msvab 👋 Thanks so much for submitting this. I left some initial comments below. Some of these review items were missed in the aws_rds_cluster_instance
resource implementation and I have a hunch that the testing updates will tease out a few additional issues (similar to #3015).
Please let us know if you have any questions or do not have time to implement the feedback.
aws/resource_aws_db_instance_test.go
Outdated
CheckDestroy: testAccCheckAWSClusterDestroy, | ||
Steps: []resource.TestStep{ | ||
{ | ||
Config: testAccAWSDBInstancePerformanceInsights(acctest.RandInt()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This random integer should be moved under the function signature so it can be consistent across multiple TestStep
, e.g.
rInt := acctest.RandInt()
// ...
Config: testAccAWSDBInstancePerformanceInsights(rInt, ...)
Thanks for the detailed feedback, I've implemented all the changes. Let me know if you need any more changes to be done! |
Any update on if the above changes will be accepted? We'd love to use this once it get released. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi again @msvab 👋 I noticed one of the feedback items wasn't commented on or resolved (regarding moving the random integer to be consistent across all test steps so it does not recreate the database instances each step). Playing with the acceptance testing to ensure we cover various update scenarios and address that issue, I ran into issues with disabling PI.
For example, having PI enabled on an instance then disabling it by removing the three performance_insights_*
arguments in the configuration, generated the following error:
--- FAIL: TestAccAWSRDSDBInstance_PerformanceInsightsEnabled (528.04s)
testing.go:538: Step 2 error: Error applying: 1 error occurred:
* aws_db_instance.bar: 1 error occurred:
* aws_db_instance.bar: Error modifying DB Instance tf-db-test-2613321838204137906: InvalidParameterValue: Invalid Performance Insights retention period. Valid values are: [7, 731]
status code: 400, request id: f840f143-1b34-472e-944a-cfb2e1122af9
Its submitting this to ModifyDBInstance
, which is invalid according to the RDS API:
2018/12/19 21:44:34 [DEBUG] DB Instance Modification request: {
ApplyImmediately: false,
DBInstanceIdentifier: "tf-db-test-2613321838204137906",
EnablePerformanceInsights: false,
PerformanceInsightsKMSKeyId: "",
PerformanceInsightsRetentionPeriod: 0
}
Here's the updated acceptance test and configuration I was working with:
func TestAccAWSRDSDBInstance_PerformanceInsightsEnabled(t *testing.T) {
var v rds.DBInstance
rInt := acctest.RandInt()
resourceName := "aws_db_instance.bar"
resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckAWSClusterDestroy,
Steps: []resource.TestStep{
{
Config: testAccAWSDBInstanceConfigPerformanceInsightsEnabled(rInt, 7),
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSDBInstanceExists(resourceName, &v),
testAccCheckAWSDBInstanceAttributes(&v),
resource.TestCheckResourceAttr(resourceName, "performance_insights_enabled", "true"),
resource.TestCheckResourceAttrPair(resourceName, "performance_insights_kms_key_id", "aws_kms_key.foo", "arn"),
resource.TestCheckResourceAttr(resourceName, "performance_insights_retention_period", "7"),
),
},
{
ResourceName: resourceName,
ImportState: true,
ImportStateVerify: true,
ImportStateVerifyIgnore: []string{
"password",
"skip_final_snapshot",
"final_snapshot_identifier",
},
},
// Test disabling Performance Insights via update
{
Config: testAccAWSDBInstanceConfigPerformanceInsightsDisabled(rInt),
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSDBInstanceExists(resourceName, &v),
testAccCheckAWSDBInstanceAttributes(&v),
resource.TestCheckResourceAttr(resourceName, "performance_insights_enabled", "false"),
resource.TestCheckResourceAttr(resourceName, "performance_insights_kms_key_id", ""),
resource.TestCheckResourceAttr(resourceName, "performance_insights_retention_period", "0"),
),
},
// Test enabling Performance Insights via update
{
Config: testAccAWSDBInstanceConfigPerformanceInsightsEnabled(rInt, 7),
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSDBInstanceExists(resourceName, &v),
testAccCheckAWSDBInstanceAttributes(&v),
resource.TestCheckResourceAttr(resourceName, "performance_insights_enabled", "true"),
resource.TestCheckResourceAttrPair(resourceName, "performance_insights_kms_key_id", "aws_kms_key.foo", "arn"),
resource.TestCheckResourceAttr(resourceName, "performance_insights_retention_period", "7"),
),
},
// Test updating only Performance Insights retention period via update
{
Config: testAccAWSDBInstanceConfigPerformanceInsightsEnabled(rInt, 731),
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSDBInstanceExists(resourceName, &v),
testAccCheckAWSDBInstanceAttributes(&v),
resource.TestCheckResourceAttr(resourceName, "performance_insights_enabled", "true"),
resource.TestCheckResourceAttrPair(resourceName, "performance_insights_kms_key_id", "aws_kms_key.foo", "arn"),
resource.TestCheckResourceAttr(resourceName, "performance_insights_retention_period", "731"),
),
},
},
})
}
func testAccAWSDBInstanceConfigPerformanceInsightsDisabled(n int) string {
return fmt.Sprintf(`
resource "aws_db_instance" "bar" {
allocated_storage = 5
backup_retention_period = 0
engine = "mysql"
engine_version = "5.6.41"
identifier = "tf-db-test-%d"
instance_class = "db.m3.medium"
name = "mydb"
password = "mustbeeightcharaters"
skip_final_snapshot = true
username = "foo"
}
`, n)
}
func testAccAWSDBInstanceConfigPerformanceInsightsEnabled(n, piRetentionPeriod int) string {
return fmt.Sprintf(`
resource "aws_kms_key" "foo" {
description = "Terraform acc test %d"
policy = <<POLICY
{
"Version": "2012-10-17",
"Id": "kms-tf-1",
"Statement": [
{
"Sid": "Enable IAM User Permissions",
"Effect": "Allow",
"Principal": {
"AWS": "*"
},
"Action": "kms:*",
"Resource": "*"
}
]
}
POLICY
}
resource "aws_db_instance" "bar" {
engine = "mysql"
engine_version = "5.6.41"
identifier = "tf-db-test-%d"
instance_class = "db.m3.medium"
allocated_storage = 5
backup_retention_period = 0
name = "mydb"
username = "foo"
password = "mustbeeightcharaters"
skip_final_snapshot = true
performance_insights_enabled = true
performance_insights_kms_key_id = "${aws_kms_key.foo.arn}"
performance_insights_retention_period = %d
}
`, n, n, piRetentionPeriod)
}
This logic here in resourceAwsDbInstanceUpdate()
needs to be updated to properly handle disabling PI:
if d.HasChange("performance_insights_enabled") {
d.SetPartial("performance_insights_enabled")
req.EnablePerformanceInsights = aws.Bool(d.Get("performance_insights_enabled").(bool))
requestUpdate = true
}
if d.HasChange("performance_insights_kms_key_id") {
d.SetPartial("performance_insights_kms_key_id")
req.PerformanceInsightsKMSKeyId = aws.String(d.Get("performance_insights_kms_key_id").(string))
requestUpdate = true
}
if d.HasChange("performance_insights_retention_period") {
d.SetPartial("performance_insights_retention_period")
req.PerformanceInsightsRetentionPeriod = aws.Int64(int64(d.Get("performance_insights_retention_period").(int)))
requestUpdate = true
}
To send the PerformanceInsightsKMSKeyId
and PerformanceInsightsRetentionPeriod
only when PI is enabled. Maybe something like this:
// NOTE: Unverified and may require additional changes
if d.HasChange("performance_insights_enabled") {
d.SetPartial("performance_insights_enabled")
req.EnablePerformanceInsights = aws.Bool(d.Get("performance_insights_enabled").(bool))
requestUpdate = true
}
if d.Get("performance_insights_enabled").(bool) && d.HasChange("performance_insights_kms_key_id") {
d.SetPartial("performance_insights_kms_key_id")
req.PerformanceInsightsKMSKeyId = aws.String(d.Get("performance_insights_kms_key_id").(string))
requestUpdate = true
}
if d.Get("performance_insights_enabled").(bool) && d.HasChange("performance_insights_retention_period") {
d.SetPartial("performance_insights_retention_period")
req.PerformanceInsightsRetentionPeriod = aws.Int64(int64(d.Get("performance_insights_retention_period").(int)))
requestUpdate = true
}
There may be other issues too with updates which is why I added the other update test steps above. We should probably also add acceptance testing to check for errors updating the KMS key, like #3015 from aws_rds_cluster
, since I would guess the ModifyDBCluster
and ModifyDBInstance
API calls likely behave similarly.
Please let us know if you have time to perform these fixes or have any questions, thanks.
@@ -571,6 +588,18 @@ func resourceAwsDbInstanceCreate(d *schema.ResourceData, meta interface{}) error | |||
requiresModifyDbInstance = true | |||
} | |||
|
|||
if attr, ok := d.GetOk("performance_insights_enabled"); ok { | |||
opts.EnablePerformanceInsights = aws.Bool(attr.(bool)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should have an acceptance test added to ensure its functionality and prevent regressions, e.g. something similar to:
func TestAccAWSDBInstance_ReplicateSourceDb_PerformanceInsightsEnabled(t *testing.T) {
var dbInstance, sourceDbInstance rds.DBInstance
rName := acctest.RandomWithPrefix("tf-acc-test")
kmsKeyResourceName := "aws_kms_key.test"
sourceResourceName := "aws_db_instance.source"
resourceName := "aws_db_instance.test"
resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckAWSDBInstanceDestroy,
Steps: []resource.TestStep{
{
Config: testAccAWSDBInstanceConfig_ReplicateSourceDb_PerformanceInsightsEnabled(rName),
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSDBInstanceExists(sourceResourceName, &sourceDbInstance),
testAccCheckAWSDBInstanceExists(resourceName, &dbInstance),
testAccCheckAWSDBInstanceReplicaAttributes(&sourceDbInstance, &dbInstance),
resource.TestCheckResourceAttr(resourceName, "performance_insights_enabled", "true"),
resource.TestCheckResourceAttrPair(resourceName, "performance_insights_kms_key_id", kmsKeyResourceName, "arn"),
resource.TestCheckResourceAttr(resourceName, "performance_insights_retention_period", "7"),
),
},
},
})
}
func testAccAWSDBInstanceConfig_ReplicateSourceDb_PerformanceInsightsEnabled(rName string) string {
return fmt.Sprintf(`
resource "aws_kms_key" "test" {
description = "Terraform acc test"
policy = <<POLICY
{
"Version": "2012-10-17",
"Id": "kms-tf-1",
"Statement": [
{
"Sid": "Enable IAM User Permissions",
"Effect": "Allow",
"Principal": {
"AWS": "*"
},
"Action": "kms:*",
"Resource": "*"
}
]
}
POLICY
}
resource "aws_db_instance" "source" {
allocated_storage = 5
backup_retention_period = 1
engine = "mysql"
engine_version = "5.6.41"
identifier = "%s-source"
instance_class = "db.m3.medium"
password = "avoid-plaintext-passwords"
username = "tfacctest"
skip_final_snapshot = true
}
resource "aws_db_instance" "test" {
identifier = %q
instance_class = "${aws_db_instance.source.instance_class}"
performance_insights_enabled = true
performance_insights_kms_key_id = "${aws_kms_key.test.arn}"
performance_insights_retention_period = 7
replicate_source_db = "${aws_db_instance.source.id}"
skip_final_snapshot = true
}
`, rName, rName)
}
@@ -422,6 +422,23 @@ func resourceAwsDbInstance() *schema.Resource { | |||
Optional: true, | |||
}, | |||
|
|||
"performance_insights_enabled": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This attribute (along with the other performance_insights_*
attributes) are not currently handled during the RestoreDBInstanceFromDBSnapshot
creation flow so attempting to creating a configuration with PI enabled and snapshot_identifier
will require two applies of the configuration to converge.
While RestoreDBInstanceFromDBSnapshot
does not directly support setting EnablePerformanceInsights
, PerformanceInsightsKmsKeyId
, and PerformanceInsightsRetentionPeriod
, we have a framework in place to handle this situation by calling ModifyDBInstance
immediately after restore, e.g.
// Provided as an example of this logic so similar logic can be added near it
if attr, ok := d.GetOk("backup_window"); ok {
modifyDbInstanceInput.PreferredBackupWindow = aws.String(attr.(string))
requiresModifyDbInstance = true
}
This should have an acceptance test added to ensure its functionality and prevent regressions, e.g. something similar to:
func TestAccAWSDBInstance_SnapshotIdentifier_PerformanceInsightsEnabled(t *testing.T) {
var dbInstance, sourceDbInstance rds.DBInstance
var dbSnapshot rds.DBSnapshot
rName := acctest.RandomWithPrefix("tf-acc-test")
kmsKeyResourceName := "aws_kms_key.test"
sourceDbResourceName := "aws_db_instance.source"
snapshotResourceName := "aws_db_snapshot.test"
resourceName := "aws_db_instance.test"
resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckAWSDBInstanceDestroy,
Steps: []resource.TestStep{
{
Config: testAccAWSDBInstanceConfig_SnapshotIdentifier_PerformanceInsightsEnabled(rName),
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSDBInstanceExists(sourceDbResourceName, &sourceDbInstance),
testAccCheckDbSnapshotExists(snapshotResourceName, &dbSnapshot),
testAccCheckAWSDBInstanceExists(resourceName, &dbInstance),
resource.TestCheckResourceAttr(resourceName, "performance_insights_enabled", "true"),
resource.TestCheckResourceAttrPair(resourceName, "performance_insights_kms_key_id", kmsKeyResourceName, "arn"),
resource.TestCheckResourceAttr(resourceName, "performance_insights_retention_period", "7"),
),
},
},
})
}
func testAccAWSDBInstanceConfig_SnapshotIdentifier_PerformanceInsightsEnabled(rName string) string {
return fmt.Sprintf(`
resource "aws_kms_key" "test" {
description = "Terraform acc test"
policy = <<POLICY
{
"Version": "2012-10-17",
"Id": "kms-tf-1",
"Statement": [
{
"Sid": "Enable IAM User Permissions",
"Effect": "Allow",
"Principal": {
"AWS": "*"
},
"Action": "kms:*",
"Resource": "*"
}
]
}
POLICY
}
resource "aws_db_instance" "source" {
allocated_storage = 5
engine = "mysql"
engine_version = "5.6.41"
identifier = "%s-source"
instance_class = "db.m3.medium"
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}"
performance_insights_enabled = true
performance_insights_kms_key_id = "${aws_kms_key.test.arn}"
performance_insights_retention_period = 7
snapshot_identifier = "${aws_db_snapshot.test.id}"
skip_final_snapshot = true
}
`, rName, rName, rName)
}
Hi @bflad, I've added the extra tests and tried to address the rest of the feedback, but I got a bit stuck and I'm not sure I can finish this on my own since I lack the knowledge of your testing framework. So once PI is enabled, the KMS Key cannot ever be changed. Even if you disable it and query the API, it will return the key in the response, then if you enable it, it will be enabled with the original key. So once KMS Key is set, you can only update
|
Any ETA for a merge? |
@msvab I looked at your PR and found there were different things that needed changing to get the tests passing. It seems there are a couple things that are misconfigured to get it working, here is the diff I found that gets everything operating nicely.
|
Thanks @RossHammer! I've updated the code and the tests are passing now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much, @msvab 🚀
Output from acceptance testing (test failures unrelated):
--- FAIL: TestAccAWSDBInstance_optionGroup (8.87s)
--- PASS: TestAccAWSDBInstance_IsAlreadyBeingDeleted (326.60s)
--- PASS: TestAccAWSDBInstance_iamAuth (366.55s)
--- PASS: TestAccAWSDBInstance_namePrefix (368.12s)
--- PASS: TestAccAWSDBInstance_basic (397.41s)
--- PASS: TestAccAWSDBInstance_DeletionProtection (462.26s)
--- PASS: TestAccAWSDBInstance_generatedName (487.97s)
--- PASS: TestAccAWSDBInstance_kmsKey (507.14s)
--- FAIL: TestAccAWSDBInstance_ReplicateSourceDb_AutoMinorVersionUpgrade (787.57s)
--- PASS: TestAccAWSDBInstance_FinalSnapshotIdentifier_SkipFinalSnapshot (810.11s)
--- PASS: TestAccAWSDBInstance_subnetGroup (841.48s)
--- PASS: TestAccAWSDBInstance_FinalSnapshotIdentifier (862.22s)
--- FAIL: TestAccAWSDBInstance_S3Import (649.43s)
--- PASS: TestAccAWSDBInstance_ReplicateSourceDb_MaintenanceWindow (1155.60s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier (806.14s)
--- PASS: TestAccAWSDBInstance_ReplicateSourceDb (1357.50s)
--- PASS: TestAccAWSDBInstance_ReplicateSourceDb_AllocatedStorage (1569.63s)
--- PASS: TestAccAWSDBInstance_ReplicateSourceDb_BackupWindow (1631.75s)
--- PASS: TestAccAWSDBInstance_ReplicateSourceDb_Monitoring (1695.58s)
--- PASS: TestAccAWSDBInstance_ReplicateSourceDb_IamDatabaseAuthenticationEnabled (1703.77s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_Io1Storage (1240.15s)
--- PASS: TestAccAWSDBInstance_ReplicateSourceDb_AvailabilityZone (1756.74s)
--- PASS: TestAccAWSDBInstance_ReplicateSourceDb_VpcSecurityGroupIds (1417.22s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_AllocatedStorage (1402.69s)
--- PASS: TestAccAWSDBInstance_ReplicateSourceDb_Port (1557.03s)
--- PASS: TestAccAWSDBInstance_ReplicateSourceDb_BackupRetentionPeriod (1953.97s)
--- PASS: TestAccAWSDBInstance_ReplicateSourceDb_ParameterGroupName (1662.77s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_AvailabilityZone (1187.13s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_AutoMinorVersionUpgrade (1211.61s)
--- PASS: TestAccAWSDBInstance_ReplicateSourceDb_MultiAZ (2058.88s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_BackupRetentionPeriod (1483.63s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_IamDatabaseAuthenticationEnabled (1080.36s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_DeletionProtection (1256.33s)
--- PASS: TestAccAWSDBInstance_portUpdate (487.46s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_BackupWindow (1445.03s)
--- PASS: TestAccAWSDBInstance_separate_iops_update (589.04s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_MaintenanceWindow (1190.17s)
--- PASS: TestAccAWSDBInstance_enhancedMonitoring (669.59s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_BackupRetentionPeriod_Unset (1844.67s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_Monitoring (1173.41s)
--- PASS: TestAccAWSDBInstance_MinorVersion (412.52s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_ParameterGroupName (1142.40s)
--- PASS: TestAccAWSDBInstance_cloudwatchLogsExportConfiguration (396.58s)
--- PASS: TestAccAWSDBInstance_ec2Classic (417.22s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_Port (1201.76s)
--- PASS: TestAccAWSDBInstance_diffSuppressInitialState (544.77s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_VpcSecurityGroupIds (1203.34s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_Tags (1304.12s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_VpcSecurityGroupIds_Tags (1193.58s)
--- PASS: TestAccAWSDBInstance_EnabledCloudwatchLogsExports_Postgresql (494.06s)
--- PASS: TestAccAWSDBInstance_EnabledCloudwatchLogsExports_Oracle (664.57s)
--- PASS: TestAccAWSDBInstance_cloudwatchLogsExportConfigurationUpdate (756.27s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_MultiAZ (1784.66s)
--- PASS: TestAccAWSDBInstance_MSSQL_TZ (1712.39s)
--- PASS: TestAccAWSRDSDBInstance_withPerformanceInsights (1063.00s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_PerformanceInsightsEnabled (1373.20s)
--- PASS: TestAccAWSDBInstance_ReplicateSourceDb_PerformanceInsightsEnabled (1436.25s)
--- PASS: TestAccAWSDBInstance_MySQL_SnapshotRestoreWithEngineVersion (1906.22s)
--- PASS: TestAccAWSDBInstance_MSSQL_DomainSnapshotRestore (3093.27s)
--- PASS: TestAccAWSDBInstance_MSSQL_Domain (3344.62s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_MultiAZ_SQLServer (4817.70s)
This has been released in version 2.12.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. |
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! |
Fixes #5259
Changes proposed in this pull request:
performance_insights_enabled
to aws_db_instanceperformance_insights_kms_key_id
to aws_db_instanceperformance_insights_retention_period
to aws_db_instanceOutput from acceptance testing: