search

BetterCloud / vault-java-driver

Zero-dependency Java client for HashiCorp's Vault
https://bettercloud.github.io/vault-java-driver/
335 stars 224 forks source link

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

Closed bklawans closed 5 years ago

bklawans commented 5 years ago

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.

steve-perkins commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

bklawans commented 5 years ago

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

steve-perkins commented 5 years ago

Awesome, thank you!

jetersen commented 5 years ago

@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 😓