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

Adding the ability to not treat 4xx response codes as errors that sho… #176

Merged
merged 2 commits into from
Aug 19, 2019

Conversation

bklawans
Copy link

Adds support for not treating 4xx status coded from vaults as errors for reads/writes, and allows them to be passed back without retries or being converted to VaultExceptions.

Use Case:
We have a spring app with a custom property resolver that can read properties from various sources, and want to add Vault as a new source. If a property does not exist in vault, it should read it from the next source. With the current version of vault-java-driver if the property is not in Vault it will retry the configured number of times, waiting between each retry, which significantly slows down the app startup. This PR allows us to turn that off and get an immediate "no such value" from Vault.

If you would prefer, I could modify this to still throw an exception but just turn off the retry logic.

…uld be retried and converted into exceptions
@steve-perkins
Copy link
Contributor

Thank you so much, @bklawans!

Taking a step back here, I wonder if we should EVER retry when the input is invalid (e.g. 400), authorization fails (e.g. 401/403), or when there's simply no data found (i.e. 404)? If not, then can we just exclude the whole 4xx as the normal behavior?

I can MAYBE see developers using a "long-polling" pattern for 404 not founds. Setting the maxRetries level really high, and waiting for some data to show up that is being populated by an asynchronous process. But that sort of edge case is really NOT the intended use for the internal retry mechanism. I would be comfortable pushing people toward implementing their own retry logic higher up the call stack.

Unless you can think of some use cases to ever retry on status codes in the 4xx range, I would honestly prefer just to make "no-retry-on-4xx" the default behavior. Rather than adding any more complexity to the retry mechanism.

If you agree with that, then this PR could be slimmed down significantly. I think such a change in behavior would warrant a major version bump for the next release, but I'm fine with that.

@bklawans
Copy link
Author

I absolutely agree - I was trying to avoid changing the default behavior, but since it sounds like we are on the same page I'll rev this PR to make it the default behavior. I'm traveling through Monday so it will take me a few days to turn around.

@steve-perkins
Copy link
Contributor

No problem at all. Thanks so much, and have a great weekend.

@bklawans
Copy link
Author

I've updated the PR to make this the default behavior, and added a new test to the VaultTests suite.

@steve-perkins
Copy link
Contributor

Awesome, thank you!

@steve-perkins steve-perkins merged commit 9419a3e into BetterCloud:master Aug 19, 2019
@jetersen
Copy link
Contributor

jetersen commented Aug 24, 2019

@bklawans hmm thoughts about vault.logical().list() not throwing errors but instead gives you a empty list. You have no way of getting the response.
Sad to see tests were not fixed.

Can we please add some CI 😓

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.

4 participants