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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ecr: Preserve/ignore order in JSON/policy #22004

Merged
merged 10 commits into from
Dec 2, 2021
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Fix various repository issues
  • Loading branch information
YakDriver committed Dec 2, 2021
commit 6b100b221bdd4ee9ca5e226f2b19344a8512e2ea
25 changes: 22 additions & 3 deletions internal/service/ecr/repository_policy.go
Original file line number Diff line number Diff line change
@@ -9,6 +9,7 @@ import (
"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"
tfiam "github.com/hashicorp/terraform-provider-aws/internal/service/iam"
@@ -49,15 +50,20 @@ func ResourceRepositoryPolicy() *schema.Resource {
func resourceRepositoryPolicyPut(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.SetRepositoryPolicyInput{
RepositoryName: aws.String(d.Get("repository").(string)),
PolicyText: aws.String(d.Get("policy").(string)),
PolicyText: aws.String(policy),
}

log.Printf("[DEBUG] Creating ECR repository policy: %#v", input)

// Retry due to IAM eventual consistency
var err error
var out *ecr.SetRepositoryPolicyOutput
err = resource.Retry(tfiam.PropagationTimeout, func() *resource.RetryError {
out, err = conn.SetRepositoryPolicy(&input)
@@ -141,7 +147,20 @@ func resourceRepositoryPolicyRead(d *schema.ResourceData, meta interface{}) erro

d.Set("repository", out.RepositoryName)
d.Set("registry_id", out.RegistryId)
d.Set("policy", out.PolicyText)

policyToSet, err := verify.SecondJSONUnlessEquivalent(d.Get("policy").(string), aws.StringValue(out.PolicyText))

if err != nil {
return fmt.Errorf("while setting policy (%s), encountered: %w", policyToSet, err)
}

policyToSet, err = structure.NormalizeJsonString(policyToSet)

if err != nil {
return fmt.Errorf("policy (%s) is invalid JSON: %w", policyToSet, err)
}

d.Set("policy", policyToSet)

return nil
}
286 changes: 222 additions & 64 deletions internal/service/ecr/repository_policy_test.go
Original file line number Diff line number Diff line change
@@ -54,7 +54,7 @@ func TestAccECRRepositoryPolicy_basic(t *testing.T) {
})
}

func TestAccECRRepositoryPolicy_iam(t *testing.T) {
func TestAccECRRepositoryPolicy_IAM_basic(t *testing.T) {
rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix)
resourceName := "aws_ecr_repository_policy.test"

@@ -65,7 +65,7 @@ func TestAccECRRepositoryPolicy_iam(t *testing.T) {
CheckDestroy: testAccCheckRepositoryPolicyDestroy,
Steps: []resource.TestStep{
{
Config: testAccRepositoryPolicyWithIAMRoleConfig(rName),
Config: testAccRepositoryPolicyIAMRoleConfig(rName),
Check: resource.ComposeTestCheckFunc(
testAccCheckRepositoryPolicyExists(resourceName),
resource.TestMatchResourceAttr(resourceName, "policy", regexp.MustCompile(rName)),
@@ -81,6 +81,39 @@ func TestAccECRRepositoryPolicy_iam(t *testing.T) {
})
}

// Reference: https://github.com/hashicorp/terraform-provider-aws/issues/19365
func TestAccECRRepositoryPolicy_IAM_principalOrder(t *testing.T) {
rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix)
resourceName := "aws_ecr_repository_policy.test"

resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { acctest.PreCheck(t) },
ErrorCheck: acctest.ErrorCheck(t, ecr.EndpointsID),
Providers: acctest.Providers,
CheckDestroy: testAccCheckRepositoryPolicyDestroy,
Steps: []resource.TestStep{
{
Config: testAccRepositoryPolicyIAMRoleOrderJSONEncodeConfig(rName),
Check: resource.ComposeTestCheckFunc(
testAccCheckRepositoryPolicyExists(resourceName),
resource.TestMatchResourceAttr(resourceName, "policy", regexp.MustCompile(rName)),
resource.TestMatchResourceAttr(resourceName, "policy", regexp.MustCompile("iam")),
),
},
{
Config: testAccRepositoryPolicyIAMRoleNewOrderJSONEncodeConfig(rName),
Check: resource.ComposeTestCheckFunc(
testAccCheckRepositoryPolicyExists(resourceName),
),
},
{
Config: testAccRepositoryPolicyIAMRoleOrderJSONEncodeConfig(rName),
PlanOnly: true,
},
},
})
}

func TestAccECRRepositoryPolicy_disappears(t *testing.T) {
rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix)
resourceName := "aws_ecr_repository_policy.test"
@@ -169,21 +202,15 @@ resource "aws_ecr_repository" "test" {
resource "aws_ecr_repository_policy" "test" {
repository = aws_ecr_repository.test.name

policy = <<EOF
{
"Version": "2008-10-17",
"Statement": [
{
"Sid": "%[1]s",
"Effect": "Allow",
"Principal": "*",
"Action": [
"ecr:ListImages"
]
}
]
}
EOF
policy = jsonencode({
Version = "2008-10-17"
Statement = [{
Sid = %[1]q
Effect = "Allow"
Principal = "*"
Action = "ecr:ListImages"
}]
})
}
`, rName)
}
@@ -197,31 +224,27 @@ resource "aws_ecr_repository" "test" {
resource "aws_ecr_repository_policy" "test" {
repository = aws_ecr_repository.test.name

policy = <<EOF
{
"Version": "2008-10-17",
"Statement": [
{
"Sid": "%[1]s",
"Effect": "Allow",
"Principal": "*",
"Action": [
"ecr:ListImages",
"ecr:DescribeImages"
]
}
]
}
EOF
policy = jsonencode({
Version = "2008-10-17"
Statement = [{
Sid = %[1]q
Effect = "Allow"
Principal = "*"
Action = [
"ecr:ListImages",
"ecr:DescribeImages",
]
}]
})
}
`, rName)
}

// testAccRepositoryPolicyWithIAMRoleConfig creates a new IAM Role and tries
// testAccRepositoryPolicyIAMRoleConfig creates a new IAM Role and tries
// to use it's ARN in an ECR Repository Policy. IAM changes need some time to
// be propagated to other services - like ECR. So the following code should
// exercise our retry logic, since we try to use the new resource instantly.
func testAccRepositoryPolicyWithIAMRoleConfig(rName string) string {
func testAccRepositoryPolicyIAMRoleConfig(rName string) string {
return fmt.Sprintf(`
resource "aws_ecr_repository" "test" {
name = %[1]q
@@ -230,42 +253,177 @@ resource "aws_ecr_repository" "test" {
resource "aws_iam_role" "test" {
name = %[1]q

assume_role_policy = <<EOF
{
"Version": "2012-10-17",
"Statement": [
{
"Action": "sts:AssumeRole",
"Effect": "Allow",
"Principal": {
"Service": "ec2.amazonaws.com"
assume_role_policy = jsonencode({
Version = "2012-10-17"
Statement = [{
Action = "sts:AssumeRole"
Effect = "Allow"
Principal = {
Service = "ec2.amazonaws.com"
}
}
]
}
EOF
}]
})
}

resource "aws_ecr_repository_policy" "test" {
repository = aws_ecr_repository.test.name

policy = <<EOF
{
"Version": "2008-10-17",
"Statement": [
{
"Sid": "%[1]s",
"Effect": "Allow",
"Principal": {
"AWS": "${aws_iam_role.test.arn}"
},
"Action": [
"ecr:ListImages"
]
}
]
policy = jsonencode({
Version = "2008-10-17"
Statement = [{
Sid = %[1]q
Effect = "Allow",
Principal = {
AWS = aws_iam_role.test.arn
}
Action = "ecr:ListImages"
}]
})
}
`, rName)
}

func testAccRepositoryPolicyIAMRoleOrderBaseConfig(rName string) string {
return fmt.Sprintf(`
data "aws_caller_identity" "current" {}

data "aws_region" "current" {}

data "aws_partition" "current" {}

resource "aws_iam_role" "test1" {
name = "%[1]s-mercedes"

assume_role_policy = jsonencode({
Statement = [{
Action = "sts:AssumeRole"
Effect = "Allow"
Principal = {
Service = "ec2.${data.aws_partition.current.dns_suffix}"
}
}]
Version = "2012-10-17"
})
}

resource "aws_iam_role" "test2" {
name = "%[1]s-redbull"

assume_role_policy = jsonencode({
Statement = [{
Action = "sts:AssumeRole"
Effect = "Allow"
Principal = {
Service = "ec2.${data.aws_partition.current.dns_suffix}"
}
}]
Version = "2012-10-17"
})
}
EOF

resource "aws_iam_role" "test3" {
name = "%[1]s-mclaren"

assume_role_policy = jsonencode({
Statement = [{
Action = "sts:AssumeRole"
Effect = "Allow"
Principal = {
Service = "ec2.${data.aws_partition.current.dns_suffix}"
}
}]
Version = "2012-10-17"
})
}

resource "aws_iam_role" "test4" {
name = "%[1]s-ferrari"

assume_role_policy = jsonencode({
Statement = [{
Action = "sts:AssumeRole"
Effect = "Allow"
Principal = {
Service = "ec2.${data.aws_partition.current.dns_suffix}"
}
}]
Version = "2012-10-17"
})
}

resource "aws_iam_role" "test5" {
name = "%[1]s-astonmartin"

assume_role_policy = jsonencode({
Statement = [{
Action = "sts:AssumeRole"
Effect = "Allow"
Principal = {
Service = "ec2.${data.aws_partition.current.dns_suffix}"
}
}]
Version = "2012-10-17"
})
}

resource "aws_ecr_repository" "test" {
name = %[1]q
}
`, rName)
}

func testAccRepositoryPolicyIAMRoleOrderJSONEncodeConfig(rName string) string {
return acctest.ConfigCompose(
testAccRepositoryPolicyIAMRoleOrderBaseConfig(rName),
fmt.Sprintf(`
resource "aws_ecr_repository_policy" "test" {
repository = aws_ecr_repository.test.name

policy = jsonencode({
Statement = [{
Sid = %[1]q
Action = "ecr:ListImages"
Effect = "Allow"
Principal = {
AWS = [
aws_iam_role.test1.arn,
aws_iam_role.test3.arn,
aws_iam_role.test2.arn,
aws_iam_role.test4.arn,
aws_iam_role.test5.arn,
]
}
}]
Version = "2012-10-17"
})
}
`, rName))
}

func testAccRepositoryPolicyIAMRoleNewOrderJSONEncodeConfig(rName string) string {
return acctest.ConfigCompose(
testAccRepositoryPolicyIAMRoleOrderBaseConfig(rName),
fmt.Sprintf(`
resource "aws_ecr_repository_policy" "test" {
repository = aws_ecr_repository.test.name

policy = jsonencode({
Statement = [{
Sid = %[1]q
Action = "ecr:ListImages"
Effect = "Allow"
Principal = {
AWS = [
aws_iam_role.test1.arn,
aws_iam_role.test5.arn,
aws_iam_role.test4.arn,
aws_iam_role.test2.arn,
aws_iam_role.test3.arn,
]
}
}]
Version = "2012-10-17"
})
}
`, rName))
}