Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow multiple versions of provider in acceptance tests #628

Closed
dak1n1 opened this issue Oct 29, 2020 · 2 comments · Fixed by #972
Closed

Allow multiple versions of provider in acceptance tests #628

dak1n1 opened this issue Oct 29, 2020 · 2 comments · Fixed by #972
Assignees
Labels
enhancement New feature or request subsystem/tests Issues and feature requests related to the testing framework.
Milestone

Comments

@dak1n1
Copy link

dak1n1 commented Oct 29, 2020

SDK version

{
  "Path": "github.com/hashicorp/terraform-plugin-sdk/v2",
  "Version": "v2.0.4-0.20200930154456-951f045a9f14"
}

Use-cases

Simulate provider upgrade on existing infrastructure. Specifically, I would like the acceptance tests to tell me when one of my local changes might accidentally delete a user's existing infrastructure. (This can happen when ForceNew and Default are both used in the same schema attribute).

Here is a specific example:

A user has a storage volume created through Terraform. This is the existing infrastructure. They go to upgrade to the latest version of a Terraform Provider which manages this volume. One of the changes pulled in through that upgrade is a new attribute of that volume resource, and someone has accidentally added a ForceNew and Default option to that attribute.

When the user goes to terraform apply, Terraform adds the new attribute to the volume, but in doing so, it has deleted the user's volume and created a new one. This bug should be caught in acceptance tests.

Attempted Solutions

Working solution

I have this work-around that's working for the Kubernetes provider, but it would be a lot neater if the SDK had a built-in way to do this instead. Basically, what I've done is compiled the provider from the local branch and named it "kubernetes-local". And I've grabbed the latest released version and named it "kubernetes-released". Then I load both of these into the acceptance test as ExternalProviders.

In the acceptance test, I first create a resource using the released version, and then try a second apply using the pre-released version. Then, I compare the UUID of the resource to make sure it hasn't been deleted and recreated. Here is the code. It's spread across a few files:

https://github.com/hashicorp/terraform-provider-kubernetes/blob/0045c05d3cc24ed13e64f2974d63e6989ed0aa52/.terraformrc
https://github.com/hashicorp/terraform-provider-kubernetes/blob/0045c05d3cc24ed13e64f2974d63e6989ed0aa52/GNUmakefile#L46-L52

https://github.com/hashicorp/terraform-provider-kubernetes/blob/0045c05d3cc24ed13e64f2974d63e6989ed0aa52/kubernetes/provider_test.go#L38-L57

https://github.com/hashicorp/terraform-provider-kubernetes/blob/0045c05d3cc24ed13e64f2974d63e6989ed0aa52/kubernetes/provider_test.go#L453-L467

https://github.com/hashicorp/terraform-provider-kubernetes/blob/0045c05d3cc24ed13e64f2974d63e6989ed0aa52/kubernetes/resource_kubernetes_pod_test.go#L970

https://github.com/hashicorp/terraform-provider-kubernetes/blob/0045c05d3cc24ed13e64f2974d63e6989ed0aa52/kubernetes/resource_kubernetes_pod_test.go#L974

https://github.com/hashicorp/terraform-provider-kubernetes/blob/0045c05d3cc24ed13e64f2974d63e6989ed0aa52/kubernetes/resource_kubernetes_pod_test.go#L1012

https://github.com/hashicorp/terraform-provider-kubernetes/blob/0045c05d3cc24ed13e64f2974d63e6989ed0aa52/kubernetes/resource_kubernetes_pod_test.go#L1047

What didn't work

I tried getting the same behavior as above, without pre-compiling the provider, but it was unable to effectively test the two versions. Here's what I tried:

First, using the code from the links above, I confirmed that my work-around does indeed catch a ForceNew. I did this by adding a Default value to a schema attribute, along with a ForceNew. This simulates the bug that would have destroyed a user's infrastructure.

kubernetes/resource_kubernetes_pod.go
+       podSpecFields["dns_policy"].Default = "Default"
+       podSpecFields["dns_policy"].ForceNew = true

 TESTARGS="-run 'TestAccKubernetesPod_regression'" make testacc

The result confirms that the work-around is able to catch the ForceNew. It errors as expected when the resource is re-created:

