fix: (storage) bucket policy delete 412 error

[gurusai-voleti]

Fixes: hashicorp/terraform-provider-google#20838
Release Note Template for Downstream PRs (will be copied)

See Write release notes for guidance.

storage: bucket with notification if policy being deleted causing 412 error, fix to avoid 412 error

[modular-magician]

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 5 files changed, 9 insertions(+), 868 deletions(-))
google-beta provider: Diff ( 5 files changed, 9 insertions(+), 868 deletions(-))

Breaking Change(s) Detected

The following breaking change(s) were detected within your pull request.

• Resource google_storage_bucket_iam_binding was either removed or renamed - reference
• Resource google_storage_bucket_iam_member was either removed or renamed - reference
• Resource google_storage_bucket_iam_policy was either removed or renamed - reference

If you believe this detection to be incorrect please raise the concern with your reviewer.
If you intend to make this change you will need to wait for a major release window.
An override-breaking-change label can be added to allow merging.

[modular-magician]

Tests analytics

Total tests: 109
Passed tests: 101
Skipped tests: 7
Affected tests: 1

Click here to see the affected service packages

• storage

Action taken

Found 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

• TestAccStorageBucketIamPolicy

Get to know how VCR tests work

[modular-magician]

🔴 Tests failed during RECORDING mode:
TestAccStorageBucketIamPolicy [Error message] [Debug log]

🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR.

View the build log or the debug log for each test

[modular-magician]

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 5 files changed, 13 insertions(+), 868 deletions(-))
google-beta provider: Diff ( 5 files changed, 13 insertions(+), 868 deletions(-))

[modular-magician]

Tests analytics

Total tests: 109
Passed tests: 101
Skipped tests: 7
Affected tests: 1

Click here to see the affected service packages

• storage

Action taken

Found 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

• TestAccStorageBucketIamPolicy

Get to know how VCR tests work

[modular-magician]

🟢 Tests passed during RECORDING mode:
TestAccStorageBucketIamPolicy [Debug log]

🟢 No issues found for passed tests after REPLAYING rerun.

---

🟢 All tests passed!

View the build log or the debug log for each test

[modular-magician]

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 5 files changed, 13 insertions(+), 883 deletions(-))
google-beta provider: Diff ( 5 files changed, 13 insertions(+), 883 deletions(-))

[modular-magician]

Tests analytics

Total tests: 109
Passed tests: 102
Skipped tests: 7
Affected tests: 0

Click here to see the affected service packages

• storage

🟢 All tests passed!

View the build log

[github-actions]

Hello! I am a robot. Tests will require approval from a repository maintainer to run.

@rileykarson, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look.

You can help make sure that review is quick by doing a self-review and by running impacted tests locally.

[modular-magician]

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 6 files changed, 99 insertions(+), 883 deletions(-))
google-beta provider: Diff ( 6 files changed, 99 insertions(+), 883 deletions(-))

[modular-magician]

Tests analytics

Total tests: 110
Passed tests: 103
Skipped tests: 7
Affected tests: 0

Click here to see the affected service packages

• storage

🟢 All tests passed!

View the build log

[modular-magician]

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 6 files changed, 99 insertions(+), 883 deletions(-))
google-beta provider: Diff ( 6 files changed, 99 insertions(+), 883 deletions(-))

[modular-magician]

Tests analytics

Total tests: 110
Passed tests: 103
Skipped tests: 7
Affected tests: 0

Click here to see the affected service packages

• storage

🟢 All tests passed!

View the build log

[rileykarson]

It seems like we added the etag on delete for GCS bucket in the first place. I can't find context on why yet, but looking into it: #2794

edit: original context in hashicorp/terraform-provider-google#1190 (comment)

When just an empty bindings block is sent we get back:

{
  "error": {
    "code": 400,
    "message": "A policy to update must be provided.",
    "errors": [
      {
        "message": "A policy to update must be provided.",
        "domain": "global",
        "reason": "required"
      }
    ]
  }
}

[gurusai-voleti]

It seems like we added the etag on delete for GCS bucket in the first place. I can't find context on why yet, but looking into it: #2794

edit: original context in hashicorp/terraform-provider-google#1190 (comment)

