sarutak commented on PR #57675: URL: https://github.com/apache/spark/pull/57675#issuecomment-5187065822
Thank you for the detailed follow-up, @dongjoon-hyun. > 1. Driver-side updateUserCredentials has no version guard Fixed. `updateUserCredentials` now uses `VersionedCredentials.updateIfNewer()` (the same helper used on the executor side), making both sides symmetric. While the driver's `DriverEndpoint` is single-threaded so races are not possible in practice, this makes the code consistent and defensive. > 2. The "version guard prevents stale credentials from overwriting newer ones" test doesn't actually verify the guard Fixed. The test now uses `askSync(RetrieveSparkAppConfig(...))` to flush the `DriverEndpoint` mailbox after sending the stale v1, then asserts that the store still holds v3 before sending v5. > 3. The version-guard lambda is duplicated in three places Extracted into `VersionedCredentials.updateIfNewer(store, version, bytes)` in the companion object. All three call sites (`CoarseGrainedExecutorBackend.receive`, `Executor.TaskRunner`, `CoarseGrainedSchedulerBackend.updateUserCredentials`) and the unit test now use this single helper. > Minor - `×` (U+00D7) → ASCII `x` in `TaskDescription` comment - Consolidated the dual store: removed `CoarseGrainedSchedulerBackend.userCredentials` field entirely. `SparkEnv.get.userCredentials` is now the single source of truth on the driver side. - `TaskSetManager` now uses the existing `env` variable instead of calling `SparkEnv.get` again. - Added `LogKeys.CREDENTIAL_VERSION` and replaced `LogKeys.COUNT`. > Test coverage - Version monotonicity assertion added to the token rotation test (asserts callbacks receive versions 1, 2 in order). - The executor-side unit test now exercises `VersionedCredentials.updateIfNewer` directly (production code, not a copy of the logic). > Security note The SPIP design document already mentions this (§Security Considerations: "All RPCs carrying credentials rely on Spark's existing RPC TLS configuration (`spark.ssl.*`)"). I'll make sure Sub-task 12 (documentation) includes a clear recommendation to enable RPC encryption when OIDC credential propagation is active. -- 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]
