Fix S3Express utils and tests for auth scheme resolution pipeline migration#6947
Conversation
| return S3_EXPRESS.equals(useS3Express); | ||
| public static boolean isS3ExpressBucket(SdkRequest request) { | ||
| return request.getValueForField("Bucket", String.class) | ||
| .map(b -> b.endsWith("--x-s3")) |
There was a problem hiding this comment.
Can we make it as a Constant ?
| public static boolean isS3ExpressBucket(SdkRequest request) { | ||
| return request.getValueForField("Bucket", String.class) | ||
| .map(b -> b.endsWith("--x-s3")) | ||
| .orElse(false); |
There was a problem hiding this comment.
Nit/Not blocking query: The endpoint ruleset
determines S3Express by checking if the bucket name ends with --x-s3. This ruleset is auto-updated without SDK code changes. isS3ExpressBucket duplicates this logic with a hardcoded endsWith("--x-s3"). If the service adds a new S3Express suffix (say, --exp-s3) in a future ruleset update, this hardcoded check would silently fail to match. Is it possible to add a test that validates the S3Express suffix in the ruleset matches what isS3ExpressBucket checks, so we catch any mismatch when the ruleset updates?There was a problem hiding this comment.
yeah makes sense, added tests for this.
| .put(S3InternalSdkHttpExecutionAttribute.OBJECT_FILE_PATH, | ||
| executionAttributes.getAttribute(OBJECT_FILE_PATH)) | ||
| .put(USE_S3_EXPRESS_AUTH, S3ExpressUtils.useS3ExpressAuthScheme(executionAttributes)) | ||
| .put(USE_S3_EXPRESS_AUTH, S3ExpressUtils.isS3ExpressAuthRequest(context.request(), executionAttributes)) |
There was a problem hiding this comment.
The previous code checked SELECTED_AUTH_SCHEME, which is set by S3AuthSchemeInterceptor.beforeExecution. The new isS3ExpressAuthRequest
depends on AUTH_SCHEME_OPTIONS_RESOLVER being available. If null, it silently returns false.
Do we have tests verifying USE_S3_EXPRESS_AUTH is correctly set in the CRT path for:
- S3 Express bucket with default config → USE_S3_EXPRESS_AUTH is true
- S3 Express bucket with disableS3ExpressSessionAuth(true) → USE_S3_EXPRESS_AUTH is false
There was a problem hiding this comment.
We dont have any currently, added tests for these cases.
| @Test | ||
| void isS3ExpressBucket_suffixMatchesEndpointRuleset() throws IOException { | ||
| String rulesetSuffix = extractBucketSuffixFromRuleset(); | ||
| GetObjectRequest request = GetObjectRequest.builder() |
There was a problem hiding this comment.
Can we please assert rulesetSuffix is equal to "--x-s3" ?
joviegas
left a comment
joviegas
left a comment
There was a problem hiding this comment.
Approved with comment to add assertion to make sure "--x-s3" = rulesetSuffix
void isS3ExpressBucket_suffixMatchesEndpointRuleset() throws IOException {
S-Saranya1
merged commit a95bb32
into
feature/master/core-interceptors-migration
|
This pull request has been closed and the conversation has been locked. Comments on closed PRs are hard for our team to see. If you need more assistance, please open a new issue that references this one. |
2 participants
Motivation and Context
After moving auth scheme resolution and endpoint resolution from interceptors to pipeline stages (#6755, #6820), S3ExpressUtils methods that checked RESOLVED_ENDPOINT or SELECTED_AUTH_SCHEME no longer work at interceptor time, these values aren't set until the pipeline stages run (after all interceptors).
Modifications
Testing
Ran existing S3Express tests.
Screenshots (if appropriate)
Types of changes
Checklist
mvn installsucceedsscripts/new-changescript and following the instructions. Commit the new file created by the script in.changes/next-releasewith your changes.License