feat: alias glaure.match as ngx.re.match #26

omegabytes · 2022-08-20T00:18:52Z

Injects the find and match functions from external gluare repo into the overridden ngx package.
This enables in-place substitution without updating the import target.

Hold until after #27

lua-tree/share/lua/5.1/kong/db/schema/typedefs.lua

omegabytes · 2022-08-22T17:56:47Z

Related to #25

mikefero

This is starting to take shape as well. I think you will run into conflicts since you have 2 PRs touching the patch, but should be pretty easy to take care of (we shall see which PR lands first). This will be nice when it is complete.

lua-tree/share/lua/5.1/kong/db/schema/typedefs.lua

mikefero · 2022-08-23T12:32:36Z

plugin/testdata/override-ngx-re-test.lua

+        },
+    }
+}
+


This test is very confusing and I feel a little incomplete; let's use a custom_validator here to test that local aliases re_find and re_match work against some fields. You can create fields for the find and the match and pass those to the custom_validator function along with another schema field which would be the thing you are searching for.

mikefero · 2022-08-23T12:38:48Z

lualibs/go/ngx/loader.go

+	ltbl := l.ToTable(ldr)
+	findFn := l.GetField(ltbl, "find")
+	matchFn := l.GetField(ltbl, "match")


These values could be null, we should handle that case and panic. Granted it shouldn't happen, but let's handle appropriately.

patches/lua-tree.patch

mikefero

Please don't squash commits during a review; it makes it harder to review the changes made during a request.

Let's improve the tests, goks is integral to Koko and we need to make sure we thoroughly test all that we can.

mikefero · 2022-08-24T12:37:34Z

patches/lua-tree.patch

-local re_match     = ngx.re.match
-local re_find      = ngx.re.find
-+local re_match     = require "go.re2".match
- local tostring     = tostring
- local concat       = table.concat
- local insert       = table.insert
+ local re_match     = ngx.re.match


This patch doesn't represent what is in the actual kong/db/schema/init.lua file within the lua-tree/share directory.

Please re-add re_find

Let's not remove local variables that are not affected by these changes.

The line after + local re_match = ngx.re.match isn't removed or changed; it isn't reflected in the diff since it is 3 lines after the last changed line. By way of example:

-- a/init.lua local tablex = require "pl.tablex" local pretty = require "pl.pretty" local utils = require "kong.tools.utils" local cjson = require "cjson" local vault = require "kong.pdk.vault".new() local is_reference = vault.is_reference local dereference = vault.get local setmetatable = setmetatable local getmetatable = getmetatable local re_match = ngx.re.match local re_find = ngx.re.find local tostring = tostring local concat = table.concat local insert = table.insert

-- b/init.lua local tablex = require "pl.tablex" local pretty = require "pl.pretty" local utils = require "kong.tools.utils" local cjson = { array_mt = {} } --- TODO(hbagdi) XXX analyze the impact local uuid = require "go.uuid".generate local setmetatable = setmetatable local getmetatable = getmetatable local re_match = ngx.re.match local re_find = ngx.re.find local tostring = tostring local concat = table.concat local insert = table.insert

I think there is some confusion being caused because this is a diff of a file containing a list diffs. Take another look at what's being added and removed from lua-tree.patch (ie changes in my commit) vs what each diff contained in lua-tree.patch is indicating should be added/removed (ie what should be the outcome of running patch -p1 < patches/lua-tree.patch).

That is the init.lua file that is checked in, not the diff between the 2.8.0 Kong Gateway OSS files in the luarock.

diff goks/patches/lua-tree.patch goks-orig/patches/lua-tree.patch 40c40 < - Use gluare instead of ngx.re --- > - Use go.re2 instead of ngx.re 52c52 < - use gluare::match instead of string.match --- > - use go.re2::match instead of string.match 64c64 < - Use gluare instead of ngx.re --- > - Use go.re2 instead of ngx.re 167c167 < @@ -1,12 +1,9 @@ --- > @@ -1,16 +1,12 @@ 181c181,186 < local re_match = ngx.re.match --- > -local re_match = ngx.re.match > -local re_find = ngx.re.find > +local re_match = require "go.re2".match > local tostring = tostring > local concat = table.concat > local insert = table.insert 304c309 < +local match = ngx.re.match --- > +local match = require "go.re2".match 413,414c418,425 < @@ -30,30 +30,8 @@ local fmt = string.format < local re_match = ngx.re.match --- > @@ -33,34 +26,12 @@ local fmt = string.format > local find = string.find > local gsub = string.gsub > local split = pl_stringx.split > -local re_find = ngx.re.find > -local re_match = ngx.re.match > +local re_find = require "go.re2".find > +local re_match = require "go.re2".match

