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

[SarahFrench]

Description

Partially fixes https://github.com/hashicorp/terraform-provider-google/issues/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:

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

• [x] 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).
• [x] Generated Terraform, and ran make test and make lint to ensure it passes unit and linter tests.
• [x] ~~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
• [x] Ran relevant acceptance tests (If the acceptance tests do not yet pass or you are unable to run them, please let your reviewer know).
• [x] 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

[modular-magician]

Hello! I am a robot who works on Magic Modules PRs.

I've detected that you're a community contributor. @melinath, a repository maintainer, has been assigned to assist you and help review your changes.

:question: First time contributing? Click here for more details

---

Your assigned reviewer will help review your code by:

• Ensuring it's backwards compatible, covers common error cases, etc.
• Summarizing the change into a user-facing changelog note.
• Passes tests, either our "VCR" suite, a set of presubmit tests, or with manual test runs.

You can help make sure that review is quick by running local tests and ensuring they're passing in between each push you make to your PR's branch. Also, try to leave a comment with each push you make, as pushes generally don't generate emails.

If your reviewer doesn't get back to you within a week after your most recent change, please feel free to leave a comment on the issue asking them to take a look! In the absence of a dedicated review dashboard most maintainers manage their pending reviews through email, and those will sometimes get lost in their inbox.

---

[modular-magician]

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 1 file changed, 1 insertion(+), 1 deletion(-)) Terraform Beta: Diff ( 1 file changed, 1 insertion(+), 1 deletion(-)) TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

[modular-magician]

Tests analytics

Total tests: 2146 Passed tests 1906 Skipped tests: 232 Failed tests: 8

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests

TestAccFirebaserulesRelease_BasicRelease|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample|TestAccComputeInstance_soleTenantNodeAffinities|TestAccCloudRunService_cloudRunServiceStaticOutboundExample|TestAccCloudFunctions2Function_fullUpdate|TestAccComputeGlobalForwardingRule_internalLoadBalancing|TestAccSqlDatabaseInstance_SqlServerAuditConfig|TestAccSqlDatabaseInstance_withPrivateNetwork_withAllocatedIpRange

[modular-magician]

Tests passed during RECORDING mode: TestAccFirebaserulesRelease_BasicRelease[view] TestAccCloudFunctions2Function_fullUpdate[view]

Tests failed during RECORDING mode: TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample[view] TestAccComputeInstance_soleTenantNodeAffinities[view] TestAccCloudRunService_cloudRunServiceStaticOutboundExample[view] TestAccComputeGlobalForwardingRule_internalLoadBalancing[view] TestAccSqlDatabaseInstance_SqlServerAuditConfig[view] TestAccSqlDatabaseInstance_withPrivateNetwork_withAllocatedIpRange[view]

Please fix these to complete your PR View the build log or the debug log for each test

[modular-magician]

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 2 files changed, 67 insertions(+), 1 deletion(-)) Terraform Beta: Diff ( 2 files changed, 67 insertions(+), 1 deletion(-)) TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

[SarahFrench]

I don't think the existing acceptance tests for IAM *_binding and *_member resources (which this change touches) are relevant to the change in this PR. All those tests have conditions where the description field set, vs being left empty, and the tests are all generated from the same template. I'm uncertain if I should edit them to also include an empty description, or if I should handwrite a IAM acceptance test for a single example of *_binding and *_member resources?

(The acceptance test I've added in this PR is only relevant to the google_iam_policy data source)

[modular-magician]

Tests analytics

Total tests: 2147 Passed tests 1906 Skipped tests: 232 Failed tests: 9

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests

TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample|TestAccFirebaserulesRelease_BasicRelease|TestAccComputeInstance_soleTenantNodeAffinities|TestAccCGCSnippet_eventarcWorkflowsExample|TestAccSqlDatabaseInstance_SqlServerAuditConfig|TestAccSqlDatabaseInstance_withPrivateNetwork_withAllocatedIpRange|TestAccComputeGlobalForwardingRule_internalLoadBalancing|TestAccComputeImageIamPolicy_googleIamPolicyDataSource|TestAccCloudRunService_cloudRunServiceStaticOutboundExample

[modular-magician]

Tests passed during RECORDING mode: TestAccFirebaserulesRelease_BasicRelease[view] TestAccComputeImageIamPolicy_googleIamPolicyDataSource[view]

Tests failed during RECORDING mode: TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample[view] TestAccComputeInstance_soleTenantNodeAffinities[view] TestAccCGCSnippet_eventarcWorkflowsExample[view] TestAccSqlDatabaseInstance_SqlServerAuditConfig[view] TestAccSqlDatabaseInstance_withPrivateNetwork_withAllocatedIpRange[view] TestAccComputeGlobalForwardingRule_internalLoadBalancing[view] TestAccCloudRunService_cloudRunServiceStaticOutboundExample[view]

Please fix these to complete your PR View the build log or the debug log for each test

[modular-magician]

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 5 files changed, 89 insertions(+), 3 deletions(-)) Terraform Beta: Diff ( 5 files changed, 89 insertions(+), 3 deletions(-)) TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

[modular-magician]

Tests analytics

Total tests: 2157 Passed tests 1915 Skipped tests: 234 Failed tests: 8

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests

TestAccSqlDatabaseInstance_withPrivateNetwork_withAllocatedIpRange|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample|TestAccFirebaserulesRelease_BasicRelease|TestAccProjectIamBinding_withCondition|TestAccComputeGlobalForwardingRule_internalLoadBalancing|TestAccCloudRunService_cloudRunServiceStaticOutboundExample|TestAccComputeInstance_soleTenantNodeAffinities|TestAccSqlDatabaseInstance_SqlServerAuditConfig

[modular-magician]

Tests passed during RECORDING mode: TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample[Debug log] TestAccFirebaserulesRelease_BasicRelease[Debug log]

Tests failed during RECORDING mode: TestAccSqlDatabaseInstance_withPrivateNetwork_withAllocatedIpRange[Error message] [Debug log] TestAccProjectIamBinding_withCondition[Error message] [Debug log] TestAccComputeGlobalForwardingRule_internalLoadBalancing[Error message] [Debug log] TestAccCloudRunService_cloudRunServiceStaticOutboundExample[Error message] [Debug log] TestAccComputeInstance_soleTenantNodeAffinities[Error message] [Debug log] TestAccSqlDatabaseInstance_SqlServerAuditConfig[Error message] [Debug log]

Please fix these to complete your PR View the build log or the debug log for each test

[modular-magician]

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 5 files changed, 95 insertions(+), 3 deletions(-)) Terraform Beta: Diff ( 5 files changed, 95 insertions(+), 3 deletions(-)) TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

[modular-magician]

Tests analytics

Total tests: 2157 Passed tests 1915 Skipped tests: 234 Failed tests: 8

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests

TestAccProjectIamBinding_withCondition|TestAccComputeGlobalForwardingRule_internalLoadBalancing|TestAccComputeInstance_soleTenantNodeAffinities|TestAccCloudRunService_cloudRunServiceStaticOutboundExample|TestAccCGCSnippet_eventarcWorkflowsExample|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample|TestAccSqlDatabaseInstance_SqlServerAuditConfig|TestAccSqlDatabaseInstance_withPrivateNetwork_withAllocatedIpRange

[modular-magician]

Tests passed during RECORDING mode: TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample[Debug log]

