Fix Inconsistent logic when getting zone/region/location (#8587) (#6340)

* fix inconsistencies in GetLocation test * fix inconsistencies in GetRegion test * fix default value in provider using region over zone * update test keys * add zone selflink check to SDK tests * add location selflink check * Update mmv1/third_party/terraform/fwresource/framework_location_test.go * Update mmv1/third_party/terraform/fwresource/framework_location_test.go * remove unnecessary test case checking that region is not set in provider * update GetLocation and tests to include region provider value * refactor selflink zone code * add test cases that handles region/zone values from provider when self link * add test cases checking region value in provider config with self link value * refactor tests in GetRegion function * add StringValue func --------- Signed-off-by: Modular Magician <magic-modules@google.com> Co-authored-by: Sarah French <15078782+SarahFrench@users.noreply.github.com>
hashicorp · Sep 22, 2023 · 3f338dc · 3f338dc
1 parent 9a34477
commit 3f338dc
Show file tree

Hide file tree

Showing 6 changed files with 95 additions and 46 deletions.
diff --git a/.changelog/8587.txt b/.changelog/8587.txt
@@ -0,0 +1,2 @@
+```release-note:none
+```
diff --git a/google-beta/fwresource/framework_location.go b/google-beta/fwresource/framework_location.go
@@ -34,12 +34,14 @@ type LocationDescription struct {
 func (ld *LocationDescription) GetLocation() (types.String, error) {
 	// Location from resource config
 	if !ld.ResourceLocation.IsNull() && !ld.ResourceLocation.IsUnknown() && !ld.ResourceLocation.Equal(types.StringValue("")) {
-		return ld.ResourceLocation, nil
+		location := tpgresource.GetResourceNameFromSelfLink(ld.ResourceLocation.ValueString()) // Location could be a self link
+		return types.StringValue(location), nil
 	}
 
 	// Location from region in resource config
 	if !ld.ResourceRegion.IsNull() && !ld.ResourceRegion.IsUnknown() && !ld.ResourceRegion.Equal(types.StringValue("")) {
-		return ld.ResourceRegion, nil
+		region := tpgresource.GetResourceNameFromSelfLink(ld.ResourceRegion.ValueString()) // Region could be a self link
+		return types.StringValue(region), nil
 	}
 
 	// Location from zone in resource config
@@ -48,9 +50,16 @@ func (ld *LocationDescription) GetLocation() (types.String, error) {
 		return types.StringValue(location), nil
 	}
 
+	// Location from region in provider config
+	if !ld.ProviderRegion.IsNull() && !ld.ProviderRegion.IsUnknown() && !ld.ProviderRegion.Equal(types.StringValue("")) {
+		location := tpgresource.GetResourceNameFromSelfLink(ld.ProviderRegion.ValueString()) // Region could be a self link
+		return types.StringValue(location), nil
+	}
+
 	// Location from zone in provider config
 	if !ld.ProviderZone.IsNull() && !ld.ProviderZone.IsUnknown() && !ld.ProviderZone.Equal(types.StringValue("")) {
-		return ld.ProviderZone, nil
+		location := tpgresource.GetResourceNameFromSelfLink(ld.ProviderZone.ValueString()) // Zone could be a self link
+		return types.StringValue(location), nil
 	}
 
 	var err error
@@ -73,17 +82,18 @@ func (ld *LocationDescription) GetRegion() (types.String, error) {
 	}
 	// Region from zone in resource config
 	if !ld.ResourceZone.IsNull() && !ld.ResourceZone.IsUnknown() && !ld.ResourceZone.Equal(types.StringValue("")) {
-		region := tpgresource.GetRegionFromZone(ld.ResourceZone.ValueString())
-		return types.StringValue(region), nil
+		region := tpgresource.GetResourceNameFromSelfLink(ld.ResourceZone.ValueString()) // Region could be a self link
+		return types.StringValue(tpgresource.GetRegionFromZone(region)), nil
 	}
 	// Region from provider config
 	if !ld.ProviderRegion.IsNull() && !ld.ProviderRegion.IsUnknown() && !ld.ProviderRegion.Equal(types.StringValue("")) {
-		return ld.ProviderRegion, nil
+		region := tpgresource.GetResourceNameFromSelfLink(ld.ProviderRegion.ValueString()) // Region could be a self link
+		return types.StringValue(region), nil
 	}
 	// Region from zone in provider config
 	if !ld.ProviderZone.IsNull() && !ld.ProviderZone.IsUnknown() && !ld.ProviderZone.Equal(types.StringValue("")) {
-		region := tpgresource.GetRegionFromZone(ld.ProviderZone.ValueString())
-		return types.StringValue(region), nil
+		region := tpgresource.GetResourceNameFromSelfLink(ld.ProviderZone.ValueString()) // Region could be a self link
+		return types.StringValue(tpgresource.GetRegionFromZone(region)), nil
 	}
 
 	var err error
@@ -105,7 +115,9 @@ func (ld *LocationDescription) GetZone() (types.String, error) {
 		return types.StringValue(zone), nil
 	}
 	if !ld.ProviderZone.IsNull() && !ld.ProviderZone.IsUnknown() && !ld.ProviderZone.Equal(types.StringValue("")) {
-		return ld.ProviderZone, nil
+		// Zone could be a self link
+		zone := tpgresource.GetResourceNameFromSelfLink(ld.ProviderZone.ValueString())
+		return types.StringValue(zone), nil
 	}
 
 	var err error

diff --git a/google-beta/fwresource/framework_location_test.go b/google-beta/fwresource/framework_location_test.go
@@ -128,11 +128,23 @@ func TestLocationDescription_GetRegion(t *testing.T) {
 			},
 			ExpectedRegion: types.StringValue("provider-zone"), // is truncated
 		},
-		"does not shorten region values when derived from a zone self link set in the resource config": {
+		"shortens region values when derived from a zone self link set in the resource config": {
 			ld: LocationDescription{
 				ResourceZone: types.StringValue("https://www.googleapis.com/compute/v1/projects/my-project/zones/us-central1-a"),
 			},
-			ExpectedRegion: types.StringValue("https://www.googleapis.com/compute/v1/projects/my-project/zones/us-central1"), // Value isn't shortened from URI to name
+			ExpectedRegion: types.StringValue("us-central1"),
+		},
+		"shortens region values set as self links in the provider config": {
+			ld: LocationDescription{
+				ProviderRegion: types.StringValue("https://www.googleapis.com/compute/v1/projects/my-project/regions/us-central1"),
+			},
+			ExpectedRegion: types.StringValue("us-central1"),
+		},
+		"shortens region values when derived from a zone self link set in the provider config": {
+			ld: LocationDescription{
+				ProviderZone: types.StringValue("https://www.googleapis.com/compute/v1/projects/my-project/zones/us-central1-a"),
+			},
+			ExpectedRegion: types.StringValue("us-central1"),
 		},
 		"returns the value of the region field in provider config when region/zone is unset in resource config": {
 			ld: LocationDescription{
@@ -230,11 +242,11 @@ func TestLocationDescription_GetLocation(t *testing.T) {
 			},
 			ExpectedLocation: types.StringValue("resource-location"),
 		},
-		"does not shorten the location value when it is set as a self link in the resource config": {
+		"shortens the location value when it is set as a self link in the resource config": {
 			ld: LocationDescription{
 				ResourceLocation: types.StringValue("https://www.googleapis.com/compute/v1/projects/my-project/locations/resource-location"),
 			},
-			ExpectedLocation: types.StringValue("https://www.googleapis.com/compute/v1/projects/my-project/locations/resource-location"),
+			ExpectedLocation: types.StringValue("resource-location"),
 		},
 		"returns the region value set in the resource config when location is not in the schema": {
 			ld: LocationDescription{
@@ -243,11 +255,11 @@ func TestLocationDescription_GetLocation(t *testing.T) {
 			},
 			ExpectedLocation: types.StringValue("resource-region"),
 		},
-		"does not shorten the region value when it is set as a self link in the resource config": {
+		"shortens the region value when it is set as a self link in the resource config": {
 			ld: LocationDescription{
 				ResourceRegion: types.StringValue("https://www.googleapis.com/compute/v1/projects/my-project/regions/resource-region"),
 			},
-			ExpectedLocation: types.StringValue("https://www.googleapis.com/compute/v1/projects/my-project/regions/resource-region"),
+			ExpectedLocation: types.StringValue("resource-region"),
 		},
 		"returns the zone value set in the resource config when neither location nor region in the schema": {
 			ld: LocationDescription{
@@ -261,18 +273,24 @@ func TestLocationDescription_GetLocation(t *testing.T) {
 			},
 			ExpectedLocation: types.StringValue("resource-zone-a"),
 		},
-		"returns the zone value from the provider config when none of location/region/zone are set in the resource config": {
+		"returns the region value from the provider config when none of location/region/zone are set in the resource config": {
 			ld: LocationDescription{
-				ProviderRegion: types.StringValue("provider-region"), // unused
+				ProviderRegion: types.StringValue("provider-region"), // Preferred to use region value over zone value if both are set
 				ProviderZone:   types.StringValue("provider-zone-a"),
 			},
+			ExpectedLocation: types.StringValue("provider-region"),
+		},
+		"returns the zone value from the provider config when none of location/region/zone are set in the resource config and region is not set in the provider config": {
+			ld: LocationDescription{
+				ProviderZone: types.StringValue("provider-zone-a"),
+			},
 			ExpectedLocation: types.StringValue("provider-zone-a"),
 		},
-		"does not shorten the zone value when it is set as a self link in the provider config": {
+		"shortens the zone value when it is set as a self link in the provider config": {
 			ld: LocationDescription{
 				ProviderZone: types.StringValue("https://www.googleapis.com/compute/v1/projects/my-project/zones/provider-zone-a"),
 			},
-			ExpectedLocation: types.StringValue("https://www.googleapis.com/compute/v1/projects/my-project/zones/provider-zone-a"),
+			ExpectedLocation: types.StringValue("provider-zone-a"),
 		},
 		// Handling of empty strings
 		"returns the region value set in the resource config when location is an empty string": {
@@ -299,13 +317,6 @@ func TestLocationDescription_GetLocation(t *testing.T) {
 			},
 			ExpectedLocation: types.StringValue("provider-zone-a"),
 		},
-		// Error states
-		"does not use the region value set in the provider config": {
-			ld: LocationDescription{
-				ProviderRegion: types.StringValue("provider-region"),
-			},
-			ExpectedError: true,
-		},
 		"returns an error when none of location/region/zone are set on the resource, and neither region or zone is set on the provider": {
 			ExpectedError: true,
 		},

diff --git a/google-beta/tpgresource/field_helpers.go b/google-beta/tpgresource/field_helpers.go
@@ -389,7 +389,8 @@ func GetRegionFromSchema(regionSchemaField, zoneSchemaField string, d TerraformR
 		return GetResourceNameFromSelfLink(v.(string)), nil
 	}
 	if v, ok := d.GetOk(zoneSchemaField); ok && zoneSchemaField != "" {
-		return GetRegionFromZone(v.(string)), nil
+		zone := GetResourceNameFromSelfLink(v.(string))
+		return GetRegionFromZone(zone), nil
 	}
 	if config.Region != "" {
 		return config.Region, nil

diff --git a/google-beta/tpgresource/regional_utils.go b/google-beta/tpgresource/regional_utils.go
@@ -19,17 +19,25 @@ func IsZone(location string) bool {
 // - location argument in the resource config
 // - region argument in the resource config
 // - zone argument in the resource config
+// - region argument in the provider config
 // - zone argument set in the provider config
 func GetLocation(d TerraformResourceData, config *transport_tpg.Config) (string, error) {
 	if v, ok := d.GetOk("location"); ok {
-		return v.(string), nil
+		return GetResourceNameFromSelfLink(v.(string)), nil
 	} else if v, isRegionalCluster := d.GetOk("region"); isRegionalCluster {
-		return v.(string), nil
+		return GetResourceNameFromSelfLink(v.(string)), nil
 	} else {
-		// If region is not explicitly set, use "zone" (or fall back to the provider-level zone).
-		// For now, to avoid confusion, we require region to be set in the config to create a regional
-		// cluster rather than falling back to the provider-level region.
-		return GetZone(d, config)
+		if v, ok := d.GetOk("zone"); ok {
+			return GetResourceNameFromSelfLink(v.(string)), nil
+		} else {
+			if config.Region != "" {
+				return GetResourceNameFromSelfLink(config.Region), nil
+			} else if config.Zone != "" {
+				return GetResourceNameFromSelfLink(config.Zone), nil
+			} else {
+				return "", fmt.Errorf("Unable to determine location: region/zone not configured in resource/provider config")
+			}
+		}
 	}
 }
 

diff --git a/google-beta/tpgresource/utils_test.go b/google-beta/tpgresource/utils_test.go
@@ -241,11 +241,11 @@ func TestGetLocation(t *testing.T) {
 			},
 			ExpectedLocation: "resource-location",
 		},
-		"does not shorten the location value when it is set as a self link in the resource config": {
+		"shortens the location value when it is set as a self link in the resource config": {
 			ResourceConfig: map[string]interface{}{
 				"location": "https://www.googleapis.com/compute/v1/projects/my-project/locations/resource-location",
 			},
-			ExpectedLocation: "https://www.googleapis.com/compute/v1/projects/my-project/locations/resource-location", // No shortening takes place
+			ExpectedLocation: "resource-location",
 		},
 		"returns the region value set in the resource config when location is not in the schema": {
 			ResourceConfig: map[string]interface{}{
@@ -254,11 +254,11 @@ func TestGetLocation(t *testing.T) {
 			},
 			ExpectedLocation: "resource-region",
 		},
-		"does not shorten the region value when it is set as a self link in the resource config": {
+		"shortens the region value when it is set as a self link in the resource config": {
 			ResourceConfig: map[string]interface{}{
 				"region": "https://www.googleapis.com/compute/v1/projects/my-project/region/resource-region",
 			},
-			ExpectedLocation: "https://www.googleapis.com/compute/v1/projects/my-project/region/resource-region", // No shortening takes place
+			ExpectedLocation: "resource-region",
 		},
 		"returns the zone value set in the resource config when neither location nor region in the schema": {
 			ResourceConfig: map[string]interface{}{
@@ -280,13 +280,23 @@ func TestGetLocation(t *testing.T) {
 			},
 			ExpectedLocation: "provider-zone-a",
 		},
-		"does not shorten the zone value when it is set as a self link in the provider config": {
-			// This behaviour makes sense because provider config values don't originate from APIs
-			// Users should always configure the provider with the short names of regions/zones
+		"returns the region value from the provider config when none of location/region/zone are set in the resource config": {
+			ProviderConfig: map[string]string{
+				"region": "provider-region",
+			},
+			ExpectedLocation: "provider-region",
+		},
+		"shortens the region value when it is set as a self link in the provider config": {
+			ProviderConfig: map[string]string{
+				"region": "https://www.googleapis.com/compute/v1/projects/my-project/region/provider-region",
+			},
+			ExpectedLocation: "provider-region",
+		},
+		"shortens the zone value when it is set as a self link in the provider config": {
 			ProviderConfig: map[string]string{
 				"zone": "https://www.googleapis.com/compute/v1/projects/my-project/zones/provider-zone-a",
 			},
-			ExpectedLocation: "https://www.googleapis.com/compute/v1/projects/my-project/zones/provider-zone-a",
+			ExpectedLocation: "provider-zone-a",
 		},
 		// Handling of empty strings
 		"returns the region value set in the resource config when location is an empty string": {
@@ -315,13 +325,18 @@ func TestGetLocation(t *testing.T) {
 			},
 			ExpectedLocation: "provider-zone-a",
 		},
-		// Error states
-		"returns an error when only a region value is set in the the provider config and none of location/region/zone are set in the resource config": {
+		"returns the region value when only a region value is set in the the provider config and none of location/region/zone are set in the resource config": {
+			ResourceConfig: map[string]interface{}{
+				"location": "",
+				"region":   "",
+				"zone":     "",
+			},
 			ProviderConfig: map[string]string{
 				"region": "provider-region",
 			},
-			ExpectError: true,
+			ExpectedLocation: "provider-region",
 		},
+		// Error states
 		"returns an error when none of location/region/zone are set on the resource, and neither region or zone is set on the provider": {
 			ExpectError: true,
 		},
@@ -500,11 +515,11 @@ func TestGetRegion(t *testing.T) {
 			},
 			ExpectedRegion: "resource-zone", // is truncated
 		},
-		"does not shorten region values when derived from a zone self link set in the resource config": {
+		"shortens region values when derived from a zone self link set in the resource config": {
 			ResourceConfig: map[string]interface{}{
 				"zone": "https://www.googleapis.com/compute/v1/projects/my-project/zones/us-central1-a",
 			},
-			ExpectedRegion: "https://www.googleapis.com/compute/v1/projects/my-project/zones/us-central1", // Value is not shortenedfrom URI to name
+			ExpectedRegion: "us-central1",
 		},
 		"returns the value of the region field in provider config when region/zone is unset in resource config": {
 			ProviderConfig: map[string]string{