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

Patch URL helper to assume use_appname=False if the host is a FQDN #918

Closed
wants to merge 1 commit into from

Conversation

kszys
Copy link
Contributor

@kszys kszys commented Sep 4, 2024

So - following the discussion in #911 - here is an idea for fixing the URL helper to automagically choose the value of the use_appname parameter.

Basically, I added one condition to check if there is a valid host being passed to the app. If the host (which I get from request.urlparts[1]) is a valid FQDN (i.e., a Fully Qualified Domain Name), the assumption is that appname should not be used in constructing the URL, and hence use_appname is set to False.

There is of course the related import and updated requirements.txt.

@mdipierro
Copy link
Contributor

It seems to me we already have a mechanism to not inject appname if HTTP_X_PY4WEB_APPNAME is present. So all we need to do in nginx is

server {
    listen         80;
    server_name    app1.example.com;
    add_header X-PY4WEB-APPNAME app1
    location / {
       proxy_pass http://localhost:8001/app1/;
  }
}

server {
    listen         80;
    server_name    app1.example.com;
    add_header X-PY4WEB-APPNAME app2
    location / {
       proxy_pass http://localhost:8001/app2/;
  }
}

(details depend on the nginx version)
The presence of the header X-PY4WEB-APPNAME will make it so URL will not add the URL prefix.

@kszys
Copy link
Contributor Author

kszys commented Sep 5, 2024

Hmm... there is a problem with this solution. I just tested, to be sure, but:

add_header directive in nginx only adds a response header - not a request header. Hence, even with this setup, py4web URL helper still adds the appname to the constructed url. Of course (and I checked) the X-PY4WEB-APPNAME = app1 is in the response headers that the browser eventually receives, but this does not help in any way for the URL helper to make a good decision about adding the appname or not 🤷

However... if one would be to use proxy_set_header directive instead, this actually works: 🎉

server {
    listen 80;
    server_name app1.example.com;
    proxy_http_version 1.1;
    proxy_set_header X-PY4WEB-APPNAME app1;
    location / {
        proxy_pass http://127.0.0.1:8000/app1$request_uri;
    }
}

@kszys
Copy link
Contributor Author

kszys commented Sep 5, 2024

Investigating a bit further, I seem to be running into some issues, though... Will report as soon as I get to the bottom of.

It appears as if adding this proxy_set_header X-PY4WEB-APPNAME app1; messes up something in Ombott, and request.fullpath, request.url, and request.urlparts start showing some imaginary things...

@kszys
Copy link
Contributor Author

kszys commented Sep 5, 2024

Right... so the problem is indeed in Ombott implementation - concretly in request_pkg/props_mixin.py. This is the function that calculates the request.fullpath:

    def fullpath(self):
        """ Request path including :attr:`script_name` (if present). """
        appname = self._env_get(self.config.app_name_header, '/')
        return urljoin(self.script_name, self.path[len(appname):].lstrip('/'))

For our example data:

path = '/app1/config/test'
appname = 'app1'

The problem is that: path[len(appname):].lstrip('/') evaluates to 1/config/test. 😔

@mdipierro
Copy link
Contributor

mdipierro commented Sep 5, 2024 via email

@kszys
Copy link
Contributor Author

kszys commented Sep 5, 2024

Yes - already have - valq7711/ombott#18

@mdipierro
Copy link
Contributor

We may need to change some of the URL generations in Auth to actually use URL if not using it already.

@kszys
Copy link
Contributor Author

kszys commented Sep 7, 2024

So... After some more testing and discussing in valq7711/ombott#18 - it seems that actually everything is already fine 😂

  • Ombott is fine
  • URL helper is fine
  • Form, Grid, and Auth is actually fine too.

I actually managed to get the logical/expected behaviour with current master of Ombott and py4web. The thing is that Form, Grid, and Auth only use the elements from Ombott's request which are already correct if the correct request header is passed to it. Essentially the only issue is to ensure that the correct nginx (or other proxy) configuration is present. Something like (for nginx) - notice the leading slash in X-PY4WEB-APPNAME value:

server {
    listen 80;
    server_name myapp.example.com;
    proxy_http_version 1.1;
    proxy_set_header Host $host;
    proxy_set_header X-PY4WEB-APPNAME /myapp;
    location / {
        proxy_pass http://127.0.0.1:8000/myapp$request_uri;
    }
}

Sorry for bothering everybody ;)

I now believe that the only changes required are to the py4web documentation to spell it out. I can propose the text Massimo, if you agree, but let me know where this should be placed. I think something like Deployment Recepies chapter would be a good idea.

@kszys
Copy link
Contributor Author

kszys commented Sep 15, 2024

I created a PR #921 for the documentation. I think this is sufficient to close this PR.

@kszys kszys closed this Sep 15, 2024
@kszys kszys deleted the URL_fix branch September 21, 2024 20:05
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.

2 participants