resource/aws_ssm_document: Update SSM Document Permission Updates to Clear Permission Before Updating

[brandonstevens]

Reference: #5308

Changes proposed in this pull request:

• Update SSM Document Permission Updates to Clear Permission Before Updating

Output from acceptance testing:

$  make testacc TESTARGS='-run=TestAccAWSSSMDocument'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -run=TestAccAWSSSMDocument -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSSSMDocument_basic
--- PASS: TestAccAWSSSMDocument_basic (20.38s)
=== RUN   TestAccAWSSSMDocument_update
--- PASS: TestAccAWSSSMDocument_update (28.27s)
=== RUN   TestAccAWSSSMDocument_permission_public
--- PASS: TestAccAWSSSMDocument_permission_public (15.73s)
=== RUN   TestAccAWSSSMDocument_permission_private
--- PASS: TestAccAWSSSMDocument_permission_private (15.64s)
=== RUN   TestAccAWSSSMDocument_permission_change
--- PASS: TestAccAWSSSMDocument_permission_change (26.85s)
=== RUN   TestAccAWSSSMDocument_params
--- PASS: TestAccAWSSSMDocument_params (15.34s)
=== RUN   TestAccAWSSSMDocument_automation
--- PASS: TestAccAWSSSMDocument_automation (29.21s)
=== RUN   TestAccAWSSSMDocument_DocumentFormat_YAML
--- PASS: TestAccAWSSSMDocument_DocumentFormat_YAML (27.88s)
=== RUN   TestAccAWSSSMDocument_Tags
--- PASS: TestAccAWSSSMDocument_Tags (38.03s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	217.371s

[bflad]

It seems like we should be performing a difference against the old and new permissions so updates do not intentionally create "downtime", especially if the second ModifyDocumentPermission API call fails.

I would recommend setting this up using something like the below which should perform this update in one API call (in pseudo-code):

if d.HasChange("permissions") {
  o, n := d.GetChange("permissions")
  oldPermissions := o.(map[string]interface{})
  newPermissions := n.(map[string]interface{})
  oldPermissionsAccountIds := make([]string, 0)
  if v, ok := oldPermissions["account_ids"]; ok && v.(string) != "" {
    oldPermissionsAccountIds = strings.Split(v.(string), ",")
  }
  newPermissionsAccountIds := make([]string, 0)
  if v, ok := newPermissions["account_ids"]; ok && v.(string) != "" {
    newPermissionsAccountIds = strings.Split(v.(string), ",")
  }

  accountIdsToRemove := make([]string, 0)
  for _, oldPermissionsAccountId := range oldPermissionsAccountIds {
    if _, contains := sliceContainsString(newPermissionsAccountIds, oldPermissionsAccountId); !contains {
      accountIdsToRemove = append(accountIdsToRemove, oldPermissionsAccountId)
    }
  }
  accountIdsToAdd := make([]string, 0)
  for _, newPermissionsAccountId := range newPermissionsAccountIds {
    if _, contains := sliceContainsString(oldPermissionsAccountIds, newPermissionsAccountId); !contains {
      accountIdsToAdd = append(accountIdsToAdd, newPermissionsAccountId)
    }
  }

  input := &ssm.ModifyDocumentPermissionInput{
    Name:               aws.String(d.Get("name").(string)),
    PermissionType:     aws.String("Share"),
    AccountIdsToAdd: aws.StringSlice(accountIdsToAdd),
    AccountIdsToRemove: aws.StringSlice(accountIdsToRemove),
  }

  log.Printf("[DEBUG] Modifying SSM document permissions: %s", input)
  _, err := ssmconn.ModifyDocumentPermission(input)
  if err != nil {
    return fmt.Errorf("error modifying SSM document permissions: %s", err)
  }
}

[bflad]

Hi @brandonstevens 👋 Thanks for submitting this. I left some initial feedback below. Can you please take a look and let us know if you have any questions or do not have time to implement?

[bflad]

Nitpick: Regardless of other changes, we prefer fmt.Errorf() over errwrap.Wrapf() as we do not need the complexity introduced by errwrap when returning simple error messages. We are trying to clean this up across the codebase. 😄

[bflad]

This check should verify the value of the account IDs either via resource.TestCheckResourceAttr("aws_ssm_document.foo", "permissions.account_ids", initialIds), or writing a new testAccCheck function that verifies it from the API.

[bflad]

We should have another TestStep that adds IDs and verifies they were updated. 👍

[bflad]

Nitpick: Go styling recommends camelCase naming instead of underscores.

[bflad]

LGTM, thanks @brandonstevens! 🚀

9 tests passed (all tests)
--- PASS: TestAccAWSSSMDocument_basic (11.08s)
--- PASS: TestAccAWSSSMDocument_update (15.30s)
--- PASS: TestAccAWSSSMDocument_params (17.94s)
--- PASS: TestAccAWSSSMDocument_automation (21.10s)
--- PASS: TestAccAWSSSMDocument_permission_private (27.91s)
--- PASS: TestAccAWSSSMDocument_DocumentFormat_YAML (28.49s)
--- PASS: TestAccAWSSSMDocument_permission_change (31.82s)
--- PASS: TestAccAWSSSMDocument_Tags (34.76s)
--- PASS: TestAccAWSSSMDocument_permission_public (55.92s)

[bflad]

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

[ghost]

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!