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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent spurious diffs in IAM policies #13813

Closed

Conversation

jbergknoff-rival
Copy link
Contributor

@jbergknoff-rival jbergknoff-rival commented Jun 17, 2020

IAM policies have several fields that are treated as arrays by the AWS API and Terraform, but are actually sets with no meaningful order (examples: actions, principals, resources). As a result, the order of the results returned by the AWS API can vary from API call to API call. This provider makes liberal use of diff_suppress_funcs.suppressEquivalentAwsPolicyDiffs which can suppress such a diff if the policy isn't changing for any other reason. However, if the policy is changing, then we will (correctly) get a plan, but it will include spurious diffs due to the arbitrary ordering of these sets.

One solution, as in #11463, is to normalize the policy document as it's read from AWS, and normalize it as it's serialized to Terraform state. This PR does that for several resources (the ones where I've noted the need, though this is probably not exhaustive).

Here's an example:

provider "aws" {
  region = "us-east-1"
}

resource "aws_iam_role" "test" {
  name = "test-role"

  assume_role_policy = jsonencode(
    {
      Version = "2012-10-17"
      Statement = [
        {
          Action = ["sts:AssumeRole"],
          Effect = "Allow",
          Principal = {
            AWS = [
              # NOTE: These account ids need to be replaced with valid ids for IAM to accept the policy
              "arn:aws:iam::111111111111:root",
              "arn:aws:iam::222222222222:root",
              "arn:aws:iam::333333333333:root",
              "arn:aws:iam::444444444444:root",
              "arn:aws:iam::555555555555:root",
            ]
          }
        }
      ]
    }
  )
}

Apply this, and then remove the middle principal and plan again. Without this patch, the plan has a random element to it, but it can look like:

~ Principal = {
    ~ AWS = [
        + "arn:aws:iam::111111111111:root",
        + "arn:aws:iam::222222222222:root",
          "arn:aws:iam::444444444444:root",
          "arn:aws:iam::555555555555:root",
        - "arn:aws:iam::333333333333:root",
        - "arn:aws:iam::222222222222:root",
        - "arn:aws:iam::111111111111:root",
      ]
  }

With this patch, the plan is:

~ Principal = {
    ~ AWS = [
          "arn:aws:iam::111111111111:root",
          "arn:aws:iam::222222222222:root",
        - "arn:aws:iam::333333333333:root",
          "arn:aws:iam::444444444444:root",
          "arn:aws:iam::555555555555:root",
      ]
  }

It would be ideal to centralize this logic, but I don't see a clear way to do that, and I'd appreciate suggestions.

Also, this PR actually doesn't touch actions and resources in the policy because they're not given any structure in iam_policy_model.go (i.e. just left as interface{}). With some guidance on how this should look, I'd be happy to make that change, as well.

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

Release note for CHANGELOG:

BUG FIXES:
* Suppress spurious diffs due to arbitrary ordering of IAM policies

Output from acceptance testing:

Unfortunately, as in #11463, this doesn't fit the mold of most changes as far as testing goes. I think the most meaningful test would be one that inspects the contents of the plan, and I don't know how to do that with the testing framework.

@jbergknoff-rival jbergknoff-rival requested a review from Jun 17, 2020
@ghost ghost added size/M service/iam service/s3 service/sqs needs-triage labels Jun 17, 2020
@jbergknoff jbergknoff force-pushed the jbergknoff/policy-ordering branch from c503fa5 to 1c26df1 Compare Oct 20, 2020
@shanestarcher-okta
Copy link

@shanestarcher-okta shanestarcher-okta commented Dec 16, 2020

We are attempting to diff plans between resources and due to the current non-deterministic nature, it makes the plan non-viable to diff in these cases. Making it deterministic as this does would I believe solve the problem.

Base automatically changed from master to main Jan 23, 2021
@breathingdust breathingdust requested a review from as a code owner Jan 23, 2021
@ewbankkit
Copy link
Contributor

@ewbankkit ewbankkit commented Jun 15, 2021

Related:

@jbergknoff-rival Thanks for the contribution 馃帀 馃憦.
I am going to close this PR as we would like a more general solution implemented in awspolicyequivalence.

@ewbankkit ewbankkit removed the needs-triage label Jun 15, 2021
@ewbankkit ewbankkit closed this Jun 15, 2021
@github-actions
Copy link

@github-actions github-actions bot commented Jul 16, 2021

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
service/iam service/s3 service/sqs size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants