magic-modules fix: if unable to calculate local md5 hash use old value of detect

if unable to calculate local md5 hash use old value of detect_md5hash Fixes: https://github.com/hashicorp/terraform-provider-google/issues/18618

Release Note Template for Downstream PRs (will be copied)

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

Oct 17 '24 08:10 gurusai-voleti

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.

Oct 17 '24 08:10 github-actions[bot]

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(+))

Oct 17 '24 09:10 modular-magician

Tests analytics

Total tests: 109 Passed tests: 100 Skipped tests: 9 Affected tests: 0

Click here to see the affected service packages

🟢 All tests passed!

View the build log

Oct 17 '24 09:10 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(+))

Oct 17 '24 12:10 modular-magician

Tests analytics

Total tests: 110 Passed tests: 100 Skipped tests: 9 Affected tests: 1

Click here to see the affected service packages

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

Get to know how VCR tests work

Oct 17 '24 12:10 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

Oct 17 '24 12:10 modular-magician

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

Oct 17 '24 13:10 kautikdk

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

Oct 18 '24 04:10 gurusai-voleti

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?

Oct 18 '24 05:10 kautikdk

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

Oct 18 '24 09:10 gurusai-voleti

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.

Oct 18 '24 11:10 kautikdk

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(+))

Oct 21 '24 06:10 modular-magician

Tests analytics

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

Click here to see the affected service packages

🟢 All tests passed!

View the build log

Oct 21 '24 07:10 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(+))

Oct 22 '24 05:10 modular-magician

Tests analytics

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

Click here to see the affected service packages

🟢 All tests passed!

View the build log

Oct 22 '24 05:10 modular-magician

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

Oct 22 '24 09:10 github-actions[bot]

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 https://github.com/GoogleCloudPlatform/magic-modules/pull/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.

Oct 22 '24 21:10 roaks3

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(+))

Oct 23 '24 11:10 modular-magician

Tests analytics

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

Click here to see the affected service packages

🟢 All tests passed!

View the build log

Oct 23 '24 11:10 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, 59 insertions(+)) google-beta provider: Diff ( 2 files changed, 59 insertions(+))

Oct 24 '24 05:10 modular-magician

Tests analytics

Total tests: 110 Passed tests: 73 Skipped tests: 9 Affected tests: 28

Click here to see the affected service packages

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

Get to know how VCR tests work

Oct 24 '24 05:10 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] 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_detect_md5hash_nofile [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

Oct 24 '24 05:10 modular-magician

Hi @roaks3, Regarding your suggestion of using CustomizeDiff, I was going through documentation of inconsistent plan remediation and the to handle this kind of situation there is one custom diff method(ComputedIf) which sets individual property value to nil and mark it as Computed for that particular terraform apply operation, Reference. These seems to be aligning with our requirement but there is one catch. It only does this remediation only for Computed attribute not for others, reference. Here detect_md5hash is not computed but is has default behavior which never allows this operation to happen. Ideally we want to use ResourceDiff.setDiff method to set attribute to nil and computed during operation but this can not be accessed outside so we might need to find another solution?

PS: It appears that this is not the hard error which makes state file inconsistent but it might be the post apply validation only. I am not very sure of this but I am referring this statement from the documentation Terraform 0.12 does not enforce this as a hard error for providers using the current version of the SDK. If It is the case, we can consider refactoring this property and behavior which aligns with our all use-cases and introduce new field or change behavior of current field in upcoming major release?

Oct 24 '24 05:10 kautikdk

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

Oct 24 '24 05:10 modular-magician

Tests analytics

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

Click here to see the affected service packages

🟢 All tests passed!

View the build log

Oct 24 '24 06:10 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, 78 insertions(+)) google-beta provider: Diff ( 2 files changed, 78 insertions(+))

Oct 24 '24 13:10 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, 78 insertions(+)) google-beta provider: Diff ( 2 files changed, 78 insertions(+))

Oct 24 '24 13:10 modular-magician

Tests analytics

Total tests: 110 Passed tests: 100 Skipped tests: 9 Affected tests: 1

Click here to see the affected service packages

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

Get to know how VCR tests work

Oct 24 '24 13:10 modular-magician

🔴 Tests failed during RECORDING mode: TestAccStorageObject_detect_md5hash_nofile [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

Oct 24 '24 13:10 modular-magician

Tests analytics

Total tests: 110 Passed tests: 100 Skipped tests: 9 Affected tests: 1

Click here to see the affected service packages

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

Get to know how VCR tests work

Oct 24 '24 13:10 modular-magician

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

Diff report

Tests analytics

Diff report

Tests analytics

Action taken

Diff report

Tests analytics

Diff report

Tests analytics

Diff report

Tests analytics

Diff report

Tests analytics

Action taken

Diff report

Tests analytics

Diff report

Diff report

Tests analytics

Action taken

Tests analytics

Action taken