Skip to content
This repository has been archived by the owner on Mar 16, 2021. It is now read-only.

Remove AWS4AuthLayer and HTTP.request extension #10

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ararslan
Copy link
Member

It's currently exceedingly difficult for packages to extend HTTP.request by providing a custom layer, which is likely why the logic for an AWS authentication layer is in HTTP rather than AWSCore. I found this out while attempting and failing to write a test that properly exercises the request extension here.

The simplest course of action would be to remove the functionality from this package and instead introduce a dependency on AWSAuth to HTTP, with the request extension using signing defined here.

It's currently exceedingly difficult for packages to extend
`HTTP.request` by providing a custom layer, which is likely why the
logic for an AWS authentication layer is in HTTP rather than AWSCore.
The simplest course of action would be to remove the functionality from
this package and instead introduce a dependency on AWSAuth to HTTP, with
the `request` extension using signing defined here.
@ararslan ararslan requested a review from omus February 20, 2019 00:11
@codecov-io
Copy link

codecov-io commented Feb 20, 2019

Codecov Report

Merging #10 into master will increase coverage by 5.47%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #10      +/-   ##
==========================================
+ Coverage   76.66%   82.14%   +5.47%     
==========================================
  Files           2        2              
  Lines          60       56       -4     
==========================================
  Hits           46       46              
+ Misses         14       10       -4
Impacted Files Coverage Δ
src/AWSAuth.jl 100% <ø> (ø) ⬆️
src/signaturev4.jl 81.81% <ø> (+5.54%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 834536c...fdf7efc. Read the comment docs.

@rofinn
Copy link
Member

rofinn commented Feb 20, 2019

Maybe make an issue in HTTP.jl describing why it's difficult to extend HTTP.request with custom layers? This seems like an issue that should be addressed in any further design/refactoring and should be documented.

@iamed2
Copy link
Member

iamed2 commented Feb 20, 2019

To be explicit, the hard part is adding the layer to the stack right? The actual creation of the custom layer is easy but getting HTTP to use it is hard?

@ararslan
Copy link
Member Author

Yes, what Eric said is exactly right. One can define a Layer subtype with a request method that does what you want, but there are some serious complications. One example is that not every request(::Layer, args...) method takes the same arguments, so to know how to formulate the next call to request in your method, you need to know where in the stack your layer is, so that you know the expected signature for the next call. Internally, HTTP gets around this by having HTTP.stack define the layers in a specific order that works. In that case, the AWS authentication layer is not the outermost layer, and to ensure that this package puts the layer in the same spot, it would need to completely reconstruct the stack by hand, and ensure that any preprocessing that's done in the user-facing request methods that get called before those which take Layers is taken care of. That essentially amounts to a reimplementation of HTTP.request.

@ararslan
Copy link
Member Author

I've opened an issue on HTTP for discussion: JuliaWeb/HTTP.jl#389

@ararslan ararslan mentioned this pull request May 23, 2019
3 tasks
@omus omus removed their request for review March 16, 2021 14:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants