Add tests around how plugin framework provider configuration code handles user_project_override values, fix potential bug (#8862) (#15776)

modular-magician · web-flow · commit 0af4c82f3fbf · 2023-09-08T12:24:57.000-04:00
* Add initial version of plugin framework provider config test affected by inaccessible functions

* Refactor provider config tests to use plugin-framework types

* Add test case about handling of Unknown values for `project`

* Update tests to check values in BOTH the data model and provider config struct after `LoadAndValidateFramework` runs

* Add some tests for `credentials` in plugin framework provider, including test case that fails

* Update `LoadAndValidateFramework` to take a pointer to the provider data model, so mutations to the data within the function change the original struct

This enables tests to track how the data is mutated

* Add remaining `credentials` test cases to check PF/SDK config parity

* Make tests unset ADC ENV automatically, update comments to tests setting ADC ENV

* Add test for behaviour when credentials value is unknown

* Add comment referring devs to where unknown value test is implemented

* Remove duplicated test case

* Fix filename so it's generated correctly

* Remove fmt line

* Update `project` tests that are affected by `LoadAndValidateFramework` now taking a pointer as an argument

* Update `FrameworkProviderConfig` to use plugin framework types package for `UserProjectOverride` boolean

* Add plugin framework version of tests for user_project_override, update test case names in SDK version of tests

* Add `user_project_override` test case for handling of unknown value

Signed-off-by: Modular Magician &lt;magic-modules@google.com&gt;
diff --git a/.changelog/8862.txt b/.changelog/8862.txt
@@ -0,0 +1,3 @@
+```release-note:bug
+provider: fixed a bug where `user_project_override` would not be not used correctly when provisioning resources implemented using the plugin framework. Currently there are no resources implemented this way, so no-one should have been impacted.
+```
diff --git a/google/fwtransport/framework_config.go b/google/fwtransport/framework_config.go
@@ -45,7 +45,7 @@ type FrameworkProviderConfig struct {
 	Scopes                     []string
 	TokenSource                oauth2.TokenSource
 	UserAgent                  string
-	UserProjectOverride        bool
+	UserProjectOverride        types.Bool
 
 	// paths for client setup
 	AccessApprovalBasePath           string
@@ -289,6 +289,7 @@ func (p *FrameworkProviderConfig) LoadAndValidateFramework(ctx context.Context,
 	p.Project = data.Project
 	p.Region = data.Region
 	p.Zone = data.Zone
+	p.UserProjectOverride = data.UserProjectOverride
 	p.PollInterval = 10 * time.Second
 	p.RequestBatcherServiceUsage = transport_tpg.NewRequestBatcher("Service Usage", ctx, batchingConfig)
 	p.RequestBatcherIam = transport_tpg.NewRequestBatcher("IAM", ctx, batchingConfig)
diff --git a/google/fwtransport/framework_config_test.go b/google/fwtransport/framework_config_test.go
@@ -882,3 +882,136 @@ func TestFrameworkProvider_LoadAndValidateFramework_accessToken(t *testing.T) {
 		})
 	}
 }
