Support using PEM files to configure SSL for driver submission #173
Conversation
| } | ||
| Utils.tryWithResource(new FileOutputStream(certPemFile)) { keyPemStream => | ||
| Utils.tryWithResource( | ||
| new OutputStreamWriter(keyPemStream, Charsets.UTF_8)) { streamWriter => |
There was a problem hiding this comment.
Missing an indent here, will fix on the next pass after review
|
This one can merge after the alpha? |
|
Correct - this should merge to the mainline branch after we cut and freeze the release branch. |
|
@robert3005 @ssuchter for review |
| val sslSecretsMap = mutable.HashMap[String, String]() | ||
| val storeBasedSslOptions = driverSubmitSslOptions.storeBasedSslOptions | ||
| val sslEnvs = mutable.Buffer[EnvVar]() |
There was a problem hiding this comment.
Can you split setting on sslEnvs and sslSecretsMap. You can just concat the transformed options which is more in line with what scala does. Then you don't need mutable buffers
| } | ||
|
|
||
| private def resolveLocalFile(file: Option[String], | ||
| fileType: String): (Boolean, Option[String]) = { |
There was a problem hiding this comment.
It's required for the error message.
| .map(Files.toString(_, Charsets.UTF_8)) | ||
| val resolvedKeyStore = (parsedArguments.keyStoreFile, parsedArguments.keyPemFile) match { | ||
| case (None, Some(keyPemFile)) => | ||
| parsedArguments.certPemFile match { |
There was a problem hiding this comment.
You're converting option to another option just to validate? This is map.orElse
There was a problem hiding this comment.
Not a fan of matching on options. See http://blog.originate.com/blog/2014/06/15/idiomatic-scala-your-options-do-not-match/ (Spark codebase doesn't like folding on them). map(...).getOrElse(throw new SparkException).
| } | ||
|
|
||
| private def parsePrivateKeyFromPemFile(keyPemFile: File): PrivateKey = { | ||
| Utils.tryWithResource(new FileInputStream(keyPemFile)) { keyPemStream => |
There was a problem hiding this comment.
Can you extract it to another function. This triply nested stream seems to be repeating
|
Looks good now. I think like there's a ton of options though that would benefit refactoring but it doesn't seem like the right place to tackle that. |
|
@robert3005 @foxish anything else needed for this to merge? |
|
I'm waiting on cutting the alpha and moving to our new branch structure. If someone is ready to do the cherrypicks after we have our 2.1-kubernetes and 2.1-kubernetes-dev branches, we can merge this. |
|
Please rebase onto |
88ea78a to
a17ea1c
Compare
| @@ -345,7 +339,7 @@ private[spark] class Client( | |||
| private def configureOwnerReferences( | |||
| kubernetesClient: KubernetesClient, | |||
| submitServerSecret: Secret, | |||
| sslSecrets: Array[Secret], | |||
| sslSecrets: Option[Secret], | |||
There was a problem hiding this comment.
rename to just sslSecret if there's only one now. Are we losing functionality by dropping from N to 1 secret?
There was a problem hiding this comment.
Nope because we only create one secret in this code. It's one secret with multiple parts.
| sslSecrets: Array[Secret], | ||
| sslPodVolume: Option[Volume], | ||
| sslPodVolumeMount: Option[VolumeMount], | ||
| sslSecrets: Option[Secret], |
| driverSubmitSslOptions.isKeyStoreLocalFile, | ||
| SUBMISSION_SSL_KEYSTORE_SECRET_NAME, | ||
| storeBasedSslOptions.keyStore, | ||
| "KeyStore") |
There was a problem hiding this comment.
maybe make KeyStore a constant? Its casing is a bit different from https://github.com/apache-spark-on-k8s/spark/pull/173/files#diff-c0d91a23f31f682a15ce930d584f0e5cR225
it's just used in logging though, so maybe no big deal?
There was a problem hiding this comment.
The casing is intentionally different I believe because in this case the type is mentioned at the beginning of the sentence of the error message and in the other case it's in the middle, hence requiring different grammar considerations in the choice of the string literal =P
| throw new SparkException(s"$secretType specified at ${file.getAbsolutePath} is not" + | ||
| s" a file or does not exist.") | ||
| } | ||
| val keyStoreBytes = Files.toByteArray(file) |
| resolveLocalFile(maybeServerCertPem, "server cert PEM") | ||
| val (isLocalKeyPem, resolvedKeyPem) = resolveLocalFile(maybeKeyPem, "key PEM") | ||
| maybeTrustStore.foreach { trustStore => | ||
| require(KubernetesFileUtils.isUriLocalFile(trustStore), s"Invalid trustStore URI" + |
| private def validateSslOptions(parsedArguments: KubernetesSparkRestServerArguments): Unit = { | ||
| parsedArguments.keyStoreFile.foreach { _ => | ||
| require(parsedArguments.keyPemFile.orElse(parsedArguments.certPemFile).isEmpty, | ||
| "Cannot provide both key/cert pem files and a keyStore file; select one or the other" + |
| parsedArguments.keyStoreType) | ||
| }) | ||
| }).getOrElse(throw new SparkException("When providing pem files, both the key and" + | ||
| " the certificate must be specified."))) |
There was a problem hiding this comment.
mention in this message that these are for the listening port on the REST server
| privateKey, | ||
| keyPassword.map(_.toCharArray).orNull, | ||
| Array(certificate)) | ||
| val keyStoreOutputPath = Paths.get(s"keystore-${UUID.randomUUID()}.$resolvedKeyStoreType") |
There was a problem hiding this comment.
this is in the current working directory of the rest service? safe to assume it's writable and has space?
There was a problem hiding this comment.
Think this is normally fine but may depend on the Docker image setup. The safest bet is to attach an emptyDir volume and use that, but that's a bit of extra complexity to make the REST server aware of the writable directory.
There was a problem hiding this comment.
let's punt on the emptydir volume complexity until it's a problem
|
|
||
| private def parseCertificateFromPemFile(certPemFile: File): X509Certificate = { | ||
| withPemParsedFromFile(certPemFile) { certPemParser => | ||
| certPemParser.readObject() match { |
There was a problem hiding this comment.
does this handle concatenated certs? meaning some PEM files have a series of -----BEGIN CERTIFICATE----- / -----END CERTIFICATE----- blocks with the whole cert chain -- e.g. root CA then intermediate CA then certificate.
There was a problem hiding this comment.
Hm, another project indicates how to do this. We should try this and test accordingly: docker-java/docker-java@c49f598
There was a problem hiding this comment.
Although once again I think meaningfully testing the processing of multiple certificates is going to be hard. That once again is best left to the unit level.
| SparkSubmit.main(args) | ||
| } | ||
|
|
||
| test("Enable SSL on the driver submit server using PEM files") { |
There was a problem hiding this comment.
difficult to add a test that verifies local:// URIs for PEM files? we'd have to bake them into the testing docker image?
There was a problem hiding this comment.
Yeah they would have to be added to the testing Docker image which isn't ideal. Perhaps possible since we build the testing Docker image through Java code right now though, so we could write the PEM and JKS files to the Docker build directory before building.
There was a problem hiding this comment.
Do you think testing that use case directly is important enough? It's definitely one I think people (and us) will be using
There was a problem hiding this comment.
I can see that being left to the unit level which we don't have coverage for yet.
a17ea1c to
82e4ac8
Compare
|
@mccheah scalastyle failure |
d8245a1 to
f9f5af4
Compare
b6dc342 to
0376817
Compare
* Support configuring SSL using PEM files. * Address some missed comments * Fix import ordering * Slight rewording of comments * Fix scalastyle
…e-spark-on-k8s#173) * Support configuring SSL using PEM files. * Address some missed comments * Fix import ordering * Slight rewording of comments * Fix scalastyle (cherry picked from commit 078697f)
…e-spark-on-k8s#173) * Support configuring SSL using PEM files. * Address some missed comments * Fix import ordering * Slight rewording of comments * Fix scalastyle
Closes #164. Note that this also changes some of the configuration keys in order to better clarify what context the certificate and key files are for (API server vs. driver submission server).