Support serving of Virtual Host requests #51
Conversation
vagaerg
force-pushed
the
vhost-support-outward
branch
2 times, most recently
from
5989fa2 to
cd70a6a
Compare
| return this; | ||
| } | ||
|
|
||
| public Optional<String> getHostName() |
| { | ||
| this.signingController = requireNonNull(signingController, "signingController is null"); | ||
| this.proxyClient = requireNonNull(proxyClient, "proxyClient is null"); | ||
| this.restConfig = requireNonNull(restConfig, "restConfig is null"); |
There was a problem hiding this comment.
Because config objects are mutable we don't use them directly. Instead, we assign the value from the config object to a immutable field.
|
|
||
| import java.util.Optional; | ||
|
|
||
| public class RestConfig |
There was a problem hiding this comment.
Given that this is only used by TrinoS3ProxyResource I'd rename it to TrinoS3ProxyConfig (and update the config names).
| import java.util.Locale; | ||
| import java.util.function.BiFunction; | ||
|
|
||
| public class MultiMapHelper |
| import static io.trino.s3.proxy.server.collections.MultiMapHelper.lowercase; | ||
| import static java.util.Objects.requireNonNull; | ||
|
|
||
| public record S3RequestInformation(String bucketName, String pathInBucket, MultivaluedMap<String, String> requestHeaders, MultivaluedMap<String, String> requestQueryParameters, |
There was a problem hiding this comment.
S3RequestInformation does not have the body of the request at present, but I think as part of #37 the request body would also be put into it - so it would not be metadata only.
What do you think about S3Request?
There was a problem hiding this comment.
Actually, what about IncomingS3Request?
There was a problem hiding this comment.
Maybe ParsedS3Request or InternalS3Request? Or leave it as is is fine too.
There was a problem hiding this comment.
ParsedS3Request sounds good!
| @@ -134,23 +131,4 @@ private static String buildRemoteHost(URI remoteUri) | |||
| } | |||
| return remoteUri.getHost() + ":" + port; | |||
| } | |||
|
|
|||
| private static String rewriteRequestPath(ContainerRequest request, String bucket) | |||
There was a problem hiding this comment.
Maybe move buildRemoteHost() too?
There was a problem hiding this comment.
Where do you think we could move this to? rewriteRequestsPath got removed because the new S3 Metadata record has been parsed enough that we don't need to compute it again.
buildRemoteHost computes the remote host, which doesn't really belong to S3RequestMetadata I don't think.
Happy to create a helper class though, perhaps RemoteRequestUtil?
There was a problem hiding this comment.
I'm not sure actually. Let's leave it.
|
|
||
| import java.util.List; | ||
|
|
||
| @TrinoS3ProxyTest(modules = {WithConfiguredBuckets.class, WithVirtualHostEnabledProxy.class}, filters = WithVirtualHostEnabledProxy.class) |
There was a problem hiding this comment.
nit: the testing builder has an addModule method so WithVirtualHostEnabledProxy as a module specified here isn't necessary. Maybe we don't even need modules in TrinoS3ProxyTest and can move everything to filters.
| @@ -69,6 +71,7 @@ public static Builder builder() | |||
| public static class Builder | |||
| { | |||
| private final ImmutableSet.Builder<Module> modules = ImmutableSet.builder(); | |||
| private final ImmutableMap.Builder<String, String> configs = ImmutableMap.builder(); | |||
There was a problem hiding this comment.
We usually name this properties.
|
@vagaerg reminder to squash the rebase commit please |
vagaerg
force-pushed
the
vhost-support-outward
branch
from
cd70a6a to
589e29a
Compare
| { | ||
| private Optional<String> hostName = Optional.empty(); | ||
|
|
||
| @Config("rest.hostname") |
There was a problem hiding this comment.
Now that the class is renamed, the value should too. maybe s3proxy.hostname
There was a problem hiding this comment.
I was a bit unsure about this one, as we may have rest and non-rest properties. But we can figure it out later, happy to rename for now
vagaerg
force-pushed
the
vhost-support-outward
branch
from
589e29a to
aaff406
Compare
trino-s3-proxy/src/main/java/io/trino/s3/proxy/server/rest/TrinoS3ProxyClient.java
Outdated
Show resolved
Hide resolved
| import static io.trino.s3.proxy.server.collections.MultiMapHelper.lowercase; | ||
| import static java.util.Objects.requireNonNull; | ||
|
|
||
| public record ParsedS3Request(String bucketName, String pathInBucket, MultivaluedMap<String, String> requestHeaders, MultivaluedMap<String, String> requestQueryParameters, |
There was a problem hiding this comment.
some NITs:
we can call this just S3Request, i don't think anything else is using that name. WDYT?
can we have each field on a new line?
There was a problem hiding this comment.
Refer to #51 (comment) for the name, I originally named this S3RequestInformation, then we discussed S3Request but settled on this.
I don't really have a strong preference between this or S3Request so happy for you & @Randgalt to discuss
There was a problem hiding this comment.
I also don't have a strong preference, so let's go with this for now!
trino-s3-proxy/src/main/java/io/trino/s3/proxy/server/rest/ParsedS3Request.java
Outdated
Show resolved
Hide resolved
trino-s3-proxy/src/main/java/io/trino/s3/proxy/server/rest/TrinoS3ProxyResource.java
Show resolved
Hide resolved
trino-s3-proxy/src/main/java/io/trino/s3/proxy/server/rest/TrinoS3ProxyConfig.java
Show resolved
Hide resolved
vagaerg
force-pushed
the
vhost-support-outward
branch
from
aaff406 to
d73cba3
Compare
Closes #19
This PR also goes slightly into #37 , by creating an
S3RequestInformationrecord - if @ragnard and @mosiac1 are happy with this choice, I'm glad to add the remaining fields and make the signer use it too.