single ingress path regex error breaks all other ingress objects processing

[r0bj]

What steps did you take and what happened:
If ingress path regex is broken (e.g. too long) it breaks all other ingress objects processing. It means that a single ingress object containing broken regex can break entire ingress controller. During that time all other ingress object are not updated, ingress changes are not reflected in envoy.

Example:
ingress:

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: broken-contour
spec:
  rules:
  - host: test.example.com
    http:
      paths:
      - backend:
          service:
            name: test-service
            port:
              number: 80
        path: /{corporate-pages:about|careers|contact|mediakit|press|partnerships|privacy-policy|do-not-sell-my-info}
        pathType: Prefix

Contour error message is generated:

time="2021-11-22T20:39:22Z" level=error msg="regex '/\\{corporate-pages:about\\|careers\\|contact\\|mediakit\\|press\\|partnerships\\|privacy-policy\\|do-not-sell-my-info\\}((\\/).*)?' RE2 program size of 121 > max program size of 100 set for the error level threshold. Increase configured max program size if necessary." code=13 connection=561 context=xds node_id=envoy-3-vxb7p node_version=v1.19.1 response_nonce=1 version_info=7

What did you expect to happen:
Even if regex is broken in single ingress object contour should be able to still process all other ingresses.

Anything else you would like to add:
It would probably be useful to have some metric that could indicate that contour ingress object processing is broken. Otherwise it's difficult to detect such case when not looking at contour logs.

Environment:

• Contour version: v1.19.1
• Kubernetes version: (use kubectl version):

Client Version: version.Info{Major:"1", Minor:"21", GitVersion:"v1.21.2", GitCommit:"092fbfbf53427de67cac1e9fa54aaa09a28371d7", GitTreeState:"clean", BuildDate:"2021-06-16T12:52:14Z", GoVersion:"go1.16.5", Compiler:"gc", Platform:"darwin/amd64"}
Server Version: version.Info{Major:"1", Minor:"21", GitVersion:"v1.21.2", GitCommit:"092fbfbf53427de67cac1e9fa54aaa09a28371d7", GitTreeState:"clean", BuildDate:"2021-06-16T12:53:14Z", GoVersion:"go1.16.5", Compiler:"gc", Platform:"linux/amd64"}

[sunjayBhatia]

with that path type (prefix) we should potentially not allow you to specify a regex, the meaning of Prefix path match type changed when we implemented Ingress v1 properly according to the spec: https://github.com/projectcontour/contour/releases/tag/v1.14.0

sidebar: you can probably achieve what you want by using the ImplementationSpecific path type strike that, currently we won't detect your regex as a regex even in ImplementationSpecific path match mode, we will need to fix that as well

[sunjayBhatia]

I think the action item here is to ensure prefix patch matches are validated as a plain path, if they have regex characters, reject them and log (we can look at incrementing a metric as well), since Ingress doesn't have a detail status field

[sunjayBhatia]

also what are you trying to match with /{corporate-pages:about|careers|contact|mediakit|press|partnerships|privacy-policy|do-not-sell-my-info}?

e.g. /{corporate-pages:about} or /corporate-pages:about (since the former would need to be escaped)

[r0bj]

Yeah, this is OR paths regex. But particular regex is not super important here. The main issue was that contour reconciliation loop was broken and other ingresses stoped being updated.
It can be problematic with multiple teams creating/updating ingresses for their services.

[sunjayBhatia]

yeah there are a couple main issues for contour here:

• we don't check if a Prefix type path is a regex and subsequently reject it:

  contour/internal/dag/ingress_processor.go

  Line 236 in 09d0488

  case networking_v1.PathTypePrefix:

• we have a naive method of finding if a given path match "looks like" a regex:

  contour/internal/dag/ingress_processor.go

  Line 252 in 09d0488

  if strings.ContainsAny(path, "^+*[]%") {

  • so your example would not get programmed as a regex
• we don't do a good job of handling NACKs from Envoy generally yet, but this is something we're aware of and taking steps to fix, typically we try to do really good validation of configuration before it gets sent to Envoy, also so there's faster feedback for the user

[sunjayBhatia]

this code:

contour/internal/dag/ingress_processor.go

Line 252 in 09d0488

if strings.ContainsAny(path, "^+*[]%") {

needs a bit of a re-examination bc it prevents one from using properly encoded url as a simple prefix, which should be allowed, e.g. path match type ImplementationSpecific with path /foo%7C (encoded form of /foo|

Envoy doesn't match requests to /foo%7C when configured with a regex, exact, or prefix match for /foo|, we do not properly encode paths it seems for any of our path matching methods

[sunjayBhatia]

I think we'll also want to insert a ^ anchor for all the regex patch matches we create, that will reduce the complexity of the regexes we generate

[sunjayBhatia]

still bandaids to fix the general NACK issue however ^

[sunjayBhatia]

it appears we also just generally need to make sure a Prefix match with a very long prefix will work, that is why this particular Ingress is being rejected, the ((\/).*)? regex suffix added for "segment prefix" matching could be improved

[youngnick]

Thanks for investigating this @sunjayBhatia. Let's xref to #1176 to make sure we capture the NACK requirement.

The actual breaking of the reconciliation loop is happening here because:

• we accept the config when we shouldn't
• we send an invalid config to Envoy
• Envoy rejects the config
• The previous three steps will continue happening until the invalid config is fixed.

I agree that properly solving #1176 will also solve this, but that's a lot of work, so we should clean up this path handling as much as possible now.

[sunjayBhatia]

Also created #4196 after some experimentation, would love people's thoughts on this as well

[sunjayBhatia]

@r0bj if you're able to use a branch to test, you can check out #4197

[github-actions]

The Contour project currently lacks enough contributors to adequately respond to all Issues.

This bot triages Issues according to the following rules:

• After 60d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, the Issue is closed

You can:

• Mark this Issue as fresh by commenting
• Close this Issue
• Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

[github-actions]

The Contour project currently lacks enough contributors to adequately respond to all Issues.

This bot triages Issues according to the following rules:

• After 60d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, the Issue is closed

You can:

• Mark this Issue as fresh by commenting
• Close this Issue
• Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack