Partly rollback changes from #11321 to fix an alias_ip_range bug

[karolgorc]

rollback from #11321
closes hashicorp/terraform-provider-google#19765 (more context in the comments of this issue)

This issue will get reintroduced hashicorp/terraform-provider-google#12286

This cannot be easily fixed without introducing new edge cases or crazy overhead for this field hence the rollback. The problems are:

• alias_ip's cannot be simultaneously removed and added in the same request. THIS IS AN API LIMITATION
• alias_ip's are a list of blocks so it cannot be validated and checked against other values or have a customized diff because you cannot access data from another block in customdiff, diffsuppress or validate functions
• Because alias_ip's are a list of blocks their order in the code matters. So if a user changes the order of them without changing the IP's this is a diff that we have to account for
• This field is getting auto sorted on the API's side (causing diff in config) and also has computed values when given a syntax like "/32". This makes it very complicated to cover all of the different cases.

Overall, i'd say that we should accept the fact that the array of alias_ip ranges get's cleared in between requests and treat it as an API limitation until it gets fixed. I don't think we should build additional UX features on top of an API method that doesn't work according to Google's standards (it should support deletion and creation at the same time)

partly rollback because i'm leaving the tests introduced in the previous PR. They could be useful if this gets fixed in the future

Release Note Template for Downstream PRs (will be copied)

See Write release notes for guidance.

compute: `alias_ip_range` on `google_compute_instance` will no longer cause ordering issues when updating with "/xx" syntax

[github-actions]

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

@shuyama1, 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 ( 3 files changed, 11 insertions(+), 97 deletions(-))
google-beta provider: Diff ( 3 files changed, 11 insertions(+), 97 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 5 insertions(+), 16 deletions(-))

[modular-magician]

Tests analytics

Total tests: 1048
Passed tests: 972
Skipped tests: 73
Affected tests: 3

Click here to see the affected service packages

• compute

Action taken

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

• TestAccComputeInstance_aliasIpRangeCommonAddresses
• TestAccComputeInstance_subnetworkUpdate
• TestAccComputeRegionPerInstanceConfig_removeInstanceOnDestroy

Get to know how VCR tests work

[modular-magician]

🟢 Tests passed during RECORDING mode:
TestAccComputeInstance_aliasIpRangeCommonAddresses [Debug log]
TestAccComputeRegionPerInstanceConfig_removeInstanceOnDestroy [Debug log]

🟢 No issues found for passed tests after REPLAYING rerun.

---

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

[karolgorc]

Something changed with allow_stopping_for_update since this was merged i'm guessing.

resource_compute_instance_test.go:2730: Step 2/6 error running import: ImportStateVerify attributes not equivalent. Difference is shown below. The - symbol indicates attributes missing after import.
        
          map[string]string{
        -       "allow_stopping_for_update": "true",
          }

Putting it in as VerifyIgnore.

[karolgorc]

Is this failed check a part of some migration? Because version guards are not changed here

[github-actions]

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

