dongjoon-hyun commented on PR #57655:
URL: https://github.com/apache/spark/pull/57655#issuecomment-5170181686
No new blocking issues beyond what has already been raised. A few
non-blocking suggestions to consider while reworking the PR after the rebase:
**1. Validate the `durationSeconds` range in `init()`**
`init()` only validates that `durationSeconds` is positive, but the valid
range for STS `AssumeRoleWithWebIdentity` is **900–43200 seconds**. A value
like `600` passes `init()` and then fails at the first `resolve()` call with an
STS `ValidationError`. Validating the range in `init()` (fail-fast) would
surface the misconfiguration much more clearly:
```java
if (durationSeconds < 900 || durationSeconds > 43200) {
throw new IllegalArgumentException(
"Configuration key '" + CONF_DURATION_SECONDS
+ "' must be between 900 and 43200 seconds, got: " + durationStr);
}
```
**2. `resolve()` neither checks nor uses `target`**
The SPI contract (`CredentialProvider#resolve`) states that `target` "must
not be null", but the implementation validates only `user`. Not using `target`
is reasonable for a role-based STS exchange, but for consistency with the
`user` validation, consider either adding a null check or noting in the javadoc
why `target` is intentionally unused.
**3. Token-redaction guarantee is only tested against mock messages**
The `SdkException` handler includes `e.getMessage()` in the wrapped message.
The "token never appears in exception messages" assertion is only verified
against a mock-generated message (`"Access denied for role"`), so in practice
the guarantee relies on AWS never echoing the token in its error messages
(which it normally does not — low actual risk). If you want to be defensive,
you could filter the token string out of the message before wrapping.
**4. Test coverage gaps**
- `sanitizeSessionName` edge cases are untested: truncation of a principal
longer than 64 chars, replacement of invalid characters with `-`, and the
fallback to `DEFAULT_SESSION_NAME` when the sanitized result is shorter than 2
chars. Only the happy path (`[email protected]`) is covered.
- `init()` with `0` or a negative `durationSeconds` should assert
`IllegalArgumentException` (currently only `"not-a-number"` is tested).
- `testServiceLoaderDiscovery` duplicates the same test already present in
the stub suite on `master`; please dedupe after rebasing.
- Once `close()` is implemented (per the earlier comment), please add a test
covering resource cleanup and the tolerated `resolve()` failure after `close()`.
On the plus side: the single-volatile-write publication of the immutable
`ResolvedConfig` matches the SPI's concurrency requirements nicely, the
`ArgumentCaptor`-based assertions on the STS request fields are thorough, and
`@since 4.3.0` is consistent with the existing SPI and branch versioning policy.
--
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]
