Region config check #1554

VenelinMartinov · 2024-01-24T19:10:40Z

Should address issue raised in #129 (comment)

Adds region validation to the GCP provider.

Error before (during up):

  gcp:cloudfunctions:Function (test-published):
    error: 1 error occurred:
        * googleapi: Error 403: Permission denied on 'locations/westus' (or it may not exist)., forbidden

Error after (during preview):

Diagnostics:
  pulumi:providers:gcp (default_7_6_0):
    error: pulumi:providers:gcp resource 'default_7_6_0' has a problem: could not validate provider configuration: region "westus" is not available for project "pulumi-development". Available regions: ["asia-east1" "asia-east2" "asia-northeast1" "asia-northeast2" "asia-northeast3" "asia-south1" "asia-south2" "asia-southeast1" "asia-southeast2" "australia-southeast1" "australia-southeast2" "europe-central2" "europe-north1" "europe-southwest1" "europe-west1" "europe-west10" "europe-west12" "europe-west2" "europe-west3" "europe-west4" "europe-west6" "europe-west8" "europe-west9" "me-central1" "me-central2" "me-west1" "northamerica-northeast1" "northamerica-northeast2" "southamerica-east1" "southamerica-west1" "us-central1" "us-east1" "us-east4" "us-east5" "us-south1" "us-west1" "us-west2" "us-west3" "us-west4"]

github-actions · 2024-01-27T13:05:22Z

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

Maintainer note: consult the runbook for dealing with any breaking changes.

provider/provider_yaml_test.go

iwahbe

This is a great change. I left a couple comments to address before we merge.

provider/provider_yaml_test.go

iwahbe · 2024-01-30T09:55:24Z

provider/provider_yaml_test.go

+	t.Setenv("GOOGLE_PROJECT", "")
+	t.Setenv("GOOGLE_ZONE", "")
+	t.Setenv("GOOGLE_REGION", "")
+	credentialsValidationRun.Store(false)


This is not safe, since a different run of a similar test could set credentialsValidationRun to true between this store and a call to replay. I would prefer if remove the global state here, making credentialsValidationRun a local variable in Provider().

I'm happy to open an issue in the bridge about creating a PreCheckConfigCallback interface, which will have the semantics of only ever being called once and we can schedule follow-up work on this. The issue you described is unlikely and will only effect tests, so I'd say it is out of scope for this PR.

That interface would be useful on the AWS side and then we can also carry it over to other t1 providers.

pulumi/pulumi-terraform-bridge#1658

Just saw pulumi/pulumi-aws#3310 (comment), good idea. Will do that instead.

provider/resources.go

t0yv0 · 2024-01-30T15:17:31Z

error: pulumi:providers:gcp resource 'default_7_6_0' has a problem: could not validate provider configuration: region "westus" is not available for project "pulumi-development". Available regions: ["asia-east1" "asia-east2" "asia-northeast1" "asia-northeast2" "asia-northeast3" "asia-south1" "asia-south2" "asia-southeast1" "asia-southeast2" "australia-southeast1" "australia-southeast2" "europe-central2" "europe-north1" "europe-southwest1" "europe-west1" "europe-west10" "europe-west12" "europe-west2" "europe-west3" "europe-west4" "europe-west6" "europe-west8" "europe-west9" "me-central1" "me-central2" "me-west1" "northamerica-northeast1" "northamerica-northeast2" "southamerica-east1" "southamerica-west1" "us-central1" "us-east1" "us-east4" "us-east5" "us-south1" "us-west1" "us-west2" "us-west3" "us-west4"]

Usability nit: the list is rather long. Any chance to replace with URL? Also curious what happens if region is required but missing. I think that's the more common case than misspelling the region. But may need to be in a separate PR. Maybe it's OK, it's one of these things, clearly this is a huge improvement on status quo!

VenelinMartinov · 2024-01-31T12:55:32Z

Usability nit: the list is rather long. Any chance to replace with URL?

