Support driver pod kubernetes credentials mounting in V2 #246

mccheah · 2017-04-26T20:49:45Z

Implements #192 for submission v2.

mccheah · 2017-04-27T19:40:22Z

rerun unit tests please

mccheah · 2017-04-27T19:41:11Z

ash211 · 2017-04-27T21:01:34Z

...urce-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/kubernetes/config.scala

+        " authenticating against Kubernetes. Typically this is configured by spark-submit from" +
+        " mounting a secret from the submitting machine into the pod, and hence this" +
+        " configuration is marked as internal, but this can also be set manually to" +
+        " use a certificate that is mounted into the driver pod via other means.")


nit: certificate -> key file

ash211 · 2017-04-27T21:02:01Z

...urce-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/kubernetes/config.scala

+        " authenticating against Kubernetes. Typically this is configured by spark-submit from" +
+        " mounting a secret from the submitting machine into the pod, and hence this" +
+        " configuration is marked as internal, but this can also be set manually to" +
+        " use a certificate that is mounted into the driver pod via other means.")


certificate -> token

ash211 · 2017-04-27T21:02:50Z

...ers/kubernetes/core/src/main/scala/org/apache/spark/deploy/kubernetes/submit/v2/Client.scala

+ * This client is primarily designed in a piecewise manner, where different aspects of the driver's
+ * execution are managed by separate components. This allows each component to be tested
+ * individually. The component providers are dependency-injected into the client via the
+ * constructor, thus allowing the providers to be stubbed out in testing the client class.


ash211 · 2017-04-27T21:05:26Z

...ala/org/apache/spark/deploy/kubernetes/submit/v2/DriverPodKubernetesCredentialsMounter.scala

