Allow the driver pod's credentials to be shipped from the submission client #186

mccheah · 2017-03-14T22:37:57Z

Provide a separate set of configurations for loading Kubernetes API server credentials for the driver pod to use when requesting executors.

Credentials specified in the submitter are POST'd to the submission server and used by the driver for interactions with the apiserver. This is a different set of credentials from those used by the submitter when starting the driver pod, allowing for fine-grained access controls between these two k8s clients if desired.

Closes #182

foxish · 2017-03-16T07:12:31Z

docs/running-on-kubernetes.md

Rather than introduce more k8s terminology, why don't we call this spark.kubernetes.authentication.*?

ash211 · 2017-03-16T08:30:53Z

Please rebase onto branch-2.1-kubernetes and send this PR into that branch instead of the k8s-support-alternate-incremental branch which is now deprecated.

ash211

what do you think of passing the creds through to the driver pod in the POST rather than via volume mount? does it make a big difference one way or the other?

ash211 · 2017-03-16T21:41:56Z

docs/running-on-kubernetes.md

+  <td><code>spark.kubernetes.authentication.driver.caCertFile</code></td>
+  <td>(none)</td>
+  <td>
+    CA cert file for connecting to Kubernetes over SSL from the driver pod when requesting executors. This file should


Kubernetes -> the Kubernetes API server

Start with "Path to the CA cert file" - and in other config options below

What URI scheme is expected, and what happens to paths with no scheme?

ash211 · 2017-03-16T21:52:24Z

...c/main/scala/org/apache/spark/deploy/kubernetes/DriverPodKubernetesCredentialsProvider.scala

