Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v2 #318 +/- ##
============================================
+ Coverage 82.24% 82.71% +0.47%
- Complexity 143 148 +5
============================================
Files 12 12
Lines 366 376 +10
Branches 45 48 +3
============================================
+ Hits 301 311 +10
Misses 58 58
Partials 7 7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
src/test/java/com/amazonaws/secretsmanager/util/JDBCSecretCacheBuilderProviderTest.java
Show resolved
Hide resolved
src/test/java/com/amazonaws/secretsmanager/util/JDBCSecretCacheBuilderProviderTest.java
Outdated
Show resolved
Hide resolved
harsheejshah
left a comment
There was a problem hiding this comment.
for the aws-crt-client dependency, consider making it since CRT natives add significant size for users who don't enable PQTLS, and strengthen the PQTLS tests to actually verify the CRT HTTP client is configured rather than just assertNotNull. Also please squash commits before merge.
|
Make sure to complete the PR checklist |
|
Making the dependency optional would require using reflection to load the AwsCrtHttpClient class at runtime, which introduces unnecessary complexity and potential runtime errors that are much harder to troubleshoot than simple compile-time failures. The current approach keeps things straightforward - when someone enables PQTLS, they get guaranteed functionality, and users who don't need it simply won't use the CRT client. |
|
The CRT client pulls in platform specific native libraries (~10-15MB per platform). This is a JDBC driver library, most users will never enable PQTLS, and forcing them to ship CRT natives is a significant size penalty. The AWS SDK itself EDIT: offline conversations made me realize this is not supposed to be an optional feature, hence no need for this comment to be addressed. We should make the agent default to true without requiring customer configuration and that way this is not an overhead at all. |
| SecretsManagerClient client = new JDBCSecretCacheBuilderProvider(configProvider).build().build(); | ||
|
|
||
| // Verify VPC endpoint, region, and PQTLS are all configured | ||
| assertEquals(vpcEndpointUrlString, client.serviceClientConfiguration().endpointOverride().get().toString()); |
There was a problem hiding this comment.
order is wrong here as well, only address if a new rev is required
There was a problem hiding this comment.
Order is correct for junit, it's (expected, actual) https://junit.org/junit4/javadoc/latest/org/junit/Assert.html#assertEquals(java.lang.Object,%20java.lang.Object)
It's the opposite for testng.
| } | ||
|
|
||
| @Test | ||
| public void test_postQuantumTls_disabledByDefault() { |
There was a problem hiding this comment.
(Non-blocking) this test and test_postQuantumTls_disabledByDefault are functionally identical both just assertNotNull(client). The "enabled" test doesn't prove PQTLS was actually configured. At minimum, add a verify() to confirm the config was read and the code path diverged:
verify(configProvider).getBooleanPropertyWithDefault(pqtlsPropertyName, false);
src/main/java/com/amazonaws/secretsmanager/util/JDBCSecretCacheBuilderProvider.java
Outdated
Show resolved
Hide resolved
Subsequent discussions offline have made us reconsider making the CRT the default HTTP client for this library. One option we have is to mark the dependency as optional and require users that want to use the PQTLS setting to add the CRT to their dependency closure in their projects. So we're trading user experience for binary size. |
Description
Why is this change being made?
What is changing?
aws-crt-clientdependency for PQTLS supportgetBooleanPropertyWithDefault()method in Config classdrivers.postQuantumTlsEnabled=trueRelated Links
Testing
How was this tested?
getBooleanPropertyWithDefault()covering: property set to true, property set to false, property not set (default), and invalid property valueWhen testing locally, provide testing artifact(s):
mvn clean install->Tests run: 141, Failures: 0, Errors: 0, Skipped: 0Reviewee Checklist
Update the checklist after submitting the PR
If not, why:
If not, why:
If not, why:
If not, why:
If not, why:
If not, why:
If not, why:
If not, why:
Reviewer Checklist
All reviewers please ensure the following are true before reviewing:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.