Add support to perform semantic version comparisons against Vault's server version

[vinay-gopalan]

This PR adds a mechanism that performs semantic version comparison against Vault’s target version to determine whether certain features are available in the current Vault client. This mechanism allows the TFVP to have knowledge of Vault's version, supported paths and version-specific schemas, allowing us to further improve upon the codebase's overall architecture.

In this PR, we demonstrate how the semantic version comparison mechanism can be used to determine whether Auth and Secret Engines can be remounted. Mount Migration is a newly available feature as of Vault 1.10.0, and hence this feature is only enabled for clients with the required version or above.

This PR also includes wrapper functions that can be used with new resources to determine whether said resources are available within the current version of Vault. If the user attempts to create a resource using this wrapper function and it does not meet the necessary version requirements, the resource shall fail to create.

Features that this PR adds semantic version comparison to:

• Kubernetes Secret Backend resource (1.11)
• Kubernetes Secret Backend Role resource (1.11)
• Managed Keys resource (1.10)
• Mount migration for AWS (1.10)

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSSecretBackend_remount'
=== RUN   TestAccAWSSecretBackend_remount
--- PASS: TestAccAWSSecretBackend_remount (2.90s)
PASS

=== RUN   TestAccKubernetesSecretBackend
--- PASS: TestAccKubernetesSecretBackend (4.38s)
PASS

=== RUN   TestAccKubernetesSecretBackendRole
--- PASS: TestAccKubernetesSecretBackendRole (4.52s)
PASS

=== RUN   TestManagedKeys
--- PASS: TestManagedKeys (4.92s)
PASS

[swenson]

Awesome! This is super useful.

[robmonte]

I was looking in the testing framework to see if there was anything to support checking whether a resource is being destroyed or not between test steps, but I didn't find anything. Do you know if there might be a way to accomplish that? It may be a useful ability to have going forward as we utilize the feature.

[vinay-gopalan]

Good point. I've been meaning to add this check in the Managed keys tests as well, but I think one way would be if Vault has a UUID for the resource then we can check if the UUIDs are different after re-creation. Other than that, I haven't come up with a better solution as of yet unfortunately

[vinay-gopalan]

For this test, I had to check my Vault logs to confirm that a mount migration had occurred successfully

[tvoran]

There is a CheckDestroy field in resource.TestCase that I've seen used elsewhere, though I think that's after the test and not between steps.

You may be able to use TestCheckNoResourceAttr to check that the old mount doesn't exist in the state at least?

[vinay-gopalan]

You are correct! CheckDestroy runs after the test completes, so unfortunately we can't use that in this case

I looked into trying to make TestCheckNoResourceAttr work. The function checks whether certain state keys exist in the TF state for a particular resource. In this case, while the mount does change (expressed in this resource as a path change), the path key isn't being deleted or removed, so it still exists in the TF state (albeit with a different value). In this case unfortunately, I don't think that would work exactly the way we want it to

I'll definitely keep looking into this as an improvement to our testing since this is also needed in a few other places!

In the future I think we also plan to refactor all secret and auth engines be mounted solely via the vault_mount resource (currently some resources mount the engine as well as the config; including the resource we are talking about: https://github.com/hashicorp/terraform-provider-vault/blob/main/vault/resource_aws_secret_backend.go#L119). This might help check the TF state for deleted mounts in a cleaner way.

[fairclothjm]

👍

[benashz]

I wonder if we should be doing the initial version check in provider.NewProviderMeta instead. This would allow the provider to abort early if it encounters an error getting the Vault version. It would also make it easier to use in the resource code since we would no longer have to handle the error. We would store the version.Version in the ProviderMeta so there would be no need to return the vault version string.

If we make minVersionString a version.Version the caller would be responsible for handling any parse errors.

[vinay-gopalan]

@benashz I'm glad you brought this up! I initially thought of this approach as well, and the main drawback that came to my mind (which I remember we briefly chatted about) was in the case where users use the same TF instance to configure multiple versions of Vault. In that case, the approach of storing the current Vault version to the ProviderMeta would explicitly disallow the use of the TFVP in those kind of scenarios, and I wanted to get your take on the larger implications/architecture before going down that route. I do agree that the current implementation adds an overhead of multiple calls to the same endpoint

[benashz]

I think we are okay here since each instance of the provider corresponds to a single Vault server instance.

[benashz]

By instance of the provider, I am referring to each provider{} block defined; terraform will spawn a separate process for each block defined.

[benashz]

Suggest making these version.Version variables instead, then there is no need to reparse them for every API request.

[benashz]

It looks like we will be calling the API for every Version check, this seems like a unnecessary increase in the overall the number of requests per operation. It is very unlikely that the vault server version will change during a run of Terraform.