Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[SPARK-16987] [None] Add spark-default.conf property to define https port for spark history server #15652

Closed
wants to merge 11 commits into from
Closed

[SPARK-16987] [None] Add spark-default.conf property to define https port for spark history server #15652

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
955a82c
added secure port configuration
Oct 26, 2016
8ac0369
added configuration
Oct 26, 2016
47108b4
change config description and fix some typos.
Oct 27, 2016
3964609
Return the unintended change
Oct 27, 2016
1e2c985
deleted unnecessary brank spaces
Nov 4, 2016
935415c
fixed documentation about particular protocol
Nov 5, 2016
62aeb6c
added comment about particular protocols
Nov 6, 2016
bd945aa
reflected some comments, and improved for trivial issues in ml library.
Nov 9, 2016
9d69f02
reflected comment
chie8842 Nov 10, 2016
30a2927
rebased unrelated changes
Nov 11, 2016
83df9bc
solve conflict between global and specific configuration
chie8842 Nov 18, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions core/src/main/scala/org/apache/spark/SSLOptions.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ private[spark] case class SSLOptions(
trustStorePassword: Option[String] = None,
trustStoreType: Option[String] = None,
protocol: Option[String] = None,
port: Int = 0,
enabledAlgorithms: Set[String] = Set.empty)
extends Logging {

Expand Down Expand Up @@ -147,6 +148,7 @@ private[spark] object SSLOptions extends Logging {
* $ - `[ns].trustStorePassword` - a password to the trust-store file
* $ - `[ns].trustStoreType` - the type of trust-store
* $ - `[ns].protocol` - a protocol name supported by a particular Java version
* $ - `[ns].port` - a port number
* $ - `[ns].enabledAlgorithms` - a comma separated list of ciphers
*
* For a list of protocols and ciphers supported by particular Java versions, you may go to
Expand Down Expand Up @@ -191,6 +193,8 @@ private[spark] object SSLOptions extends Logging {
val protocol = conf.getOption(s"$ns.protocol")
.orElse(defaults.flatMap(_.protocol))

val port = conf.getInt(s"$ns.port", defaultValue = defaults.map(_.port).getOrElse(0))

val enabledAlgorithms = conf.getOption(s"$ns.enabledAlgorithms")
.map(_.split(",").map(_.trim).filter(_.nonEmpty).toSet)
.orElse(defaults.map(_.enabledAlgorithms))
Expand All @@ -207,6 +211,7 @@ private[spark] object SSLOptions extends Logging {
trustStorePassword,
trustStoreType,
protocol,
port,
enabledAlgorithms)
}

Expand Down
12 changes: 10 additions & 2 deletions core/src/main/scala/org/apache/spark/ui/JettyUtils.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,14 +308,22 @@ private[spark] object JettyUtils extends Logging {

sslOptions.createJettySslContextFactory().foreach { factory =>
// If the new port wraps around, do not try a privileged port.

require(sslOptions.port == 0 || (1024 <= sslOptions.port && sslOptions.port < 65536))

val securePort =
if (currentPort != 0) {
(currentPort + 400 - 1024) % (65536 - 1024) + 1024
if (1024 < sslOptions.port && sslOptions.port < 65536) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This condition should only happen when the port is 0 right? given the require above. It would be clearer to check against 0 only.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the require() above you have 1024 <= sslOptions.port but in this if() you have 1024 < sslOptions.port. Just wanted to check if thats intentional.

sslOptions.port
} else {
(currentPort + 400 - 1024) % (65536 - 1024) + 1024
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment in the code explaining the math done here ? It will help readability of the code.

}
} else {
0
}
val scheme = "https"
// Create a connector on port securePort to listen for HTTPS requests
// Create a connector on port securePort to listen for HTTPS requests.

val connector = new ServerConnector(server, factory)
connector.setPort(securePort)

Expand Down
2 changes: 2 additions & 0 deletions core/src/test/scala/org/apache/spark/SSLOptionsSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ class SSLOptionsSuite extends SparkFunSuite with BeforeAndAfterAll {
"TLS_RSA_WITH_AES_128_CBC_SHA, TLS_RSA_WITH_AES_256_CBC_SHA")
conf.set("spark.ui.ssl.enabledAlgorithms", "ABC, DEF")
conf.set("spark.ssl.protocol", "SSLv3")
conf.set("spark.ssl.port", "18999")

val defaultOpts = SSLOptions.parse(conf, "spark.ssl", defaults = None)
val opts = SSLOptions.parse(conf, "spark.ui.ssl", defaults = Some(defaultOpts))
Expand All @@ -128,6 +129,7 @@ class SSLOptionsSuite extends SparkFunSuite with BeforeAndAfterAll {
assert(opts.keyStorePassword === Some("12345"))
assert(opts.keyPassword === Some("password"))
assert(opts.protocol === Some("SSLv3"))
assert(opts.port === 18999)
assert(opts.enabledAlgorithms === Set("ABC", "DEF"))
}

Expand Down
10 changes: 9 additions & 1 deletion docs/configuration.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1598,7 +1598,7 @@ Apart from these, the following properties are also available, and may be useful
<td>false</td>
<td>
<p>Whether to enable SSL connections on all supported protocols.</p>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you can, remove this change

<p>When <code>spark.ssl.enabled</code> is configured, <code>spark.ssl.protocol</code>
is required.</p>

Expand Down Expand Up @@ -1663,6 +1663,14 @@ Apart from these, the following properties are also available, and may be useful
page.
</td>
</tr>
<tr>
<td><code>spark.ssl.<particular protocol>.port</code></td>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does <particular protocol> mean here ? Is this config name parameterized ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree; see #15652 (comment) This should just refer to spark.ssl.port because the rest is documented above.

Copy link
Contributor Author

@chie8842 chie8842 Nov 3, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has a reason for this.
If an user set spark.ssl.port, the port of history server, SparkUI, MasterWebUI, and WorkerWebUI will collapse as I mentioned at the beginning of this PR.
It is because WebUI.scala is inheried by HistoryServer.scala, SparkUI.scala, MasterWebUI.scala, and WorkerWebUI.scala.
How do you think about it?

I created a pull request, but there is a problem.
Port should be separated for each particular process such as history server and spark
ui.
But user can configure spark.ssl.port with this code and when configure it, each
process will try to create connections with same port and it doesn't work good.
Because of structure of existing function, it has impact to other functions to solve the
problem.
So there are three answer about it.

Use this fix code so user can configure a specific secure port, but spark.ssl.port doesn't > work good.
Don't change and close issue.
We should be able to configure a specific secure port completely, so drastically change > many functions.
Please comments.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, setting spark.ssl.port sets the SSL port for all services. That's how it's documented to work above, and you can override with spark.ui.ssl.port, etc. I'm just saying that this is already documented, and to be consistent with other props, the config key in this section can be spark.ssl.port -- or did I misunderstand?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are saying about following documentation at "spark.ssl.enabled", aren't you?
<p>Use <code>spark.ssl.YYY.XXX</code> settings to overwrite the global configuration for particular protocol denoted by <code>YYY</code>. Example values for <code>YYY</code> include <code>fs</code>, <code>ui</code>, <code>standalone</code>, and <code>historyServer</code>. See <a href="security.html#ssl-configuration">SSL Configuration</a> for details on hierarchical SSL configuration for services.</p>
I think I should refer about particular process at spark.ssl.port again. because another ssl parameter doesn't clash processes with grobal configuration and only "spark.ssl.port" clash processes. But if it's not so important, I'll fix documentation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that's what I'm referring to. I would still use spark.ssl.port as the key for this section for consistency. It's OK to maybe add some text reminding the reader that this port can and probably should be set separately for each service.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I understood. I refered about attention, and changed the key to spark.ssl.port.

<td>0</td>
<td>
Port number to listen on for SSL connections.
Default value of 0 means the port will be determined automatically.
</td>
</tr>
<tr>
<td><code>spark.ssl.needClientAuth</code></td>
<td>false</td>
Expand Down