Here you can see that there are more entries is the original patch then the current.

What are you confused about?

I'm sorry, I don't understand. I've looked at this several different ways and validated the desired contents of lua-tree.patch against the contents of the corresponding files in lua-tree/share.

mikefero · 2022-08-24T12:40:37Z

patches/lua-tree.patch

-@@ -33,34 +26,12 @@ local fmt           = string.format
- local find          = string.find
- local gsub          = string.gsub
- local split         = pl_stringx.split
-local re_find       = ngx.re.find
-local re_match      = ngx.re.match
-+local re_find       = require "go.re2".find
-+local re_match      = require "go.re2".match
+@@ -30,30 +30,8 @@ local fmt           = string.format
+ local re_match      = ngx.re.match


This patch doesn't represent what is in the actual kong/tools/utils.lua file within the lua-tree/share directory.

Please re-add re_find

Let's not remove local variables that are not affected by these changes.

Since I'm also removing pre-loading the gluare package, I've removed re_find where it is being declared but it is unused. I've only done so where we were previously using "go.re2".find. I would probably need to keep the pre-loader in vm.go to accurately leave this unchanged.

mikefero · 2022-08-24T15:23:29Z

plugin/testdata/override-ngx-re-test.lua

+        },
+        },
+    },
+}


nit: Two spaces for indents of Lua files (not four)

fields could have two regexes and a subject to simplify

custom validator should utilize these new regex fields for extending tests on the go side

nit: no need to create re_find and re_match, just use ngx.re.find|match directly

nit: just call plugin name ngx-re-test

A better schema for validation and testing with varying values could be (untested schema)

local typedefs = require "kong.db.schema.typedefs" return { name = "ngx-re-test", fields = { { config = { type = "record", fields = { { find_regex = { type = "string", is_regex = true }, }, { match_regex = { type = "string", is_regex = true }, }, { subject = { type = "string", required = true }, }, }, custom_validator = function(config) if config.find_regex ~= ngx.null and #config.find_regex > 0 then local from = ngx.re.find(config.subject, config.find_regex) if not from then return false, "failed to find subject " .. config.subject .. " using regex " .. config.find_regex end end if config.match_regex ~= ngx.null and #config.match_regex > 0 then local m = ngx.re.match(config.subject, config.match_regex) if not m then return false, "failed to match subject " .. config.subject .. " using regex " .. config.match_regex end end return true end, }, }, }, entity_checks = { { at_least_one_of = { "config.find_regex", "config.match_regex" } }, }, }

I looked at 19 regex/target strings used in match calls across several lua files. I've added 4 unit tests that use existing literals. I came across several regexes that seemingly have a malformed input. Others did not have existing tests or adequate documentation for me to efficiently derive a potential matching string to test against.

Complete list:

tested: * uuid regex ^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$ * header name ^[a-zA-Z0-9-_]+$ * authorization_header: ([^:]+):(.*) * /directory$ malformed? * vault: [[([^/]+)$]] mismatched ) * vault: (/[^/]+) unescaped / * casandra: [==[[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\:\$\ dangling backslash * get response type: [[^\s*q=([0-9]*[\.][0-9]+)\s*$]] unmatched closing peren ) * parse_directive_header: [[^\s*([^=]+)(?:=(.+))?]] unmatched closing peren ) unsure of input: * decode array arg: [[(.+)\[(\d*)\]$]] * decode arg [[^\.+|\.$]] * get response type [[^\s*(\S+\/\S+)\s*$]] * no Route matched with those values * /$ * [[\.well-known/]] .. acme-challenge .. /(.+) * ^([" .. [[\x21-\x2B\x2D\x2E\x30-\x7E\x80-\xF4]] .. "]+(?:,|$))+$ * ^([" .. [[\x21-\x2B\x2D\x2E\x30-\x7E\x80-\xF4]] .. "]+(?:/|$))+$ * (VALUE\)]==] * specific string "any string"

mikefero · 2022-08-24T15:23:44Z

plugin/validator_test.go

+			err = v.Validate(testCase)
+			assert.Nil(t, err)
+		}
+	})


Let's isolate this into it's own test; doesn't need to be with the LoadSchema tests.

Add more robust tests; see schema update suggestions

Something like this (test is not complete not tested)

