Fix OSS sealunwrapper adding extra get + put request to all storage get requests

[andybao-dd]

Problem

Starting after commit 0ec3524, for each GET request Vault makes to the storage backend, the OSS seal unwrapper will make 1 additional GET request plus 1 additional PUT request. This is because the seal unwrapper now tries to unwrap all storage entries, even if the entry was not wrapped in the first place! Prior to 0ec3524, there was a check to see if the storage entry was wrapped before proceeding with the unwrap procedure, which prevented this behavior.

The increased storage latency, writes, and traffic resulting from the extra requests causes noticeable performance degradation for some workloads. 0ec3524 was first included in Vault v1.15.0, and after upgrading from Vault v1.14.9 to v1.15.6, some of our Vault clusters would take 2-3x longer to start up.

Other users have also reported storage-related issues after upgrading to Vault v1.15+: #24726.

Description

This PR restores the check to prevent Vault from unwrapping a storage entry if the storage entry is not wrapped. This should eliminate the extra GET and PUT requests.

Testing

A small test is included in this PR to confirm that at least the extra PUT request is no longer being made.

I also ran some benchmarks using vault-benchmark (awesome tool BTW!) + a small wrapper script. The chart below shows the p90 of the time needed to perform different benchmark operations on a sample Vault instance. The Vault instance was configured with Amazon S3 for storage and caching was explicitly disabled.

[📊 Additional benchmark results can be found here.]

From the graph, it seems like:

• Operations on commit 0ec3524 of Vault (labeled post-0ec3524 in the graph) take significantly longer compared to operations made on a build of the parent commit (labeled pre-0ec3524).
• As of today, Vault built off of the main branch continues to exhibit similar slow operation issues.
• Vault built off of this PR branch (labeled main+fix) shows improved performance, matching the performance level of pre-0ec3524.

Other notes

Why did you disable caching during the benchmarks?

Due to how vault-benchmark benchmarks are structured, it is difficult to evaluate storage performance without disabling caching. vault-benchmark benchmarks start off with an empty cluster and then write a bunch of state to it. Then they might read that state back or perform other write operations.

The problem is that with a few exceptions, all write operations will write to both the storage and the in-memory cache. So by starting off with an empty cluster and then setting up the state by performing writes, we have effectively written the entire cluster state to the in-memory cache. Therefore most read benchmarks we perform will be benchmarking the in-memory cache instead of the actual storage codepath.

I have still included the benchmark results with the cache enabled however, in case they are helpful.

[victorr]

Hi @andybao-dd, thank you for submitting this PR. I will be reviewing it.

Good job identifying the missing check!

[victorr]

Thank you for being so thorough, your attention to detail is very appreciated.

[victorr]

The failed linters job is due to an unrelated change that has already been fixed on main.