feat: Support configurable base path for server #714
Conversation
| if basePath != "" && r.URL.Path == "/" { | ||
| http.Redirect(w, r, basePath, http.StatusSeeOther) | ||
| return | ||
| } |
There was a problem hiding this comment.
If the root of the server is hit and we have a base path configured redirect to that base path for convenience.
There was a problem hiding this comment.
I've tested this locally with an nginx reverse proxy (configuration below) but encountered an issue when accessing via the proxy path http://localhost/dagu/. It seems the path prefix check might be the cause. The following code, however, works as expected:
if strings.HasPrefix(r.URL.Path, "/api") {
next.ServeHTTP(w, r)
} else {
defaultHandler.ServeHTTP(w, r)
}Would you mind taking a look when you have a chance?
Test setup
server {
listen 80;
server_name localhost;
location /dagu/ {
proxy_pass http://localhost:8080/;
proxy_set_header Host $host;
proxy_set_header X-Real-IP $remote_addr;
proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
proxy_set_header X-Forwarded-Proto $scheme;
}
}~/.dagu/admin.cfg
basePath: /dagu/
port: 8080There was a problem hiding this comment.
The issue is the trailing slash. I was only testing with /dagu and missed this case.
I have made the BasePath more robust
- Normalize the value in config.go so that all parts of the app can be sure it has the same shape
- changed
prefixCheckerto strip the prefix first, then do the strings.HasPrefix check in a HandlerFunc to not have to worry about the prefix.
I tested with
- no basepath set at all
- basepath set as an empty string
- basePath set as
/ - basePath set as
/dagu - basePath set as
/dagu/ - basePath set as
/dagu/foo
I believe this should be good.
I have pushed up the fix as a !fixup commit to make it easier to review. This can be "squashed" when merging or I can squash it before this is ready to merge.
| return | ||
| } | ||
|
|
||
| if strings.HasPrefix(r.URL.Path, path.Join(basePath, "/api")) { |
There was a problem hiding this comment.
Is there a better solution to path.Join here? It doesn't feel robust.
It really feels like /api should be a handler which is hung off of http.StripPrefix(basePath, next) such that any route registered under it would be handled by /api
I wasn't sure of the consequences of such a change however, so I just updated the string comparison.
There was a problem hiding this comment.
See #714 (comment) this has been improved to not rely on path.join for this prefix check
| http.StripPrefix(basePath, next).ServeHTTP(w, r) | ||
| } else { | ||
| defaultHandler.ServeHTTP(w, r) | ||
| http.StripPrefix(basePath, defaultHandler).ServeHTTP(w, r) |
There was a problem hiding this comment.
http.StripPrefix seemed like the cleanest way to allow all existing routes to work as before.
This is my first time writing Go so if there's a more idiomatic way of doing this let me know!
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #714 +/- ##
==========================================
- Coverage 61.80% 61.77% -0.03%
==========================================
Files 53 53
Lines 5249 5251 +2
==========================================
Hits 3244 3244
- Misses 1751 1753 +2
Partials 254 254
Continue to review full report in Codecov by Sentry.
|
| if basePath != "" && r.URL.Path == "/" { | ||
| http.Redirect(w, r, basePath, http.StatusSeeOther) | ||
| return | ||
| } |
There was a problem hiding this comment.
I've tested this locally with an nginx reverse proxy (configuration below) but encountered an issue when accessing via the proxy path http://localhost/dagu/. It seems the path prefix check might be the cause. The following code, however, works as expected:
if strings.HasPrefix(r.URL.Path, "/api") {
next.ServeHTTP(w, r)
} else {
defaultHandler.ServeHTTP(w, r)
}Would you mind taking a look when you have a chance?
Test setup
server {
listen 80;
server_name localhost;
location /dagu/ {
proxy_pass http://localhost:8080/;
proxy_set_header Host $host;
proxy_set_header X-Real-IP $remote_addr;
proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
proxy_set_header X-Forwarded-Proto $scheme;
}
}~/.dagu/admin.cfg
basePath: /dagu/
port: 8080
|
@yohamta This should be good for another review, thank you! |
Closes: #576
Closes: #575
This PR Adds a BasePath config that allows the go server to respond to requests at a specific sub path.
To test supply the BaseBath config though the environment variable
DAGU_BASE_PATH=/fooor through the admin.yaml file//foo