[ca]: Add a 5-second timeout to an external CA signing request #2064
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2064 +/- ##
==========================================
- Coverage 54.26% 54.15% -0.12%
==========================================
Files 111 111
Lines 19351 19326 -25
==========================================
- Hits 10501 10466 -35
- Misses 7582 7584 +2
- Partials 1268 1276 +8Continue to review full report at Codecov.
|
ca/external.go
Outdated
| @@ -154,7 +155,8 @@ func (eca *ExternalCA) CrossSignRootCA(ctx context.Context, rca RootCA) ([]byte, | |||
| } | |||
|
|
|||
| func makeExternalSignRequest(ctx context.Context, client *http.Client, url string, csrJSON []byte) (cert []byte, err error) { | |||
| resp, err := ctxhttp.Post(ctx, client, url, "application/json", bytes.NewReader(csrJSON)) | |||
| requestCtx, _ := context.WithTimeout(ctx, 5*time.Second) | |||
There was a problem hiding this comment.
Call the cancel function after the context is no longer needed. That will ensure resources get freed, instead of possibly having them tied up for 5 seconds.
|
Thanks. Have you tested this? |
cyli
force-pushed
the
timeout-remote-sign-request
branch
2 times, most recently
from
1da8ad8 to
d15a5b3
Compare
|
@aaronlehmann Previously I only tested manually, but I've added a test that an individual call to an external server is timed out. |
ca/external.go
Outdated
| func makeExternalSignRequest(ctx context.Context, client *http.Client, url string, csrJSON []byte) (cert []byte, err error) { | ||
| resp, err := ctxhttp.Post(ctx, client, url, "application/json", bytes.NewReader(csrJSON)) | ||
| func makeExternalSignRequest(ctx context.Context, client *http.Client, url string, csrJSON []byte, timeout time.Duration) (cert []byte, err error) { | ||
| requestCtx, cancel := context.WithTimeout(ctx, timeout) |
There was a problem hiding this comment.
I'd prefer to move the context creation outside makeExternalSignRequest. Passing in a timeout is a bit redundant with passing in a context.
|
Thanks for the test! |
cyli
force-pushed
the
timeout-remote-sign-request
branch
from
d15a5b3 to
5bc6637
Compare
|
LGTM |
|
ping @diogomonica please review |
Signed-off-by: cyli <ying.li@docker.com>
cyli
force-pushed
the
timeout-remote-sign-request
branch
from
5bc6637 to
7498dcc
Compare
cc @aaronlehmann