From 22e8eb5b84f79477bc7640447540292c142b7d99 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Thu, 9 Dec 2021 16:36:53 -0500 Subject: [PATCH 1/4] ecr/lifecycle_policy: Fix diffs when policy equivalent --- internal/service/ecr/lifecycle_policy.go | 141 +++++++++++++++++- internal/service/ecr/lifecycle_policy_test.go | 120 +++++++++++++++ 2 files changed, 253 insertions(+), 8 deletions(-) diff --git a/internal/service/ecr/lifecycle_policy.go b/internal/service/ecr/lifecycle_policy.go index 7265f215ab3..e3e87ecc9f0 100644 --- a/internal/service/ecr/lifecycle_policy.go +++ b/internal/service/ecr/lifecycle_policy.go @@ -1,18 +1,23 @@ package ecr import ( + "bytes" + "encoding/json" "fmt" "log" + "sort" + "strings" "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/private/protocol/json/jsonutil" "github.com/aws/aws-sdk-go/service/ecr" "github.com/hashicorp/aws-sdk-go-base/tfawserr" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/structure" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation" "github.com/hashicorp/terraform-provider-aws/internal/conns" "github.com/hashicorp/terraform-provider-aws/internal/tfresource" - "github.com/hashicorp/terraform-provider-aws/internal/verify" ) func ResourceLifecyclePolicy() *schema.Resource { @@ -32,11 +37,19 @@ func ResourceLifecyclePolicy() *schema.Resource { ForceNew: true, }, "policy": { - Type: schema.TypeString, - Required: true, - ForceNew: true, - ValidateFunc: validation.StringIsJSON, - DiffSuppressFunc: verify.SuppressEquivalentJSONDiffs, + Type: schema.TypeString, + Required: true, + ForceNew: true, + ValidateFunc: validation.StringIsJSON, + DiffSuppressFunc: func(k, old, new string, d *schema.ResourceData) bool { + equal, _ := equivalentLifecyclePolicyJSON(old, new) + + return equal + }, + StateFunc: func(v interface{}) string { + json, _ := structure.NormalizeJsonString(v) + return json + }, }, "registry_id": { Type: schema.TypeString, @@ -49,9 +62,15 @@ func ResourceLifecyclePolicy() *schema.Resource { func resourceLifecyclePolicyCreate(d *schema.ResourceData, meta interface{}) error { conn := meta.(*conns.AWSClient).ECRConn + policy, err := structure.NormalizeJsonString(d.Get("policy").(string)) + + if err != nil { + return fmt.Errorf("policy (%s) is invalid JSON: %w", policy, err) + } + input := &ecr.PutLifecyclePolicyInput{ RepositoryName: aws.String(d.Get("repository").(string)), - LifecyclePolicyText: aws.String(d.Get("policy").(string)), + LifecyclePolicyText: aws.String(policy), } resp, err := conn.PutLifecyclePolicy(input) @@ -118,7 +137,22 @@ func resourceLifecyclePolicyRead(d *schema.ResourceData, meta interface{}) error d.Set("repository", resp.RepositoryName) d.Set("registry_id", resp.RegistryId) - d.Set("policy", resp.LifecyclePolicyText) + + equivalent, err := equivalentLifecyclePolicyJSON(d.Get("policy").(string), aws.StringValue(resp.LifecyclePolicyText)) + + if err != nil { + return fmt.Errorf("while comparing policy (state: %s) (from AWS: %s), encountered: %w", d.Get("policy").(string), aws.StringValue(resp.LifecyclePolicyText), err) + } + + if !equivalent { + policyToSet, err := structure.NormalizeJsonString(aws.StringValue(resp.LifecyclePolicyText)) + + if err != nil { + return fmt.Errorf("policy (%s) is invalid JSON: %w", policyToSet, err) + } + + d.Set("policy", policyToSet) + } return nil } @@ -143,3 +177,94 @@ func resourceLifecyclePolicyDelete(d *schema.ResourceData, meta interface{}) err return nil } + +type lifecyclePolicyRuleSelection struct { + TagStatus *string `locationName:"tagStatus" type:"string" enum:"tagStatus" required:"true"` + TagPrefixList []*string `locationName:"tagPrefixList" type:"list"` + CountType *string `locationName:"countType" type:"string" enum:"countType" required:"true"` + CountUnit *string `locationName:"countUnit" type:"string" enum:"countType"` + CountNumber *int64 `locationName:"countNumber" min:"1" type:"integer"` +} + +type lifecyclePolicyRuleAction struct { + ActionType *string `locationName:"type" type:"string" required:"true"` +} + +type lifecyclePolicyRule struct { + RulePriority *int64 `locationName:"countNumber" type:"integer" required:"true"` + Description *string `locationName:"description" type:"string"` + Selection *lifecyclePolicyRuleSelection `location:"selection" type:"structure" required:"true"` + Action *lifecyclePolicyRuleAction `location:"action" type:"structure" required:"true"` +} + +type lifecyclePolicy struct { + Rules []*lifecyclePolicyRule `locationName:"rules" min:"1" type:"list" required:"true"` +} + +func (lp *lifecyclePolicy) reduce() error { + sort.Slice(lp.Rules, func(i, j int) bool { + return aws.Int64Value(lp.Rules[i].RulePriority) < aws.Int64Value(lp.Rules[j].RulePriority) + }) + + for _, rule := range lp.Rules { + _ = rule.Selection.reduce() + } + + return nil +} + +func (lprs *lifecyclePolicyRuleSelection) reduce() error { + sort.Slice(lprs.TagPrefixList, func(i, j int) bool { + return aws.StringValue(lprs.TagPrefixList[i]) < aws.StringValue(lprs.TagPrefixList[j]) + }) + + if len(lprs.TagPrefixList) == 0 { + lprs.TagPrefixList = nil + } + + return nil +} + +func equivalentLifecyclePolicyJSON(str1, str2 string) (bool, error) { + if strings.TrimSpace(str1) == "" { + str1 = "{}" + } + + if strings.TrimSpace(str2) == "" { + str2 = "{}" + } + + var lp1, lp2 lifecyclePolicy + + if err := json.Unmarshal([]byte(str1), &lp1); err != nil { + return false, err + } + + _ = lp1.reduce() + + canonicalJSON1, err := jsonutil.BuildJSON(lp1) + + if err != nil { + return false, err + } + + if err := json.Unmarshal([]byte(str2), &lp2); err != nil { + return false, err + } + + _ = lp2.reduce() + + canonicalJSON2, err := jsonutil.BuildJSON(lp2) + + if err != nil { + return false, err + } + + equal := bytes.Equal(canonicalJSON1, canonicalJSON2) + + if !equal { + log.Printf("[DEBUG] Canonical Lifecycle Policy JSONs are not equal.\nFirst: %s\nSecond: %s\n", canonicalJSON1, canonicalJSON2) + } + + return equal, nil +} diff --git a/internal/service/ecr/lifecycle_policy_test.go b/internal/service/ecr/lifecycle_policy_test.go index 28ed022a2e5..50c794de741 100644 --- a/internal/service/ecr/lifecycle_policy_test.go +++ b/internal/service/ecr/lifecycle_policy_test.go @@ -40,6 +40,30 @@ func TestAccECRLifecyclePolicy_basic(t *testing.T) { }) } +func TestAccECRLifecyclePolicy_ignoreEquivalent(t *testing.T) { + rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + resourceName := "aws_ecr_lifecycle_policy.test" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { acctest.PreCheck(t) }, + ErrorCheck: acctest.ErrorCheck(t, ecr.EndpointsID), + Providers: acctest.Providers, + CheckDestroy: testAccCheckLifecyclePolicyDestroy, + Steps: []resource.TestStep{ + { + Config: testAccEcrLifecyclePolicyOrderConfig(rName), + Check: resource.ComposeTestCheckFunc( + testAccCheckLifecyclePolicyExists(resourceName), + ), + }, + { + Config: testAccEcrLifecyclePolicyNewOrderConfig(rName), + PlanOnly: true, + }, + }, + }) +} + func testAccCheckLifecyclePolicyDestroy(s *terraform.State) error { conn := acctest.Provider.Meta().(*conns.AWSClient).ECRConn @@ -116,3 +140,99 @@ EOF } `, rName) } + +func testAccEcrLifecyclePolicyOrderConfig(rName string) string { + return fmt.Sprintf(` +resource "aws_ecr_repository" "test" { + name = "%s" +} + +resource "aws_ecr_lifecycle_policy" "test" { + repository = aws_ecr_repository.test.name + + policy = jsonencode({ + rules = [ + { + rulePriority = 1 + description = "Expire images older than 14 days" + selection = { + tagStatus = "untagged" + countType = "sinceImagePushed" + countUnit = "days" + countNumber = 14 + } + action = { + type = "expire" + } + }, + { + rulePriority = 2 + description = "Expire tagged images older than 14 days" + selection = { + tagStatus = "tagged" + tagPrefixList = [ + "first", + "second", + "third", + ] + countType = "sinceImagePushed" + countUnit = "days" + countNumber = 14 + } + action = { + type = "expire" + } + }, + ] + }) +} +`, rName) +} + +func testAccEcrLifecyclePolicyNewOrderConfig(rName string) string { + return fmt.Sprintf(` +resource "aws_ecr_repository" "test" { + name = "%s" +} + +resource "aws_ecr_lifecycle_policy" "test" { + repository = aws_ecr_repository.test.name + + policy = jsonencode({ + rules = [ + { + rulePriority = 2 + description = "Expire tagged images older than 14 days" + selection = { + tagStatus = "tagged" + tagPrefixList = [ + "third", + "first", + "second", + ] + countType = "sinceImagePushed" + countUnit = "days" + countNumber = 14 + } + action = { + type = "expire" + } + }, + { + rulePriority = 1 + description = "Expire images older than 14 days" + selection = { + tagStatus = "untagged" + countType = "sinceImagePushed" + countUnit = "days" + countNumber = 14 + } + action = { + type = "expire" + } + }, + ] + }) +} +`, rName) +} From 2d1c1cb2f65649e45f79810996440a2a6d79b2a5 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Thu, 9 Dec 2021 16:38:18 -0500 Subject: [PATCH 2/4] Add changelog --- .changelog/22142.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/22142.txt diff --git a/.changelog/22142.txt b/.changelog/22142.txt new file mode 100644 index 00000000000..4fea91bee26 --- /dev/null +++ b/.changelog/22142.txt @@ -0,0 +1,3 @@ +```release-note:bug +resource/aws_ecr_lifecycle_policy: Fix erroneous diffs in `policy` when no changes made or policies are equivalent +``` \ No newline at end of file From 537052c7e12c2ff5421e88ec8e280958bc56b841 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Thu, 9 Dec 2021 16:42:06 -0500 Subject: [PATCH 3/4] Lint Nicholas --- internal/service/ecr/lifecycle_policy_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/service/ecr/lifecycle_policy_test.go b/internal/service/ecr/lifecycle_policy_test.go index 50c794de741..1d13ec7562a 100644 --- a/internal/service/ecr/lifecycle_policy_test.go +++ b/internal/service/ecr/lifecycle_policy_test.go @@ -144,7 +144,7 @@ EOF func testAccEcrLifecyclePolicyOrderConfig(rName string) string { return fmt.Sprintf(` resource "aws_ecr_repository" "test" { - name = "%s" + name = %[1]q } resource "aws_ecr_lifecycle_policy" "test" { @@ -167,7 +167,7 @@ resource "aws_ecr_lifecycle_policy" "test" { }, { rulePriority = 2 - description = "Expire tagged images older than 14 days" + description = "Expire tagged images older than 14 days" selection = { tagStatus = "tagged" tagPrefixList = [ @@ -202,7 +202,7 @@ resource "aws_ecr_lifecycle_policy" "test" { rules = [ { rulePriority = 2 - description = "Expire tagged images older than 14 days" + description = "Expire tagged images older than 14 days" selection = { tagStatus = "tagged" tagPrefixList = [ From 220c0bf94ea2f23f744a4c975f9a1f56be6e952e Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Fri, 10 Dec 2021 10:47:01 -0500 Subject: [PATCH 4/4] Fix funky funcs --- internal/service/ecr/lifecycle_policy.go | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/internal/service/ecr/lifecycle_policy.go b/internal/service/ecr/lifecycle_policy.go index e3e87ecc9f0..e792b9f6898 100644 --- a/internal/service/ecr/lifecycle_policy.go +++ b/internal/service/ecr/lifecycle_policy.go @@ -201,19 +201,17 @@ type lifecyclePolicy struct { Rules []*lifecyclePolicyRule `locationName:"rules" min:"1" type:"list" required:"true"` } -func (lp *lifecyclePolicy) reduce() error { +func (lp *lifecyclePolicy) reduce() { sort.Slice(lp.Rules, func(i, j int) bool { return aws.Int64Value(lp.Rules[i].RulePriority) < aws.Int64Value(lp.Rules[j].RulePriority) }) for _, rule := range lp.Rules { - _ = rule.Selection.reduce() + rule.Selection.reduce() } - - return nil } -func (lprs *lifecyclePolicyRuleSelection) reduce() error { +func (lprs *lifecyclePolicyRuleSelection) reduce() { sort.Slice(lprs.TagPrefixList, func(i, j int) bool { return aws.StringValue(lprs.TagPrefixList[i]) < aws.StringValue(lprs.TagPrefixList[j]) }) @@ -221,8 +219,6 @@ func (lprs *lifecyclePolicyRuleSelection) reduce() error { if len(lprs.TagPrefixList) == 0 { lprs.TagPrefixList = nil } - - return nil } func equivalentLifecyclePolicyJSON(str1, str2 string) (bool, error) { @@ -240,7 +236,7 @@ func equivalentLifecyclePolicyJSON(str1, str2 string) (bool, error) { return false, err } - _ = lp1.reduce() + lp1.reduce() canonicalJSON1, err := jsonutil.BuildJSON(lp1) @@ -252,7 +248,7 @@ func equivalentLifecyclePolicyJSON(str1, str2 string) (bool, error) { return false, err } - _ = lp2.reduce() + lp2.reduce() canonicalJSON2, err := jsonutil.BuildJSON(lp2)