Correct SecurityGateway to be global only

[Berro321]

Change is for correcting a mistake in the original submission from last week: #12695

beyondcorp: corrected `location` to always be global in `google_beyondcorp_security_gateway` 

[github-actions]

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

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

[Berro321]

@melinath Is it reasonable to add the override-breaking-change label to this PR? This is a correction for a mistake in the previous change, we don't have any customers using this yet.

[melinath]

I don't think we should roll this back, since it would break folks who start using this resource using v6.16.0. To put it in terms of our policies, in minor releases we only allow breaking changes that impact all valid configurations. However, in this case, setting location to "global" works, and removing the field would break that configuration.

Instead, I would suggest changing the field from required to optional and setting a default_value of global.

If the field is expected to continue only supporting global indefinitely, you could also add client-side validation to ensure that it is set to "global". Otherwise, you could document that it only allows "global" or link to c.g.c documentation that lists the allowed values.

[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 ( 4 files changed, 15 insertions(+), 16 deletions(-))
google-beta provider: Diff ( 4 files changed, 15 insertions(+), 16 deletions(-))
Open in Cloud Shell: Diff ( 1 file changed, 1 deletion(-))

Breaking Change(s) Detected

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

• Field location default value changed from to global on google_beyondcorp_security_gateway - 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.

Missing test report

Your PR includes resource fields which are not covered by any test.

Resource: google_beyondcorp_security_gateway (3 total tests)
Please add an acceptance test which includes these fields. The test should include the following:

resource "google_beyondcorp_security_gateway" "primary" {
  location = # value needed
}

[modular-magician]

Tests analytics

Total tests: 20
Passed tests: 20
Skipped tests: 0
Affected tests: 0

Click here to see the affected service packages

• beyondcorp

🟢 All tests passed!

View the build log

[melinath]

LGTM - breaking change is fine since using values other than global wouldn't work.

By the way, if you want to remove this field, you can do so in the next major release (which will have a contribution window this summer).

[Berro321]

Before submitting, I want to test these changes will work with the IAM policy i'm adding in #12782. Is it ok to change the import_format for the IAM policy to only have "global"? In our case, only the global location supports IAM.

[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 ( 4 files changed, 15 insertions(+), 16 deletions(-))
google-beta provider: Diff ( 4 files changed, 15 insertions(+), 16 deletions(-))
Open in Cloud Shell: Diff ( 1 file changed, 1 deletion(-))

Breaking Change(s) Detected

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

• Field location default value changed from to global on google_beyondcorp_security_gateway - 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.

Missing test report

Your PR includes resource fields which are not covered by any test.

Resource: google_beyondcorp_security_gateway (3 total tests)
Please add an acceptance test which includes these fields. The test should include the following:

resource "google_beyondcorp_security_gateway" "primary" {
  location = # value needed
}

[modular-magician]

Tests analytics

Total tests: 20
Passed tests: 20
Skipped tests: 0
Affected tests: 0

Click here to see the affected service packages

• beyondcorp

🟢 All tests passed!

View the build log

[melinath]

I think you will need to change the URL definitions to all use locations/global instead of locations/{{location}} - that should prevent the location field from being added to the IAM resource, because of how the generation of IAM code works (but it would probably be worth double-checking that it actually works before we merge this.)

If you ever expect to support non-global locations it's probably best to just leave this as is.

If you never expect to support non-global locations, it would probably be good to mark the location field as deprecated for extra clarity.

[Berro321]

If I change all url defs (IAM policy and all outside of it) then it works and doesn't add the locations field. I'll add the deprecation warning since we don't ever plan to have these as global resources. Since the tests will fail if the location is not 'global', is there a way to limit the VCR tests in the PR to only use the global location? Is that through the test_env_vars field in examples?

[melinath]

The tests in this PR are already using the global location; the tests in the IAM PR should also use the global location once the location field no longer exists.

[Berro321]

I'm seeing that the test projects in the IAM PR run out of quota since there is a limit of 1 resource per project. Is there a list of projects that we can increase the quota for?