LuciferYang opened a new pull request, #58343:
URL: https://github.com/apache/spark/pull/58343
### What changes were proposed in this pull request?
`ExecutorKubernetesCredentialsFeatureStep` asks whether the executor pod
template already names a service account by reading `spec.serviceAccount`, the
field Kubernetes deprecated in favor of `spec.serviceAccountName`. A template
is deserialized client side by `KubernetesUtils.loadPodFromTemplate`, with no
API-server defaulting, so a template that names `serviceAccountName` arrives
with `serviceAccount` null and the guard concludes the template named nothing.
`buildPodWithServiceAccount` then writes both fields with
`spark.kubernetes.authenticate.executor.serviceAccountName`, or with
`spark.kubernetes.authenticate.driver.serviceAccountName` when that one is
unset.
The guard now reads both fields, with an empty value counting as unset,
which is what `SetDefaults_PodSpec` does:
```go
if len(obj.ServiceAccountName) == 0 {
obj.ServiceAccountName = obj.DeprecatedServiceAccount
}
obj.DeprecatedServiceAccount = obj.ServiceAccountName
```
That second line is also why leaving `serviceAccount` null in process is
harmless: once the pod reaches the API server the two fields end up in sync
either way. The read prefers `serviceAccountName` for the same reason the
snippet does, though the preference has no effect in this step, which only asks
whether either field names something.
SPARK-58872 added the same two-field read on the driver side, for the
warning it logs there.
I also reindented the method body from six spaces to four, which is why
lines I did not otherwise touch show up in the diff.
The pod template table in `running-on-kubernetes.md` also gets corrected.
Its `serviceAccount` and `serviceAccountName` rows ended with "Executor pods
will remain unaffected", which no release ever matched: the rows arrived with
pod templates in SPARK-24434 and the executor step landed in the same 3.0.0
release. The step overrides both fields on executor pods whenever the template
names no account and one of the two configurations is set, today using
`spark.kubernetes.authenticate.executor.serviceAccountName` and falling back to
`spark.kubernetes.authenticate.driver.serviceAccountName`. The rows now state
the driver rule and the executor rule separately, and the entry for the
executor config says a template that names an account wins.
Stating both rules in one place makes the asymmetry visible: a
template-named account loses on the driver and wins on the executor. That
asymmetry is long-standing, not new here. The driver applies its configured
account unconditionally when it mounts no credentials secret, and the rows have
said Spark overrides the template's account on driver pods since pod templates
arrived in SPARK-24434, while the executor step has always described its
configured account as a fallback for a template that names none.
### Why are the changes needed?
The step's own comment states the contract it does not keep: the configured
account is a fallback "if not setup by the pod template". For a template that
names an account, that holds only when the deprecated `spec.serviceAccount`
field is present, since the guard's null check never looks at
`spec.serviceAccountName`.
The executors instead run as the configured account, so the RBAC bound to
the account the template names does not apply.
The guard has read only the alias since SPARK-27872 (`7912ab85a6f`) in
3.0.0. The executor-side account came later, in SPARK-30122 (`f9f06eee985`,
3.1.0), so before that the account that won was the driver's configured one.
### Does this PR introduce _any_ user-facing change?
Yes, in two ways. First, an executor pod template that names an account in
`spec.serviceAccountName` and carries no `spec.serviceAccount` field at all:
with a service account configured through either
`spark.kubernetes.authenticate.executor.serviceAccountName` or
`spark.kubernetes.authenticate.driver.serviceAccountName`, those executor pods
used to run as the configured account and now run as the one the template
names. Second, a template whose `spec.serviceAccount` is present but empty,
with `spec.serviceAccountName` empty or absent: that used to count as naming an
account, which dropped the configured one and left the pod on the namespace
default, and now counts as unset, so the configured account applies. Every
other template behaves as before, including one that names a real account in
`spec.serviceAccountName` alongside a present `spec.serviceAccount`, which the
old guard kept too because it only asked whether `spec.serviceAccount` was null.
One thing this PR deliberately does not do: when the template wins, the
executor config is dropped without a word in the log. SPARK-58872 added that
WARN on the driver side, for the same user-visible situation: a configuration
set, and nothing saying why it did nothing. It is an improvement rather than
part of this fix, so it should go in a separate change that does not travel to
the maintenance branches. Filed as SPARK-59053.
### How was this patch tested?
Two tests in `ExecutorKubernetesCredentialsFeatureStepSuite`. `SPARK-58910:
keep the service account named by the executor pod template` configures the
executor account, then asserts that a template naming `serviceAccountName`
comes out with that name and a null alias, and that a template naming the
deprecated `serviceAccount` comes out with that alias and a null name.
Asserting the two fields separately instead of going through the suite's
`assertSAName` is deliberate: the step returns the pod untouched in this
branch, so whichever field the template left alone has to stay null.
`SPARK-58910: an empty service account name in the template counts as unset`
covers the other half. An empty value in either field, with the other absent,
means the pod has no account, so the configured one applies.
Every clause of the new expression has a mutation that turns one of the two
tests red, and I ran all of them: `master`'s original guard, which reads the
deprecated alias only and without the emptiness filter, and fails with
`"[executor]-name" did not equal "[template]-name"`; reading only
`serviceAccountName`; dropping the `nonEmpty` filter on `serviceAccountName`;
dropping it on the alias, which goes red through a null dereference inside
`assertSAName` rather than an equality failure.
`build/sbt -Pkubernetes kubernetes/test`: 392 tests across 42 suites, all
passing. `kubernetes/scalastyle` and `kubernetes/Test/scalastyle` 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]
