From 3f338dc6bcea8d0d8b312f7c496c716c76e6cbdb Mon Sep 17 00:00:00 2001 From: The Magician Date: Fri, 22 Sep 2023 10:47:12 -0400 Subject: [PATCH] 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 Co-authored-by: Sarah French <15078782+SarahFrench@users.noreply.github.com> --- .changelog/8587.txt | 2 + google-beta/fwresource/framework_location.go | 30 +++++++++---- .../fwresource/framework_location_test.go | 45 ++++++++++++------- google-beta/tpgresource/field_helpers.go | 3 +- google-beta/tpgresource/regional_utils.go | 20 ++++++--- google-beta/tpgresource/utils_test.go | 41 +++++++++++------ 6 files changed, 95 insertions(+), 46 deletions(-) create mode 100644 .changelog/8587.txt diff --git a/.changelog/8587.txt b/.changelog/8587.txt new file mode 100644 index 0000000000..e4e22fc1e4 --- /dev/null +++ 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 index 6aea39fc10..72c93c6f37 100644 --- 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 index de8f846dcc..a790bcdd97 100644 --- 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 index 8bbbfe1016..9ca932ac34 100644 --- 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 index e6379d21f1..7a060e50c6 100644 --- 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 index 44ebca4b8a..7f92858ee9 100644 --- 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{