Pathrewrite which maintains trailling slash without repeated config????

techmunk commented 3 years ago

What steps did you take and what happened: Was trying to create a pathRewritePolicy where the replacement ALWAYS keeps the trailing slash. I found in several configuration attempts, that this was not the case, and envoy ended up with a config which had either no route for a specified replacement, or multiple routes, one with a leading slash, and one without. I found I had to repeat myself quite a bit to get to the config I desired.

What did you expect to happen: I expected to be able to succinctly create a route with path replacement that maintained the trailing slash as I had specified.

Anything else you would like to add: The config I had which worked and ended up with a result I was happy with:

apiVersion: projectcontour.io/v1
kind: HTTPProxy
metadata:
  name: test
spec:
  virtualhost:
    fqdn: test.example.com
  routes:
    - conditions:
        - prefix: /
      pathRewritePolicy:
        replacePrefix:
          - replacement: /path-with-trailling-slash/
      services:
        - name: test-service
          port: 80
    - conditions:
        - prefix: /rq-path
      pathRewritePolicy:
        replacePrefix:
          - replacement: /path-with-trailling-slash/
      services:
        - name: test-service
          port: 80
    - conditions:
        - prefix: /rq-path/
      pathRewritePolicy:
        replacePrefix:
          - replacement: /path-with-trailling-slash/
      services:
        - name: test-service
          port: 80

In envoy, we end up with route config such as:

[
  {
    "match": {
      "prefix": "/rq-path/"
    },
    "route": {
      "prefix_rewrite": "/path-with-trailling-slash/"
    }
  },
  {
    "match": {
      "prefix": "/rq-path"
    },
    "route": {
      "prefix_rewrite": "/path-with-trailling-slash/"
    }
  },
  {
    "match": {
      "prefix": "/"
    },
    "route": {
      "prefix_rewrite": "/path-with-trailling-slash/"
    }
  }
]

My initial attempt which I believe should've worked:

routes:
  - conditions:
      - prefix: /
    pathRewritePolicy:
      replacePrefix:
        - replacement: /path-with-trailling-slash/
        - prefix: /rq-path
          replacement: /path-with-trailling-slash/

Envoy routes - curiously, there is no route/matcher for the /rq-path prefix (bug?):

[
    {
    "match": {
      "prefix": "/"
    },
    "route": {
      "prefix_rewrite": "/path-with-trailling-slash/"
    }
  }
]

My next attempt

routes:
  - conditions:
      - prefix: /
    pathRewritePolicy:
      replacePrefix:
        - replacement: /path-with-trailling-slash/
    services:
      - name: test-service
        port: 80
  - conditions:
      - prefix: /rq-path
    pathRewritePolicy:
      replacePrefix:
        - replacement: /path-with-trailling-slash/

Envoy config - This one was closer, however there are two matches for the /rq-path route. One with a trailing slash, one without. As I explicitly specified a trailing slash in my replacement, I'd expect it to still be there:

[
  {
    "match": {
      "prefix": "/rq-path/"
    },
    "route": {
      "prefix_rewrite": "/path-with-trailling-slash/"
    }
  },
  {
    "match": {
      "prefix": "/rq-path"
    },
    "route": {
      "prefix_rewrite": "/path-with-trailling-slash"
    }
  },
  {
    "match": {
      "prefix": "/"
    },
    "route": {
      "prefix_rewrite": "/path-with-trailling-slash/"
    }
  }
]

As you can see above, I've managed to find a solution that works, but my understanding from the documentation is that I should've been able to get this to work with much less repeated configuration. So either there's something not quite right in the code, or the documentation might need some extra clarification.

Environment:

Contour version: v1.14.1

Kubernetes version: (use kubectl version):

Client Version: version.Info{Major:"1", Minor:"20", GitVersion:"v1.20.5", GitCommit:"6b1d87acf3c8253c123756b9e61dac642678305f", GitTreeState:"archive", BuildDate:"2021-03-25T19:02:48Z", GoVersion:"go1.16.2", Compiler:"gc", Platform:"linux/amd64"}
Server Version: version.Info{Major:"1", Minor:"19", GitVersion:"v1.19.2", GitCommit:"f5743093fd1c663cb0cbc89748f730662345d44d", GitTreeState:"clean", BuildDate:"2020-09-16T13:32:58Z", GoVersion:"go1.15", Compiler:"gc", Platform:"linux/amd64"}

Kubernetes installer & version: Kubespray - version unknown
Cloud provider or hardware configuration: Locally built cluster
OS (e.g. from /etc/os-release): Centos 7

youngnick commented 3 years ago

I agree that this either should work as you expect @techmunk, or it should be documented not to. :)

I'll drop this into our "Needs Investigation", and we'll see what we can do.

alemonmk commented 3 years ago

I would like to add something. I'm trying to rewrite log out path of netbox to logout path of oauth2-proxy, the former have trailing slash while the latter have not. The result is similar to this issue.

First attempt:

spec:
  routes:
  - conditions:
    - prefix: /
  - conditions:
    - prefix: /static/
  - conditions:
    - prefix: /logout/
    pathRewritePolicy:
      replacePrefix:
      - replacement: /oauth2/sign_out

Result:

[
    {
        "match": {
            "prefix": "/static/"
        },
        "route": {
        }
    },
    {
        "match": {
            "prefix": "/logout/"
        },
        "route": {
            "prefix_rewrite": "/oauth2/sign_out/"
        }
    },
    {
        "match": {
            "prefix": "/logout"
        },
        "route": {
            "prefix_rewrite": "/oauth2/sign_out"
        }
    },
    {
        "match": {
            "prefix": "/"
        },
        "route": {
        }
    }
]

/logout/ got routed upstream and 404 me, though manually accessing /logout works.

Second attempt, which appeared it should work, but infinite redirection occurs because netbox sends me back to /logout/:

routes:
  - conditions:
      - prefix: /
  - conditions:
      - prefix: /static/
  - conditions:
      - prefix: /logout/
    pathRewritePolicy:
      replacePrefix:
        - replacement: /logout
  - conditions:
      - prefix: /logout
    pathRewritePolicy:
      replacePrefix:
        - replacement: /oauth2/sign_out

Result:

[
    {
        "match": {
            "prefix": "/static/"
        },
        "route": {
        }
    },
    {
        "match": {
            "prefix": "/logout/"
        },
        "route": {
            "prefix_rewrite": "/logout"
        }
    },
    {
        "match": {
            "prefix": "/logout"
        },
        "route": {
            "prefix_rewrite": "/oauth2/sign_out"
        }
    },
    {
        "match": {
            "prefix": "/"
        },
        "route": {
        }
    }
]

Maybe just rewriting response header with location: /oauth2/sign_out is a way out...

youngnick commented 3 years ago

hi @alemonmk, thanks for this additional information.

In your first example, I don't understand what happens. You said that /logout/ 404s? Does the redirect need to be /oauth2/sign_out/ or /oauth2/sign_out? It seems like the error here is that Contour adds a trailing / if there is one on the to-be-replaced prefix? (If so, there may be reasons related to HTTPProxy inclusion interfering, but want to check what's happening before going too far into problem-solving mode).

alemonmk commented 3 years ago

After a second (or more) thought I think I probably misunderstood what situation rewrites can be applied to solve problem.

To clarify, oauth2-proxy does not want the trailing slash. /logout/ in first example got rewritten to /oauth2/sign_out/, oauth2-proxy did not recognize it as logout path so the request was forwarded to upstream, then upstream 404'd me. Rewriting location header works great so there's no rush from me.

youngnick commented 2 years ago

Okay, sorry about the way too long delay on this, I spent some time recreating the original test case from @techmunk today.

I can definitely see the behavior you're talking about.

In the first case, you had two replacements, one that specified no prefix, and one that specified /rq-path. In the proxy you are using that in, /rq-path doesn't match the prefix you're matching, so the /rq-path rewrite config doesn't do anything. (In the reference docs, it says that the prefix in the replacePrefix block has to match the prefix used for routing including any inclusions, although I can see how this is unclear. We have to do this to be able to allow included HTTPProxies to rewrite under the path they are included under, but it means that the prefix field will be ignored if it doesn't match).

That's pretty unclear, so here's the snippet:

  - conditions:
      - prefix: /
    pathRewritePolicy:
      replacePrefix:
        - replacement: /path-with-trailling-slash
        - prefix: /rq-path  # This whole replacement doesn't ever take effect because it doesn't match the `prefix: /` field above
          replacement: /path-with-trailling-slash/

For the second try, this is a case where we're actually trying to help.

See the following comment I cut from the code (internal/dag/httpproxy_processor.go):

// expandPrefixMatches adds new Routes to account for the difference
// between prefix replacement when matching on '/foo' and '/foo/'.
//
// The table below shows the behavior of Envoy prefix rewrite. If we
// match on only `/foo` or `/foo/`, then the unwanted rewrites marked
// with X can result. This means that we need to generate separate
// prefix matches (and replacements) for these cases.
//
// | Matching Prefix | Replacement | Client Path | Rewritten Path |
// |-----------------|-------------|-------------|----------------|
// | `/foo`          | `/bar`      | `/foosball` |   `/barsball`  |
// | `/foo`          | `/`         | `/foo/v1`   | X `//v1`       |
// | `/foo/`         | `/bar`      | `/foo/type` | X `/bartype`   |
// | `/foo`          | `/bar/`     | `/foosball` | X `/bar/sball` |
// | `/foo/`         | `/bar/`     | `/foo/type` |   `/bar/type`  |

I believe that what you're requesting would come under the fourth case there.

I guess the question I would ask is: With your second config, what would you expect a request to /rq-pathsomething to be changed to?

/path-with-trailling-slash/something
/path-with-trailling-slashsomething

It seems like your expectation was that if you explicitly specify a trailing slash in the replacement, then you don't care about what you matched, so we should give you the second option (/bar/sball in the fourth item in the above table). Does that sound right?

This is one of those times where I think we've tried to help ensure people get what they actually want, not just what they asked for, but it sounds like it's not really working.

Sorry for the essay!

techmunk commented 2 years ago

@youngnick thanks for the detailed response.

For the first part, I think I assumed from the documentation you referenced, that it was also a prefix match. i.e. / is a prefix of /rq-path, so it would work/match, but it seems like it's more of an exact match scenario possibly? Rather than a prefix match as I had assumed? This is likely just down to how I interpreted the documentation on this, and I think how you have described contour working makes sense, even if it did not match my immediate expectation.

As for your question, if my replacement path has a trailing slash, then it should always be added, and /bar/sball should be what the path is rewritten to, as the addition of a trailing slash in a replacement path in my opinion is very deliberate, and should be respected. I would also expect it to be smart enough to not result in a double slash such as option 2 does but I can appreciate the complexities in fixing something so seemingly simple.

I think this was the biggest surprise to me, that I asked for a trailing slash always, but was not always getting it.

It might make sense to have an option to force the slash to be kept for both /foo and /foo/ so that both redirect to /bar/.

I also think that if the replacement path does not have a trailing slash, that the current behavior is probably OK.

youngnick commented 2 years ago

Okay, so it seems like we have one definite action: Clarify the documentation around the prefix field in the path rewrite to make it more clear how it works. Maybe an example will make it clearer.

For the second, it seems like the rule you expected is that, when you supply a trailing slash, you should always get it, since it's more specific. I'd like to hear from @skriss, as he was the one who did the initial implementation, but I think that rule makes sense to me.

youngnick commented 2 years ago

Ping @skriss again here, to see if he has anything to add.

skriss commented 2 years ago

I don't think I worked on this feature, but I'll take a look and see if I have anything to add.

projectcontour / contour

Pathrewrite which maintains trailling slash without repeated config???? #3613