Skip to content

[SPARK-58910][K8S] Keep the executor pod template's service account - #58343

Open
LuciferYang wants to merge 2 commits into
apache:masterfrom
LuciferYang:fix-k8s-executor-sa-alias
Open

[SPARK-58910][K8S] Keep the executor pod template's service account#58343
LuciferYang wants to merge 2 commits into
apache:masterfrom
LuciferYang:fix-k8s-executor-sa-alias

Conversation

@LuciferYang

Copy link
Copy Markdown
Contributor

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:

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)

The guard read only the deprecated `serviceAccount` alias, so a template that
named `serviceAccountName` looked like it had no account and the configured one
overwrote it.
The two rows claimed executor pods are unaffected, which no release matched.
Also spells out on the executor config entry that a template naming an account
wins, and pins the test's null assertion in a comment.
@dongjoon-hyun

Copy link
Copy Markdown
Member

Thanks for the fix, @LuciferYang. I went through the diagnosis and ran the suite locally.

Verification

build/sbt -Pkubernetes 'kubernetes/testOnly *ExecutorKubernetesCredentialsFeatureStepSuite' passes all 5 tests, and kubernetes/scalastyle / kubernetes/Test/scalastyle are clean.

I also reproduced the mutation you described. Reverting the guard to master's Option(spec.getServiceAccount) turns exactly the two new tests red ("[executor]-name" did not equal "[template]-name") and leaves the three existing ones green, so the new tests pin the fix without touching the existing contract.

The root cause checks out: loadPodFromTemplate parses the template client side, so no SetDefaults_PodSpec runs, and a template that only sets spec.serviceAccountName reaches the step with spec.serviceAccount == null.

Two things worth discussing

  1. Consider including the WARN in this PR instead of deferring it to SPARK-59053. SPARK-58872 added a warning for a situation that already existed on the driver side; this PR is different in that it introduces the case where spark.kubernetes.authenticate.executor.serviceAccountName is set and silently has no effect. Since the account an executor runs as decides its RBAC, a silent flip is hard to trace from the user's side, so I would rather ship the warning together with the behavior change.

  2. Consider extracting the two-field read into KubernetesUtils. DriverKubernetesCredentialsFeatureStep now carries the same "prefer serviceAccountName, treat empty as unset" rule, and a helper such as def podServiceAccount(pod: SparkPod): Option[String] would keep it in one place and be reusable by the follow-up.

Related: it is worth deciding explicitly how far this should be backported. Executor pods that used to run as the configured account will now run as the one the template names, and without the warning above users have nothing pointing at the change.

Minor, take or leave

  • In the pod template table, the serviceAccount and serviceAccountName rows repeat the same long paragraph, and the "Modified value" column now holds a sentence rather than a value. Keeping that column short and putting the executor rule only in the description would read better.
  • templateServiceAccount's value is never used, only isEmpty, so the serviceAccountName-first ordering has no effect today. Extracting the helper above resolves this naturally.
  • KUBERNETES_EXECUTOR_SERVICE_ACCOUNT_NAME's .doc(...) in Config.scala still carries the old wording that you corrected in running-on-kubernetes.md.
  • The new tests set only the executor config. One case with only spark.kubernetes.authenticate.driver.serviceAccountName set would pin the .orElse(driverServiceAccount) path as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants