[#4992] support credential vending framework #4995
Conversation
FANNG1
force-pushed
the
credential-framework
branch
from
55c1ad6
to
bdf04fa
Compare
| import java.util.Map; | ||
|
|
||
| /** Interface representing a credential with type, expiration time, and additional information. */ | ||
| public interface Credential { |
There was a problem hiding this comment.
What is the purpose of put it to common package?
There was a problem hiding this comment.
It maybe used in the client side.
| ServiceLoader<CredentialProvider> serviceLoader = | ||
| ServiceLoader.load(CredentialProvider.class, classLoader); | ||
| List<Class<? extends CredentialProvider>> providers = | ||
| Streams.stream(serviceLoader.iterator()) |
There was a problem hiding this comment.
What is the consideration of using service loader?
There was a problem hiding this comment.
Different catalogs may use different credential providers so that users could place the s3 provider in catalogA, place gcs provider in catalogB, so we using service loader to load corresponding jar in catalog classpaths.
| * | ||
| * <p>A credential provider is responsible for managing and retrieving credentials. | ||
| */ | ||
| public interface CredentialProvider { |
There was a problem hiding this comment.
I think you can change to like public interface Credential extends Closeable to avoid defining void stop() here.
| * | ||
| * @return a map of credential information. | ||
| */ | ||
| Map<String, String> getCredentialInfo(); |
There was a problem hiding this comment.
I would suggest change the style of getXXX to xxx, like credentialType().
There was a problem hiding this comment.
Do we need to define an interface for Token like Hadoop?
There was a problem hiding this comment.
I would suggest that you can refer to Hadoop's Credentials, Token and TokenIdentifer to see how to define our credential system.
There was a problem hiding this comment.
Credential is like Token in hadoop, the difference is Token in hadoop focus on the security of the identifer, while we focus on how to represent diverse tokens like S3-token, AKSK, delegation token with different properties.
There was a problem hiding this comment.
The concrete Credential has the clear definition , take S3TokenCredential for example
public class S3TokenCredential implements Credential {
private String accessKeyId;
private String secretAccessKey;
private String sessionToken;
private long expireMs;
public S3TokenCredential(
String accessKeyId, String secretAccessKey, String sessionToken, long expireMs) {
this.accessKeyId = accessKeyId;
this.secretAccessKey = secretAccessKey;
this.sessionToken = sessionToken;
this.expireMs = expireMs;
}
@Override
public String getCredentialType() {
return CredentialConstants.S3_TOKEN_CREDENTIAL_TYPE;
}
@Override
public long getExpireTime() {
return expireMs;
}
@Override
public Map<String, String> getCredentialInfo() {
return (new ImmutableMap.Builder<String, String>())
.put(S3Properties.GRAVITINO_S3_ACCESS_KEY_ID, accessKeyId)
.put(S3Properties.GRAVITINO_S3_SECRET_ACCESS_KEY, secretAccessKey)
.put(S3Properties.GRAVITINO_S3_TOKEN, sessionToken)
.build();
}
}| @@ -28,6 +28,7 @@ plugins { | |||
|
|
|||
| dependencies { | |||
| implementation(project(":api")) | |||
| implementation(project(":catalogs:catalog-common")) | |||
There was a problem hiding this comment.
The dependency here looks strange if you only want to define two properties in catalog-common, it is easy to introduce the cyclic dependency.
There was a problem hiding this comment.
catalog-common is mainly used to define some properties used by catalog, client, connectors, I think it's ok to add the dependence here.
What changes were proposed in this pull request?
support credential vending framework
Why are the changes needed?
Fix: #4992
Does this PR introduce any user-facing change?
no
How was this patch tested?