Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

After Contour 0.5, envoy match config converted to "prefix" rather than "regex" #1243

Closed
johnnason opened this issue Jul 11, 2019 · 8 comments · Fixed by #1276
Closed

After Contour 0.5, envoy match config converted to "prefix" rather than "regex" #1243

johnnason opened this issue Jul 11, 2019 · 8 comments · Fixed by #1276
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Milestone

Comments

@johnnason
Copy link

We've been running a production workload using Contour 0.5.0 with Envoy 1.6.0 for some time.

I've tried to upgrade to every release between that and 0.11.0, and can not get some of our needed Ingress working.

Here's a contrived example that exhibits the class of problem:

      - backend:
          serviceName: settings
          servicePort: 80
        path: /[^/]+/settings

At 0.5.0 this would generate an envoy config such as:

    {
     "match": {
      "regex": "/[^/]+/settings"
     },
     "route": {
      "cluster": "ns/settings/80"
     }
    },

As of > 0.5.0 this would generate an envoy config such as:

         {
          "match": {
           "prefix": "/[^/]+/settings"
          },
          "route": {
           "cluster": "ns/settings/80/da39a3ee5e",
           "request_headers_to_add": [
            {
             "header": {
              "key": "x-request-start",
              "value": "t=%START_TIME(%s.%3f)%"
             },
             "append": true
            }
           ]
          }
         },

I'm kind-of stuck - I can't figure out how to get those regex routes working again.

@johnnason johnnason changed the title After Contour 0.5, envoy config converted to "prefix" rather than "regex" After Contour 0.5, envoy match config converted to "prefix" rather than "regex" Jul 11, 2019
@davecheney
Copy link
Contributor

Thank you for raising this issue; is this using Ingress or IngressRoute?

@davecheney davecheney added the blocked/needs-info Categorizes the issue or PR as blocked because there is insufficient information to advance it. label Jul 12, 2019
@davecheney davecheney self-assigned this Jul 12, 2019
@johnnason
Copy link
Author

Originally using Ingress, but I tried IngressRoute as well this morning with the exact same behavior exhibited. I'm OK using either, as long as I can get a regex path. I thought I understood IngressRoute as explicitly not supporting regex which is why I had not tried it earlier.

@davecheney
Copy link
Contributor

@johnnason thank you for confirming. We have never supported regex matching for IngressRoute paths. If that worked in 0.5 it was unintentional and I apologise for the confusion.

For ingress, the path spec is defined as a regex. We have to do a bit of work to translate that to Envoy because regex's are a different type of matcher function to straight prefix matching. Thus we try to detect "does this path have regex meta characters in it" as a switch to use a regex matcher. It is probable that our matching is not accurate.

Can you please confirm, is this the regex you tried to use with Ingress?

/[^/]+/settings

@johnnason
Copy link
Author

@davecheney

Can you please confirm, is this the regex you tried to use with Ingress?

I modified it slightly for simplicity. An actual ingress path that exhibits the problem:
/[^/]+/invoices(/.*|/?)
I had omitted the trailing match but including now in case it's significant, and understanding a bit more now wrt matching code.

@davecheney
Copy link
Contributor

Thank you for providing the repo case.

I looked into the code that should be switching between prefix matching and regex matching and at least on the first pass last night, I think this code is broken. If that's the case it's probably been broken for some time (maybe as early as 0.6). I'll try to restore this functionality for 0.14

@davecheney davecheney added kind/bug Categorizes issue or PR as related to a bug. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. and removed blocked/needs-info Categorizes the issue or PR as blocked because there is insufficient information to advance it. labels Jul 18, 2019
@davecheney davecheney added this to the 0.14.0 milestone Jul 18, 2019
@davecheney
Copy link
Contributor

I'm sorry I don't think this can be fixed for 0.14. I'm going to move to 0.15 and evaluate if we need to do a 0.14.1 release once the fix lands in master in the 0.15 cycle.

@davecheney davecheney removed their assignment Jul 19, 2019
@davecheney davecheney added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. labels Jul 19, 2019
@davecheney davecheney modified the milestones: 0.14.0, 0.15.0 Jul 19, 2019
@stevesloka stevesloka self-assigned this Jul 26, 2019
@davecheney
Copy link
Contributor

This has been backported to 0.14.1, https://github.com/heptio/contour/releases/tag/v0.14.1

@johnnason
Copy link
Author

Upgraded to 0.14.1 and it worked like a charm! Our regexp paths are back.

Thanks for knocking this out for us 💯 🎉 🎊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants