Skip to content
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

Assignment multiple users or groups via aws_ssoadmin_account_assignment #18739

Open
daimon243 opened this issue Apr 10, 2021 · 11 comments · May be fixed by #23292
Open

Assignment multiple users or groups via aws_ssoadmin_account_assignment #18739

daimon243 opened this issue Apr 10, 2021 · 11 comments · May be fixed by #23292
Labels
enhancement Requests to existing resources that expand the functionality or scope. service/ssoadmin Issues and PRs that pertain to the ssoadmin service.

Comments

@daimon243
Copy link

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue 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 issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

Description

I use aws_ssoadmin_account_assignment resource like this:

resource aws_ssoadmin_account_assignment assignment {
  for_each = toset(var.list_users)
  instance_arn = tolist(data.aws_ssoadmin_instances.sso.arns)[0]
  principal_type = "USER"
  target_id = var.target_account_id
  target_type = "AWS_ACCOUNT"
  permission_set_arn = var.permission_set_arn
  principal_id = each.value
}

When apply multiple user assignments via use "for_each" for resource "aws_ssoadmin_account_assignment", created send-receive request for every "principal_id" .

The every request contain all assignment for permission_set in target_account. In my case for 1000+ users one request have above 300Kbytes and paginator splitting one request to 20 pages. For all users traffic more then 300 Mbytes and approximately 20k pagination token for any assignments change and more than 5 hours in progress.

terraform apply failed with some error message like:
  • "Rate exceeded"
  • "Error: error reading SSO Account Assignment for Principal ...: ValidationException: Invalid pagination token"

After splitting all users by blocks for 100 users on some time receive error: "Error: error reading SSO Account Assignment for Principal ...: ValidationException: Invalid pagination token"

According to https://docs.aws.amazon.com/sdk-for-go/api/service/ssoadmin/#ListAccountAssignmentsInput do not have filter by "principal_id". Therefore need join multiple assignments to one aws_ssoadmin_account_assignment resource.

New or Affected Resource(s)

  • aws_ssoadmin_account_assignment

Potential Terraform Configuration

# assignment users
resource aws_ssoadmin_account_assignment assignment {
  instance_arn = tolist(data.aws_ssoadmin_instances.sso.arns)[0]
  principal_type = "USER"
  target_id = var.target_account_id
  target_type = "AWS_ACCOUNT"
  permission_set_arn = var.permission_set_arn
  principal_id = [user1,user2,user3,...userN]
}

References

  • #0000
@daimon243 daimon243 added the enhancement Requests to existing resources that expand the functionality or scope. label Apr 10, 2021
@ghost ghost added the service/ssoadmin Issues and PRs that pertain to the ssoadmin service. label Apr 10, 2021
@github-actions github-actions bot added the needs-triage Waiting for first response or review from a maintainer. label Apr 10, 2021
@YakDriver
Copy link
Member

@daimon243 Thank you for letting us know about this! While I cannot guarantee when we'll have a chance to work on this, at first glance, your idea of changing principal_id to a list or set, seems plausible. Another idea would be to create a new resource aws_ssoadmin_account_assignments.

@YakDriver YakDriver removed the needs-triage Waiting for first response or review from a maintainer. label Apr 13, 2021
@YakDriver
Copy link
Member

Related #18812

@daimon243
Copy link
Author

daimon243 commented Apr 14, 2021

Yes i think your idea is better, because anyone using "aws_ssoadmin_account_assignment" will not need to fix previously written infrastructure code. I totally agree. Therefore Potential Terraform Configuration some change:

Potential Terraform Configuration

# new resource assignment multiple users
resource aws_ssoadmin_account_assignments assignments {
  instance_arn = tolist(data.aws_ssoadmin_instances.sso.arns)[0]
  principal_type = "USER" /*or "GROUP"*/
  target_id = var.target_account_id
  target_type = "AWS_ACCOUNT"
  permission_set_arn = var.permission_set_arn
  principal_id = [principal_id1, principal_id2, principal_id3,...principal_idN]
}

@gingersnapz
Copy link

I would really love to see an array option for target_id as well. That way when we want to provide access to the same principal across multiple accounts, we can do it in the same resource.

@emanuelflp
Copy link

As an workaround, you can create a loop using for_each or count to iterate over the list and create all mapping.

