Skip to content

Conversation

raskchanky
Copy link
Contributor

Description

Here is the impetus for this change: https://hashicorp.atlassian.net/browse/VAULT-34426
Here is the Vault CE PR that is required in order for this change to work: hashicorp/vault#31346

tl;dr - once the "allowed response headers" field is changed from its initial empty value to something non-empty, you can't set it back to being empty again. This was a fundamental limitation of Vault's API and the CE PR updates that API to make that possible. This change updates the TFVP to take advantage of the new shape of that API.

@fairclothjm @vinay-gopalan The changes to go.mod and go.sum are because I added a replace directive in order to point this to my local vault checkout and go mod tidy was required to get that working. I can back out those updates if you'd prefer. It's not entirely clear to me if I need to do anything specific to either this PR or my Vault CE PR in order to make sure that this code uses the new code I wrote for Vault's API. Does that make sense?

Also technically this is a behavior change, despite the fact that we're really just fixing a bug. Is that bad? How does TFVP handle these sorts of things?

Relates OR Closes #0000

Checklist

  • Added CHANGELOG entry (only for user-facing changes)
  • Acceptance tests where run against all supported Vault Versions

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccXXX'

...

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

PCI review checklist

  • I have documented a clear reason for, and description of, the change I am making.

  • If applicable, I've documented a plan to revert these changes if they require more than reverting the pull request.

  • If applicable, I've documented the impact of any changes to security controls.

    Examples of changes to security controls include using new access control methods, adding or removing logging pipelines, etc.

@raskchanky raskchanky requested a review from a team as a code owner July 21, 2025 19:12
@raskchanky raskchanky requested a review from arnab-hashi July 21, 2025 19:12
@fairclothjm
Copy link
Contributor

@raskchanky Hey! Thanks for working on this! :)

It looks like there is a build error with this branch:

$ make dev
==> Checking that code complies with gofmt requirements...
go build -o terraform-provider-vault
# github.com/hashicorp/terraform-provider-vault/vault
vault/resource_mount.go:235:41: cannot use &s (value of type *[]string) as []string value in assignment
vault/resource_mount.go:332:35: cannot use headers (variable of type *[]string) as []string value in assignment
vault/structures.go:44:33: cannot use &s (value of type *[]string) as []string value in assignment
make: *** [dev] Error 1

The changes to go.mod and go.sum

I think we are ok to keep those as they are all minor and/or patch releases. We recently expanded the test and build matrix to include more Terraform versions so if you re-push then we should be able to be reasonably confident in these changes based on the build result.

It's not entirely clear to me if I need to do anything specific to either this PR or my Vault CE PR in order to make sure that this code uses the new code I wrote for Vault's API. Does that make sense?

I think we will need to tag Vault's API and then pull those changes into TFVP.

Also technically this is a behavior change, despite the fact that we're really just fixing a bug. Is that bad? How does TFVP handle these sorts of things?

Looking at the changes in Vault, I am not exactly sure what we want to do here. That change in Vault's API seems questionable. Is that a breaking change for consumers of the Vault API? I think we usually try to work around Vault's API in these cases as opposed to making a behavior change in Vault. That said, there likely is no workaround in this case. Alternatively, is it possible to make a new addition to Vault's API to enable this change in TFVP? That way we don't break existing consumers of the API?

Another thing to test/consider is, does the Vault server need to be on a specific version for this change to work. If yes, then we will need to call that out in the changelog and handle it gracefully so that we don't get unexpected behavior in the provider.

Hopefully that was helpful. Please let me know if you need more clarity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants