From 1ea56a2bebf884d244671228f88fe76778a98ee1 Mon Sep 17 00:00:00 2001 From: Sarah French Date: Tue, 19 Sep 2023 23:13:38 +0100 Subject: [PATCH 1/6] Stop validating `""` as valid `credentials` value in the PF provider config --- mmv1/third_party/terraform/fwprovider/framework_validators.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/mmv1/third_party/terraform/fwprovider/framework_validators.go b/mmv1/third_party/terraform/fwprovider/framework_validators.go index 31bf849d0368..3a0af4733d92 100644 --- a/mmv1/third_party/terraform/fwprovider/framework_validators.go +++ b/mmv1/third_party/terraform/fwprovider/framework_validators.go @@ -7,7 +7,6 @@ import ( "time" "github.com/hashicorp/terraform-plugin-framework/schema/validator" - "github.com/hashicorp/terraform-plugin-framework/types" googleoauth "golang.org/x/oauth2/google" ) @@ -31,7 +30,7 @@ func (v credentialsValidator) MarkdownDescription(ctx context.Context) string { // ValidateString performs the validation. func (v credentialsValidator) ValidateString(ctx context.Context, request validator.StringRequest, response *validator.StringResponse) { - if request.ConfigValue.IsNull() || request.ConfigValue.IsUnknown() || request.ConfigValue.Equal(types.StringValue("")) { + if request.ConfigValue.IsNull() || request.ConfigValue.IsUnknown() { return } From c5d4bfe5f5130aa410df3b112ad40291f91681b2 Mon Sep 17 00:00:00 2001 From: Sarah French Date: Thu, 21 Sep 2023 14:57:00 +0100 Subject: [PATCH 2/6] Remove code that makes PF ignore empty values --- .../fwtransport/framework_config.go.erb | 81 ------------------- 1 file changed, 81 deletions(-) diff --git a/mmv1/third_party/terraform/fwtransport/framework_config.go.erb b/mmv1/third_party/terraform/fwtransport/framework_config.go.erb index 8a1bc8c7b6ba..8c510cbe155c 100644 --- a/mmv1/third_party/terraform/fwtransport/framework_config.go.erb +++ b/mmv1/third_party/terraform/fwtransport/framework_config.go.erb @@ -17,7 +17,6 @@ import ( "google.golang.org/grpc" "github.com/hashicorp/go-cleanhttp" - "github.com/hashicorp/terraform-plugin-framework/attr" "github.com/hashicorp/terraform-plugin-framework/diag" "github.com/hashicorp/terraform-plugin-framework/types" "github.com/hashicorp/terraform-plugin-log/tflog" @@ -56,15 +55,6 @@ type FrameworkProviderConfig struct { // LoadAndValidateFramework handles the bulk of configuring the provider // it is pulled out so that we can manually call this from our testing provider as well func (p *FrameworkProviderConfig) LoadAndValidateFramework(ctx context.Context, data *fwmodels.ProviderModel, tfVersion string, diags *diag.Diagnostics, providerversion string) { - - - // Make the plugin framwork code behave like the SDK by ignoring zero values. This means re-setting zero values to null. - // This is added to fix https://github.com/hashicorp/terraform-provider-google/issues/14255 in a v4.x.x release - // TODO(SarahFrench) remove as part of https://github.com/hashicorp/terraform-provider-google/issues/14447 in 5.0.0 - p.HandleZeroValues(ctx, data, diags) - if diags.HasError() { - return - } // Set defaults if needed p.HandleDefaults(ctx, data, diags) @@ -115,77 +105,6 @@ func (p *FrameworkProviderConfig) LoadAndValidateFramework(ctx context.Context, p.RequestBatcherIam = transport_tpg.NewRequestBatcher("IAM", ctx, batchingConfig) } -// HandleZeroValues will make the plugin framework act like the SDK; zero value, particularly empty strings, are converted to null. -// This causes the plugin framework to treat the field as unset, just like how the SDK ignores empty strings. -func (p *FrameworkProviderConfig) HandleZeroValues(ctx context.Context, data *fwmodels.ProviderModel, diags *diag.Diagnostics) { - - // Change empty strings to null values - if data.AccessToken.Equal(types.StringValue("")) { - data.AccessToken = types.StringNull() - } - if data.BillingProject.Equal(types.StringValue("")) { - data.BillingProject = types.StringNull() - } - if data.Credentials.Equal(types.StringValue("")) { - data.Credentials = types.StringNull() - } - if data.ImpersonateServiceAccount.Equal(types.StringValue("")) { - data.ImpersonateServiceAccount = types.StringNull() - } - if data.Project.Equal(types.StringValue("")) { - data.Project = types.StringNull() - } - if data.Region.Equal(types.StringValue("")) { - data.Region = types.StringNull() - } - if data.RequestReason.Equal(types.StringValue("")) { - data.RequestReason = types.StringNull() - } - if data.RequestTimeout.Equal(types.StringValue("")) { - data.RequestTimeout = types.StringNull() - } - if data.Zone.Equal(types.StringValue("")) { - data.Zone = types.StringNull() - } - - // Change lists that aren't null or unknown with length of zero to null lists - if !data.Scopes.IsNull() && !data.Scopes.IsUnknown() && (len(data.Scopes.Elements()) == 0) { - data.Scopes = types.ListNull(types.StringType) - } - if !data.ImpersonateServiceAccountDelegates.IsNull() && !data.ImpersonateServiceAccountDelegates.IsUnknown() && (len(data.ImpersonateServiceAccountDelegates.Elements()) == 0) { - data.ImpersonateServiceAccountDelegates = types.ListNull(types.StringType) - } - - // Batching implementation will change in future, but this code will be removed in 5.0.0 so may be unaffected - if !data.Batching.IsNull() && !data.Batching.IsUnknown() && (len(data.Batching.Elements()) > 0) { - var pbConfigs []fwmodels.ProviderBatching - d := data.Batching.ElementsAs(ctx, &pbConfigs, true) - diags.Append(d...) - if diags.HasError() { - return - } - if pbConfigs[0].SendAfter.Equal(types.StringValue("")) { - pbConfigs[0].SendAfter = types.StringNull() // Convert empty string to null - } - b, _ := types.ObjectValue( - map[string]attr.Type{ - "enable_batching": types.BoolType, - "send_after": types.StringType, - }, - map[string]attr.Value{ - "enable_batching": pbConfigs[0].EnableBatching, - "send_after": pbConfigs[0].SendAfter, - }, - ) - newBatching, d := types.ListValue(types.ObjectType{}.WithAttributeTypes(fwmodels.ProviderBatchingAttributes), []attr.Value{b}) - diags.Append(d...) - if diags.HasError() { - return - } - data.Batching = newBatching - } -} - // HandleDefaults will handle all the defaults necessary in the provider func (p *FrameworkProviderConfig) HandleDefaults(ctx context.Context, data *fwmodels.ProviderModel, diags *diag.Diagnostics) { if (data.AccessToken.IsNull() || data.AccessToken.IsUnknown()) && (data.Credentials.IsNull() || data.Credentials.IsUnknown()) { From 932d90239d855d5f1e73a3ca19cbd85911196b1f Mon Sep 17 00:00:00 2001 From: Sarah French Date: Thu, 21 Sep 2023 15:00:27 +0100 Subject: [PATCH 3/6] Update PF config code tests to match change in PF config code behaviour --- .../fwtransport/framework_config_test.go.erb | 154 +++++++++--------- 1 file changed, 76 insertions(+), 78 deletions(-) diff --git a/mmv1/third_party/terraform/fwtransport/framework_config_test.go.erb b/mmv1/third_party/terraform/fwtransport/framework_config_test.go.erb index 37c11069bd51..594d4b97d779 100644 --- a/mmv1/third_party/terraform/fwtransport/framework_config_test.go.erb +++ b/mmv1/third_party/terraform/fwtransport/framework_config_test.go.erb @@ -102,22 +102,22 @@ func TestFrameworkProvider_LoadAndValidateFramework_project(t *testing.T) { ExpectedConfigStructValue: types.StringNull(), }, // Handling empty strings in config - "when project is set as an empty string the field is treated as if it's unset, without error": { + "when project is set as an empty string the empty string is used and not ignored": { ConfigValues: fwmodels.ProviderModel{ Project: types.StringValue(""), }, - ExpectedDataModelValue: types.StringNull(), - ExpectedConfigStructValue: types.StringNull(), + ExpectedDataModelValue: types.StringValue(""), + ExpectedConfigStructValue: types.StringValue(""), }, - "when project is set as an empty string an environment variable will be used": { + "when project is set as an empty string, the empty string is not ignored in favor of an environment variable": { ConfigValues: fwmodels.ProviderModel{ Project: types.StringValue(""), }, EnvVariables: map[string]string{ "GOOGLE_PROJECT": "project-from-GOOGLE_PROJECT", }, - ExpectedDataModelValue: types.StringValue("project-from-GOOGLE_PROJECT"), - ExpectedConfigStructValue: types.StringValue("project-from-GOOGLE_PROJECT"), + ExpectedDataModelValue: types.StringValue(""), + ExpectedConfigStructValue: types.StringValue(""), }, // Handling unknown values "when project is an unknown value, the provider treats it as if it's unset and uses an environment variable instead": { @@ -143,7 +143,7 @@ func TestFrameworkProvider_LoadAndValidateFramework_project(t *testing.T) { tfVersion := "foobar" providerversion := "999" diags := diag.Diagnostics{} - + data := tc.ConfigValues data.Credentials = types.StringValue(transport_tpg.TestFakeCredentialsPath) impersonateServiceAccountDelegates, _ := types.ListValue(types.StringType, []attr.Value{}) // empty list @@ -194,40 +194,40 @@ func TestFrameworkProvider_LoadAndValidateFramework_credentials(t *testing.T) { }{ "credentials can be configured as a path to a credentials JSON file": { ConfigValues: fwmodels.ProviderModel{ - Credentials: types.StringValue(transport_tpg.TestFakeCredentialsPath), - }, - ExpectedDataModelValue: types.StringValue(transport_tpg.TestFakeCredentialsPath), - }, - "configuring credentials as a path to a non-existent file results in an error": { - ConfigValues: fwmodels.ProviderModel{ - Credentials: types.StringValue(pathToMissingFile), - }, - ExpectError: true, - }, - "credentials set in the config are not overridden by environment variables": { - ConfigValues: fwmodels.ProviderModel{ - Credentials: types.StringValue(acctest.GenerateFakeCredentialsJson("test")), - }, - EnvVariables: map[string]string{ - "GOOGLE_CREDENTIALS": acctest.GenerateFakeCredentialsJson("GOOGLE_CREDENTIALS"), - "GOOGLE_CLOUD_KEYFILE_JSON": acctest.GenerateFakeCredentialsJson("GOOGLE_CLOUD_KEYFILE_JSON"), - "GCLOUD_KEYFILE_JSON": acctest.GenerateFakeCredentialsJson("GCLOUD_KEYFILE_JSON"), - "GOOGLE_APPLICATION_CREDENTIALS": acctest.GenerateFakeCredentialsJson("GOOGLE_APPLICATION_CREDENTIALS"), - }, - ExpectedDataModelValue: types.StringValue(acctest.GenerateFakeCredentialsJson("test")), - }, - "when credentials is unset in the config, environment variables are used: GOOGLE_CREDENTIALS used first": { - ConfigValues: fwmodels.ProviderModel{ - Credentials: types.StringNull(), // unset - }, - EnvVariables: map[string]string{ - "GOOGLE_CREDENTIALS": acctest.GenerateFakeCredentialsJson("GOOGLE_CREDENTIALS"), - "GOOGLE_CLOUD_KEYFILE_JSON": acctest.GenerateFakeCredentialsJson("GOOGLE_CLOUD_KEYFILE_JSON"), - "GCLOUD_KEYFILE_JSON": acctest.GenerateFakeCredentialsJson("GCLOUD_KEYFILE_JSON"), - "GOOGLE_APPLICATION_CREDENTIALS": acctest.GenerateFakeCredentialsJson("GOOGLE_APPLICATION_CREDENTIALS"), - }, - ExpectedDataModelValue: types.StringValue(acctest.GenerateFakeCredentialsJson("GOOGLE_CREDENTIALS")), + Credentials: types.StringValue(transport_tpg.TestFakeCredentialsPath), }, + ExpectedDataModelValue: types.StringValue(transport_tpg.TestFakeCredentialsPath), + }, + "configuring credentials as a path to a non-existent file results in an error": { + ConfigValues: fwmodels.ProviderModel{ + Credentials: types.StringValue(pathToMissingFile), + }, + ExpectError: true, + }, + "credentials set in the config are not overridden by environment variables": { + ConfigValues: fwmodels.ProviderModel{ + Credentials: types.StringValue(acctest.GenerateFakeCredentialsJson("test")), + }, + EnvVariables: map[string]string{ + "GOOGLE_CREDENTIALS": acctest.GenerateFakeCredentialsJson("GOOGLE_CREDENTIALS"), + "GOOGLE_CLOUD_KEYFILE_JSON": acctest.GenerateFakeCredentialsJson("GOOGLE_CLOUD_KEYFILE_JSON"), + "GCLOUD_KEYFILE_JSON": acctest.GenerateFakeCredentialsJson("GCLOUD_KEYFILE_JSON"), + "GOOGLE_APPLICATION_CREDENTIALS": acctest.GenerateFakeCredentialsJson("GOOGLE_APPLICATION_CREDENTIALS"), + }, + ExpectedDataModelValue: types.StringValue(acctest.GenerateFakeCredentialsJson("test")), + }, + "when credentials is unset in the config, environment variables are used: GOOGLE_CREDENTIALS used first": { + ConfigValues: fwmodels.ProviderModel{ + Credentials: types.StringNull(), // unset + }, + EnvVariables: map[string]string{ + "GOOGLE_CREDENTIALS": acctest.GenerateFakeCredentialsJson("GOOGLE_CREDENTIALS"), + "GOOGLE_CLOUD_KEYFILE_JSON": acctest.GenerateFakeCredentialsJson("GOOGLE_CLOUD_KEYFILE_JSON"), + "GCLOUD_KEYFILE_JSON": acctest.GenerateFakeCredentialsJson("GCLOUD_KEYFILE_JSON"), + "GOOGLE_APPLICATION_CREDENTIALS": acctest.GenerateFakeCredentialsJson("GOOGLE_APPLICATION_CREDENTIALS"), + }, + ExpectedDataModelValue: types.StringValue(acctest.GenerateFakeCredentialsJson("GOOGLE_CREDENTIALS")), + }, "when credentials is unset in the config, environment variables are used: GOOGLE_CLOUD_KEYFILE_JSON used second": { ConfigValues: fwmodels.ProviderModel{ Credentials: types.StringNull(), // unset @@ -264,15 +264,15 @@ func TestFrameworkProvider_LoadAndValidateFramework_credentials(t *testing.T) { }, ExpectedDataModelValue: types.StringNull(), }, - // Handling empty strings in config - "when credentials is set to an empty string in the config (and access_token unset), GOOGLE_APPLICATION_CREDENTIALS is used": { + // Error states + "when credentials is set to an empty string in the config the value isn't ignored and results in an error": { ConfigValues: fwmodels.ProviderModel{ Credentials: types.StringValue(""), }, EnvVariables: map[string]string{ "GOOGLE_APPLICATION_CREDENTIALS": transport_tpg.TestFakeCredentialsPath, // needs to be a path to a file when used by code }, - ExpectedDataModelValue: types.StringNull(), + ExpectError: true, }, // NOTE: these tests can't run in Cloud Build due to ADC locating credentials despite `GOOGLE_APPLICATION_CREDENTIALS` being unset // See https://cloud.google.com/docs/authentication/application-default-credentials#search_order @@ -433,22 +433,22 @@ func TestFrameworkProvider_LoadAndValidateFramework_billingProject(t *testing.T) ExpectedConfigStructValue: types.StringNull(), }, // Handling empty strings in config - "when billing_project is set as an empty string the field is treated as if it's unset, without error": { + "when billing_project is set as an empty string the empty string is used and not ignored": { ConfigValues: fwmodels.ProviderModel{ BillingProject: types.StringValue(""), }, - ExpectedDataModelValue: types.StringNull(), - ExpectedConfigStructValue: types.StringNull(), + ExpectedDataModelValue: types.StringValue(""), + ExpectedConfigStructValue: types.StringValue(""), }, - "when billing_project is set as an empty string an environment variable will be used": { + "when billing_project is set as an empty string, the empty string is not ignored in favor of an environment variable": { ConfigValues: fwmodels.ProviderModel{ BillingProject: types.StringValue(""), }, EnvVariables: map[string]string{ "GOOGLE_BILLING_PROJECT": "billing-project-from-env", }, - ExpectedDataModelValue: types.StringValue("billing-project-from-env"), - ExpectedConfigStructValue: types.StringValue("billing-project-from-env"), + ExpectedDataModelValue: types.StringValue(""), + ExpectedConfigStructValue: types.StringValue(""), }, } @@ -496,7 +496,6 @@ func TestFrameworkProvider_LoadAndValidateFramework_billingProject(t *testing.T) } } - func TestFrameworkProvider_LoadAndValidateFramework_region(t *testing.T) { // Note: In the test function we need to set the below fields in test case's fwmodels.ProviderModel value @@ -548,22 +547,22 @@ func TestFrameworkProvider_LoadAndValidateFramework_region(t *testing.T) { ExpectedConfigStructValue: types.StringNull(), }, // Handling empty strings in config - "when region is set as an empty string the field is treated as if it's unset, without error": { + "when region is set as an empty string the empty string is used and not ignored": { ConfigValues: fwmodels.ProviderModel{ Region: types.StringValue(""), }, - ExpectedDataModelValue: types.StringNull(), - ExpectedConfigStructValue: types.StringNull(), + ExpectedDataModelValue: types.StringValue(""), + ExpectedConfigStructValue: types.StringValue(""), }, - "when region is set as an empty string an environment variable will be used": { + "when region is set as an empty string, the empty string is not ignored in favor of an environment variable": { ConfigValues: fwmodels.ProviderModel{ Region: types.StringValue(""), }, EnvVariables: map[string]string{ "GOOGLE_REGION": "region-from-env", }, - ExpectedDataModelValue: types.StringValue("region-from-env"), - ExpectedConfigStructValue: types.StringValue("region-from-env"), + ExpectedDataModelValue: types.StringValue(""), + ExpectedConfigStructValue: types.StringValue(""), }, // Handling unknown values "when region is an unknown value, the provider treats it as if it's unset and uses an environment variable instead": { @@ -698,22 +697,22 @@ func TestFrameworkProvider_LoadAndValidateFramework_zone(t *testing.T) { ExpectedConfigStructValue: types.StringNull(), }, // Handling empty strings in config - "when zone is set as an empty string the field is treated as if it's unset, without error": { + "when zone is set as an empty string the empty string is used and not ignored": { ConfigValues: fwmodels.ProviderModel{ Zone: types.StringValue(""), }, - ExpectedDataModelValue: types.StringNull(), - ExpectedConfigStructValue: types.StringNull(), + ExpectedDataModelValue: types.StringValue(""), + ExpectedConfigStructValue: types.StringValue(""), }, - "when zone is set as an empty string an environment variable will be used": { + "when zone is set as an empty string, the empty string is not ignored in favor of an environment variable": { ConfigValues: fwmodels.ProviderModel{ Zone: types.StringValue(""), }, EnvVariables: map[string]string{ "GOOGLE_ZONE": "zone-from-env", }, - ExpectedDataModelValue: types.StringValue("zone-from-env"), - ExpectedConfigStructValue: types.StringValue("zone-from-env"), + ExpectedDataModelValue: types.StringValue(""), + ExpectedConfigStructValue: types.StringValue(""), }, // Handling unknown values "when zone is an unknown value, the provider treats it as if it's unset and uses an environment variable instead": { @@ -780,11 +779,11 @@ func TestFrameworkProvider_LoadAndValidateFramework_accessToken(t *testing.T) { // - ImpersonateServiceAccountDelegates: If we don't set this, we get a nil pointer exception ¯\_(ツ)_/¯ cases := map[string]struct { - ConfigValues fwmodels.ProviderModel - EnvVariables map[string]string - ExpectedDataModelValue basetypes.StringValue // Sometimes the value is mutated, and no longer matches the original value we supply + ConfigValues fwmodels.ProviderModel + EnvVariables map[string]string + ExpectedDataModelValue basetypes.StringValue // Sometimes the value is mutated, and no longer matches the original value we supply // ExpectedConfigStructValue not used here, as credentials info isn't stored in the config struct - ExpectError bool + ExpectError bool }{ "access_token configured in the provider can be invalid without resulting in errors": { ConfigValues: fwmodels.ProviderModel{ @@ -815,21 +814,20 @@ func TestFrameworkProvider_LoadAndValidateFramework_accessToken(t *testing.T) { ExpectedDataModelValue: types.StringNull(), }, // Handling empty strings in config - "when access_token is set as an empty string the field is treated as if it's unset, without error (as long as credentials supplied in its absence)": { + "when access_token is set as an empty string the empty string is used and not ignored": { ConfigValues: fwmodels.ProviderModel{ AccessToken: types.StringValue(""), - Credentials: types.StringValue(transport_tpg.TestFakeCredentialsPath), }, - ExpectedDataModelValue: types.StringNull(), + ExpectedDataModelValue: types.StringValue(""), }, - "when access_token is set as an empty string in the config, an environment variable is used": { + "when access_token is set as an empty string, the empty string is not ignored in favor of an environment variable": { ConfigValues: fwmodels.ProviderModel{ AccessToken: types.StringValue(""), }, EnvVariables: map[string]string{ "GOOGLE_OAUTH_ACCESS_TOKEN": "value-from-GOOGLE_OAUTH_ACCESS_TOKEN", }, - ExpectedDataModelValue: types.StringValue("value-from-GOOGLE_OAUTH_ACCESS_TOKEN"), + ExpectedDataModelValue: types.StringValue(""), }, // Handling unknown values "when access_token is an unknown value, the provider treats it as if it's unset and uses an environment variable instead": { @@ -1058,20 +1056,20 @@ func TestFrameworkProvider_LoadAndValidateFramework_impersonateServiceAccount(t ExpectedDataModelValue: types.StringNull(), }, // Handling empty strings in config - "when impersonate_service_account is set as an empty string the field is treated as if it's unset, without error": { + "when impersonate_service_account is set as an empty string the empty string is used and not ignored": { ConfigValues: fwmodels.ProviderModel{ ImpersonateServiceAccount: types.StringValue(""), }, - ExpectedDataModelValue: types.StringNull(), + ExpectedDataModelValue: types.StringValue(""), }, - "when impersonate_service_account is set as an empty string in the config, an environment variable is used": { + "when impersonate_service_account is set as an empty string, the empty string is not ignored in favor of an environment variable": { ConfigValues: fwmodels.ProviderModel{ ImpersonateServiceAccount: types.StringValue(""), }, EnvVariables: map[string]string{ "GOOGLE_IMPERSONATE_SERVICE_ACCOUNT": "value-from-env@example.com", }, - ExpectedDataModelValue: types.StringValue("value-from-env@example.com"), + ExpectedDataModelValue: types.StringValue(""), }, // Handling unknown values "when impersonate_service_account is an unknown value, the provider treats it as if it's unset and uses an environment variable instead": { @@ -1158,13 +1156,13 @@ func TestFrameworkProvider_LoadAndValidateFramework_impersonateServiceAccountDel }, // Note: no environment variables can be used for impersonate_service_account_delegates "when no impersonate_service_account_delegates value is provided via config, the field remains unset without error": { - SetAsNull: true, // not setting impersonate_service_account_delegates + SetAsNull: true, // not setting impersonate_service_account_delegates ExpectedNull: true, }, // Handling empty values in config - "when impersonate_service_account_delegates is set as an empty array the field is treated as if it's unset, without error": { + "when impersonate_service_account_delegates is set as an empty array, that value isn't ignored": { ImpersonateServiceAccountDelegatesValue: []string{}, - ExpectedDataModelValue: nil, + ExpectedDataModelValue: []string{}, }, // Handling unknown values "when impersonate_service_account_delegates is an unknown value, the provider treats it as if it's unset, without error": { From 65199423ba507bf97de83947c015b70ef3b26713 Mon Sep 17 00:00:00 2001 From: Sarah French Date: Thu, 21 Sep 2023 16:21:16 +0100 Subject: [PATCH 4/6] Update remaining PF config code tests --- .../fwtransport/framework_config_test.go.erb | 34 ++++++++----------- 1 file changed, 15 insertions(+), 19 deletions(-) diff --git a/mmv1/third_party/terraform/fwtransport/framework_config_test.go.erb b/mmv1/third_party/terraform/fwtransport/framework_config_test.go.erb index 594d4b97d779..07e1accd25de 100644 --- a/mmv1/third_party/terraform/fwtransport/framework_config_test.go.erb +++ b/mmv1/third_party/terraform/fwtransport/framework_config_test.go.erb @@ -1369,20 +1369,20 @@ func TestFrameworkProvider_LoadAndValidateFramework_requestReason(t *testing.T) ExpectedDataModelValue: types.StringNull(), }, // Handling empty strings in config - "when request_reason is set as an empty string in the config it is overridden by environment variables": { + "when request_reason is set as an empty string, the empty string is not ignored in favor of an environment variable": { ConfigValues: fwmodels.ProviderModel{ RequestReason: types.StringValue(""), }, EnvVariables: map[string]string{ "CLOUDSDK_CORE_REQUEST_REASON": "foo", }, - ExpectedDataModelValue: types.StringValue("foo"), + ExpectedDataModelValue: types.StringValue(""), }, - "when request_reason is set as an empty string in the config the field is treated as if it's unset, without error": { + "when request_reason is set as an empty string the empty string is used and not ignored": { ConfigValues: fwmodels.ProviderModel{ RequestReason: types.StringValue(""), }, - ExpectedDataModelValue: types.StringNull(), + ExpectedDataModelValue: types.StringValue(""), }, // Handling unknown values "when request_reason is an unknown value, the provider treats it as if it's unset and uses an environment variable instead": { @@ -1464,6 +1464,12 @@ func TestFrameworkProvider_LoadAndValidateFramework_requestTimeout(t *testing.T) }, ExpectError: true, }, + "when request_timeout is set as an empty string, the empty string isn't ignored and an error will occur": { + ConfigValues: fwmodels.ProviderModel{ + RequestTimeout: types.StringValue(""), + }, + ExpectError: true, + }, // In the SDK version of the provider config code, this scenario results in a value of "0s" // instead of "120s", but the final 'effective' value is also "120s" // See : https://github.com/hashicorp/terraform-provider-google/blob/09cb850ee64bcd78e4457df70905530c1ed75f19/google/transport/config.go#L1228-L1233 @@ -1473,13 +1479,6 @@ func TestFrameworkProvider_LoadAndValidateFramework_requestTimeout(t *testing.T) }, ExpectedDataModelValue: types.StringValue("120s"), }, - // Handling empty strings in config - "when request_timeout is set as an empty string, the default value is 120s.": { - ConfigValues: fwmodels.ProviderModel{ - RequestTimeout: types.StringValue(""), - }, - ExpectedDataModelValue: types.StringValue("120s"), - }, // Handling unknown values "when request_timeout is an unknown value, the provider treats it as if it's unset and uses the default value 120s": { ConfigValues: fwmodels.ProviderModel{ @@ -1583,13 +1582,6 @@ func TestFrameworkProvider_LoadAndValidateFramework_batching(t *testing.T) { ExpectEnableBatchingValue: types.BoolValue(true), ExpectSendAfterValue: types.StringValue("3s"), }, - // Handling empty strings in config - "when batching is configured with send_after as an empty string, send_after will be set to a default value": { - EnableBatchingValue: types.BoolValue(true), - SendAfterValue: types.StringValue(""), - ExpectEnableBatchingValue: types.BoolValue(true), - ExpectSendAfterValue: types.StringValue("10s"), // When batching block is present but has missing arguments inside, default is 10s - }, // Handling unknown values "when batching is an unknown value, the provider treats it as if it's unset (align to SDK behaviour)": { SetBatchingAsUnknown: true, @@ -1609,6 +1601,11 @@ func TestFrameworkProvider_LoadAndValidateFramework_batching(t *testing.T) { ExpectSendAfterValue: types.StringValue("45s"), }, // Error states + "when batching is configured with send_after as an empty string, the empty string is not ignored and results in an error": { + EnableBatchingValue: types.BoolValue(true), + SendAfterValue: types.StringValue(""), + ExpectError: true, + }, "if batching is configured with send_after as an invalid value, there's an error": { SendAfterValue: types.StringValue("invalid value"), ExpectError: true, @@ -1714,4 +1711,3 @@ func TestFrameworkProvider_LoadAndValidateFramework_batching(t *testing.T) { }) } } - From 10531cd6d51d4de1cd3ad31d2f1af5c9c1403bf8 Mon Sep 17 00:00:00 2001 From: Sarah French Date: Thu, 21 Sep 2023 18:08:43 +0100 Subject: [PATCH 5/6] Update test for the PF version of credentials validator --- .../terraform/fwprovider/framework_provider_internal_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/mmv1/third_party/terraform/fwprovider/framework_provider_internal_test.go b/mmv1/third_party/terraform/fwprovider/framework_provider_internal_test.go index e81196794205..aa866ac10451 100644 --- a/mmv1/third_party/terraform/fwprovider/framework_provider_internal_test.go +++ b/mmv1/third_party/terraform/fwprovider/framework_provider_internal_test.go @@ -44,10 +44,11 @@ func TestFrameworkProvider_CredentialsValidator(t *testing.T) { return types.StringValue(stringContents) }, }, - "configuring credentials as an empty string is valid": { + "configuring credentials as an empty string is not valid": { ConfigValue: func(t *testing.T) types.String { return types.StringValue("") }, + ExpectedErrorCount: 1, }, "leaving credentials unconfigured is valid": { ConfigValue: func(t *testing.T) types.String { From 64fcf9948b99a9e8414be3ce957666186f622b2a Mon Sep 17 00:00:00 2001 From: Sarah French Date: Thu, 21 Sep 2023 18:10:33 +0100 Subject: [PATCH 6/6] Make not on SDK version of credentials validator that it cannot distinguish between `""` in config vs zero values --- mmv1/third_party/terraform/provider/provider.go.erb | 2 ++ mmv1/third_party/terraform/provider/provider_internal_test.go | 4 +++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/mmv1/third_party/terraform/provider/provider.go.erb b/mmv1/third_party/terraform/provider/provider.go.erb index 82a2fe84ca80..271bda2397f2 100644 --- a/mmv1/third_party/terraform/provider/provider.go.erb +++ b/mmv1/third_party/terraform/provider/provider.go.erb @@ -774,6 +774,8 @@ func ValidateCredentials(v interface{}, k string) (warnings []string, errors []e if v == nil || v.(string) == "" { return } + // NOTE: Above we have to allow empty string as valid because we don't know if it's a zero value or not + creds := v.(string) // if this is a path and we can stat it, assume it's ok if _, err := os.Stat(creds); err == nil { diff --git a/mmv1/third_party/terraform/provider/provider_internal_test.go b/mmv1/third_party/terraform/provider/provider_internal_test.go index 27a15710ffda..00bcac3d5926 100644 --- a/mmv1/third_party/terraform/provider/provider_internal_test.go +++ b/mmv1/third_party/terraform/provider/provider_internal_test.go @@ -42,7 +42,9 @@ func TestProvider_ValidateCredentials(t *testing.T) { return string(contents) }, }, - "configuring credentials as an empty string is valid": { + // There's a risk of changing the validator to saying "" is invalid, as it may mean that + // everyone not using the credentials field would get validation errors. + "configuring credentials as an empty string is not identified as invalid by the function, as it can't distinguish from zero values ": { ConfigValue: func(t *testing.T) interface{} { return "" },