S3 vhost customisation #832
Conversation
This implements a change suggested by this comment: brendanhay#797 (comment) as part of a fix to the following issue: brendanhay#760 This modifies the code generation for AWS endpoints so that requests which make other requests all share the same (passed-in) `Service`. This is done by making `request` take an extra `Service` argument. This also adds a new parent class of `AWSRequest`, called `AWSService`, which defines the default service used for each request (this being `defaultService`.) These are some breaking changes, but they are required to allow modified `Service`s to be used in complex requests. This is necessary to allow VHost rewriting to be turned off for local mocking and other use cases at the `Service` level. Co-authored-by: Bas van Dijk <v.dijk.bas@gmail.com>
We have no need for types which associate a `Service` but are not `Request`s.
Originally "Non-working attempt to fix the test failure in PR brendanhay#797" This adds a new field to `Service` which controls if requests to the service should have their URLs rewritten using `s3vhost`. This is particularly relevant for mocks of S3 which don't support having the bucket name in the hostname (e.g. because they are bound to `localhost`, which isn't a domain and can't have a subdomain.) This field is defaulted to `True` to preserve the current behaviour for all services. Co-authored-by: Isaac van Bakel <isaac@supercede.com>
endgame
force-pushed
the
s3-vhost-customisation
branch
2 times, most recently
from
d785589
to
71c5da2
Compare
This makes our S3-vhost-rewriting rules configurable in the same way as boto3.
We need to regenerate everything anyway, so we may as well keep up with service definitions.
endgame
force-pushed
the
s3-vhost-customisation
branch
from
71c5da2
to
d2ca5b5
Compare
endgame
force-pushed
the
s3-vhost-customisation
branch
from
d2ca5b5
to
f360921
Compare
I don't know enough to comment on this in general, since I just cribbed this from Bas' code. But in the case where
Will do.
All good with me 👍. |
|
I will test this soon. If anyone needs a version with all services regenerated, it's available on my fork with the same branch name. |
|
I can confirm that our integration tests work just fine with aws mocked with localstack and endpoint override configured not to use vhosts. |
|
Looks like these changes work fine for us, for both real AWS usage and for local mocking with Moto. |
brendanhay/amazonka#832 should have solved our problems, so we shouldn't need the fork.
Path style is not supported for newer buckets, more info: https://aws.amazon.com/blogs/aws/amazon-s3-path-deprecation-plan-the-rest-of-the-story/ All object storage providers (like MinIO, ScalityRing, etc) might not work with vhost style addressing, so this change introduces a new configuration option in cargohold as aws.s3AddressingStyle to choose the addressing style. Other changes: * Move wire-server from using forked version of amazonka to upstream HEAD. The option to choose S3 Addressing Style has been implemented in brendanhay/amazonka#832 * Makefile: Skip schema migrations for packages without DB This allows running something like `make ci package=cargohold` even if cargohold doesn't produce a cargohold-schema executable.
Implement S3 vhost rewriting customisation using the design proposed by @basvandijk. The basic design is the same:
request :: AWSRequest a => Service -> a -> Request ainclass AWSRequestis now accepts the service to send the request to, instead of generators hardcodingdefaultService. This allows theRequestto be computed from the overridenServiceinstead ofdefaultService.service :: AWSRequest a => Proxy a -> Servicesomewhere. We apply overrides to it before passing it torequest.request :: AWSRequest a => (Service -> Service) -> a -> Request a, but every option I could think of seemed worse than a new typeclass method.There are some differences to the original design (which I cherry-picked from @ivb-supercede's branch with generator fixes):
servicefunction is a method ofclass AWSRequest ainstead of a separateclass AWSService aand makingclass AWSService a => AWSRequest a; I couldn't see much benefit in a separate typeclass. Am I wrong?Bool, I added a sum type to match the S3 addressing style optionsBecause we're going to have to regenerate everything, I updated botocore as well. A full regeneration is available at
endgame:s3-vhost-customisation-generationand will be merged once this branch lands. I have rebuilt the entire repo locally and it at least compiles.Ping @basvandijk @ivb-supercede @kfigiela @akshaymankar @periodic who might be interested — please test the branch above.
@basvandijk / @ivb-supercede: Is it OK that I've cherry-picked your commits here?
Fixes #760
Closes #797