Remove readonly properties from DMS ReplicationTaskSettings #13476

aburgel · 2020-05-24T02:02:47Z

Community Note

Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

The DMS API DescribeReplicationTasks includes read only properties in ReplicationTaskSettings. This causes terraform plan to show that there are changes to apply since your local state will aways be different from remote state. This PR removes these read only properties when setting state so that we only compare properties that are under our control.

Release note for CHANGELOG:

resource/aws_dms_replication_task Remove read only properties from ReplicationTaskSettings

Output from acceptance testing:

$ make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSDmsReplicationTaskBasic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -count 1 -parallel 1 -run=TestAccAWSDmsReplicationTaskBasic -timeout 120m
=== RUN   TestAccAWSDmsReplicationTaskBasic
=== PAUSE TestAccAWSDmsReplicationTaskBasic
=== CONT  TestAccAWSDmsReplicationTaskBasic
--- PASS: TestAccAWSDmsReplicationTaskBasic (687.25s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	690.785s

aburgel · 2020-05-24T02:03:53Z

aws/diff_suppress_funcs.go

@@ -8,7 +8,7 @@ import (
 	"strings"

 	"github.com/hashicorp/terraform-plugin-sdk/helper/schema"
-	"github.com/jen20/awspolicyequivalence"
+	awspolicy "github.com/jen20/awspolicyequivalence"


Not sure why this appeared here, but the tests pass and it seems like it should be this way... Happy to remove it if it's not needed. (FWIW, I'm relatively new to golang)

aburgel · 2020-05-24T02:09:07Z

aws/resource_aws_dms_replication_task.go

+		delete(logging, "CloudWatchLogGroup")
+		delete(logging, "CloudWatchLogStream")


If you define these in your task settings, DMS API will return an error that they are read only.

aburgel · 2020-05-24T02:11:25Z

aws/resource_aws_dms_replication_task.go

+
+	controlTablesSettings, ok := settingsData["ControlTablesSettings"].(map[string]interface{})
+	if ok {
+		delete(controlTablesSettings, "historyTimeslotInMinutes")


There is another property HistoryTimeslotInMinutes (with a capital H) that is the real property. I don't know why this one exists, but I'm cleaning it out since leaving it in when we set state would cause a difference to show.

aburgel · 2020-05-24T02:12:25Z

aws/resource_aws_dms_replication_task_test.go

+			{
+				Config:             dmsReplicationTaskConfig(randId),
+				PlanOnly:           true,
+				ExpectNonEmptyPlan: false,
+			},


Now we run plan after create to assert that there are no changes. Without the fix, this will fail.

aburgel · 2020-05-24T02:16:07Z

aws/resource_aws_dms_replication_task_test.go

+data "aws_iam_policy_document" "dms_assume_role_policy_document" {
+  statement {
+    actions = ["sts:AssumeRole"]
+
+    principals {
+      identifiers = ["dms.amazonaws.com"]
+      type        = "Service"
+    }
+  }
+}
+
+resource "aws_iam_role" "dms_vpc_role" {
+  assume_role_policy = data.aws_iam_policy_document.dms_assume_role_policy_document.json
+  name = "dms-vpc-role"
+}
+
+resource "aws_iam_role" "dms_cloudwatch_logs_role" {
+  assume_role_policy = data.aws_iam_policy_document.dms_assume_role_policy_document.json
+  name = "dms-cloudwatch-logs-role"
+}
+
+resource "aws_iam_role_policy_attachment" "dms_vpc_access_policy" {
+  role       = aws_iam_role.dms_vpc_role.name
+  policy_arn = "arn:aws:iam::aws:policy/service-role/AmazonDMSVPCManagementRole"
+}
+
+resource "aws_iam_role_policy_attachment" "dms_cloudwatch_logs_access_policy" {
+  role       = aws_iam_role.dms_cloudwatch_logs_role.name
+  policy_arn = "arn:aws:iam::aws:policy/service-role/AmazonDMSCloudWatchLogsRole"
+}


Configuring DMS via the API requires two access policies, without them you'll get an error:

Error: AccessDeniedFault: The IAM Role arn:aws:iam::<accountid>:role/dms-vpc-role is not configured properly.

I'm assuming that people run these tests against an empty AWS account, but if that's not the case, then this may cause failures because the role names are fixed.

aburgel · 2020-05-24T02:23:55Z

aws/resource_aws_dms_replication_task_test.go

@@ -178,7 +219,7 @@ resource "aws_dms_replication_task" "dms_replication_task" {
  migration_type            = "full-load"
  replication_instance_arn  = "${aws_dms_replication_instance.dms_replication_instance.replication_instance_arn}"
  replication_task_id       = "tf-test-dms-replication-task-%[1]s"
-  replication_task_settings = "{\"TargetMetadata\":{\"TargetSchema\":\"\",\"SupportLobs\":true,\"FullLobMode\":false,\"LobChunkSize\":0,\"LimitedSizeLobMode\":true,\"LobMaxSize\":32,\"LoadMaxFileSize\":0,\"ParallelLoadThreads\":0,\"BatchApplyEnabled\":false},\"FullLoadSettings\":{\"FullLoadEnabled\":true,\"ApplyChangesEnabled\":false,\"TargetTablePrepMode\":\"DROP_AND_CREATE\",\"CreatePkAfterFullLoad\":false,\"StopTaskCachedChangesApplied\":false,\"StopTaskCachedChangesNotApplied\":false,\"ResumeEnabled\":false,\"ResumeMinTableSize\":100000,\"ResumeOnlyClusteredPKTables\":true,\"MaxFullLoadSubTasks\":8,\"TransactionConsistencyTimeout\":600,\"CommitRate\":10000},\"Logging\":{\"EnableLogging\":false,\"LogComponents\":[{\"Id\":\"SOURCE_UNLOAD\",\"Severity\":\"LOGGER_SEVERITY_DEFAULT\"},{\"Id\":\"TARGET_LOAD\",\"Severity\":\"LOGGER_SEVERITY_DEFAULT\"},{\"Id\":\"SOURCE_CAPTURE\",\"Severity\":\"LOGGER_SEVERITY_DEFAULT\"},{\"Id\":\"TARGET_APPLY\",\"Severity\":\"LOGGER_SEVERITY_DEFAULT\"},{\"Id\":\"TASK_MANAGER\",\"Severity\":\"LOGGER_SEVERITY_DEFAULT\"}],\"CloudWatchLogGroup\":null,\"CloudWatchLogStream\":null},\"ControlTablesSettings\":{\"historyTimeslotInMinutes\":5,\"ControlSchema\":\"\",\"HistoryTimeslotInMinutes\":5,\"HistoryTableEnabled\":false,\"SuspendedTablesTableEnabled\":false,\"StatusTableEnabled\":false},\"StreamBufferSettings\":{\"StreamBufferCount\":3,\"StreamBufferSizeInMB\":8,\"CtrlStreamBufferSizeInMB\":5},\"ChangeProcessingDdlHandlingPolicy\":{\"HandleSourceTableDropped\":true,\"HandleSourceTableTruncated\":true,\"HandleSourceTableAltered\":true},\"ErrorBehavior\":{\"DataErrorPolicy\":\"LOG_ERROR\",\"DataTruncationErrorPolicy\":\"LOG_ERROR\",\"DataErrorEscalationPolicy\":\"SUSPEND_TABLE\",\"DataErrorEscalationCount\":0,\"TableErrorPolicy\":\"SUSPEND_TABLE\",\"TableErrorEscalationPolicy\":\"STOP_TASK\",\"TableErrorEscalationCount\":0,\"RecoverableErrorCount\":-1,\"RecoverableErrorInterval\":5,\"RecoverableErrorThrottling\":true,\"RecoverableErrorThrottlingMax\":1800,\"ApplyErrorDeletePolicy\":\"IGNORE_RECORD\",\"ApplyErrorInsertPolicy\":\"LOG_ERROR\",\"ApplyErrorUpdatePolicy\":\"LOG_ERROR\",\"ApplyErrorEscalationPolicy\":\"LOG_ERROR\",\"ApplyErrorEscalationCount\":0,\"FullLoadIgnoreConflicts\":true},\"ChangeProcessingTuning\":{\"BatchApplyPreserveTransaction\":true,\"BatchApplyTimeoutMin\":1,\"BatchApplyTimeoutMax\":30,\"BatchApplyMemoryLimit\":500,\"BatchSplitSize\":0,\"MinTransactionSize\":1000,\"CommitTimeout\":1,\"MemoryLimitTotal\":1024,\"MemoryKeepTime\":60,\"StatementCacheSize\":50}}"
+  replication_task_settings = "{\"BeforeImageSettings\":null,\"ChangeProcessingDdlHandlingPolicy\":{\"HandleSourceTableAltered\":true,\"HandleSourceTableDropped\":true,\"HandleSourceTableTruncated\":true},\"ChangeProcessingTuning\":{\"BatchApplyMemoryLimit\":500,\"BatchApplyPreserveTransaction\":true,\"BatchApplyTimeoutMax\":30,\"BatchApplyTimeoutMin\":1,\"BatchSplitSize\":0,\"CommitTimeout\":1,\"MemoryKeepTime\":60,\"MemoryLimitTotal\":1024,\"MinTransactionSize\":1000,\"StatementCacheSize\":50},\"CharacterSetSettings\":null,\"ControlTablesSettings\":{\"ControlSchema\":\"\",\"HistoryTableEnabled\":false,\"HistoryTimeslotInMinutes\":5,\"StatusTableEnabled\":false,\"SuspendedTablesTableEnabled\":false},\"ErrorBehavior\":{\"ApplyErrorDeletePolicy\":\"IGNORE_RECORD\",\"ApplyErrorEscalationCount\":0,\"ApplyErrorEscalationPolicy\":\"LOG_ERROR\",\"ApplyErrorFailOnTruncationDdl\":false,\"ApplyErrorInsertPolicy\":\"LOG_ERROR\",\"ApplyErrorUpdatePolicy\":\"LOG_ERROR\",\"DataErrorEscalationCount\":0,\"DataErrorEscalationPolicy\":\"SUSPEND_TABLE\",\"DataErrorPolicy\":\"LOG_ERROR\",\"DataTruncationErrorPolicy\":\"LOG_ERROR\",\"FailOnNoTablesCaptured\":false,\"FailOnTransactionConsistencyBreached\":false,\"FullLoadIgnoreConflicts\":true,\"RecoverableErrorCount\":-1,\"RecoverableErrorInterval\":5,\"RecoverableErrorThrottling\":true,\"RecoverableErrorThrottlingMax\":1800,\"TableErrorEscalationCount\":0,\"TableErrorEscalationPolicy\":\"STOP_TASK\",\"TableErrorPolicy\":\"SUSPEND_TABLE\"},\"FullLoadSettings\":{\"CommitRate\":10000,\"CreatePkAfterFullLoad\":false,\"MaxFullLoadSubTasks\":8,\"StopTaskCachedChangesApplied\":false,\"StopTaskCachedChangesNotApplied\":false,\"TargetTablePrepMode\":\"DROP_AND_CREATE\",\"TransactionConsistencyTimeout\":600},\"Logging\":{\"EnableLogging\":false,\"LogComponents\":[{\"Id\":\"TRANSFORMATION\",\"Severity\":\"LOGGER_SEVERITY_DEFAULT\"},{\"Id\":\"SOURCE_UNLOAD\",\"Severity\":\"LOGGER_SEVERITY_DEFAULT\"},{\"Id\":\"IO\",\"Severity\":\"LOGGER_SEVERITY_DEFAULT\"},{\"Id\":\"TARGET_LOAD\",\"Severity\":\"LOGGER_SEVERITY_DEFAULT\"},{\"Id\":\"PERFORMANCE\",\"Severity\":\"LOGGER_SEVERITY_DEFAULT\"},{\"Id\":\"SOURCE_CAPTURE\",\"Severity\":\"LOGGER_SEVERITY_DEFAULT\"},{\"Id\":\"SORTER\",\"Severity\":\"LOGGER_SEVERITY_DEFAULT\"},{\"Id\":\"REST_SERVER\",\"Severity\":\"LOGGER_SEVERITY_DEFAULT\"},{\"Id\":\"VALIDATOR_EXT\",\"Severity\":\"LOGGER_SEVERITY_DEFAULT\"},{\"Id\":\"TARGET_APPLY\",\"Severity\":\"LOGGER_SEVERITY_DEFAULT\"},{\"Id\":\"TASK_MANAGER\",\"Severity\":\"LOGGER_SEVERITY_DEFAULT\"},{\"Id\":\"TABLES_MANAGER\",\"Severity\":\"LOGGER_SEVERITY_DEFAULT\"},{\"Id\":\"METADATA_MANAGER\",\"Severity\":\"LOGGER_SEVERITY_DEFAULT\"},{\"Id\":\"FILE_FACTORY\",\"Severity\":\"LOGGER_SEVERITY_DEFAULT\"},{\"Id\":\"COMMON\",\"Severity\":\"LOGGER_SEVERITY_DEFAULT\"},{\"Id\":\"ADDONS\",\"Severity\":\"LOGGER_SEVERITY_DEFAULT\"},{\"Id\":\"DATA_STRUCTURE\",\"Severity\":\"LOGGER_SEVERITY_DEFAULT\"},{\"Id\":\"COMMUNICATION\",\"Severity\":\"LOGGER_SEVERITY_DEFAULT\"},{\"Id\":\"FILE_TRANSFER\",\"Severity\":\"LOGGER_SEVERITY_DEFAULT\"}]},\"LoopbackPreventionSettings\":null,\"PostProcessingRules\":null,\"StreamBufferSettings\":{\"CtrlStreamBufferSizeInMB\":5,\"StreamBufferCount\":3,\"StreamBufferSizeInMB\":8},\"TargetMetadata\":{\"BatchApplyEnabled\":false,\"FullLobMode\":false,\"InlineLobMaxSize\":0,\"LimitedSizeLobMode\":true,\"LoadMaxFileSize\":0,\"LobChunkSize\":0,\"LobMaxSize\":32,\"ParallelApplyBufferSize\":0,\"ParallelApplyQueuesPerThread\":0,\"ParallelApplyThreads\":0,\"ParallelLoadBufferSize\":0,\"ParallelLoadQueuesPerThread\":0,\"ParallelLoadThreads\":0,\"SupportLobs\":true,\"TargetSchema\":\"\",\"TaskRecoveryTableEnabled\":false}}"


I'm updating the task settings to include all the writeable properties that the DescribeReplicationTasks call returns. Even with my changes, this is the only way to get a clean plan since the endpoint returns all properties, even ones that are set to defaults or unused. Perhaps there is some future work in the same vein that would intelligently handle defaults or disabled properties.

As for what actually changed on this line, I added things like BeforeImageSettings and more LogComponent items, and I removed CloudWatchLogGroup, CloudWatchLogStream, and historyTimeslotInMinutes.

atrepca · 2020-09-22T16:02:46Z

@anGie44 @bflad any chance you could give this PR an eye? It would be great to have these read only DMS tasks settings removed, right now creating a task with logging enabled fails with:

Error: InvalidParameterValueException: Task Settings CloudWatchLogGroup or CloudWatchLogStream cannot be modified.
	status code: 400, request id: <REMOVED>

As a workaround we're creating tasks with EnableLogging: false, then enabling and reflecting the changes in code, but it would be great to have this merged, 🙏 and 🙇‍♂️.

burninmedia · 2020-11-02T19:28:35Z

Since this is an active issue, and has many people reporting they are effected. Can someone please get this committed to master? Just 1 more test to pass it looks like we just need a linting test bug squashed.

scottmlikens · 2021-01-05T21:57:34Z

It's a new year; the tests are passing, and this isn't merged yet? Is there anything I can do to help, or should I wait another 6 months?

bobnautic · 2021-01-26T13:40:44Z

I would appreciate it a lot if this gets finally merged

brettcave · 2021-02-03T12:45:09Z

Would be really great to get this merged, closing in on a year and it looks like the community wants this based on 👍 .

robbruce · 2021-02-23T17:39:55Z

@aburgel - are you able to resolve the merge conflicts on aws/resource_aws_dms_replication_task.go ?

aburgel · 2021-02-23T18:08:30Z

just resolved the merge conflicts

robbruce · 2021-03-06T16:45:17Z

@ewbankkit please can be looked at to be in the next release?

YakDriver · 2021-03-24T15:58:50Z

@aburgel Thank you for this PR! I will be looking at it in the near future. I may need to make some minor changes. Coordinate with me if you intend to push to this branch and make sure to pull any changes I've made. Please note that you will still receive all credit for the PR and your code. Thanks again for your interest!

YakDriver

Looks good! 🎉

Acceptance tests in commercial AWS:

--- PASS: TestAccAWSDmsReplicationTask_basic (453.75s)

Acceptance tests in GovCloud:

--- PASS: TestAccAWSDmsReplicationTask_basic (447.48s)

YakDriver · 2021-04-02T01:11:16Z

@aburgel Thanks for your work on this! I apologize it took so long to merge. It looks like AWS regularly adds new settings since between opening the PR and now 3 more were added (see below). That means we'll probably need to keep checking this.

ControlTablesSettings: FullLoadExceptionTableEnabled
ErrorBehavior: RecoverableErrorStopRetryAfterThrottlingMax
FailTaskWhenCleanTaskResourceFailed

aburgel · 2021-04-02T13:32:48Z

Thanks for your work on this! I apologize it took so long to merge.

Thank you for getting it merged @YakDriver !

It looks like AWS regularly adds new settings since between opening the PR and now 3 more were added

Now that we have this pattern in place, it will be a lot easier to keep on top of new settings.

Any tips for getting a faster turn around on PRs? Or is there some other mechanism we can introduce to make these kind of issues easier to manage?

ghost · 2021-04-09T02:41:58Z

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

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

ghost · 2021-05-02T17:10:13Z

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!

aburgel requested a review from a team May 24, 2020 02:02

ghost added size/M Managed by automation to categorize the size of a PR. needs-triage Waiting for first response or review from a maintainer. service/databasemigrationservice tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels May 24, 2020

aburgel commented May 24, 2020

View reviewed changes

aburgel mentioned this pull request May 26, 2020

aws_dms_replication_task replication_task_settings always reports as needing modification #1513

Closed

anGie44 force-pushed the master branch from 54db70d to 48ba2f7 Compare July 21, 2020 18:26

aburgel requested a review from a team as a code owner November 5, 2020 00:21

ienders mentioned this pull request Nov 13, 2020

Support for RabbitMQ in AmazonMQ #16030

Closed

Base automatically changed from master to main January 23, 2021 00:57

YakDriver self-assigned this Mar 24, 2021

YakDriver removed the needs-triage Waiting for first response or review from a maintainer. label Mar 24, 2021

aburgel added 5 commits April 1, 2021 19:11

Suppress readonly properties from DMS ReplicationTaskSettings

bc9c2be

Move logic from diff suppression to state

35b33af

Improve DMS acceptance test

1229f9a

Run terrafmt

f02b209

AWS partition linter

13820ec

YakDriver force-pushed the dms-json-diff branch from d7c5b26 to 13820ec Compare April 1, 2021 23:17

ghost removed size/M Managed by automation to categorize the size of a PR. labels Apr 1, 2021

ghost added size/L Managed by automation to categorize the size of a PR. and removed size/M Managed by automation to categorize the size of a PR. labels Apr 1, 2021

YakDriver added 8 commits April 1, 2021 19:23

r/dms_replication_task: Avoid hardcoded DNS suffix

2cf0ea5

r/dms_replication_task: Remove errant rebase fodder

531f9ef

r/dms_replication_task: Simplify, cleanup names

9030219

tests/dms_replication_task: Reduce duplication of HCL

6474fe8

tests/dms_replication_task: Lint

32d5501

tests/dms_replication_task: Remove quotes

d281a50

r/dms_replication_task: Add changelog

af2c65c

r/dms_replication_task: Minor cleanups

58f2525

YakDriver approved these changes Apr 2, 2021

View reviewed changes

YakDriver added this to the v3.36.0 milestone Apr 2, 2021

YakDriver merged commit 285bd81 into hashicorp:main Apr 2, 2021

aburgel deleted the dms-json-diff branch April 2, 2021 01:06

ghost locked as resolved and limited conversation to collaborators May 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove readonly properties from DMS ReplicationTaskSettings #13476

Remove readonly properties from DMS ReplicationTaskSettings #13476

aburgel commented May 24, 2020

aburgel May 24, 2020

aburgel May 24, 2020

aburgel May 24, 2020

aburgel May 24, 2020

aburgel May 24, 2020

aburgel May 24, 2020

atrepca commented Sep 22, 2020

burninmedia commented Nov 2, 2020

scottmlikens commented Jan 5, 2021 •

edited

bobnautic commented Jan 26, 2021

brettcave commented Feb 3, 2021

robbruce commented Feb 23, 2021

aburgel commented Feb 23, 2021

robbruce commented Mar 6, 2021

YakDriver commented Mar 24, 2021

YakDriver left a comment

YakDriver commented Apr 2, 2021

aburgel commented Apr 2, 2021

ghost commented Apr 9, 2021

ghost commented May 2, 2021

		delete(logging, "CloudWatchLogGroup")
		delete(logging, "CloudWatchLogStream")

Remove readonly properties from DMS ReplicationTaskSettings #13476

Remove readonly properties from DMS ReplicationTaskSettings #13476

Conversation

aburgel commented May 24, 2020

Community Note

aburgel May 24, 2020

Choose a reason for hiding this comment

aburgel May 24, 2020

Choose a reason for hiding this comment

aburgel May 24, 2020

Choose a reason for hiding this comment

aburgel May 24, 2020

Choose a reason for hiding this comment

aburgel May 24, 2020

Choose a reason for hiding this comment

aburgel May 24, 2020

Choose a reason for hiding this comment

atrepca commented Sep 22, 2020

burninmedia commented Nov 2, 2020

scottmlikens commented Jan 5, 2021 • edited

bobnautic commented Jan 26, 2021

brettcave commented Feb 3, 2021

robbruce commented Feb 23, 2021

aburgel commented Feb 23, 2021

robbruce commented Mar 6, 2021

YakDriver commented Mar 24, 2021

YakDriver left a comment

Choose a reason for hiding this comment

YakDriver commented Apr 2, 2021

aburgel commented Apr 2, 2021

ghost commented Apr 9, 2021

ghost commented May 2, 2021

scottmlikens commented Jan 5, 2021 •

edited