feat: support serving under a path prefix #120

waschik · 2023-05-19T20:22:07Z

Make relative links in main help page. Some links (like absolute-redirekt and links) will not work with prefix. But they still work without.

Could be tested with this program:

package main

import (
	"github.com/mccutchen/go-httpbin/v2/httpbin"
	"net/http"
)

const (
	prefix = "/prefix"
)

func main() {
	var handler http.Handler = httpbin.New()
	handler = http.StripPrefix(prefix, handler)

	mux := http.NewServeMux()
	mux.Handle(prefix+"/", handler)
	http.ListenAndServe(":8090", mux)
}

While running this result can be checked at http://localhost:8090/prefix/

mccutchen · 2023-05-19T21:12:13Z

Ah, interesting use case, this isn't something I've considered before. Unfortunately, fixing the links on the index page is an incomplete solution; as you mentioned, any endpoint that generates links to other endpoints (like /redirect, /absolute-redirect, etc) will break:

$ curl -v http://localhost:8090/prefix/redirect/5 2>&1 | grep -e '> GET' -e '< HTTP' -e '< Location'
> GET /prefix/redirect/5 HTTP/1.1
< HTTP/1.1 302 Found
< Location: /relative-redirect/4

So, if we want go-httpbin to properly function when being served under a path prefix, we need a more holistic solution. Probably adding an optional --prefix configuration that would be used to compute URLs. (Unfortunately, this would also require us to turn the static index.html into a template of some sort, but we can probably keep that pretty simple and render it only once at startup and continue serving it quickly from memory.)

Are you interested in taking on that challenge?

waschik · 2023-05-20T16:17:10Z

Okay. I tried to add prefix to code and make the index html and form post a template. Executable size increased on my system by about 20% (7.5 MB to 8.9 MB). Because software then needs to know the prefix I made an option like you suggested and also called the StripPrefix inside handler creation.

Then after

./go-httpbin -port 8090 -prefix /prefix &

I could run the following:

$ curl -v http://localhost:8090/prefix/redirect/5 2>&1 | grep -e '> GET' -e '< HTTP' -e '< Location'
> GET /prefix/redirect/5 HTTP/1.1
< HTTP/1.1 302 Found
< Location: /prefix/relative-redirect/4

I did not change all unit tests. This would make tests more complicated (mostly for-loops with and without prefix). I do not know what you prefer. Another option (maybe not the nicest) would be that I write a complete wrapper and do some search and replacement in result body and headers (e.g. Location header).

mloskot · 2023-12-08T22:00:44Z

@mccutchen

Ah, interesting use case, this isn't something I've considered before.

I sometimes deploy go-httpbin behind Kong proxy, for testing, and where I apply path-based routing, so the service is available at e.g. https://example.com/tests/httpbin and it would be nice if the endpoints on the frontpage could be generated to

/tests/httpbin/anything
/tests/httpbin/base64
...

mccutchen · 2023-12-08T22:07:35Z

Yeah, my apologies to @waschik and anyone else interested in this change. I dropped the ball on getting this reviewed and merged, and then I refactored a few things along with the whole dang test suite, leading to a bunch of churn that will need to be addressed.

I'll try to make time this weekend circle back to it!

…nto relative-doc-links

waschik · 2023-12-09T16:36:09Z

@mccutchen I tried to fixed the merge failures. Hope you did not do them parallel and I home I do not have to do it again.

Do you know https://github.com/stretchr/testify? Looks a bit similar to your assert. With that I would not have to add a assert.Contains method which I needed because of two tests of body.

mccutchen

Thanks a lot for picking this back up! It's looking good overall, but I've left a bit of feedback below. Please let me know if it makes sense to you!

One more high level comment: Would you mind adding a .tmpl extension to the end of the two files that have change from static html to golang templates? I.e. index.html -> index.html.tmpl.

httpbin/httpbin.go

mccutchen · 2023-12-10T15:39:42Z

httpbin/httpbin.go

+	index_html      []byte
+	forms_post_html []byte
+
+	specialCases map[int]*statusCase


Naming nit: Since we're moving this onto the HTTPBin struct, let's make it a bit more specific. I think redirectSpecialCases would be a useful clarification for this name.

