Copilot commented on code in PR #16553:
URL: https://github.com/apache/pinot/pull/16553#discussion_r2264995090
##########
pinot-plugins/pinot-file-system/pinot-s3/src/main/java/org/apache/pinot/plugin/filesystem/S3PinotFS.java:
##########
@@ -117,78 +120,159 @@ 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() {
+ Preconditions.checkArgument(StringUtils.isNotEmpty(_s3Config.getRegion()),
"Region can't be null or empty");
+
+ _disableAcl = _s3Config.getDisableAcl();
+ setServerSideEncryption(_s3Config.getServerSideEncryption(), _s3Config);
AwsCredentialsProvider awsCredentialsProvider;
try {
- if (StringUtils.isNotEmpty(s3Config.getAccessKey()) &&
StringUtils.isNotEmpty(s3Config.getSecretKey())) {
+ if (StringUtils.isNotEmpty(_s3Config.getAccessKey()) &&
StringUtils.isNotEmpty(_s3Config.getSecretKey())) {
AwsBasicCredentials awsBasicCredentials =
- AwsBasicCredentials.create(s3Config.getAccessKey(),
s3Config.getSecretKey());
+ AwsBasicCredentials.create(_s3Config.getAccessKey(),
_s3Config.getSecretKey());
awsCredentialsProvider =
StaticCredentialsProvider.create(awsBasicCredentials);
- } else if (s3Config.isAnonymousCredentialsProvider()) {
+ } else if (_s3Config.isAnonymousCredentialsProvider()) {
awsCredentialsProvider = AnonymousCredentialsProvider.create();
} else {
awsCredentialsProvider = DefaultCredentialsProvider.builder().build();
}
// IAM Role based access
- if (s3Config.isIamRoleBasedAccess()) {
+ if (_s3Config.isIamRoleBasedAccess()) {
AssumeRoleRequest.Builder assumeRoleRequestBuilder =
-
AssumeRoleRequest.builder().roleArn(s3Config.getRoleArn()).roleSessionName(s3Config.getRoleSessionName())
- .durationSeconds(s3Config.getSessionDurationSeconds());
+
AssumeRoleRequest.builder().roleArn(_s3Config.getRoleArn()).roleSessionName(_s3Config.getRoleSessionName())
+ .durationSeconds(_s3Config.getSessionDurationSeconds());
AssumeRoleRequest assumeRoleRequest;
- if (StringUtils.isNotEmpty(s3Config.getExternalId())) {
- assumeRoleRequest =
assumeRoleRequestBuilder.externalId(s3Config.getExternalId()).build();
+ if (StringUtils.isNotEmpty(_s3Config.getExternalId())) {
+ assumeRoleRequest =
assumeRoleRequestBuilder.externalId(_s3Config.getExternalId()).build();
} else {
assumeRoleRequest = assumeRoleRequestBuilder.build();
}
StsClient stsClient =
-
StsClient.builder().region(Region.of(s3Config.getRegion())).credentialsProvider(awsCredentialsProvider)
+
StsClient.builder().region(Region.of(_s3Config.getRegion())).credentialsProvider(awsCredentialsProvider)
.build();
awsCredentialsProvider =
StsAssumeRoleCredentialsProvider.builder().stsClient(stsClient).refreshRequest(assumeRoleRequest)
-
.asyncCredentialUpdateEnabled(s3Config.isAsyncSessionUpdateEnabled()).build();
+
.asyncCredentialUpdateEnabled(_s3Config.isAsyncSessionUpdateEnabled()).build();
}
- S3ClientBuilder s3ClientBuilder =
S3Client.builder().forcePathStyle(true).region(Region.of(s3Config.getRegion()))
-
.credentialsProvider(awsCredentialsProvider).crossRegionAccessEnabled(s3Config.isCrossRegionAccessEnabled());
- if (StringUtils.isNotEmpty(s3Config.getEndpoint())) {
+ S3ClientBuilder s3ClientBuilder =
S3Client.builder().forcePathStyle(true).region(Region.of(_s3Config.getRegion()))
+
.credentialsProvider(awsCredentialsProvider).crossRegionAccessEnabled(_s3Config.isCrossRegionAccessEnabled());
+ if (StringUtils.isNotEmpty(_s3Config.getEndpoint())) {
try {
- s3ClientBuilder.endpointOverride(new URI(s3Config.getEndpoint()));
+ s3ClientBuilder.endpointOverride(new URI(_s3Config.getEndpoint()));
} catch (URISyntaxException e) {
throw new RuntimeException(e);
}
}
- if (s3Config.getHttpClientBuilder() != null) {
- s3ClientBuilder.httpClientBuilder(s3Config.getHttpClientBuilder());
+ if (_s3Config.getHttpClientBuilder() != null) {
+ s3ClientBuilder.httpClientBuilder(_s3Config.getHttpClientBuilder());
}
- if (s3Config.getStorageClass() != null) {
- _storageClass = StorageClass.fromValue(s3Config.getStorageClass());
+ if (_s3Config.getStorageClass() != null) {
+ _storageClass = StorageClass.fromValue(_s3Config.getStorageClass());
assert (_storageClass != StorageClass.UNKNOWN_TO_SDK_VERSION);
}
- if (s3Config.getRequestChecksumCalculationWhenRequired() ==
RequestChecksumCalculation.WHEN_REQUIRED) {
+ if (_s3Config.getRequestChecksumCalculationWhenRequired() ==
RequestChecksumCalculation.WHEN_REQUIRED) {
s3ClientBuilder.responseChecksumValidation(ResponseChecksumValidation.WHEN_REQUIRED);
}
- if (s3Config.getResponseChecksumValidationWhenRequired() ==
ResponseChecksumValidation.WHEN_REQUIRED) {
+ if (_s3Config.getResponseChecksumValidationWhenRequired() ==
ResponseChecksumValidation.WHEN_REQUIRED) {
s3ClientBuilder.requestChecksumCalculation(RequestChecksumCalculation.WHEN_REQUIRED);
}
- if (s3Config.useLegacyMd5Plugin()) {
+ if (_s3Config.useLegacyMd5Plugin()) {
s3ClientBuilder.addPlugin(LegacyMd5Plugin.create());
}
_s3Client = s3ClientBuilder.build();
- setMultiPartUploadConfigs(s3Config);
+ setMultiPartUploadConfigs(_s3Config);
} catch (S3Exception e) {
throw new RuntimeException("Could not initialize S3PinotFS", e);
}
}
+ /**
+ * Masks a sensitive key, showing only the first and last 3 characters, with
the middle characters replaced by '*'.
+ * If the key is null or shorter than or equal to 6 characters, returns
"***".
+ *
+ * @param key the sensitive key string to mask
+ * @return the masked key string
+ */
+ 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();
+ }
+
+ private void logAwsCredentials(String when) {
+ AwsCredentials credentials = getAwsCredentials();
+ if (credentials != null) {
+ LOGGER.warn("S3 credentials {} - Access Key: {}", when,
maskKey(credentials.accessKeyId()));
+ LOGGER.warn("S3 credentials {} - Secret Key: {}", when,
maskKey(credentials.secretAccessKey()));
+ } else {
+ LOGGER.warn("S3 credentials {} - Unable to retrieve AWS credentials
(access key & secret key unavailable)", when);
Review Comment:
Logging AWS credentials (even masked) at WARN level poses a security risk as
it exposes partial credential information in logs. Consider using DEBUG level
or removing credential logging entirely.
```suggestion
LOGGER.debug("S3 credentials {} - Access Key: {}", when,
maskKey(credentials.accessKeyId()));
LOGGER.debug("S3 credentials {} - Secret Key: {}", when,
maskKey(credentials.secretAccessKey()));
} else {
LOGGER.debug("S3 credentials {} - Unable to retrieve AWS credentials
(access key & secret key unavailable)", when);
```
##########
pinot-plugins/pinot-file-system/pinot-s3/src/main/java/org/apache/pinot/plugin/filesystem/S3PinotFS.java:
##########
@@ -499,18 +585,20 @@ public boolean delete(URI segmentUri, boolean forceDelete)
for (S3Object s3Object : listObjectsV2Response.contents()) {
DeleteObjectRequest deleteObjectRequest =
DeleteObjectRequest.builder().bucket(segmentUri.getHost()).key(s3Object.key()).build();
-
- DeleteObjectResponse deleteObjectResponse =
_s3Client.deleteObject(deleteObjectRequest);
+ DeleteObjectResponse deleteObjectResponse =
retryWithS3CredentialRefresh(() ->
+ _s3Client.deleteObject(deleteObjectRequest));
deleteSucceeded &=
deleteObjectResponse.sdkHttpResponse().isSuccessful();
+ LOGGER.error("delete response result {}", deleteSucceeded);
Review Comment:
This debug log statement uses ERROR level inappropriately and appears to be
leftover debug code. It should be removed or changed to DEBUG level with a more
descriptive message.
```suggestion
LOGGER.debug("Delete operation for object '{}' in bucket '{}'
succeeded: {}", s3Object.key(), segmentUri.getHost(),
deleteObjectResponse.sdkHttpResponse().isSuccessful());
```
##########
pinot-plugins/pinot-file-system/pinot-s3/src/main/java/org/apache/pinot/plugin/filesystem/S3PinotFS.java:
##########
@@ -490,7 +576,7 @@ public boolean delete(URI segmentUri, boolean forceDelete)
if (prefix.equals(DELIMITER)) {
ListObjectsV2Request listObjectsV2Request =
listObjectsV2RequestBuilder.build();
- listObjectsV2Response =
_s3Client.listObjectsV2(listObjectsV2Request);
+ listObjectsV2Response = retryWithS3CredentialRefresh(() ->
_s3Client.listObjectsV2(listObjectsV2Request));
} else {
ListObjectsV2Request listObjectsV2Request =
listObjectsV2RequestBuilder.prefix(prefix).build();
listObjectsV2Response =
_s3Client.listObjectsV2(listObjectsV2Request);
Review Comment:
This S3 operation is not wrapped with retryWithS3CredentialRefresh() unlike
other similar operations in the same method, which means it won't benefit from
automatic credential refresh on auth failures.
```suggestion
listObjectsV2Response = retryWithS3CredentialRefresh(() ->
_s3Client.listObjectsV2(listObjectsV2Request));
```
--
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]