func TestValidator_NginxRegex(t *testing.T) { schema, err := os.ReadFile("testdata/override-ngx-re-test.lua") require.NoError(t, err) require.NotNil(t, schema) v, err := NewValidator(ValidatorOpts{}) require.NoError(t, err) require.NotNil(t, v) pluginName, err := v.LoadSchema(string(schema)) require.NoError(t, err) require.Equal(t, "override-ngx-re-test", pluginName) tests := []struct { name string config string expectedErr string }{ { name: "ensure ngx.re.find is loaded and contains subject", config: `"find_regex": "([0-9])", "subject": "A123"`, }, { name: "ensure ngx.re.find is loaded and fails to contain subject", config: `"find_regex": "([a-z][0-9]+)", "subject": "A123"`, expectedErr: "failed to find subject A123 using regex ([a-z][0-9]+)", }, { name: "ensure ngx.re.match is loaded and matches subject", config: `"match_regex": "([A-Z][0-9]+)", "subject": "A123"`, }, { name: "ensure ngx.re.match is loaded and fails to match subject", config: `"match_regex": "([a-z][0-9]+)", "subject": "A123"`, expectedErr: "failed to match subject A123 using regex ([a-z][0-9]+)", }, } for _, test := range tests { plugin := fmt.Sprintf(`{ "name": "ngx-re-test", "config": { %s }, "enabled": true, "protocols": ["http"] }`, test.config) err = v.Validate(plugin) if len(test.expectedErr) > 0 { require.ErrorContains(t, err, test.expectedErr) } else { require.NoError(t, err) } } }

omegabytes · 2022-08-24T21:50:28Z

plugin/validator_test.go

+			// {
+			// 	name: "ensure ngx.re.find works for validate_path",
+			// 	config: `"find_regex": "[[^[a-zA-Z0-9\.\-_~/%]*$]]",
+			// 	"subject": "/path"`,
+			// },


I'm guessing what we want to do with these tests is validate some (all?) of the in-use regex strings. I would hope the existing tests that cover these functions would be sufficient, but I understand why we might want to directly test the behavior with specific unit tests.

Either way, when I placed the regex string found in validate_path_with_regexes under test, it is rejected with an unexpected error: invalid character '.' in string escape code. Dropping the escape characters from the regex resolves ([[^[a-zA-Z0-9\.\-_~/%]*$]] -> [[^[a-zA-Z0-9.-_~/%]*$]]). That would suggest that the existing regex isn't valid? Running the full test suite (the failing test excluded) does not result in any failures, so maybe this specific example isn't exercised by an existing test and I've found a regression.

I also noticed the following signature in use: ngx.re.find("", path, "aj"). It's interesting to me that the 3rd arg (options) is a string originally, still a string here but is rejected by our aliased gluare.re_find function when I use it in a test as it requires an int, not a string.

My understanding was the overrides in lualibs/go/ngx/loader.go are global. These observations make it seem like that might not be so?

So it looks like the signature for gluare::reFind() is different than ngx.re.find so this replacement is not a 1-to-1 and something that wasn't noticed early in the development of goks. This must also mean that ngx.re.find is not used in our plugin validation or only the subject and regex parameters were used.

gluare::reFind()

parameters

subject

regex

starting index

plaintext flag

returns

found position index

error

ngx.re.find

parameters

subject

regex

options; how the operation is performed

context

captured index to return

returns

from

to

error

Since parameters 1 and 2 are required for both implementations it is a close fit, but definitely not an exact; this may need to be re-evaluated and a ticket created to handle find in the event we need it in the future.

mikefero

Just a couple of nickel and dime things left; let's clean these up and try and merge these two goks PRs you have open.

patches/lua-tree.patch

mikefero · 2022-08-25T13:35:27Z

plugin/validator_test.go

+			// {
+			// 	name: "ensure ngx.re.find works for validate_path",
+			// 	config: `"find_regex": "[[^[a-zA-Z0-9\.\-_~/%]*$]]",
+			// 	"subject": "/path"`,
+			// },


So it looks like the signature for gluare::reFind() is different than ngx.re.find so this replacement is not a 1-to-1 and something that wasn't noticed early in the development of goks. This must also mean that ngx.re.find is not used in our plugin validation or only the subject and regex parameters were used.

gluare::reFind()

parameters

subject

regex

starting index

plaintext flag

returns

found position index

error

ngx.re.find

parameters

subject

regex

options; how the operation is performed

context

captured index to return

returns

from

to

error

Since parameters 1 and 2 are required for both implementations it is a close fit, but definitely not an exact; this may need to be re-evaluated and a ticket created to handle find in the event we need it in the future.

