Add http credentials provider #136
Conversation
...-proxy/src/main/java/io/trino/aws/proxy/server/credentials/http/HttpCredentialsProvider.java
Outdated
Show resolved
Hide resolved
...-proxy/src/main/java/io/trino/aws/proxy/server/credentials/http/HttpCredentialsProvider.java
Outdated
Show resolved
Hide resolved
...-proxy/src/main/java/io/trino/aws/proxy/server/credentials/http/HttpCredentialsProvider.java
Outdated
Show resolved
Hide resolved
...-proxy/src/main/java/io/trino/aws/proxy/server/credentials/http/HttpCredentialsProvider.java
Show resolved
Hide resolved
...-proxy/src/main/java/io/trino/aws/proxy/server/credentials/http/HttpCredentialsProvider.java
Outdated
Show resolved
Hide resolved
| public Optional<Credentials> credentials(String emulatedAccessKey, Optional<String> session) | ||
| { | ||
| Request request = prepareGet() | ||
| .setUri(UriBuilder.fromUri(httpCredentialsProviderConfig.getEndpoint()).path(emulatedAccessKey).build()) |
There was a problem hiding this comment.
Don't forget to set the Content-Type
There was a problem hiding this comment.
This is currently a get request, Should we set the contenttype ?
|
Looks good - a few things to address |
...xy/src/test/java/io/trino/aws/proxy/server/credentials/http/TestHttpCredentialsProvider.java
Outdated
Show resolved
Hide resolved
...ws-proxy/src/main/java/io/trino/aws/proxy/server/credentials/http/HttpCredentialsModule.java
Outdated
Show resolved
Hide resolved
...-proxy/src/main/java/io/trino/aws/proxy/server/credentials/http/HttpCredentialsProvider.java
Outdated
Show resolved
Hide resolved
...-proxy/src/main/java/io/trino/aws/proxy/server/credentials/http/HttpCredentialsProvider.java
Outdated
Show resolved
Hide resolved
| requireNonNull(config, "Config is null"); | ||
| requireNonNull(httpClient, "httpClient is null"); | ||
| requireNonNull(objectMapper, "objectMapper is null"); | ||
| ObjectMapper adjustedObjectMapper = objectMapper.registerModule(new SimpleModule().addAbstractTypeMapping(Identity.class, identityClass)); |
There was a problem hiding this comment.
Could we use a jsonCodecBinder? Like this: https://github.com/trinodb/trino/blob/2cb2b5d79b53594f8d898e5936c2dbd75fb554d5/plugin/trino-opa/src/main/java/io/trino/plugin/opa/OpaAccessControlModule.java#L56
There was a problem hiding this comment.
Since we are mapping abstract types, it is not possible to have the jsonCodecBinder registered in the module. However, once the abstract type has been mapped at a higher level, we could register the jsonCodecBinder. I'll address that in a separate PR since this change affects the FileBasedCredentialsProvider as well.
| @Inject | ||
| public HttpCredentialsProvider(@ForHttpCredentialsProvider HttpClient httpClient, HttpCredentialsProviderConfig config, ObjectMapper objectMapper, Class<? extends Identity> identityClass) | ||
| { | ||
| requireNonNull(config, "Config is null"); |
There was a problem hiding this comment.
We're accessing a field in config already so we don't need the requireNonNull.
I'd however suggest using requireNonNull to check that config.getEndpoint() does not return null
There was a problem hiding this comment.
Since config used in the constructor, it's good to have the requireNonNull for config as well.
The Config class already has the @NonNull for the endpoint - https://github.com/pranavr12/aws-proxy/blob/http-creds/trino-aws-proxy/src/main/java/io/trino/aws/proxy/server/credentials/http/HttpCredentialsProviderConfig.java#L25.
There was a problem hiding this comment.
We don't usually add requireNonNull on configs.
There was a problem hiding this comment.
Removed requireNonNull on config
| Request request = prepareGet() | ||
| .setUri(UriBuilder.fromUri(httpCredentialsProviderEndpoint).path(emulatedAccessKey).build()) | ||
| .build(); |
There was a problem hiding this comment.
Should we add the session as a query parameter if present? I know we don't have a use for this currently but its nice to have
There was a problem hiding this comment.
Added session as a query param with tests
pranavr12
force-pushed
the
http-creds
branch
3 times, most recently
from
26886b5
to
beaea8b
Compare
| Request request = prepareGet() | ||
| .setUri(uriBuilder.build()) | ||
| .build(); | ||
| JsonResponse<Credentials> response = httpClient.execute(request, createFullJsonResponseHandler(jsonCodec)); |
There was a problem hiding this comment.
If the endpoint returns a 404 I imagine it would not return a properly formatted JSON that we could decode.
Should we wrap this in a try / catch and return Optional.empty() if we get status 404 or otherwise propagate the exception?
There was a problem hiding this comment.
Here are the possible scenarios:
Server returns 404 -> Credentials provider returns Optional.empty()
Server returns 200, but incorrect response -> The parsing fails and the response value is null (No exception is thrown). I've added a condition to check the null status. Credentials provider returns Optional.empty()
Server return 200, with correct response: Parsing succeeds and the correct value is returned
The Http credentials provider can be used to fetch the remote credentials and identity from a HTTP service