Add support for S3 compatible storage#3188
Conversation
Signed-off-by: Go Yakami <gyakami@lycorp.co.jp>
Signed-off-by: Go Yakami <gyakami@lycorp.co.jp>
Summary of ChangesHello @gyakami, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the flexibility of Athenz's S3 integration by enabling support for generic S3-compatible storage solutions. It achieves this by introducing configurable options for custom endpoints, custom CA certificates for secure connections, and optional checksum validation for data integrity. These additions broaden the deployment possibilities for Athenz components that rely on S3 for storage, without impacting existing AWS S3 setups. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for S3-compatible storage by enabling custom endpoints and CA certificates, along with an option for checksum validation. The changes are well-implemented across S3ChangeLogStore and S3ClientFactory, with corresponding configuration updates and tests. My review focuses on improving maintainability by reducing code duplication and enhancing exception handling for better robustness.
| if (!StringUtil.isEmpty(s3CaCert)) { | ||
| try { | ||
| ApacheHttpClient.Builder httpClientBuilder = ApacheHttpClient.builder(); | ||
| X509Certificate[] certs = Crypto.loadX509Certificates(s3CaCert); | ||
| KeyStore keyStore = KeyStore.getInstance(KeyStore.getDefaultType()); | ||
| keyStore.load(null, null); // Initialize empty keystore | ||
| int i = 0; | ||
| for (X509Certificate cert : certs) { | ||
| keyStore.setCertificateEntry("custom-ca-" + i++, cert); | ||
| } | ||
| TrustManagerFactory tmf = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm()); | ||
| tmf.init(keyStore); | ||
|
|
||
| httpClientBuilder.tlsTrustManagersProvider(tmf::getTrustManagers); | ||
| s3ClientBuilder.httpClient(httpClientBuilder.build()); | ||
| } catch (Exception ex) { | ||
| LOGGER.error("S3ChangeLogStore: unable to load custom ca cert: {}", s3CaCert, ex); | ||
| throw new RuntimeException("S3ChangeLogStore: unable to load custom ca cert"); | ||
| } | ||
| } |
There was a problem hiding this comment.
This block of code for handling custom CA certificates is nearly identical to the one in S3ClientFactory.java. To improve maintainability and reduce code duplication, consider extracting this logic into a shared utility method. This method could take the caCertPath and return a configured TlsTrustManagersProvider or directly configure an ApacheHttpClient.Builder.
...server_aws_common/src/main/java/io/athenz/server/aws/common/store/impl/S3ChangeLogStore.java
Show resolved
Hide resolved
| if (!Config.isEmpty(caCertPath)) { | ||
| X509Certificate[] certs = Crypto.loadX509Certificates(caCertPath); | ||
| KeyStore keyStore = KeyStore.getInstance(KeyStore.getDefaultType()); | ||
| keyStore.load(null, null); // Initialize empty keystore | ||
| int i = 0; | ||
| for (X509Certificate cert : certs) { | ||
| keyStore.setCertificateEntry("custom-ca-" + i++, cert); | ||
| } | ||
| TrustManagerFactory tmf = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm()); | ||
| tmf.init(keyStore); | ||
|
|
||
| httpClientBuilder.tlsTrustManagersProvider(tmf::getTrustManagers); | ||
| } |
There was a problem hiding this comment.
This logic for handling custom CA certificates is duplicated in S3ChangeLogStore.java. To avoid code duplication and improve maintainability, this logic should be extracted into a common utility method.
| @Test | ||
| public void testGetS3ClientWithChecksumValidationEnabled() throws Exception { | ||
| // Setup configuration | ||
| System.setProperty(Config.PROP_PREFIX + Config.SYNC_CFG_PARAM_ROOT_PATH, TestUtils.TESTROOT); | ||
| System.setProperty(Config.PROP_PREFIX + Config.SYNC_CFG_PARAM_AWS_BUCKET, "test-bucket"); | ||
| System.setProperty(Config.PROP_PREFIX + Config.SYNC_CFG_PARAM_AWS_S3_CHECKSUM_VALIDATION, "true"); | ||
| System.setProperty(Config.PROP_PREFIX + Config.SYNC_CFG_PARAM_AWS_KEY_ID, "test-key"); | ||
| System.setProperty(Config.PROP_PREFIX + Config.SYNC_CFG_PARAM_AWS_ACCESS_KEY, "test-secret"); | ||
|
|
||
| Config.getInstance().loadProperties(); | ||
|
|
||
| // Mocks | ||
| try (MockedStatic<ApacheHttpClient> mockHttpClientStatic = mockStatic(ApacheHttpClient.class); | ||
| MockedStatic<S3Client> mockS3ClientStatic = mockStatic(S3Client.class)) { | ||
|
|
||
| // Mock ApacheHttpClient builder | ||
| ApacheHttpClient.Builder mockHttpBuilder = mock(ApacheHttpClient.Builder.class); | ||
| SdkHttpClient mockHttpClient = mock(SdkHttpClient.class); | ||
|
|
||
| mockHttpClientStatic.when(ApacheHttpClient::builder).thenReturn(mockHttpBuilder); | ||
| when(mockHttpBuilder.connectionTimeout(any())).thenReturn(mockHttpBuilder); | ||
| when(mockHttpBuilder.socketTimeout(any())).thenReturn(mockHttpBuilder); | ||
| when(mockHttpBuilder.build()).thenReturn(mockHttpClient); | ||
|
|
||
| // Mock S3Client builder | ||
| S3ClientBuilder mockS3Builder = mock(S3ClientBuilder.class); | ||
| S3Client mockS3Client = mock(S3Client.class); | ||
|
|
||
| mockS3ClientStatic.when(S3Client::builder).thenReturn(mockS3Builder); | ||
| when(mockS3Builder.httpClient(any(SdkHttpClient.class))).thenReturn(mockS3Builder); | ||
| when(mockS3Builder.region(any(Region.class))).thenReturn(mockS3Builder); | ||
| when(mockS3Builder.requestChecksumCalculation(any(RequestChecksumCalculation.class))).thenReturn(mockS3Builder); | ||
| when(mockS3Builder.responseChecksumValidation(any(ResponseChecksumValidation.class))).thenReturn(mockS3Builder); | ||
| when(mockS3Builder.credentialsProvider(any())).thenReturn(mockS3Builder); | ||
| when(mockS3Builder.build()).thenReturn(mockS3Client); | ||
|
|
||
| // Mock HeadBucket to pass verification | ||
| when(mockS3Client.headBucket(any(HeadBucketRequest.class))).thenReturn(HeadBucketResponse.builder().build()); | ||
|
|
||
| // Execute | ||
| S3Client client = S3ClientFactory.getS3Client(); | ||
|
|
||
| // Verify | ||
| assertNotNull(client); | ||
|
|
||
| // Verify checksum calculation and validation were enabled | ||
| verify(mockS3Builder).requestChecksumCalculation(RequestChecksumCalculation.WHEN_REQUIRED); | ||
| verify(mockS3Builder).responseChecksumValidation(ResponseChecksumValidation.WHEN_REQUIRED); | ||
| } finally { | ||
| // Cleanup | ||
| System.clearProperty(Config.PROP_PREFIX + Config.SYNC_CFG_PARAM_ROOT_PATH); | ||
| System.clearProperty(Config.PROP_PREFIX + Config.SYNC_CFG_PARAM_AWS_BUCKET); | ||
| System.clearProperty(Config.PROP_PREFIX + Config.SYNC_CFG_PARAM_AWS_S3_CHECKSUM_VALIDATION); | ||
| System.clearProperty(Config.PROP_PREFIX + Config.SYNC_CFG_PARAM_AWS_KEY_ID); | ||
| System.clearProperty(Config.PROP_PREFIX + Config.SYNC_CFG_PARAM_AWS_ACCESS_KEY); | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| public void testGetS3ClientWithChecksumValidationDisabled() throws Exception { | ||
| // Setup configuration | ||
| System.setProperty(Config.PROP_PREFIX + Config.SYNC_CFG_PARAM_ROOT_PATH, TestUtils.TESTROOT); | ||
| System.setProperty(Config.PROP_PREFIX + Config.SYNC_CFG_PARAM_AWS_BUCKET, "test-bucket"); | ||
| System.setProperty(Config.PROP_PREFIX + Config.SYNC_CFG_PARAM_AWS_S3_CHECKSUM_VALIDATION, "false"); | ||
| System.setProperty(Config.PROP_PREFIX + Config.SYNC_CFG_PARAM_AWS_KEY_ID, "test-key"); | ||
| System.setProperty(Config.PROP_PREFIX + Config.SYNC_CFG_PARAM_AWS_ACCESS_KEY, "test-secret"); | ||
|
|
||
| Config.getInstance().loadProperties(); | ||
|
|
||
| // Mocks | ||
| try (MockedStatic<ApacheHttpClient> mockHttpClientStatic = mockStatic(ApacheHttpClient.class); | ||
| MockedStatic<S3Client> mockS3ClientStatic = mockStatic(S3Client.class)) { | ||
|
|
||
| // Mock ApacheHttpClient builder | ||
| ApacheHttpClient.Builder mockHttpBuilder = mock(ApacheHttpClient.Builder.class); | ||
| SdkHttpClient mockHttpClient = mock(SdkHttpClient.class); | ||
|
|
||
| mockHttpClientStatic.when(ApacheHttpClient::builder).thenReturn(mockHttpBuilder); | ||
| when(mockHttpBuilder.connectionTimeout(any())).thenReturn(mockHttpBuilder); | ||
| when(mockHttpBuilder.socketTimeout(any())).thenReturn(mockHttpBuilder); | ||
| when(mockHttpBuilder.build()).thenReturn(mockHttpClient); | ||
|
|
||
| // Mock S3Client builder | ||
| S3ClientBuilder mockS3Builder = mock(S3ClientBuilder.class); | ||
| S3Client mockS3Client = mock(S3Client.class); | ||
|
|
||
| mockS3ClientStatic.when(S3Client::builder).thenReturn(mockS3Builder); | ||
| when(mockS3Builder.httpClient(any(SdkHttpClient.class))).thenReturn(mockS3Builder); | ||
| when(mockS3Builder.region(any(Region.class))).thenReturn(mockS3Builder); | ||
| when(mockS3Builder.credentialsProvider(any())).thenReturn(mockS3Builder); | ||
| when(mockS3Builder.build()).thenReturn(mockS3Client); | ||
|
|
||
| // Mock HeadBucket to pass verification | ||
| when(mockS3Client.headBucket(any(HeadBucketRequest.class))).thenReturn(HeadBucketResponse.builder().build()); | ||
|
|
||
| // Execute | ||
| S3Client client = S3ClientFactory.getS3Client(); | ||
|
|
||
| // Verify | ||
| assertNotNull(client); | ||
|
|
||
| // Verify checksum calculation and validation were NOT called | ||
| verify(mockS3Builder, never()).requestChecksumCalculation(any()); | ||
| verify(mockS3Builder, never()).responseChecksumValidation(any()); | ||
| } finally { | ||
| // Cleanup | ||
| System.clearProperty(Config.PROP_PREFIX + Config.SYNC_CFG_PARAM_ROOT_PATH); | ||
| System.clearProperty(Config.PROP_PREFIX + Config.SYNC_CFG_PARAM_AWS_BUCKET); | ||
| System.clearProperty(Config.PROP_PREFIX + Config.SYNC_CFG_PARAM_AWS_S3_CHECKSUM_VALIDATION); | ||
| System.clearProperty(Config.PROP_PREFIX + Config.SYNC_CFG_PARAM_AWS_KEY_ID); | ||
| System.clearProperty(Config.PROP_PREFIX + Config.SYNC_CFG_PARAM_AWS_ACCESS_KEY); | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| public void testGetS3ClientWithChecksumValidationNotConfigured() throws Exception { | ||
| // Setup configuration without checksum validation parameter | ||
| System.setProperty(Config.PROP_PREFIX + Config.SYNC_CFG_PARAM_ROOT_PATH, TestUtils.TESTROOT); | ||
| System.setProperty(Config.PROP_PREFIX + Config.SYNC_CFG_PARAM_AWS_BUCKET, "test-bucket"); | ||
| System.setProperty(Config.PROP_PREFIX + Config.SYNC_CFG_PARAM_AWS_KEY_ID, "test-key"); | ||
| System.setProperty(Config.PROP_PREFIX + Config.SYNC_CFG_PARAM_AWS_ACCESS_KEY, "test-secret"); | ||
|
|
||
| Config.getInstance().loadProperties(); | ||
|
|
||
| // Mocks | ||
| try (MockedStatic<ApacheHttpClient> mockHttpClientStatic = mockStatic(ApacheHttpClient.class); | ||
| MockedStatic<S3Client> mockS3ClientStatic = mockStatic(S3Client.class)) { | ||
|
|
||
| // Mock ApacheHttpClient builder | ||
| ApacheHttpClient.Builder mockHttpBuilder = mock(ApacheHttpClient.Builder.class); | ||
| SdkHttpClient mockHttpClient = mock(SdkHttpClient.class); | ||
|
|
||
| mockHttpClientStatic.when(ApacheHttpClient::builder).thenReturn(mockHttpBuilder); | ||
| when(mockHttpBuilder.connectionTimeout(any())).thenReturn(mockHttpBuilder); | ||
| when(mockHttpBuilder.socketTimeout(any())).thenReturn(mockHttpBuilder); | ||
| when(mockHttpBuilder.build()).thenReturn(mockHttpClient); | ||
|
|
||
| // Mock S3Client builder | ||
| S3ClientBuilder mockS3Builder = mock(S3ClientBuilder.class); | ||
| S3Client mockS3Client = mock(S3Client.class); | ||
|
|
||
| mockS3ClientStatic.when(S3Client::builder).thenReturn(mockS3Builder); | ||
| when(mockS3Builder.httpClient(any(SdkHttpClient.class))).thenReturn(mockS3Builder); | ||
| when(mockS3Builder.region(any(Region.class))).thenReturn(mockS3Builder); | ||
| when(mockS3Builder.credentialsProvider(any())).thenReturn(mockS3Builder); | ||
| when(mockS3Builder.build()).thenReturn(mockS3Client); | ||
|
|
||
| // Mock HeadBucket to pass verification | ||
| when(mockS3Client.headBucket(any(HeadBucketRequest.class))).thenReturn(HeadBucketResponse.builder().build()); | ||
|
|
||
| // Execute | ||
| S3Client client = S3ClientFactory.getS3Client(); | ||
|
|
||
| // Verify | ||
| assertNotNull(client); | ||
|
|
||
| // Verify checksum calculation and validation were NOT called (default behavior) | ||
| verify(mockS3Builder, never()).requestChecksumCalculation(any()); | ||
| verify(mockS3Builder, never()).responseChecksumValidation(any()); | ||
| } finally { | ||
| // Cleanup | ||
| System.clearProperty(Config.PROP_PREFIX + Config.SYNC_CFG_PARAM_ROOT_PATH); | ||
| System.clearProperty(Config.PROP_PREFIX + Config.SYNC_CFG_PARAM_AWS_BUCKET); | ||
| System.clearProperty(Config.PROP_PREFIX + Config.SYNC_CFG_PARAM_AWS_KEY_ID); | ||
| System.clearProperty(Config.PROP_PREFIX + Config.SYNC_CFG_PARAM_AWS_ACCESS_KEY); | ||
| } | ||
| } |
There was a problem hiding this comment.
The three new tests for checksum validation (testGetS3ClientWithChecksumValidationEnabled, testGetS3ClientWithChecksumValidationDisabled, testGetS3ClientWithChecksumValidationNotConfigured) contain a lot of duplicated code for setting up system properties and mocks. This can be refactored into a private helper method to reduce duplication and improve test readability and maintainability. The helper method could take the checksum configuration value and a Consumer<S3ClientBuilder> for verification logic as parameters.
2 participants
Description
This PR enables support for S3-compatible storage services other than AWS S3.
Changes
Extended the implementation to allow configuring:
Backward Compatibility: This change does not affect existing AWS S3 configurations. The default settings maintain the original behavior.
Contribution Checklist:
Attach Screenshots (Optional)