Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/3571#issuecomment-70435538
Hi @jacek-lewandowski,
Thanks for bringing this up to date. I took a quick pass through and left
some minor comments.
Just to clarify: this only adds SSL support for internal HttpServer and
Akka traffic, and not the Spark web UI? When we last discussed this in #2739,
I think the idea was that SSLOptions could use namespaced configs in order to
allow the web UI to use different SSL configurations than, say, Akka. I see
that there's some namespace support built into this patch (the `ns` argument to
`parse`); is this support sufficient to support HTTPs in the UI? Also, does it
support scenarios where I want to enable SSL only for the UI or only for Akka?
Settings like `spark.ssl.enabled` sound like they're systemwide settings, so we
should think through how these might interact with different UI configurations,
etc. I'm not asking to implement SSL for the UI in this patch, but I'd like to
just make sure that the SSLOptions configuration code will be compatible with
it.
It would be great if you could add a short summary of this PR's changes to
the PR description, since that description will become this PR's commit message.
There's a big block comment at the top of `SecurityManager.scala` which
should be updated to reflect this PR's changes (it currently says " We
currently do not support SSL (https) ...").
It would also be great to add a small section to the security documentation
(`docs/security.md`) to mention how to configure this. The documentation
should mention the relevant Spark options, describe how/why someone would use
the `useNodeLocalConf` setting, etc. It could also contain a pointer to
external instructions for generating your own keystore / truststores, etc.,
since this isn't a trivial process. The new configuration options should also
be documented in `docs/configuration.md` alongside the other security
configurations. In addition, the documentation should describe how the key
stores are / aren't distributed depending on the choice of cluster manager. If
this works in fundamentally different ways on different cluster managers, then
the docs should make this clear so users know what to expect.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