I've adjusted the error message to point to the gcp docs. I've also separated them in txt files as in AWS.

Also curious what happens if region is required but missing

Unfortunately I don't believe we can do anything about that without breaking existing programs, since region is not required.:

Some resources in GCP are actually global (like bucket), unlike AWS, so region is not needed.
Users can specify the region on the resource itself without a global provider config.

VenelinMartinov · 2024-01-31T15:45:38Z

https://github.com/pulumi/pulumi-gcp/actions/runs/7726974699/job/21064805014?pr=1554 More test failures due to the different project names during recording and CI.

I'll add a flag to skip the region check and use that in the tests. Might be good to have for users anyway, in case I'm not seeing some case in which this breaks. AWS has similar flags for the checks in preConfigureCallback

VenelinMartinov · 2024-01-31T18:10:50Z

provider/provider_test.go

 	if config != nil {
 		for k, v := range config {
 			test.SetConfig(k, v)
 		}
 	}
+	// The SetConfig above does not seem to be working here.
+	test.T().Setenv("PULUMI_GCP_SKIP_REGION_VALIDATION", "true")


I think this is pulumi/providertest#56 will look into it later.

VenelinMartinov · 2024-01-31T18:11:16Z

provider/resources.go

+			return fmt.Errorf(noCredentialsErr, err)
+		}
+
+		skipRegionValidation := tfbridge.ConfigBoolValue(


I added a config flag to control the region validation behaviour.

iwahbe

This looks good to me. LGTM

This reverts commit fa56f09.

This reverts #1554 Fixes #1641

Should fix #129 by reintroducing #1554 reverted in #1650 I've also added a mechanism to pass in options to the GCP client via `preConfigureCallback` as well as tests for that around the region list behaviour. I haven't tested with OIDC auth since it seems quite involved. The new tests should ensure that we have not introduced any failures there, LMK if you don't think this is sufficient.

VenelinMartinov added 3 commits January 24, 2024 19:21

dedup gcp preconfigure check

3cae8c9

formatting

ff5c0d3

add validation for the gcp region

4951297

VenelinMartinov changed the base branch from master to vvm/dedup_gcp_preconfigure_check January 24, 2024 19:11

VenelinMartinov marked this pull request as draft January 24, 2024 19:15

VenelinMartinov mentioned this pull request Jan 24, 2024

Missing configuration leads to poor errors #129

Closed

Base automatically changed from vvm/dedup_gcp_preconfigure_check to master January 27, 2024 12:51

Merge branch 'master' into vvm/region_check

ba27205

VenelinMartinov mentioned this pull request Jan 27, 2024

Upgrade pulumi-terraform-bridge to v3.72.0 #1580

Merged

VenelinMartinov added 3 commits January 27, 2024 15:28

Merge branch 'master' into vvm/region_check

1906ab3

region tests

f41aff0

test woraround for atomic flag

99d95ee

VenelinMartinov marked this pull request as ready for review January 29, 2024 12:09

VenelinMartinov added 6 commits January 29, 2024 14:11

Merge branch 'master' into vvm/region_check

91a955b

unset more project env

a1279d9

use t.setenv for testing env

f6c0152

add skips for short/non-short mode

95c563c

fix tests

29b634b

remove project

3a20fce

VenelinMartinov requested review from iwahbe, guineveresaenger and t0yv0 January 29, 2024 22:21

skip broken test

a1b3ffc

guineveresaenger reviewed Jan 30, 2024

View reviewed changes

provider/provider_yaml_test.go Outdated Show resolved Hide resolved

remove skipped test

625cdc9

iwahbe reviewed Jan 30, 2024

View reviewed changes

VenelinMartinov mentioned this pull request Jan 30, 2024

Create a PreCheckConfigCallback interface for validating credentials/config with guarantees of only being called once. pulumi/pulumi-terraform-bridge#1658

Closed

iwahbe mentioned this pull request Jan 31, 2024

Workflow failure: master #1610

Closed

adjust error messages

69333a2

VenelinMartinov added 7 commits January 31, 2024 14:58

make credential validation flag local

04af558

use tab not spaces

939dcd4

remove global flag references

3edf482

Merge branch 'master' into vvm/region_check

d22187b

change

2de5f43

revert formatting

e896d33

fix test

db92dcd

VenelinMartinov added 2 commits January 31, 2024 17:53

add skipRegionValidation flag and use it for upgrade tests

0b96c47

add skip validation as an extra config

62fb869

VenelinMartinov commented Jan 31, 2024

View reviewed changes

VenelinMartinov requested a review from iwahbe January 31, 2024 18:11

VenelinMartinov added 2 commits January 31, 2024 20:12

use t instead of test.T()

62bfc78

schema and sdk

c4b1143

iwahbe approved these changes Jan 31, 2024

View reviewed changes

VenelinMartinov added 2 commits January 31, 2024 21:11

remove explicit config

07c76cd

remove explicit config

f44fcbd

VenelinMartinov enabled auto-merge (squash) January 31, 2024 19:17

VenelinMartinov merged commit fa56f09 into master Jan 31, 2024
16 checks passed

VenelinMartinov deleted the vvm/region_check branch January 31, 2024 19:44

t0yv0 mentioned this pull request Feb 7, 2024

Failed to get regions list failure in previously working scripts #1641

Closed

t0yv0 added a commit that referenced this pull request Feb 8, 2024

Revert "Region config check (#1554)"

9834b63

This reverts commit fa56f09.

t0yv0 mentioned this pull request Feb 8, 2024

Revert broken region validation #1650

Merged

t0yv0 added a commit that referenced this pull request Feb 8, 2024

Revert "Region config check (#1554)"

dc5f07e

This reverts commit fa56f09.

t0yv0 added a commit that referenced this pull request Feb 8, 2024

Revert broken region validation (#1650)

55dea34

This reverts #1554 Fixes #1641

VenelinMartinov mentioned this pull request Feb 13, 2024

Reintroduce region checks #1666

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Region config check #1554

Region config check #1554

VenelinMartinov commented Jan 24, 2024 •

edited

Loading

github-actions bot commented Jan 27, 2024

iwahbe left a comment

iwahbe Jan 30, 2024

VenelinMartinov Jan 30, 2024

VenelinMartinov Jan 30, 2024

VenelinMartinov Jan 30, 2024

t0yv0 commented Jan 30, 2024 •

edited

Loading

VenelinMartinov commented Jan 31, 2024 •

edited

Loading

VenelinMartinov commented Jan 31, 2024

VenelinMartinov Jan 31, 2024

VenelinMartinov Jan 31, 2024

iwahbe left a comment

Region config check #1554

Region config check #1554

Conversation

VenelinMartinov commented Jan 24, 2024 • edited Loading

github-actions bot commented Jan 27, 2024

Does the PR have any schema changes?

Does the PR have any schema changes?

iwahbe left a comment

Choose a reason for hiding this comment

iwahbe Jan 30, 2024

Choose a reason for hiding this comment

VenelinMartinov Jan 30, 2024

Choose a reason for hiding this comment

VenelinMartinov Jan 30, 2024

Choose a reason for hiding this comment

VenelinMartinov Jan 30, 2024

Choose a reason for hiding this comment

t0yv0 commented Jan 30, 2024 • edited Loading

VenelinMartinov commented Jan 31, 2024 • edited Loading

VenelinMartinov commented Jan 31, 2024

VenelinMartinov Jan 31, 2024

Choose a reason for hiding this comment

VenelinMartinov Jan 31, 2024

Choose a reason for hiding this comment

iwahbe left a comment

Choose a reason for hiding this comment

VenelinMartinov commented Jan 24, 2024 •

edited

Loading

t0yv0 commented Jan 30, 2024 •

edited

Loading

VenelinMartinov commented Jan 31, 2024 •

edited

Loading