Refactor muxing 1 : Re-use same config to configure the SDK and PF providers, fix VCR testing

.changelog/11903.txt

@@ -0,0 +1,3 @@
+```release-note:note
+provider: refactored how the provider configuration is handled internally
+```

google/acctest/provider_test_utils.go

@@ -22,7 +22,6 @@ var testAccProvider *schema.Provider
 
 func init() {
 	configs = make(map[string]*transport_tpg.Config)
-	fwProviders = make(map[string]*frameworkTestProvider)
 	sources = make(map[string]VcrSource)
 	testAccProvider = provider.Provider()
 	TestAccProviders = map[string]*schema.Provider{

google/acctest/test_utils.go

@@ -82,9 +82,13 @@ func CheckDataSourceStateMatchesResourceStateWithIgnores(dataSourceName, resourc
 func MuxedProviders(testName string) (func() tfprotov5.ProviderServer, error) {
 	ctx := context.Background()
 
+	// primary is the SDKv2 implementation of the provider
+	// If tests are run in VCR mode, the provider will use a cached config specific to the test name
+	primary := GetSDKProvider(testName)
+
 	providers := []func() tfprotov5.ProviderServer{
-		providerserver.NewProtocol5(NewFrameworkTestProvider(testName)), // framework provider
-		GetSDKProvider(testName).GRPCProvider,                           // sdk provider
+		primary.GRPCProvider, // sdk provider
+		providerserver.NewProtocol5(NewFrameworkTestProvider(testName, primary)), // framework provider
 	}
 
 	muxServer, err := tf5muxserver.NewMuxServer(ctx, providers...)

google/acctest/vcr_utils.go

@@ -23,7 +23,6 @@ import (
 	"testing"
 	"time"
 
-	"github.com/hashicorp/terraform-provider-google/google/fwmodels"
 	"github.com/hashicorp/terraform-provider-google/google/fwprovider"
 	tpgprovider "github.com/hashicorp/terraform-provider-google/google/provider"
 	"github.com/hashicorp/terraform-provider-google/google/tpgresource"
@@ -32,12 +31,9 @@ import (
 	"github.com/dnaeon/go-vcr/cassette"
 	"github.com/dnaeon/go-vcr/recorder"
 
-	"github.com/hashicorp/terraform-plugin-framework/attr"
 	"github.com/hashicorp/terraform-plugin-framework/datasource"
-
 	fwDiags "github.com/hashicorp/terraform-plugin-framework/diag"
 	"github.com/hashicorp/terraform-plugin-framework/provider"
-	"github.com/hashicorp/terraform-plugin-framework/types"
 	"github.com/hashicorp/terraform-plugin-go/tfprotov5"
 	"github.com/hashicorp/terraform-plugin-log/tflog"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
@@ -55,7 +51,6 @@ var configsLock = sync.RWMutex{}
 var sourcesLock = sync.RWMutex{}
 
 var configs map[string]*transport_tpg.Config
-var fwProviders map[string]*frameworkTestProvider
 
 var sources map[string]VcrSource
 
@@ -205,39 +200,6 @@ func closeRecorder(t *testing.T) {
 		delete(sources, t.Name())
 		sourcesLock.Unlock()
 	}
-
-	configsLock.RLock()
-	fwProvider, fwOk := fwProviders[t.Name()]
-	configsLock.RUnlock()
-	if fwOk {
-		// We did not cache the config if it does not use VCR
-		if !t.Failed() && IsVcrEnabled() {
-			// If a test succeeds, write new seed/yaml to files
-			err := fwProvider.Client.Transport.(*recorder.Recorder).Stop()
-			if err != nil {
-				t.Error(err)
-			}
-			envPath := os.Getenv("VCR_PATH")
-
-			sourcesLock.RLock()
-			vcrSource, ok := sources[t.Name()]
-			sourcesLock.RUnlock()
-			if ok {
-				err = writeSeedToFile(vcrSource.seed, vcrSeedFile(envPath, t.Name()))
-				if err != nil {
-					t.Error(err)
-				}
-			}
-		}
-		// Clean up test config
-		configsLock.Lock()
-		delete(fwProviders, t.Name())
-		configsLock.Unlock()
-
-		sourcesLock.Lock()
-		delete(sources, t.Name())
-		sourcesLock.Unlock()
-	}
 }
 
 func isReleaseDiffEnabled() bool {
@@ -321,6 +283,11 @@ func reformConfigWithProvider(config, provider string) string {
 	return string(resourceHeader.ReplaceAll(configBytes, providerReplacementBytes))
 }
 
+// HandleVCRConfiguration configures the recorder (github.com/dnaeon/go-vcr/recorder) used in the VCR test
+// This includes:
+//   - Setting the recording/replaying mode
+//   - Determining the path to the file API interactions will be recorded to/read from
+//   - Determining the logic used to match requests against recorded HTTP interactions (see rec.SetMatcher)
 func HandleVCRConfiguration(ctx context.Context, testName string, rndTripper http.RoundTripper, pollInterval time.Duration) (time.Duration, http.RoundTripper, fwDiags.Diagnostics) {
 	var diags fwDiags.Diagnostics
 	var vcrMode recorder.Mode
@@ -404,45 +371,32 @@ func NewVcrMatcherFunc(ctx context.Context) func(r *http.Request, i cassette.Req
 // test versions of the provider that will call the same configure function, only append the VCR
 // configuration to it.
 
-func NewFrameworkTestProvider(testName string) *frameworkTestProvider {
+func NewFrameworkTestProvider(testName string, primary *schema.Provider) *frameworkTestProvider {
 	return &frameworkTestProvider{
 		FrameworkProvider: fwprovider.FrameworkProvider{
 			Version: "test",
+			Primary: primary,
 		},
 		TestName: testName,
 	}
 }
 
-// frameworkTestProvider is a test version of the plugin-framework version of the provider
-// that embeds FrameworkProvider whose configure function we can use
-// the Configure function is overwritten in the framework_provider_test file
+// frameworkTestProvider is a test version of the plugin-framework version of the provider.
+// This facilitates overwriting the Configure function that's used during acceptance tests.
 type frameworkTestProvider struct {
 	fwprovider.FrameworkProvider
 	TestName string
 }
 
 // Configure is here to overwrite the FrameworkProvider configure function for VCR testing
 func (p *frameworkTestProvider) Configure(ctx context.Context, req provider.ConfigureRequest, resp *provider.ConfigureResponse) {
-	p.FrameworkProvider.Configure(ctx, req, resp)
-	if IsVcrEnabled() {
-		if resp.Diagnostics.HasError() {
-			return
-		}
-
-		var diags fwDiags.Diagnostics
-		p.PollInterval, p.Client.Transport, diags = HandleVCRConfiguration(ctx, p.TestName, p.Client.Transport, p.PollInterval)
-		if diags.HasError() {
-			resp.Diagnostics.Append(diags...)
-			return
-		}
 
-		configsLock.Lock()
-		fwProviders[p.TestName] = p
-		configsLock.Unlock()
-		return
-	} else {
-		tflog.Debug(ctx, "VCR_PATH or VCR_MODE not set, skipping VCR")
-	}
+	// When creating the frameworkTestProvider struct we took in a pointer to the the SDK provider.
+	// That SDK provider was configured using `GetSDKProvider` and `getCachedConfig`, so this framework provider will also
+	// use a cached client for the correct test name.
+	// No extra logic is required here, but in future when the SDK provider is removed this function will
+	// need to be updated with logic similar to that in `GetSDKProvider`.
+	p.FrameworkProvider.Configure(ctx, req, resp)
 }
 
 // DataSources overrides the provider's DataSources function so that we can append test-specific data sources to the list of data sources on the provider.
@@ -453,21 +407,9 @@ func (p *frameworkTestProvider) DataSources(ctx context.Context) []func() dataso
 	return ds
 }
 
-func configureApiClient(ctx context.Context, p *fwprovider.FrameworkProvider, diags *fwDiags.Diagnostics) {
-	var data fwmodels.ProviderModel
-	var d fwDiags.Diagnostics
-
-	// Set defaults if needed - the only attribute without a default is ImpersonateServiceAccountDelegates
-	// this is a bit of a hack, but we'll just initialize it here so that it's been initialized at least
-	data.ImpersonateServiceAccountDelegates, d = types.ListValue(types.StringType, []attr.Value{})
-	diags.Append(d...)
-	if diags.HasError() {
-		return
-	}
-	p.LoadAndValidateFramework(ctx, &data, "test", diags, p.Version)
-}
-
-// GetSDKProvider gets the SDK provider with an overwritten configure function to be called by MuxedProviders
+// GetSDKProvider gets the SDK provider for use in acceptance tests
+// If VCR is in use, the configure function is overwritten.
+// See usage in MuxedProviders
 func GetSDKProvider(testName string) *schema.Provider {
 	prov := tpgprovider.Provider()
 

google/fwprovider/data_source_provider_config_plugin_framework.go

@@ -6,13 +6,14 @@ import (
 	"context"
 	"fmt"
 
+	"github.com/hashicorp/terraform-plugin-framework/attr"
 	"github.com/hashicorp/terraform-plugin-framework/datasource"
 	"github.com/hashicorp/terraform-plugin-framework/datasource/schema"
 	"github.com/hashicorp/terraform-plugin-framework/diag"
 	"github.com/hashicorp/terraform-plugin-framework/types"
 	"github.com/hashicorp/terraform-provider-google/google/fwmodels"
 	"github.com/hashicorp/terraform-provider-google/google/fwresource"
-	"github.com/hashicorp/terraform-provider-google/google/fwtransport"
+	transport_tpg "github.com/hashicorp/terraform-provider-google/google/transport"
 )
 
 // Ensure the data source satisfies the expected interfaces.
@@ -27,15 +28,17 @@ func NewGoogleProviderConfigPluginFrameworkDataSource() datasource.DataSource {
 }
 
 type GoogleProviderConfigPluginFrameworkDataSource struct {
-	providerConfig *fwtransport.FrameworkProviderConfig
+	providerConfig *transport_tpg.Config
 }
 
+// GoogleProviderConfigPluginFrameworkModel describes the data source and matches the schema. Its fields match those in a Config struct (google/transport/config.go) but uses a different type system.
+//   - In the original Config struct old SDK/Go primitives types are used, e.g. `string`
+//   - Here in the GoogleProviderConfigPluginFrameworkModel struct we need to use  the terraform-plugin-framework/types type system, e.g. `types.String`
+//   - This is needed because the PF type system is 'baked into' how we define schemas. The schema will expect a nullable type.
+//   - See terraform-plugin-framework/datasource/schema#StringAttribute's CustomType: https://pkg.go.dev/github.com/hashicorp/[email protected]/datasource/schema#StringAttribute
+//   - Due to the different type systems of Config versus GoogleProviderConfigPluginFrameworkModel struct, we need to convert from primitive types to terraform-plugin-framework/types when we populate
+//     GoogleProviderConfigPluginFrameworkModel structs with data in this data source's Read method.
 type GoogleProviderConfigPluginFrameworkModel struct {
-	// Currently this reflects the FrameworkProviderConfig struct and ProviderModel in google/fwmodels/provider_model.go
-	// which means it uses the plugin-framework type system where values can be explicitly Null or Unknown.
-	//
-	// As part of future muxing fixes/refactoring we'll change this struct to reflect structs used in the SDK code, and will move to
-	// using the SDK type system.
 	Credentials                        types.String `tfsdk:"credentials"`
 	AccessToken                        types.String `tfsdk:"access_token"`
 	ImpersonateServiceAccount          types.String `tfsdk:"impersonate_service_account"`
@@ -55,12 +58,12 @@ type GoogleProviderConfigPluginFrameworkModel struct {
 	TerraformAttributionLabelAdditionStrategy types.String `tfsdk:"terraform_attribution_label_addition_strategy"`
 }
 
-func (m *GoogleProviderConfigPluginFrameworkModel) GetLocationDescription(providerConfig *fwtransport.FrameworkProviderConfig) fwresource.LocationDescription {
+func (m *GoogleProviderConfigPluginFrameworkModel) GetLocationDescription(providerConfig *transport_tpg.Config) fwresource.LocationDescription {
 	return fwresource.LocationDescription{
 		RegionSchemaField: types.StringValue("region"),
 		ZoneSchemaField:   types.StringValue("zone"),
-		ProviderRegion:    providerConfig.Region,
-		ProviderZone:      providerConfig.Zone,
+		ProviderRegion:    types.StringValue(providerConfig.Region),
+		ProviderZone:      types.StringValue(providerConfig.Zone),
 	}
 }
 
@@ -174,11 +177,11 @@ func (d *GoogleProviderConfigPluginFrameworkDataSource) Configure(ctx context.Co
 		return
 	}
 
-	p, ok := req.ProviderData.(*fwtransport.FrameworkProviderConfig)
+	p, ok := req.ProviderData.(*transport_tpg.Config)
 	if !ok {
 		resp.Diagnostics.AddError(
 			"Unexpected Data Source Configure Type",
-			fmt.Sprintf("Expected *fwtransport.FrameworkProviderConfig, got: %T. Please report this issue to the provider developers.", req.ProviderData),
+			fmt.Sprintf("Expected *transport_tpg.Config, got: %T. Please report this issue to the provider developers.", req.ProviderData),
 		)
 		return
 	}
@@ -204,23 +207,54 @@ func (d *GoogleProviderConfigPluginFrameworkDataSource) Read(ctx context.Context
 	}
 
 	// Copy all values from the provider config into this data source
+	//    - The 'meta' from the provider configuration process uses Go primitive types (e.g. `string`) but this data source needs to use the plugin-framework type system due to being PF-implemented
+	//    - As a result we have to make conversions between type systems in the value assignments below
+	data.Credentials = types.StringValue(d.providerConfig.Credentials)
+	data.AccessToken = types.StringValue(d.providerConfig.AccessToken)
+	data.ImpersonateServiceAccount = types.StringValue(d.providerConfig.ImpersonateServiceAccount)
 
-	data.Credentials = d.providerConfig.Credentials
-	data.AccessToken = d.providerConfig.AccessToken
-	data.ImpersonateServiceAccount = d.providerConfig.ImpersonateServiceAccount
-	data.ImpersonateServiceAccountDelegates = d.providerConfig.ImpersonateServiceAccountDelegates
-	data.Project = d.providerConfig.Project
-	data.Region = d.providerConfig.Region
-	data.BillingProject = d.providerConfig.BillingProject
-	data.Zone = d.providerConfig.Zone
-	data.UniverseDomain = d.providerConfig.UniverseDomain
-	data.Scopes = d.providerConfig.Scopes
-	data.UserProjectOverride = d.providerConfig.UserProjectOverride
-	data.RequestReason = d.providerConfig.RequestReason
-	data.RequestTimeout = d.providerConfig.RequestTimeout
-	data.DefaultLabels = d.providerConfig.DefaultLabels
-	data.AddTerraformAttributionLabel = d.providerConfig.AddTerraformAttributionLabel
-	data.TerraformAttributionLabelAdditionStrategy = d.providerConfig.TerraformAttributionLabelAdditionStrategy
+	delegateAttrs := make([]attr.Value, len(d.providerConfig.ImpersonateServiceAccountDelegates))
+	for i, delegate := range d.providerConfig.ImpersonateServiceAccountDelegates {
+		delegateAttrs[i] = types.StringValue(delegate)
+	}
+	delegates, di := types.ListValue(types.StringType, delegateAttrs)
+	if di.HasError() {
+		resp.Diagnostics.Append(di...)
+	}
+	data.ImpersonateServiceAccountDelegates = delegates
+
+	data.Project = types.StringValue(d.providerConfig.Project)
+	data.Region = types.StringValue(d.providerConfig.Region)
+	data.BillingProject = types.StringValue(d.providerConfig.BillingProject)
+	data.Zone = types.StringValue(d.providerConfig.Zone)
+	data.UniverseDomain = types.StringValue(d.providerConfig.UniverseDomain)
+
+	scopeAttrs := make([]attr.Value, len(d.providerConfig.Scopes))
+	for i, scope := range d.providerConfig.Scopes {
+		scopeAttrs[i] = types.StringValue(scope)
+	}
+	scopes, di := types.ListValue(types.StringType, scopeAttrs)
+	if di.HasError() {
+		resp.Diagnostics.Append(di...)
+	}
+	data.Scopes = scopes
+
+	data.UserProjectOverride = types.BoolValue(d.providerConfig.UserProjectOverride)
+	data.RequestReason = types.StringValue(d.providerConfig.RequestReason)
+	data.RequestTimeout = types.StringValue(d.providerConfig.RequestTimeout.String())
+
+	lbs := make(map[string]attr.Value, len(d.providerConfig.DefaultLabels))
+	for k, v := range d.providerConfig.DefaultLabels {
+		lbs[k] = types.StringValue(v)
+	}
+	labels, di := types.MapValueFrom(ctx, types.StringType, lbs)
+	if di.HasError() {
+		resp.Diagnostics.Append(di...)
+	}
+	data.DefaultLabels = labels
+
+	data.AddTerraformAttributionLabel = types.BoolValue(d.providerConfig.AddTerraformAttributionLabel)
+	data.TerraformAttributionLabelAdditionStrategy = types.StringValue(d.providerConfig.TerraformAttributionLabelAdditionStrategy)
 
 	// Warn users against using this data source
 	resp.Diagnostics.Append(diag.NewWarningDiagnostic(