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

glue/resource_policy: Fix diffs with equivalent policies #22167

Merged
merged 4 commits into from
Dec 13, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
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
3 changes: 3 additions & 0 deletions .changelog/22167.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
resource/aws_glue_resource_policy: Fix erroneous diffs in `policy` when no changes made or policies are equivalent
```
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import (
"github.com/hashicorp/terraform-provider-aws/internal/acctest"
)

func testAccGlueDataCatalogEncryptionSettingsDataSource_basic(t *testing.T) {
func testAccDataCatalogEncryptionSettingsDataSource_basic(t *testing.T) {
resourceName := "aws_glue_data_catalog_encryption_settings.test"
dataSourceName := "data.aws_glue_data_catalog_encryption_settings.test"

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import (
"github.com/hashicorp/terraform-provider-aws/internal/conns"
)

func testAccGlueDataCatalogEncryptionSettings_basic(t *testing.T) {
func testAccDataCatalogEncryptionSettings_basic(t *testing.T) {
var settings glue.DataCatalogEncryptionSettings

rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix)
Expand Down
5 changes: 3 additions & 2 deletions internal/service/glue/glue_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,15 @@ import "testing"
func TestAccGlue_serial(t *testing.T) {
testCases := map[string]map[string]func(t *testing.T){
"DataCatalogEncryptionSettings": {
"basic": testAccGlueDataCatalogEncryptionSettings_basic,
"DataSource": testAccGlueDataCatalogEncryptionSettingsDataSource_basic,
"basic": testAccDataCatalogEncryptionSettings_basic,
"dataSource": testAccDataCatalogEncryptionSettingsDataSource_basic,
},
"ResourcePolicy": {
"basic": testAccResourcePolicy_basic,
"update": testAccResourcePolicy_update,
"hybrid": testAccResourcePolicy_hybrid,
"disappears": testAccResourcePolicy_disappears,
"equivalent": testAccResourcePolicy_ignoreEquivalent,
},
}

Expand Down
23 changes: 20 additions & 3 deletions internal/service/glue/resource_policy.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"github.com/aws/aws-sdk-go/service/glue"
"github.com/hashicorp/aws-sdk-go-base/tfawserr"
"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/verify"
Expand All @@ -29,6 +30,10 @@ func ResourceResourcePolicy() *schema.Resource {
Required: true,
ValidateFunc: validation.StringIsJSON,
DiffSuppressFunc: verify.SuppressEquivalentPolicyDiffs,
StateFunc: func(v interface{}) string {
json, _ := structure.NormalizeJsonString(v)
return json
},
},
"enable_hybrid": {
Type: schema.TypeString,
Expand All @@ -43,16 +48,22 @@ func resourceResourcePolicyPut(condition string) func(d *schema.ResourceData, me
return func(d *schema.ResourceData, meta interface{}) error {
conn := meta.(*conns.AWSClient).GlueConn

policy, err := structure.NormalizeJsonString(d.Get("policy").(string))

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

input := &glue.PutResourcePolicyInput{
PolicyInJson: aws.String(d.Get("policy").(string)),
PolicyInJson: aws.String(policy),
PolicyExistsCondition: aws.String(condition),
}

if v, ok := d.GetOk("enable_hybrid"); ok {
input.EnableHybrid = aws.String(v.(string))
}

_, err := conn.PutResourcePolicy(input)
_, err = conn.PutResourcePolicy(input)
if err != nil {
return fmt.Errorf("error putting policy request: %s", err)
}
Expand All @@ -78,7 +89,13 @@ func resourceResourcePolicyRead(d *schema.ResourceData, meta interface{}) error
//Since the glue resource policy is global we expect it to be deleted when the policy is empty
d.SetId("")
} else {
d.Set("policy", resourcePolicy.PolicyInJson)
policyToSet, err := verify.PolicyToSet(d.Get("policy").(string), aws.StringValue(resourcePolicy.PolicyInJson))

if err != nil {
return err
}

d.Set("policy", policyToSet)
}
return nil
}
Expand Down
227 changes: 151 additions & 76 deletions internal/service/glue/resource_policy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,24 +15,6 @@ import (
tfglue "github.com/hashicorp/terraform-provider-aws/internal/service/glue"
)

func CreateTablePolicy(action string) string {
return fmt.Sprintf(`{
"Version" : "2012-10-17",
"Statement" : [
{
"Effect" : "Allow",
"Action" : [
"%s"
],
"Principal" : {
"AWS": "*"
},
"Resource" : "arn:%s:glue:%s:%s:*"
}
]
}`, action, acctest.Partition(), acctest.Region(), acctest.AccountID())
}

func testAccResourcePolicy_basic(t *testing.T) {
resourceName := "aws_glue_resource_policy.test"
resource.Test(t, resource.TestCase{
Expand All @@ -42,7 +24,7 @@ func testAccResourcePolicy_basic(t *testing.T) {
CheckDestroy: testAccCheckResourcePolicyDestroy,
Steps: []resource.TestStep{
{
Config: testAccResourcePolicy_Required("glue:CreateTable"),
Config: testAccResourcePolicyRequiredConfig("glue:CreateTable"),
Check: resource.ComposeTestCheckFunc(
testAccResourcePolicy(resourceName, "glue:CreateTable"),
),
Expand All @@ -65,7 +47,7 @@ func testAccResourcePolicy_hybrid(t *testing.T) {
CheckDestroy: testAccCheckResourcePolicyDestroy,
Steps: []resource.TestStep{
{
Config: testAccResourcePolicyHybrid("glue:CreateTable", "TRUE"),
Config: testAccResourcePolicyHybridConfig("glue:CreateTable", "TRUE"),
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr(resourceName, "enable_hybrid", "TRUE"),
),
Expand All @@ -77,13 +59,13 @@ func testAccResourcePolicy_hybrid(t *testing.T) {
ImportStateVerifyIgnore: []string{"enable_hybrid"},
},
{
Config: testAccResourcePolicyHybrid("glue:CreateTable", "FALSE"),
Config: testAccResourcePolicyHybridConfig("glue:CreateTable", "FALSE"),
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr(resourceName, "enable_hybrid", "FALSE"),
),
},
{
Config: testAccResourcePolicyHybrid("glue:CreateTable", "TRUE"),
Config: testAccResourcePolicyHybridConfig("glue:CreateTable", "TRUE"),
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr(resourceName, "enable_hybrid", "TRUE"),
),
Expand All @@ -100,7 +82,7 @@ func testAccResourcePolicy_disappears(t *testing.T) {
CheckDestroy: testAccCheckResourcePolicyDestroy,
Steps: []resource.TestStep{
{
Config: testAccResourcePolicy_Required("glue:CreateTable"),
Config: testAccResourcePolicyRequiredConfig("glue:CreateTable"),
Check: resource.ComposeTestCheckFunc(
testAccResourcePolicy(resourceName, "glue:CreateTable"),
acctest.CheckResourceDisappears(acctest.Provider, tfglue.ResourceResourcePolicy(), resourceName),
Expand All @@ -112,57 +94,6 @@ func testAccResourcePolicy_disappears(t *testing.T) {
})
}

func testAccResourcePolicy_Required(action string) string {
return fmt.Sprintf(`
data "aws_caller_identity" "current" {}

