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.

Sign up for GitHub

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

Jump to bottom

feat: support serving under a path prefix #120

Merged
merged 13 commits into from
Dec 12, 2023
Merged

feat: support serving under a path prefix #120

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
bfcb998
Make documentation links relative
waschik May 19, 2023
6906ef9
Add .gitignore for executable
waschik May 20, 2023
55b2b2a
Create redirects for prefix
waschik May 20, 2023
aef0101
Merge remote-tracking branch 'github.com/mccutchen/go-httpbin/main' i…
waschik Dec 9, 2023
be02898
Fix many of @mccutchen's comments
waschik Dec 10, 2023
566858e
Add .tmpl file suffix for template files
waschik Dec 10, 2023
5ef2dd3
Fix expected result of command line
waschik Dec 10, 2023
41818fc
Provide doRedirect method (suggest by mccutchen)
waschik Dec 10, 2023
e5a2611
Make relative links absolute again
waschik Dec 10, 2023
454e91d
Renamed specialCases to statusSpecialCases
waschik Dec 11, 2023
b593a82
Use doRedirect for all redirects
waschik Dec 11, 2023
7d6acb7
Add PREFIX environment variable
waschik Dec 11, 2023
3a796c7
Check for valid prefix values (including test)
waschik Dec 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions cmd/go-httpbin/.gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
/go-httpbin
mccutchen marked this conversation as resolved.
Show resolved Hide resolved
5 changes: 5 additions & 0 deletions httpbin/cmd/cmd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,9 @@ func mainImpl(args []string, getEnv func(string) string, getHostname func() (str
httpbin.WithObserver(httpbin.StdLogObserver(logger)),
httpbin.WithExcludeHeaders(cfg.ExcludeHeaders),
}
if cfg.Prefix != "" {
opts = append(opts, httpbin.WithPrefix(cfg.Prefix))
}
if cfg.RealHostname != "" {
opts = append(opts, httpbin.WithHostname(cfg.RealHostname))
}
Expand Down Expand Up @@ -106,6 +109,7 @@ type config struct {
ListenPort int
MaxBodySize int64
MaxDuration time.Duration
Prefix string
RealHostname string
TLSCertFile string
TLSKeyFile string
Expand Down Expand Up @@ -142,6 +146,7 @@ func loadConfig(args []string, getEnv func(string) string, getHostname func() (s
fs.IntVar(&cfg.ListenPort, "port", defaultListenPort, "Port to listen on")
fs.StringVar(&cfg.rawAllowedRedirectDomains, "allowed-redirect-domains", "", "Comma-separated list of domains the /redirect-to endpoint will allow")
fs.StringVar(&cfg.ListenHost, "host", defaultListenHost, "Host to listen on")
fs.StringVar(&cfg.Prefix, "prefix", "", "Path prefix (empty or start with slash and does not end with slash)")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every config option may be set by CLI arguments and by environment variables. I think we need to add support for setting the prefix via a PREFIX env var.

We also need to update this section of the README to note the new option:
https://github.com/mccutchen/go-httpbin/blob/main/README.md#configuration

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I implemented this. Now also with check for slash at beginning and end. But will be checked only on command line as the httpbin itself does not config checking.

fs.StringVar(&cfg.TLSCertFile, "https-cert-file", "", "HTTPS Server certificate file")
fs.StringVar(&cfg.TLSKeyFile, "https-key-file", "", "HTTPS Server private key file")
fs.StringVar(&cfg.ExcludeHeaders, "exclude-headers", "", "Drop platform-specific headers. Comma-separated list of headers key to drop, supporting wildcard matching.")
Expand Down
68 changes: 34 additions & 34 deletions httpbin/handlers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,12 @@ func (h *HTTPBin) Index(w http.ResponseWriter, r *http.Request) {
return
}
w.Header().Set("Content-Security-Policy", "default-src 'self'; style-src 'self' 'unsafe-inline'; img-src 'self' camo.githubusercontent.com")
writeHTML(w, mustStaticAsset("index.html"), http.StatusOK)
writeHTML(w, h.index_html, http.StatusOK)
}

// FormsPost renders an HTML form that submits a request to the /post endpoint
func (h *HTTPBin) FormsPost(w http.ResponseWriter, _ *http.Request) {
writeHTML(w, mustStaticAsset("forms-post.html"), http.StatusOK)
writeHTML(w, h.forms_post_html, http.StatusOK)
}

// UTF8 renders an HTML encoding stress test
Expand Down Expand Up @@ -161,13 +161,13 @@ type statusCase struct {
body []byte
}

var (
statusRedirectHeaders = &statusCase{
func createSpecialCases(prefix string) map[int]*statusCase {
statusRedirectHeaders := &statusCase{
headers: map[string]string{
"Location": "/redirect/1",
"Location": prefix + "/redirect/1",
},
}
statusNotAcceptableBody = []byte(`{
statusNotAcceptableBody := []byte(`{
"message": "Client did not request a supported media type",
"accept": [
"image/webp",
Expand All @@ -178,31 +178,31 @@ var (
]
}
`)
statusHTTP300body = []byte(`<!doctype html>
statusHTTP300body := []byte(fmt.Sprintf(`<!doctype html>
<head>
<title>Multiple Choices</title>
</head>
<body>
<ul>
<li><a href="/image/jpeg">/image/jpeg</a></li>
<li><a href="/image/png">/image/png</a></li>
<li><a href="/image/svg">/image/svg</a></li>
<li><a href="%[1]s/image/jpeg">/image/jpeg</a></li>
<li><a href="%[1]s/image/png">/image/png</a></li>
<li><a href="%[1]s/image/svg">/image/svg</a></li>
mccutchen marked this conversation as resolved.
Show resolved Hide resolved
</body>
</html>`)
</html>`, prefix))

statusHTTP308Body = []byte(`<!doctype html>
statusHTTP308Body := []byte(fmt.Sprintf(`<!doctype html>
<head>
<title>Permanent Redirect</title>
</head>
<body>Permanently redirected to <a href="/image/jpeg">/image/jpeg</a>
<body>Permanently redirected to <a href="%[1]s/image/jpeg">%[1]s/image/jpeg</a>
</body>
</html>`)
</html>`, prefix))

statusSpecialCases = map[int]*statusCase{
return map[int]*statusCase{
300: {
body: statusHTTP300body,
headers: map[string]string{
"Location": "/image/jpeg",
"Location": prefix + "/image/jpeg",
},
},
301: statusRedirectHeaders,
Expand All @@ -213,7 +213,7 @@ var (
308: {
body: statusHTTP308Body,
headers: map[string]string{
"Location": "/image/jpeg",
"Location": prefix + "/image/jpeg",
},
},
401: {
Expand Down Expand Up @@ -245,7 +245,7 @@ var (
},
},
}
)
}

// Status responds with the specified status code. TODO: support random choice
// from multiple, optionally weighted status codes.
Expand All @@ -265,7 +265,7 @@ func (h *HTTPBin) Status(w http.ResponseWriter, r *http.Request) {
// for special cases
w.Header().Set("Content-Type", textContentType)

if specialCase, ok := statusSpecialCases[code]; ok {
if specialCase, ok := h.specialCases[code]; ok {
for key, val := range specialCase.headers {
w.Header().Set(key, val)
}
Expand Down Expand Up @@ -326,16 +326,16 @@ func (h *HTTPBin) ResponseHeaders(w http.ResponseWriter, r *http.Request) {
mustMarshalJSON(w, args)
}

func redirectLocation(r *http.Request, relative bool, n int) string {
func (h *HTTPBin) redirectLocation(r *http.Request, relative bool, n int) string {
var location string
var path string

if n < 1 {
path = "/get"
path = h.prefix + "/get"
} else if relative {
path = fmt.Sprintf("/relative-redirect/%d", n)
path = fmt.Sprintf("%s/relative-redirect/%d", h.prefix, n)
} else {
path = fmt.Sprintf("/absolute-redirect/%d", n)
path = fmt.Sprintf("%s/absolute-redirect/%d", h.prefix, n)
}

if relative {
Expand All @@ -350,7 +350,7 @@ func redirectLocation(r *http.Request, relative bool, n int) string {
return location
}

func doRedirect(w http.ResponseWriter, r *http.Request, relative bool) {
func (h *HTTPBin) doRedirect(w http.ResponseWriter, r *http.Request, relative bool) {
parts := strings.Split(r.URL.Path, "/")
if len(parts) != 3 {
writeError(w, http.StatusNotFound, nil)
Expand All @@ -365,7 +365,7 @@ func doRedirect(w http.ResponseWriter, r *http.Request, relative bool) {
return
}

w.Header().Set("Location", redirectLocation(r, relative, n-1))
w.Header().Set("Location", h.redirectLocation(r, relative, n-1))
w.WriteHeader(http.StatusFound)
}

Expand All @@ -375,17 +375,17 @@ func doRedirect(w http.ResponseWriter, r *http.Request, relative bool) {
func (h *HTTPBin) Redirect(w http.ResponseWriter, r *http.Request) {
params := r.URL.Query()
relative := strings.ToLower(params.Get("absolute")) != "true"
doRedirect(w, r, relative)
h.doRedirect(w, r, relative)
}

// RelativeRedirect responds with an HTTP 302 redirect a given number of times
func (h *HTTPBin) RelativeRedirect(w http.ResponseWriter, r *http.Request) {
doRedirect(w, r, true)
h.doRedirect(w, r, true)
}

// AbsoluteRedirect responds with an HTTP 302 redirect a given number of times
func (h *HTTPBin) AbsoluteRedirect(w http.ResponseWriter, r *http.Request) {
doRedirect(w, r, false)
h.doRedirect(w, r, false)
}

// RedirectTo responds with a redirect to a specific URL with an optional
Expand Down Expand Up @@ -447,7 +447,7 @@ func (h *HTTPBin) SetCookies(w http.ResponseWriter, r *http.Request) {
HttpOnly: true,
})
}
w.Header().Set("Location", "/cookies")
w.Header().Set("Location", h.prefix+"/cookies")
w.WriteHeader(http.StatusFound)
}

Expand All @@ -464,7 +464,7 @@ func (h *HTTPBin) DeleteCookies(w http.ResponseWriter, r *http.Request) {
Expires: time.Now().Add(-1 * 24 * 365 * time.Hour),
})
}
w.Header().Set("Location", "/cookies")
w.Header().Set("Location", h.prefix+"/cookies")
w.WriteHeader(http.StatusFound)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given how often this pattern now shows up across handlers, I propose that we wrap the logic up in a helper method:

func (h *HTTPBin) doRedirect(path string, code int) {
    w.Header().Set("Location", h.prefix+"/cookies")
    w.WriteHeader(code)
}

(That name would clash with the existing doRedirect() helper, which is a bit higher level and serves only the Redirect/AbsoluteRedirect/RelativeRedirect handler. I'd suggest we rename the existing doRedirect to something like handleRedirect instead.)

Copy link
Contributor Author

@waschik waschik Dec 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean a doRedirectCookies method or cookies a parameter to some method?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean there are now a bunch of places where we've got code like this, where we're constructing a redirect URL and must remember to include the prefix, so I'd like to have a helper method like the one I showed above.

This code would then become something like

h.doRedirect("/cookies", http.StatusFound)
return

and the new doRedirect() helper would take care of handling the prefix.

Where it gets messy is that there's already an existing, higher level helper method doRedirect() helper that will need to be renamed. I think renaming the existing helper to handleRedirect() makes sense, as that better captures what the existing helper is actually doing.

Copy link
Contributor Author

@waschik waschik Dec 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I renamed the doRedirect to handleRedirect and used new doRedirect at two places. Is there a reason why https://github.com/waschik/go-httpbin/blob/7e81a9365c6fe9f1a4691742792dcd2e3d8a1fa6/httpbin/handlers.go#L883 is using r.URL.String() and not r.URL.Path? If r.URL.Path would be okay it would be possible to replace it also with the new doRedirect. For r.URL.String() I am not sure if it will not sometimes include the schema and then prefix might be wrong.

For the absolute redirects new doRedirect would also not possible otherwise I get locations like /a-prefixhttp://...

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why https://github.com/waschik/go-httpbin/blob/7e81a9365c6fe9f1a4691742792dcd2e3d8a1fa6/httpbin/handlers.go#L883 is using r.URL.String() and not r.URL.Path?

I think this is to ensure that any query params are carried through the redirects, but it looks like we don't have any explicit test cases covering that behavior. Let's keep it as-is for now.

For the absolute redirects new doRedirect would also not possible otherwise I get locations like /a-prefixhttp://...

Ah that's a good point. Maybe he new doRedirect() should change its behavior based on whether the given argument starts with http:// or https://?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it works if prefix is only added if arguments start with /. I'll experiment a bit with this. Otherwise option would be to parse URL or add another parameter.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I think pivoting on whether the first character is / would be fine here!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I implemented this with slash checking. Wrong adding of prefix of external prefix was shown in test cases. Hope it is correct now.

}

Expand Down Expand Up @@ -916,18 +916,18 @@ func (h *HTTPBin) Links(w http.ResponseWriter, r *http.Request) {
writeError(w, http.StatusBadRequest, fmt.Errorf("invalid offset: %w", err))
return
}
doLinksPage(w, r, n, offset)
h.doLinksPage(w, r, n, offset)
return
}

// Otherwise, redirect from /links/<n> to /links/<n>/0
r.URL.Path = r.URL.Path + "/0"
r.URL.Path = h.prefix + r.URL.Path + "/0"
w.Header().Set("Location", r.URL.String())
w.WriteHeader(http.StatusFound)
}

// doLinksPage renders a page with a series of N links
func doLinksPage(w http.ResponseWriter, _ *http.Request, n int, offset int) {
func (h *HTTPBin) doLinksPage(w http.ResponseWriter, _ *http.Request, n int, offset int) {
w.Header().Add("Content-Type", htmlContentType)
w.WriteHeader(http.StatusOK)

Expand All @@ -936,7 +936,7 @@ func doLinksPage(w http.ResponseWriter, _ *http.Request, n int, offset int) {
if i == offset {
fmt.Fprintf(w, "%d ", i)
} else {
fmt.Fprintf(w, `<a href="/links/%d/%d">%d</a> `, n, i, i)
fmt.Fprintf(w, `<a href="%s/links/%d/%d">%d</a> `, h.prefix, n, i, i)
}
}
w.Write([]byte("</body></html>"))
Expand Down
Loading
Loading