don't use absolute path in redirect for visualizer #2785

timkpaine · 2019-09-14T02:53:48Z

In reverse proxy setups, you sometimes want to setup sites behind a sub path e.g. my.site.com/luigi/
This PR makes sure the initial redirect is relative to the base_url, not absolute.

honnix · 2019-09-20T12:38:53Z

Thanks for the PR. Have you tested this will work for both behind reverse proxy and direct access?

Tarrasch

If this isn't testable, then maybe you can add a code comment that explains why there's no "/"?

timkpaine · 2019-09-25T23:34:29Z

will get back to this this week, got distracted

timkpaine · 2019-10-01T18:48:29Z

@honnix works fine for both, @Tarrasch it might be possible to modify this test to test explicitly

luigi/test/server_test.py

Line 101 in 865cc4e

def test_visualiser(self):

honnix · 2019-10-01T18:56:47Z

Something like this would work https://github.com/tornadoweb/tornado/blob/master/tornado/test/httpclient_test.py#L293

stale · 2020-01-30T02:31:42Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If closed, you may revisit when your time allows and reopen! Thank you for your contributions.

timkpaine · 2020-06-04T19:50:47Z

@honnix @Tarrasch
finally got around to this and added a test that specifically checks there is no leading slash:

response = self.fetch("/", follow_redirects=False)
self.assertEqual(response.code, 302)
self.assertEqual(response.headers['Location'], 'static/visualiser/index.html')  # assert that doesnt beging with leading slash !

honnix

LGTM. @Tarrasch Would like you to take another look at this as well?

dlstadther

lgtm

timkpaine · 2020-09-24T22:55:48Z

thanks!

tashrifbillah · 2020-11-13T20:13:57Z

This work missed my eyes. I have reported the need in many places before including issue #2981. A similar issue is #2670. Thanks for the modification. I shall test it and let you know.

tashrifbillah · 2020-11-14T00:07:46Z

Unfortunately, the PR didn't work for me. See below a comparison between plotly server and luigi server:

Launching luigid

luigid --port 8083

Confirming luigi is functional w/o proxy

curl -L http://localhost:8083

Configuring nginx proxy

    server {

        root /var/www/html/;


        location /luigi/ {
            proxy_pass http://localhost:8083;
        }

        location /dash/ {
            proxy_pass http://localhost:8084;
        }


    }

Issue

# works gracefully
curl -L http://myIP/dash/

# fails :(
curl -L http://myIP/luigi/

Failure logs:

console log

<html><title>404: Not Found</title><body>404: Not Found</body></html>

nginx/access.log

170.223.221.217 - [13/Nov/2020:18:54:43 -0500] "GET /luigi/ HTTP/1.1" 404 69 "-" "curl/7.29.0" "-"

nginx/error.log

2020/11/13 19:05:09 [notice] 106027#0: signal process started

Tornado log

2020-11-13 19:25:05,597 luigi.scheduler[145038] INFO: Attempting to load state from /home/tb571/luigi-state.pickle
2020-11-13 19:25:05,602 luigi.server[145038] INFO: Scheduler starting up
2020-11-13 19:25:39,761 tornado.access[145038] WARNING: 404 GET /luigi/ (::1) 1.48ms

@timkpaine @honnix

timkpaine · 2020-11-14T00:19:06Z

The logs you posted are nginx telling you that the route is missing. Nothing to do with Luigi, are there Luigi logs indicating the process was hit with the request?

timkpaine · 2020-11-14T00:24:51Z

also you installed from master right?

timkpaine · 2020-11-14T00:30:18Z

don't you need a trailing slash on proxy_pass http://localhost:8083;? otherwise you're redirecting to localhost:8083/luigi/, and whereas this is a valid dash route, its not a valid luigi route

tashrifbillah · 2020-11-14T00:30:20Z

also you installed from master right?

Upon release 3.0.2, I applied your change, which is just one / removal.

Can you see my edited comment above for Tornado log?

timkpaine · 2020-11-14T00:31:29Z

don't you need a trailing slash on proxy_pass http://localhost:8083;? otherwise you're redirecting to localhost:8083/luigi/, and whereas this is a valid dash route, its not a valid luigi route