data "aws_partition" "current" {}

data "aws_region" "current" {}

data "aws_iam_policy_document" "glue-example-policy" {
statement {
actions = ["%s"]
resources = ["arn:${data.aws_partition.current.partition}:glue:${data.aws_region.current.name}:${data.aws_caller_identity.current.account_id}:*"]
principals {
identifiers = ["*"]
type = "AWS"
}
}
}

resource "aws_glue_resource_policy" "test" {
policy = data.aws_iam_policy_document.glue-example-policy.json
}
`, action)
}

func testAccResourcePolicyHybrid(action, hybrid string) string {
return fmt.Sprintf(`
data "aws_caller_identity" "current" {}

data "aws_partition" "current" {}

data "aws_region" "current" {}

data "aws_iam_policy_document" "glue-example-policy" {
statement {
actions = ["%[1]s"]
resources = ["arn:${data.aws_partition.current.partition}:glue:${data.aws_region.current.name}:${data.aws_caller_identity.current.account_id}:*"]
principals {
identifiers = ["*"]
type = "AWS"
}
}
}

resource "aws_glue_resource_policy" "test" {
policy = data.aws_iam_policy_document.glue-example-policy.json
enable_hybrid = %[2]q
}
`, action, hybrid)
}

func testAccResourcePolicy_update(t *testing.T) {
resourceName := "aws_glue_resource_policy.test"
resource.Test(t, resource.TestCase{
Expand All @@ -172,13 +103,13 @@ func testAccResourcePolicy_update(t *testing.T) {
CheckDestroy: testAccCheckResourcePolicyDestroy,
Steps: []resource.TestStep{
{
Config: testAccResourcePolicy_Required("glue:CreateTable"),
Config: testAccResourcePolicyRequiredConfig("glue:CreateTable"),
Check: resource.ComposeTestCheckFunc(
testAccResourcePolicy(resourceName, "glue:CreateTable"),
),
},
{
Config: testAccResourcePolicy_Required("glue:DeleteTable"),
Config: testAccResourcePolicyRequiredConfig("glue:DeleteTable"),
Check: resource.ComposeTestCheckFunc(
testAccResourcePolicy(resourceName, "glue:DeleteTable"),
),
Expand All @@ -192,6 +123,29 @@ func testAccResourcePolicy_update(t *testing.T) {
})
}

func testAccResourcePolicy_ignoreEquivalent(t *testing.T) {
resourceName := "aws_glue_resource_policy.test"

resource.Test(t, resource.TestCase{
PreCheck: func() { acctest.PreCheck(t) },
ErrorCheck: acctest.ErrorCheck(t, glue.EndpointsID),
Providers: acctest.Providers,
CheckDestroy: testAccCheckResourcePolicyDestroy,
Steps: []resource.TestStep{
{
Config: testAccResourcePolicyEquivalentConfig(),
Check: resource.ComposeTestCheckFunc(
testAccResourcePolicy(resourceName, "glue:CreateTable"),
),
},
{
Config: testAccResourcePolicyEquivalent2Config(),
PlanOnly: true,
},
},
})
}

func testAccResourcePolicy(n string, action string) resource.TestCheckFunc {
return func(s *terraform.State) error {
rs, ok := s.RootModule().Resources[n]
Expand Down Expand Up @@ -243,3 +197,124 @@ func testAccCheckResourcePolicyDestroy(s *terraform.State) error {
}
return nil
}

func CreateTablePolicy(action string) string {
return fmt.Sprintf(`{
"Version" : "2012-10-17",
"Statement" : [
{
"Effect" : "Allow",
"Action" : [
"%s"
],
"Principal" : {
"AWS": "*"
},
"Resource" : "arn:%s:glue:%s:%s:*"
}
]
}`, action, acctest.Partition(), acctest.Region(), acctest.AccountID())
}

func testAccResourcePolicyRequiredConfig(action string) string {
return fmt.Sprintf(`
data "aws_caller_identity" "current" {}

