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.

Sign up for GitHub

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

Fix #1402 Migrating AWS S3 SDK to v2 #1403

Merged
merged 1 commit into from
Dec 6, 2024
Merged

Fix #1402 Migrating AWS S3 SDK to v2 #1403

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions bolt-jakarta-servlet/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,8 @@
<scope>test</scope>
</dependency>
<dependency>
<groupId>com.amazonaws</groupId>
<artifactId>aws-java-sdk-s3</artifactId>
<groupId>software.amazon.awssdk</groupId>
<artifactId>s3</artifactId>
<version>${aws.s3.version}</version>
<scope>test</scope>
</dependency>
Expand Down
4 changes: 2 additions & 2 deletions bolt-kotlin-examples/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,8 @@
</dependency>

<dependency>
<groupId>com.amazonaws</groupId>
<artifactId>aws-java-sdk-s3</artifactId>
<groupId>software.amazon.awssdk</groupId>
<artifactId>s3</artifactId>
<version>${aws.s3.version}</version>
</dependency>

Expand Down
4 changes: 2 additions & 2 deletions bolt-servlet/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,8 @@
<scope>test</scope>
</dependency>
<dependency>
<groupId>com.amazonaws</groupId>
<artifactId>aws-java-sdk-s3</artifactId>
<groupId>software.amazon.awssdk</groupId>
<artifactId>s3</artifactId>
<version>${aws.s3.version}</version>
<scope>test</scope>
</dependency>
Expand Down
4 changes: 2 additions & 2 deletions bolt/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,8 @@
</dependency>

<dependency>
<groupId>com.amazonaws</groupId>
<artifactId>aws-java-sdk-s3</artifactId>
<groupId>software.amazon.awssdk</groupId>
<artifactId>s3</artifactId>
<version>${aws.s3.version}</version>
<scope>provided</scope>
</dependency>
Expand Down
169 changes: 101 additions & 68 deletions bolt/src/main/java/com/slack/api/bolt/service/builtin/AmazonS3InstallationService.java
View file
Open in desktop

Large diffs are not rendered by default.

90 changes: 54 additions & 36 deletions bolt/src/main/java/com/slack/api/bolt/service/builtin/AmazonS3OAuthStateService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,18 @@
package com.slack.api.bolt.service.builtin;

import com.amazonaws.auth.AWSCredentials;
import com.amazonaws.auth.DefaultAWSCredentialsProviderChain;
import com.amazonaws.services.s3.AmazonS3;
import com.amazonaws.services.s3.AmazonS3ClientBuilder;
import com.amazonaws.services.s3.model.AmazonS3Exception;
import com.amazonaws.services.s3.model.PutObjectResult;
import com.amazonaws.services.s3.model.S3Object;
import com.amazonaws.util.IOUtils;
import com.slack.api.bolt.Initializer;
import com.slack.api.bolt.service.OAuthStateService;
import com.slack.api.bolt.util.JsonOps;
import lombok.extern.slf4j.Slf4j;
import software.amazon.awssdk.auth.credentials.AwsCredentials;
import software.amazon.awssdk.auth.credentials.AwsCredentialsProvider;
import software.amazon.awssdk.auth.credentials.DefaultCredentialsProvider;
import software.amazon.awssdk.core.ResponseBytes;
import software.amazon.awssdk.core.sync.RequestBody;
import software.amazon.awssdk.core.sync.ResponseTransformer;
import software.amazon.awssdk.services.s3.S3Client;
import software.amazon.awssdk.services.s3.model.*;

import java.io.IOException;
import java.nio.charset.StandardCharsets;

