Colons can't be escaped in path syntax

[DestyNova]

Describe the bug

Paths containing a literal colon can't be parsed literally and (apparently) fail to match.

To reproduce

Example RouteGroup definition:

    - path: /api/static/hello:world
      filters:
        - preserveHost("false")
        - setResponseHeader("Content-Type", "application/json")
        - 'inlineContent(`
          {
            "status": "Get to the choppa!"
          }
          `)'

2. Trying to call it:

curl -vH "Content-Type: application/json" \
	"$HOST/api/static/hello:world"

Expected behavior

Response code 200 with JSON content as defined in the inlineContent example.

Observed behavior

< HTTP/2 404
< date: Fri, 23 Apr 2021 14:20:04 GMT
< server: Skipper
<
{ [0 bytes data]

The same thing happens with both path and pathSubtree, probably because they're trying to parse :world as a placeholder for a path component. It probably expects to see those placeholders only preceded by a / rather than hello, so maybe that's why it won't match on the route I tried with curl. I tried escaping the colon in the path expression with %3a, %3A, \\: and \:, none of which worked, and the last one seemed to cause a YAML parsing failure.

Workaround

A colleague found that the pathRegexp filter treats the colon literally and presumably doesn't have path variable parsing, so we were able to use that to match successfully.

Possible fixes

Escaping the colon could be a bit awkward since \: seemed to cause a problem parsing the manifest. I guess we could come up with a different special escape sequence, but nobody likes that. We could also be more explicit about when a :word sequence is treated as a path variable placeholder -- it could be defined more strictly as only valid after a / path separator, and treated literally in all other cases. Or we could define it to be invalid in all other cases unless escaped, if we come up with a comfortable and intuitive way to escape them.

[aryszka]

as we already discussed, one possible solution can be to use a \ based escape style. E.g: \:

In this case the main caveat is that \\ should also become escaped, and this may break those possible cases where somebody used \ in the path. Question to everybody: are we aware of such a configuration?

If we go with the \ based escaping, then I am in favor of allowing any character to be escaped, not only the current special ones, even if most syntaxes limit the escaping to a fixed set of escape sequences. Any thoughts?

Of course, also open for alternative escaping, e.g. doubling: :: -> : But TBH, I personally prefer escaping via \. Any thoughts, or other alternatives?

[DestyNova]

If we can get \: or even \\: that would seem pretty decent.

[aryszka]

one more caveat: we often put these expressions into YAML documents in Kubernetes manifests. Using \ there can lead to various confusion. E.g if \: means : and \\ means \, then when wrapped with YAML, they would become \\: and \\\\. The doubling :: may be a better solution after all.

[AlexanderYastrebov]

Any thoughts, or other alternatives?

The Path spec https://github.com/zalando/skipper/blob/master/docs/reference/predicates.md#path says

The wildcards must follow a /

So maybe we may lift-up the :* check somehow https://github.com/zalando/skipper/blob/f27cb2d71530acf167e5c06e721a9b0286dbc9cb/pathmux/tree.go#L119-L121 and reduce prohibited paths to only those where segments start with : or *.

If we go with the \ based escaping

There is eskip \-based escaping as well but this one should carry \ to the path parsing stage. In the yaml string backslash should be also escaped so we need to be careful and clear on how this will work.

[aryszka]

...and reduce prohibited paths to only those where segments start with : or *.

Sounds like a simple pragmatic solution, though contains the future problem when somebody wants to use paths like /:foo . I can live with this for the time being, though.