LIVY-615: Set context path using property SERVER_BASE_PATH.#188
LIVY-615: Set context path using property SERVER_BASE_PATH.#188fdeantoni wants to merge 6 commits into
Conversation
The default value of livy.ui.basePath has been changed from empty string to "/" instead. As the property was not working before this should be a safe change. Nonetheless, if empty string is used for this property things will still work as expected. Any value that does not start with a slash will fail however. Changes have also been made to make things pass the scala style check.
Codecov Report
@@ Coverage Diff @@
## master #188 +/- ##
============================================
- Coverage 68.50% 68.43% -0.07%
Complexity 841 841
============================================
Files 103 103
Lines 5940 5944 +4
Branches 898 899 +1
============================================
- Hits 4069 4068 -1
- Misses 1312 1315 +3
- Partials 559 561 +2
Continue to review full report at Codecov.
|
| val context = new ServletContextHandler() | ||
|
|
||
| context.setContextPath("/") | ||
| private val contextPath = "/" + livyConf.get(LivyConf.SERVER_BASE_PATH).stripPrefix("/") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The API part is actually another problem. Here since your original problem is related to UI, let's focus on solving UI issue first.
There was a problem hiding this comment.
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:
- Keep the current broken implementation
- Rename
livy.ui.basePathtolivy.server.basePathand 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. - Implement
livy.server.basePathalongsidelivy.ui.basePathwherelivy.server.basePathwill set the basePath for both API and UI, andlivy.ui.basePaththe 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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
|
@ajbozarth would you please help to review. Thanks! |
ajbozarth
left a comment
ajbozarth
left a comment
There was a problem hiding this comment.
A few code related comments as a follow up to my two inline comments in the thread
| 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("/") |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
You are right that the default could actually stay an empty string, but after some pondering I changed the default for two reasons:
- 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 uselivy.ui.basePath = /because - 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Alright, I will remove it from the template and set the default to empty string.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
That's a good point lets leave it in the config template # livy.server.basePath = then
| 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.ui.basePath", "/") |
There was a problem hiding this comment.
Based on the change in use case and original intent I believe we should change this to livy.server.basePath (and since it was released already add a back-compatibility handler)
There was a problem hiding this comment.
I agree, it will be much clearer to the user what the intent is. The old property can simply reference the new livy.server.basePath property to make it backwards compatible.
There was a problem hiding this comment.
It's been a while since I wrote it, but IIRC there's a section of code to put old config names in and what new config to point to, we made it for deprecating configs.
There was a problem hiding this comment.
I think I found it: LivyConf.configsWithAlternatives. I will add livy.ui.basePath there, pointing it to the key livy.server.basepath instead.
There was a problem hiding this comment.
That sounds like the correct code
|
|
||
| context.setContextPath("/") | ||
| private val contextPath = "/" + livyConf.get(LivyConf.SERVER_BASE_PATH).stripPrefix("/") | ||
| require(contextPath.startsWith("/"), |
There was a problem hiding this comment.
this require will always pass since / is always added to contextPath above, you should remove it.
There was a problem hiding this comment.
You are correct, it should be removed.
| private val contextPath = "/" + livyConf.get(LivyConf.SERVER_BASE_PATH).stripPrefix("/") | ||
| require(contextPath.startsWith("/"), | ||
| s"Configuration property ${LivyConf.SERVER_BASE_PATH.key} must start with /, e.g. /my-livy") | ||
| context.setContextPath(contextPath) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
|
Also @m-wcislo would you be willing to read through our discussion here and provide some insight? You originally added the feature being discussed. |
start with a slash.
with backwards compatibility. Also adjusted default value back to empty string.
|
Hi all, any chance this will be merged into master? We are currently using the forked branch but would prefer to use master... |
ajbozarth
left a comment
ajbozarth
left a comment
There was a problem hiding this comment.
This LGTM, @jerryshao you ok with this as is, if so lets merge it
|
Hi @ajbozarth , @jerryshao , any chance this can be included in 0.8? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #188 +/- ##
============================================
- Coverage 68.50% 68.43% -0.07%
Complexity 841 841
============================================
Files 103 103
Lines 5940 5944 +4
Branches 898 899 +1
============================================
- Hits 4069 4068 -1
- Misses 1312 1315 +3
- Partials 559 561 +2 ☔ View full report in Codecov by Sentry. |
|
This pull request has been automatically marked as stale because it has had no activity for at least 3 months. If you are still working on this change or plan to move it forward, please leave a comment or push a new commit so we know to keep it open. Otherwise, this PR will be closed automatically in about one month. Thank you for your contribution to Apache Livy! |
5 participants
What changes were proposed in this pull request?
This pull request is to address https://issues.apache.org/jira/browse/LIVY-615
How was this patch tested?
This patch was tested using manual tests: