yangshangqing95 commented on code in PR #17914:
URL: https://github.com/apache/iceberg/pull/17914#discussion_r3907069510
##########
aws/src/test/java/org/apache/iceberg/aws/s3/signer/TestS3V4RestSignerClient.java:
##########
@@ -233,4 +237,42 @@ public static Stream<Arguments> legacySignerProperties() {
"https://legacy-signer.com";,
"https://legacy-signer.com/v1/legacy/sign";));
}
+
+ @Test
+ void testSignedComponentCacheKeyIsolation() throws Exception {
Review Comment:
Nit: Iceberg's current convention for newly added tests avoids the `test`
prefix, so this could be `signedComponentCacheKeyIsolation()` or, if converted
to a behavior level regression test, something like
`doesNotReuseSignedComponentAcrossSignerClients()`.
##########
aws/src/test/java/org/apache/iceberg/aws/s3/signer/TestS3V4RestSignerClient.java:
##########
@@ -233,4 +237,42 @@ public static Stream<Arguments> legacySignerProperties() {
"https://legacy-signer.com";,
"https://legacy-signer.com/v1/legacy/sign";));
}
+
+ @Test
+ void testSignedComponentCacheKeyIsolation() throws Exception {
+ Map<String, String> properties1 =
+ Map.of(
+ CatalogProperties.URI,
+ "https://signer.com";,
+ RESTCatalogProperties.REMOTE_SIGNING_ENDPOINT,
+ "v1/sign",
+ OAuth2Properties.CREDENTIAL,
+ "user1:secret1");
+ Map<String, String> properties2 =
+ Map.of(
+ CatalogProperties.URI,
+ "https://signer.com";,
+ RESTCatalogProperties.REMOTE_SIGNING_ENDPOINT,
+ "v1/sign",
+ OAuth2Properties.CREDENTIAL,
+ "user2:secret2");
+
+ try (S3V4RestSignerClient client1 =
+
ImmutableS3V4RestSignerClient.builder().properties(properties1).build();
+ S3V4RestSignerClient client2 =
+
ImmutableS3V4RestSignerClient.builder().properties(properties2).build()) {
+ RemoteSignRequest request =
+ ImmutableRemoteSignRequest.builder()
+ .method("GET")
+ .region("us-east-1")
+ .uri(URI.create("https://bucket.s3.amazonaws.com/key";))
+ .provider(S3V4RestSignerClient.S3_PROVIDER)
+ .build();
+
+ S3V4RestSignerClient.Key key1 = S3V4RestSignerClient.Key.from(request,
client1);
+ S3V4RestSignerClient.Key key2 = S3V4RestSignerClient.Key.from(request,
client2);
+
+ assertThat(key1).isNotEqualTo(key2);
Review Comment:
Could we make this a regression test for the actual cache behavior rather
than testing Key.equals directly?
For #17801, I'd expect the test to populate a cacheable signed response
through signer/client A, then make the same method/region/URI request through
signer/client B and verify that B still calls its signer and receives B's
signed component. That directly protects the security invariant and doesn't
couple the test to the current cache-key implementation.
--
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]