/**
* OAuthStateService implementation using Amazon S3.
Expand All @@ -24,6 +23,7 @@
public class AmazonS3OAuthStateService implements OAuthStateService {

private final String bucketName;
private AwsCredentialsProvider credentialsProvider;

public AmazonS3OAuthStateService(String bucketName) {
this.bucketName = bucketName;
Expand All @@ -33,74 +33,92 @@
public Initializer initializer() {
return (app) -> {
// The first access to S3 tends to be slow on AWS Lambda.
AWSCredentials credentials = getCredentials();
if (credentials == null || credentials.getAWSAccessKeyId() == null) {
this.credentialsProvider = DefaultCredentialsProvider.create();
AwsCredentials credentials = createCredentials(credentialsProvider);
if (credentials == null || credentials.accessKeyId() == null) {
throw new IllegalStateException("AWS credentials not found");
}
if (log.isDebugEnabled()) {
log.debug("AWS credentials loaded (access key id: {})", credentials.getAWSAccessKeyId());
log.debug("AWS credentials loaded (access key id: {})", credentials.accessKeyId());
}
boolean bucketExists = false;
Exception ex = null;
try (S3Client s3 = createS3Client()) {
bucketExists = s3.headBucket(HeadBucketRequest.builder().bucket(bucketName).build()) != null;
Copy link

Choose a reason for hiding this comment

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

@seratch San: Do you think that this implementation from the AWS is better than then using HeadBucketRequest

Copy link
Member Author

Choose a reason for hiding this comment

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

@acharyashashank Thanks for the suggestion!

I switched to headBucket because the AWS migration guide recommends using the API in version 2: https://docs.aws.amazon.com/sdk-for-java/latest/developer-guide/migration-service-changes.html. Do you think using getBucketAcl instead could bring benefits such as faster runtime performance?

Even if I use the code snippet provided there, I won't follow AwsServiceException error handling because the goal of this code is to verify if the client has permissions to fetch/modify S3 bucket data, rather than checking the bucket's existence.

} catch (Exception e) { // NoSuchBucketException etc.
ex = e;

Check warning on line 49 in bolt/src/main/java/com/slack/api/bolt/service/builtin/AmazonS3OAuthStateService.java

View check run for this annotation

Codecov / codecov/patch

bolt/src/main/java/com/slack/api/bolt/service/builtin/AmazonS3OAuthStateService.java#L48-L49 

Added lines #L48 - L49 were not covered by tests
}
boolean bucketExists = createS3Client().doesBucketExistV2(bucketName);
if (!bucketExists) {
throw new IllegalStateException("Failed to access the Amazon S3 bucket (name: " + bucketName + ")");
String error = ex != null ? ex.getClass().getName() + ":" + ex.getMessage() : "-";
String message = "Failed to access the Amazon S3 bucket (name: " + bucketName + ", error: " + error + ")";
throw new IllegalStateException(message);

Check warning on line 54 in bolt/src/main/java/com/slack/api/bolt/service/builtin/AmazonS3OAuthStateService.java

View check run for this annotation

Codecov / codecov/patch

bolt/src/main/java/com/slack/api/bolt/service/builtin/AmazonS3OAuthStateService.java#L53-L54 

Added lines #L53 - L54 were not covered by tests
}
};
}

@Override
public void addNewStateToDatastore(String state) throws Exception {
AmazonS3 s3 = this.createS3Client();
String value = "" + (System.currentTimeMillis() + getExpirationInSeconds() * 1000);
PutObjectResult putObjectResult = s3.putObject(bucketName, getKey(state), value);
PutObjectResponse putObjectResult;
try (S3Client s3 = this.createS3Client()) {
String value = "" + (System.currentTimeMillis() + getExpirationInSeconds() * 1000);
putObjectResult = s3.putObject(
PutObjectRequest.builder().bucket(bucketName).key(getKey(state)).build(),
RequestBody.fromString(value)
);
}
if (log.isDebugEnabled()) {
log.debug("AWS S3 putObject result of state data - {}", JsonOps.toJsonString(putObjectResult));
log.debug("AWS S3 putObject result of state data - {}", putObjectResult.toString());
}
}

@Override
public boolean isAvailableInDatabase(String state) {
AmazonS3 s3 = this.createS3Client();
S3Object s3Object = getObject(s3, getKey(state));
S3Client s3 = this.createS3Client();
ResponseBytes<GetObjectResponse> s3Object = getObject(s3, getKey(state));
if (s3Object == null) {
return false;
}
String millisToExpire = null;
try {
millisToExpire = IOUtils.toString(s3Object.getObjectContent());
return Long.valueOf(millisToExpire) > System.currentTimeMillis();
} catch (IOException e) {
log.error("Failed to load a state data for state: {}", state, e);
return false;
millisToExpire = s3Object.asString(StandardCharsets.UTF_8);
return Long.parseLong(millisToExpire) > System.currentTimeMillis();
} catch (NumberFormatException ne) {
log.error("Invalid state value detected - state: {}, millisToExpire: {}", state, millisToExpire);
return false;
} catch (Exception e) {
log.error("Failed to load a state data for state: {}", state, e);
return false;

Check warning on line 90 in bolt/src/main/java/com/slack/api/bolt/service/builtin/AmazonS3OAuthStateService.java

View check run for this annotation

Codecov / codecov/patch

bolt/src/main/java/com/slack/api/bolt/service/builtin/AmazonS3OAuthStateService.java#L88-L90 

Added lines #L88 - L90 were not covered by tests
}
}

@Override
public void deleteStateFromDatastore(String state) throws Exception {
AmazonS3 s3 = this.createS3Client();
s3.deleteObject(bucketName, getKey(state));
try (S3Client s3 = this.createS3Client()) {
s3.deleteObject(DeleteObjectRequest.builder().bucket(bucketName).key(getKey(state)).build());
}
}

protected AWSCredentials getCredentials() {
return DefaultAWSCredentialsProviderChain.getInstance().getCredentials();
protected AwsCredentials createCredentials(AwsCredentialsProvider provider) {
return provider.resolveCredentials();

Check warning on line 102 in bolt/src/main/java/com/slack/api/bolt/service/builtin/AmazonS3OAuthStateService.java

View check run for this annotation

Codecov / codecov/patch

bolt/src/main/java/com/slack/api/bolt/service/builtin/AmazonS3OAuthStateService.java#L102 

Added line #L102 was not covered by tests
}

protected AmazonS3 createS3Client() {
return AmazonS3ClientBuilder.defaultClient();
protected S3Client createS3Client() {
return S3Client.builder().credentialsProvider(this.credentialsProvider).build();

Check warning on line 106 in bolt/src/main/java/com/slack/api/bolt/service/builtin/AmazonS3OAuthStateService.java

View check run for this annotation

Codecov / codecov/patch

bolt/src/main/java/com/slack/api/bolt/service/builtin/AmazonS3OAuthStateService.java#L106 

Added line #L106 was not covered by tests
}

private String getKey(String state) {
return "state/" + state;
}

private S3Object getObject(AmazonS3 s3, String fullKey) {
private ResponseBytes<GetObjectResponse> getObject(S3Client s3, String fullKey) {
try {
return s3.getObject(bucketName, fullKey);
} catch (AmazonS3Exception e) {
return s3.getObject(
GetObjectRequest.builder().bucket(bucketName).key(fullKey).build(),
ResponseTransformer.toBytes()
);
} catch (Exception e) {

Check warning on line 119 in bolt/src/main/java/com/slack/api/bolt/service/builtin/AmazonS3OAuthStateService.java

View check run for this annotation

Codecov / codecov/patch

bolt/src/main/java/com/slack/api/bolt/service/builtin/AmazonS3OAuthStateService.java#L119 

Added line #L119 was not covered by tests
if (log.isDebugEnabled()) {
log.debug("Amazon S3 object metadata not found (key: {}, AmazonS3Exception: {})", fullKey, e.toString());
log.debug("Amazon S3 object metadata not found (key: {}, Exception: {})", fullKey, e.toString());

Check warning on line 121 in bolt/src/main/java/com/slack/api/bolt/service/builtin/AmazonS3OAuthStateService.java

View check run for this annotation

Codecov / codecov/patch

bolt/src/main/java/com/slack/api/bolt/service/builtin/AmazonS3OAuthStateService.java#L121 

Added line #L121 was not covered by tests
}
return null;
}
Expand Down
98 changes: 39 additions & 59 deletions bolt/src/test/java/test_locally/service/AmazonS3InstallationServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,47 +1,44 @@
package test_locally.service;

import com.amazonaws.SdkClientException;
import com.amazonaws.auth.AWSCredentials;
import com.amazonaws.services.s3.AmazonS3;
import com.slack.api.bolt.model.builtin.DefaultBot;
import com.slack.api.bolt.model.builtin.DefaultInstaller;
import com.slack.api.bolt.service.builtin.AmazonS3InstallationService;
import org.junit.Test;
import software.amazon.awssdk.auth.credentials.AwsCredentials;
import software.amazon.awssdk.auth.credentials.AwsCredentialsProvider;
import software.amazon.awssdk.core.ResponseBytes;
import software.amazon.awssdk.core.sync.RequestBody;
import software.amazon.awssdk.core.sync.ResponseTransformer;
import software.amazon.awssdk.services.s3.S3Client;
import software.amazon.awssdk.services.s3.model.*;

import java.nio.charset.StandardCharsets;

import static org.junit.Assert.assertNotNull;
import static org.mockito.Mockito.*;

public class AmazonS3InstallationServiceTest {

@Test(expected = IllegalStateException.class)
public void initializer_no_credentials() {
AmazonS3 s3 = mock(AmazonS3.class);
AmazonS3InstallationService service = new AmazonS3InstallationService("test-bucket") {
@Override
protected AWSCredentials getCredentials() {
return null;
}

@Override
protected AmazonS3 createS3Client() {
return s3;
}
};
service.initializer().accept(null);
static S3Client setupMocks(S3Client s3) {
when(s3.headBucket((HeadBucketRequest) notNull())).thenReturn(HeadBucketResponse.builder().build());
when(s3.headObject((HeadObjectRequest) notNull())).thenReturn(HeadObjectResponse.builder().build());
when(s3.getObject((GetObjectRequest) notNull(), (ResponseTransformer<GetObjectResponse, ResponseBytes<GetObjectResponse>>) notNull()))
.thenReturn(ResponseBytes.fromByteArray(GetObjectResponse.builder().build(), "{}".getBytes(StandardCharsets.UTF_8)));
when(s3.putObject((PutObjectRequest) notNull(), (RequestBody) notNull())).thenReturn(PutObjectResponse.builder().build());
return s3;
}

@Test(expected = IllegalStateException.class)
public void initializer_no_bucket() {
AWSCredentials credentials = mock(AWSCredentials.class);
AmazonS3 s3 = mock(AmazonS3.class);
public void initializer_no_credentials() {
S3Client s3 = setupMocks(mock(S3Client.class));
AmazonS3InstallationService service = new AmazonS3InstallationService("test-bucket") {
@Override
protected AWSCredentials getCredentials() {
return credentials;
protected AwsCredentials createCredentials(AwsCredentialsProvider provider) {
return null;
}

@Override
protected AmazonS3 createS3Client() {
protected S3Client createS3Client() {
return s3;
}
};
Expand All @@ -50,19 +47,18 @@ protected AmazonS3 createS3Client() {

@Test
public void initializer() {
AWSCredentials credentials = mock(AWSCredentials.class);
when(credentials.getAWSAccessKeyId()).thenReturn("valid key");
AmazonS3 s3 = mock(AmazonS3.class);
when(s3.doesBucketExistV2(anyString())).thenReturn(true);
AwsCredentials credentials = mock(AwsCredentials.class);
when(credentials.accessKeyId()).thenReturn("valid key");
S3Client s3 = setupMocks(mock(S3Client.class));

AmazonS3InstallationService service = new AmazonS3InstallationService("test-bucket") {
@Override
protected AWSCredentials getCredentials() {
protected AwsCredentials createCredentials(AwsCredentialsProvider provider) {
return credentials;
}

@Override
protected AmazonS3 createS3Client() {
protected S3Client createS3Client() {
return s3;
}
};
Expand All @@ -71,19 +67,18 @@ protected AmazonS3 createS3Client() {

@Test
public void operations() throws Exception {
AWSCredentials credentials = mock(AWSCredentials.class);
when(credentials.getAWSAccessKeyId()).thenReturn("valid key");
AmazonS3 s3 = mock(AmazonS3.class);
when(s3.doesBucketExistV2(anyString())).thenReturn(true);
AwsCredentials credentials = mock(AwsCredentials.class);
when(credentials.accessKeyId()).thenReturn("valid key");
S3Client s3 = setupMocks(mock(S3Client.class));

AmazonS3InstallationService service = new AmazonS3InstallationService("test-bucket") {
@Override
protected AWSCredentials getCredentials() {
protected AwsCredentials createCredentials(AwsCredentialsProvider provider) {
return credentials;
}

@Override
protected AmazonS3 createS3Client() {
protected S3Client createS3Client() {
return s3;
}
};
Expand All @@ -98,19 +93,18 @@ protected AmazonS3 createS3Client() {

@Test
public void operations_historical_data_enabled() throws Exception {
AWSCredentials credentials = mock(AWSCredentials.class);
when(credentials.getAWSAccessKeyId()).thenReturn("valid key");
AmazonS3 s3 = mock(AmazonS3.class);
when(s3.doesBucketExistV2(anyString())).thenReturn(true);
AwsCredentials credentials = mock(AwsCredentials.class);
when(credentials.accessKeyId()).thenReturn("valid key");
S3Client s3 = setupMocks(mock(S3Client.class));

AmazonS3InstallationService service = new AmazonS3InstallationService("test-bucket") {
@Override
protected AWSCredentials getCredentials() {
protected AwsCredentials createCredentials(AwsCredentialsProvider provider) {
return credentials;
}

@Override
protected AmazonS3 createS3Client() {
protected S3Client createS3Client() {
return s3;
}
};
Expand All @@ -129,32 +123,18 @@ public MyService(String bucketName) {
super(bucketName);
}

public AWSCredentials credentials() {
return getCredentials();
}

public AmazonS3 s3() {
public S3Client s3() {
return createS3Client();
}
}

@Test
public void credentials() {
MyService service = new MyService("test-bucket");
try {
AWSCredentials credentials = service.credentials();
assertNotNull(credentials);
} catch (SdkClientException ignored) {
}
}

@Test
public void s3() {
MyService service = new MyService("test-bucket");
try {
AmazonS3 s3 = service.s3();
S3Client s3 = service.s3();
assertNotNull(s3);
} catch (SdkClientException ignored) {
} catch (Exception ignored) {
}
}

Expand Down
Loading
Loading