Support SSL on the file staging server #221
Conversation
…es' into submission-v2-file-server
|
Note that this is merging into #220 , which in turn is merging into #212. We should:
|
We still want to post them because in the future we can use these credentials to monitor the API server and handle cleaning up the data accordingly.
| .stringConf | ||
| .createOptional | ||
|
|
||
| private[spark] val DEPENDENCY_SERVER_CERT_PEM = |
There was a problem hiding this comment.
put all these new config options into the docs file. Also needs to include what of file:// local:// hdfs:// protocols are supported here
There was a problem hiding this comment.
I was planning to save all documentation for a final sweep where we eliminate all of the v1 submission logic and replace it with v2. At that point the docs should be updated to remove the old options and replace them with the new ones.
|
rerun integration test please |
| connector.setPort(port) | ||
| server.addConnector(connector) | ||
| server.setHandler(contextHandler) | ||
| server.start() |
There was a problem hiding this comment.
does this log some kind of "server started on hostname:port" message? Want to have a clear "I'm ready" message to look for in server logs
There was a problem hiding this comment.
I don't think we have the hostname here but Spark tends to log "Started server listening on port X", and we should do something similar here.
| } | ||
|
|
||
| private[spark] class ResourceStagingServerSslOptionsProviderImpl(sparkConf: SparkConf) | ||
| extends ResourceStagingServerSslOptionsProvider { |
There was a problem hiding this comment.
I find a server logging all its non-sensitive config on startup helps with debugging later -- maybe DEBUG log everything in SparkConf and make sure DEBUG is enabled for this class?
| "Shouldn't provide both the keyStore password value and the keyStore password file.") | ||
| requireNandDefined(baseSslOptions.keyPassword, maybeKeyPasswordFile, | ||
| "Shouldn't provide both the keyStore key password value and the keyStore key password file.") | ||
| maybeKeyPem.foreach { _ => |
There was a problem hiding this comment.
refactor to a requireSecondIfFirst method
| class ResourceStagingServerSslOptionsProviderSuite extends SparkFunSuite with BeforeAndAfter { | ||
|
|
||
| private val SSL_TEMP_DIR = Utils.createTempDir(namePrefix = "resource-staging-server-ssl-test") | ||
| private val KEYSTORE_FILE = new File(SSL_TEMP_DIR, "keyStore.jks") |
There was a problem hiding this comment.
convention is all-lowercase keystore.jks I think
There was a problem hiding this comment.
The convention is to capitalize the "S" on keyStore.
| sparkConf = new SparkConf(true) | ||
| sslOptionsProvider = new ResourceStagingServerSslOptionsProviderImpl(sparkConf) | ||
| } | ||
|
|
There was a problem hiding this comment.
clear the SSL_TEMP_DIR between tests to reduce risk of polluting other test runs
| server.stop() | ||
| test("Enable SSL on the server") { | ||
| val (keyStore, trustStore) = SSLUtils.generateKeyStoreTrustStorePair( | ||
| "127.0.0.1", "changeit", "changeit", "changeit") |
There was a problem hiding this comment.
make these 3 values different so it's clear what's what and make sure the values a couple lines down are lined up
ash211
changed the base branch from
split-v1-and-v2-submission-packages
to
branch-2.1-kubernetes
|
rerun unit tests please |
* Staging server for receiving application dependencies. * Move packages around to split between v1 work and v2 work * Add unit test for file writing * Remove unnecessary main * Allow the file staging server to be secured with TLS. * Add back license header * Minor fixes * Fix integration test with renamed package for client. Fix scalastyle. * Remove unused import * Force json serialization to consider the different package. * Revert extraneous log * Fix scalastyle * Remove getting credentials from the API We still want to post them because in the future we can use these credentials to monitor the API server and handle cleaning up the data accordingly. * Fix build * Randomize name and namespace in test to prevent collisions * Generalize to resource staging server outside of Spark * Update code documentation * Val instead of var * Fix unit tests. * Fix build * Fix naming, remove unused import * Move suites from integration test package to core * Fix unit test * Use TrieMap instead of locks * Address comments * Fix imports * Address comments * Change main object name * Change config variable names * Change paths, use POST instead of PUT * Use a resource identifier as well as a resource secret
* Staging server for receiving application dependencies. * Move packages around to split between v1 work and v2 work * Add unit test for file writing * Remove unnecessary main * Allow the file staging server to be secured with TLS. * Add back license header * Minor fixes * Fix integration test with renamed package for client. Fix scalastyle. * Remove unused import * Force json serialization to consider the different package. * Revert extraneous log * Fix scalastyle * Remove getting credentials from the API We still want to post them because in the future we can use these credentials to monitor the API server and handle cleaning up the data accordingly. * Fix build * Randomize name and namespace in test to prevent collisions * Generalize to resource staging server outside of Spark * Update code documentation * Val instead of var * Fix unit tests. * Fix build * Fix naming, remove unused import * Move suites from integration test package to core * Fix unit test * Use TrieMap instead of locks * Address comments * Fix imports * Address comments * Change main object name * Change config variable names * Change paths, use POST instead of PUT * Use a resource identifier as well as a resource secret
Allow providing a keyStore or key+certificate PEM files to have the staging server listen over TLS.
Note that we're also preemptively allowing the server's configuration to be provided in a properties file. This is to allow properties to be provided through a ConfigMap more easily if this file staging server were to be deployed in Kubernetes. The alternative would be to specify all of these properties as system properties or command-line arguments, but I think our previous work on similar constructs has shown that parsing out command line arguments and expecting them to be set correctly in the Dockerfile can be tedious.
In the case that the user prefer not to have the key and keystore passwords be provided through the ConfigMap - e.g. they would prefer to mount K8s secrets - the key and keystore password values can be provided in files as well.