From 9b1da31ca418165e793fb262020652742a4b1364 Mon Sep 17 00:00:00 2001 From: Paddy Date: Thu, 2 Mar 2017 14:00:45 -0800 Subject: [PATCH 1/3] provider/google: ignore expanded v collapsed policies in diff When comparing the config and state for google_project_iam_policy, always merge the bindings down to a common representation, to avoid a perpetual diff. Fixes #11763. --- .../resource_google_project_iam_policy.go | 2 + ...resource_google_project_iam_policy_test.go | 93 ++++++++++++++++++- 2 files changed, 94 insertions(+), 1 deletion(-) diff --git a/builtin/providers/google/resource_google_project_iam_policy.go b/builtin/providers/google/resource_google_project_iam_policy.go index cf9c87ef8..4b2ec79b7 100644 --- a/builtin/providers/google/resource_google_project_iam_policy.go +++ b/builtin/providers/google/resource_google_project_iam_policy.go @@ -373,6 +373,8 @@ func jsonPolicyDiffSuppress(k, old, new string, d *schema.ResourceData) bool { log.Printf("[ERROR] Could not unmarshal new policy %s: %v", new, err) return false } + oldPolicy.Bindings = mergeBindings(oldPolicy.Bindings) + newPolicy.Bindings = mergeBindings(newPolicy.Bindings) if newPolicy.Etag != oldPolicy.Etag { return false } diff --git a/builtin/providers/google/resource_google_project_iam_policy_test.go b/builtin/providers/google/resource_google_project_iam_policy_test.go index 59903ca8a..f0a897e2c 100644 --- a/builtin/providers/google/resource_google_project_iam_policy_test.go +++ b/builtin/providers/google/resource_google_project_iam_policy_test.go @@ -254,7 +254,24 @@ func TestAccGoogleProjectIamPolicy_basic(t *testing.T) { }) } -func testAccCheckGoogleProjectIamPolicyIsMerged(projectRes, policyRes, pid string) resource.TestCheckFunc { +// Test that a non-collapsed IAM policy doesn't perpetually diff +func TestAccGoogleProjectIamPolicy_expanded(t *testing.T) { + pid := "terraform-" + acctest.RandString(10) + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: testAccGoogleProjectAssociatePolicyExpanded(pid, pname, org), + Check: resource.ComposeTestCheckFunc( + testAccCheckGoogleProjectIamPolicyExists("google_project_iam_policy.acceptance", "data.google_iam_policy.expanded", pid), + ), + }, + }, + }) +} + +func testAccCheckGoogleProjectIamPolicyExists(projectRes, policyRes, pid string) resource.TestCheckFunc { return func(s *terraform.State) error { // Get the project resource project, ok := s.RootModule().Resources[projectRes] @@ -290,11 +307,56 @@ func testAccCheckGoogleProjectIamPolicyIsMerged(projectRes, policyRes, pid strin } // The bindings in both policies should be identical + projectP.Bindings = mergeBindings(projectP.Bindings) + policyP.Bindings = mergeBindings(policyP.Bindings) sort.Sort(sortableBindings(projectP.Bindings)) sort.Sort(sortableBindings(policyP.Bindings)) if !reflect.DeepEqual(derefBindings(projectP.Bindings), derefBindings(policyP.Bindings)) { return fmt.Errorf("Project and data source policies do not match: project policy is %+v, data resource policy is %+v", derefBindings(projectP.Bindings), derefBindings(policyP.Bindings)) } + return nil + } +} + +func testAccCheckGoogleProjectIamPolicyIsMerged(projectRes, policyRes, pid string) resource.TestCheckFunc { + return func(s *terraform.State) error { + // Get the project resource + project, ok := s.RootModule().Resources[projectRes] + if !ok { + return fmt.Errorf("Not found: %s", projectRes) + } + // The project ID should match the config's project ID + if project.Primary.ID != pid { + return fmt.Errorf("Expected project %q to match ID %q in state", pid, project.Primary.ID) + } + + err := testAccCheckGoogleProjectIamPolicyExists(projectRes, policyRes, pid)(s) + if err != nil { + return err + } + + var projectP, policyP cloudresourcemanager.Policy + // The project should have a policy + ps, ok := project.Primary.Attributes["policy_data"] + if !ok { + return fmt.Errorf("Project resource %q did not have a 'policy_data' attribute. Attributes were %#v", project.Primary.Attributes["id"], project.Primary.Attributes) + } + if err := json.Unmarshal([]byte(ps), &projectP); err != nil { + return fmt.Errorf("Could not unmarshal %s:\n: %v", ps, err) + } + + // The data policy resource should have a policy + policy, ok := s.RootModule().Resources[policyRes] + if !ok { + return fmt.Errorf("Not found: %s", policyRes) + } + ps, ok = policy.Primary.Attributes["policy_data"] + if !ok { + return fmt.Errorf("Data policy resource %q did not have a 'policy_data' attribute. Attributes were %#v", policy.Primary.Attributes["id"], project.Primary.Attributes) + } + if err := json.Unmarshal([]byte(ps), &policyP); err != nil { + return err + } // Merge the project policy in Terraform state with the policy the project had before the config was applied expected := make([]*cloudresourcemanager.Binding, 0) @@ -634,3 +696,32 @@ resource "google_project" "acceptance" { billing_account = "%s" }`, pid, name, org, billing) } + +func testAccGoogleProjectAssociatePolicyExpanded(pid, name, org string) string { + return fmt.Sprintf(` +resource "google_project" "acceptance" { + project_id = "%s" + name = "%s" + org_id = "%s" +} +resource "google_project_iam_policy" "acceptance" { + project = "${google_project.acceptance.id}" + policy_data = "${data.google_iam_policy.expanded.policy_data}" + authoritative = false +} +data "google_iam_policy" "expanded" { + binding { + role = "roles/viewer" + members = [ + "user:paddy@carvers.co", + ] + } + + binding { + role = "roles/viewer" + members = [ + "user:paddy@hashicorp.com", + ] + } +}`, pid, name, org) +} From 6ca92fbbc155d83082e506b78fa84758877d106c Mon Sep 17 00:00:00 2001 From: Paddy Date: Mon, 13 Mar 2017 23:20:27 -0700 Subject: [PATCH 2/3] Refactored into helpers. Refactored some helpers out that help with retrieving the policies from state and comparing them, hopefully leading to less code duplication. --- ...resource_google_project_iam_policy_test.go | 132 ++++++++---------- 1 file changed, 56 insertions(+), 76 deletions(-) diff --git a/builtin/providers/google/resource_google_project_iam_policy_test.go b/builtin/providers/google/resource_google_project_iam_policy_test.go index f0a897e2c..63811db4d 100644 --- a/builtin/providers/google/resource_google_project_iam_policy_test.go +++ b/builtin/providers/google/resource_google_project_iam_policy_test.go @@ -271,48 +271,60 @@ func TestAccGoogleProjectIamPolicy_expanded(t *testing.T) { }) } +func getStatePrimaryResource(s *terraform.State, res, expectedID string) (*terraform.InstanceState, error) { + // Get the project resource + resource, ok := s.RootModule().Resources[res] + if !ok { + return nil, fmt.Errorf("Not found: %s", res) + } + if resource.Primary.Attributes["id"] != expectedID && expectedID != "" { + return nil, fmt.Errorf("Expected project %q to match ID %q in state", resource.Primary.ID, expectedID) + } + return resource.Primary, nil +} + +func getGoogleProjectIamPolicyFromResource(resource *terraform.InstanceState) (cloudresourcemanager.Policy, error) { + var p cloudresourcemanager.Policy + ps, ok := resource.Attributes["policy_data"] + if !ok { + return p, fmt.Errorf("Resource %q did not have a 'policy_data' attribute. Attributes were %#v", resource.ID, resource.Attributes) + } + if err := json.Unmarshal([]byte(ps), &p); err != nil { + return p, fmt.Errorf("Could not unmarshal %s:\n: %v", ps, err) + } + return p, nil +} + +func getGoogleProjectIamPolicyFromState(s *terraform.State, res, expectedID string) (cloudresourcemanager.Policy, error) { + project, err := getStatePrimaryResource(s, res, expectedID) + if err != nil { + return cloudresourcemanager.Policy{}, err + } + return getGoogleProjectIamPolicyFromResource(project) +} + +func compareBindings(a, b []*cloudresourcemanager.Binding) bool { + a = mergeBindings(a) + b = mergeBindings(b) + sort.Sort(sortableBindings(a)) + sort.Sort(sortableBindings(b)) + return reflect.DeepEqual(derefBindings(a), derefBindings(b)) +} + func testAccCheckGoogleProjectIamPolicyExists(projectRes, policyRes, pid string) resource.TestCheckFunc { return func(s *terraform.State) error { - // Get the project resource - project, ok := s.RootModule().Resources[projectRes] - if !ok { - return fmt.Errorf("Not found: %s", projectRes) + projectPolicy, err := getGoogleProjectIamPolicyFromState(s, projectRes, pid) + if err != nil { + return fmt.Errorf("Error retrieving IAM policy for project from state: %s", err) } - // The project ID should match the config's project ID - if project.Primary.ID != pid { - return fmt.Errorf("Expected project %q to match ID %q in state", pid, project.Primary.ID) - } - - var projectP, policyP cloudresourcemanager.Policy - // The project should have a policy - ps, ok := project.Primary.Attributes["policy_data"] - if !ok { - return fmt.Errorf("Project resource %q did not have a 'policy_data' attribute. Attributes were %#v", project.Primary.Attributes["id"], project.Primary.Attributes) - } - if err := json.Unmarshal([]byte(ps), &projectP); err != nil { - return fmt.Errorf("Could not unmarshal %s:\n: %v", ps, err) - } - - // The data policy resource should have a policy - policy, ok := s.RootModule().Resources[policyRes] - if !ok { - return fmt.Errorf("Not found: %s", policyRes) - } - ps, ok = policy.Primary.Attributes["policy_data"] - if !ok { - return fmt.Errorf("Data policy resource %q did not have a 'policy_data' attribute. Attributes were %#v", policy.Primary.Attributes["id"], project.Primary.Attributes) - } - if err := json.Unmarshal([]byte(ps), &policyP); err != nil { - return err + policyPolicy, err := getGoogleProjectIamPolicyFromState(s, policyRes, "") + if err != nil { + return fmt.Errorf("Error retrieving IAM policy for data_policy from state: %s", err) } // The bindings in both policies should be identical - projectP.Bindings = mergeBindings(projectP.Bindings) - policyP.Bindings = mergeBindings(policyP.Bindings) - sort.Sort(sortableBindings(projectP.Bindings)) - sort.Sort(sortableBindings(policyP.Bindings)) - if !reflect.DeepEqual(derefBindings(projectP.Bindings), derefBindings(policyP.Bindings)) { - return fmt.Errorf("Project and data source policies do not match: project policy is %+v, data resource policy is %+v", derefBindings(projectP.Bindings), derefBindings(policyP.Bindings)) + if !compareBindings(projectPolicy.Bindings, policyPolicy.Bindings) { + return fmt.Errorf("Project and data source policies do not match: project policy is %+v, data resource policy is %+v", derefBindings(projectPolicy.Bindings), derefBindings(policyPolicy.Bindings)) } return nil } @@ -320,49 +332,21 @@ func testAccCheckGoogleProjectIamPolicyExists(projectRes, policyRes, pid string) func testAccCheckGoogleProjectIamPolicyIsMerged(projectRes, policyRes, pid string) resource.TestCheckFunc { return func(s *terraform.State) error { - // Get the project resource - project, ok := s.RootModule().Resources[projectRes] - if !ok { - return fmt.Errorf("Not found: %s", projectRes) - } - // The project ID should match the config's project ID - if project.Primary.ID != pid { - return fmt.Errorf("Expected project %q to match ID %q in state", pid, project.Primary.ID) - } - err := testAccCheckGoogleProjectIamPolicyExists(projectRes, policyRes, pid)(s) if err != nil { return err } - var projectP, policyP cloudresourcemanager.Policy - // The project should have a policy - ps, ok := project.Primary.Attributes["policy_data"] - if !ok { - return fmt.Errorf("Project resource %q did not have a 'policy_data' attribute. Attributes were %#v", project.Primary.Attributes["id"], project.Primary.Attributes) - } - if err := json.Unmarshal([]byte(ps), &projectP); err != nil { - return fmt.Errorf("Could not unmarshal %s:\n: %v", ps, err) - } - - // The data policy resource should have a policy - policy, ok := s.RootModule().Resources[policyRes] - if !ok { - return fmt.Errorf("Not found: %s", policyRes) - } - ps, ok = policy.Primary.Attributes["policy_data"] - if !ok { - return fmt.Errorf("Data policy resource %q did not have a 'policy_data' attribute. Attributes were %#v", policy.Primary.Attributes["id"], project.Primary.Attributes) - } - if err := json.Unmarshal([]byte(ps), &policyP); err != nil { - return err + projectPolicy, err := getGoogleProjectIamPolicyFromState(s, projectRes, pid) + if err != nil { + return fmt.Errorf("Error retrieving IAM policy for project from state: %s", err) } // Merge the project policy in Terraform state with the policy the project had before the config was applied - expected := make([]*cloudresourcemanager.Binding, 0) + var expected []*cloudresourcemanager.Binding expected = append(expected, originalPolicy.Bindings...) - expected = append(expected, projectP.Bindings...) - expectedM := mergeBindings(expected) + expected = append(expected, projectPolicy.Bindings...) + expected = mergeBindings(expected) // Retrieve the actual policy from the project c := testAccProvider.Meta().(*Config) @@ -370,13 +354,9 @@ func testAccCheckGoogleProjectIamPolicyIsMerged(projectRes, policyRes, pid strin if err != nil { return fmt.Errorf("Failed to retrieve IAM Policy for project %q: %s", pid, err) } - actualM := mergeBindings(actual.Bindings) - - sort.Sort(sortableBindings(actualM)) - sort.Sort(sortableBindings(expectedM)) // The bindings should match, indicating the policy was successfully applied and merged - if !reflect.DeepEqual(derefBindings(actualM), derefBindings(expectedM)) { - return fmt.Errorf("Actual and expected project policies do not match: actual policy is %+v, expected policy is %+v", derefBindings(actualM), derefBindings(expectedM)) + if !compareBindings(actual.Bindings, expected) { + return fmt.Errorf("Actual and expected project policies do not match: actual policy is %+v, expected policy is %+v", derefBindings(actual.Bindings), derefBindings(expected)) } return nil From 47e5547ce792752ed1730989e1fe89d015afba01 Mon Sep 17 00:00:00 2001 From: Paddy Date: Mon, 13 Mar 2017 23:23:32 -0700 Subject: [PATCH 3/3] Fix variable indents. Tabs vs spaces is the worst. I really need a way to run terraform fmt on these inline configs. --- ...resource_google_project_iam_policy_test.go | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/builtin/providers/google/resource_google_project_iam_policy_test.go b/builtin/providers/google/resource_google_project_iam_policy_test.go index 63811db4d..24052c961 100644 --- a/builtin/providers/google/resource_google_project_iam_policy_test.go +++ b/builtin/providers/google/resource_google_project_iam_policy_test.go @@ -633,8 +633,8 @@ func testAccGoogleProjectAssociatePolicyBasic(pid, name, org string) string { return fmt.Sprintf(` resource "google_project" "acceptance" { project_id = "%s" - name = "%s" - org_id = "%s" + name = "%s" + org_id = "%s" } resource "google_project_iam_policy" "acceptance" { project = "${google_project.acceptance.id}" @@ -662,8 +662,8 @@ func testAccGoogleProject_create(pid, name, org string) string { return fmt.Sprintf(` resource "google_project" "acceptance" { project_id = "%s" - name = "%s" - org_id = "%s" + name = "%s" + org_id = "%s" }`, pid, name, org) } @@ -671,9 +671,9 @@ func testAccGoogleProject_createBilling(pid, name, org, billing string) string { return fmt.Sprintf(` resource "google_project" "acceptance" { project_id = "%s" - name = "%s" - org_id = "%s" - billing_account = "%s" + name = "%s" + org_id = "%s" + billing_account = "%s" }`, pid, name, org, billing) } @@ -692,16 +692,16 @@ resource "google_project_iam_policy" "acceptance" { data "google_iam_policy" "expanded" { binding { role = "roles/viewer" - members = [ - "user:paddy@carvers.co", - ] + members = [ + "user:paddy@carvers.co", + ] } binding { role = "roles/viewer" - members = [ - "user:paddy@hashicorp.com", - ] + members = [ + "user:paddy@hashicorp.com", + ] } }`, pid, name, org) }