Breaking change - Add integration for subnetworks with internal ranges API

[daanheikens]

Fixes: hashicorp/terraform-provider-google#17881

Second attempt, superseding: #10507

Release Note Template for Downstream PRs (will be copied)

compute: added fields `reserved_internal_range` and `secondary_ip_ranges[].reserved_internal_range` to `google_compute_subnetwork` resource

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

[daanheikens]

@trodge FYI! As discussed on: hashicorp/terraform-provider-google#18300

@c2thorn +1 for visibility

[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, 189 insertions(+), 32 deletions(-))
google-beta provider: Diff ( 3 files changed, 319 insertions(+), 35 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 21 insertions(+))

[modular-magician]

Tests analytics

Total tests: 958
Passed tests: 881
Skipped tests: 74
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

TestAccComputeInstanceNetworkIntefaceWithSecurityPolicy|TestAccComputeSubnetwork_subnetworkReservedInternalRangeExample|TestAccComputeSubnetwork_subnetworkReservedSecondaryRangeExample

Get to know how VCR tests work

[modular-magician]

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccComputeSubnetwork_subnetworkReservedSecondaryRangeExample[Debug log]

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$

---

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccComputeInstanceNetworkIntefaceWithSecurityPolicy[Error message] [Debug log]
TestAccComputeSubnetwork_subnetworkReservedInternalRangeExample[Error message] [Debug log]

$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$
View the build log or the debug log for each test

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

[github-actions]

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

[roaks3]

@trodge passing this one to you, since it looks like you did the initial review. I'm not adding disable-review-reminders since you may want to take a look first, but otherwise it seems like we could add it here to keep the notifications quiet.

[c2thorn]

I've updated the base branch to point to the major release branch. This should be good to continue.

I've forgotten what the exact breaking change is here - but please follow the steps here to edit the upgrade guide so User's know how to handle the breaking change.

@daanheikens @trodge

[daanheikens]

I've updated the base branch to point to the major release branch. This should be good to continue.

I've forgotten what the exact breaking change is here - but please follow the steps here to edit the upgrade guide so User's know how to handle the breaking change.

@daanheikens @trodge

Thank you @c2thorn, the issue was that one of the fields was treated required while it was optional. See also: #10697

If I recall correctly, it was related to this issue: hashicorp/terraform-provider-google#18115 where @rileykarson explains the issue in detail. I am not sure if it's already fixed in this major release, if not, we indeed have to update the documentation and add more tests I think?

Happy to hear your advice.

[c2thorn]

@daanheikens Thanks for the information. As I now understand, this is a breaking change due to the parent field having schema_config_mode_attr.

I planned on testing the effect of removing schema_config_mode_attr as part of 6.0, but have not gotten to it yet.

With schema_config_mode_attr, adding this new field is a breaking change because it becomes required.
Without schema_config_mode_attr, this should not be a breaking change, but it is unclear if removing schema_config_mode_attr itself is a breaking change. If you are interested in testing this out yourself as part of this PR that would be helpful, but otherwise I will have to report back in a few weeks after I've had time to test.

Assuming schema_config_mode_attr is removed outside of this PR, this will no longer be a breaking change and not need the associated breaking change documentation. But until someone is able to test removing schema_config_mode_attr, we should wait.

[trodge]

Removing schema_config_mode_attr would be a breaking change in that a config could produce this error:

  error=
  | exit status 1
  | 
  | Error: Unsupported argument
  | 
  |   on terraform_plugin_test.tf line 12, in resource "google_compute_subnetwork" "network-with-private-secondary-ip-ranges":
  |   12:   secondary_ip_range = []
  | 
  | An argument named "secondary_ip_range" is not expected here. Did you mean to
  | define a block of type "secondary_ip_range"?

