New resource: aws_ssoadmin_customer_managed_policy_attachment

[oakbramble]

trafficstars

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

Closes #25904

This is my first contribution and first time working with Go, so thank you in advance for your feedback. I have added aws_ssoadmin_customer_managed_policy_attachment and manually tested that it is working, creating the following resource:

resource "aws_ssoadmin_customer_managed_policy_attachment" "example" {
  customer_managed_policy_name         = aws_iam_policy.example.name
  customer_managed_policy_path         = "/" # optional
  instance_arn       = tolist(data.aws_ssoadmin_instances.example.arns)[0]
  permission_set_arn = aws_ssoadmin_permission_set.example.arn
}

I based the acceptance tests on managed_policy_attachment_test.go. They are passing (see below), however there are a couple of areas I am unsure about:

• I have used some random names in the test resources being created, to avoid conflicts, however am unsure if I'm doing this properly?
• Not all of the resources are being destroyed after the tests, I assume this could be related to the above and testAccCheckCustomerManagedPolicyAttachmentDestroy?

Update: The tests are working and all resources being destroyed properly. I have marked the PR ready for review

Thanks for bearing with me as I get used to the repo and I hope this PR is helpful.

Output from acceptance testing:

$ make testacc TESTS=TestAccSSOAdminCustomerManagedPolicyAttachment PKG=ssoadmin

==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/ssoadmin/... -v -count 1 -parallel 20 -run='TestAccSSOAdminCustomerManagedPolicyAttachment'  -timeout 5m
=== RUN   TestAccSSOAdminCustomerManagedPolicyAttachment_basic
=== PAUSE TestAccSSOAdminCustomerManagedPolicyAttachment_basic
=== RUN   TestAccSSOAdminCustomerManagedPolicyAttachment_forceNew
=== PAUSE TestAccSSOAdminCustomerManagedPolicyAttachment_forceNew
=== RUN   TestAccSSOAdminCustomerManagedPolicyAttachment_disappears
--- PASS: TestAccSSOAdminCustomerManagedPolicyAttachment_disappears (27.91s)
=== RUN   TestAccSSOAdminCustomerManagedPolicyAttachment_Disappears_permissionSet
--- PASS: TestAccSSOAdminCustomerManagedPolicyAttachment_Disappears_permissionSet (22.14s)
=== RUN   TestAccSSOAdminCustomerManagedPolicyAttachment_multipleManagedPolicies
=== PAUSE TestAccSSOAdminCustomerManagedPolicyAttachment_multipleManagedPolicies
=== CONT  TestAccSSOAdminCustomerManagedPolicyAttachment_basic
=== CONT  TestAccSSOAdminCustomerManagedPolicyAttachment_multipleManagedPolicies
=== CONT  TestAccSSOAdminCustomerManagedPolicyAttachment_forceNew
--- PASS: TestAccSSOAdminCustomerManagedPolicyAttachment_basic (31.19s)
--- PASS: TestAccSSOAdminCustomerManagedPolicyAttachment_multipleManagedPolicies (51.96s)
--- PASS: TestAccSSOAdminCustomerManagedPolicyAttachment_forceNew (56.06s)
PASS
ok  	github.com/hashicorp/terraform-provider-aws/internal/service/ssoadmin	106.228s

[oakbramble]

Ok, I had the tests working, however now I am having trouble attaching multiple policies, both when testing manually and in the acc tests.

  Error: error attaching Customer Managed Policy to SSO Permission Set (arn:aws:sso:::permissionSet/ssoins-68049fb1eee8a70d/ps-69539f9a26bf4698): ConflictException: Conflicting operation occurred on Permission Set with Id: ps-69539f9a26bf4698 and SSOInstanceArn: arn:aws:sso:::instance/ssoins-68049fb1eee8a70d.

It has a conflict when adding or deleting more than one aws_ssoadmin_customer_managed_policy_attachment resource at the same time. Adding them sequentially is ok. It's producing a ConflictException when calling AttachCustomerManagedPolicyReferenceToPermissionSet for the second policy attachment.

Any advice on how I can make sure these calls are handled properly, so they do not cause conflict, would be much apprecated.

Update: I think this was an issue in my environment. Everything is working now and I have tested repeatedly, both manually and the acc tests.

[csanders-git]

Tested with your branch and I can reproduce the same error on both additions and deletions. When adding or detaching multiple CMPolicies. This is def a raise condition documented in the code.

