terraform-aws-sso icon indicating copy to clipboard operation
terraform-aws-sso copied to clipboard

All scope limited changes trigger destroy and create of all account assignments

Open codehopper-uk opened this issue 2 years ago • 5 comments

Found a bug? Maybe our Slack Community can help.

Slack Community

Describe the Bug

The bug seems to be that terraform wants to destroy and recreate all account assignments even if only one policy statement, assigned to one account is changed. In our case, we have 34 assignments and all 34 assignments are planned for destroy and creation with every change. Could this be due to the fact that the assignments are defined in a list, any changes to which triggers a recreation of the listed resources?

Expected Behavior

We expected only the policy and if necessary, the account(s) assigned to plan for changes, not all accounts.

Steps to Reproduce

Steps to reproduce the behavior:

  1. Define multiple permission sets:
module "permission_sets" {
  source  = "cloudposse/sso/aws//modules/permission-sets"
  version = "0.6.2"

  permission_sets = [
    { 
      name               = local.permission_sets.standard,
      session_duration   = local.session_duration,
      description        = local.description,
      relay_state        = local.relay_state,
      tags               = local.tags,
      inline_policy      = data.aws_iam_policy_document.abacv1.json,
      policy_attachments = ["arn:aws:iam::aws:policy/ReadOnlyAccess"]
    },
    {
      name               = local.permission_sets.billing_read,
      session_duration   = local.session_duration,
      description        = local.description,
      relay_state        = local.relay_state,
      tags               = local.tags,
      inline_policy      = data.aws_iam_policy_document.billing_readonly.json,
      policy_attachments = []
    },
    {
      name               = local.permission_sets.budget,
      session_duration   = local.session_duration,
      description        = local.description,
      relay_state        = local.relay_state,
      tags               = local.tags,
      inline_policy      = data.aws_iam_policy_document.budget_management.json,
      policy_attachments = []
    }
  ]
}
  1. Define multiple account assignments:
module "account_assignments" {
  source     = "cloudposse/sso/aws//modules/account-assignments"
  version    = "0.6.2"
  depends_on = [module.permission_sets]

  account_assignments = [
   { 
      name               = local.permission_sets.admin,
      session_duration   = local.session_duration,
      description        = local.description,
      relay_state        = local.relay_state,
      tags               = local.tags,
      inline_policy      = "",
      policy_attachments = ["arn:aws:iam::aws:policy/AdministratorAccess"]
    },
    {
      account             = local.accounts.production,
      permission_set_arn  = module.permission_sets.permission_sets[local.permission_sets.admin].arn,
      permission_set_name = local.permission_sets.admin,
      principal_type      = local.principal_type,
      principal_name      = local.principal_names.devops
    },
    {
      account             = local.accounts.staging,
      permission_set_arn  = module.permission_sets.permission_sets[local.permission_sets.admin].arn,
      permission_set_name = local.permission_sets.admin,
      principal_type      = local.principal_type,
      principal_name      = local.principal_names.devops
    }
  ]
}
  1. Plan and Apply terraform
  2. change a single policy doc
  3. Plan shows all account assignments are to be replaced, convoluting the true change in the plan, which is the single policy change
# module.account_assignments.aws_ssoadmin_account_assignment.this["XXXXXXXXXX"XXXXXX-G-aws-sso-devops-AdministratorAccess"] must be replaced
-/+ resource "aws_ssoadmin_account_assignment" "this" {
      ~ id                 = "XXXXXXXXXX"XXXXXX,AWS_ACCOUNT,arn:aws:sso:::permissionSet/ssoins-72238ef87c17d7ab/ps-XXXXXXXXXX"XXXXXX,arn:aws:sso:::instance/ssoins-XXXXXXXXXX"XXXXXX" -> (known after apply)
      ~ instance_arn       = "arn:aws:sso:::instance/ssoins-XXXXXXX" -> (known after apply) # forces replacement
      ~ principal_id       = "XXXXXXXXXX"XXXXXX -> (known after apply) # forces replacement
        # (4 unchanged attributes hidden)
    }

The above replacement plan is displayed for all account assignments, even though the policy change affects a lesser number.

Screenshots

If applicable, add screenshots or logs to help explain your problem. In our implementation, we have 34 account assignments and upon simply commenting out some lines in one policy assigned in one account, we see a plan to destroy 34 and add 34 resources, whilst registering the change to a single policy: Plan: 34 to add, 1 to change, 34 to destroy.

Environment (please complete the following information):

multi-account, multi-environment

Anything that will help us triage the bug will help. Here are some ideas:

  • Terraform required_version = ">= 1.1.0"
  • Module version "0.6.2"

Additional Context

Add any other context about the problem here.

codehopper-uk avatar Jul 01 '22 10:07 codehopper-uk

This issue might be related https://github.com/hashicorp/terraform-provider-aws/issues/22188

jagnk avatar Aug 24 '22 12:08 jagnk

This issue is caused by the depends_on behavior when used on a module with data sources. To fix the issue remove in the account_assignments module the depends_on line:

depends_on = [module.permission_sets]

It's anyway not needed as the module.permission_sets is used for the variables so that module is anyway directly dependet.

See #33 my use case of this issue which I could not solve but direct dependency.

Reference: https://itnext.io/beware-of-depends-on-for-modules-it-might-bite-you-da4741caac70

Suggest to close this issue as it's a behavior of terraform and not a bug in the module.

simonweil avatar Oct 31 '22 21:10 simonweil

@codehopper-uk thoughts on @simonweil's response?

Gowiem avatar May 26 '23 16:05 Gowiem

This issue should be closed as #33 is merged

simonweil avatar May 28 '23 21:05 simonweil

@simonweil that looks legit. Closing this out -- @codehopper-uk or @jagnk, if #33 doesn't fix your issue, please open another! Thanks all!

Gowiem avatar May 29 '23 14:05 Gowiem

Closing as fixed by #33

Nuru avatar Nov 12 '24 23:11 Nuru