Tests failed during RECORDING mode: TestAccProjectIamBinding_withCondition[Error message] [Debug log] TestAccComputeGlobalForwardingRule_internalLoadBalancing[Error message] [Debug log] TestAccComputeInstance_soleTenantNodeAffinities[Error message] [Debug log] TestAccCloudRunService_cloudRunServiceStaticOutboundExample[Error message] [Debug log] TestAccCGCSnippet_eventarcWorkflowsExample[Error message] [Debug log] TestAccSqlDatabaseInstance_SqlServerAuditConfig[Error message] [Debug log] TestAccSqlDatabaseInstance_withPrivateNetwork_withAllocatedIpRange[Error message] [Debug log]

Please fix these to complete your PR View the build log or the debug log for each test

[modular-magician]

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 5 files changed, 101 insertions(+), 7 deletions(-)) Terraform Beta: Diff ( 5 files changed, 101 insertions(+), 7 deletions(-)) TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

[modular-magician]

Tests analytics

Total tests: 2157 Passed tests 1915 Skipped tests: 234 Failed tests: 8

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests

TestAccSqlDatabaseInstance_withPrivateNetwork_withAllocatedIpRange|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample|TestAccProjectIamBinding_withCondition|TestAccComputeInstance_soleTenantNodeAffinities|TestAccComputeGlobalForwardingRule_internalLoadBalancing|TestAccCloudRunService_cloudRunServiceStaticOutboundExample|TestAccCGCSnippet_eventarcWorkflowsExample|TestAccSqlDatabaseInstance_SqlServerAuditConfig

[modular-magician]

Tests passed during RECORDING mode: TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample[Debug log] TestAccProjectIamBinding_withCondition[Debug log]

Tests failed during RECORDING mode: TestAccSqlDatabaseInstance_withPrivateNetwork_withAllocatedIpRange[Error message] [Debug log] TestAccComputeInstance_soleTenantNodeAffinities[Error message] [Debug log] TestAccComputeGlobalForwardingRule_internalLoadBalancing[Error message] [Debug log] TestAccCloudRunService_cloudRunServiceStaticOutboundExample[Error message] [Debug log] TestAccCGCSnippet_eventarcWorkflowsExample[Error message] [Debug log] TestAccSqlDatabaseInstance_SqlServerAuditConfig[Error message] [Debug log]

Please fix these to complete your PR View the build log or the debug log for each test

[SarahFrench]

For reference, these are the tests affected by this PR:

• (new) TestAccComputeImageIamPolicy_googleIamPolicyDataSource
• TestAccProjectIamBinding_withCondition
• TestAccOrganizationIamMembersAndBindings
  • testAccOrganizationIamBinding_condition test case
  • testAccOrganizationIamMember_condition test case
• TestAccFolderIamMember_multiple
• TestAccFolderIamMember_remove

[modular-magician]

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 5 files changed, 104 insertions(+), 7 deletions(-)) Terraform Beta: Diff ( 5 files changed, 104 insertions(+), 7 deletions(-)) TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

[modular-magician]

Tests analytics

Total tests: 2157 Passed tests 1916 Skipped tests: 234 Failed tests: 7

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests

TestAccSqlDatabaseInstance_SqlServerAuditConfig|TestAccComputeInstance_soleTenantNodeAffinities|TestAccComputeGlobalForwardingRule_internalLoadBalancing|TestAccCloudRunService_cloudRunServiceStaticOutboundExample|TestAccCGCSnippet_eventarcWorkflowsExample|TestAccSqlDatabaseInstance_withPrivateNetwork_withAllocatedIpRange|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample

[modular-magician]

Tests passed during RECORDING mode: TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample[Debug log]

Tests failed during RECORDING mode: TestAccSqlDatabaseInstance_SqlServerAuditConfig[Error message] [Debug log] TestAccComputeInstance_soleTenantNodeAffinities[Error message] [Debug log] TestAccComputeGlobalForwardingRule_internalLoadBalancing[Error message] [Debug log] TestAccCloudRunService_cloudRunServiceStaticOutboundExample[Error message] [Debug log] TestAccCGCSnippet_eventarcWorkflowsExample[Error message] [Debug log] TestAccSqlDatabaseInstance_withPrivateNetwork_withAllocatedIpRange[Error message] [Debug log]

