aws-ia / terraform-aws-iam-identity-center

Apache License 2.0
16 stars 6 forks source link

Account assignments are recreated on each apply #33

Closed shenderson-gala closed 3 weeks ago

shenderson-gala commented 1 month ago

Hello,

I've run into an issue where ALL account assignments are destroyed and recreated every time I add an additional permission set or account assignment to the module configuration. This creates an issue where for a 5ish minute window users lose access but also creates new AWSReservedSSO role names in each account as well which breaks EKS authentication.

I am using existing groups and users synced through an external IdP.

# module.aws-iam-identity-center.aws_ssoadmin_account_assignment.account_assignment["Type:GROUP__Principal:XXX - XXX__Permission:XXX-XXX-QA-S3-RW__Account:XXX"] must be replaced -/+ resource "aws_ssoadmin_account_assignment" "account_assignment" { ~ id = "XXX-XXX-XXX-XXX-XXX,GROUP,XXX,AWS_ACCOUNT,arn:aws:sso:::permissionSet/ssoins-XXX/ps-XXX,arn:aws:sso:::instance/ssoins-XXX" -> (known after apply) ~ permission_set_arn = "arn:aws:sso:::permissionSet/ssoins-XXX/ps-XXX" # forces replacement -> (known after apply)

Any advice on how to avoid this?

Thanks so much, Scott

0011001011 commented 1 month ago

Same issue

Basically, adding (or removing) a user makes all AWSReservedSSO role recreated in each account, thus breaking EKS authentication.

We are using EKS clusters across various accounts, all managed by a single account SSO

hacker65536 commented 1 month ago

WIP

20

https://github.com/aws-ia/terraform-aws-iam-identity-center/pull/20/commits/de009add86329ea475200f8cb0765d3a48d70fdc

novekm commented 1 month ago

Hi there, can you attach some snippets? The following code snippets would be helpful:

  1. Existing Terraform config where module is declared
  2. Subsequent output of terraform plan with no changes made to the config
  3. Changes that are made using the module (e.g. adding an additional permission set or account assignment)
  4. Subsequent output of terraform plan after making changes

Thanks!

smellyspice commented 1 month ago

Simply removing a group from sso_groups breaks it for me as well. It wants to recreate all the module.aws-iam-identity-center.aws_identitystore_group_membership.sso_group_membership resources and all their dependencies.

smellyspice commented 1 month ago

Hey - I seemed to have solved my issue by commenting out all the data block depends_on. Here is a link that describes what's going on better than I could. Hope it helps folks.

novekm commented 1 month ago

Hi @smellyspice,

Can you attach some snippets? The following code snippets would be helpful:

  1. Existing Terraform config where module is declared
  2. Subsequent output of terraform plan with no changes made to the config
  3. Changes that are made using the module (e.g. removing a group from sso_groups variable)
  4. Subsequent output of terraform plan after making changes

Thanks

novekm commented 1 month ago

Also, the reason the depends_on is present for the data sources, is because when you run terraform apply the data sources will attempt to be fetched. If the resource does not yet exist, the apply will fail with an error similar to this:

