Copilot commented on code in PR #16553:
URL: https://github.com/apache/pinot/pull/16553#discussion_r2261836389
##########
pinot-plugins/pinot-file-system/pinot-s3/src/main/java/org/apache/pinot/plugin/filesystem/S3PinotFS.java:
##########
@@ -690,16 +696,50 @@ private void visitFiles(URI fileUri, boolean recursive,
Consumer<S3Object> objec
}
}
+ private static String maskKey(String key) {
+ if (key == null || key.length() <= 6) {
+ return "***";
+ }
+ int maskLength = key.length() - 6;
+ StringBuilder sb = new StringBuilder();
+ sb.append(key, 0, 3);
+ for (int i = 0; i < maskLength; i++) {
+ sb.append("*");
+ }
+ sb.append(key, key.length() - 3, key.length());
+ return sb.toString();
+ }
+
+ public AwsCredentials getAwsCredentials() {
+ AwsCredentialsProvider credentialsProvider = (AwsCredentialsProvider)
_s3Client
+ .serviceClientConfiguration()
+ .credentialsProvider();
+ return credentialsProvider.resolveCredentials();
Review Comment:
Unsafe cast without type checking. The
serviceClientConfiguration().credentialsProvider() may not return an
AwsCredentialsProvider type, which could cause a ClassCastException.
```suggestion
Object provider =
_s3Client.serviceClientConfiguration().credentialsProvider();
if (provider instanceof AwsCredentialsProvider) {
return ((AwsCredentialsProvider) provider).resolveCredentials();
} else {
throw new IllegalStateException("S3 client credentialsProvider is not
an AwsCredentialsProvider: " +
(provider != null ? provider.getClass().getName() : "null"));
}
```
##########
pinot-plugins/pinot-file-system/pinot-s3/src/main/java/org/apache/pinot/plugin/filesystem/S3PinotFS.java:
##########
@@ -117,73 +119,77 @@ public class S3PinotFS extends BasePinotFS {
@Override
public void init(PinotConfiguration config) {
- S3Config s3Config = new S3Config(config);
- Preconditions.checkArgument(StringUtils.isNotEmpty(s3Config.getRegion()),
"Region can't be null or empty");
+ _s3Config = new S3Config(config);
+ initOrRefreshS3Client();
+ }
- _disableAcl = s3Config.getDisableAcl();
- setServerSideEncryption(s3Config.getServerSideEncryption(), s3Config);
+ public void initOrRefreshS3Client() {
Review Comment:
Method is not thread-safe. Multiple threads calling this method concurrently
could lead to race conditions when modifying _s3Client and other instance
variables.
```suggestion
public synchronized void initOrRefreshS3Client() {
```
##########
pinot-plugins/pinot-file-system/pinot-s3/src/main/java/org/apache/pinot/plugin/filesystem/S3PinotFS.java:
##########
@@ -690,16 +696,50 @@ private void visitFiles(URI fileUri, boolean recursive,
Consumer<S3Object> objec
}
}
+ private static String maskKey(String key) {
+ if (key == null || key.length() <= 6) {
+ return "***";
+ }
+ int maskLength = key.length() - 6;
+ StringBuilder sb = new StringBuilder();
+ sb.append(key, 0, 3);
+ for (int i = 0; i < maskLength; i++) {
+ sb.append("*");
+ }
+ sb.append(key, key.length() - 3, key.length());
+ return sb.toString();
+ }
+
+ public AwsCredentials getAwsCredentials() {
+ AwsCredentialsProvider credentialsProvider = (AwsCredentialsProvider)
_s3Client
+ .serviceClientConfiguration()
+ .credentialsProvider();
+ return credentialsProvider.resolveCredentials();
+ }
+
@Override
public void copyToLocalFile(URI srcUri, File dstFile)
throws Exception {
- LOGGER.info("Copy {} to local {}", srcUri, dstFile.getAbsolutePath());
- URI base = getBase(srcUri);
- FileUtils.forceMkdir(dstFile.getParentFile());
- String prefix = sanitizePath(base.relativize(srcUri).getPath());
- GetObjectRequest getObjectRequest =
GetObjectRequest.builder().bucket(srcUri.getHost()).key(prefix).build();
+ try {
+ LOGGER.info("Copy {} to local {}", srcUri, dstFile.getAbsolutePath());
+ URI base = getBase(srcUri);
+ FileUtils.forceMkdir(dstFile.getParentFile());
+ String prefix = sanitizePath(base.relativize(srcUri).getPath());
+ GetObjectRequest getObjectRequest =
GetObjectRequest.builder().bucket(srcUri.getHost()).key(prefix).build();
- _s3Client.getObject(getObjectRequest, ResponseTransformer.toFile(dstFile));
+ _s3Client.getObject(getObjectRequest,
ResponseTransformer.toFile(dstFile));
+ } catch (Exception e) {
+ AwsCredentials credentials = getAwsCredentials();
+ LOGGER.warn("S3 credentials BEFORE refresh - Access Key: {}",
maskKey(credentials.accessKeyId()));
+ LOGGER.warn("S3 credentials BEFORE refresh - Secret Key: {}",
maskKey(credentials.secretAccessKey()));
+
+ initOrRefreshS3Client();
+
+ credentials = getAwsCredentials();
+ LOGGER.warn("S3 credentials AFTER refresh - Access Key: {}",
maskKey(credentials.accessKeyId()));
+ LOGGER.warn("S3 credentials AFTER refresh - Secret Key: {}",
maskKey(credentials.secretAccessKey()));
+ throw new Exception("Unexpected exception during S3 copy to local file",
e);
Review Comment:
The original exception is caught but after credential refresh, the original
operation is not retried. This will always throw an exception even if the
credential refresh was successful.
```suggestion
// Retry the operation after refreshing credentials
try {
URI base = getBase(srcUri);
FileUtils.forceMkdir(dstFile.getParentFile());
String prefix = sanitizePath(base.relativize(srcUri).getPath());
GetObjectRequest getObjectRequest =
GetObjectRequest.builder().bucket(srcUri.getHost()).key(prefix).build();
_s3Client.getObject(getObjectRequest,
ResponseTransformer.toFile(dstFile));
} catch (Exception retryException) {
throw new Exception("Unexpected exception during S3 copy to local
file after credential refresh", retryException);
}
```
##########
pinot-plugins/pinot-file-system/pinot-s3/src/main/java/org/apache/pinot/plugin/filesystem/S3PinotFS.java:
##########
@@ -47,6 +47,7 @@
import org.slf4j.LoggerFactory;
import software.amazon.awssdk.auth.credentials.AnonymousCredentialsProvider;
import software.amazon.awssdk.auth.credentials.AwsBasicCredentials;
+import software.amazon.awssdk.auth.credentials.AwsCredentials;
Review Comment:
[nitpick] The AwsCredentials import is only used in the getAwsCredentials()
method. Consider moving this import closer to its usage or evaluating if this
dependency is necessary.
##########
pinot-plugins/pinot-file-system/pinot-s3/src/main/java/org/apache/pinot/plugin/filesystem/S3PinotFS.java:
##########
@@ -690,16 +696,50 @@ private void visitFiles(URI fileUri, boolean recursive,
Consumer<S3Object> objec
}
}
+ private static String maskKey(String key) {
+ if (key == null || key.length() <= 6) {
+ return "***";
+ }
+ int maskLength = key.length() - 6;
+ StringBuilder sb = new StringBuilder();
+ sb.append(key, 0, 3);
+ for (int i = 0; i < maskLength; i++) {
+ sb.append("*");
+ }
+ sb.append(key, key.length() - 3, key.length());
+ return sb.toString();
+ }
+
+ public AwsCredentials getAwsCredentials() {
+ AwsCredentialsProvider credentialsProvider = (AwsCredentialsProvider)
_s3Client
+ .serviceClientConfiguration()
+ .credentialsProvider();
+ return credentialsProvider.resolveCredentials();
+ }
+
@Override
public void copyToLocalFile(URI srcUri, File dstFile)
throws Exception {
- LOGGER.info("Copy {} to local {}", srcUri, dstFile.getAbsolutePath());
- URI base = getBase(srcUri);
- FileUtils.forceMkdir(dstFile.getParentFile());
- String prefix = sanitizePath(base.relativize(srcUri).getPath());
- GetObjectRequest getObjectRequest =
GetObjectRequest.builder().bucket(srcUri.getHost()).key(prefix).build();
+ try {
+ LOGGER.info("Copy {} to local {}", srcUri, dstFile.getAbsolutePath());
+ URI base = getBase(srcUri);
+ FileUtils.forceMkdir(dstFile.getParentFile());
+ String prefix = sanitizePath(base.relativize(srcUri).getPath());
+ GetObjectRequest getObjectRequest =
GetObjectRequest.builder().bucket(srcUri.getHost()).key(prefix).build();
- _s3Client.getObject(getObjectRequest, ResponseTransformer.toFile(dstFile));
+ _s3Client.getObject(getObjectRequest,
ResponseTransformer.toFile(dstFile));
+ } catch (Exception e) {
+ AwsCredentials credentials = getAwsCredentials();
+ LOGGER.warn("S3 credentials BEFORE refresh - Access Key: {}",
maskKey(credentials.accessKeyId()));
+ LOGGER.warn("S3 credentials BEFORE refresh - Secret Key: {}",
maskKey(credentials.secretAccessKey()));
+
+ initOrRefreshS3Client();
+
+ credentials = getAwsCredentials();
+ LOGGER.warn("S3 credentials AFTER refresh - Access Key: {}",
maskKey(credentials.accessKeyId()));
+ LOGGER.warn("S3 credentials AFTER refresh - Secret Key: {}",
maskKey(credentials.secretAccessKey()));
Review Comment:
Logging credentials (even masked) in warning level may expose sensitive
information in logs. Consider using debug level or removing credential logging
entirely.
```suggestion
LOGGER.debug("S3 credentials BEFORE refresh - Access Key: {}",
maskKey(credentials.accessKeyId()));
LOGGER.debug("S3 credentials BEFORE refresh - Secret Key: {}",
maskKey(credentials.secretAccessKey()));
initOrRefreshS3Client();
credentials = getAwsCredentials();
LOGGER.debug("S3 credentials AFTER refresh - Access Key: {}",
maskKey(credentials.accessKeyId()));
LOGGER.debug("S3 credentials AFTER refresh - Secret Key: {}",
maskKey(credentials.secretAccessKey()));
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