data "aws_partition" "current" {}

data "aws_region" "current" {}

data "aws_iam_policy_document" "glue-example-policy" {
statement {
actions = [%[1]q]
resources = ["arn:${data.aws_partition.current.partition}:glue:${data.aws_region.current.name}:${data.aws_caller_identity.current.account_id}:*"]
principals {
identifiers = ["*"]
type = "AWS"
}
}
}

resource "aws_glue_resource_policy" "test" {
policy = data.aws_iam_policy_document.glue-example-policy.json
}
`, action)
}

func testAccResourcePolicyHybridConfig(action, hybrid string) string {
return fmt.Sprintf(`
data "aws_caller_identity" "current" {}

data "aws_partition" "current" {}

data "aws_region" "current" {}

data "aws_iam_policy_document" "glue-example-policy" {
statement {
actions = [%[1]q]
resources = ["arn:${data.aws_partition.current.partition}:glue:${data.aws_region.current.name}:${data.aws_caller_identity.current.account_id}:*"]
principals {
identifiers = ["*"]
type = "AWS"
}
}
}

resource "aws_glue_resource_policy" "test" {
policy = data.aws_iam_policy_document.glue-example-policy.json
enable_hybrid = %[2]q
}
`, action, hybrid)
}

func testAccResourcePolicyEquivalentConfig() string {
return `
data "aws_caller_identity" "current" {}

data "aws_partition" "current" {}

data "aws_region" "current" {}

resource "aws_glue_resource_policy" "test" {
policy = jsonencode({
Version = "2012-10-17"
Statement = {
Action = "glue:CreateTable"
Effect = "Allow"
Resource = [
"arn:${data.aws_partition.current.partition}:glue:${data.aws_region.current.name}:${data.aws_caller_identity.current.account_id}:*"
]
Principal = {
AWS = "*"
}
}
})
}
`
}

func testAccResourcePolicyEquivalent2Config() string {
return `
data "aws_caller_identity" "current" {}

data "aws_partition" "current" {}

data "aws_region" "current" {}

resource "aws_glue_resource_policy" "test" {
policy = jsonencode({
Version = "2012-10-17"
Statement = {
Effect = "Allow"
Action = [
"glue:CreateTable",
]
Resource = "arn:${data.aws_partition.current.partition}:glue:${data.aws_region.current.name}:${data.aws_caller_identity.current.account_id}:*"
Principal = {
AWS = ["*"]
}
}
})
}
`
}