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

Add 3.x regex prefix for HTTPRoute #2873

Closed
wants to merge 22 commits into from
Closed

Add 3.x regex prefix for HTTPRoute #2873

wants to merge 22 commits into from

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Aug 25, 2022

What this PR does / why we need it:

Kong/kong#9022 requires that regular expression paths in Kong routes include a ~ prefix. If we receive an HTTPRoute with a regular expression path (whose canonical Gateway API form is the regex as-is, with no prefix) or some other path type that we convert into a regex (for example, exact match), prepend the prefix.

Which issue this PR fixes:

This addresses the error @randmonkey found in the 3.0 test logs:

2022/08/24 17:02:36 [error] 2142#0: *558 [lua] handler.lua:1153: could not rebuild router via timer: /usr/local/share/lua/5.1/kong/router/atc_compat.lua:417:  --> 1:23
  |
1 | (http.path ^= "/regex-\d{3}-httpbin")
  |                       ^---
  |
  = expected str_char or str_esc, context: ngx.timer

Special notes for your reviewer:

HTTPRoutes have a pretty clear path to handling this: they have a designated regular expression match type, and its canonical form is a regex without our special prefix. If we're using 3.x and we have a regex match, we prepend our prefix when creating the Kong route path.

Ingress is a bit less clear. There's no dedicated regex path type, just ImplementationSpecific. We could read that as requiring that you do provide the path in the exact form the Implementation expects, i.e. that you need to prepend the ~ yourself, but that's not obvious and presents an upgrade path nightmare. If we prepend it for you, it'll break configuration for anyone that reads Gateway docs and does prepend it themselves.

Checking to see if the prefix is present and adding it if not does seem fine though. Regular expressions without the prefix but with ~ as their first actual match character will not work, but these shouldn't occur since that's not a reserved character and should be vanishingly uncommon given that paths usually begin with / (apparently paths that do not are valid per the path-rootless in https://www.rfc-editor.org/rfc/rfc3986#section-3.3, but I don't think those are relevant to HTTP at least). Worst case if you need a regex that begins with ~ you can write your own ~~ to provide both that and the prefix.

Lastly, although we do not know if ImplementationSpecific paths are regexes or not, using the old Kong heuristic seems a valid approach. AFAICT this was a performance optimization on the gateway's side, not something discarded because it wasn't working.

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

  • the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

@rainest rainest added the ci/run-nightly Run nightly Kong image integration tests label Aug 25, 2022
@rainest rainest temporarily deployed to Configure ci August 25, 2022 19:51 Inactive
@rainest rainest added ci/run-nightly Run nightly Kong image integration tests and removed ci/run-nightly Run nightly Kong image integration tests labels Aug 25, 2022
@rainest rainest temporarily deployed to Configure ci August 25, 2022 20:13 Inactive
@rainest rainest temporarily deployed to Configure ci August 25, 2022 21:54 Inactive
@rainest rainest temporarily deployed to Configure ci August 25, 2022 22:16 Inactive
@rainest rainest temporarily deployed to Configure ci August 25, 2022 22:26 Inactive
@rainest rainest added ci/run-nightly Run nightly Kong image integration tests and removed ci/run-nightly Run nightly Kong image integration tests labels Aug 25, 2022
@rainest rainest temporarily deployed to Configure ci August 25, 2022 22:49 Inactive
@randmonkey randmonkey temporarily deployed to Configure ci August 26, 2022 09:50 Inactive
@randmonkey randmonkey added ci/run-nightly Run nightly Kong image integration tests and removed ci/run-nightly Run nightly Kong image integration tests labels Aug 26, 2022
@randmonkey randmonkey temporarily deployed to Configure ci August 26, 2022 10:10 Inactive
@randmonkey randmonkey temporarily deployed to Configure ci August 26, 2022 15:14 Inactive
@randmonkey randmonkey added ci/run-nightly Run nightly Kong image integration tests and removed ci/run-nightly Run nightly Kong image integration tests labels Aug 26, 2022
@randmonkey randmonkey temporarily deployed to Configure ci August 26, 2022 15:36 Inactive
@mflendrich
Copy link
Contributor

I think the cause is Kong/kong#9027, not 9022 (wrong link in the description?)

@rainest rainest temporarily deployed to Configure ci September 1, 2022 00:53 Inactive
@rainest rainest temporarily deployed to Configure ci September 1, 2022 01:47 Inactive
@rainest rainest removed the ci/run-nightly Run nightly Kong image integration tests label Sep 1, 2022
@rainest rainest temporarily deployed to Configure ci September 1, 2022 02:21 Inactive
@rainest rainest added the ci/run-nightly Run nightly Kong image integration tests label Sep 1, 2022
@rainest rainest added ci/run-nightly Run nightly Kong image integration tests and removed ci/run-nightly Run nightly Kong image integration tests labels Sep 1, 2022
Minor formatting and spelling fixes.

Change cleanIngress to panic instead of error. The error condition was
always author error, and not requiring error handling makes it simpler
to use in tests. Rename cleanIngress to addIngressToCleaner to better
reflect that it's queueing the clean, not immediately cleaning (and
should not be deferred as such).

Move default IngressClass testing to an E2E test. Using a default
IngressClass interferes with other tests that check if removing class
information from a resource removes it from configuration. The
controller ingress class is set at startup and integration tests cannot
use an alternate IngressClass.

Remove a spammy helper log in the E2E Kong deployer. Diagnostic dumps
will capture the Deployment state if it is the cause of a test failure.
@rainest rainest temporarily deployed to Configure ci September 1, 2022 19:58 Inactive
@rainest rainest temporarily deployed to Configure ci September 1, 2022 20:21 Inactive
@rainest rainest mentioned this pull request Sep 1, 2022
1 task
@rainest
Copy link
Contributor Author

rainest commented Sep 1, 2022

Closed in favor of #2883 to discard the mountain of force push/test/add label events. Ask me if you want the original branch with separated (if not particularly well-organized) commits.

@rainest rainest closed this Sep 1, 2022
@rainest rainest deleted the fix/3x-regex branch September 1, 2022 21:50
@rainest rainest restored the fix/3x-regex branch September 2, 2022 22:44
@rainest rainest deleted the fix/3x-regex branch September 2, 2022 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/run-nightly Run nightly Kong image integration tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants