motiv-labs / janus

An API Gateway written in Go
https://hellofresh.gitbooks.io/janus
MIT License
2.79k stars 317 forks source link

Format taget URL with parameters captured from the listened path #260

Closed olvlvl closed 6 years ago

olvlvl commented 6 years ago

Hi,

I'm trying to get the target URL formatted with the parameters captured form the listened path, but the whole path is appended to the target path. Please consider the following spec (excerpt):

{
    "proxy" : {
        "listen_path" : "/api/recipes/{id:[\\da-f]{24}}",
        "upstreams" : {
            "targets" : [
                {
                    "target" : "http://localhost:8069/recipes/{id}"
                }
            ]
        }
    },

I'd like paths such as /api/recipes/5981814cfeb6f31e78535e7d to be redirected to http://localhost:8069/recipes/5981814cfeb6f31e78535e7d.

Expected behavior:

/api/recipes/5981814cfeb6f31e78535e7d is redirected to http://localhost:8069/recipes/5981814cfeb6f31e78535e7d.

Observed behavior:

/api/recipes/5981814cfeb6f31e78535e7d is redirected to http://localhost:8069/recipes/%7Bid%7D/api/recipes/5981814cfeb6f31e78535e7d.

Janus version: Latest OS and version: Linux

vgarvardt commented 6 years ago

Looks like this is what you need:

{
    "proxy" : {
        "listen_path" : "/api/recipes/*",
        "append_path": true,
        "upstreams" : {
            "targets" : [
                {
                    "target" : "http://localhost:8069/recipes/"
                }
            ]
        }
    },
olvlvl commented 6 years ago

Not all endpoints under /recipes/ can be redirected to the recipe service e.g. /recipes/{id}/favorites, /recipes/{id}/ratings, /recipes/{id}/menus, …

stefanoj3 commented 6 years ago

I wrote a quick and -very dirty- test:

pkg/proxy/register_test.go
func TestT(t *testing.T) {
    d := &Definition{
        ListenPath: "/api/recipes/*/activate",
        Upstreams: &Upstreams{
            Balancing: "roundrobin",
            Targets:   []*Target{{Target: "http://recipes-service:9089/recipes/"}},
        },
        Methods: []string{"ALL"},
        AppendPath: true,
        StripPath: true,
    }

    r := router.NewChiRouter()
    regist := createRegister(r)

    fun := regist.createDirector(d, NewRoundrobinBalancer())

    req, err := http.NewRequest("GET", "http://localhost/api/recipes/1234/activate", nil)
    if err != nil {
        t.Fatal(err.Error())
    }

    t.Log(req.URL.Path)
    fun(req)
    t.Log(req.URL.Path)
}

And the output I get in the console is:

register_test.go:143: /api/recipes/1234/activate
register_test.go:145: /recipes/1234/activate

if I start using a regex(supported by https://github.com/go-chi/chi) instead of * in the ListenPath(eg /api/recipes/{id:[\\da-f]{24}}/activate) I get

register_test.go:143: /api/recipes/1234/activate
register_test.go:145: /recipes/api/recipes/1234/activate

(the same result mentioned by @olvlvl )

My understanding right now is that we need to change pkg/router/listen_path_matcher.go since right now it supports only * as a wildcard, but we need to be able to support stuff like {id:[\\da-f]{24}}. Currently the reverse proxy director rely on it to correctly strip the path when StripPath == true.

I haven't extensively tested my finding but it seems that it's all about it.

The only concern I would raise now is: do we have a way to define "priorities" for routes? Afaik the way chi(the underlying router) works is that it will match the first route possible.

So we need to be careful in defining routes eg: if we define first /api/recipes/* and then /api/recipes/{id:[\\da-f]{24}}/something the first route should be the one always selected.

Am I missing something?

olvlvl commented 6 years ago

I was told that static routes are matched first: https://github.com/go-chi/chi/issues/291

italolelis commented 6 years ago

Solved on #262