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.

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

[SPARK-2750] support https in spark web ui #1980

Closed
wants to merge 24 commits into from
Closed

Conversation

scwf
Copy link
Contributor

@scwf scwf commented Aug 16, 2014

https://issues.apache.org/jira/browse/SPARK-2750

enable spark web ui to support https, also compatible with old web ui(http).

user can switch between https and http by configure "spark.http.policy".

@SparkQA
Copy link

SparkQA commented Sep 5, 2014

Can one of the admins verify this patch?

@andrewor14
Copy link
Contributor

ok to test

@andrewor14
Copy link
Contributor

Looks like this is a duplicate of #1714, which is now closed, however. @WangTaoTheTonic you originally filed the issue to support this. Is there a particular motivation? How is this related to the view ACLs we already have (http://spark.apache.org/docs/latest/security.html)?

@SparkQA
Copy link

SparkQA commented Sep 11, 2014

QA tests have started for PR 1980 at commit 8f7cc96.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 11, 2014

QA tests have finished for PR 1980 at commit 8f7cc96.

  • This patch passes unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@WangTaoTheTonic
Copy link
Contributor

I did not make very much stuty of SecurityManager, but it only does authentication while not the encryption in communication.
I am not sure if it is appropriate using https to ensure the security in Spark ui communication, and not considering consistency of encryption in different underlying mechanisms.
BTW, @scwf is my workmate. We wanna encrypt the Web ui communiation now, maybe other protocols in future.

@@ -30,7 +30,7 @@ private[spark] class WorkerInfo(
val cores: Int,
val memory: Int,
val actor: ActorRef,
val webUiPort: Int,
val webUiAddress: String,
val publicAddress: String)
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this publicAddress field is never read anymore, so we should remove it.

@JoshRosen
Copy link
Contributor

Is spark.http.policy the best name for this configuration option? Do you think that this can be a boolean option, or are there cases for wanting to have values other than http and https? It looks like Hadoop has a dfs.http.policy option that accepts three values, HTTP_ONLY, HTTPS_ONLY, and HTTP_AND_HTTPS.

Whatever configuration options we end up using, we should add documentation for them to the Spark Security and Configuration guides.