[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 ( 3 files changed, 11 insertions(+), 97 deletions(-))
google-beta provider: Diff ( 3 files changed, 11 insertions(+), 97 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 5 insertions(+), 16 deletions(-))

[modular-magician]

Tests analytics

Total tests: 1058
Passed tests: 959
Skipped tests: 73
Affected tests: 26

Click here to see the affected service packages

• compute

Action taken

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

• TestAccComputeForwardingRule_forwardingRuleHttpLbExample
• TestAccComputeForwardingRule_forwardingRuleRegionalHttpXlbExample
• TestAccComputeForwardingRule_internalHttpLbWithMigBackendExample
• TestAccComputeForwardingRule_internalTcpUdpLbWithMigBackendExample
• TestAccComputeForwardingRule_internalTcpUdpLbWithMigBackendExampleUpdate
• TestAccComputeRegionAutoscaler_regionAutoscalerBasicExample
• TestAccComputeRegionAutoscaler_scaleDownControl
• TestAccComputeRegionAutoscaler_scaleInControl
• TestAccComputeRegionAutoscaler_scalingSchedule
• TestAccComputeRegionAutoscaler_update
• TestAccComputeRegionBackendService_regionBackendServiceBalancingModeExample
• TestAccComputeRegionBackendService_withBackendInternalManaged
• TestAccComputeRegionPerInstanceConfig_removeInstanceOnDestroy
• TestAccComputeRegionPerInstanceConfig_statefulIps
• TestAccComputeRegionPerInstanceConfig_update
• TestAccComputeRegionResizeRequest_computeRmigResizeRequestExample
• TestAccRegionInstanceGroupManager_APISideListRecordering
• TestAccRegionInstanceGroupManager_autoHealingPolicies
• TestAccRegionInstanceGroupManager_basic
• TestAccRegionInstanceGroupManager_distributionPolicy
• TestAccRegionInstanceGroupManager_resourceManagerTags
• TestAccRegionInstanceGroupManager_stateful
• TestAccRegionInstanceGroupManager_stoppedSuspendedTargetSize
• TestAccRegionInstanceGroupManager_targetSizeZero
• TestAccRegionInstanceGroupManager_update
• TestAccRegionInstanceGroupManager_versions

Get to know how VCR tests work

[modular-magician]

🟢 Tests passed during RECORDING mode:
TestAccComputeForwardingRule_forwardingRuleHttpLbExample [Debug log]
TestAccComputeForwardingRule_forwardingRuleRegionalHttpXlbExample [Debug log]
TestAccComputeForwardingRule_internalHttpLbWithMigBackendExample [Debug log]
TestAccComputeForwardingRule_internalTcpUdpLbWithMigBackendExample [Debug log]
TestAccComputeForwardingRule_internalTcpUdpLbWithMigBackendExampleUpdate [Debug log]
TestAccComputeRegionAutoscaler_regionAutoscalerBasicExample [Debug log]
TestAccComputeRegionAutoscaler_scaleDownControl [Debug log]
TestAccComputeRegionAutoscaler_scaleInControl [Debug log]
TestAccComputeRegionAutoscaler_scalingSchedule [Debug log]
TestAccComputeRegionAutoscaler_update [Debug log]
TestAccComputeRegionBackendService_regionBackendServiceBalancingModeExample [Debug log]
TestAccComputeRegionBackendService_withBackendInternalManaged [Debug log]
TestAccComputeRegionPerInstanceConfig_removeInstanceOnDestroy [Debug log]
TestAccComputeRegionPerInstanceConfig_statefulIps [Debug log]
TestAccComputeRegionPerInstanceConfig_update [Debug log]
TestAccComputeRegionResizeRequest_computeRmigResizeRequestExample [Debug log]
TestAccRegionInstanceGroupManager_APISideListRecordering [Debug log]
TestAccRegionInstanceGroupManager_autoHealingPolicies [Debug log]
TestAccRegionInstanceGroupManager_basic [Debug log]
TestAccRegionInstanceGroupManager_distributionPolicy [Debug log]
TestAccRegionInstanceGroupManager_resourceManagerTags [Debug log]
TestAccRegionInstanceGroupManager_stateful [Debug log]
TestAccRegionInstanceGroupManager_stoppedSuspendedTargetSize [Debug log]
TestAccRegionInstanceGroupManager_targetSizeZero [Debug log]
TestAccRegionInstanceGroupManager_update [Debug log]
TestAccRegionInstanceGroupManager_versions [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]

@GoogleCloudPlatform/terraform-team @shuyama1 This PR has been waiting for review for 1 week. Please take a look! Use the label disable-review-reminders to disable these notifications.

[shuyama1]

Sorry for the delay on review. Taking a look now!

[shuyama1]

/gcbrun

[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 ( 3 files changed, 11 insertions(+), 97 deletions(-))
google-beta provider: Diff ( 3 files changed, 11 insertions(+), 97 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 5 insertions(+), 16 deletions(-))

[modular-magician]

Tests analytics

Total tests: 1059
Passed tests: 986
Skipped tests: 73
Affected tests: 0

Click here to see the affected service packages

• compute

🟢 All tests passed!

View the build log

[shuyama1]

The rollback seems reasonable to me, as the original fix is potentially a breaking change(?) since it changes the order of alias_ip_range blocks in state. The change has led to another issue that breaks existing configs, as discussed in hashicorp/terraform-provider-google#19765.

I'm inviting @melinath, the reviewer of the original PR #11321, to weigh in on this rollback, to ensure I haven’t overlooked any potential issues that could impact users.

[shuyama1]

Is this failed check a part of some migration? Because version guards are not changed here

I believe this should be resolved by #12208. If you rebase your PR, the check should pass on the next run.

[melinath]

yeah, sorting by config order assumes that all the blocks are explicitly defined in the config; since the API can add computed values into the mix that we can't tie back to a specific block in the config, sorting by config order is insufficient. Rolling this back seems reasonable.

[melinath]

What are the problems besides the /32 generating a random value? That seems like it would be relatively easy to compensate for. (But I may be missing something.)

[karolgorc]

The "/32" isn't that big of an issue. I already wrote and tested an algorithm that solved this issue but the big problems start when a user adds a new alias_ip block in the middle of 2 other ones, or at the beginning. same goes for deletion and update. Then the sorting just doesn't work and gets lost in the midst of wrong ordering in local state, unexplainable sorting from API, and being given unknown values.

Overall a very edge-case heavy implementation and it seems like too much of a hustle to implement this kind of logic for a field that's out of the ordinary when it comes to functionality for Google

Edit: just to clarify. The order of the blocks in code reflects the order of the IP's saved in state. So when added in the middle (not at the end) this also adds the IP in the middle of an array making it harder to compare against API output. This also enables weird behaviour when the first IP is deleted. All the other shift one place up. This also isn't that big of a problem by it self but add the unknown values to it and you've got a headache of an implementation 😄

Edit: Rebased branch, this "api limitation" was forwarded to network API team as well

[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 ( 3 files changed, 11 insertions(+), 97 deletions(-))
google-beta provider: Diff ( 3 files changed, 11 insertions(+), 97 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 5 insertions(+), 16 deletions(-))

[modular-magician]

Tests analytics

Total tests: 1060
Passed tests: 987
Skipped tests: 73
Affected tests: 0

Click here to see the affected service packages

• compute

🟢 All tests passed!

View the build log

[melinath]

It sounds like there may be some issues with how the "sort by config order" helper functions work that would be nice to resolve (alongside the /32 issue). I'm not sure I have a good feel for whether the current behavior or the previous permadiff is worse; I don't really want us to back out this behavior just to introduce the old bug.

FWIW it should be possible to do a diff suppress with CustomizeDiff, since that operates on the entire resource at once. (Though it looks like it might have some trouble with known-after-apply values...)

@shuyama1 what's your take?

[github-actions]

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