=== RUN   TestAccKubernetesPod_regression
    resource_kubernetes_pod_test.go:968: Step 2/2 error: Check failed: 2 errors occurred:
                * Check 11/34 error: kubernetes_pod.test: Attribute 'spec.0.dns_policy' expected "ClusterFirst", got "Default"
                * Check 34/34 error: Expecting pod UIDs to be the same: expected 17675004-ae7d-4d07-8713-bf530ed49a6b got bcdea13f-0c9c-4568-b7bd-1a2ab166c719
--- FAIL: TestAccKubernetesPod_regression (61.76s)
FAIL

For the second test, I removed the "kubernetes-local" provider and attempted to use the regular ProviderFactories instead.

kubernetes/resource_kubernetes_pod_test.go
@@ -968,6 +968,7 @@ func TestAccKubernetesPod_regression(t *testing.T) {                                                                                
        resource.Test(t, resource.TestCase{                                                                                                             
                PreCheck:          func() { testAccPreCheck(t) },                                                                                       
                ExternalProviders: testAccExternalProviders,
+               ProviderFactories: testAccProviderFactories,

-                               Config: requiredProviders() + testAccKubernetesPod_regression("kubernetes-local", name, imageName),
+                               Config: requiredProviders() + testAccKubernetesPod_regression("kubernetes", name, imageName),

However, the results were not as expected. The test did not catch the ForceNew. I'm not sure why. The full diff is here which shows all the code that was modified during this test.

=== RUN   TestAccKubernetesPod_regression
    resource_kubernetes_pod_test.go:968: Step 1/2 error: Check failed: 1 error occurred:
                * Check 11/33 error: kubernetes_pod.test: Attribute 'spec.0.dns_policy' expected "ClusterFirst", got "Default"
        
--- FAIL: TestAccKubernetesPod_regression (23.22s)

So this is why I think the SDK does not yet support my use case.

Proposal

If we could use ProviderFactories together with ExternalProviders, we could test multiple versions of the provider. It might just need to run terraform init in between TestSteps. Alternatively, any solution which allows me to simulate a provider upgrade would be really appreciated!

References

@dak1n1 dak1n1 added the enhancement New feature or request label Oct 29, 2020
@paddycarver paddycarver added the subsystem/tests Issues and feature requests related to the testing framework. label Dec 17, 2020
@bflad bflad self-assigned this May 26, 2022
@bflad bflad added this to the v2.17.0 milestone May 26, 2022
bflad added a commit that referenced this issue May 26, 2022
Reference: #253
Reference: #628
Reference: #779

Provider developers can now select whether to configure providers for acceptance testing at the `TestCase` or `TestStep` level. Only one level may be used in this current implementation, however it may be possible to allow merged `TestCase` and `TestStep` configuration with additional validation logic to ensure a single provider is not specified multiple times across the merge result of all fields.

This change also introduces some upfront `TestCase` and `TestStep` configuration validation when calling any of the `Test` functions, failing the test early if a problem is detected. There are other validations that are possible, however these are considered out of scope.
bflad added a commit that referenced this issue May 31, 2022
Reference: #253
Reference: #628
Reference: #779

Provider developers can now select whether to configure providers for acceptance testing at the `TestCase` or `TestStep` level. Only one level may be used in this current implementation, however it may be possible to allow merged `TestCase` and `TestStep` configuration with additional validation logic to ensure a single provider is not specified multiple times across the merge result of all fields.

This change also introduces some upfront `TestCase` and `TestStep` configuration validation when calling any of the `Test` functions, failing the test early if a problem is detected. There are other validations that are possible, however these are considered out of scope.
@bflad
Copy link
Contributor

bflad commented May 31, 2022

The acceptance testing framework (helper/resource) in v2.17.0 now allows provider developers to specify providers at the TestStep level rather than just TestCase. This means that real world state migration/upgrade testing can now be implemented acceptance tests. e.g.

resource.Test(t, resource.TestCase{
	Steps: []resource.TestStep{
		{
			ExternalProviders: map[string]resource.ExternalProvider{
				"random": {
					VersionConstraint: "...", // last version of old schema version
					Source:            "example-namespace/example",
				},
			},
			Config: "...",
			Check:  /* ... */,
		},
		{
			ProviderFactories: testAccProviders,
			Config:            "...",
			Check:             /* ... */,
		},
	},
})

@github-actions
Copy link

github-actions bot commented Jul 1, 2022

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 github-actions bot locked as resolved and limited conversation to collaborators Jul 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request subsystem/tests Issues and feature requests related to the testing framework.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants