Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Im 542 get s3 region by analyzing the asset href #224

Merged
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,15 @@
import java.util.List;
import java.util.stream.Collectors;

import org.integratedmodelling.klab.exceptions.KlabResourceAccessException;

import software.amazon.awssdk.auth.credentials.AnonymousCredentialsProvider;
import software.amazon.awssdk.http.apache.ApacheHttpClient;
import software.amazon.awssdk.regions.Region;
import software.amazon.awssdk.services.s3.S3Client;
import software.amazon.awssdk.services.s3.model.GetBucketLocationRequest;
import software.amazon.awssdk.services.s3.model.GetBucketLocationResponse;
import software.amazon.awssdk.services.s3.model.HeadObjectRequest;
import software.amazon.awssdk.services.s3.model.S3Exception;

public class S3RegionResolver {

Expand All @@ -25,8 +26,6 @@ public static Region resolveBucketRegion(String bucketName, String objectKey) {
.credentialsProvider(AnonymousCredentialsProvider.create()) // Anonymous credentials
.build()) {

System.out.println("Attempting to resolve region dynamically for bucket: " + bucketName);

GetBucketLocationRequest request = GetBucketLocationRequest.builder()
.bucket(bucketName)
.build();
Expand All @@ -36,19 +35,21 @@ public static Region resolveBucketRegion(String bucketName, String objectKey) {

// Handle "null" or "global" regions
if (location == null || location.equalsIgnoreCase("null")) {
System.out.println("Bucket region resolved dynamically to: us-east-1");
return Region.US_EAST_1;
}

Region resolvedRegion = Region.of(location);
System.out.println("Bucket region resolved dynamically to: " + resolvedRegion);
return resolvedRegion;

} catch (Exception e) {
System.err.println("Failed to resolve region dynamically for bucket: " + bucketName + ". Error: " + e.getMessage());
// try another one
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This catch is very general, could it be interesting to write a warning to the log file?

}

// Step 2: Iterate through all regions and test lightweight requests
return resolveRegionByTesting(bucketName, objectKey);
}

private static List<Region> getAwsRegions() {
// List of regions to exclude (e.g., isolated regions or restricted access regions)
List<Region> excludedRegions = List.of(
Region.US_ISO_EAST_1, // Restricted to isolated networks
Expand All @@ -58,22 +59,14 @@ public static Region resolveBucketRegion(String bucketName, String objectKey) {
);

// Get the list of all AWS regions, excluding problematic ones
List<Region> regions = Region.regions().stream()
return Region.regions().stream()
.filter(region -> !excludedRegions.contains(region))
.collect(Collectors.toList());

System.out.println("Falling back to region testing for bucket: " + bucketName);


// Step 2: Iterate through all regions and test lightweight requests
return resolveRegionByTesting(bucketName, objectKey);
}

private static Region resolveRegionByTesting(String bucketName, String objectKey) {
// Get the list of all AWS regions
List<Region> regions = Region.regions();

System.out.println("Falling back to region testing for bucket: " + bucketName);
List<Region> regions = getAwsRegions();

// Iterate through regions to perform a lightweight test
for (Region region : regions) {
Expand All @@ -93,17 +86,12 @@ private static Region resolveRegionByTesting(String bucketName, String objectKey
s3.headObject(request); // If no exception is thrown, the region is correct
System.out.println("Region confirmed by testing: " + region);
return region;

} catch (S3Exception e) {
// Continue testing other regions if the bucket is not found
if (e.statusCode() != 403 && e.statusCode() != 404) {
System.err.println("Error testing region " + region + ": " + e.awsErrorDetails().errorMessage());
}
} catch (Exception e) {
System.err.println("Exception testing region " + region + ": " + e.getMessage());
// Continue testing other regions if the bucket is not found
continue;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This catch is very general, could it be interesting to write a warning to the log file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure about adding the logging class here, as the loop will fail most times. I could manage the exceptions in a more specialized way.

}
}

throw new RuntimeException("Unable to resolve region for bucket: " + bucketName);
throw new KlabResourceAccessException("Unable to resolve region for bucket: " + bucketName);
}
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.integratedmodelling.klab.test.utils;

import org.integratedmodelling.klab.exceptions.KlabResourceAccessException;
import org.integratedmodelling.klab.utils.s3.S3RegionResolver;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
Expand All @@ -19,11 +20,12 @@ public void resolveBucketRegion_resolutionSuccessful() {
}

@Test
// Warning: a relatively costly test (sometimes over 30 seconds)
public void resolveBucketRegion_resolutionUnsuccessful() {
String bucket = "fake-bucket";
String objectKey = "fake-object.tif";

Assertions.assertThrows(RuntimeException.class, () -> {
Assertions.assertThrows(KlabResourceAccessException.class, () -> {
S3RegionResolver.resolveBucketRegion(bucket, objectKey);
});
}
Expand Down