Maybe again statusSpeciaCases like in current state of your main branch?

go-httpbin/httpbin/handlers.go

Lines 201 to 280 in b63a792

statusSpecialCases = map[int]*statusCase{

300: {

body: statusHTTP300body,

headers: map[string]string{

"Location": "/image/jpeg",

},

},

301: statusRedirectHeaders,

302: statusRedirectHeaders,

303: statusRedirectHeaders,

305: statusRedirectHeaders,

307: statusRedirectHeaders,

308: {

body: statusHTTP308Body,

headers: map[string]string{

"Location": "/image/jpeg",

},

},

401: {

headers: map[string]string{

"WWW-Authenticate": `Basic realm="Fake Realm"`,

},

},

402: {

body: []byte("Fuck you, pay me!"),

headers: map[string]string{

"X-More-Info": "http://vimeo.com/22053820",

},

},

406: {

body: statusNotAcceptableBody,

headers: map[string]string{

"Content-Type": jsonContentType,

},

},

407: {

headers: map[string]string{

"Proxy-Authenticate": `Basic realm="Fake Realm"`,

},

},

418: {

body: []byte("I'm a teapot!"),

headers: map[string]string{

"X-More-Info": "http://tools.ietf.org/html/rfc2324",

},

},

}

)

// Status responds with the specified status code. TODO: support random choice

// from multiple, optionally weighted status codes.

func (h *HTTPBin) Status(w http.ResponseWriter, r *http.Request) {

parts := strings.Split(r.URL.Path, "/")

if len(parts) != 3 {

writeError(w, http.StatusNotFound, nil)

return

}

code, err := parseStatusCode(parts[2])

if err != nil {

writeError(w, http.StatusBadRequest, err)

return

}

// default to plain text content type, which may be overriden by headers

// for special cases

w.Header().Set("Content-Type", textContentType)

if specialCase, ok := statusSpecialCases[code]; ok {

for key, val := range specialCase.headers {

w.Header().Set(key, val)

}

w.WriteHeader(code)

if specialCase.body != nil {

w.Write(specialCase.body)

}

return

}

w.WriteHeader(code)

}

The 40x status codes are not really redirects. So redirectSpecialCases might be (in my point of view) a bit confusing.

Ah yeah, statusSpecialCases is much, much better. Good idea!

I renamed it back.

mccutchen · 2023-12-10T15:48:44Z

httpbin/static/index.html

+<li><a href="."><code>{{.Prefix}}/</code></a> This page.</li>
+<li><a href="absolute-redirect/6"><code>{{.Prefix}}/absolute-redirect/:n</code></a> 302 Absolute redirects <em>n</em> times.</li>
+<li><a href="anything"><code>{{.Prefix}}/anything/:anything</code></a> Returns anything that is passed to request.</li>
+<li><a href="base64/aHR0cGJpbmdvLm9yZw=="><code>{{.Prefix}}/base64/:value</code></a> Decodes a Base64-encoded string.</li>
+<li><a href="base64/decode/aHR0cGJpbmdvLm9yZw=="><code>{{.Prefix}}/base64/decode/:value</code></a> Explicit URL for decoding a Base64 encoded string.</li>


I know this is gonna be annoying and repetitive, but I'd prefer that we make these absolute URLs in both places:

<li><a href="{{.Prefix}}/anything"><code>{{.Prefix}}/anything/:anything</code></a> Returns anything that is passed to request.</li>

I fixed it. But I would prefer relative links. That was my original goal for the MR. All other stuff was only for completeness of redirect/location handlers. Thats why I named the branch relative-doc-links. I did not change documentation because that might change something. For href the browser will go to direct link.

I'm curious, why do you prefer relative links here? I think it should accomplish the exact same thing, and I just prefer the explicitness of absolute links (even though they're more verbose in the template).

For relative links it would be possible to set prefix also from outside (e.g. with nginx) without setting prefix if you do not care for the redirects. So I think relative will not hurt. But sure as it is it works now.

httpbin/handlers_test.go

httpbin/handlers.go

