redpanda-data / connect

Fancy stream processing made operationally mundane
https://docs.redpanda.com/redpanda-connect/about/
8.09k stars 817 forks source link

http_server: Breaking change introduced with v4.14.0? #2018

Closed kabukky closed 1 year ago

kabukky commented 1 year ago

Hi,

I've just upgraded from v4.11.0 to v4.18.0. This broke my streams that use the http_server input.

You can replicate it by starting Benthos in streams mode with these two files and the -prefix-stream-endpoints=false option: streams.zip

This will make the HTTP route /api/v2/entries capture all requests made to /api/v2/entries/{id}. You can test that by making a GET request to "/api/v2/entries/foo" which will return "405 Method not allowed". This is because GET is not allowed by /api/v2/entries.

I think I found the change that introduces this behaviour: #61a Specifically using t.mux.PathPrefix(path) creates this issue. That way, the first matching prefix captures all paths that have this prefix. See also: https://stackoverflow.com/questions/28574875/go-gorilla-mux-not-routing-as-expected

So it doesn't seem to be possible to register routes that begin with a prefix that has already been registered before, such as "/foo" and "/foo/{name}". Is this correct? This is a big limitation for us. Is this just an oversight or intented behaviour?

Thanks!

kabukky commented 1 year ago

A small addition: I've just read the gorilla/mux documentation and I'm not sure if PathPrefix is a good choice. It introduces a catch all "wildcard" behaviour: "Note that the path provided to PathPrefix() represents a "wildcard": calling PathPrefix("/static/").Handler(...) means that the handler will be passed any request that matches "/static/*". This makes it easy to serve static files with mux:"

See: https://pkg.go.dev/github.com/gorilla/mux#readme-static-files

Jeffail commented 1 year ago

Hey @kabukky, I've marked this as a bug, will try and figure out a solution today.

Unfortunately, we are way beyond the point where we can eliminate any chance of backwards compatibility issues since there seem to be far more discrete differences between http.ServeMux and mux.Router than we'd ever anticipated. Trying to get consistency has been a bit like playing whack-a-mole.

Treating all paths as prefixes I believe (although kind of odd) was actually the original behaviour we were getting from http.ServeMux, and so we can't revert that without causing ourselves even more headaches. At the very least, what we can do for now is add a config field to toggle the behaviour so that you can work with a basic Path call.

Another factor in this great game of mux tennis is that the Go team are considering expanding the standard library multiplexer to support paths, and it's actually going to solve a lot of these ambiguities. From what I've seen we're going to 100% want to adopt it when it's ready: https://github.com/golang/go/issues/61410

kabukky commented 1 year ago

Hey @Jeffail thanks for your fast response!

Yeah, I've seen the change to the gorilla/mux package and assumed that's where it broke. I've created a pull request that fixes the behaviour for me. But a toggle should also work :)

Best!

Jeffail commented 1 year ago

@kabukky, okay the solution I've settled on is to continue using PathPrefix, but only in cases where the path ends with a slash. It's a bit of a dodgy work around but this should roughly match up with the old http.ServeMux behaviour whilst fixing the ambiguity you're facing. I've also added a caveat section to both http_server component docs pages that warns against path collisions for catch-alls: https://github.com/benthosdev/benthos/commit/fecd1cbf4dbdcf28f93e513ccefbef2cf82158e4

kabukky commented 1 year ago

@Jeffail That works quite well for my use case. Thanks! And thanks for the fast response, I'll be using that commit until the release.

Best!