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

Stop provider sending empty description fields in IAM conditions, to avoid permadiffs in plans #12653

Conversation

modular-magician
Copy link
Collaborator

Description

Partially fixes #8701

This PR changes the expandIamCondition function, which is used in all IAM _binding and _member resources to read parts of the resources state and convert it into a cloudresourcemanager.Expr variable.

By removing "Description" from the ForceSendFields it stops those variables containing empty Description fields. In the case of the google_iam_policy data source, this means that all the zero valued description fields are ignored when the policy_data JSON is generated from the config input.

This prevents the policy_data JSON containing empty description fields:

"policy_data": "{\"bindings\":[{\"condition\":{👉 \"description\":\"\"👈 ,\"expression\":\"request.host == 'foobar-b.example.com'\",\"title\":\"foobar-b\"},\"members\":[\"group:<email-here>\"],\"role\":\"roles/iap.httpsResourceAccessor\"}]}"

versus

"policy_data": "{\"bindings\":[{\"condition\":{\"expression\":\"request.host == 'foobar-b.example.com'\",\"title\":\"foobar-b\"},\"members\":[\"group:<email-here>\"],\"role\":\"roles/iap.httpsResourceAccessor\"}]}"

If the JSON contains empty description fields there is a perma-diff that is only present in plans after a user makes a legitimate change to the config. Then, the plan contains lots of additional meaningless diffs to add empty descriptions:

Screenshot 2022-08-25 at 17 20 06

If this PR is for Terraform, I acknowledge that I have:

  • Searched through the issue tracker for an open issue that this either resolves or contributes to, commented on it to claim it, and written "fixes {url}" or "part of {url}" in this PR description. If there were no relevant open issues, I opened one and commented that I would like to work on it (not necessary for very small changes).
  • Generated Terraform, and ran make test and make lint to ensure it passes unit and linter tests.
  • Ensured that all new fields I added that can be set by a user appear in at least one example (for generated resources) or third_party test (for handwritten resources or update tests). N/A
  • Ran relevant acceptance tests (If the acceptance tests do not yet pass or you are unable to run them, please let your reviewer know).
  • Read the Release Notes Guide before writing my release note below.

Release Note Template for Downstream PRs (will be copied)

iam: fixed permadiff resulting from empty fields being sent in requests to set conditional IAM policies

Derived from GoogleCloudPlatform/magic-modules#6473

…avoid permadiffs in plans (hashicorp#6473)

* Stop provider sending empty description field, to avoid permadiffs in plans

* Add test asserting that `description` fields are omitted from `policy_data` if it's not set in the config

* Update existing IAM tests to include conditions with empty descriptions

* Fix "Too many condition blocks" error in `google_project_iam_binding` test

* Make titles unique between multiple `google_project_iam_binding` resources

* Update new test to assert both policy_data match after resource creation finishes

Previously this wasn't the case; the resource was Read from the API after create and saved to state, resulting in changes in the policy from the API being saved to state and causing diffs

* Change tests to hardcode the title of the second condition

This makes this branchs changes consistent and minimises the change on function signatures. Also, the new bindings are not imported so the data is not needed in the body of the test.

* Allow IAM condition test generation in GA provider

* Allow IAM condition docs generation in GA provider

* Update context for IAM condition acctests to include descriptions and fields for bindings without descriptions

* Update import step of acctests to navigate possible SDK bug

* Fix variable name and expected policy order in admin IAM condition tests

* Add missing member type to IAM condition test, update code comments

* Change test back to using ImportStateVerify instead of ImportStateCheck for IAM condition acctests

* Add checks to test steps, remove changes to handwritten tests & new test

* Update comment about bindings with no descriptions

* Add more explicit check that empty descriptions aren't stored in google_iam_policy data sources

* Fix comment syntax

* Add resources and import steps for `*_member` and `*_binding` IAM conditions without descriptions

Signed-off-by: Modular Magician <magic-modules@google.com>
@modular-magician modular-magician merged commit dc5a374 into hashicorp:main Sep 27, 2022
@github-actions
Copy link

I'm going to lock this pull request 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 related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

google_iam_policy with multiple bindings show changes on every plan
1 participant