https://serverfault.com/questions/562756/how-to-remove-the-path-with-an-nginx-proxy-pass

tashrifbillah · 2020-11-14T00:36:19Z

Thanks Tim, it looks like the trailing slash is required. So is it the trailing slash that makes sure the redirection is towards http://localhost:8083?

That said, I am going to do a few more tests tonight with other tabs such as /history.

timkpaine · 2020-11-14T00:42:59Z

yeah all of this has nothing to do with luigi, its all nginx configuration. The trailing slash ensures that myip/luigi/a/path -> localhost:8083/a/path. You just happened to get unlucky with the url as dash directs its traffic through /dash anyway.

tashrifbillah · 2020-11-14T05:29:32Z

Hello again @timkpaine , unfortunately, we are not set yet. Have you tried to run a job through your modification? See the issue I figured:

nginx.conf

    server {
        root /var/www/html/;
        location /luigi/ {
            proxy_pass http://localhost:8083/;
        }
    }

luigi.cfg:

[core]
default-scheduler-url = http://myIP/luigi/

[scheduler]
record_task_history = True
state_path = ${HOME}/luigi-state.pickle

[task_history]
db_connection = sqlite:///${HOME}/luigi-task-hist.db

Error

Then if you try my simple task, it should run into the following error:

WARNING: Failed connecting to remote scheduler 'http://myIP/luigi'
Traceback (most recent call last):
  File "/tmp/new_luigi/miniconda3/lib/python3.7/site-packages/luigi/rpc.py", line 163, in _fetch
    response = self._fetcher.fetch(full_url, body, self._connect_timeout)
  File "/tmp/new_luigi/miniconda3/lib/python3.7/site-packages/luigi/rpc.py", line 117, in fetch
    resp.raise_for_status()
  File "/tmp/new_luigi/miniconda3/lib/python3.7/site-packages/requests/models.py", line 940, in raise_for_status
    raise HTTPError(http_error_msg, response=self)
requests.exceptions.HTTPError: 404 Client Error: Not Found for url: http://myIP/api/add_task

As apparent from http://myIP/api/add_task, I think my nginx URL rewriting isn't right. http://myIP/api/add_task isn't even hitting Luigi. It should be http://localhost:8083/api/add_task.

Any suggestion about the URL rewriting for @proxy_pass in the above nginx.conf?

timkpaine · 2020-11-14T10:32:54Z

I don't use nginx for this but I think you'll have a lot easier time if you set it up as a sub domain rather than a path, e.g. luigi.myip rather than myip/luigi. When you reverse proxy under paths, often times you need a lot more library modifications.

tashrifbillah · 2020-11-14T11:33:24Z

Did you test the job scheduling part other than visualizer?

timkpaine · 2020-11-14T11:35:59Z

I run jobs from the same machine so my scheduler url is local host. But it's reasonable to assume that the client would still use absolute paths. This would still work fine with a sub domain instead of a path

tashrifbillah · 2020-11-14T11:41:50Z

I shall try what you suggested. More info for you--the /history tab in the seemingly functional visualizer is missing style sheets. Did you notice that? Because of reverse proxy, it can't find them.

tashrifbillah · 2020-11-15T16:23:04Z

Hi @timkpaine , could you at least check if style sheets load for you in the /history tab?

tashrifbillah · 2020-12-10T23:33:33Z

I solved the style sheet loading issue by another location block:

    server {
        root /var/www/html/;
        location /luigi/ {
            proxy_pass http://localhost:8083/;
        }

        location /static/ {
            proxy_pass http://localhost:8083/static/;
        }

    }

It is a hack that does not make me so happy but it works.

Edit:

I needed another block to run a job:

         location /api/ {
             proxy_pass http://127.0.0.1:8082/api/;
         }

The fix is just not complete.

don't use absolute path in redirect for visualizer

cc396df

timkpaine requested review from dlstadther, honnix and Tarrasch as code owners September 14, 2019 02:53

Tarrasch reviewed Sep 23, 2019

View reviewed changes

add comment

f7e8963

timkpaine mentioned this pull request Nov 19, 2019

Reverse proxying webserver #2780

Closed

stale bot added wontfix and removed wontfix labels Jan 30, 2020