+    sparkConf: SparkConf,
+    kubernetesAppId: String) {
+
+  def getDriverPodKubernetesCredentials(): DriverPodKubernetesCredentials = {


call this just get since it's pretty clear what you're getting from a DriverPodKubernetesCredentialsProvider and it's the only public method

ash211 · 2017-03-16T21:54:14Z

...c/main/scala/org/apache/spark/deploy/kubernetes/DriverPodKubernetesCredentialsProvider.scala

+        throw new SparkException(s"File provided for ${conf.key} at ${file.getAbsolutePath}" +
+          s" does not exist or is not a file.")
+      }
+      (secretName, BaseEncoding.base64().encode(Files.toByteArray(file)))


does Files.toByteArray close the File object?

The File object is not opened or closed, but rather the stream that pulls the data has to be opened and closed, which Guava takes care of.

ash211 · 2017-03-16T21:56:49Z

...c/main/scala/org/apache/spark/deploy/kubernetes/DriverPodKubernetesCredentialsProvider.scala

+      .withMountPath(DRIVER_CONTAINER_KUBERNETES_CREDENTIALS_SECRETS_BASE_DIR)
+      .build()
+    // Cannot use both service account and mounted secrets
+    sparkConf.get(KUBERNETES_SERVICE_ACCOUNT_NAME).foreach { _ =>


move this validation up to right after these are created (validate early)

ash211 · 2017-03-16T21:58:08Z

...c/main/scala/org/apache/spark/deploy/kubernetes/DriverPodKubernetesCredentialsProvider.scala

+    val credentialsSecretVolumeMount = new VolumeMountBuilder()
+      .withName(credentialsSecretVolume.getName)
+      .withReadOnly(true)
+      .withMountPath(DRIVER_CONTAINER_KUBERNETES_CREDENTIALS_SECRETS_BASE_DIR)


I'm not totally sure I follow the mounting here -- why do we need to create a volume mount to store data to a file? Is the docker image read-only or something? Or we want to ensure the secrets are disposed of in a secure manner?

The docker image only provides a static state that the container starts with, and the volume mounts are attached to that container to add dynamic data from Volume plugins.

so this is a mechanism to inject files into the running container?

Not an already-running container exactly, but more precisely to attach one on startup of the container

We use this everywhere we've been providing secrets; the other way secrets can be consumed is via environment variables, but in all of our cases we've been loading them from mounted files.

ash211 · 2017-03-16T22:02:08Z

...ernetes/core/src/main/scala/org/apache/spark/deploy/kubernetes/KubernetesClientBuilder.scala

      .withApiVersion("v1")
      .withMasterUrl(KUBERNETES_MASTER_INTERNAL_URL)
      .withNamespace(kubernetesNamespace)



should we do the just-one-auth validation here too?

require((MOUNTED_TOKEN.isFile || MOUNTED_CA_CERT.isFile || MOUNTED_CLIENT_KEY.isFile || MOUNTED_CLIENT_CERT.isFile) ^ (SERVICE_ACCOUNT_CA_CERT.isFile || SERVICE_ACCOUNT_TOKEN.isFile))

Think it's fine to leave it out here and rely on the client to check.

The service account token file is actually always present, so this check wouldn't be correct.

ash211 · 2017-03-16T22:03:07Z

...rnetes/core/src/main/scala/org/apache/spark/deploy/kubernetes/SslConfigurationProvider.scala

  private val sslSecretsName = s"$SUBMISSION_SSL_SECRETS_PREFIX-$kubernetesAppId"
-  private val sslSecretsDirectory = s"$DRIVER_CONTAINER_SECRETS_BASE_DIR/$kubernetesAppId-ssl"
+  private val sslSecretsDirectory = DRIVER_CONTAINER_SUBMISSION_SECRETS_BASE_DIR
+    s"/$kubernetesAppId-ssl"


does Scala magically concatenate these two adjacent strings?

Nope, this is a typo

I think this is failing the integration tests

but but but how does it compile? does the hanging string just not get assigned to anything and disappear?

Believe that's the case yeah

ash211 · 2017-03-16T22:07:12Z

...ests/src/test/scala/org/apache/spark/deploy/kubernetes/integrationtest/KubernetesSuite.scala

        .pods
        .withLabel("spark-app-name", "spark-file-existence-test")
        .watch(watch)) { _ =>
-      SparkSubmit.main(args)


why change from SparkSubmit.main(args) to new Client(...) ?

SparkSubmit.main is unwieldy because

(1) It's difficult to re-use arguments since they have to be formatted as an array rather than as a Map, and more importantly
(2) They deal with system properties which is global state across all tests in the JVM.

It's beneficial to test using SparkSubmit if we expect SparkSubmit to process its arguments in a way that our Client doesn't expect, but for the most part otherwise we should call the Client directly.

ash211 · 2017-03-16T22:09:53Z

@kimoonkim heads up another flake in SparkContextSuite: http://spark-k8s-jenkins.pepperdata.org:8080/job/PR-spark-k8s-unit-tests/25/consoleFull#-1164448660c3852bdb-3a63-45ca-a0d4-973026729059

mccheah · 2017-03-16T22:14:29Z

Hm, I wanted to avoid making the submission server more complex by adding more data to the POST, and in the future if we switch to a different submission mechanism we'd have to rewrite more logic there. But I suppose creating fewer Kubernetes resources is beneficial as well particularly if these objects are short-lived.

ash211 · 2017-03-16T23:19:45Z

rerun integration test please

ash211 · 2017-03-16T23:20:21Z

rerun unit tests please

ash211 · 2017-03-16T23:42:07Z

rerun integration test please

(previous failed: http://spark-k8s-jenkins.pepperdata.org:8080/job/PR-spark-k8s-integration-test-/20/consoleFull#-167881559204b09c7b-0d94-4ce5-8a08-8f343248b3d8 )

ash211 · 2017-03-16T23:49:03Z

rerun integration test please

mccheah · 2017-03-17T00:32:21Z

Hm, should we also allow pre-mounted files, like we currently do for the keyStore? e.g. local:// paths. I'm not sure how common it would be to bake Kubernetes credentials into docker images though, where it might be more common for TLS keys and certificates; the case here seems more user-specific while the TLS scenario could be fixed across many users of a Spark Docker image.

ash211 · 2017-03-17T00:38:00Z

It seems reasonable to bake in a truststore for the apiserver into docker images, but I'm guessing most places would avoid putting creds into images for security reasons.

I'd propose we don't support local:// paths for the various credentials on the driver until it affects someone.

Also some quality of life fixes, most notably formatting all of the documentation strings in config.scala to no longer use triple quotes. Triple quoted strings are difficult to format consistently.

mccheah · 2017-03-17T02:16:01Z

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

-          | this can also be passed to spark-submit via the
-          | --kubernetes-namespace command line argument.
-        """.stripMargin)
+      .doc("The namespace that will be used for running the driver and executor pods. When using" +


I reformatted all of these because it's difficult to get triple-quoting to have a consistent format.

This matches what YARN has.

ash211 · 2017-03-17T06:23:06Z

rerun unit tests please

ash211

so the process is (using oauth token but same for the client cert pair):

user sets oauth token in config key spark.kubernetes.authentication.driver.oauthToken
submission client reads that out of SparkConf and creates a KubernetesCredentials out of it
then redacts the token out of SparkConf
posts the KubernetesCredentials to the submission server as part of job launch
submission server then writes the token to disk in writeKubernetesCredentials and saves its path to SparkConf in the spark.kubernetes.authentication.driver .mounted.oauthTokenFile key
driver then reads that path from its SparkConf and assembles a kubernetes client with it, in buildFromWithinPod

This seems pretty reasonable, and POST'ing those things directly to the driver pod vs the temporary volumes follows the pattern of other parts being posted.

No more major comments from me -- @foxish have you had a chance to give this a review?

ash211 · 2017-03-17T06:25:57Z

docs/running-on-kubernetes.md

+  <td>(none)</td>
+  <td>
+    Path to the client key file for authenticating against the Kubernetes API server from the driver pod when requesting
+    executors. This file should be located on the submitting machine's disk, and will be uploaded to the driver pod.


should -> must ?

ash211 · 2017-03-17T06:27:34Z

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

        } finally {
          Utils.tryLogNonFatalError {
-            kubernetesResourceCleaner.deleteAllRegisteredResourcesFromKubernetes(kubernetesClient)
+//            kubernetesResourceCleaner.deleteAllRegisteredResourcesFromKubernetes(kubernetesClient)


was this line accidentally included?

ash211 · 2017-03-17T06:58:17Z

...ernetes/core/src/main/scala/org/apache/spark/deploy/kubernetes/KubernetesClientBuilder.scala

+    val configBuilder = oauthTokenFile
+        .orElse(caCertFile)
+        .orElse(clientKeyFile)
+        .orElse(clientCertFile)


this seems like an unusual pattern for "if any of these are present". Does Scala have an any(Seq[Option[]) method like python's any ?

Don't see such an API, unfortunately.

Looks like the Scala way (from 2011 via http://stackoverflow.com/questions/6390797/scala-any-and-all-functions and 2013 via http://stackoverflow.com/questions/15932137/checking-that-all-items-in-a-collection-match-a-predicate-in-scala) is:

Seq(oauthTokenFile, caCertFile, clientKeyFile, clientCertFile).any(_.isDefined)

Do you find that clearer than the .orElse chain ?

ash211 · 2017-03-17T07:00:01Z

...ernetes/core/src/main/scala/org/apache/spark/deploy/kubernetes/KubernetesClientBuilder.scala

-      if (MOUNTED_CA_CERT.isFile) {
-        clientConfigBuilder = clientConfigBuilder.withCaCertFile(MOUNTED_CA_CERT.getAbsolutePath)
+      caCertFile.foreach { caFile =>
+        mountedAuthConfigBuilder = mountedAuthConfigBuilder.withCaCertFile(caFile)


this has to be reassigned every time? would be nice if the fluent API allowed repeated calls on the same reference in these kinds of situations

Reading the Kubernetes client code indicates that we don't have to assign here - but I actually think it's clearer to follow this pattern currently, as it semantically means "This builder is equal to the old builder with these updates". The phrasing of the API (builder.withCaCertFile) made it sound like it doesn't mutate the underlying object - I would have expected setCaCertFile if it did.

ash211 · 2017-03-17T07:37:46Z

docs/running-on-kubernetes.md

 </tr>
 <tr>
-  <td><code>spark.kubernetes.submit.caCertFile</code></td>
+  <td><code>spark.kubernetes.authentication.submission.caCertFile</code></td>


this key is kinda long -- thoughts on shortening authentication to just auth ?

They use spark.authenticate in other places - but not sure if we should be following that precedent: https://spark.apache.org/docs/latest/configuration.html

ash211

LGTM!

@foxish any last comments? I'm planning to merge this EOD if you don't have anything.

foxish · 2017-03-17T20:25:00Z

I didn't get time to look into it in detail. At a high level, it seems fine. lgtm

ash211 · 2017-03-17T20:25:49Z

Sounds good -- going ahead and merging

ash211 · 2017-03-17T23:03:05Z

Oh uh oh, I merged instead of squashed and merged... which gave us a dirty history

going to:

temporarily enable force pushing to branch-2.1-kubernetes
force push to branch-2.1-kubernetes removing the merge commit
disable force pushing to branch-2.1-kubernetes
open a new PR from mounted-secrets-auth -> branch-2.1-kubernetes
squash and merge that PR

ash211 · 2017-03-17T23:10:38Z

Done

foxish reviewed Mar 16, 2017

View reviewed changes

mccheah changed the base branch from k8s-support-alternate-incremental to branch-2.1-kubernetes March 16, 2017 19:51

Allow the driver pod's credentials to be shipped through secrets.

8fca03e

mccheah force-pushed the mounted-secrets-auth branch from d8abc21 to 8fca03e Compare March 16, 2017 19:57

Fix scalastyle

e42c03a

mccheah mentioned this pull request Mar 16, 2017

Authentication + SSL Changes from Kubernetes Upstream palantir/spark#133

Closed

Change apiserver -> authentication

3420d84

ash211 reviewed Mar 16, 2017

View reviewed changes

mccheah mentioned this pull request Mar 16, 2017

Support using PEM files to configure SSL for driver submission #173

Merged

mccheah added 3 commits March 16, 2017 19:09

Address comments.

9ca7b57

Also some quality of life fixes, most notably formatting all of the documentation strings in config.scala to no longer use triple quotes. Triple quoted strings are difficult to format consistently.

Fix scalastyle

5a8ba57

Fix comment

caf247d

mccheah commented Mar 17, 2017

View reviewed changes

mccheah added 2 commits March 16, 2017 19:17

Remove unnecessary constants

b06b88c

Remove unnecessary whitespace

75c714f

mccheah changed the title ~~Allow the driver pod's credentials to be shipped through secrets~~ Allow the driver pod's credentials to be shipped from the submission client Mar 17, 2017

ash211 reviewed Mar 17, 2017

View reviewed changes

Authentication -> Authenticate

2f33ff3

ash211 approved these changes Mar 17, 2017

View reviewed changes

ash211 merged commit d8245a1 into branch-2.1-kubernetes Mar 17, 2017

ash211 deleted the mounted-secrets-auth branch March 17, 2017 20:25

mccheah restored the mounted-secrets-auth branch March 17, 2017 23:00

ash211 mentioned this pull request Mar 17, 2017

Allow the driver pod's credentials to be shipped from the submission client (squashed) #192

Merged

ash211 deleted the mounted-secrets-auth branch March 17, 2017 23:10

Allow the driver pod's credentials to be shipped from the submission client #186

Allow the driver pod's credentials to be shipped from the submission client #186

Uh oh!

Conversation

mccheah commented Mar 14, 2017 • edited by ash211 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ash211 commented Mar 16, 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!

mccheah Mar 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ash211 commented Mar 16, 2017

Uh oh!

mccheah commented Mar 16, 2017

Uh oh!

ash211 commented Mar 16, 2017

Uh oh!

ash211 commented Mar 16, 2017

Uh oh!

ash211 commented Mar 16, 2017

Uh oh!

ash211 commented Mar 16, 2017

Uh oh!

mccheah commented Mar 17, 2017

Uh oh!

ash211 commented Mar 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mccheah commented Mar 14, 2017 •

edited by ash211

Loading

mccheah Mar 16, 2017 •

edited

Loading