Error: reading AWS SSO Identity Store Group Data Source (x-000000): operation error identitystore: GetGroupId, https response error StatusCode: 400, RequestID: xx-xx-xx, ResourceNotFoundException: GROUP not found.
│ 
│   with module.aws-iam-identity-center.data.aws_identitystore_group.identity_store_group["Audit"],
│   on ../../data.tf line 56, in data "aws_identitystore_group" "identity_store_group":
│   56: data "aws_identitystore_group" "identity_store_group" {

The reason this doesn't occur for you after making changes is likely because you removed the depends_on after the initial apply, which means the data sources would be able to successfully be able to fetch your remaining sso_groups (all besides what you removed).

smellyspice commented 1 month ago

Hi @novekm - it appears that this module is only meant for 2 use cases:

  1. Manage all users and groups or
  2. Manage no users and no groups

We have use-case 3: Don't manage users, as they are coming in via SCIM from Google as the IdP and because Google doesn't support Groups over SCIM, we need to manage Groups - but not users.

For the above reason, I had to make a bunch of modifications to this module to support our use-case. Thus, providing you with snippets using a hacked module would likely not be helpful.

I understand why the depends_on is there, but as was discussed in the other thread, it is not ideal to use depends_on - especially not in case where Terraform is going to create these resources to begin with.

I have worked around the issue to solve our problem. Perhaps the OP here is in a similar situation or perhaps this module could be made to handle more use-cases and this could prevent others from experiencing this problem.

novekm commented 1 month ago

Hi @smellyspice,

Correct - those were the main module use cases for the initial release. For your second point - yes, the idea was that if using SCIM, users/groups would be managed in the external IdP and synced to IAM IdC. Then from there, the module would reference those existing users and groups to assign permission sets and account assignments. However we are working option 3 - an update to support edge cases like only managing groups and associated user group membership for Google Workspace where SCIM does not currently sync groups, only users.

You're not the first to present this use case - part of the reason I was asking for a code snippet was to get a sense for the way you'd like to be able to use the module for this 🙂 does the example I posted in this PR seem like the functionality you're looking for?

Thanks for the feedback!

skripted-io commented 1 month ago

First of all thanks for providing this module. It's definitely a great start and there aren't many alternatives out there.

However, I'm not sure I understand why the data sources are there in the first place. As others have mentioned, the problem is that the module modifies the same resources it requests information about. The results can be unpredictable.

If the data sources are there so this module can be used with existing resources, then importing those resources into the state before running the initial apply would be a much cleaner approach. Your state will always represent what has been provisioned. It the data sources are there so it supports modifications made outside of Terraform, then I think that's just a bad practice we should stay away from.

I've created a version of this module that eliminates all the data sources and it works just fine. It will never re-recreate account assignments no matter what you add or change to users, groups or permissions. I can share it if anyone's interested.

hacker65536 commented 1 month ago

If a permission set, group, or user is defined in the module, it is referenced; otherwise, the data resource is referenced.

I have modified it this way. https://github.com/aws-ia/terraform-aws-iam-identity-center/pull/20/files#diff-dc46acf24afd63ef8c556b77c126ccc6e578bc87e3aa09a931f33d9bf2532fbbR227-R230

By the way, I am also using an external IdP and would like to manage groups with IAM IdC.

novekm commented 1 month ago

Hi @skripted-io, thanks for the feedback! One of the reasons data sources are used is because the module supports existing users and groups. A common use case is to connect an external IdP (Entra ID/Azure AD, Okta, Google Workspace, etc.) to IAM Identity Center. If using an external IdP, these will always be created outside of Terraform and are synced to IAM IdC via SCIM (with the limitation that Google Workspace SCIM doesn't support syncing of Groups, just users). There are also a number of permission sets created by AWS as part of an AWS Control Tower deployment. We wanted to remove the limitation of having to constantly import these values if changed (e.g. if a user/group is deleted or added in the external IdP).

Further, the upstream AWS API for creating a group membership currently only supports doing this by referencing a GroupId and UserId which are computed values (known after apply). If this API supported GroupName and UserName as the Amazon Cognito API does, we could more easily manage this without using data sources (which fetch the necessary user ids and group ids).

We are currently reviewing @hacker65536's PR and are separately working to add support to manage group membership for users that are synced to IAM IdC, with groups that are created/managed via the module (mainly to get around the Google Workspace limitation). See the conversation in #32 for more info on this.

skripted-io commented 1 month ago

@novekm thanks for clarifying. I had no trouble grabbing the GroupID and UserID this way:

locals {
  group_memberships = flatten([
    for user_key, details in var.sso_users : [
      for group in details.group_membership : {
        user_key   = user_key
        group_name = group
      }
    ]
  ])
}

# - Identity Store Group Membership -
resource "aws_identitystore_group_membership" "sso_group_membership" {
  for_each          = { for gm in local.group_memberships : "${gm.user_key}_${gm.group_name}" => gm }
  identity_store_id = local.sso_instance_id
  group_id          = aws_identitystore_group.sso_groups[each.value.group_name].group_id
  member_id         = aws_identitystore_user.sso_users[each.value.user_key].user_id
}
skripted-io commented 1 month ago

If a permission set, group, or user is defined in the module, it is referenced; otherwise, the data resource is referenced.

I have modified it this way. https://github.com/aws-ia/terraform-aws-iam-identity-center/pull/20/files#diff-dc46acf24afd63ef8c556b77c126ccc6e578bc87e3aa09a931f33d9bf2532fbbR227-R230

By the way, I am also using an external IdP and would like to manage groups with IAM IdC.

@hacker65536 yeah I saw that. Just ran a quick test with it and it seems to work. I'll keep an eye on the PR.

novekm commented 3 weeks ago

@hacker65536 Thanks for your contribution! These changes have been merged and are in v0.0.3 🚀 For everyone else, please update to the latest version and the error should be resolved.

novekm commented 1 week ago

Hi @smellyspice, just wanted to follow up - the functionality you were looking for to handle the unique scenario of how SCIM works with Google Workspace has been added in v0.0.5 which was released today 🙂