[SPARK-17874][core] Add SSL port configuration. #16625
Conversation
Make the SSL port configuration explicit, instead of deriving it from the non-SSL port, but retain the existing functionality in case anyone depends on it. The change starts the HTTPS and HTTP connectors separately, so that it's possible to use independent ports for each. For that to work, the initialization of the server needs to be shuffled around a bit. The change also makes it so the initialization of both connectors is similar, and end up using the same Scheduler - previously only the HTTP connector would use the correct one. Also removed some outdated documentation about a SSL config namespace that was removed a long time ago. Tested with unit tests and by running spark-shell with SSL configs.
| <td>Empty</td> | ||
| <td> | ||
| A comma separated list of ciphers. The specified ciphers must be supported by JVM. | ||
| The reference list of protocols one can find on |
There was a problem hiding this comment.
pretty sure it was grammatically correct before
|
|
||
| val port = conf.getOption(s"$ns.port").map(_.toInt) | ||
| port.foreach { p => | ||
| require(p >= 0, "Port number must be a positive value.") |
There was a problem hiding this comment.
nit. positive -> non-negative?
|
Test build #71534 has finished for PR 16625 at commit
|
|
Test build #71543 has finished for PR 16625 at commit
|
|
Test build #71551 has finished for PR 16625 at commit
|
|
Test build #71622 has finished for PR 16625 at commit
|
|
Test build #72054 has finished for PR 16625 at commit
|
|
@sarutak want to look at this since you looked at the somewhat mildly related PR? |
|
I'll take a look at this during this week. |
| Configuration</a> for details on hierarchical SSL configuration for services. | ||
| </td> | ||
| </tr> | ||
| <tr> |
There was a problem hiding this comment.
How about saying spark.ssl.port instead to be consistent with any other property related to SSL?
There was a problem hiding this comment.
This was intentional. "spark.ssl.port" doesn't make that much sense if you think about it; you want things like the master UI and history server UI to have different, well known ports, so having this shared config key here doesn't make a lot of sense. For the other configs, such as algorithms and keystore locations, sharing configuration is ok.
| <th>Config Namespace</th> | ||
| <th>Component</th> | ||
| </tr> | ||
| <tr> |
There was a problem hiding this comment.
The namespace fs seems to be still referred (https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/SecurityManager.scala#L259 ).
We shouldn't remove this description should we?
Also it's chance to modify the description about the namespace in SecurityManager.scala.
* All the SSL settings like `spark.ssl.xxx` where `xxx` is a particular configuration property,
* denote the global configuration for all the supported protocols. In order to override the global
* configuration for the particular protocol, the properties must be overwritten in the
* protocol-specific namespace. Use `spark.ssl.yyy.xxx` settings to overwrite the global
* configuration for particular protocol denoted by `yyy`. Currently `yyy` can be only`fs` for
* broadcast and file server.
There was a problem hiding this comment.
Hmmm... that code is actually being used, but not to set up the file server, but to configure HTTP clients that download from SSL-enabled servers. Let me see about making that clear in the configuration docs.
| val httpsURI = createRedirectURI(scheme, baseRequest.getServerName, securePort, | ||
| baseRequest.getRequestURI, baseRequest.getQueryString) | ||
| response.setContentLength(0) | ||
| response.encodeRedirectURL(httpsURI) |
|
Test build #72470 has finished for PR 16625 at commit
|
|
Unless I hear back I'll push this tomorrow morning. |
|
LGTM. Merging into |
Make the SSL port configuration explicit, instead of deriving it from the non-SSL port, but retain the existing functionality in case anyone depends on it. The change starts the HTTPS and HTTP connectors separately, so that it's possible to use independent ports for each. For that to work, the initialization of the server needs to be shuffled around a bit. The change also makes it so the initialization of both connectors is similar, and end up using the same Scheduler - previously only the HTTP connector would use the correct one. Also fixed some outdated documentation about a couple of services that were removed long ago. Tested with unit tests and by running spark-shell with SSL configs. Author: Marcelo Vanzin <[email protected]> Closes apache#16625 from vanzin/SPARK-17874.
Make the SSL port configuration explicit, instead of deriving it from the non-SSL port, but retain the existing functionality in case anyone depends on it. The change starts the HTTPS and HTTP connectors separately, so that it's possible to use independent ports for each. For that to work, the initialization of the server needs to be shuffled around a bit. The change also makes it so the initialization of both connectors is similar, and end up using the same Scheduler - previously only the HTTP connector would use the correct one. Also fixed some outdated documentation about a couple of services that were removed long ago. Tested with unit tests and by running spark-shell with SSL configs. Author: Marcelo Vanzin <[email protected]> Closes apache#16625 from vanzin/SPARK-17874.
5 participants
Make the SSL port configuration explicit, instead of deriving it
from the non-SSL port, but retain the existing functionality in
case anyone depends on it.
The change starts the HTTPS and HTTP connectors separately, so
that it's possible to use independent ports for each. For that to
work, the initialization of the server needs to be shuffled around
a bit. The change also makes it so the initialization of both
connectors is similar, and end up using the same Scheduler - previously
only the HTTP connector would use the correct one.
Also fixed some outdated documentation about a couple of services
that were removed long ago.
Tested with unit tests and by running spark-shell with SSL configs.