│ Error: error detaching Customer Managed Policy (potato2) from SSO Permission Set (arn:aws:sso:::permissionSet/ssoins-722321efc6569e0f/ps-e49642a3f1f3d8ca): ConflictException: Conflicting operation occurred on Permission Set with Id: ps-e49642a3f1f3d8ca and SSOInstanceArn: arn:aws:sso:::instance/ssoins-722321efc6569e0f.

The suggested mitigation in the AWS docs is to perform a backoff and retry (https://github.com/aws/aws-sdk-go/blob/ea9ec95e2434772e82b415093f9b660bd849322f/service/ssoadmin/api.go#L92).

Initial testing has shown the nicer solution might be to querying ListCustomerManagedPolicyReferencesInPermissionSetInput until the result is returned (or not returned), with some sane limits on the amount of times this would be tried before returning the conflict error to the user.

       input := &ssoadmin.ListCustomerManagedPolicyReferencesInPermissionSetInput{
                InstanceArn: aws.String(instanceArn),
                MaxResults: aws.Int64(1),
                PermissionSetArn: aws.String(permissionSetArn),
       }

[oakbramble]

Thanks @csanders-git for taking a look and providing feedback. I have added retry conditions to the create and delete attachment functions (I know you mention a nicer solution, but this is what I could get working). I find that the tests are now consistently passing and with manual testing I can attach and detach multiple customer managed policies at the same time.

(Side note: I do wonder why this conflict doesn't seem to be an issue for the AWS managed policy attachments, unless I am missing a different retry logic around them?)

Please let me know if there is anything else you see issue with, or if you do not find that this solution for multiple attachments is working.

[nomeelnoj]

The AWS CLI and API docs both use a config block for the CustomerManagedPolicyReference. We should do the same here (easy to extend in the future if/when they allow searching for the policy by tags, etc.)

I also think we should call it what it is, which is a reference to a policy. Please also make sure in the docs it is clear that in order for this to work, the customer managed policy has to first be present in the account that is getting assigned to the permission set.

resource "aws_ssoadmin_customer_managed_policy_attachment" "example" {
  customer_managed_policy_reference { # This should end in `reference`, as well as be a config block
    name         = aws_iam_policy.example.name
    path         = aws_iam_policy.example.path # Optional
  }
  instance_arn       = tolist(data.aws_ssoadmin_instances.example.arns)[0]
  permission_set_arn = aws_ssoadmin_permission_set.example.arn
}

This would be more inline with the AWS API docs and be easier to understand and support.

Also, doing it with a config block allows for better terraform logic with the ability to use dynamics to find and attach policies only for permission sets that are deployed in accounts where the policies exist.

[paulschwarzenberger]

(Side note: I do wonder why this conflict doesn't seem to be an issue for the AWS managed policy attachments, unless I am missing a different retry logic around them?)

@oakbramble thank you so much for working on this, I'll be using the new feature as soon as it's released!

I believe I've seen the equivalent issue with AWS managed policy attachments, resulting in timeouts and failure to apply when trying to attach managed policies to large numbers of accounts and groups at the same time via a module. I worked around the issue by adding an otherwise unnecessary for_each statement to the resource.

[oakbramble]

Thanks for the feedback @nomeelnoj . I had been thinking from a user perspective it would be more readable without a config block, particularly when path will likely not be used so often, but I understand the reasoning to make it more like the API.

I have not had much time to look at this today, however have reconfigured to use the config block and tested manually that this is working (basic, multiple policies, force new and delete). I have not sorted the tests or docs yet, but will look at them tomorrow. In the meantime, I have pushed the WIP, just in case there are any further tips forthcoming :smile: (I am getting to grips with the repo/ Go, thank you for bearing with me.)

@paulschwarzenberger thanks for the info about the managed policy attachments - I'll watch out for that :)

[oakbramble]

Following @nomeelnoj 's comments, there is now a functioning customer_managed_policy_reference config block and the tests have been updated to reflect this.

@ColinHarrington thanks for the review. I will look at removing the path default and updating documentation tomorrow morning.

[oakbramble]

Ok, docs have been updated and all looks to be in order. To update on the initial post, this is what the final resource looks like:

resource "aws_ssoadmin_customer_managed_policy_attachment" "example" {
  instance_arn       = tolist(data.aws_ssoadmin_instances.example.arns)[0]
  permission_set_arn = aws_ssoadmin_permission_set.example.arn
  customer_managed_policy_reference {
    name       = aws_iam_policy.example.name
    path         = "/" # optional
  }
}

This is the current output from the acceptance testing:

$ make testacc TESTS=TestAccSSOAdminCustomerManagedPolicyAttachment PKG=ssoadmin

==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/ssoadmin/... -v -count 1 -parallel 20 -run='TestAccSSOAdminCustomerManagedPolicyAttachment'  -timeout 180m
=== RUN   TestAccSSOAdminCustomerManagedPolicyAttachment_basic
=== PAUSE TestAccSSOAdminCustomerManagedPolicyAttachment_basic
=== RUN   TestAccSSOAdminCustomerManagedPolicyAttachment_forceNew
=== PAUSE TestAccSSOAdminCustomerManagedPolicyAttachment_forceNew
=== RUN   TestAccSSOAdminCustomerManagedPolicyAttachment_disappears
--- PASS: TestAccSSOAdminCustomerManagedPolicyAttachment_disappears (22.10s)
=== RUN   TestAccSSOAdminCustomerManagedPolicyAttachment_Disappears_permissionSet
--- PASS: TestAccSSOAdminCustomerManagedPolicyAttachment_Disappears_permissionSet (49.79s)
=== RUN   TestAccSSOAdminCustomerManagedPolicyAttachment_multipleManagedPolicies
=== PAUSE TestAccSSOAdminCustomerManagedPolicyAttachment_multipleManagedPolicies
=== CONT  TestAccSSOAdminCustomerManagedPolicyAttachment_basic
=== CONT  TestAccSSOAdminCustomerManagedPolicyAttachment_multipleManagedPolicies
=== CONT  TestAccSSOAdminCustomerManagedPolicyAttachment_forceNew
--- PASS: TestAccSSOAdminCustomerManagedPolicyAttachment_basic (24.02s)
--- PASS: TestAccSSOAdminCustomerManagedPolicyAttachment_multipleManagedPolicies (39.16s)
--- PASS: TestAccSSOAdminCustomerManagedPolicyAttachment_forceNew (42.87s)
PASS
ok  	github.com/hashicorp/terraform-provider-aws/internal/service/ssoadmin	114.810s

[oakbramble]

After the pipeline tests were run last week, I had some errors to correct (sorry I did not see all these extra tests in the make file beforehand). Below is summary of the corrections I have made, based on these errors.

• Remove a trailing whitespace in documentation (here)
• Switch from depreciated acctest.ProviderFactories to acctest.ProtoV5ProviderFactories (here)
• Add TimedOut to a Retry and remove unnecessary type conversions (here)
• Correct import statement in docs (here)
• Update tf code in tests to pass terrafmt (here)

In addition to the acc tests, the following are now also passing when I run locally:

• make semgrep
• terrafmt
• make fmt
• make importlint
• make golang-ci-lint
• make docs-lint

I'm having trouble running make providerlint however the provider lint tests did pass in the pipeline already.

Please let me know if there are any more concerns, or if there's anything else I should be doing to get this merged.

(I know the branch doesn't match the naming convention, but I didn't see this until it was too late; will do better next time.)

[csanders-git]

I have retested the latest version of the PR, it works flawlessly.

The contribution guidelines indicate that comments are also an important part of prioritizing PRs, so here we go:

Let me take a stab at discussing why this is so critical to many orgs. AWS-SSO allows for a proper SCIM based integration with IdP's such as Okta. This is only available in a hacky manner via the IAM SAML integration because it doesn't speak SCIM AND it doesn't provide an OIDC Authorization Server for accessing credentials via the CLI which leads to terrible command line tools to assume roles via CLI. The most recent change corrects a long standing issue with AWS-SSO that effectively made it incompatible with how IAM is supposed to be used in a secure way. Traditionally, IAM encourages organizations to make fine grained reusable policies (customer managed policies) to augment the preset AWS managed policies and provide fine-grained, least priv access.
AWS-SSO broke this model because it (previously) ONLY supported inline (considered to be bad security practice) and AWS Managed Policies. Not only did this make it difficult for most organizations that were properly using IAM to migrate, it also meant there was a functional limit on the size of policies that was different from IAM (i.e one inline policy can only hold N chars where N is less than N*10 customer managed policies). AWS fixed this issue recently by allowing (this admittedly sorta hacky solution) to let users associate existing customer managed policies with a permission set (translation between SCIM groups/users and Roles).

This change, that @oakbramble so quickly added support for, frees up many organizations to migrate to AWS-SSO and GREATLY improve their security posture.

[tom-henderson]

Going to add another vote here to help prioritise this. Naturally AWS released this feature about 2 weeks after we complete our migration to SSO managed inline policies, but this would still be very useful to us.

[josemdav]

In addition to what @csanders-git mentions, another business need that support for customer managed policies in permission sets is providing flexibility to application owners / developers to directly manage the permissions at the AWS account level without having to modify the centrally defined permission set. For organizations in which an internal shared responsibility model is implemented, this adds agility.

[bgupta]

Please note that many AWS customers were waiting for Amazon to release this feature, and many are now blocked from deploying if they are Terraform customers, even if they don't realize it yet. The inability to manage AWS permissions in Okta via Terraform has been a long-standing pain point, and we're very much looking forward to seeing this feature implemented!

[c0mput3rj0n3s]

This is a huge deal for us as we're in the midst of a migration from IAM users to AWS SSO in our org. We can't be fine-grained with the SSO inline policies, and we easily bust the 10KB limits if we try, and this feature appearing when it did would have been excellent had the Terraform provider been updated to support it. This would solve so many problems that I'd say it's desperately needed.

[raizyr]

I am in the same boat as @c0mput3rj0n3s. We are trying to migrate from IAM to AWS SSO and have similar concerns. This terraform resource is badly needed.

[nocode99]

A hacky workaround is to use a null-resource and use the AWS CLI. The caveat is if you need to delete something, you need to do it manually.

resource "null_resource" "cmp" {
  for_each = local.cmp_aws_policies

  triggers = {
    new_policies = each.value
  }

  provisioner "local-exec" {
    command = "aws sso-admin attach-customer-managed-policy-reference-to-permission-set --instance-arn ${local.aws_ssoadmin_instance_arn} --permission-set-arn ${aws_ssoadmin_permission_set.pm.arn} --customer-managed-policy-reference Name=${each.value}"
  }
}

FWIW - AWS does NOT validate whether the CMP's exist or not.

[tommyalatalo]

Is there anything preventing the merge of this PR as it is now? We along with many others are in rather urgent need of getting this into the provider and I'm personally trying to fend off implementing custom permissions manually via the management console as best I can in hoping that this will be pushed out soon.

[oakbramble]

Thanks @csanders-git for kicking off the comments to get this noticed. If there's anything I should or could be doing to get this noticed, or anything I have missed then I am absolutely here to do that. I know the branch name isn't in the convention (as I already mentioned, I missed this when starting the PR) but as far as I'm aware, there's nothing I can do about that.

To add to the comments above, I am also blocked at work by this resource not being available. I have been waiting for this feature to come to SSO for months as the inline policy limit is hugely restrictive when working with fine-grained permissions (and is just generally not a great way to do things). Since starting this PR I have been working on refactoring the Terraform for our internal platform and these policy attachments are now the only thing remaining. It would be a real shame to have to go and do all of this work manually and untracked in the console whilst waiting for this to become available in the TF provider.

[oakbramble]

Ok, the tests were triggered. The only check that failed was the markdown links, but that looks to be because the link to the contribution pages has changed in the past weeks and this has already been fixed throughout the provider. I can't see anything failing that is actually to do with the PR. (Please correct me if I am missing something.)

If anyone has advice on what I should do next, please let me know. Should I rebase on the current main to pass this test? (And then keep checking if I need to rebase again as I don't know when the tests will next be run..?)

Update: I will rebase so it passes this MD links test and then tag whoever ran the tests. If anyone notices something else that won't be solved by the rebase, please let me know. Thanks!

[oakbramble]

Thanks for the comments @bschaatsbergen :relaxed: I have updated everything you mentioned. If you have any further feedback, just let me know.

[oakbramble]

Ok, the semgrep check failed on the last run. I saw that several semgrep changes were merged yesterday and it looks like that was the cause of the failure. I have rebased again to remedy this. As ever, if you think that isn't the case and I have missed something, please let me know.

[bschaatsbergen]

Nice work @oakbramble :+1:

[github-actions[bot]]

This functionality has been released in v4.30.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. Thank you!

[github-actions[bot]]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.