From 9aba1fb894a4ebe886246b5a147e9bd74409c3b4 Mon Sep 17 00:00:00 2001 From: DrFaust92 Date: Wed, 15 Jul 2020 22:39:58 +0300 Subject: [PATCH 1/7] add plan time validation to `policy` use enums read after update add disappears tests + make tests more specific --- aws/resource_aws_ecr_repository_policy.go | 74 ++++----- ...resource_aws_ecr_repository_policy_test.go | 141 ++++++++++++++---- 2 files changed, 140 insertions(+), 75 deletions(-) diff --git a/aws/resource_aws_ecr_repository_policy.go b/aws/resource_aws_ecr_repository_policy.go index ba710623901..4a74c90d3c3 100644 --- a/aws/resource_aws_ecr_repository_policy.go +++ b/aws/resource_aws_ecr_repository_policy.go @@ -6,10 +6,10 @@ import ( "time" "github.com/aws/aws-sdk-go/aws" - "github.com/aws/aws-sdk-go/aws/awserr" "github.com/aws/aws-sdk-go/service/ecr" "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/validation" ) func resourceAwsEcrRepositoryPolicy() *schema.Resource { @@ -31,6 +31,7 @@ func resourceAwsEcrRepositoryPolicy() *schema.Resource { "policy": { Type: schema.TypeString, Required: true, + ValidateFunc: validation.StringIsJSON, DiffSuppressFunc: suppressEquivalentAwsPolicyDiffs, }, "registry_id": { @@ -49,7 +50,7 @@ func resourceAwsEcrRepositoryPolicyCreate(d *schema.ResourceData, meta interface PolicyText: aws.String(d.Get("policy").(string)), } - log.Printf("[DEBUG] Creating ECR resository policy: %s", input) + log.Printf("[DEBUG] Creating ECR resository policy: %#v", input) // Retry due to IAM eventual consistency var err error @@ -57,7 +58,7 @@ func resourceAwsEcrRepositoryPolicyCreate(d *schema.ResourceData, meta interface err = resource.Retry(2*time.Minute, func() *resource.RetryError { out, err = conn.SetRepositoryPolicy(&input) - if isAWSErr(err, "InvalidParameterException", "Invalid repository policy provided") { + if isAWSErr(err, ecr.ErrCodeInvalidParameterException, "Invalid repository policy provided") { return resource.RetryableError(err) } if err != nil { @@ -69,15 +70,12 @@ func resourceAwsEcrRepositoryPolicyCreate(d *schema.ResourceData, meta interface out, err = conn.SetRepositoryPolicy(&input) } if err != nil { - return fmt.Errorf("Error creating ECR Repository Policy: %s", err) + return fmt.Errorf("error creating ECR Repository Policy: %w", err) } - repositoryPolicy := *out + log.Printf("[DEBUG] ECR repository policy created: %s", aws.StringValue(out.RepositoryName)) - log.Printf("[DEBUG] ECR repository policy created: %s", *repositoryPolicy.RepositoryName) - - d.SetId(aws.StringValue(repositoryPolicy.RepositoryName)) - d.Set("registry_id", repositoryPolicy.RegistryId) + d.SetId(aws.StringValue(out.RepositoryName)) return resourceAwsEcrRepositoryPolicyRead(d, meta) } @@ -90,26 +88,20 @@ func resourceAwsEcrRepositoryPolicyRead(d *schema.ResourceData, meta interface{} RepositoryName: aws.String(d.Id()), }) if err != nil { - if ecrerr, ok := err.(awserr.Error); ok { - switch ecrerr.Code() { - case "RepositoryNotFoundException", "RepositoryPolicyNotFoundException": - d.SetId("") - return nil - default: - return err - } + if isAWSErr(err, ecr.ErrCodeRepositoryNotFoundException, "") || + isAWSErr(err, ecr.ErrCodeRepositoryPolicyNotFoundException, "") { + log.Printf("[WARN] ECR Repositoy Policy %s not found, removing", d.Id()) + d.SetId("") + return nil } return err } - log.Printf("[DEBUG] Received repository policy %s", out) - - repositoryPolicy := out + log.Printf("[DEBUG] Received repository policy %#v", out) - d.SetId(aws.StringValue(repositoryPolicy.RepositoryName)) - d.Set("repository", repositoryPolicy.RepositoryName) - d.Set("registry_id", repositoryPolicy.RegistryId) - d.Set("policy", repositoryPolicy.PolicyText) + d.Set("repository", out.RepositoryName) + d.Set("registry_id", out.RegistryId) + d.Set("policy", out.PolicyText) return nil } @@ -117,25 +109,20 @@ func resourceAwsEcrRepositoryPolicyRead(d *schema.ResourceData, meta interface{} func resourceAwsEcrRepositoryPolicyUpdate(d *schema.ResourceData, meta interface{}) error { conn := meta.(*AWSClient).ecrconn - if !d.HasChange("policy") { - return nil - } - input := ecr.SetRepositoryPolicyInput{ - RepositoryName: aws.String(d.Get("repository").(string)), + RepositoryName: aws.String(d.Id()), RegistryId: aws.String(d.Get("registry_id").(string)), PolicyText: aws.String(d.Get("policy").(string)), } - log.Printf("[DEBUG] Updating ECR resository policy: %s", input) + log.Printf("[DEBUG] Updating ECR resository policy: %#v", input) // Retry due to IAM eventual consistency var err error - var out *ecr.SetRepositoryPolicyOutput err = resource.Retry(2*time.Minute, func() *resource.RetryError { - out, err = conn.SetRepositoryPolicy(&input) + _, err = conn.SetRepositoryPolicy(&input) - if isAWSErr(err, "InvalidParameterException", "Invalid repository policy provided") { + if isAWSErr(err, ecr.ErrCodeInvalidParameterException, "Invalid repository policy provided") { return resource.RetryableError(err) } if err != nil { @@ -144,18 +131,13 @@ func resourceAwsEcrRepositoryPolicyUpdate(d *schema.ResourceData, meta interface return nil }) if isResourceTimeoutError(err) { - out, err = conn.SetRepositoryPolicy(&input) + _, err = conn.SetRepositoryPolicy(&input) } if err != nil { - return fmt.Errorf("Error updating ECR Repository Policy: %s", err) + return fmt.Errorf("error updating ECR Repository Policy: %w", err) } - repositoryPolicy := *out - - d.SetId(aws.StringValue(repositoryPolicy.RepositoryName)) - d.Set("registry_id", repositoryPolicy.RegistryId) - - return nil + return resourceAwsEcrRepositoryPolicyRead(d, meta) } func resourceAwsEcrRepositoryPolicyDelete(d *schema.ResourceData, meta interface{}) error { @@ -166,13 +148,9 @@ func resourceAwsEcrRepositoryPolicyDelete(d *schema.ResourceData, meta interface RegistryId: aws.String(d.Get("registry_id").(string)), }) if err != nil { - if ecrerr, ok := err.(awserr.Error); ok { - switch ecrerr.Code() { - case "RepositoryNotFoundException", "RepositoryPolicyNotFoundException": - return nil - default: - return err - } + if isAWSErr(err, ecr.ErrCodeRepositoryNotFoundException, "") || + isAWSErr(err, ecr.ErrCodeRepositoryPolicyNotFoundException, "") { + return nil } return err } diff --git a/aws/resource_aws_ecr_repository_policy_test.go b/aws/resource_aws_ecr_repository_policy_test.go index 4bbff0651e7..6721199be89 100644 --- a/aws/resource_aws_ecr_repository_policy_test.go +++ b/aws/resource_aws_ecr_repository_policy_test.go @@ -2,10 +2,10 @@ package aws import ( "fmt" + "regexp" "testing" "github.com/aws/aws-sdk-go/aws" - "github.com/aws/aws-sdk-go/aws/awserr" "github.com/aws/aws-sdk-go/service/ecr" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/acctest" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" @@ -13,8 +13,8 @@ import ( ) func TestAccAWSEcrRepositoryPolicy_basic(t *testing.T) { - randString := acctest.RandString(10) - resourceName := "aws_ecr_repository_policy.default" + rName := acctest.RandomWithPrefix("tf-acc-test") + resourceName := "aws_ecr_repository_policy.test" resource.ParallelTest(t, resource.TestCase{ PreCheck: func() { testAccPreCheck(t) }, @@ -22,9 +22,12 @@ func TestAccAWSEcrRepositoryPolicy_basic(t *testing.T) { CheckDestroy: testAccCheckAWSEcrRepositoryPolicyDestroy, Steps: []resource.TestStep{ { - Config: testAccAWSEcrRepositoryPolicy(randString), + Config: testAccAWSEcrRepositoryPolicyConfig(rName), Check: resource.ComposeTestCheckFunc( testAccCheckAWSEcrRepositoryPolicyExists(resourceName), + resource.TestCheckResourceAttrPair(resourceName, "repository", "aws_ecr_repository.test", "name"), + resource.TestMatchResourceAttr(resourceName, "policy", regexp.MustCompile(rName)), + testAccCheckResourceAttrAccountID(resourceName, "registry_id"), ), }, { @@ -32,13 +35,23 @@ func TestAccAWSEcrRepositoryPolicy_basic(t *testing.T) { ImportState: true, ImportStateVerify: true, }, + { + Config: testAccAWSEcrRepositoryPolicyConfigUpdated(rName), + Check: resource.ComposeTestCheckFunc( + testAccCheckAWSEcrRepositoryPolicyExists(resourceName), + resource.TestCheckResourceAttrPair(resourceName, "repository", "aws_ecr_repository.test", "name"), + resource.TestMatchResourceAttr(resourceName, "policy", regexp.MustCompile(rName)), + resource.TestMatchResourceAttr(resourceName, "policy", regexp.MustCompile("ecr:DescribeImages")), + testAccCheckResourceAttrAccountID(resourceName, "registry_id"), + ), + }, }, }) } func TestAccAWSEcrRepositoryPolicy_iam(t *testing.T) { - randString := acctest.RandString(10) - resourceName := "aws_ecr_repository_policy.default" + rName := acctest.RandomWithPrefix("tf-acc-test") + resourceName := "aws_ecr_repository_policy.test" resource.ParallelTest(t, resource.TestCase{ PreCheck: func() { testAccPreCheck(t) }, @@ -46,9 +59,11 @@ func TestAccAWSEcrRepositoryPolicy_iam(t *testing.T) { CheckDestroy: testAccCheckAWSEcrRepositoryPolicyDestroy, Steps: []resource.TestStep{ { - Config: testAccAWSEcrRepositoryPolicyWithIAMRole(randString), + Config: testAccAWSEcrRepositoryPolicyWithIAMRoleConfig(rName), Check: resource.ComposeTestCheckFunc( testAccCheckAWSEcrRepositoryPolicyExists(resourceName), + resource.TestMatchResourceAttr(resourceName, "policy", regexp.MustCompile(rName)), + resource.TestMatchResourceAttr(resourceName, "policy", regexp.MustCompile("iam")), ), }, { @@ -60,6 +75,48 @@ func TestAccAWSEcrRepositoryPolicy_iam(t *testing.T) { }) } +func TestAccAWSEcrRepositoryPolicy_disappears(t *testing.T) { + rName := acctest.RandomWithPrefix("tf-acc-test") + resourceName := "aws_ecr_repository_policy.test" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckAWSEcrRepositoryPolicyDestroy, + Steps: []resource.TestStep{ + { + Config: testAccAWSEcrRepositoryPolicyConfig(rName), + Check: resource.ComposeTestCheckFunc( + testAccCheckAWSEcrRepositoryPolicyExists(resourceName), + testAccCheckResourceDisappears(testAccProvider, resourceAwsEcrRepositoryPolicy(), resourceName), + ), + ExpectNonEmptyPlan: true, + }, + }, + }) +} + +func TestAccAWSEcrRepositoryPolicy_disappears_repository(t *testing.T) { + rName := acctest.RandomWithPrefix("tf-acc-test") + resourceName := "aws_ecr_repository_policy.test" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckAWSEcrRepositoryPolicyDestroy, + Steps: []resource.TestStep{ + { + Config: testAccAWSEcrRepositoryPolicyConfig(rName), + Check: resource.ComposeTestCheckFunc( + testAccCheckAWSEcrRepositoryPolicyExists(resourceName), + testAccCheckResourceDisappears(testAccProvider, resourceAwsEcrRepository(), resourceName), + ), + ExpectNonEmptyPlan: true, + }, + }, + }) +} + func testAccCheckAWSEcrRepositoryPolicyDestroy(s *terraform.State) error { conn := testAccProvider.Meta().(*AWSClient).ecrconn @@ -70,10 +127,11 @@ func testAccCheckAWSEcrRepositoryPolicyDestroy(s *terraform.State) error { _, err := conn.GetRepositoryPolicy(&ecr.GetRepositoryPolicyInput{ RegistryId: aws.String(rs.Primary.Attributes["registry_id"]), - RepositoryName: aws.String(rs.Primary.Attributes["repository"]), + RepositoryName: aws.String(rs.Primary.ID), }) if err != nil { - if ecrerr, ok := err.(awserr.Error); ok && ecrerr.Code() == "RepositoryNotFoundException" { + if isAWSErr(err, ecr.ErrCodeRepositoryNotFoundException, "") || + isAWSErr(err, ecr.ErrCodeRepositoryPolicyNotFoundException, "") { return nil } return err @@ -94,21 +152,21 @@ func testAccCheckAWSEcrRepositoryPolicyExists(name string) resource.TestCheckFun } } -func testAccAWSEcrRepositoryPolicy(randString string) string { +func testAccAWSEcrRepositoryPolicyConfig(rName string) string { return fmt.Sprintf(` -resource "aws_ecr_repository" "foo" { - name = "tf-acc-test-ecr-%s" +resource "aws_ecr_repository" "test" { + name = %[1]q } -resource "aws_ecr_repository_policy" "default" { - repository = aws_ecr_repository.foo.name +resource "aws_ecr_repository_policy" "test" { + repository = aws_ecr_repository.test.name policy = < Date: Sat, 29 Aug 2020 00:27:20 +0300 Subject: [PATCH 2/7] tf 12 syntax --- aws/resource_aws_ecr_repository_policy_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/aws/resource_aws_ecr_repository_policy_test.go b/aws/resource_aws_ecr_repository_policy_test.go index 6721199be89..0c151de512c 100644 --- a/aws/resource_aws_ecr_repository_policy_test.go +++ b/aws/resource_aws_ecr_repository_policy_test.go @@ -187,7 +187,7 @@ resource "aws_ecr_repository" "test" { } resource "aws_ecr_repository_policy" "test" { - repository = "${aws_ecr_repository.test.name}" + repository = aws_ecr_repository.test.name policy = < Date: Tue, 8 Sep 2020 15:43:04 +0300 Subject: [PATCH 3/7] Update aws/resource_aws_ecr_repository_policy.go Co-authored-by: Carl Henrik Lunde --- aws/resource_aws_ecr_repository_policy.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/aws/resource_aws_ecr_repository_policy.go b/aws/resource_aws_ecr_repository_policy.go index 4a74c90d3c3..b2509fe87f2 100644 --- a/aws/resource_aws_ecr_repository_policy.go +++ b/aws/resource_aws_ecr_repository_policy.go @@ -90,7 +90,7 @@ func resourceAwsEcrRepositoryPolicyRead(d *schema.ResourceData, meta interface{} if err != nil { if isAWSErr(err, ecr.ErrCodeRepositoryNotFoundException, "") || isAWSErr(err, ecr.ErrCodeRepositoryPolicyNotFoundException, "") { - log.Printf("[WARN] ECR Repositoy Policy %s not found, removing", d.Id()) + log.Printf("[WARN] ECR Repository Policy %s not found, removing", d.Id()) d.SetId("") return nil } From 746298ec072fb8f3bf962b99eaf30e6f43aa8a7e Mon Sep 17 00:00:00 2001 From: Ilia Lazebnik Date: Tue, 8 Sep 2020 15:44:12 +0300 Subject: [PATCH 4/7] Update aws/resource_aws_ecr_repository_policy.go Co-authored-by: Carl Henrik Lunde --- aws/resource_aws_ecr_repository_policy.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/aws/resource_aws_ecr_repository_policy.go b/aws/resource_aws_ecr_repository_policy.go index b2509fe87f2..2110ef2d814 100644 --- a/aws/resource_aws_ecr_repository_policy.go +++ b/aws/resource_aws_ecr_repository_policy.go @@ -115,7 +115,7 @@ func resourceAwsEcrRepositoryPolicyUpdate(d *schema.ResourceData, meta interface PolicyText: aws.String(d.Get("policy").(string)), } - log.Printf("[DEBUG] Updating ECR resository policy: %#v", input) + log.Printf("[DEBUG] Updating ECR repository policy: %#v", input) // Retry due to IAM eventual consistency var err error From 7d0c8cfaaa44bdc45521b1f37f7fa4ddd208c74e Mon Sep 17 00:00:00 2001 From: Ilia Lazebnik Date: Tue, 8 Sep 2020 15:44:35 +0300 Subject: [PATCH 5/7] Update aws/resource_aws_ecr_repository_policy.go Co-authored-by: Carl Henrik Lunde --- aws/resource_aws_ecr_repository_policy.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/aws/resource_aws_ecr_repository_policy.go b/aws/resource_aws_ecr_repository_policy.go index 2110ef2d814..348246a69df 100644 --- a/aws/resource_aws_ecr_repository_policy.go +++ b/aws/resource_aws_ecr_repository_policy.go @@ -50,7 +50,7 @@ func resourceAwsEcrRepositoryPolicyCreate(d *schema.ResourceData, meta interface PolicyText: aws.String(d.Get("policy").(string)), } - log.Printf("[DEBUG] Creating ECR resository policy: %#v", input) + log.Printf("[DEBUG] Creating ECR repository policy: %#v", input) // Retry due to IAM eventual consistency var err error From 7b7a02d90623723df87ef3fa34fa92cb95f5cc67 Mon Sep 17 00:00:00 2001 From: drfaust92 Date: Fri, 18 Dec 2020 01:11:01 +0200 Subject: [PATCH 6/7] remove update func --- aws/resource_aws_ecr_repository_policy.go | 40 ++--------------------- 1 file changed, 3 insertions(+), 37 deletions(-) diff --git a/aws/resource_aws_ecr_repository_policy.go b/aws/resource_aws_ecr_repository_policy.go index 348246a69df..53d0c2e9859 100644 --- a/aws/resource_aws_ecr_repository_policy.go +++ b/aws/resource_aws_ecr_repository_policy.go @@ -14,9 +14,9 @@ import ( func resourceAwsEcrRepositoryPolicy() *schema.Resource { return &schema.Resource{ - Create: resourceAwsEcrRepositoryPolicyCreate, + Create: resourceAwsEcrRepositoryPolicyPut, Read: resourceAwsEcrRepositoryPolicyRead, - Update: resourceAwsEcrRepositoryPolicyUpdate, + Update: resourceAwsEcrRepositoryPolicyPut, Delete: resourceAwsEcrRepositoryPolicyDelete, Importer: &schema.ResourceImporter{ State: schema.ImportStatePassthrough, @@ -42,7 +42,7 @@ func resourceAwsEcrRepositoryPolicy() *schema.Resource { } } -func resourceAwsEcrRepositoryPolicyCreate(d *schema.ResourceData, meta interface{}) error { +func resourceAwsEcrRepositoryPolicyPut(d *schema.ResourceData, meta interface{}) error { conn := meta.(*AWSClient).ecrconn input := ecr.SetRepositoryPolicyInput{ @@ -106,40 +106,6 @@ func resourceAwsEcrRepositoryPolicyRead(d *schema.ResourceData, meta interface{} return nil } -func resourceAwsEcrRepositoryPolicyUpdate(d *schema.ResourceData, meta interface{}) error { - conn := meta.(*AWSClient).ecrconn - - input := ecr.SetRepositoryPolicyInput{ - RepositoryName: aws.String(d.Id()), - RegistryId: aws.String(d.Get("registry_id").(string)), - PolicyText: aws.String(d.Get("policy").(string)), - } - - log.Printf("[DEBUG] Updating ECR repository policy: %#v", input) - - // Retry due to IAM eventual consistency - var err error - err = resource.Retry(2*time.Minute, func() *resource.RetryError { - _, err = conn.SetRepositoryPolicy(&input) - - if isAWSErr(err, ecr.ErrCodeInvalidParameterException, "Invalid repository policy provided") { - return resource.RetryableError(err) - } - if err != nil { - return resource.NonRetryableError(err) - } - return nil - }) - if isResourceTimeoutError(err) { - _, err = conn.SetRepositoryPolicy(&input) - } - if err != nil { - return fmt.Errorf("error updating ECR Repository Policy: %w", err) - } - - return resourceAwsEcrRepositoryPolicyRead(d, meta) -} - func resourceAwsEcrRepositoryPolicyDelete(d *schema.ResourceData, meta interface{}) error { conn := meta.(*AWSClient).ecrconn From 5ad49fc92c9a4be3b993cf155687fe206b00848b Mon Sep 17 00:00:00 2001 From: drfaust92 Date: Thu, 11 Mar 2021 01:36:26 +0200 Subject: [PATCH 7/7] changelog --- .changelog/14193.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/14193.txt diff --git a/.changelog/14193.txt b/.changelog/14193.txt new file mode 100644 index 00000000000..9d588853c0d --- /dev/null +++ b/.changelog/14193.txt @@ -0,0 +1,3 @@ +```release-note:enhancement +resource/aws_ecr_repository_policy: Add plan time validation for `policy` +```