Hook more events into presign functionality #1340
Conversation
Specifically ensure the following: * The emission of provide-client-params event * The emission of before-parameter-build event * Registering s3 accelerate handler to before-sign so s3 accelerate presigned url's can be created
kyleknap
requested review from
dstufft,
jamesls,
stealthycoin,
joguSD and
JordonPhillips
Codecov Report
@@ Coverage Diff @@
## develop #1340 +/- ##
===========================================
+ Coverage 98.17% 98.18% +<.01%
===========================================
Files 46 46
Lines 7736 7748 +12
===========================================
+ Hits 7595 7607 +12
Misses 141 141
Continue to review full report at Codecov.
|
tests/functional/test_sts.py
Outdated
| @@ -23,9 +23,6 @@ class TestSTSPresignedUrl(BaseSessionTest): | |||
| def setUp(self): | |||
| super(TestSTSPresignedUrl, self).setUp() | |||
| self.client = self.session.create_client('sts', 'us-west-2') | |||
| # Makes sure that no requests will go through | |||
There was a problem hiding this comment.
This was removed because the stubber hooks into the parameter building related event for validating API/parameter calls, but since we did not previously add any expected API calls in test, the stubber will now complain about an unexpected API call.
It seems strange that a stubber would be used for presigned URL's in the first place, but I could see it potentially affect downstream tests though if someone used a stubber to stub out an API call and used the same stubbed client to generate a presigned url in their tests. What people's thoughts are on this?
There was a problem hiding this comment.
Is there another event the stubber could rely on?
There was a problem hiding this comment.
Yeah I'd prefer if we could preserve this behavior somehow. The fact that we're changing a functional test suggests it's possible others will be impacted, especially given the stubber is a public API.
What are the other options here?
There was a problem hiding this comment.
@joguSD don't think there is another event the stubber could rely on. This PR is having the presigners emitprovide-client-params and before-parameter-build which are the events that would contain the unserialized parameters. Any of the other events down the stack use the serialized version of the parameters. So if we were to hook into another event, we would need to create a new one which we would not want to do.
@jamesls One idea initially that I have is to inject a value into the context object a flag like is_presign_request in the presigner logic and then in the Stubber class we can check for this flag and pass over asserting the request if the presign flag is present in the context. What's your thoughts on this? Not sure if there really any better options given the stubber currently does not have a way to disambiguate between a presign request and a normal client request.
There was a problem hiding this comment.
Sorry didn't see this until now. I'm not a huge fan on this because it unnecessarily couples the stubber and signers. Ideally the stubber shouldn't care about (or know) whether a request is presigned or not, it would just have better events it could hook into.
However, I'm not sure it's worth the trouble at this point to introduce new events so I think this is fine for now.
| @@ -727,7 +728,7 @@ def test_generate_presigned_url(self): | |||
| 'query_string': {}, | |||
| 'url_path': u'/mybucket/mykey', | |||
| 'method': u'GET', | |||
| 'context': {}} | |||
| 'context': mock.ANY} | |||
There was a problem hiding this comment.
This was switched to mock.ANY because we are now emitting the client parameter related events that inject values into the context. Thus using the context's exact value for these tests will be a maintenance burden if anymore customizations are added that inject into the context.
There was a problem hiding this comment.
I don't think review comments on your own PR are the best place for these type of contextual comments. Either a) A comment in the code or b) in the commit message.
kyleknap
force-pushed
the
presign-event
branch
from
8165f78
to
27bc626
Compare
kyleknap
added
pr/needs-review
|
I think this looks good now. |
Specifically ensure the following:
provide-client-paramsevent to hook in injection of parametersbefore-parameter-buildevent event to hook in internal customizations related to parameters such as SSE-C MD5 injection.before-signso s3 accelerate presigned url's can be created. It is important to note that I decided not to move therequest-createdevent to the presigner because the signers are already hooked up to that event so re-emitting that event will cause the signers to get triggered again. Furthermore, putting it atbefore-signis consistent with the handlers we use to change addressing style.Fixes #978