feat: provide context with app transport signer#119
feat: provide context with app transport signer#119jamestelfer wants to merge 3 commits intobradleyfalzon:masterfrom
Conversation
Adds the "SignWithContext" interface that allows implementations that use I/O (e.g. calls to a remote KMS) to use the context of the current request, allowing it to participate in the cancellation/timeout as appropriate.
jamestelfer
force-pushed
the
contextsigner
branch
from
c5edb74 to
74c1f7d
Compare
|
Note: if the approach in #119 is OK, I'd change this implementation to implement the newer options interface. |
wlynch
left a comment
wlynch
left a comment
There was a problem hiding this comment.
This looks good - thanks!
| t.Fatalf("error calling RoundTrip: %v", err) | ||
| } | ||
|
|
||
| if !check.Captured { |
There was a problem hiding this comment.
nit: you can omit this - check.Value will be nil if this is not set, and we're always comparing against a populated want value.
There was a problem hiding this comment.
That's quite true. I added this so the test failure was more self explanatory even though it's not strictly necessary. Happy to remove it though, LMK what you think.
The test doesn't require key variability, so using a simpler type makes a lot of sense.
jamestelfer
left a comment
jamestelfer
left a comment
There was a problem hiding this comment.
Marking this for re-review, LMK whether you'd like the explicit test failure message or the implicit behaviour.
|
@wlynch / @bradleyfalzon do you have any further refinements or comments on how you would like this PR to proceed? I'm happy to put some more effort into it if that's what it needs. |
|
@wlynch / @bradleyfalzon following up on this to see whether or not you'd like me to proceed, and to let you know that I'm still monitoring this PR. This is not meant to add pressure: I realise we are all busy on other things. |
2 participants
Adds the
SignWithContextinterface that allows implementations that use I/O (e.g. calls to a remote KMS) to use the context of the current request, allowing it to participate in cancellation/timeout as appropriate.Backwards compatibility is preserved by wrapping a
Signerimplementation in wrappingSignerWithContextAdapter. The existingSignerinterface has been marked as deprecated to signal to implementors of the new implementation.The existing
RSASignernow implements both interfaces, with theSignmethod marked as deprecated. This preserves backwards compatibility and avoids requiring an adaptor for the default implementation.Fixes #117