When just an empty bindings block is sent we get back:

{
  "error": {
    "code": 400,
    "message": "A policy to update must be provided.",
    "errors": [
      {
        "message": "A policy to update must be provided.",
        "domain": "global",
        "reason": "required"
      }
    ]
  }
}

agree but the request body we are sending will look like below without etag
{
"version": 3
}

the request is accepted and policy got updated and response is 200

response:
{
"kind": "storage#policy",
"resourceId": "projects/_/buckets/bucket-name",
"version": 1,
"etag": "CAQ="
}

[kautikdk]

I believe there is a server side validation for rejecting an empty Buckets:setIamPolicy request. I tested compute setIamPolicy method which allows empty policy and clears if provided. Specifying policy version in the Buckets:setIamPolicy method request should be enough to clear IAM bindings.

[rileykarson]

Yep- in my testing any value in addition to the empty bindings array passed the validation and cleared the bindings. Sorry for the delay on my end- considering whether we can just remove the etag value here for all IAM policies or not, since that would obviate this change.

[gurusai-voleti]

Yep- in my testing any value in addition to the empty bindings array passed the validation and cleared the bindings. Sorry for the delay on my end- considering whether we can just remove the etag value here for all IAM policies or not, since that would obviate this change.

I added below logic to remove etag only when bindings are empty or null in this change and converted auto generated to handwritten resource, below is only extra logic added to existing iam_bucket

if policy.Bindings == nil || len(policy.Bindings) == 0 {
policy.Etag = ""
}

[modular-magician]

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 6 files changed, 99 insertions(+), 883 deletions(-))
google-beta provider: Diff ( 6 files changed, 99 insertions(+), 883 deletions(-))

[modular-magician]

Tests analytics

Total tests: 110
Passed tests: 103
Skipped tests: 7
Affected tests: 0

Click here to see the affected service packages

• storage

🟢 All tests passed!

View the build log

[kautikdk]

Do update here when PR is ready for review. I don't see handwritten documentation file and tests for other cases.

[modular-magician]

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 6 files changed, 99 insertions(+), 883 deletions(-))
google-beta provider: Diff ( 6 files changed, 99 insertions(+), 883 deletions(-))

[modular-magician]

Tests analytics

Total tests: 110
Passed tests: 103
Skipped tests: 7
Affected tests: 0

Click here to see the affected service packages

• storage

🟢 All tests passed!

View the build log

[modular-magician]

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 5 files changed, 117 insertions(+), 123 deletions(-))
google-beta provider: Diff ( 5 files changed, 117 insertions(+), 123 deletions(-))

[modular-magician]

Tests analytics

Total tests: 118
Passed tests: 103
Skipped tests: 9
Affected tests: 6

Click here to see the affected service packages

• storage

#### Non-exercised tests

🔴 Tests were added that are skipped in VCR:

• TestAccStorageBucket_iamBindingWithAndWithoutCondition
• TestAccStorageBucket_iamMemberWithAndWithoutCondition

Action taken

Found 6 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

• TestAccStorageBucket_basicIamBinding
• TestAccStorageBucket_basicIamMemeber
• TestAccStorageBucket_basicIamPolicy
• TestAccStorageBucket_iamBindingWithCondition
• TestAccStorageBucket_iamMemberWithCondition
• TestAccStorageBucket_iamPolicyGeneratedWithCondition

Get to know how VCR tests work

[modular-magician]

🟢 Tests passed during RECORDING mode:
TestAccStorageBucket_basicIamBinding [Debug log]
TestAccStorageBucket_basicIamMemeber [Debug log]
TestAccStorageBucket_basicIamPolicy [Debug log]
TestAccStorageBucket_iamBindingWithCondition [Debug log]
TestAccStorageBucket_iamMemberWithCondition [Debug log]
TestAccStorageBucket_iamPolicyGeneratedWithCondition [Debug log]

🟢 No issues found for passed tests after REPLAYING rerun.

---

🟢 All tests passed!

View the build log or the debug log for each test

[github-actions]

@rileykarson This PR has been waiting for review for 3 weekdays. Please take a look! Use the label disable-review-reminders to disable these notifications.