Skip to content

Commit

Permalink
Merge pull request #22167 from hashicorp/b-glue-resource-policy-diffs
Browse files Browse the repository at this point in the history 
glue/resource_policy: Fix diffs with equivalent policies
  • Loading branch information
@YakDriver
YakDriver authored Dec 13, 2021
2 parents c9d2668 + cbb9968 commit fee79fa
Show file tree
Hide file tree
Showing 6 changed files with 179 additions and 83 deletions.
3 changes: 3 additions & 0 deletions .changelog/22167.txt
View file
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
```
2 changes: 1 addition & 1 deletion internal/service/glue/data_catalog_encryption_settings_data_source_test.go
View file
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
2 changes: 1 addition & 1 deletion internal/service/glue/data_catalog_encryption_settings_test.go
View file
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
View file
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
View file
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
View file
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 = ["*"]
}
}
})
}
`
}

0 comments on commit fee79fa

Please sign in to comment.