If the user is attempting to use the current recommended method of specifying an empty block in order to remove secondary ip ranges from their config. (This being the workaround Riley mentions in hashicorp/terraform-provider-google#12824 )

If we're no longer concerned about maintaining 0.12 -> 0.13 forwards compatibility in the 6.0 version of the provider, could we remove schema_config_mode_attr and all tests/docs mentioning the [] syntax?

[daanheikens]

Removing schema_config_mode_attr would be a breaking change in that a config could produce this error:

  error=
  | exit status 1
  | 
  | Error: Unsupported argument
  | 
  |   on terraform_plugin_test.tf line 12, in resource "google_compute_subnetwork" "network-with-private-secondary-ip-ranges":
  |   12:   secondary_ip_range = []
  | 
  | An argument named "secondary_ip_range" is not expected here. Did you mean to
  | define a block of type "secondary_ip_range"?

If the user is attempting to use the current recommended method of specifying an empty block in order to remove secondary ip ranges from their config. (This being the workaround Riley mentions in hashicorp/terraform-provider-google#12824 )

If we're no longer concerned about maintaining 0.12 -> 0.13 forwards compatibility in the 6.0 version of the provider, could we remove schema_config_mode_attr and all tests/docs mentioning the [] syntax?

@trodge @c2thorn I can try some variations here and write some tests/examples with schema_config_mode_attr removed if you are OK with that? Could you please guide me a bit in where I can start? Let me know, happy to help.

[c2thorn]

@trodge @c2thorn I can try some variations here and write some tests/examples with schema_config_mode_attr removed if you are OK with that? Could you please guide me a bit in where I can start? Let me know, happy to help.

I plan to get to testing other fields this week, but if you have time to start with this field, that would be helpful. With removing schema_config_mode_attr, we'd essentially just need to manually verify if it is possible to remove the field secondary_ip_ranges or not. I don't think there is a specific test that needs to be added to MMv1, what do you think @trodge

[trodge]

@trodge @c2thorn I can try some variations here and write some tests/examples with schema_config_mode_attr removed if you are OK with that? Could you please guide me a bit in where I can start? Let me know, happy to help.

I plan to get to testing other fields this week, but if you have time to start with this field, that would be helpful. With removing schema_config_mode_attr, we'd essentially just need to manually verify if it is possible to remove the field secondary_ip_ranges or not. I don't think there is a specific test that needs to be added to MMv1, what do you think @trodge

We have a test already that removes secondary ip ranges using secondary_ip_range = [] syntax. That would need to be updated to whatever the new syntax is along with the documentation.

[github-actions]

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

[c2thorn]

Just updating that I'm getting to this now and should hopefully have an update tomorrow on the effect of removing schema_config_mode_attr and how to handle emptying fields without it.

[c2thorn]

After looking into this, this PR can be merged without removing schema_config_mode_attr from secondaryIpRange

I plan to remove schema_config_mode_attr separately after adding a 5.X workaround with #11410

So once that's done this should not be a breaking change, but it does need to go into the 6.0 branch.

[daanheikens]

After looking into this, this PR can be merged without removing schema_config_mode_attr from secondaryIpRange

I plan to remove schema_config_mode_attr separately after adding a 5.X workaround with #11410

So once that's done this should not be a breaking change, but it does need to go into the 6.0 branch.

Ah great news @c2thorn. I do see the VCR test is still failing, anything we need to do for that?

[c2thorn]

Ah great news @c2thorn. I do see the VCR test is still failing, anything we need to do for that?

yes it looks like the error for TestAccComputeSubnetwork_subnetworkReservedInternalRangeExample is real

        Error: "name" ("") doesn't match regexp "^(?:[a-z](?:[-a-z0-9]{0,61}[a-z0-9])?)$"
        
          with google_compute_network.default,
          on terraform_plugin_test.tf line 12, in resource "google_compute_network" "default":
          12:   name                    = ""

@trodge

[trodge]

This should fix the test error @c2thorn mentioned.

[daanheikens]

This should fix the test error @c2thorn mentioned.

Applied the suggestions!

[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, 189 insertions(+), 32 deletions(-))
google-beta provider: Diff ( 3 files changed, 319 insertions(+), 35 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 21 insertions(+))

[modular-magician]

Tests analytics

Total tests: 981
Passed tests: 909
Skipped tests: 72
Affected tests: 0

Click here to see the affected service packages

• compute

$\textcolor{green}{\textsf{All tests passed!}}$

View the build log