Copilot commented on code in PR #9237:
URL: https://github.com/apache/gravitino/pull/9237#discussion_r2558333242
##########
common/src/main/java/org/apache/gravitino/credential/CredentialProvider.java:
##########
@@ -53,4 +58,37 @@ public interface CredentialProvider extends Closeable {
*/
@Nullable
Credential getCredential(CredentialContext context);
+
+ /**
+ * Returns the fully qualified class name of the {@link CredentialGenerator}
implementation. This
+ * generator will be loaded via reflection to perform the actual credential
creation.
+ *
+ * @return The class name of the credential generator.
+ * @throws UnsupportedOperationException if the provider does not use a
generator.
+ */
+ default String getGeneratorClassName() {
+ throw new UnsupportedOperationException(
+ "This CredentialProvider does not use a CredentialGenerator.");
+ }
+
+ /**
+ * Lazily loads and returns an instance of the {@link CredentialGenerator}.
This default
+ * implementation uses reflection and a no-argument constructor.
Review Comment:
The JavaDoc states this method uses "a no-argument constructor", but the
implementation at line 87 uses `setAccessible(true)` which implies it can also
work with non-public constructors. The documentation should clarify that it
supports both public and non-public no-argument constructors, or the
implementation should be restricted to only public constructors to match the
documentation.
```suggestion
* implementation uses reflection and a no-argument constructor (public or
non-public).
```
##########
bundles/azure/src/main/java/org/apache/gravitino/abs/credential/ADLSTokenProvider.java:
##########
@@ -68,70 +48,30 @@ public String credentialType() {
return ADLSTokenCredential.ADLS_TOKEN_CREDENTIAL_TYPE;
}
+ @Override
+ public String getGeneratorClassName() {
+ return "org.apache.gravitino.abs.credential.AzureTokenGenerator";
+ }
+
@Override
public Credential getCredential(CredentialContext context) {
if (!(context instanceof PathBasedCredentialContext)) {
return null;
}
Review Comment:
The null check for `PathBasedCredentialContext` is performed here, but after
this check the generator is still loaded and invoked. The generator should
handle this context type check instead. If the check remains here, the method
should return `null` immediately after line 60 to avoid unnecessarily loading
the generator.
```suggestion
} // Early return to avoid loading generator for invalid context
```
##########
common/src/main/java/org/apache/gravitino/credential/CredentialProvider.java:
##########
@@ -53,4 +58,37 @@ public interface CredentialProvider extends Closeable {
*/
@Nullable
Credential getCredential(CredentialContext context);
+
+ /**
+ * Returns the fully qualified class name of the {@link CredentialGenerator}
implementation. This
+ * generator will be loaded via reflection to perform the actual credential
creation.
+ *
+ * @return The class name of the credential generator.
+ * @throws UnsupportedOperationException if the provider does not use a
generator.
+ */
+ default String getGeneratorClassName() {
+ throw new UnsupportedOperationException(
+ "This CredentialProvider does not use a CredentialGenerator.");
+ }
+
+ /**
+ * Lazily loads and returns an instance of the {@link CredentialGenerator}.
This default
+ * implementation uses reflection and a no-argument constructor.
+ *
+ * @param <T> The type of the credential.
+ * @return An instance of the credential generator.
+ * @throws RuntimeException if the generator cannot be instantiated.
+ */
+ @SuppressWarnings("unchecked")
+ default <T extends Credential> CredentialGenerator<T> loadGenerator() {
+ try {
+ Class<?> generatorClass = Class.forName(getGeneratorClassName());
+ Constructor<?> constructor = generatorClass.getDeclaredConstructor();
+ constructor.setAccessible(true); // Allow instantiation of non-public
classes/constructors
Review Comment:
Setting `accessible` to true on constructors can bypass intended access
restrictions and may pose security risks. Consider requiring generators to have
public no-arg constructors instead, which would eliminate the need for
`setAccessible(true)` and make the design more explicit about the requirement.
##########
bundles/azure/src/main/java/org/apache/gravitino/abs/credential/ADLSTokenProvider.java:
##########
@@ -68,70 +48,30 @@ public String credentialType() {
return ADLSTokenCredential.ADLS_TOKEN_CREDENTIAL_TYPE;
}
+ @Override
+ public String getGeneratorClassName() {
+ return "org.apache.gravitino.abs.credential.AzureTokenGenerator";
Review Comment:
The generator class name returned is
`"org.apache.gravitino.abs.credential.AzureTokenGenerator"` but the actual file
is named `ADLSTokenGenerator.java` and the class is `ADLSTokenGenerator`. This
mismatch will cause a `ClassNotFoundException` at runtime. Change to
`"org.apache.gravitino.abs.credential.ADLSTokenGenerator"`.
```suggestion
return "org.apache.gravitino.abs.credential.ADLSTokenGenerator";
```
##########
common/src/main/java/org/apache/gravitino/credential/CredentialProvider.java:
##########
@@ -53,4 +58,37 @@ public interface CredentialProvider extends Closeable {
*/
@Nullable
Credential getCredential(CredentialContext context);
+
+ /**
+ * Returns the fully qualified class name of the {@link CredentialGenerator}
implementation. This
+ * generator will be loaded via reflection to perform the actual credential
creation.
+ *
+ * @return The class name of the credential generator.
+ * @throws UnsupportedOperationException if the provider does not use a
generator.
+ */
+ default String getGeneratorClassName() {
+ throw new UnsupportedOperationException(
+ "This CredentialProvider does not use a CredentialGenerator.");
+ }
+
+ /**
+ * Lazily loads and returns an instance of the {@link CredentialGenerator}.
This default
+ * implementation uses reflection and a no-argument constructor.
+ *
+ * @param <T> The type of the credential.
+ * @return An instance of the credential generator.
+ * @throws RuntimeException if the generator cannot be instantiated.
+ */
+ @SuppressWarnings("unchecked")
+ default <T extends Credential> CredentialGenerator<T> loadGenerator() {
+ try {
+ Class<?> generatorClass = Class.forName(getGeneratorClassName());
+ Constructor<?> constructor = generatorClass.getDeclaredConstructor();
+ constructor.setAccessible(true); // Allow instantiation of non-public
classes/constructors
+ return (CredentialGenerator<T>) constructor.newInstance();
+ } catch (Exception e) {
+ throw new RuntimeException(
+ "Failed to load or instantiate CredentialGenerator: " +
getGeneratorClassName(), e);
Review Comment:
The error message could be more helpful by including the specific exception
type and message. Consider using `"Failed to load or instantiate
CredentialGenerator: " + getGeneratorClassName() + ". Reason: " +
e.getMessage()` to provide more debugging context.
```suggestion
"Failed to load or instantiate CredentialGenerator: " +
getGeneratorClassName()
+ ". Reason: [" + e.getClass().getName() + "] " + e.getMessage(),
e);
```
##########
bundles/aws/src/main/java/org/apache/gravitino/s3/credential/S3TokenProvider.java:
##########
@@ -19,57 +19,30 @@
package org.apache.gravitino.s3.credential;
-import java.net.URI;
-import java.util.Arrays;
-import java.util.HashMap;
import java.util.Map;
-import java.util.Objects;
-import java.util.Set;
-import java.util.stream.Stream;
-import org.apache.commons.lang3.StringUtils;
import org.apache.gravitino.credential.Credential;
import org.apache.gravitino.credential.CredentialContext;
+import org.apache.gravitino.credential.CredentialGenerator;
import org.apache.gravitino.credential.CredentialProvider;
-import org.apache.gravitino.credential.PathBasedCredentialContext;
import org.apache.gravitino.credential.S3TokenCredential;
-import org.apache.gravitino.credential.config.S3CredentialConfig;
-import software.amazon.awssdk.auth.credentials.AwsBasicCredentials;
-import software.amazon.awssdk.auth.credentials.AwsCredentialsProvider;
-import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider;
-import software.amazon.awssdk.policybuilder.iam.IamConditionOperator;
-import software.amazon.awssdk.policybuilder.iam.IamEffect;
-import software.amazon.awssdk.policybuilder.iam.IamPolicy;
-import software.amazon.awssdk.policybuilder.iam.IamResource;
-import software.amazon.awssdk.policybuilder.iam.IamStatement;
-import software.amazon.awssdk.regions.Region;
-import software.amazon.awssdk.services.sts.StsClient;
-import software.amazon.awssdk.services.sts.StsClientBuilder;
-import software.amazon.awssdk.services.sts.model.AssumeRoleRequest;
-import software.amazon.awssdk.services.sts.model.AssumeRoleResponse;
-import software.amazon.awssdk.services.sts.model.Credentials;
-/** Generates S3 token to access S3 data. */
+/**
+ * A lightweight credential provider for S3. It delegates the actual
credential generation to {@link
+ * S3TokenCredentialGenerator} which is loaded via reflection to avoid
classpath issues.
+ */
public class S3TokenProvider implements CredentialProvider {
- private StsClient stsClient;
- private String roleArn;
- private String externalID;
- private int tokenExpireSecs;
+ private Map<String, String> properties;
+ private volatile CredentialGenerator<S3TokenCredential> generator;
@Override
public void initialize(Map<String, String> properties) {
- S3CredentialConfig s3CredentialConfig = new S3CredentialConfig(properties);
- this.roleArn = s3CredentialConfig.s3RoleArn();
- this.externalID = s3CredentialConfig.externalID();
- this.tokenExpireSecs = s3CredentialConfig.tokenExpireInSecs();
- this.stsClient = createStsClient(s3CredentialConfig);
+ this.properties = properties;
}
@Override
public void close() {
- if (stsClient != null) {
- stsClient.close();
- }
+ // No-op, as the generator manages the client lifecycle per-call.
Review Comment:
The comment states "the generator manages the client lifecycle per-call",
but this is an implementation detail of the generator. Consider clarifying that
this comment refers to the `S3TokenCredentialGenerator` implementation, or
simply state "No resources to close in this provider" for consistency with
other providers like `GCSTokenProvider` (line 45 in its file).
```suggestion
// No resources to close in this provider.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
