From 378fea60af361ae4b782c0c4bdb828ac06dabfca Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Tue, 7 Dec 2021 21:48:23 -0500 Subject: [PATCH 1/5] iam/role: Fix issue with non-diffs --- internal/service/iam/role.go | 83 +++++++++++++++++++++++++++++++++--- 1 file changed, 78 insertions(+), 5 deletions(-) diff --git a/internal/service/iam/role.go b/internal/service/iam/role.go index aec0d9344b1b..59c8216397e7 100644 --- a/internal/service/iam/role.go +++ b/internal/service/iam/role.go @@ -10,6 +10,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/iam" "github.com/hashicorp/aws-sdk-go-base/tfawserr" + awspolicy "github.com/hashicorp/awspolicyequivalence" "github.com/hashicorp/go-multierror" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" @@ -126,7 +127,7 @@ func ResourceRole() *schema.Resource { Schema: map[string]*schema.Schema{ "name": { Type: schema.TypeString, - Optional: true, + Optional: true, // semantically required but syntactically optional to allow empty inline_policy ValidateFunc: validation.All( validation.StringIsNotEmpty, validRolePolicyName, @@ -134,12 +135,19 @@ func ResourceRole() *schema.Resource { }, "policy": { Type: schema.TypeString, - Optional: true, + Optional: true, // semantically required but syntactically optional to allow empty inline_policy ValidateFunc: verify.ValidIAMPolicyJSON, DiffSuppressFunc: verify.SuppressEquivalentPolicyDiffs, }, }, }, + DiffSuppressFunc: func(k, _, _ string, d *schema.ResourceData) bool { + if d.Id() == "" { + return false + } + + return !inlinePoliciesActualDiff(d) + }, }, "managed_policy_arns": { @@ -292,8 +300,16 @@ func resourceRoleRead(d *schema.ResourceData, meta interface{}) error { if err != nil { return fmt.Errorf("reading inline policies for IAM role %s, error: %s", d.Id(), err) } - if err := d.Set("inline_policy", flattenRoleInlinePolicies(inlinePolicies)); err != nil { - return fmt.Errorf("error setting inline_policy: %w", err) + + var configPoliciesList []*iam.PutRolePolicyInput + if v := d.Get("inline_policy").(*schema.Set); v.Len() > 0 { + configPoliciesList = expandRoleInlinePolicies(aws.StringValue(role.RoleName), v.List()) + } + + if !inlinePoliciesEquivalent(inlinePolicies, configPoliciesList) { + if err := d.Set("inline_policy", flattenRoleInlinePolicies(inlinePolicies)); err != nil { + return fmt.Errorf("error setting inline_policy: %w", err) + } } managedPolicies, err := readRolePolicyAttachments(conn, aws.StringValue(role.RoleName)) @@ -393,18 +409,22 @@ func resourceRoleUpdate(d *schema.ResourceData, meta interface{}) error { } } - if d.HasChange("inline_policy") { + if d.HasChange("inline_policy") && inlinePoliciesActualDiff(d) { roleName := d.Get("name").(string) + o, n := d.GetChange("inline_policy") + if o == nil { o = new(schema.Set) } + if n == nil { n = new(schema.Set) } os := o.(*schema.Set) ns := n.(*schema.Set) + remove := os.Difference(ns).List() add := ns.Difference(os).List() @@ -793,3 +813,56 @@ func readRoleInlinePolicies(roleName string, meta interface{}) ([]*iam.PutRolePo return apiObjects, nil } + +func inlinePoliciesActualDiff(d *schema.ResourceData) bool { + roleName := d.Get("name").(string) + o, n := d.GetChange("inline_policy") + if o == nil { + o = new(schema.Set) + } + if n == nil { + n = new(schema.Set) + } + + os := o.(*schema.Set) + ns := n.(*schema.Set) + + osPolicies := expandRoleInlinePolicies(roleName, os.List()) + nsPolicies := expandRoleInlinePolicies(roleName, ns.List()) + + if !inlinePoliciesEquivalent(osPolicies, nsPolicies) { + return true + } + + return false +} + +func inlinePoliciesEquivalent(one, two []*iam.PutRolePolicyInput) bool { + if one == nil && two == nil { + return true + } + + if len(one) != len(two) { + return false + } + + matches := 0 + + for _, policyOne := range one { + for _, policyTwo := range two { + if aws.StringValue(policyOne.PolicyName) == aws.StringValue(policyTwo.PolicyName) { + matches++ + if equivalent, err := awspolicy.PoliciesAreEquivalent(aws.StringValue(policyOne.PolicyDocument), aws.StringValue(policyTwo.PolicyDocument)); err != nil || !equivalent { + return false + } + break + } + } + } + + if matches != len(one) { + return false + } + + return true +} From 6c550390a473843296fe115421da67dcc5826fe7 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Tue, 7 Dec 2021 21:49:01 -0500 Subject: [PATCH 2/5] tests/iam/role: Adjust test for non-diffs --- internal/service/iam/role_test.go | 48 +++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/internal/service/iam/role_test.go b/internal/service/iam/role_test.go index 0e79d04b954e..3d68f6e8c771 100644 --- a/internal/service/iam/role_test.go +++ b/internal/service/iam/role_test.go @@ -472,10 +472,19 @@ func TestAccIAMRole_InlinePolicy_ignoreOrder(t *testing.T) { resource.TestCheckResourceAttr(resourceName, "managed_policy_arns.#", "0"), ), }, + { + Config: testAccRolePolicyInlineActionOrderConfig(rName), + PlanOnly: true, + }, { Config: testAccRolePolicyInlineActionNewOrderConfig(rName), PlanOnly: true, }, + { + Config: testAccRolePolicyInlineActionOrderActualDiffConfig(rName), + PlanOnly: true, + ExpectNonEmptyPlan: true, + }, }, }) } @@ -1698,6 +1707,45 @@ resource "aws_iam_role" "test" { `, roleName) } +func testAccRolePolicyInlineActionOrderActualDiffConfig(roleName string) string { + return fmt.Sprintf(` +data "aws_partition" "current" {} + +resource "aws_iam_role" "test" { + name = %[1]q + + assume_role_policy = jsonencode({ + Version = "2012-10-17" + Statement = [{ + Action = "sts:AssumeRole", + Principal = { + Service = "ec2.${data.aws_partition.current.dns_suffix}", + } + Effect = "Allow" + Sid = "" + }] + }) + + inline_policy { + name = %[1]q + + policy = jsonencode({ + Version = "2012-10-17" + Statement = [{ + Action = [ + "ec2:DescribeScheduledInstances", + "ec2:DescribeElasticGpus", + "ec2:DescribeScheduledInstanceAvailability", + ] + Effect = "Allow" + Resource = "*" + }] + }) + } +} +`, roleName) +} + func testAccRolePolicyInlineActionNewOrderConfig(roleName string) string { return fmt.Sprintf(` data "aws_partition" "current" {} From 1e23e8b1ddec8d0acdb1e61fbaa65d4289f6317a Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Tue, 7 Dec 2021 21:49:18 -0500 Subject: [PATCH 3/5] docs/iam/role: Clarify --- website/docs/r/iam_role.html.markdown | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/website/docs/r/iam_role.html.markdown b/website/docs/r/iam_role.html.markdown index 7e6865c5fbd6..b220d08d2e0f 100644 --- a/website/docs/r/iam_role.html.markdown +++ b/website/docs/r/iam_role.html.markdown @@ -183,7 +183,7 @@ The following arguments are optional: * `description` - (Optional) Description of the role. * `force_detach_policies` - (Optional) Whether to force detaching any policies the role has before destroying it. Defaults to `false`. -* `inline_policy` - (Optional) Configuration block defining an exclusive set of IAM inline policies associated with the IAM role. Defined below. If no blocks are configured, Terraform will ignore any managing any inline policies in this resource. Configuring one empty block (i.e., `inline_policy {}`) will cause Terraform to remove _all_ inline policies. +* `inline_policy` - (Optional) Configuration block defining an exclusive set of IAM inline policies associated with the IAM role. See below. If no blocks are configured, Terraform will not manage any inline policies in this resource. Configuring one empty block (i.e., `inline_policy {}`) will cause Terraform to remove _all_ inline policies added out of band on `apply`. * `managed_policy_arns` - (Optional) Set of exclusive IAM managed policy ARNs to attach to the IAM role. If this attribute is not configured, Terraform will ignore policy attachments to this resource. When configured, Terraform will align the role's managed policy attachments with this set by attaching or detaching managed policies. Configuring an empty set (i.e., `managed_policy_arns = []`) will cause Terraform to remove _all_ managed policy attachments. * `max_session_duration` - (Optional) Maximum session duration (in seconds) that you want to set for the specified role. If you do not specify a value for this setting, the default maximum of one hour is applied. This setting can have a value from 1 hour to 12 hours. * `name` - (Optional, Forces new resource) Friendly name of the role. If omitted, Terraform will assign a random, unique name. See [IAM Identifiers](https://docs.aws.amazon.com/IAM/latest/UserGuide/Using_Identifiers.html) for more information. @@ -196,6 +196,8 @@ The following arguments are optional: This configuration block supports the following: +~> **NOTE:** Since one empty block (i.e., `inline_policy {}`) is valid syntactically to remove out of band policies on `apply`, `name` and `policy` are technically _optional_. However, they are both _required_ in order to manage actual inline policies. Not including one or the other may not result in Terraform errors but will result in unpredictable and incorrect behavior. + * `name` - (Required) Name of the role policy. * `policy` - (Required) Policy document as a JSON formatted string. For more information about building IAM policy documents with Terraform, see the [AWS IAM Policy Document Guide](https://learn.hashicorp.com/tutorials/terraform/aws-iam-policy). From 0e708058bb5de3ab525323fd0b27096ecf5fe03b Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Tue, 7 Dec 2021 21:50:56 -0500 Subject: [PATCH 4/5] Add changelog --- .changelog/22099.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/22099.txt diff --git a/.changelog/22099.txt b/.changelog/22099.txt new file mode 100644 index 000000000000..ea382adf0660 --- /dev/null +++ b/.changelog/22099.txt @@ -0,0 +1,3 @@ +```release-note:bug +resource/aws_iam_role: Fix order-related diffs in `policy` +``` \ No newline at end of file From df9fa2d138bfad1410e9bb208cb5cb2610c602cd Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Tue, 7 Dec 2021 22:59:44 -0500 Subject: [PATCH 5/5] iam/role: Simplify --- internal/service/iam/role.go | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/internal/service/iam/role.go b/internal/service/iam/role.go index 59c8216397e7..bb866b5cc04e 100644 --- a/internal/service/iam/role.go +++ b/internal/service/iam/role.go @@ -830,11 +830,7 @@ func inlinePoliciesActualDiff(d *schema.ResourceData) bool { osPolicies := expandRoleInlinePolicies(roleName, os.List()) nsPolicies := expandRoleInlinePolicies(roleName, ns.List()) - if !inlinePoliciesEquivalent(osPolicies, nsPolicies) { - return true - } - - return false + return !inlinePoliciesEquivalent(osPolicies, nsPolicies) } func inlinePoliciesEquivalent(one, two []*iam.PutRolePolicyInput) bool { @@ -860,9 +856,5 @@ func inlinePoliciesEquivalent(one, two []*iam.PutRolePolicyInput) bool { } } - if matches != len(one) { - return false - } - - return true + return matches == len(one) }