mccutchen · 2023-12-10T16:08:31Z

httpbin/handlers.go

-	w.Header().Set("Location", "/cookies")
+	w.Header().Set("Location", h.prefix+"/cookies")
 	w.WriteHeader(http.StatusFound)


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.)

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

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.

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://...

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://?

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.

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

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

httpbin/handlers_test.go

cmd/go-httpbin/.gitignore

mccutchen · 2023-12-10T16:20:48Z

Do you know https://github.com/stretchr/testify? Looks a bit similar to your assert. With that I would not have to add a assert.Contains method which I needed because of two tests of body.

Yes, I use that and a variety of other testing helper packages in other projects. For this project, an explicit goal is using zero dependencies outside the stdlib, in order to make it easier for other projects to decide to pull it into their own test suites.

- camel case method names - unnecessary app field handlers_test.go - add missing appending for prefix environment test - missing local variable for env

- rename doRedirect to handleRedirect - create new doRedirect - make link tests also for prefix

httpbin/static/index.html.tmpl

mccutchen

Almost there! Thanks a lot for the effort you've put into this @waschik, I know it turned into a much bigger change than you originally intended.

mccutchen · 2023-12-11T15:01:36Z

httpbin/cmd/cmd.go

@@ -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)")


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

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.

- use method doRedirect for all redirects - do not add prefix for external redirects (starting with schema, not starting with slash)

mccutchen

👍 LGTM! Really nice work here @waschik, thanks again for your persistence in getting this implemented!

mccutchen · 2023-12-12T23:12:10Z

FYI @waschik @mloskot these changes were released in v2.13.0

mloskot · 2023-12-12T23:13:54Z

Thank you @waschik & @mccutchen !

mloskot · 2023-12-13T16:48:32Z

The prefix works like a charm!

I just had to tweak Ingress for Kong Ingress Controller in my cluster to 1) match path used for path-based with the prefix 2) not not strip the path when passing request to the go-httpbin backend, for example:

---
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: httpbin
  namespace: tests
  annotations:
    konghq.com/strip-path: "false"
spec:
  ingressClassName: kong
  rules:
  - host: my.example.com
    http:
      paths:
      - path: /tests/httpbin
        pathType: ImplementationSpecific
        backend:
          service:
            name: httpbin
            port:
              number: 8080

Make documentation links relative

bfcb998

waschik added 2 commits May 20, 2023 14:48

Add .gitignore for executable

6906ef9

Create redirects for prefix

55b2b2a

waschik changed the title ~~Make documentation links relative~~ Support path prefix May 20, 2023

Merge remote-tracking branch 'github.com/mccutchen/go-httpbin/main' i…

aef0101

…nto relative-doc-links

mccutchen reviewed Dec 10, 2023

View reviewed changes

mccutchen changed the title ~~Support path prefix~~ feat: support serving under a path prefix Dec 10, 2023

waschik added 5 commits December 10, 2023 19:15

Fix many of @mccutchen's comments

be02898

- camel case method names - unnecessary app field handlers_test.go - add missing appending for prefix environment test - missing local variable for env

Add .tmpl file suffix for template files

566858e

Fix expected result of command line

5ef2dd3

Provide doRedirect method (suggest by mccutchen)

41818fc

- rename doRedirect to handleRedirect - create new doRedirect - make link tests also for prefix

Make relative links absolute again

e5a2611

waschik commented Dec 10, 2023

View reviewed changes

httpbin/static/index.html.tmpl Outdated Show resolved Hide resolved

mccutchen reviewed Dec 11, 2023

View reviewed changes

waschik added 4 commits December 11, 2023 22:52

Renamed specialCases to statusSpecialCases

454e91d

Use doRedirect for all redirects

b593a82

- use method doRedirect for all redirects - do not add prefix for external redirects (starting with schema, not starting with slash)

Add PREFIX environment variable

7d6acb7

Check for valid prefix values (including test)

3a796c7

mccutchen approved these changes Dec 12, 2023

View reviewed changes

mccutchen merged commit c86dfa0 into mccutchen:main Dec 12, 2023
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support serving under a path prefix #120

feat: support serving under a path prefix #120

