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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[VAULT-3157] Add X-Vault-Index header functionality #1170

Closed
wants to merge 5 commits into from

Conversation

vinay-gopalan
Copy link
Contributor

@vinay-gopalan vinay-gopalan commented Sep 14, 2021

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

Relates OR Closes #0000
https://hashicorp.atlassian.net/browse/VAULT-3157

Release note for CHANGELOG:

Add X-Vault-Index header to ensure that the state resulting from a write request that modifies storage is visible to a subsequent request.

vault/provider.go Outdated Show resolved Hide resolved
@vinay-gopalan vinay-gopalan marked this pull request as ready for review October 4, 2021 22:16
@@ -705,6 +706,16 @@ func providerToken(d *schema.ResourceData) (string, error) {
return strings.TrimSpace(token), nil
}

func RecordMultipleStates(lastStates ...string) func(*api.Response) {
return func(resp *api.Response) {
var m sync.Mutex
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to move this variable declaration outside the anonymous function, otherwise all invocations of the function will have their own mutex variable and not actually synchronise.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call! Updated and moved the declaration outside the anonymous function but still under RecordMultipleStates

var m sync.Mutex
newState := resp.Header.Get("X-Vault-Index")
m.Lock()
lastStates = api.MergeStates(lastStates, newState)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this will actually update the lastStates slice in providerConfigure's scope. Maybe worth adding a test to verify either way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, lastState will not have the updated value outside of this func (RecordState accepts a string pointer which is why it works there).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@calvn is there a way to make this work with a string slice? Or are string pointers our best bet here? (can we pass arrays by pointers in Go?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though this is not common, we could have as the argument type *[]string and we pass in the pointer to the slice for lastStates whenever this is called.

@@ -856,12 +871,19 @@ func providerConfigure(d *schema.ResourceData) (interface{}, error) {
// Set the token to the generated child token
client.SetToken(childToken)

client = client.WithRequestCallbacks(api.RequireState(lastStates...)).WithResponseCallbacks()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this chained like so in there? If this is necessary, can you leave a comment around this line?

@vinay-gopalan
Copy link
Contributor Author

Closing this branch because this work is incorporated in this PR that adds client control support in the form of a ClientFactory: #1188

@vinay-gopalan vinay-gopalan deleted the fix-vault-index-header branch October 7, 2021 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants