-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
fix: to support --rootpath #3503
Conversation
1. when using https, redirect to the right URL. 2. when rootpath is set, handle healthz, swagger, etc.
Codecov Report
@@ Coverage Diff @@
## master #3503 +/- ##
==========================================
+ Coverage 43.04% 43.07% +0.02%
==========================================
Files 183 183
Lines 20266 20206 -60
Branches 237 273 +36
==========================================
- Hits 8724 8703 -21
+ Misses 10538 10497 -41
- Partials 1004 1006 +2
Continue to review full report at Codecov.
|
server/server.go
Outdated
return &http.Server{ | ||
Addr: fmt.Sprintf("localhost:%d", port), | ||
Addr: addr, | ||
Handler: http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { | ||
target := "https://" + req.Host + req.URL.Path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The server still redirects to root path. The changes have to be implemented in handler implementation. E.g.
Handler: http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
target := "https://" + req.Host
if rootPath != "" {
target += strings.TrimRight(strings.TrimLeft(rootPath, "/"), "/")
}
target += req.URL.Path
if len(req.URL.RawQuery) > 0 {
target += "?" + req.URL.RawQuery
}
http.Redirect(w, req, target, http.StatusTemporaryRedirect)
}),
1. when using https, redirect to the right URL. 2. when rootpath is set, handle healthz only.
1. when using https, redirect to the right URL. 2. when rootpath is set, handle healthz only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried to verify https->http redirect behavior and noticed a bug.
http://localhost:8080/argocd1/applications
was redirected to https://localhost:8080argocd1/applications
. Can you take a look please?
server/server.go
Outdated
@@ -677,7 +677,7 @@ func newRedirectServer(port int, rootPath string) *http.Server { | |||
return &http.Server{ | |||
Addr: addr, | |||
Handler: http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { | |||
target := "https://" + req.Host | |||
target := "https://" + req.Host + "/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The req.URL.Path starts with /
, so we might get double slash at https://github.com/argoproj/argo-cd/pull/3503/files/fd61815dfb544c9227f3814b9d44783a277b501e..b4cbc25337e748139cea282975dd6ed3fbe486d0#diff-91bbeda7eb98a7adc57b9e47e2cf5c2bR684.
target := "https://" + req.Host + "/" | |
target := "https://" + req.Host |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Checklist: