Skip to content

LIVY-615: Set context path using property SERVER_BASE_PATH. #188

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

Open
fdeantoni wants to merge 6 commits into
base: master
Choose a base branch
from
+12 −8
Open

LIVY-615: Set context path using property SERVER_BASE_PATH. #188

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a114e0c
LIVY-615: Set context path using property SERVER_BASE_PATH.
fdeantoni Jul 31, 2019
afde7d4
LIVY-615: Validate livy.ui.basePath property
fdeantoni Jul 31, 2019
383fecb
LIVY-615: Require check removed as contextPath variable will always
fdeantoni Aug 2, 2019
c55e165
LIVY-615: Property livy.ui.basePath renamed to livy.server.basePath
fdeantoni Aug 5, 2019
4bae94c
Merge branch 'master' into LIVY-615
fdeantoni Apr 9, 2020
8b1511a
Merge branch 'master' into LIVY-615
fdeantoni Oct 12, 2020
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
6 changes: 3 additions & 3 deletions conf/livy.conf.template
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,9 @@
# What port to start the server on.
# livy.server.port = 8998

# What base path ui should work on. By default UI is mounted on "/".
# E.g.: livy.ui.basePath = /my_livy - result in mounting UI on /my_livy/
# livy.ui.basePath = ""
# What base path the server should use. By default the server is mounted on "/".
# E.g.: livy.server.basePath = /my_livy - result in mounting on /my_livy/
# livy.server.basePath =

