Allow client certificate PEM for resource staging server #257
Conversation
|
rerun unit tests please |
|
@mccheah seems like a legit test failure |
dc19d14 to
c2bbe6f
Compare
4a4dd42 to
067dcb8
Compare
| val maybeKeyStorePasswordFile = sparkConf.get(RESOURCE_STAGING_SERVER_KEYSTORE_PASSWORD_FILE) | ||
| val maybeKeyPasswordFile = sparkConf.get(RESOURCE_STAGING_SERVER_KEYSTORE_KEY_PASSWORD_FILE) | ||
| val maybeClientCertPem = sparkConf.get(RESOURCE_STAGING_SERVER_CERT_PEM) |
There was a problem hiding this comment.
typo: RESOURCE_STAGING_SERVER_CLIENT_CERT_PEM
| } | ||
| } | ||
| val resolvedTrustStorePassword = baseSslOptions.trustStorePassword | ||
| .orElse(maybeClientCertPem.map( _ => "defaultTrustStorePassword")) |
There was a problem hiding this comment.
will this default password ever actually work? I wonder if it's better to require a password upfront rather than failing later when the default inevitably doesn't match
There was a problem hiding this comment.
It will match because the SSL options provider is generating the trustStore with the given password from the certificate file.
There was a problem hiding this comment.
Ah got it -- didn't realize this same codepath was being used both during creating the temp truststore and reading it on the other side
There was a problem hiding this comment.
I'm open to ideas of making the flow clearer here
There was a problem hiding this comment.
I think it's fine as is -- if I'd read more closely the context around I think I would've seen that
| @@ -87,6 +87,32 @@ class ResourceStagingServerSslOptionsProviderSuite extends SparkFunSuite with Be | |||
| } | |||
| } | |||
|
|
|||
| test("Setting pem files without setting passwords should use random passwords.") { | |||
There was a problem hiding this comment.
I think we want at least one integration test that verifies the whole flow works with full-on SSL/certs everywhere, including this PR to use client certificates from init container -> resource staging server. Is there an existing test we can extend to have more SSL, or does that need to be a new test to get that coverage?
There was a problem hiding this comment.
The existing tests all use keyStores and trustStores and we would need to write a new test to cover using PEM files instead.
|
Requires rebasing onto branch-2.1-kubernetes. |
ddc98c1 to
505ce93
Compare
|
Finished rebasing. |
Allows the TLS certificate for reaching the resource staging server to be specified as a PEM file from the submitter's disk. Note that this is still uploaded as a trustStore file into the init-container - the client creates a temporary trustStore file from the certificate PEM. This is to simplify the secret generation so that we don't have to worry about differentiating between mounting the PEM vs. mounting a trustStore.
Depends on #246 which in turn depends on #251 .