Please fix these to complete your PR View the build log or the debug log for each test

[modular-magician]

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 5 files changed, 97 insertions(+), 2 deletions(-)) Terraform Beta: Diff ( 5 files changed, 97 insertions(+), 2 deletions(-)) TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

[modular-magician]

Tests analytics

Total tests: 2157 Passed tests 1915 Skipped tests: 234 Failed tests: 8

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests

TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample|TestAccProjectIamBinding_withCondition|TestAccFirebaserulesRelease_BasicRelease|TestAccComputeGlobalForwardingRule_internalLoadBalancing|TestAccComputeInstance_soleTenantNodeAffinities|TestAccCloudRunService_cloudRunServiceStaticOutboundExample|TestAccSqlDatabaseInstance_SqlServerAuditConfig|TestAccSqlDatabaseInstance_withPrivateNetwork_withAllocatedIpRange

[modular-magician]

Tests passed during RECORDING mode: TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample[Debug log] TestAccProjectIamBinding_withCondition[Debug log] TestAccFirebaserulesRelease_BasicRelease[Debug log]

Tests failed during RECORDING mode: TestAccComputeGlobalForwardingRule_internalLoadBalancing[Error message] [Debug log] TestAccComputeInstance_soleTenantNodeAffinities[Error message] [Debug log] TestAccCloudRunService_cloudRunServiceStaticOutboundExample[Error message] [Debug log] TestAccSqlDatabaseInstance_SqlServerAuditConfig[Error message] [Debug log] TestAccSqlDatabaseInstance_withPrivateNetwork_withAllocatedIpRange[Error message] [Debug log]

Please fix these to complete your PR View the build log or the debug log for each test

[SarahFrench]

Hi @melinath ! Sorry to chase, but I wanted to get some feedback about how to test this change.

I tried editing the IAM tests in a 'centralised' way by editing templates/terraform/examples/base_configs/iam_test_file.go.erb to add extra condition blocks with empty descriptions, but after generating the provider code I couldn't see any diffs resulting from those changes.

It seems like the remaining option is to edit handwritten IAM tests to add the new condition blocks. I've done that for a few tests now and I was wondering:

• Should I extend those edits to all the handwritten tests? This would avoid this type of test being accidentally lost in a refactor.
• Should I add checks like below to the handwritten tests? This would let us avoid adding a new test via this PR.

https://github.com/GoogleCloudPlatform/magic-modules/blob/62cf88b71644e6333314aeb488c9044ce0da5d55/mmv1/third_party/terraform/tests/data_source_google_iam_policy_test.go#L29-L32

[melinath]

hmm, that's odd - changing iam_test_file.go.erb should result in changes being generated - at least in the beta provider. I think conditions are not GA yet.

If that doesn't work - adding additional tests & checks to all the handwritten files seems fine.

But if it's not possible to adjust the iam test template that's also a problem that would be worth investigating and fixing.

[melinath]

FWIW I am able to get a diff in the beta provider by modifying iam_test_file.go.erb

[SarahFrench]

FWIW I am able to get a diff in the beta provider by modifying iam_test_file.go.erb

Ah I think I know what may have happened - when IAM conditions were promoted from beta to ga I don't think iam_test_file.go.erb was updated. Tests like TestAccComputeImageIamBindingGenerated_withCondition aren't even made in the GA provider.

I'll investigate fixing that and it should help this PR a lot

[SarahFrench]

I've encountered a possible bug with the plugin SDK, and the last commit was a version of the tests that triggers the bug

Edit: But the tests have passed ok below, interesting. Something must be different between my local and the CI tests (😱)

[modular-magician]

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 38 files changed, 6696 insertions(+), 137 deletions(-)) Terraform Beta: Diff ( 41 files changed, 1770 insertions(+), 736 deletions(-)) TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))