leoberliant-adobe commented on PR #3232: URL: https://github.com/apache/jackrabbit-oak/pull/3232#issuecomment-6416044449
Assessment Reviewed PR #3232 at 756f2f142b, including all seven changed files, discussion, relevant AWS SDK 2.37.5 behavior, and CI failure logs. Verdict: no new high-confidence correctness defects found. The earlier nested-profile finding is addressed. I would hold the merge pending successful validation of the changed S3 module. Code review **Earlier nested-profile issue** | Addressed. The credential provider now applies the first-attempt policy to every loaded profile. This matches the SDK’s behavior of selecting metadata settings from the source profile. **Fallback handling** | The retry logic logs only after successful recovery, switches providers only after success, and uses an atomic guard for the one-time diagnostic. **Compatibility** | Explicit Oak access/secret keys retain their existing path. Default-chain ordering and explicit AWS system/environment overrides are preserved. No global system-property mutation is introduced. **Resource lifecycle** | The presigner replacement builds replacements before switching, retains working signing when construction fails, and closes superseded resources without closing the shared main client. Resource lifecycle The [presigner replacement](https://github.com/apache/jackrabbit-oak/blob/756f2f142b41861457cd544b700e3524bf4454ab/oak-blob-cloud/src/main/java/org/apache/jackrabbit/oak/blob/cloud/s3/S3Backend.java#L289) builds replacements before switching, retains working signing when construction fails, and closes superseded resources without closing the shared main client. **Remaining validation concerns** 1. CI does not yet validate this change. The Maven build failed in LeaseUpdateSocketTimeoutIT because its mongo:8.0 container failed to start. The reactor then skipped Oak Cloud Blob Store. This looks unrelated to the patch, but it provides no passing evidence for the changed module. SonarQube was also skipped. 2. Nested-profile tests are useful but not fully end-to-end. The regression tests construct the nested source’s metadata provider directly. They verify the policy defect, but do not exercise the complete default-chain → nested STS role → metadata → logged retry path. That is a non-blocking coverage improvement. What this means for AEM This is primarily fallback observability and policy wrapping, not the introduction of IMDSv2 support—the existing SDK already prefers IMDSv2. - IMDSv1 remains allowed by default. - Explicit AWS settings can permit fallback during the first attempt without Oak’s retry diagnostic. - The ERROR indicates successful credential-chain recovery; it does not prove IMDSv1 was used. - For instance-level enforcement, use HttpTokens=required, after verifying IMDSv2 access. Recommendation: rerun CI or obtain a successful targeted module build before merging. I would not repeat the previous nested-profile objection against this revision. -- 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]
