Pluggable certificate storage (following on from #284) #332
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Duplicate of #330, which I accidentally opened from the wrong branch. Unfortunately there doesn't seem to be a way to change the source branch, so I've had to re-create the PR.
Following on from the work @btwotch did in #284, this builds on their PR to address @oec's comment (I encountered a similar issue).
Recap
To recap, the current implementation will regenerate TLS certificates every request. Generating certificates is computationally expensive, so this leads to performance issues when there are many requests to the same host.
@btwotch made it possible to add a custom certificate store to alleviate this issue. However, multiple concurrent requests to the same host would result in the certificate being generated multiple times in parallel.
Changes in this PR
This PR adapts the certificate store's interface. Rather than having
Store()
andLoad()
as separate functions, a single function -Fetch()
is provided with the cache key (hostname) and a function that returns the certificate to cache, and it handles the storage and retrieval internally.While a simple storage implementation may mimic the previous behaviour (leaving the described problem in place), an alternative implementation of the certificate storage might take a lock on the
hostname when generating the certificate, meaning only one certificate will be generated and other clients will wait until it is present in the store.
Benchmark
Using a simple in-memory certificate store that takes a lock for each hostname while generating the certificate, I got the following results from some simple load testing using vegeta.
Without the certificate store
Note the mean latency of 1.3s, p99 latency of 11.8s, and success rate of 11.2%.
With the certificate store
Note the mean latency of 0.3s, p99 latency of 0.8ms, and success rate of 99.6. Quite an improvement!
Credit to @btwotch for the foundation of this PR - their original commit was cherry-picked and used as the starting point.
If you think it'd be valuable, I'd also be happy to add a PR to include the in-memory storage implementation I described in this comment.