+
+func TestFrameworkProvider_LoadAndValidateFramework_userProjectOverride(t *testing.T) {
+
+	// Note: In the test function we need to set the below fields in test case's fwmodels.ProviderModel value
+	// this is to stop the code under tests experiencing errors, and could be addressed in future refactoring.
+	// - Credentials: If we don't set this then the test looks for application default credentials and can fail depending on the machine running the test
+	// - 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.BoolValue
+		ExpectedConfigStructValue basetypes.BoolValue
+		ExpectError               bool
+	}{
+		"user_project_override value set in the provider schema is not overridden by ENVs": {
+			ConfigValues: fwmodels.ProviderModel{
+				UserProjectOverride: types.BoolValue(false),
+			},
+			EnvVariables: map[string]string{
+				"USER_PROJECT_OVERRIDE": "true",
+			},
+			ExpectedDataModelValue:    types.BoolValue(false),
+			ExpectedConfigStructValue: types.BoolValue(false),
+		},
+		"user_project_override can be set by environment variable: value = true": {
+			ConfigValues: fwmodels.ProviderModel{
+				UserProjectOverride: types.BoolNull(), // not set
+			},
+			EnvVariables: map[string]string{
+				"USER_PROJECT_OVERRIDE": "true",
+			},
+			ExpectedDataModelValue:    types.BoolValue(true),
+			ExpectedConfigStructValue: types.BoolValue(true),
+		},
+		"user_project_override can be set by environment variable: value = false": {
+			ConfigValues: fwmodels.ProviderModel{
+				UserProjectOverride: types.BoolNull(), // not set
+			},
+			EnvVariables: map[string]string{
+				"USER_PROJECT_OVERRIDE": "false",
+			},
+			ExpectedDataModelValue:    types.BoolValue(false),
+			ExpectedConfigStructValue: types.BoolValue(false),
+		},
+		"user_project_override can be set by environment variable: value = 1": {
+			ConfigValues: fwmodels.ProviderModel{
+				UserProjectOverride: types.BoolNull(), // not set
+			},
+			EnvVariables: map[string]string{
+				"USER_PROJECT_OVERRIDE": "1",
+			},
+			ExpectedDataModelValue:    types.BoolValue(true),
+			ExpectedConfigStructValue: types.BoolValue(true),
+		},
+		"user_project_override can be set by environment variable: value = 0": {
+			ConfigValues: fwmodels.ProviderModel{
+				UserProjectOverride: types.BoolNull(), // not set
+			},
+			EnvVariables: map[string]string{
+				"USER_PROJECT_OVERRIDE": "0",
+			},
+			ExpectedDataModelValue:    types.BoolValue(false),
+			ExpectedConfigStructValue: types.BoolValue(false),
+		},
+		"setting user_project_override using a non-boolean environment variables results in an error": {
+			EnvVariables: map[string]string{
+				"USER_PROJECT_OVERRIDE": "I'm not a boolean",
+			},
+			ExpectError: true,
+		},
+		"when no user_project_override values are provided via config or environment variables, the field remains unset without error": {
+			ConfigValues: fwmodels.ProviderModel{
+				UserProjectOverride: types.BoolNull(), // not set
+			},
+			ExpectedDataModelValue:    types.BoolNull(),
+			ExpectedConfigStructValue: types.BoolNull(),
+		},
+		// Handling unknown values
+		// TODO(SarahFrench) make these tests pass to address: https://github.com/hashicorp/terraform-provider-google/issues/14444
+		// "when user_project_override is an unknown value, the provider treats it as if it's unset (align to SDK behaviour)": {
+		// 	ConfigValues: fwmodels.ProviderModel{
+		// 		UserProjectOverride: types.BoolUnknown(),
+		// 	},
+		// 	ExpectedDataModelValue:    types.BoolNull(),
+		// 	ExpectedConfigStructValue: types.BoolNull(),
+		// },
+	}
+
+	for tn, tc := range cases {
+		t.Run(tn, func(t *testing.T) {
+
+			// Arrange
+			acctest.UnsetTestProviderConfigEnvs(t)
+			acctest.SetupTestEnvs(t, tc.EnvVariables)
+
+			ctx := context.Background()
+			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
+			data.ImpersonateServiceAccountDelegates = impersonateServiceAccountDelegates
+
+			p := fwtransport.FrameworkProviderConfig{}
+
+			// Act
+			p.LoadAndValidateFramework(ctx, &data, tfVersion, &diags, providerversion)
+
+			// Assert
+			if diags.HasError() && tc.ExpectError {
+				return
+			}
+			if diags.HasError() && !tc.ExpectError {
+				for i, err := range diags.Errors() {
+					num := i + 1
+					t.Logf("unexpected error #%d : %s", num, err.Summary())
+				}
+				t.Fatalf("did not expect error, but [%d] error(s) occurred", diags.ErrorsCount())
+			}
+			// Checking mutation of the data model
+			if !data.UserProjectOverride.Equal(tc.ExpectedDataModelValue) {
+				t.Fatalf("want user_project_override in the `fwmodels.ProviderModel` struct to be `%s`, but got the value `%s`", tc.ExpectedDataModelValue, data.UserProjectOverride.String())
+			}
+			// Checking the value passed to the config structs
+			if !p.UserProjectOverride.Equal(tc.ExpectedConfigStructValue) {
+				t.Fatalf("want user_project_override in the `FrameworkProviderConfig` struct to be `%s`, but got the value `%s`", tc.ExpectedConfigStructValue, p.UserProjectOverride.String())
+			}
+		})
+	}
+}
diff --git a/google/fwtransport/framework_transport.go b/google/fwtransport/framework_transport.go
@@ -25,7 +25,7 @@ func SendFrameworkRequestWithTimeout(p *FrameworkProviderConfig, method, project
 	reqHeaders.Set("User-Agent", userAgent)
 	reqHeaders.Set("Content-Type", "application/json")
 
-	if p.UserProjectOverride && project != "" {
+	if p.UserProjectOverride.ValueBool() && project != "" {
 		// When project is "NO_BILLING_PROJECT_OVERRIDE" in the function GetCurrentUserEmail,
 		// set the header X-Goog-User-Project to be empty string.
 		if project == "NO_BILLING_PROJECT_OVERRIDE" {
diff --git a/google/provider/provider_internal_test.go b/google/provider/provider_internal_test.go
@@ -1175,7 +1175,7 @@ func TestProvider_ProviderConfigure_userProjectOverride(t *testing.T) {
 			},
 			ExpectedValue: false,
 		},
-		"error returned due to non-boolean environment variables": {
+		"setting user_project_override using a non-boolean environment variables results in an error": {
 			EnvVariables: map[string]string{
 				"USER_PROJECT_OVERRIDE": "I'm not a boolean",
 			},

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	+```release-note:bug
	`2`	+provider: fixed a bug where `user_project_override` would not be not used correctly when provisioning resources implemented using the plugin framework. Currently there are no resources implemented this way, so no-one should have been impacted.
	`3`	+```