fix: if unable to calculate local md5 hash use old value of detect_md5hash

[gurusai-voleti]

if unable to calculate local md5 hash use old value of detect_md5hash
Fixes: hashicorp/terraform-provider-google#18618

Release Note Template for Downstream PRs (will be copied)

storage: if unable to calculate local md5 hash use old value of detect_md5hash

[github-actions]

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

@roaks3, 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 ( 1 file changed, 3 insertions(+))
google-beta provider: Diff ( 1 file changed, 3 insertions(+))

[modular-magician]

Tests analytics

Total tests: 109
Passed tests: 100
Skipped tests: 9
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 ( 2 files changed, 51 insertions(+))
google-beta provider: Diff ( 2 files changed, 51 insertions(+))

[modular-magician]

Tests analytics

Total tests: 110
Passed tests: 100
Skipped tests: 9
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

• TestAccStorageObject_detect_md5hash_nofile

Get to know how VCR tests work

[modular-magician]

🟢 Tests passed during RECORDING mode:
TestAccStorageObject_detect_md5hash_nofile[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

[kautikdk]

Hi @gurusai-voleti, This change appears to be the risky fix as we are setting localMd5Hash = old in DiffSuppressFunc when the file is not present. This will prevent terraform diff generation for google_storage_bucket_object resource when the source file is removed. Eventually it will make unnecessary diff in the next terraform plan.
Setting Hash to previous one also prevents diff for file content change in-between two terraform apply if the files are removed locally and file content is changed in terraform config.
Let me know if I am missing something.

PS: Added test appears to be testing different behavior. I suggest, writing test steps and test configs by referring bug reproductions steps.
cc: @roaks3

[gurusai-voleti]

Hi @gurusai-voleti, This change appears to be the risky fix as we are setting localMd5Hash = old in DiffSuppressFunc when the file is not present. This will prevent terraform diff generation for google_storage_bucket_object resource when the source file is removed. Eventually it will make unnecessary diff in the next terraform plan. Setting Hash to previous one also prevents diff for file content change in-between two terraform apply if the files are removed locally and file content is changed in terraform config. Let me know if I am missing something.

PS: Added test appears to be testing different behavior. I suggest, writing test steps and test configs by referring bug reproductions steps. cc: @roaks3

with this fix in terraform plan wont show any difference and in terraform apply it will create files and wont do anything else unless there are really infra changes to deploy, this is working as expected without this change also with out file delete.
added test case used the same example which is reported by customer

[kautikdk]

Hi @gurusai-voleti, This change appears to be the risky fix as we are setting localMd5Hash = old in DiffSuppressFunc when the file is not present. This will prevent terraform diff generation for google_storage_bucket_object resource when the source file is removed. Eventually it will make unnecessary diff in the next terraform plan. Setting Hash to previous one also prevents diff for file content change in-between two terraform apply if the files are removed locally and file content is changed in terraform config. Let me know if I am missing something.
PS: Added test appears to be testing different behavior. I suggest, writing test steps and test configs by referring bug reproductions steps. cc: @roaks3

with this fix in terraform plan wont show any difference and in terraform apply it will create files and wont do anything else unless there are really infra changes to deploy, this is working as expected without this change also with out file delete. added test case used the same example which is reported by customer

But if there is a resource dependency then Ideally customer expects a difference. If the source file is being removed, Ideally there should be some difference, Right?

[gurusai-voleti]

Hi @gurusai-voleti, This change appears to be the risky fix as we are setting localMd5Hash = old in DiffSuppressFunc when the file is not present. This will prevent terraform diff generation for google_storage_bucket_object resource when the source file is removed. Eventually it will make unnecessary diff in the next terraform plan. Setting Hash to previous one also prevents diff for file content change in-between two terraform apply if the files are removed locally and file content is changed in terraform config. Let me know if I am missing something.
PS: Added test appears to be testing different behavior. I suggest, writing test steps and test configs by referring bug reproductions steps. cc: @roaks3

with this fix in terraform plan wont show any difference and in terraform apply it will create files and wont do anything else unless there are really infra changes to deploy, this is working as expected without this change also with out file delete. added test case used the same example which is reported by customer

But if there is a resource dependency then Ideally customer expects a difference. If the source file is being removed, Ideally there should be some difference, Right?

yes if the file is created through a resource block it will show up in the plan, only thing it will not display in the plan is md5 hash difference with this fix, which is the real problem of the issue between plan and apply, I can modify my test case more

[kautikdk]

Hi @roaks3, Is there any way to handle intermediate value change of attribute during terraform apply operation? I believe, The error is coming because of two different values of same attribute during one apply operation.

@gurusai-voleti can you modify PR description as per this guidelines:https://googlecloudplatform.github.io/magic-modules/contribute/create-pr/#requirements. Currently there is no link between this PR and the issue that it is going to fix.

[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 ( 2 files changed, 51 insertions(+))
google-beta provider: Diff ( 2 files changed, 51 insertions(+))

[modular-magician]

Tests analytics

Total tests: 110
Passed tests: 101
Skipped tests: 9
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 ( 2 files changed, 51 insertions(+))
google-beta provider: Diff ( 2 files changed, 51 insertions(+))

[modular-magician]

Tests analytics

Total tests: 110
Passed tests: 101
Skipped tests: 9
Affected tests: 0

Click here to see the affected service packages

• storage

🟢 All tests passed!

View the build log

[github-actions]

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

[roaks3]

Yea, this is relatively complicated. The detect_md5hash solution seems to fundamentally be at odds with Terraform paradigms (see https://developer.hashicorp.com/terraform/plugin/sdkv2/guides/terraform-0.12-compatibility#inaccurate-plans), in that it is explicitly communicating a planned value of different hash to the user, but then expects to change it to the actual hash later. I suspect this worked fine with ForceNew for the ~6 years it has been like this (although I'm not certain exactly why), but when it was changed to be mutable in #10038, I think we broke some assumptions and are now seeing this error.

Per the link, you might have luck working with CustomizeDiff to handle the detect_md5hash value properly, but even that could result in an unintended breaking change.

I would recommend narrowing in on the exact failure case, as it is odd that the source file can be removed and then apply works the first time, but then not on subsequent times. Hopefully there is a specific edge case that can be accounted for.

As mentioned by @kautikdk , I'm a bit concerned about the proposed solution in the PR being too broad, and impacting behavior that is working as intended.

[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 ( 2 files changed, 51 insertions(+))
google-beta provider: Diff ( 2 files changed, 51 insertions(+))

[modular-magician]

Tests analytics

Total tests: 110
Passed tests: 101
Skipped tests: 9
Affected tests: 0

Click here to see the affected service packages

• storage

🟢 All tests passed!

View the build log

[roaks3]

To follow up: I was able to confirm with the team the CustomizeDiff was either unavailable, or rarely used, when this functionality was originally introduced, so that is likely a path we can take to fix this issue and possibly simplify the behavior.

[modular-magician]

Tests analytics

Total tests: 115
Passed tests: 106
Skipped tests: 9
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 ( 2 files changed, 47 insertions(+), 37 deletions(-))
google-beta provider: Diff ( 2 files changed, 47 insertions(+), 37 deletions(-))

[modular-magician]

Tests analytics

Total tests: 115
Passed tests: 106
Skipped tests: 9
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 ( 2 files changed, 48 insertions(+), 35 deletions(-))
google-beta provider: Diff ( 2 files changed, 48 insertions(+), 35 deletions(-))

[modular-magician]

Tests analytics

Total tests: 115
Passed tests: 106
Skipped tests: 9
Affected tests: 0

Click here to see the affected service packages

• storage

🟢 All tests passed!

View the build log

[gurusai-voleti]

I think overall this is still changing too much about the existing behavior. The goal is for detect_md5hash to do the same thing it does now, but additionally offer the ability to use a source_md5hash field, to configure things more cleanly and get around some of the issues that have been found. We don't want to break users of detect_md5hash, otherwise this would need to wait for a major release.

Is there any particular difficulty in setting it up that way, or perhaps was the thinking to wait until the next major?

@roaks3 yes without this breaking change the issue seems cannot be fixed, the diffsuppressfunc for detect_md5hash and has change("detect_md5hash") is the main issue and user is facing the issue with that related logic

to support the new field along with detect_md5hash, it can be done but the user reported issue will not be fixed, we can mark detect_md5hash as deprecated and ask customers to use new field source_md5hash?

[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 ( 2 files changed, 49 insertions(+), 57 deletions(-))
google-beta provider: Diff ( 2 files changed, 49 insertions(+), 57 deletions(-))

[modular-magician]

Tests analytics

Total tests: 115
Passed tests: 78
Skipped tests: 9
Affected tests: 28

Click here to see the affected service packages

• storage

Action taken

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

• TestAccDataSourceGoogleStorageBucketObject_basic
• TestAccDataSourceGoogleStorageBucketObjects_basic
• TestAccDataSourceStorageBucketObjectContent_Basic
• TestAccStorageFolder_FolderForceDestroy
• TestAccStorageManagedFolder_storageManagedFolderUpdate
• TestAccStorageObjectAccessControl_storageObjectAccessControlPublicObjectExample
• TestAccStorageObjectAccessControl_update
• TestAccStorageObjectAccessControl_updateWithSlashes
• TestAccStorageObjectAcl_basic
• TestAccStorageObjectAcl_downgrade
• TestAccStorageObjectAcl_explicitToPredefined
• TestAccStorageObjectAcl_predefined
• TestAccStorageObjectAcl_predefinedToExplicit
• TestAccStorageObjectAcl_unordered
• TestAccStorageObjectAcl_upgrade
• TestAccStorageObjectKms
• TestAccStorageObject_basic
• TestAccStorageObject_cacheControl
• TestAccStorageObject_content
• TestAccStorageObject_customerEncryption
• TestAccStorageObject_dynamicContent
• TestAccStorageObject_folder
• TestAccStorageObject_holds
• TestAccStorageObject_metadata
• TestAccStorageObject_recreate
• TestAccStorageObject_retention
• TestAccStorageObject_storageClass
• TestAccStorageObject_withContentCharacteristics

Get to know how VCR tests work

[modular-magician]

🔴 Tests failed during RECORDING mode:
TestAccDataSourceGoogleStorageBucketObject_basic [Error message] [Debug log]
TestAccDataSourceGoogleStorageBucketObjects_basic [Error message] [Debug log]
TestAccDataSourceStorageBucketObjectContent_Basic [Error message] [Debug log]
TestAccStorageFolder_FolderForceDestroy [Error message] [Debug log]
TestAccStorageManagedFolder_storageManagedFolderUpdate [Error message] [Debug log]
TestAccStorageObjectAccessControl_storageObjectAccessControlPublicObjectExample [Error message] [Debug log]
TestAccStorageObjectAccessControl_update [Error message] [Debug log]
TestAccStorageObjectAccessControl_updateWithSlashes [Error message] [Debug log]
TestAccStorageObjectAcl_basic [Error message] [Debug log]
TestAccStorageObjectAcl_downgrade [Error message] [Debug log]
TestAccStorageObjectAcl_explicitToPredefined [Error message] [Debug log]
TestAccStorageObjectAcl_predefined [Error message] [Debug log]
TestAccStorageObjectAcl_predefinedToExplicit [Error message] [Debug log]
TestAccStorageObjectAcl_unordered [Error message] [Debug log]
TestAccStorageObjectAcl_upgrade [Error message] [Debug log]
TestAccStorageObjectKms [Error message] [Debug log]
TestAccStorageObject_basic [Error message] [Debug log]
TestAccStorageObject_cacheControl [Error message] [Debug log]
TestAccStorageObject_content [Error message] [Debug log]
TestAccStorageObject_customerEncryption [Error message] [Debug log]
TestAccStorageObject_dynamicContent [Error message] [Debug log]
TestAccStorageObject_folder [Error message] [Debug log]
TestAccStorageObject_holds [Error message] [Debug log]
TestAccStorageObject_metadata [Error message] [Debug log]
TestAccStorageObject_recreate [Error message] [Debug log]
TestAccStorageObject_retention [Error message] [Debug log]
TestAccStorageObject_storageClass [Error message] [Debug log]
TestAccStorageObject_withContentCharacteristics [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 ( 2 files changed, 48 insertions(+), 59 deletions(-))
google-beta provider: Diff ( 2 files changed, 48 insertions(+), 59 deletions(-))

[modular-magician]

Tests analytics

Total tests: 115
Passed tests: 106
Skipped tests: 9
Affected tests: 0

Click here to see the affected service packages

• storage

🟢 All tests passed!

View the build log

[github-actions]

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

[roaks3]

the user reported issue will not be fixed, we can mark detect_md5hash as deprecated and ask customers to use new field source_md5hash?

Yea, that's the direction I thought we were going to go in. Perhaps @kautikdk could weigh in? If we cannot do that, or it isn't a satisfactory solution, then we would need to wait until ~Sept to merge this.

[kautikdk]

Hi, Got off track on this issue.
What is the solution that we are proposing?

[gurusai-voleti]

Hi, Got off track on this issue. What is the solution that we are proposing?

@kautikdk the issue is with detect_md5hash field,
one solution is to make detect_md5hash field mark as deprecated without any change to existing code and add a new field optional source_md5hash so the user will provide the value if needed, this will not fix the user reported issue but providing an alternative solution
(like here https://github.com/hashicorp/terraform-provider-aws/blob/302d58d1b5b52c5496e373338753d89481b9ad42/internal/service/s3/bucket_object.go#L174

https://github.com/hashicorp/terraform-provider-aws/blob/302d58d1b5b52c5496e373338753d89481b9ad42/internal/service/lambda/function.go#L374)

else do a breaking change by removing suppressfunc of field detect_md5hash and avoid detect_md5hash in update and read methods and add new field source_md5hash (as per current PR changes)

@roaks3 anything you want to add?

[roaks3]

Yea that's correct. Based on the numerous attempts above, I don't believe we will have success trying to fix the existing detect_md5hash behavior.

[github-actions]

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

[roaks3]

Clearing review

[kautikdk]

So with the source_md5hash field, we expect users to provide md5 Hash? and we won't calculate within provider itself?

[wiktorn]

So with the source_md5hash field, we expect users to provide md5 Hash? and we won't calculate within provider itself?

If this is true, will the fix work if the source_md5hash will be known at apply stage?

[gurusai-voleti]

So with the source_md5hash field, we expect users to provide md5 Hash? and we won't calculate within provider itself?

initially we wont expect the user to provide source_md5hash, if any change/update of object is required let the user provide the source_md5hash

another computed field md5hash will always be there and no change for it

[gurusai-voleti]

So with the source_md5hash field, we expect users to provide md5 Hash? and we won't calculate within provider itself?

If this is true, will the fix work if the source_md5hash will be known at apply stage?

yes if the user provides in source_md5hash in configuration of tf or any update is required for object

[kautikdk]

So with the source_md5hash field, we expect users to provide md5 Hash? and we won't calculate within provider itself?

initially we wont expect the user to provide source_md5hash, if any change/update of object is required let the user provide the source_md5hash

another computed field md5hash will always be there and no change for it

Just a intuition, won't this put burden on users? For example, If a user specifies source field with the path of an object and then user changes that object. Now just to detect this change, user has to calculate md5 hash of the new object and update new value in the terraform config. This necessitates one additional step of calculating hash for users to update an object using Terraform which is not required as of now. Currently detect_md5hash field does this step for users and with this change we are planning to deprecate it.

[gurusai-voleti]

So with the source_md5hash field, we expect users to provide md5 Hash? and we won't calculate within provider itself?

initially we wont expect the user to provide source_md5hash, if any change/update of object is required let the user provide the source_md5hash
another computed field md5hash will always be there and no change for it

Just a intuition, won't this put burden on users? For example, If a user specifies source field with the path of an object and then user changes that object. Now just to detect this change, user has to calculate md5 hash of the new object and update new value in the terraform config. This necessitates one additional step of calculating hash for users to update an object using Terraform which is not required as of now. Currently detect_md5hash field does this step for users and with this change we are planning to deprecate it.

source_md5hash is an optional field, if the object changes at the path which is mentioned in source as source field is force new it will object and upload to bucket

the new field source_md5hash is required by the user only when the user wants to have track of md5hash of the object locally otherwise not needed as source, content already handled for changes