@@ -53,6 +53,9 @@ private[spark] class WorkerArguments(args: Array[String], conf: SparkConf) {
if (System.getenv("SPARK_WORKER_DIR") != null) {
workDir = System.getenv("SPARK_WORKER_DIR")
}
if (conf.contains("worker.ui.port")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be spark.worker.ui.port.

Copy link
Contributor

Choose a reason for hiding this comment

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

In fact, spark.worker.ui.port is already read on line 51, so this is unnecessary.

@JoshRosen
Copy link
Contributor

Also, it looks like this adds configuration options under several (new) namespaces:

  • spark.http.policy
  • spark.client.https.need-auth
  • spark.ssl.server.keystore.*
  • spark.ssl.server.truststore.*

Are these the best names? What if we decide to add HTTPS / SSL to other components?

What is spark.client?

@JoshRosen
Copy link
Contributor

What happens if I've configured the web UI to use https then attempt to browse to the http URL? Is it easy to set up an automatic redirect?

@SparkQA
Copy link

SparkQA commented Sep 21, 2014

QA tests have started for PR 1980 at commit c90d84e.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 21, 2014

QA tests have finished for PR 1980 at commit c90d84e.

  • This patch fails unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@scwf
Copy link
Contributor Author

scwf commented Sep 21, 2014

@JoshRosen, here i named the configuration options refer to how hadoop does(actually just use spark instead hadoop). spark.client is the browser which connect to UIServer.
Now browse to httpui when configure http will get error, and i will check that how to set up automatic redirect(maybe configure this in jetty )

@SparkQA
Copy link

SparkQA commented Sep 21, 2014

QA tests have started for PR 1980 at commit de8d1bd.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 21, 2014

QA tests have started for PR 1980 at commit 35074fd.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 21, 2014

QA tests have finished for PR 1980 at commit de8d1bd.

  • This patch fails unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Sep 21, 2014

QA tests have finished for PR 1980 at commit 35074fd.

  • This patch fails unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Sep 21, 2014

QA tests have started for PR 1980 at commit 967950f.

  • This patch merges cleanly.

val url = newURI(schema, baseRequest.getServerName, securePort,
baseRequest.getRequestURI, baseRequest.getQueryString)
response.setContentLength(0)
response.sendRedirect(url)
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be a good idea to call response.encodeRedirectURL() first.

@vanzin
Copy link
Contributor

vanzin commented Oct 6, 2014

For some reason I downloaded the patch but it doesn't apply cleanly to my copy of the repo... I'll try again later.

@SparkQA
Copy link

SparkQA commented Oct 7, 2014

QA tests have started for PR 1980 at commit 8b32853.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Oct 7, 2014

QA tests have finished for PR 1980 at commit 8b32853.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@@ -205,10 +231,74 @@ private[spark] object JettyUtils extends Logging {
ServerInfo(server, boundPort, collection)
}

// to generate a new url string scheme://server:port+path
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @scwf,

The reason I asked for a comment is that it seems like the method is doing a little more than just that. For example, L238 seems to be doing some sort of parsing of the server string, so it's more than just concatenating the different arguments into a URL.

It would be nice if the comment explained exactly what the relationship between the input and the output is. A unit test wouldn't hurt either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @vanzin, actually here i just refer to the code of jetty 9 see(https://github.com/eclipse/jetty.project/blob/master/jetty-util/src/main/java/org/eclipse/jetty/util/URIUtil.java#L726-L733) since there is no newURImethod in spark jetty version(spark use jetty 8).

And L238 is for the case to handle IPv6 address, here we can remove it if unnecessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you put that reference in the code itself, so we know where it comes from? Thanks!

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, updated.

@SparkQA
Copy link

SparkQA commented Oct 10, 2014

QA tests have started for PR 1980 at commit 3b01d3a.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Oct 10, 2014

Tests timed out for PR 1980 at commit 3b01d3a after a configured wait of 120m.

@SparkQA
Copy link

SparkQA commented Oct 10, 2014

QA tests have started for PR 1980 at commit 4ae834b.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Oct 10, 2014

QA tests have finished for PR 1980 at commit 4ae834b.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@jacek-lewandowski
Copy link
Contributor

@scwf can you take a look at #2739? I created PR which aim is to add SSL support for Akka based connections and for HttpServer which in turn bases on Jetty. I use SSLOptions object to configure SSL. All the settings are being kept in a separate file ssl.conf. You can use different name spaces to separate configurations for say UI, control and data. Would you take a look? It would be nice to have a single place to configure SSL.

@jacek-lewandowski
Copy link
Contributor

Btw. I was trying to do SSL for UI as well. One thing I'm afraid of is that this solution would break any recovery information for Spark Master (different signature of WorkerInfo). Also, it doesn't allow to run the cluster in mixed mode during upgrade - some nodes running older Spark and some nodes running newer Spark, because it changes the signature of messages. Anyway, this is very minor I think.

@scwf
Copy link
Contributor Author

scwf commented Oct 23, 2014

@jacek-lewandowski, you mean in ssl.conf to set ssl configs for ui and akka and httpserver? then ssl.con f is the single place to configure ssl, right? if so, i think it's ok.

For the second one "some nodes running older Spark and some nodes running newer Spark", i don't think there is this situation.

@JoshRosen @vanzin is this ok to go? I think we can merge this first then in #2739 to unify the ssl configs.

@JoshRosen
Copy link
Contributor

Hi @scwf,

I just merged #3571, which added SSL support for Akka and the HTTPServer. That PR added a common configuration mechanism for SSL, plus some useful utilities for testing this functionality. If you're interested, it would be great to revisit HTTPS for the web UI now that we've merged that other patch.

@scwf
Copy link
Contributor Author

scwf commented Feb 3, 2015

@JoshRosen in that PR now no https support for web ui right? if so i will rework this to support it.

@JoshRosen
Copy link
Contributor

@scwf Yep, that PR only added SSL support to Akka and the HTTP FileServer, not the web UI or the shuffle service.

@scwf
Copy link
Contributor Author

scwf commented Apr 23, 2015

i am closing this in favor of #5664

@scwf scwf closed this Apr 23, 2015
WangTaoTheTonic added a commit to WangTaoTheTonic/spark that referenced this pull request Apr 23, 2015
@pritpalm
Copy link

I want to enable https on spark UI. I added following config to spark-defaults.config, but when we access spark ui via https::/:8080 or https://:443 or https://:8480, it's not able to connect.

spark.ui.https.enabled true
spark.ssl.keyPassword abcd
spark.ssl.keyStore rtmqa-clientid.jks
spark.ssl.keyStorePassword changeit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants