Expand OCSP Source interface#772
Conversation
Codecov Report
@@ Coverage Diff @@
## master #772 +/- ##
==========================================
- Coverage 57.67% 57.62% -0.06%
==========================================
Files 79 76 -3
Lines 6965 6942 -23
==========================================
- Hits 4017 4000 -17
+ Misses 2521 2519 -2
+ Partials 427 423 -4
Continue to review full report at Codecov.
|
|
is this ready? |
ocsp/responder.go
Outdated
| } | ||
| } | ||
|
|
||
| func writeExtraHeaders(response http.ResponseWriter, headers http.Header) { |
There was a problem hiding this comment.
this feels more like overrideHeaders, you intentionally delete original key value and add headers
There was a problem hiding this comment.
Makes sense.
| // object and pass it to http.Handle. | ||
| type Source interface { | ||
| Response(*ocsp.Request) ([]byte, bool) | ||
| Response(*ocsp.Request) ([]byte, http.Header, error) |
There was a problem hiding this comment.
I prefer some docs around why we design this func. It implies that each response will have its own set of header. And in this stub library we actually never use this feature. Can header be specific to one response, or just specific to the entire source.
There was a problem hiding this comment.
I'll write some extra docs here, the main impetus is that we'd like to calculate the cache control period for each response based on the ThisUpdate/NextUpdate fields instead of setting a static cache period over the entire source (as well as set some CDN specific headers on a per-response basis).
ocsp/responder.go
Outdated
| if err != nil { | ||
| if err == ErrNotFound { | ||
| if extraHeaders != nil { | ||
| writeExtraHeaders(response, extraHeaders) |
There was a problem hiding this comment.
Go idiom is to avoid returning meaningful values when an err return is non-nil, so we should probably ignore the extraHeaders on error returns.
|
Ready for re-review. |
|
LGTM |
|
lgtm |
|
Hoping osx build won't time out this time, I just restarted it. |
|
OS X build still seems unhappy. |
|
I will deal with it at master branch |
4 participants
Changes the
ocsp.Sourceinterface fromResponse(*ocsp.Request) ([]byte, bool)toResponse(*ocsp.Request) ([]byte, http.Header, error)so that a source can provide more insight into what has happened when looking for a response than found/not found as well as allowing it to provide specific headers to be set in the subsequent response.Will add a few more tests + better documentation of the new interface style but wanted to get maintainers thoughts before getting too much deeper.
Fixes #732.