mikefero · 2022-09-08T13:02:37Z

internal/vm/vm.go

@@ -55,7 +54,6 @@ func New(opts Opts) (*VM, error) {
 	l.PreloadModule("go.rand", rand.Loader)
 	l.PreloadModule("go.uuid", uuid.Loader)
 	l.PreloadModule("go.ipmatcher", ipmatcher.Loader)
-	l.PreloadModule("go.re2", gluare.Loader)


We should keep these; no reason to remove them.

Leaving this here means I load in two places?
https://github.com/Kong/goks/pull/26/files#diff-52bbf82efe0ff91b7d6fef2ba5c50f71e7ee2b462edb2a74b0424fa40f73a910R26

yep, does that cause problems?

Not that I can tell. I've made some updates such that we don't reference go.re2 at all. If you agree with those changes, then I suggest we don't import here.

So because we have multiple goks PRs in flight and @tjasko has #27 in flight which uses go.re2 we should keep it. We can always make another PR to clean up the last little bit of go.re2 usage in our Lua code, but since it doesn't hurt I think we should keep it; especially since it is not a maintenance burden.

mikefero · 2022-09-08T13:05:15Z

lua-tree/share/lua/5.1/kong/tools/utils.lua

@@ -26,8 +26,7 @@ local fmt           = string.format
 local find          = string.find
 local gsub          = string.gsub
 local split         = pl_stringx.split
-local re_find       = require "go.re2".find


Weird that this is even here, since this removal has been in the patch for months. Nothing to do here.

That's why I wanted to clean it up. I'll leave it in per your suggestion.

Actually, I went back and looked at both find and match signatures between gluare and ngx.re. They both methods are not an exact match. I've changed this line to instead reference the original ngx.re.find (even though it is still unused). This satisfies the goal of removing the need to patch custom plugins.

nope they are not a 1:1 either unfortunately; we have had to make changes to some of the regex formats used.

mikefero · 2022-09-08T13:17:20Z

lualibs/go/ngx/loader.go

+	// fetch gluare.find and gluare.match functions
+	// see https://github.com/yuin/gluare/blob/master/gluare.go


Could we add a blurb here talking about the differences between ngx.re.find and go.re2.find? Since ngx.re.find uses PCRE and go.re2.find uses RE2 we know there is a difference and we should let futureselves know when we look at the code again.

Documented LoadNgx as well as the overrides of find and match.

mikefero · 2022-09-08T19:13:22Z

patches/lua-tree.patch

@@ -37,7 +37,7 @@ schema/init.lua:
 - rework goto statements
 - Use an empty lua table for cjson.array_mt, this can potentially
  introduce serilialization errors
- Use gluare instead of ngx.re
+- Use go.re2::match instead of ngx.re.match


for all of these we can remove the lines like you did previously; we aren't modifying the Lua files anymore so we will know to check the ngx loader.

patches/lua-tree.patch

mikefero

One small request for the comments in the patch, please make an overall comment about ngx.re.<find|match> usage and remove the comments from the 3 file sections.

patches/lua-tree.patch

Injects the find and match functions from external gluare repo into the overridden ngx package. This enables in-place substitution without updating the import target.

mikefero

Nice work @omegabytes

omegabytes force-pushed the feat/re-find-match-fn-aliasing branch from f306f48 to 4e4c7e0 Compare August 22, 2022 17:33

omegabytes commented Aug 22, 2022

View reviewed changes

lua-tree/share/lua/5.1/kong/db/schema/typedefs.lua Show resolved Hide resolved

omegabytes force-pushed the feat/re-find-match-fn-aliasing branch from 4e4c7e0 to 7d9974b Compare August 22, 2022 17:41

omegabytes marked this pull request as ready for review August 22, 2022 17:42

omegabytes requested a review from a team as a code owner August 22, 2022 17:42

omegabytes requested review from javierguerragiraldez and mikefero August 22, 2022 17:42

omegabytes mentioned this pull request Aug 22, 2022

feat: override socket.url with patched.url for all imports #25

Merged

mikefero reviewed Aug 23, 2022

View reviewed changes

omegabytes force-pushed the feat/re-find-match-fn-aliasing branch 3 times, most recently from d208cd1 to d5b8e31 Compare August 23, 2022 23:10

omegabytes requested a review from mikefero August 23, 2022 23:10

mikefero reviewed Aug 24, 2022

View reviewed changes

omegabytes commented Aug 24, 2022

View reviewed changes

omegabytes force-pushed the feat/re-find-match-fn-aliasing branch 2 times, most recently from 8b20df1 to 02b50b2 Compare August 30, 2022 19:00

omegabytes requested a review from mikefero August 30, 2022 19:15

omegabytes changed the title ~~feat: add ngx.re.find and ngx.re.match~~ feat: ngx.re.match Aug 30, 2022

omegabytes changed the title ~~feat: ngx.re.match~~ feat: alias glaure.match as ngx.re.match Aug 30, 2022

omegabytes force-pushed the feat/re-find-match-fn-aliasing branch from 02b50b2 to 68dd8fc Compare September 7, 2022 17:32

mikefero reviewed Sep 8, 2022

View reviewed changes

omegabytes added the hold/do-not-merge label Sep 8, 2022

mikefero reviewed Sep 8, 2022

View reviewed changes

omegabytes force-pushed the feat/re-find-match-fn-aliasing branch 2 times, most recently from bd23651 to 752597f Compare September 8, 2022 20:49

mikefero previously approved these changes Sep 12, 2022

View reviewed changes

patches/lua-tree.patch Outdated Show resolved Hide resolved

mikefero dismissed their stale review via 71a5d2f September 13, 2022 21:26

mikefero force-pushed the feat/re-find-match-fn-aliasing branch from 752597f to 71a5d2f Compare September 13, 2022 21:26

mikefero force-pushed the feat/re-find-match-fn-aliasing branch 2 times, most recently from 8ce5c69 to a598673 Compare September 13, 2022 21:40

feat: add ngx.re.find and ngx.re.match

a4fb835

Injects the find and match functions from external gluare repo into the overridden ngx package. This enables in-place substitution without updating the import target.

mikefero force-pushed the feat/re-find-match-fn-aliasing branch from a598673 to a4fb835 Compare September 13, 2022 21:43

mikefero approved these changes Sep 13, 2022

View reviewed changes

mikefero removed the hold/do-not-merge label Sep 13, 2022

mikefero merged commit 8553e0d into main Sep 13, 2022

mikefero deleted the feat/re-find-match-fn-aliasing branch September 13, 2022 21:51

omegabytes mentioned this pull request Dec 7, 2022

feat: update Lua tree generation script & patch file for Kong 3.1.0-0 Luarock #35

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: alias glaure.match as ngx.re.match #26

feat: alias glaure.match as ngx.re.match #26

omegabytes commented Aug 20, 2022 •

edited

Loading

omegabytes commented Aug 22, 2022

mikefero left a comment

mikefero Aug 23, 2022

mikefero Aug 23, 2022

mikefero left a comment

mikefero Aug 24, 2022

omegabytes Aug 24, 2022 •

edited

Loading

mikefero Aug 24, 2022

mikefero Aug 24, 2022

omegabytes Aug 24, 2022 •

edited

Loading

mikefero Aug 24, 2022

omegabytes Aug 30, 2022

mikefero Aug 24, 2022

omegabytes Aug 30, 2022

mikefero Aug 24, 2022

omegabytes Aug 24, 2022 •

edited

Loading

mikefero Aug 25, 2022

mikefero left a comment

mikefero Aug 25, 2022

mikefero Sep 8, 2022

omegabytes Sep 8, 2022

mikefero Sep 8, 2022

omegabytes Sep 8, 2022

mikefero Sep 8, 2022

mikefero Sep 8, 2022

omegabytes Sep 8, 2022

omegabytes Sep 8, 2022

mikefero Sep 8, 2022

mikefero Sep 8, 2022

omegabytes Sep 8, 2022

mikefero Sep 8, 2022

mikefero left a comment

mikefero left a comment

		// fetch gluare.find and gluare.match functions
		// see https://github.com/yuin/gluare/blob/master/gluare.go

feat: alias glaure.match as ngx.re.match #26

feat: alias glaure.match as ngx.re.match #26

Conversation

omegabytes commented Aug 20, 2022 • edited Loading

omegabytes commented Aug 22, 2022

mikefero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikefero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omegabytes Aug 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omegabytes Aug 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omegabytes Aug 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gluare::reFind()

ngx.re.find

mikefero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gluare::reFind()

ngx.re.find

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikefero left a comment

Choose a reason for hiding this comment

mikefero left a comment

Choose a reason for hiding this comment

omegabytes commented Aug 20, 2022 •

edited

Loading

omegabytes Aug 24, 2022 •

edited

Loading

omegabytes Aug 24, 2022 •

edited

Loading

omegabytes Aug 24, 2022 •

edited

Loading