LuciferYang opened a new pull request, #58163:
URL: https://github.com/apache/spark/pull/58163
### What changes were proposed in this pull request?
Log a warning when a submission sets both
`spark.kubernetes.authenticate.driver.serviceAccountName` and any of the
driver's own client credentials, since the credentials win and the service
account is never applied to the driver pod. The message names the account that
was dropped, the credential configs that took precedence, and the way to have
both. It stays quiet when the pod spec already names that same account, because
then nothing was lost. Also spell out the same in `running-on-kubernetes.md`.
Behavior is unchanged. Two supporting bits: collecting the credential config
names for the message replaces the four-way `isDefined` disjunction that
computed `shouldMountSecret`, and `CONFIGS` joins `LogKeys` so the joined list
gets a plural MDC key like `PATHS` and `EXECUTOR_IDS` rather than reusing
`CONFIG2`, which everywhere else in the tree holds a single key.
### Why are the changes needed?
`DriverKubernetesCredentialsFeatureStep.configurePod` applies the service
account only on the branch with no credentials secret to mount:
```scala
override def configurePod(pod: SparkPod): SparkPod = {
if (!shouldMountSecret) {
pod.copy(pod = buildPodWithServiceAccount(driverServiceAccount,
pod).getOrElse(pod.pod))
} else {
// mounts the credentials secret; driverServiceAccount is never read here
```
`shouldMountSecret` is true if any of `oauthToken`, `caCertFile`,
`clientKeyFile`, `clientCertFile` was given for the driver.
That precedence is intended. `docs/running-on-kubernetes.md` has said since
2.3.0 that the service account "cannot be specified alongside a CA cert file,
client key file, client cert file, and/or OAuth token", and the config's own
doc says the driver "will favor using those credentials instead". But nothing
enforces it and nothing reports it, so a submission setting both is accepted
and looks honored, while the driver pod is left with the service account its
spec already names, or the namespace's `default`, and whatever RBAC that
carries. `running-on-kubernetes.md` is candid that the default account "may or
may not have the role that allows driver pods to create pods and services", so
the mistake can surface much later as an authorization failure when the driver
requests executors.
`caCertFile` is the easiest of the four to trip over. It only establishes
TLS trust in the API server, so it is not by itself something the driver
authenticates with, yet it is enough on its own to drop the account:
```
--conf spark.kubernetes.authenticate.driver.serviceAccountName=spark \
--conf spark.kubernetes.authenticate.driver.caCertFile=/path/to/ca.pem
```
The driver then reaches the API server as the namespace default account,
whose in-pod token `Config.autoConfigure` picks up.
The warning points at `spark.kubernetes.authenticate.driver.mounted.*`
rather than telling the user to drop one of the two. Those configs read paths
already present in the driver pod, never feed `shouldMountSecret`, and
therefore keep the service account, so they are the one setting that satisfies
both intents. "Unset one of them" would have been bad advice in both
directions: unsetting `caCertFile` gives up TLS trust in a private-CA API
server, and unsetting the account also moves executor pods off it, since
`ExecutorKubernetesCredentialsFeatureStep` falls back to the driver's account
when `spark.kubernetes.authenticate.executor.serviceAccountName` is unset. This
is not a matter of swapping the prefix, though: there is no
`mounted.oauthToken`, only `mounted.oauthTokenFile`, so a submitted token has
to become a file inside the pod first. The message says to put the credentials
in the pod and point the `mounted.*` configs at them, which covers all four.
One case is worth suppressing. A driver pod template that already names the
same account in its spec loses nothing on this branch, since the branch keeps
whatever the spec names, so warning there is only noise. It is also exactly the
configuration a user lands on after reading the `mounted.*` advice and deciding
to pin the account in a template instead. Reading the account off the spec
needs both fields. `serviceAccount` is Kubernetes' deprecated alias of
`serviceAccountName`, and `serviceAccountName` wins when a spec sets the two to
different values. A pod template is deserialized straight into the model object
with no API-server defaulting, so a template naming only one of them leaves the
other null. An explicitly empty `serviceAccountName` counts as unset, matching
`SetDefaults_PodSpec`, which copies the alias up in that case.
I went with a warning rather than a `require`. A `require` would match the
documented "cannot" and is the more honest reading of the docs, but it would
reject submissions that are accepted today, which needs a release note and a
deprecation path. Happy to switch if reviewers would rather enforce it.
### Does this PR introduce _any_ user-facing change?
A new WARN at submit time for submissions that set both. No API,
configuration, or pod-spec change.
### How was this patch tested?
Two tests in `DriverKubernetesCredentialsFeatureStepSuite`, split along what
they pin.
`warn when driver credentials drop the driver service account` sets the
account plus a `caCertFile` and checks that the pod spec's `serviceAccount` and
`serviceAccountName` are still null, pinning the behavior the warning
describes; that exactly one WARN names both configs; that it does *not* name
the three credential configs which were not set, which pins the filter rather
than just the fact that something was logged; and that it still points at the
`mounted.*` prefix, since that sentence is the whole value of the message. It
then requires the warning for two pod shapes that name a different account, one
of which sets `serviceAccount` and `serviceAccountName` to different values, so
reversing the field precedence fails.
`stay quiet when the driver service account survives` covers the three cases
that must not warn: the account alone, where it *is* applied; the account
alongside `mounted.caCertFile`, the escape hatch the message and the docs point
at; and four pod template shapes naming the same account, covering
`serviceAccountName` only, `serviceAccount` only, both, and the alias plus an
empty `serviceAccountName`.
Each assertion has a mutation that turns it red, and I ran all of them:
dropping the new guard; dropping the `mounted.*` sentence; letting the
`mounted.*` keys feed `shouldMountSecret`; reading only `getServiceAccount`;
reading only `getServiceAccountName`; widening the guard to "the pod names any
account"; dropping the empty-string filter; and reversing the two fields. One
in the other direction as well: adding a `logInfo` to the step keeps all three
quiet assertions green, since they filter for WARN.
Against `master` the first test fails:
```
[info] - SPARK-58872: warn when driver credentials drop the driver service
account *** FAILED ***
[info] 0 did not equal 1 expected one warning naming both
spark.kubernetes.authenticate.driver.serviceAccountName and
spark.kubernetes.authenticate.driver.caCertFile, got: List()
```
`build/sbt -Pkubernetes kubernetes/test`: 386 tests across 42 suites, all
passing. `common-utils/testOnly org.apache.spark.util.LogKeysSuite` passes, so
`CONFIGS` is in sorted position. `kubernetes/scalastyle`,
`kubernetes/Test/scalastyle`, and `common-utils-java/Compile/checkstyle` report
no issues.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (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]
