dongjoon-hyun commented on PR #57796:
URL: https://github.com/apache/spark/pull/57796#issuecomment-5768382535
I reviewed this and the core design checks out. For the record, I verified
the assumptions it rests on:
- All in-tree `RequestMasterState` callers (`MasterPage`, `ApplicationPage`,
`MasterWebUI`, and every test) go through `master.self.askSync`, and
`NettyRpcEnv.askAbortable` takes the `dispatcher.postLocalMessage` path when
`remoteAddr == address`, so no serialization happens and the Master UI is
unaffected.
- `WorkerInfo.executors` / `WorkerInfo.drivers` are `@transient`, so there
is no un-redacted `ApplicationInfo` reachable through
`ExecutorDesc.application` in the serialized graph. The `workers` array is not
a bypass.
- `ObjectOutputStream` does not re-invoke `writeReplace` when the
replacement has the same class, and `_conf` is `@transient` on the replacement
anyway, so there is no recursion.
- `defaultCores` was already a field before this PR (the `getOrElse` lambda
in `requestedCores` captures it -- `javap` shows `private final int
defaultCores`), so referencing it from `redactedCopy` does not change
`ApplicationInfo`'s default `serialVersionUID`. Master recovery from
`ZooKeeperPersistenceEngine` / `FileSystemPersistenceEngine` stays compatible.
Worth stating explicitly, since a SUID change there would have silently deleted
persisted apps/drivers on upgrade (`deserializeFromFile` catches and
`zk.delete()`s).
Three findings, all minor:
### 1. `javaOpts` redaction is never actually asserted (`MasterSuite.scala`
L282, L291, L319, L327)
```scala
assert(!redactedApp.desc.command.javaOpts.contains("env-token"))
```
`Seq[String].contains` is element equality, and no element ever equals
`"env-token"` -- it is `-Dspark.executorEnv.TOKEN=env-token` before redaction
and `-Dspark.executorEnv.TOKEN=*********(redacted)` after. The assertion is
vacuously true in both states.
Concretely: drop `javaOpts = Utils.redactCommandLineArgs(conf, javaOpts)`
from `Command.redactedCopy` and all three new tests still pass. The `-Xmx2g`
assertions and the `PASSWORD` env assertions are unaffected, and only the
negative test (`without withConf`) compares the full
`-Dspark.executorEnv.TOKEN=env-token` string. So half of what `redactedCopy`
does is currently uncovered.
Suggested:
```scala
assert(redactedApp.desc.command.javaOpts.contains(
s"-Dspark.executorEnv.TOKEN=${Utils.REDACTION_REPLACEMENT_TEXT}"))
```
### 2. `Map.contains` checks keys, not values (`MasterSuite.scala` L279,
L288, L316, L323)
```scala
assert(!redactedApp.desc.command.environment.contains("topsecret"))
```
`scala.collection.Map.contains` is a key lookup, and no key is ever
`"topsecret"`, so this holds even if `Utils.redact` were removed entirely. The
real coverage is the next line (`environment("PASSWORD") ==
Utils.REDACTION_REPLACEMENT_TEXT`), which is correct -- these four lines just
read as if they were checking values when they are not.
`!environment.values.exists(_.contains("topsecret"))` if a value-level check is
wanted, otherwise they can go.
### 3. `ApplicationInfo.redactedCopy` drops `_retryCount`, contradicting its
own comment
```scala
// Any future non-transient field added to ApplicationInfo must be copied
here
// to avoid silently dropping it from the serialized (redacted) form.
private[deploy] def redactedCopy(conf: SparkConf): ApplicationInfo = {
```
`private var _retryCount = 0` has no `@transient`, so it is already part of
the serialized form (`javap`: `private int _retryCount;`), and the new instance
leaves it at 0. An app that has been retried N times (`Master.scala`,
`log"failed ${MDC(LogKeys.NUM_RETRY, appInfo.retryCount)} times"`) goes over
the wire as 0.
The practical impact is nil today -- no in-tree consumer reads it off the
wire -- but the invariant the comment states is not actually held by the code
as written, which will mislead whoever adds the next field. Either copy it
(`copy.resetRetryCount()` is not enough; it needs a setter or a constructor
param) or narrow the comment to say which fields are intentionally not carried
over.
### Non-blocking notes
- `withConf` is opt-in / fail-open: a future `MasterStateResponse` reply
site that forgets it leaks silently. There is only one construction site today
(`Master.scala`), so this is not a defect, but it is worth a thought vs. making
redaction the default and having the persistence path opt out.
- The comment in the third test ("The persistence/recovery path relies on
this contract to preserve original secrets on disk") is a bit off:
`PersistenceEngine` serializes `ApplicationInfo` / `DriverInfo` directly, never
`MasterStateResponse`, so it does not depend on this contract. The PR
description gets this right; just the test comment overstates it.
- The same class of leak still exists on the Worker side:
`RequestWorkerState` -> `WorkerStateResponse` carries `ExecutorRunner.appDesc`
and `DriverRunner.driverDesc`, both with an unredacted `Command`. Out of scope
here, but a natural follow-up now that `Command.redactedCopy` exists.
Line lengths and ASCII are clean on the changed files.
Reviewed with Claude Opus 5.
--
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]