+  }
+
+  override def createCredentialsSecret(): Option[Secret] = {
+    val allSecretData = resolveSecretData(


nit: drop resolveSecretData onto the next line so it lines up with the other resolveSecretDatas

ash211 · 2017-04-27T21:07:29Z

...ala/org/apache/spark/deploy/kubernetes/submit/v2/DriverPodKubernetesCredentialsMounter.scala

+
+  /**
+   * Mount any Kubernetes credentials from the submitting machine's disk into the
+   * driver pod.


note that the credentialsSecret passed in here is the output of createCredentialsSecret(), allowing implementations of this trait to not have to save state between method invocations

ash211 · 2017-04-27T21:14:47Z

...ernetes/core/src/test/scala/org/apache/spark/deploy/kubernetes/submit/v2/ClientV2Suite.scala

  // Tests with local dependencies with the mounted dependency manager.
  test("Uploading local dependencies should create Kubernetes secrets and config map") {
-    val initContainerConfigMap = getInitContainerConfigMap()
-    val initContainerSecret = getInitContainerSecret()


after this change is initContainerSecret's type ConfigMap or a () -> ConfigMap function that creates it?

What's Spark style? cc @tnachen

ash211 · 2017-04-27T21:15:30Z

...ernetes/core/src/test/scala/org/apache/spark/deploy/kubernetes/submit/v2/ClientV2Suite.scala

  }

-  private def getInitContainerSecret(): Secret = {
+  private def getInitContainerSecret: Secret = {


I think Spark style is to not use getX for getters.

http://docs.scala-lang.org/style/naming-conventions.html#methods

ash211 · 2017-04-27T21:16:25Z

...rg/apache/spark/deploy/kubernetes/submit/v2/DriverPodKubernetesCredentialsMounterSuite.scala

+      APP_ID, KubernetesCredentials(None, None, None, None), None, None, None, None)
+
+  // Test matrices
+  private val TEST_MATRIX_EXPECTED_SPARK_CONFS = Table(


ash211 · 2017-04-27T21:18:45Z

...ts/src/test/scala/org/apache/spark/deploy/kubernetes/integrationtest/KubernetesV2Suite.scala

+      kubernetesTestComponents.clientConfig.getClientCertFile)
+    sparkConf.set(KUBERNETES_DRIVER_CA_CERT_FILE,
+      kubernetesTestComponents.clientConfig.getCaCertFile)
+    runSparkAppAndVerifyCompletion(SparkLauncher.NO_RESOURCE)


this verifies that the spark app finishes when these confs are set, but I'm not sure it verifies that the key/cert files are used when requesting (e.g. this conf could just be ignored and the test would still pass). How can we test those?

ash211 · 2017-04-27T21:27:19Z

...ers/kubernetes/core/src/main/scala/org/apache/spark/deploy/kubernetes/submit/v2/Client.scala

+          basePod, driverContainer.getName, driverPodKubernetesCredentialsSecret)
+      nonDriverPodKubernetesResources ++= driverPodKubernetesCredentialsSecret.toSeq
+      val sparkConfWithDriverPodKubernetesCredentialLocations =
+        driverPodKubernetesCredentialsMounter.setDriverPodKubernetesCredentialLocations(sparkConf)


I wonder if it's better to have this be all one method that returns a tuple of values, some new and some updated, and then apply those appropriately. E.g. (driverPodWithMountedCreds, sparkConfWithMountedCredLocations)

Would that save any complexity here?

mccheah · 2017-05-01T23:21:45Z

rerun unit tests please

ash211 · 2017-05-10T06:52:28Z

rerun integration test please

ash211

Looks good to me (did most of the review a few weeks ago). Main question is about the coverage of the integration test

ash211 · 2017-05-17T23:22:35Z

...ts/src/test/scala/org/apache/spark/deploy/kubernetes/integrationtest/KubernetesV2Suite.scala

    runSparkPiAndVerifyCompletion(SparkLauncher.NO_RESOURCE)
  }

+  test("Use client key and client cert file when requesting executors") {


is minikube configured at this point to require auth via the key/cert and CA cert files? Theoretically this test could pass because nothing happens to the config, rather than that the config is set and used in the request to the apiserver which requires the auth config

I was under the impression it either needs the service account token or the client key-cert pair. It's unfortunately difficult to test just this in isolation since we fall back to using the service account token if the user does not provide other credentials.

Oh yeah, we'd need to configure minikube specially for this, or inspect minikube output to verify this. Without a reasonably available way to verify this, I think the test is good as is.

…edentials

ash211

LGTM, merging

…246)

[NOSQUASH] Resync from apache-spark-on-k8s upstream

…pache-spark-on-k8s#246)

mccheah mentioned this pull request Apr 27, 2017

[WIP] Use the dependency download init-containers on executors. #249

Closed

ash211 reviewed Apr 27, 2017

View reviewed changes

This was referenced Apr 27, 2017

Download remotely-located resources on driver startup. #240

Closed

Download remotely-located resources on driver and executor startup via init-container #251

Merged

mccheah force-pushed the submission-v2-kubernetes-credentials branch from 95fa714 to 940bc9e Compare April 28, 2017 20:14

mccheah changed the base branch from init-container-downloads-remote-files to init-containers-on-executors-with-remote-files April 28, 2017 20:14

mccheah mentioned this pull request May 1, 2017

Allow client certificate PEM for resource staging server #257

Merged

Re-implement driver pod kubernetes credentials mounting for V2.

c2bbe6f

mccheah force-pushed the submission-v2-kubernetes-credentials branch from dc19d14 to c2bbe6f Compare May 17, 2017 19:17

mccheah changed the base branch from init-containers-on-executors-with-remote-files to branch-2.1-kubernetes May 17, 2017 19:19

ash211 changed the title ~~Re-implement driver pod kubernetes credentials mounting for V2.~~ Support driver pod kubernetes credentials mounting in V2 May 17, 2017

ash211 reviewed May 18, 2017

View reviewed changes

ash211 added 2 commits May 17, 2017 17:17

Merge branch 'branch-2.1-kubernetes' into submission-v2-kubernetes-cr…

06fe6aa

…edentials

Merge branch 'branch-2.1-kubernetes' into submission-v2-kubernetes-cr…

a8e587e

…edentials

ash211 approved these changes May 18, 2017

View reviewed changes

ash211 merged commit 9d6665c into branch-2.1-kubernetes May 18, 2017

mccheah mentioned this pull request May 23, 2017

TPR Support #284

Closed

foxish pushed a commit that referenced this pull request Jul 24, 2017

Support driver pod kubernetes credentials mounting in V2 submission (#…

a6cebcb

…246)

ifilonenko pushed a commit to ifilonenko/spark that referenced this pull request Feb 26, 2019

Merge pull request apache-spark-on-k8s#246 from palantir/resync-kube

701bd2a

[NOSQUASH] Resync from apache-spark-on-k8s upstream

puneetloya pushed a commit to puneetloya/spark that referenced this pull request Mar 11, 2019

Support driver pod kubernetes credentials mounting in V2 submission (a…

f31ca1b

…pache-spark-on-k8s#246)

Support driver pod kubernetes credentials mounting in V2 #246

Support driver pod kubernetes credentials mounting in V2 #246

Uh oh!

Conversation

mccheah commented Apr 26, 2017

Uh oh!

mccheah commented Apr 27, 2017

Uh oh!

mccheah commented Apr 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mccheah commented May 1, 2017

Uh oh!

ash211 commented May 10, 2017

Uh oh!

ash211 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ash211 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants