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

provider/google: ignore expanded v collapsed policies in diff #12387

Merged
merged 3 commits into from
Mar 14, 2017

Conversation

paddycarver
Copy link
Contributor

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.

I also added a test to ensure the fix worked. Output of the test without the fix in place:

==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/03/02 13:59:46 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/google -v -run=TestAccGoogleProjectIamPolicy_expanded -timeout 120m
=== RUN   TestAccGoogleProjectIamPolicy_expanded
--- FAIL: TestAccGoogleProjectIamPolicy_expanded (20.58s)
        testing.go:265: Step 0 error: After applying this step, the plan was not empty:

                DIFF:

                UPDATE: google_project_iam_policy.acceptance
                  policy_data: "{\"bindings\":[{\"members\":[\"user:[email protected]\",\"user:[email protected]\"],\"role\":\"roles/viewer\"}]}" => "{\"bindings\":[{\"members\":[\"user:[email protected]\"],\"role\":\"roles/viewer\"},{\"members\":[\"user:[email protected]\"],\"role\":\"roles/viewer\"}]}"

                STATE:

                data.google_iam_policy.expanded:
                  ID = 1322415602
                  binding.# = 2
                  binding.1102091722.members.# = 1
                  binding.1102091722.members.2912398366 = user:[email protected]
                  binding.1102091722.role = roles/viewer
                  binding.4049261743.members.# = 1
                  binding.4049261743.members.360503667 = user:[email protected]
                  binding.4049261743.role = roles/viewer
                  policy_data = {"bindings":[{"members":["user:[email protected]"],"role":"roles/viewer"},{"members":["user:[email protected]"],"role":"roles/viewer"}]}
                google_project.acceptance:
                  ID = terraform-pjewz40hq3
                  name = Terraform Acceptance Tests
                  number = 609045719484
                  org_id = 622235425790
                  project_id = terraform-pjewz40hq3
                google_project_iam_policy.acceptance:
                  ID = terraform-pjewz40hq3
                  authoritative = false
                  etag = BwVJxo1shsw=
                  policy_data = {"bindings":[{"members":["user:[email protected]","user:[email protected]"],"role":"roles/viewer"}]}
                  project = terraform-pjewz40hq3
                  restore_policy = {"bindings":[{"members":["serviceAccount:terraform-acceptance-tests@hc-terraform-testing.iam.gserviceaccount.com"],"role":"roles/owner"}],"etag":"BwVJxo0AAQ4=","version":1}

                  Dependencies:
                    google_project.acceptance
                    data.google_iam_policy.expanded
FAIL
exit status 1
FAIL    github.com/hashicorp/terraform/builtin/providers/google 20.584s
Makefile:48: recipe for target 'testacc' failed
make: *** [testacc] Error 1

Output after fix:

==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/03/02 14:00:32 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/google -v -run=TestAccGoogleProjectIamPolicy_expanded -timeout 120m
=== RUN   TestAccGoogleProjectIamPolicy_expanded
--- PASS: TestAccGoogleProjectIamPolicy_expanded (19.65s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/google 19.659s

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.
}
}

func testAccCheckGoogleProjectIamPolicyIsMerged(projectRes, policyRes, pid string) resource.TestCheckFunc {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot of the error checking in this function is duplicated from testAccCheckGoogleProjectIamPolicyExists, which is called from this fn. I wouldn't mind seeing something like (either or some combination of):

  • The error-checking is removed from this function since calling the other one takes care of it
  • A helper function could take care of getting the projectP and policyP values (and doing the error-checking) that gets called from both functions

data "google_iam_policy" "expanded" {
binding {
role = "roles/viewer"
members = [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: indentation (make this spaces instead of tabs)


binding {
role = "roles/viewer"
members = [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and here

Refactored some helpers out that help with retrieving the policies from
state and comparing them, hopefully leading to less code duplication.
Tabs vs spaces is the worst. I really need a way to run terraform fmt on
these inline configs.
@paddycarver
Copy link
Contributor Author

Comments should be addressed. :)

@danawillow
Copy link
Contributor

Looks good, thanks!

@paddycarver paddycarver merged commit 4f235c8 into master Mar 14, 2017
@grubernaut grubernaut deleted the paddy_11763_gcp_iam_diff branch May 11, 2017 00:34
@ghost
Copy link

ghost commented Apr 12, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

providers/google: equivalent IAM policies are treated as different
2 participants