terraform-plugin-sdk icon indicating copy to clipboard operation
terraform-plugin-sdk copied to clipboard

SDK v2.23.0 provider merging causes duplicate provider test failures

Open dghubble opened this issue 3 years ago • 4 comments

SDK version

Upgrading from v2.22.0 to v2.23.0 shows the breakage. See https://github.com/poseidon/terraform-provider-matchbox/pull/114

Relevant provider source code

Here's an example https://github.com/poseidon/terraform-provider-matchbox/pull/114

The Providers field is set (required) and an HCL provider block is defined as part of the config. https://github.com/poseidon/terraform-provider-matchbox/blob/main/matchbox/resource_group_test.go#L45

Providers: testProviders,                   <--- provider
Steps: []resource.TestStep{
 {
  Config: srv.AddProviderConfig(groupWithAllFields),    <--- provider HCL
  Check: resource.ComposeAggregateTestCheckFunc(
...

The provider block is added to the config by AddProviderConfig to configure the provider with neccessary credentials for testing. Overall, I think its a pretty typical situation.

What changed is that SDK v2.23.0 added merging logic for providers in https://github.com/hashicorp/terraform-plugin-sdk/pull/1057 (file helper/resource/teststep_providers.go) https://github.com/hashicorp/terraform-plugin-sdk/blob/main/helper/resource/teststep_providers.go#L16 that introduces duplication.

I've been able to checkout sdk v2.23.0, add a replace directive to go.mod, show it breaks my provider, revert https://github.com/hashicorp/terraform-plugin-sdk/pull/1057, and show the problem is resolved.

Terraform Configuration Files

These details aren't important for repro, but I'll include just since the issue template asks.

provider "matchbox" {
  endpoint = "some-endpoint"
  client_cert = <<CERT
some cert
CERT
  client_key = <<KEY
some key
KEY
  ca         = <<CA
some ca
CA
}

resource "matchbox_profile" "default" {
  name   = "default"
  kernel = "foo"
}

Debug Output

Expected Behavior

Tests should continue working on v2.23.0 as they did on v2.22.0

Actual Behavior

SDK tests always see this as duplicated providers (although the SDK is the one adding the duplicate).

Error: Duplicate provider configuration
        
          on terraform_plugin_test.tf line 3:
           3: 		provider "matchbox" {
        
        A default (non-aliased) provider configuration for "matchbox" was already
        given at terraform_plugin_test.tf:1,1-20. If multiple configurations are
        required, set the "alias" argument for alternative configurations.

Steps to Reproduce

https://github.com/poseidon/terraform-provider-matchbox/pull/114 is a great example. Rollback to v2.22.0 and it works again.

References

https://github.com/hashicorp/terraform-plugin-sdk/pull/1057 @bflad

dghubble avatar Sep 20 '22 17:09 dghubble

Hi @dghubble 👋 Thank you for reporting this and sorry the upgrade gave you unexpected issues.

Just to confirm things, if you run your testing with the TF_LOG environment variable set to TRACE and search for Setting Terraform configuration right before the testing errors out, does it show a configuration with multiple provider "matchbox" blocks? The logic that's adding the terraform/provider configuration blocks should only take affect for TestCase or TestStep that use the ExternalProviders field, but ... okay I found the code issue:

https://github.com/hashicorp/terraform-plugin-sdk/blob/fa9858369d6c11bc9d7df53470263be774c9e606/helper/resource/testcase_providers.go#L15-L21

Apparently TestCase Providers field is treated special. 😖 My apologies for missing that. The configuration merging logic will need to account for that by skipping the providers blocks if provider is already found in the configuration.

bflad avatar Sep 20 '22 17:09 bflad

It might be best if we just unilaterally skipped the provider source merge if the terraform or any provider configuration blocks are found in the given configurations.

bflad avatar Sep 20 '22 17:09 bflad

Another workaround in your case without any changes in the SDK would be switching from the deprecated Providers field to the ProviderFactories field. That should skip the duplicate provider ... being written into the merged configuration.

bflad avatar Sep 20 '22 17:09 bflad

Just to confirm things...

It doesn't show the exact configuration, but there seem to be two "matchbox" providers. That may be related to the original problem you were solving with hashicorp vs non-hashicorp.

[TRACE] sdk.helper_resource: Setting Terraform CLI reattach configuration: test_step_number=1 test_terraform_path=/usr/local/bin/terraform test_working_directory=/tmp/plugintest1199335445 tf_reattach_config="map[registry.terraform.io/-/matchbox:{grpc 5 557795 true {unix /tmp/plugin1472369595}} registry.terraform.io/hashicorp/matchbox:{grpc 5 557795 true {unix /tmp/plugin1472369595}}]" test_name=TestResourceProfile_Read

Another workaround in your case without any changes in the SDK would be switching from the deprecated Providers field to the ProviderFactories field

Interesting. Yeah, I can change to ProviderFactories and it doesn't cause the problem anymore. Nice

dghubble avatar Sep 21 '22 01:09 dghubble

@bflad we are having same issue:

Step 1/2 error: Error running pre-apply refresh: exit status 1
        
        Error: Duplicate provider configuration
        
          on terraform_plugin_test.tf line 14:
          14: provider "aws" {
        
        A default (non-aliased) provider configuration for "aws" was already given at
        terraform_plugin_test.tf:11,1-[15](https://github.com/castai/terraform-provider-castai/actions/runs/3226033532/jobs/5279063009#step:8:16). If multiple configurations are required,
        set the "alias" argument for alternative configurations.

we are using external providers

ExternalProviders: map[string]resource.ExternalProvider{
			"aws": {
				Source:            "hashicorp/aws",
				VersionConstraint: "~> 4.0",
			},
		},

and

provider "aws" {
  region = "eu-central-1"
}

switching to v2.22.0 works as expected

varnastadeus avatar Oct 11 '22 10:10 varnastadeus

Submitted #1092, which should prevent the testing framework from adding its own provider configuration blocks if it detects a TestStep.Config that already has them. 👍

bflad avatar Nov 08 '22 16:11 bflad

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

github-actions[bot] avatar Dec 15 '22 02:12 github-actions[bot]