waschik commented May 19, 2023

mccutchen commented May 19, 2023

waschik commented May 20, 2023 •

edited

Loading

mloskot commented Dec 8, 2023

mccutchen commented Dec 8, 2023

waschik commented Dec 9, 2023

mccutchen left a comment

mccutchen Dec 10, 2023

waschik Dec 10, 2023

mccutchen Dec 11, 2023

waschik Dec 11, 2023

mccutchen Dec 10, 2023

waschik Dec 10, 2023

mccutchen Dec 11, 2023

waschik Dec 11, 2023

mccutchen Dec 10, 2023

waschik Dec 10, 2023 •

edited

Loading

mccutchen Dec 10, 2023

waschik Dec 10, 2023 •

edited

Loading

mccutchen Dec 11, 2023

waschik Dec 11, 2023

mccutchen Dec 11, 2023

waschik Dec 11, 2023

mccutchen commented Dec 10, 2023

mccutchen left a comment

mccutchen Dec 11, 2023

waschik Dec 11, 2023

mccutchen left a comment

mccutchen commented Dec 12, 2023

mloskot commented Dec 12, 2023

mloskot commented Dec 13, 2023

	statusSpecialCases = map[int]*statusCase{
	300: {
	body: statusHTTP300body,
	headers: map[string]string{
	"Location": "/image/jpeg",
	},
	},
	301: statusRedirectHeaders,
	302: statusRedirectHeaders,
	303: statusRedirectHeaders,
	305: statusRedirectHeaders,
	307: statusRedirectHeaders,
	308: {
	body: statusHTTP308Body,
	headers: map[string]string{
	"Location": "/image/jpeg",
	},
	},
	401: {
	headers: map[string]string{
	"WWW-Authenticate": `Basic realm="Fake Realm"`,
	},
	},
	402: {
	body: []byte("Fuck you, pay me!"),
	headers: map[string]string{
	"X-More-Info": "http://vimeo.com/22053820",
	},
	},
	406: {
	body: statusNotAcceptableBody,
	headers: map[string]string{
	"Content-Type": jsonContentType,
	},
	},
	407: {
	headers: map[string]string{
	"Proxy-Authenticate": `Basic realm="Fake Realm"`,
	},
	},
	418: {
	body: []byte("I'm a teapot!"),
	headers: map[string]string{
	"X-More-Info": "http://tools.ietf.org/html/rfc2324",
	},
	},
	}
	)

	// Status responds with the specified status code. TODO: support random choice
	// from multiple, optionally weighted status codes.
	func (h HTTPBin) Status(w http.ResponseWriter, r http.Request) {
	parts := strings.Split(r.URL.Path, "/")
	if len(parts) != 3 {
	writeError(w, http.StatusNotFound, nil)
	return
	}
	code, err := parseStatusCode(parts[2])
	if err != nil {
	writeError(w, http.StatusBadRequest, err)
	return
	}

	// default to plain text content type, which may be overriden by headers
	// for special cases
	w.Header().Set("Content-Type", textContentType)

	if specialCase, ok := statusSpecialCases[code]; ok {
	for key, val := range specialCase.headers {
	w.Header().Set(key, val)
	}
	w.WriteHeader(code)
	if specialCase.body != nil {
	w.Write(specialCase.body)
	}
	return
	}

	w.WriteHeader(code)
	}

feat: support serving under a path prefix #120

feat: support serving under a path prefix #120

Conversation

waschik commented May 19, 2023

mccutchen commented May 19, 2023

waschik commented May 20, 2023 • edited Loading

mloskot commented Dec 8, 2023

mccutchen commented Dec 8, 2023

waschik commented Dec 9, 2023

mccutchen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waschik Dec 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waschik Dec 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mccutchen commented Dec 10, 2023

mccutchen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mccutchen left a comment

Choose a reason for hiding this comment

mccutchen commented Dec 12, 2023

mloskot commented Dec 12, 2023

mloskot commented Dec 13, 2023

waschik commented May 20, 2023 •

edited

Loading

waschik Dec 10, 2023 •

edited

Loading

waschik Dec 10, 2023 •

edited

Loading