timkpaine added 2 commits June 4, 2020 15:33

Merge branch 'master' into reverse-proxy

44aed8d

add test

ba5ff73

honnix approved these changes Jun 6, 2020

View reviewed changes

dlstadther approved these changes Jun 6, 2020

View reviewed changes

honnix merged commit d4eb8a6 into spotify:master Sep 24, 2020

honnix mentioned this pull request Sep 24, 2020

Relativize unnecessary absolute URL #3002

Closed

timkpaine deleted the reverse-proxy branch September 24, 2020 22:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

don't use absolute path in redirect for visualizer #2785

don't use absolute path in redirect for visualizer #2785

timkpaine commented Sep 14, 2019

honnix commented Sep 20, 2019

Tarrasch left a comment

timkpaine commented Sep 25, 2019

timkpaine commented Oct 1, 2019

honnix commented Oct 1, 2019

stale bot commented Jan 30, 2020

timkpaine commented Jun 4, 2020 •

edited

Loading

honnix left a comment

dlstadther left a comment

timkpaine commented Sep 24, 2020

tashrifbillah commented Nov 13, 2020

tashrifbillah commented Nov 14, 2020 •

edited

Loading

timkpaine commented Nov 14, 2020 •

edited

Loading

timkpaine commented Nov 14, 2020

timkpaine commented Nov 14, 2020 •

edited

Loading

tashrifbillah commented Nov 14, 2020

timkpaine commented Nov 14, 2020 •

edited

Loading

tashrifbillah commented Nov 14, 2020

timkpaine commented Nov 14, 2020

tashrifbillah commented Nov 14, 2020 •

edited

Loading

timkpaine commented Nov 14, 2020

tashrifbillah commented Nov 14, 2020

timkpaine commented Nov 14, 2020

tashrifbillah commented Nov 14, 2020 •

edited

Loading

tashrifbillah commented Nov 15, 2020

tashrifbillah commented Dec 10, 2020 •

edited

Loading

don't use absolute path in redirect for visualizer #2785

don't use absolute path in redirect for visualizer #2785

Conversation

timkpaine commented Sep 14, 2019

honnix commented Sep 20, 2019

Tarrasch left a comment

Choose a reason for hiding this comment

timkpaine commented Sep 25, 2019

timkpaine commented Oct 1, 2019

honnix commented Oct 1, 2019

stale bot commented Jan 30, 2020

timkpaine commented Jun 4, 2020 • edited Loading

honnix left a comment

Choose a reason for hiding this comment

dlstadther left a comment

Choose a reason for hiding this comment

timkpaine commented Sep 24, 2020

tashrifbillah commented Nov 13, 2020

tashrifbillah commented Nov 14, 2020 • edited Loading

Launching luigid

Confirming luigi is functional w/o proxy

Configuring nginx proxy

Issue

timkpaine commented Nov 14, 2020 • edited Loading

timkpaine commented Nov 14, 2020

timkpaine commented Nov 14, 2020 • edited Loading

tashrifbillah commented Nov 14, 2020

timkpaine commented Nov 14, 2020 • edited Loading

tashrifbillah commented Nov 14, 2020

timkpaine commented Nov 14, 2020

tashrifbillah commented Nov 14, 2020 • edited Loading

nginx.conf

luigi.cfg:

Error

timkpaine commented Nov 14, 2020

tashrifbillah commented Nov 14, 2020

timkpaine commented Nov 14, 2020

tashrifbillah commented Nov 14, 2020 • edited Loading

tashrifbillah commented Nov 15, 2020

tashrifbillah commented Dec 10, 2020 • edited Loading

timkpaine commented Jun 4, 2020 •

edited

Loading

tashrifbillah commented Nov 14, 2020 •

edited

Loading

timkpaine commented Nov 14, 2020 •

edited

Loading

timkpaine commented Nov 14, 2020 •

edited

Loading

timkpaine commented Nov 14, 2020 •

edited

Loading

tashrifbillah commented Nov 14, 2020 •

edited

Loading

tashrifbillah commented Nov 14, 2020 •

edited

Loading

tashrifbillah commented Dec 10, 2020 •

edited

Loading