improve S3 vhost matching on any domain #7870
Conversation
b36e946 to
97c345b
Compare
There was a problem hiding this comment.
Looks great, thanks for jumping on this @bentsku ! 🚀
The only question I have is: is s3.localhost.localstack.cloud:{edge_port} always resolvable from LocalStack? Or is it safer to target localhost when proxying the request to ourselves? I'd rather target s3.localhost.localstack.cloud:{edge_port} as the service parser will match more effectively against S3 than localhost.
I like the approach you've taken here - it's a good mix of efficient forwarding on loopback (using config.get_edge_url()), as well as forwarding the *.s3.localhost.localstack.cloud host header to facilitate service name parsing.
Minor nitpick - but will pick it up in the other branch (no need to change). 👍
|
|
||
| If the region is contained in the host-name we remove it (for now) as moto cannot handle the region correctly | ||
|
|
||
| :param url: the original url | ||
| :param domain: the domain name | ||
| :param bucket: the bucket name | ||
| :param region: the region name | ||
| :return: re-written url as string |
There was a problem hiding this comment.
nit: could add the port parameter to the docstring as well
There was a problem hiding this comment.
Oops, thanks for catching that!
While testing lambda transparent endpoint injection, @thrau came across the issue that we were too restrictive with our domain matching. This is also part of @simonrw #7774, and should simplify the logic concerning matching on external hostname. @whummer also came across the new provider issue at the same time today.
This PR is building on the new test Waldemar implemented, and add more scenarios where we should properly match the route. This should be merged right after #7868.
The only question I have is: iss3.localhost.localstack.cloud:{edge_port}always resolvable from LocalStack? Or is it safer to targetlocalhostwhen proxying the request to ourselves? I'd rather targets3.localhost.localstack.cloud:{edge_port}as the service parser will match more effectively against S3 thanlocalhost.edit: after checking with Thomas and Waldemar, it's faster to use
localhostfor the loopback network device. So we're target the request tolocalhost, but set theHostheader tos3.localhost.localstack.cloud:{edge_port}to allow the service name parser to short-circuit and quickly know it's an S3 request.