# What spark master Livy sessions should use.
# livy.spark.master = local
Expand Down
5 changes: 3 additions & 2 deletions server/src/main/scala/org/apache/livy/LivyConf.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ object LivyConf {

val SERVER_HOST = Entry("livy.server.host", "0.0.0.0")
val SERVER_PORT = Entry("livy.server.port", 8998)
val SERVER_BASE_PATH = Entry("livy.ui.basePath", "")
val SERVER_BASE_PATH = Entry("livy.server.basePath", "")

val UI_ENABLED = Entry("livy.ui.enabled", true)

Expand Down Expand Up @@ -272,7 +272,8 @@ object LivyConf {
YARN_APP_LEAKAGE_CHECK_TIMEOUT.key ->
DepConf("livy.server.yarn.app-leakage.check_timeout", "0.4"),
YARN_APP_LEAKAGE_CHECK_INTERVAL.key ->
DepConf("livy.server.yarn.app-leakage.check_interval", "0.4")
DepConf("livy.server.yarn.app-leakage.check_interval", "0.4"),
SERVER_BASE_PATH.key -> DepConf("livy.ui.basePath", "0.7")
)

private val deprecatedConfigs: Map[String, DeprecatedConf] = {
Expand Down
4 changes: 3 additions & 1 deletion server/src/main/scala/org/apache/livy/server/LivyServer.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,9 @@ class LivyServer extends Logging {

val host = livyConf.get(SERVER_HOST)
val port = livyConf.getInt(SERVER_PORT)
val basePath = livyConf.get(SERVER_BASE_PATH)
val basePath = livyConf.get(SERVER_BASE_PATH).stripSuffix("/")

@ajbozarth ajbozarth Aug 1, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Based on the code you wrote here you should be able to leave the default value in livy.conf.template and LivyConf.scala as "" rather than / since you immediately remove the trailing / here.

But nice catch about the trailing /, removing that is key to the js code functioning correctly and would have been a mis-config triggered bug

@fdeantoni fdeantoni Aug 2, 2019 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

You are right that the default could actually stay an empty string, but after some pondering I changed the default for two reasons:

  1. The original double quote in the template to indicate an empty string (i.e. "") actually causes the contextPath to be come http://localhost:8998/""/ui when enabled (this at least when I tested it). So the template should actually be livy.ui.basePath = but by changing it I figured I might as well use livy.ui.basePath = / because
  2. the basePath must start with a /. I figured by having the default a / it would give a hint to the user that this is the minimum this property must have.

@ajbozarth ajbozarth Aug 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

that makes sense and my bad, I had suggested to change livy.ui.basePath = to livy.ui.basePath = "" in the previous PR not realizing what it'd do. But since this is an optional config maybe it'd be better to just remove it from the template and leave the default in the code as "", I would argue leaving it unset is better than setting it to the default in a config.

@fdeantoni fdeantoni Aug 2, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Alright, I will remove it from the template and set the default to empty string.

@fdeantoni fdeantoni Aug 2, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thinking about the livy.conf.template some more, maybe we should still keep it there? That is actually how I found out about the possibility of adding a basePath. Perhaps we can just tweak it to the following:

# The base path the server should use. By default the server is mounted on "/".
# E.g.: livy.server.basePath = /my_livy - results in mounting on /my_livy/
# livy.server.basePath =

There are plenty of other options in that file with empty string as default (e.g. livy.spark.deploy-mode) so having it like that would not make this config option an odd one out of the pack.

@ajbozarth ajbozarth Aug 5, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That's a good point lets leave it in the config template # livy.server.basePath = then

require(basePath.isEmpty || (basePath.length > 1 && basePath.startsWith("/")),
s"Configuration property ${SERVER_BASE_PATH.key} must start with /, e.g. /my-livy")
val multipartConfig = MultipartConfig(
maxFileSize = Some(livyConf.getLong(LivyConf.FILE_UPLOAD_MAX_SIZE))
).toMultipartConfigElement
Expand Down
5 changes: 3 additions & 2 deletions server/src/main/scala/org/apache/livy/server/WebServer.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,8 @@ class WebServer(livyConf: LivyConf, var host: String, var port: Int) extends Log

val context = new ServletContextHandler()

context.setContextPath("/")
private val contextPath = "/" + livyConf.get(LivyConf.SERVER_BASE_PATH).stripPrefix("/")

@jerryshao jerryshao Jul 31, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Seems like this will also change the path of REST API, am I right? IIUC the configuration is only used for UI, so the fix should only be in UI side.

@fdeantoni fdeantoni Jul 31, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

That is correct. There seems to be a mismatch between the property name SERVER_BASE_PATH and its property key "livy.ui.basePath" in the configuration file. The key implies it only changes the UI, whilst the property name assumes it changes the base path for the entire server (i.e. UI and API).

When I looked through the history of this property, it seemed it was added to support reverse proxying (see LIVY-468). Indeed, the property is used in both the WebServer and LivyServer classes, so it seems its intention was to apply them to UI and API.

Is there a need to have separate context paths for UI and API? Our deployment scenario (using reverse proxying) does not require it. If there is a need to separate the two, then perhaps a new property livy.server.basePath needs to be added. This may introduce some additional complexities though.

Perhaps a better alternative is to simply rename livy.ui.basePath to livy.server.basePath so that it is clear it changes the context path of the entire server?

@jerryshao jerryshao Jul 31, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Indeed, the property is used in both the WebServer and LivyServer classes, so it seems its intention was to apply them to UI and API.

From the changes in LIVY-468, seems it only applies to UI part, not the API part.

Also can you please describe how do you get 404 error when configured this, how to reproduce this issue.

@fdeantoni fdeantoni Jul 31, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I see. I must have misunderstood that part. To reproduce I simply set the following in the conf file:

livy.ui.basePath = /my-livy

Then when accessing the URL http://localhost:8998/my-livy I get a 404 error. If I access http://localhost:8998 it redirects to http://localhost:8998/my-livy/ui with a 404 error.

Note that the API URLs are still accessible on http://localhost:8998/sessions and http://localhost:8998/batches, but I assumed wrongly that this was not the intention.

Am I missing perhaps another configuration option that should be set to make the UI work correctly?

If the intention of LIVY-458 was to only set the context path for the UI, then perhaps LIVY-615 (and this pull request) can be converted into a change request to also support setting the context path for the API? For our deployment scenario we need the API reverse proxied (not so much the UI but it is nice if that one works as well).

@jerryshao jerryshao Jul 31, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think LIVY-468 only related to UI part, if it cannot be worked, then we should fix it.

Mostly for the UI part, because of the path of the static files, so it is necessary to add a prefix to correctly load the files. But for the API part, I'm not so sure if it is necessary to add prefix. Typically when using nginx to do reverse proxy, it is not necessary to set prefix, nginx will properly rewrite the request.

@fdeantoni fdeantoni Jul 31, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I tried using it with HAProxy but was having trouble with it. This made me dig deeper and try to set the livy.ui.basePath property, which let me to the UI not working even without reverse proxy, etc, etc.

If a basePath can be set for the API, then the HAProxy configuration is quite trivial, e.g.

frontend livy_https_in
  bind *:443 ssl crt /etc/ssl/cert.pem
  mode http
  acl path_livy_10050 path_beg /my-livy

Without it it gets a lot more complex.

@jerryshao jerryshao Jul 31, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The API part is actually another problem. Here since your original problem is related to UI, let's focus on solving UI issue first.

@fdeantoni fdeantoni Aug 1, 2019 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

To fix setting the basePath only for the UI, I think you would have to add the basePath to the mount of the UI servlets and remove the UI Redirect servlet, i.e. line 222 onwards in LivyServer.scala would look as follows:

          mount(context, uiServlet, basePath + "/ui/*")
          mount(context, staticResourceServlet, basePath + "/static/*")
          //mount(context, uiRedirectServlet(basePath + "/ui/"), "/*")

After that is done, all the javascript codes need to be updated to remove the preprendBasePath() method from any XHR method that needs to access either batches or sessions API.

Note that applying these changes will break the implementation currently in this pull request.

Considering that this feature seems to be broken in the 0.6.0 release, and that it did not exist yet in the 0.5.0 release, is the "UI only" basePath requirement actually used? Since the UI uses the API as well, I do not see how keeping the UI elements on a different basePath actually makes sense from a deployment perspective? The UI will break (like it is now) if you do not give it access to the API.

If the UI only basePath property is used in production somehow (not sure how though since it seems to be broken) then perhaps we can introduce a second property livy.server.basePath that will set the basePath for both the API and UI. So by default the livy.ui.basePath would equal the livy.server.basePath. Only if you specifically set the livy.ui.basePath property will the UI be "mounted" on a different path from the API...

So for this one I think we have three options:

  1. Keep the current broken implementation
  2. Rename livy.ui.basePath to livy.server.basePath and have it set the contextPath (i.e. for both API and UI like this pull request currently does). This would be a relatively minor change.
  3. Implement livy.server.basePath alongside livy.ui.basePath where livy.server.basePath will set the basePath for both API and UI, and livy.ui.basePath the basePath only for the UI. This would also require changes to the JS code to implement the additional basePath property for sessions and batches URLs.

A fourth option would of course be to revert LIVY-468 but I definitely dont want to do that! It would be a shame to remove all the work done so far to make the basePath configurable. Option 2 or 3 seem to be the most sensible I think, but that is purely my opinion. What do you guys think?

BTW, also just want to say also that Livy is a great project and hope it will come out of incubating status soon!

@ajbozarth ajbozarth Aug 1, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ok so I think I'm up to speed, but just to make sure. You're saying that the reverse proxy featured we added a while ago just got released in 0.6 but is broken and doesn't work (I would assume normal non-reverse proxy works fine). The problem being that the implementation meant to only tinker with the UI but needed to affect the whole server instead. This PR is a fix that tweaks the reverse proxy config to affect the whole server rather than the UI only. This would bring the usage in line with how the code was written rather that how it was intended to work. You chose this option because bringing the code in line with its intention would require more and potentially messy changes as you explained. IIUC I believe this PR is the correct course of action and if merged would warrant a patch release.

@ajbozarth ajbozarth Aug 1, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As a follow up I read through the original PR and both this and its JIRAs to trace the intention, implementation, conversations, etc. Based on the original PR conversation I believe this PR does what the original author intended. I believe @jerryshao and myself misunderstood the author's intention previously and thus missed the necessity of this extra bit of code. With this in mind I'll do a proper code review of this PR

context.setContextPath(contextPath)

@ajbozarth ajbozarth Aug 1, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

with the change to context here do the ui related mounts below still work as expected? Especially the uiRedirectServlets or do they need to be updated?

@fdeantoni fdeantoni Aug 2, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I believe those are correct as is. When testing the changes made things seemed to work correctly. The redirect seems to function as expected, I see a 302 if I access the basePath without the ui part, with the browser redirecting correctly. All the static resources, version, metrics, and API are accessible as expected (no 404s seen when using the browser developer tools).

context.addServlet(classOf[DefaultServlet], "/")

val handlers = new HandlerCollection
Expand Down Expand Up @@ -114,7 +115,7 @@ class WebServer(livyConf: LivyConf, var host: String, var port: Int) extends Log
}
port = connector.getLocalPort

info("Starting server on %s://%s:%d" format (protocol, host, port))
info("Starting server on %s://%s:%d%s" format (protocol, host, port, contextPath))
}

def join(): Unit = {
Expand Down