resource "aws_ssoadmin_account_assignment" "this" {
  for_each = toset(['group1','group2','group3','group4'])
  instance_arn       = tolist(data.aws_ssoadmin_instances.this.arns)[0]
  permission_set_arn = var.permission_set_arn

  principal_id   = each.value
  principal_type = "GROUP"

  target_type = "AWS_ACCOUNT"
  target_id   = var.target_account_id
}

@mattrobinsonsre
Copy link
Contributor

I've raised a PR to implement an aws_ssoadmin_account_assignments resource as our iac would benefit from this.
Support for that PR from anybody following this issue would be helpful.

mattrobinsonsre added a commit to mattrobinsonsre/terraform-provider-aws that referenced this issue Feb 19, 2022
…icorp/terraform-provider-aws/blob/main/docs/contributing --->

<!--- Please keep this note for the community --->

* Please vote on this pull request by adding a 👍 [reaction](https://blog.github.com/2016-03-10-add-reactions-to-pull-requests-issues-and-comments/) 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

<!--- Thank you for keeping this note for the community --->

<!--- If your PR fully resolves and should automatically close the linked issue, use Closes. Otherwise, use Relates --->
Closes hashicorp#18739

Output from acceptance testing:

<!--
Replace TestAccXXX with a pattern that matches the tests affected by this PR.

Replace ec2 with the service package corresponding to your tests.

For more information on the `-run` flag, see the `go test` documentation at https://tip.golang.org/cmd/go/#hdr-Testing_flags.
-->
```
$ make testacc PKG=ssoadmin
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/ssoadmin/... -v -count 1 -parallel 20   -timeout 180m
=== RUN   TestAccSSOAdminAccountAssignment_Basic_group
=== PAUSE TestAccSSOAdminAccountAssignment_Basic_group
=== RUN   TestAccSSOAdminAccountAssignment_Basic_user
=== PAUSE TestAccSSOAdminAccountAssignment_Basic_user
=== RUN   TestAccSSOAdminAccountAssignment_disappears
=== PAUSE TestAccSSOAdminAccountAssignment_disappears
=== RUN   TestAccSSOAdminAccountAssignments_Basic_group
=== PAUSE TestAccSSOAdminAccountAssignments_Basic_group
=== RUN   TestAccSSOAdminAccountAssignments_Basic_user
=== PAUSE TestAccSSOAdminAccountAssignments_Basic_user
=== RUN   TestAccSSOAdminInstancesDataSource_basic
=== PAUSE TestAccSSOAdminInstancesDataSource_basic
=== RUN   TestAccSSOAdminManagedPolicyAttachment_basic
=== PAUSE TestAccSSOAdminManagedPolicyAttachment_basic
=== RUN   TestAccSSOAdminManagedPolicyAttachment_forceNew
=== PAUSE TestAccSSOAdminManagedPolicyAttachment_forceNew
=== RUN   TestAccSSOAdminManagedPolicyAttachment_disappears
--- PASS: TestAccSSOAdminManagedPolicyAttachment_disappears (27.91s)
=== RUN   TestAccSSOAdminManagedPolicyAttachment_Disappears_permissionSet
--- PASS: TestAccSSOAdminManagedPolicyAttachment_Disappears_permissionSet (18.53s)
=== RUN   TestAccSSOAdminManagedPolicyAttachment_multipleManagedPolicies
=== PAUSE TestAccSSOAdminManagedPolicyAttachment_multipleManagedPolicies
=== RUN   TestAccSSOAdminPermissionSetDataSource_arn
=== PAUSE TestAccSSOAdminPermissionSetDataSource_arn
=== RUN   TestAccSSOAdminPermissionSetDataSource_name
=== PAUSE TestAccSSOAdminPermissionSetDataSource_name
=== RUN   TestAccSSOAdminPermissionSetDataSource_nonExistent
=== PAUSE TestAccSSOAdminPermissionSetDataSource_nonExistent
=== RUN   TestAccSSOAdminPermissionSetInlinePolicy_basic
=== PAUSE TestAccSSOAdminPermissionSetInlinePolicy_basic
=== RUN   TestAccSSOAdminPermissionSetInlinePolicy_update
=== PAUSE TestAccSSOAdminPermissionSetInlinePolicy_update
=== RUN   TestAccSSOAdminPermissionSetInlinePolicy_disappears
--- PASS: TestAccSSOAdminPermissionSetInlinePolicy_disappears (20.30s)
=== RUN   TestAccSSOAdminPermissionSetInlinePolicy_Disappears_permissionSet
--- PASS: TestAccSSOAdminPermissionSetInlinePolicy_Disappears_permissionSet (20.05s)
=== RUN   TestAccSSOAdminPermissionSet_basic
=== PAUSE TestAccSSOAdminPermissionSet_basic
=== RUN   TestAccSSOAdminPermissionSet_tags
--- PASS: TestAccSSOAdminPermissionSet_tags (55.21s)
=== RUN   TestAccSSOAdminPermissionSet_updateDescription
=== PAUSE TestAccSSOAdminPermissionSet_updateDescription
=== RUN   TestAccSSOAdminPermissionSet_updateRelayState
=== PAUSE TestAccSSOAdminPermissionSet_updateRelayState
=== RUN   TestAccSSOAdminPermissionSet_updateSessionDuration
=== PAUSE TestAccSSOAdminPermissionSet_updateSessionDuration
=== RUN   TestAccSSOAdminPermissionSet_RelayState_updateSessionDuration
=== PAUSE TestAccSSOAdminPermissionSet_RelayState_updateSessionDuration
=== RUN   TestAccSSOAdminPermissionSet_mixedPolicyAttachments
=== PAUSE TestAccSSOAdminPermissionSet_mixedPolicyAttachments
=== CONT  TestAccSSOAdminAccountAssignment_Basic_group
=== CONT  TestAccSSOAdminPermissionSetDataSource_nonExistent
=== CONT  TestAccSSOAdminPermissionSet_updateRelayState
=== CONT  TestAccSSOAdminPermissionSet_basic
=== CONT  TestAccSSOAdminPermissionSet_updateDescription
=== CONT  TestAccSSOAdminManagedPolicyAttachment_basic
=== CONT  TestAccSSOAdminPermissionSetDataSource_arn
=== CONT  TestAccSSOAdminAccountAssignment_disappears
=== CONT  TestAccSSOAdminAccountAssignments_Basic_group
=== CONT  TestAccSSOAdminPermissionSetInlinePolicy_update
=== CONT  TestAccSSOAdminManagedPolicyAttachment_multipleManagedPolicies
=== CONT  TestAccSSOAdminAccountAssignment_Basic_user
=== CONT  TestAccSSOAdminPermissionSetDataSource_name
=== CONT  TestAccSSOAdminPermissionSetInlinePolicy_basic
=== CONT  TestAccSSOAdminPermissionSet_mixedPolicyAttachments
=== CONT  TestAccSSOAdminManagedPolicyAttachment_forceNew
=== CONT  TestAccSSOAdminPermissionSet_updateSessionDuration
=== CONT  TestAccSSOAdminAccountAssignments_Basic_user
=== CONT  TestAccSSOAdminPermissionSet_RelayState_updateSessionDuration
=== CONT  TestAccSSOAdminInstancesDataSource_basic
--- PASS: TestAccSSOAdminPermissionSetDataSource_nonExistent (10.53s)
--- PASS: TestAccSSOAdminInstancesDataSource_basic (22.39s)
--- PASS: TestAccSSOAdminPermissionSetDataSource_arn (30.25s)
--- PASS: TestAccSSOAdminPermissionSet_basic (33.43s)
--- PASS: TestAccSSOAdminAccountAssignment_disappears (41.19s)
--- PASS: TestAccSSOAdminPermissionSetDataSource_name (43.32s)
--- PASS: TestAccSSOAdminPermissionSetInlinePolicy_basic (43.53s)
--- PASS: TestAccSSOAdminAccountAssignments_Basic_group (46.83s)
--- PASS: TestAccSSOAdminAccountAssignment_Basic_user (47.01s)
--- PASS: TestAccSSOAdminManagedPolicyAttachment_basic (50.46s)
--- PASS: TestAccSSOAdminPermissionSet_updateSessionDuration (54.33s)
--- PASS: TestAccSSOAdminPermissionSet_updateDescription (54.34s)
--- PASS: TestAccSSOAdminPermissionSet_updateRelayState (54.41s)
--- PASS: TestAccSSOAdminPermissionSet_RelayState_updateSessionDuration (54.94s)
--- PASS: TestAccSSOAdminPermissionSetInlinePolicy_update (60.59s)
--- PASS: TestAccSSOAdminPermissionSet_mixedPolicyAttachments (61.51s)
--- PASS: TestAccSSOAdminManagedPolicyAttachment_forceNew (70.69s)
--- PASS: TestAccSSOAdminAccountAssignment_Basic_group (76.75s)
--- PASS: TestAccSSOAdminAccountAssignments_Basic_user (76.81s)
--- PASS: TestAccSSOAdminManagedPolicyAttachment_multipleManagedPolicies (91.68s)
PASS
ok  	github.com/hashicorp/terraform-provider-aws/internal/service/ssoadmin	237.127s

```
@gdavison
Copy link
Contributor

HI @daimon243 and everyone else. I've been trying to reproduce this issue, and have not seen similar long times. This is not to say that we don't want to fix this resource, since it can still take a long time. I want to better understand the origin of the problem.

With 1,200 existing users, a plan operation takes approximately 14 minutes. With the 1,200 users and reducing to 1,100, the apply without a refresh takes 185 seconds. Adding back the 100 users (to a total of 1,200) takes a total of 16 minutes including the refresh.

Are you still seeing much longer times as reported above? If so, what version of the provider and terraform are you using? How many users do you have in SSO?

Some things that may be related to the lower times I'm seeing:

  • I think AWS may have made some changes on their side, for instance the read API returns the maximum number per pagination request.
  • I've used an AWS Managed Microsoft AD as the identity source, since I can use the AD API to provision users.

@gdavison
Copy link
Contributor

Hi @gingersnapz. One option you have is to use for_each along with the function setproduct to combine the principal_id and target_id lists into a common list.

@jsimoni
Copy link

jsimoni commented Apr 11, 2022

@gdavison I'm having a similar issue. I've tried using setproduct, but I get

│ The "for_each" value depends on resource attributes that cannot be
│ determined until apply, so Terraform cannot predict how many instances will
│ be created. To work around this, use the -target argument to first apply
│ only the resources that the for_each depends on.

I was going to create a new issue so here is my:

Reproduction Code [Required]

data "aws_organizations_organization" "this" {}

locals {
  account_id_list = data.aws_organizations_organization.this.accounts
}

module "permissionset" {
  source = "../../..//modules/permissionset"

  permission_set_name        = "name"
  permission_set_description = "description"
  awssso_instance_arn        = var.awsinstance_arn
  managed_policies_to_attach = ["arn:aws:iam::aws:policy/job-function/ViewOnlyAccess"]
}

locals {
  account_permissionset_product = setproduct(var.account_id_list[*].id, var.permission_set_arn_list)
}

# associate the group to the permission set(s) & account(s)
resource "aws_ssoadmin_account_assignment" "group_permissionset_account_assignments" {
  for_each = {
    for association in local.account_permissionset_product : "${association[0]}-${association[1]}" => {
      account_id         = association[0]
      permission_set_arn = association[1]
    }
  }
  instance_arn       = var.awssso_instance_arn
  permission_set_arn = each.value.permission_set_arn

  principal_id   = data.aws_identitystore_group.awssso_group.group_id
  principal_type = "GROUP"

  target_id   = sensitive(each.value.account_id)
  target_type = "AWS_ACCOUNT"
}

Additional context

If I use -target as suggested elsewhere (4149) to provision the permission set first, then the assignments works. However, the explanations I've seen for this error is that Terraform is unable to determine the number of resources to provision at plan so it fails. But in my case, while the arns of the permission sets would be unknown, Terraform can determine the number of assignments to create.

@gdavison
Copy link
Contributor

Hi @jsimoni. Unfortunately, this is a limitation of for_each and its interaction with count, and sometimes affects only the first run. Since for_each and count are handled by Terraform core, you should open an issue on the hashicorp/terraform project.

@daimon243
Copy link
Author

Hi @gdavison. I rewrote the infrastructure and instead of one folder with permission sets assigned, I created many folders, divided into small groups. Now, with a total number of users in SSO of about 1000 and assigning rights to 20 employees, I get the maximum execution time from 1 to 6 minutes. In this particular case, there is no problem. With this approach, the request rate limit is not exceeded. But using a list of users as a parameter instead of using for_each seems to be preferable because we can run queries to api in batch.

@gdavison gdavison self-assigned this Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Requests to existing resources that expand the functionality or scope. service/ssoadmin Issues and PRs that pertain to the ssoadmin service.
Projects
None yet
7 participants