Skip to content

[SPARK-27702][K8S] Allow using some alternatives for service accounts #24601

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Udbhav30 wants to merge 1 commit into from
Closed

[SPARK-27702][K8S] Allow using some alternatives for service accounts #24601

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions ...core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterManager.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,14 @@ private[spark] class KubernetesClusterManager extends ExternalClusterManager wit
require(sc.conf.get(KUBERNETES_DRIVER_POD_NAME).isDefined,
"If the application is deployed using spark-submit in cluster mode, the driver pod name " +
"must be provided.")
val serviceAccountToken =
Some(new File(Config.KUBERNETES_SERVICE_ACCOUNT_TOKEN_PATH)).filter(_.exists)
val serviceAccountCaCrt =
Some(new File(Config.KUBERNETES_SERVICE_ACCOUNT_CA_CRT_PATH)).filter(_.exists)
Copy link
Member

@dongjoon-hyun dongjoon-hyun Jan 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current logic looks like this.

  1. The difference of this logic is returning None when the file doesn't exist.
  2. However, None is not used when the given oauthTokenFileConf points a valid file because it's called as orElse.
    val oauthTokenFile = sparkConf.getOption(oauthTokenFileConf)
      .map(new File(_))
      .orElse(defaultServiceAccountToken)

Is the above correct?

Copy link
Member

@dongjoon-hyun dongjoon-hyun Jan 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing code already tries to use the given file first. Where did you check the following?

If it does not exist, other configurations are allowed to use, such as spark.kubernetes.authenticate.driver.mounted.oauthTokenFile.

Copy link
Contributor Author

@Udbhav30 Udbhav30 Jan 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing code already tries to use the given file first. Where did you check the following?

If it does not exist, other configurations are allowed to use, such as spark.kubernetes.authenticate.driver.mounted.oauthTokenFile.

Yes but still as we are passing 'KUBERNETES_SERVICE_ACCOUNT_TOKEN_PATH' to create the kubernetesClient , the kubernetes fabric client will use that service account path and will throw error as @wackxu mentioned.

Copy link
Contributor

@wackxu wackxu Feb 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Udbhav30 @dongjoon-hyun That is what I means. Let's merge this as soon as possible

(KUBERNETES_AUTH_DRIVER_MOUNTED_CONF_PREFIX,
sc.conf.get(KUBERNETES_DRIVER_MASTER_URL),
Some(new File(Config.KUBERNETES_SERVICE_ACCOUNT_TOKEN_PATH)),
Some(new File(Config.KUBERNETES_SERVICE_ACCOUNT_CA_CRT_PATH)))
serviceAccountToken,
serviceAccountCaCrt)
Copy link
Member

@dongjoon-hyun dongjoon-hyun May 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Udbhav30 . Could you add a test case this your PR?

} else {
(KUBERNETES_AUTH_CLIENT_MODE_PREFIX,
KubernetesUtils.parseMasterUrl(masterURL),
Expand Down