dongjoon-hyun commented on PR #57655:
URL: https://github.com/apache/spark/pull/57655#issuecomment-5220152797
Thank you for the contribution. Overall this looks well done — the
single-volatile immutable config holder correctly satisfies the SPI
thread-safety contract, the anonymous STS client is the right choice for
`AssumeRoleWithWebIdentity`, and the test coverage is thorough. A few comments
before merging:
1. **Token redaction does not cover the deep cause chain.** In the
`SdkException` catch block, only the top-level `e.getMessage()` is checked and
redacted; when redaction happens, `e.getCause()` is preserved as-is. If a
nested cause message ever contains the token (e.g., an HTTP-level exception
echoing the request body), it would still leak. Since this PR explicitly claims
the token is never exposed, consider walking the cause chain, or dropping the
cause entirely when the token is detected in the top-level message.
2. **`@DeveloperApi` removal.** The stub carried `@DeveloperApi`, which this
PR removes while keeping `@since 4.3.0` in the javadoc. Removing it seems
reasonable for a ServiceLoader-discovered implementation class, but could you
confirm this is intentional and mention it in the PR description?
3. **Configured `sessionName` is not validated.** A principal-derived
session name is sanitized against the STS constraint (`[\w+=,.@-]{2,64}`), but
a user-configured `spark.security.oidc.aws.sessionName` is only trimmed and
passed through. An invalid value would surface as an opaque STS error at
resolve time; validating it in `init()` (fail-fast) would be friendlier.
4. **Resource leak in tests.** The tests that go through the real `init()`
path (e.g., `testInitWithMinimumValidDurationSucceeds`,
`testInitWithEndpointAndRegionResolvesCorrectly`, etc.) build real SDK HTTP
clients and never close them. Please call `provider.close()` at the end of
those tests.
5. **(Minor) `resolve()` after `close()` escapes as an unchecked
exception.** A closed SDK client throws `IllegalStateException`, not
`SdkException`, so it bypasses the catch and propagates unwrapped. The SPI
contract allows in-flight `resolve()` to fail after `close()`, so this is not a
violation — but catching `RuntimeException` and wrapping it into
`CredentialResolutionException` would be more consistent for callers.
6. **(Minor) Docs.** This adds five user-facing `spark.security.oidc.aws.*`
keys with no `docs/` update. If documentation is planned as a follow-up task of
the SPIP, a pointer in the PR description would help.
--
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]
