dongjoon-hyun commented on PR #57675: URL: https://github.com/apache/spark/pull/57675#issuecomment-5180962515
Thank you for working on this, @sarutak. The overall design looks solid — the three delivery paths are well-motivated and clearly documented, the `DriverEndpoint`-mediated broadcast is consistent with the existing `UpdateDelegationTokens` pattern, raw identity tokens never appear in any payload, and everything is `private[spark]` and gated behind `spark.security.oidc.enabled=false`. A few comments: ### Main points **1. Driver-side `updateUserCredentials` has no version guard** The executor side (`CoarseGrainedExecutorBackend.receive` and `Executor.TaskRunner`) applies credentials via `updateAndGet` with a version check, but the driver-side `updateUserCredentials` unconditionally `set`s both stores. There is a narrow but real race: in `setupUserCredentialManager`, the `onCredentialsUpdate` callback asynchronously sends the v1 message, and if a renewal (v2) message gets processed by `DriverEndpoint` before the synchronous `userCredentials.set(v1)` runs, the driver stores regress to v1. Applying the same `updateAndGet` guard on the driver side would eliminate this class of issue entirely and make the two sides symmetric. **2. The "version guard prevents stale credentials from overwriting newer ones" test doesn't actually verify the guard** It sends v3 → v1 → v5 and only asserts the final state is v5 — which passes even with no guard at all, since v5 is sent last (and indeed the driver path currently has no guard, per point 1). An assertion between the v1 and v5 sends (that the store still holds v3) is needed to make the test meaningful. **3. The version-guard lambda is duplicated in three places** `if (current == null || version > current.version)` appears inline in `CoarseGrainedExecutorBackend.receive`, `Executor.TaskRunner`, and again re-implemented in the "executor-side credential store version guard" test. Extracting a helper (e.g. `SparkEnv.updateUserCredentialsIfNewer(version, bytes)`) would remove the duplication and let that test exercise the production code instead of a copy of the logic. ### Minor - `TaskDescription.scala`: the comment `credential size × tasks-in-flight` contains a non-ASCII `×` (U+00D7). Scalastyle's nonascii checker doesn't flag comments so CI is green, but plain ASCII `x` would match project convention. - On the driver, `CoarseGrainedSchedulerBackend.userCredentials` and `SparkEnv.get.userCredentials` are always updated together with the same value. Unlike the `delegationTokens` precedent, nothing distinguishes them here — could this be consolidated into the `SparkEnv` store alone? - `TaskSetManager` already has `val env = SparkEnv.get`; the new code could use `env.userCredentials` instead of calling `SparkEnv.get` again. - `LogKeys.COUNT` is used for logging the version; a `*_VERSION`-style key would be more precise. - `UserCredentialManager` is only started from `CoarseGrainedSchedulerBackend.start()`, so enabling OIDC in local mode is a silent no-op. If intended, it may be worth a warning log or a note in the upcoming docs sub-task. ### Test coverage - `UserCredentialManagerSuite` never asserts version monotonicity — all callbacks discard the version with `(_, bytes) =>`. The existing renewal (rotating ingestor) test could simply assert the callback receives version 1 then 2. - There is no test covering `TaskSetManager.resourceOffer` actually attaching the current credentials to the `TaskDescription`. ### Security note Like delegation tokens, the credential bytes travel over RPC (`SparkAppConfig`, `UpdateUserCredentials`) and in `TaskDescription`, so they are plaintext without `spark.authenticate` + network encryption. Not a new threat model, but it would be good for the docs sub-task to recommend RPC encryption when OIDC is enabled. -- 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]
