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

Add context-aware functions to vault/api #14388

Merged
merged 28 commits into from
Mar 23, 2022

Conversation

averche
Copy link
Contributor

@averche averche commented Mar 7, 2022

Credits

This pull request is based on a community contribution #14152 by ilyakaznacheev and resolves #14070

Description

For each API wrapper function in vault/api, we are adding a context-aware equivalent. These new functions will have WithContext appended to their names.

Implementation Notes

One of the issues that I discovered in the original PR (#14152) is a potential context leak due to this piece of code inside RawRequestWithContext:

	if timeout != 0 {
		// Note: we purposefully do not call cancel manually. The reason is
		// when canceled, the request.Body will EOF when reading due to the way
		// it streams data in. Cancel will still be run when the timeout is
		// hit, so this doesn't really harm anything.
		ctx, _ = context.WithTimeout(ctx, timeout)
	}

To prevent context leaks, the timeout logic has been extracted out of RawRequestWithContext and into each ...WithContext wrapper (using withConfiguredTimeout helper). Additionally, since RawRequestWithContext is still used outside of vautl/api, its implementation has been moved into unexported rawRequestWithContext

See this comment for more details.

Testing

All existing tests that were calling vault/api functions have been switched to call WithContext equivalents with context.Background() passed in. Since WithContext functions invoke the regular ones, this change should increase code coverage without any negative side effects.

@vercel vercel bot temporarily deployed to Preview – vault March 7, 2022 17:19 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook March 7, 2022 17:22 Inactive
@vercel vercel bot temporarily deployed to Preview – vault March 7, 2022 17:36 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook March 7, 2022 17:36 Inactive
@vercel vercel bot temporarily deployed to Preview – vault March 7, 2022 17:40 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook March 7, 2022 17:40 Inactive
@vercel vercel bot temporarily deployed to Preview – vault March 7, 2022 18:52 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook March 7, 2022 18:52 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook March 7, 2022 19:18 Inactive
@vercel vercel bot temporarily deployed to Preview – vault March 7, 2022 19:18 Inactive
@vercel vercel bot temporarily deployed to Preview – vault March 22, 2022 16:34 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook March 22, 2022 16:34 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook March 22, 2022 16:56 Inactive
@vercel vercel bot temporarily deployed to Preview – vault March 22, 2022 16:56 Inactive
@averche averche force-pushed the ilyakaznacheev/api-context-functions branch from 1938afe to a850e0f Compare March 22, 2022 17:23
@vercel vercel bot temporarily deployed to Preview – vault March 22, 2022 17:23 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook March 22, 2022 17:23 Inactive
api/sys_capabilities.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@digivava digivava left a comment

Choose a reason for hiding this comment

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

Whew, good work! LGTM, aside from some inconsistency in when the http.MethodGet is used vs the hard-coded "GET".

I am starting to second-guess whether adding such a large amount of duplicate methods is a better choice than just adding ctx to all the existing ones and bumping to a new major version (where breaking changes would presumably be expected)... but it's a little late for that now so we can rethink that in the new version of the client I suppose!

@vercel vercel bot temporarily deployed to Preview – vault March 22, 2022 18:36 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook March 22, 2022 18:37 Inactive
@averche
Copy link
Contributor Author

averche commented Mar 22, 2022

Whew, good work! LGTM, aside from some inconsistency in when the http.MethodGet is used vs the hard-coded "GET".

Good point, I think I'll just revert all of the http.MethodGet / http.MethodPut changes and create another PR for that after this one. It's not a good to mix two things in one PR (especially a huge one like this).

I am starting to second-guess whether adding such a large amount of duplicate methods is a better choice than just adding ctx to all the existing ones and bumping to a new major version (where breaking changes would presumably be expected)... but it's a little late for that now so we can rethink that in the new version of the client I suppose!

This is meant as a stop-gap solution until we have a new shiny version. In the new version, we'll most likely default to context-only functions.

@vercel vercel bot temporarily deployed to Preview – vault March 22, 2022 18:48 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook March 22, 2022 18:48 Inactive
@heatherezell heatherezell linked an issue Mar 22, 2022 that may be closed by this pull request
@averche averche merged commit 8234a66 into main Mar 23, 2022
@averche averche deleted the ilyakaznacheev/api-context-functions branch March 23, 2022 21:47
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.

Support context propagation in http clients Pass Context to http requests
4 participants