Support custom prefixes for S3 and STS #75
Conversation
| @Config("s3proxy.hostname") | ||
| @ConfigDescription("Hostname to use for REST operations, virtual-host style addressing is only supported if this is set") | ||
| @Config("s3proxy.s3.hostname") | ||
| @ConfigDescription("Hostname to use for S3 REST operations, virtual-host style addressing is only supported if this is set") | ||
| public TrinoS3ProxyConfig setHostName(String hostName) | ||
| { | ||
| this.hostName = Optional.ofNullable(hostName); |
There was a problem hiding this comment.
Let's rename this (and the method) s3Hostname
| public static final String S3_PATH = BASE_PATH + "s3"; | ||
| public static final String STS_PATH = BASE_PATH + "sts"; | ||
| private static final String BASE_PATH = "/api/v1/s3Proxy/"; | ||
| public static final String DEFAULT_S3_PATH = BASE_PATH + "s3"; |
There was a problem hiding this comment.
These constants aren't really useful any more. I'd inline them. Let's make BASE_PATH a config as well.
There was a problem hiding this comment.
The idea was that S3_PATH and STS_PATH are now fully independent. BASE_PATH is only used here for the default.
So someone may have s3proxy.s3.prefix=/s3 and s3proxy.sts.prefix=/sts and there's no common part to them
Agreed on removing the constants
vagaerg
force-pushed
the
configurable-paths
branch
from
4ebd242 to
e34a0df
Compare
| @@ -52,11 +53,16 @@ public class TrinoS3ProxyServerModule | |||
| @Override | |||
| protected void setup(Binder binder) | |||
| { | |||
| configBinder(binder).bindConfig(SigningControllerConfig.class); | |||
| configBinder(binder).bindConfig(TrinoS3ProxyConfig.class); | |||
There was a problem hiding this comment.
buildConfigObject already does the binding so this is redundant
|
|
||
| @Config("s3proxy.s3.prefix") | ||
| @ConfigDescription("URL Prefix for S3 operations, optional") | ||
| public TrinoS3ProxyConfig setS3Prefix(String s3Prefix) |
There was a problem hiding this comment.
Is prefix the right noun? This is the full path.
There was a problem hiding this comment.
By prefix I meant "all S3 operations will be under this path" - meaning all S3 URLs will be this path or something nested within it.
Happy to change it to s3Path if you think that's more appropriate but I thought prefix was reasonable
vagaerg
force-pushed
the
configurable-paths
branch
from
e34a0df to
54ae608
Compare
| @@ -45,7 +44,7 @@ public TrinoS3ProxyResource(SigningController signingController, TrinoS3ProxyCli | |||
| { | |||
| this.signingController = requireNonNull(signingController, "signingController is null"); | |||
| this.proxyClient = requireNonNull(proxyClient, "proxyClient is null"); | |||
| this.serverHostName = requireNonNull(trinoS3ProxyConfig, "restConfig is null").getHostName(); | |||
| this.serverHostName = requireNonNull(trinoS3ProxyConfig, "trinoS3ProxyConfig is null").getS3HostName(); | |||
There was a problem hiding this comment.
We don't usually check for null on any object we're using - i.e. config objects. You'd get an early NPE anyway.
There was a problem hiding this comment.
I was debating this one, thanks for clarifying
vagaerg
force-pushed
the
configurable-paths
branch
from
54ae608 to
a46893d
Compare
| @@ -62,8 +60,8 @@ public Object createTestInstance(TestInstanceFactoryContext factoryContext, Exte | |||
|
|
|||
| TestingTrinoS3ProxyServer trinoS3ProxyServer = builder | |||
| .withMockS3Container() | |||
| .addModule(binder -> binder.bind(TestingS3ClientProvider.TestingS3ClientConfig.class).in(Scopes.SINGLETON)) | |||
There was a problem hiding this comment.
This should be in the child binder where TestingS3ClientProvider is bound
vagaerg
force-pushed
the
configurable-paths
branch
from
a46893d to
ffe546e
Compare
vagaerg
force-pushed
the
configurable-paths
branch
from
ffe546e to
2ec2f45
Compare
Closes #57