fix: required changes for 3.1 compatibility #36

omegabytes · 2022-12-09T00:56:40Z

Additional updates required before goks works with kong v3.1. While this is in draft, please assume additional unobserved failures exist due to the iterative nature of the failures.

omegabytes · 2022-12-09T01:23:45Z

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

+-- `table.nkeys()` is a LuaJIT function and is not available to the Lua VM that goks uses.
+local nkeys
+do
+  local ok
+  ok, nkeys = pcall(require, "table.nkeys")
+  if not ok then
+    nkeys = function (tab)
+      local count = 0
+      for _, v in pairs(tab) do
+        if v ~= nil then
+          count = count + 1
+        end
+      end
+      return count
+    end
+  end
+end


This is copypasta from resty/healthecheck.lua. I'm following the pattern of local_tab initialized on L46 of this file. This addresses the error:

lua-tree/share/lua/5.1/kong/db/schema/init.lua:4: module table.nkeys not found: no field package.preload['table.nkeys'] open /table/nkeys.lua: file does not exist open lua-tree/share/lua/5.1/table/nkeys.lua: file does not exist open lua-tree/share/lua/5.1/table/nkeys/init.lua: file does not exist,

I can't tell if this resolves the issue, however. I get a new error:

Received unexpected error: lua-tree/share/lua/5.1/kong/constants.lua:222: table index is nil stack traceback: lua-tree/share/lua/5.1/kong/constants.lua:222: in function <lua-tree/share/lua/5.1/kong/constants.lua:0> [G]: in function 'require' lua-tree/share/lua/5.1/kong/db/schema/typedefs.lua:6: in function <lua-tree/share/lua/5.1/kong/db/schema/typedefs.lua:0> [G]: in function 'require' setup.lua:6: in main chunk [G]: ?

I'm able to validate the method is called. The if block beginning on line 62 is entered but nkeys = function (tab) on line 63 panics. I'm not sure if it's the method call itself or something else. A print statement on line 64 is never executed. Curiously, print statements before the method is called are also not executed?

The stacktrace is also not very illuminating: lua-tree/share/lua/5.1/kong/constants.lua:222: in function <lua-tree/share/lua/5.1/kong/constants.lua:0> is on a different line in kong-ee and points to LOG_LEVELS. This looks new to 3.1, I don't see it in this 3.0 branch.

My guess is either:

This is a new error. The introduction of LOG_LEVELS is causing something to break. What and why is unclear.

The update I made to nkeys somehow breaks this import, or something that processes this import.

Number 1 is the most reasonable, but I don't really understand what table index is nil means in this context and I'm struggling to trace where exactly this is breaking.

I don't really understand what table index is nil means in this context and I'm struggling to trace where exactly this is breaking.

the LOG_LEVELS table, starting in constants.lua:222 has this form:

LOG_LEVELS = { debug = ngx.DEBUG, info = ngx.INFO, notice = ngx.NOTICE, warn = ngx.WARN, error = ngx.ERR, crit = ngx.CRIT, alert = ngx.ALERT, emerg = ngx.EMERG, [ngx.DEBUG] = "debug", [ngx.INFO] = "info", [ngx.NOTICE] = "notice", [ngx.WARN] = "warn", [ngx.ERR] = "error", [ngx.CRIT] = "crit", [ngx.ALERT] = "alert", [ngx.EMERG] = "emerg", },

lines 231-238 use the form [key-expression] = value-expression, so it evaluates the ngx.DEBUG, ngx.INFO, etc to get the actual keys (unlike key-name=value-expression, which directly sets the key as a string constant). if any of these is nil, this error will trigger. unfortunately, this aborts the whole (sub) table construction, so we don't know which of these is failing.

Now to find where are those ngx.XXXX values....

Now to find where are those ngx.XXXX values....

Can't find them. I think the easiest would be to add to internal/vm/setup.go

omegabytes · 2022-12-09T01:25:01Z

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

-  local decode_base64url = require "ngx.base64".decode_base64url
+  local decode_base64url = ngx.decode_base64url


Addresses:

lua-tree/share/lua/5.1/kong/tools/utils.lua:1270: module ngx.base64 not found: no field package.preload['ngx.base64'] open /ngx/base64.lua: file does not exist open lua-tree/share/lua/5.1/ngx/base64.lua: file does not exist open lua-tree/share/lua/5.1/ngx/base64/init.lua: file does not exist,

This (and other instances) will need to be added to lua-tree.patch or fixed in kong-ee.

there's already lualibs/go/ngx/base64.go, which implements the ngx.decode_base64 function. i guess the URL variant should be added there, so line 1270 could go unmodified.

Adding a decode_base64url aliased function to lualibs/go/ngx/base64.go was my first approach. However, I noticed that updating the import without the rest of the work also resolved the issue. My guess is there may be a runtime error for calls to ngx.decode_base64url? I'll look at extending lualibs/go/ngx/base64.go and adding some test coverage if I find gaps.

there's already lualibs/go/ngx/base64.go, which implements the ngx.decode_base64 function. i guess the URL variant should be added there, so line 1270 could go unmodified.

We have encode_base64, no decode functions. Any way I change this will be internally inconsistent. I'd like to leave this change as-is without adding any new methods to the go.ngx package.

omegabytes · 2022-12-09T01:27:27Z

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

-    return bin
-  end
-end
+local sha256_bin = require "sha256".sha256_bin


Addresses:

Received unexpected error: lua-tree/share/lua/5.1/resty/openssl/digest.lua:1: module ffi not found: no field package.preload['ffi'] open /ffi.lua: file does not exist open lua-tree/share/lua/5.1/ffi.lua: file does not exist open lua-tree/share/lua/5.1/ffi/init.lua: file does not exist, stack traceback: [G]: in function 'require' lua-tree/share/lua/5.1/resty/openssl/digest.lua:1: in function <lua-tree/share/lua/5.1/resty/openssl/digest.lua:0> [G]: in function 'require' lua-tree/share/lua/5.1/kong/tools/utils.lua:1304: in function <lua-tree/share/lua/5.1/kong/tools/utils.lua:0> [G]: in function 'require'

This will need to be added to lua-tree.patch.

omegabytes · 2022-12-09T01:28:24Z

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

-  local to_hex       = require "resty.string".to_hex
+  local to_hex       = require "hex".to_hex


Addresses another instance of module ffi not found

javierguerragiraldez · 2022-12-09T10:47:23Z

internal/vm/vm.go

+	l.PreloadModule("sha256", sha256.Loader)
+	l.PreloadModule("hex", hex.Loader)


We're using the pattern "go.xxx" for those replacement modules:

Suggested change

l.PreloadModule("sha256", sha256.Loader)

l.PreloadModule("hex", hex.Loader)

l.PreloadModule("go.sha256", sha256.Loader)

l.PreloadModule("go.hex", hex.Loader)

lua-tree/share/lua/5.1/kong/constants.lua

omegabytes · 2022-12-10T03:55:17Z

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

@@ -11,6 +9,18 @@ local table_concat = table.concat
 local ngx_re_find = ngx.re.find
 local ngx_re_gsub = ngx.re.gsub

+-- `table.new()` is a LuaJIT function and is not available to the Lua VM that goks uses.
+local new_tab


Addresses

lua-tree/share/lua/5.1/kong/tools/uri.lua:1: module table.new not found

See

goks/lua-tree/share/lua/5.1/kong/db/schema/init.lua

Line 46 in 788f46c

local new_tab

lua-tree/share/lua/5.1/kong/pdk/private/node.lua

omegabytes · 2022-12-10T06:55:18Z

@javierguerragiraldez @GGabriele I'm seeking feedback on current progress and guidance on next steps.

I ripped out a bunch of stuff to get this to a point where tests are passing. I'm trying to get a complete picture and determine what can be removed vs what needs to be reworked. I took a shortcut and completely removed the atc and node files since they required a lot of ngx changes to work.

Here's the current status as I understand it:

Some of the stuff I removed should probably be replaced with go or alternative lua logic. I've tried to comment where I think this is the case, but I'm sure I'm missing the big picture.
Can I really remove the atc and node files wholesale? If not, what should be done? Good pairing opportunity ;-)
I'm duplicating a few things across files (table.new and table.clear, etc). These should probably be consolidated into a utils package.
I only addressed what caused the existing tests to fail, but I'm sure I'm missing some things. How do I increase my confidence in my changes? I can add more tests? Seems like that could explode the scope pretty quickly.
Speaking of TDD: I iteratively walked through failing tests and added a new commit at every step (git log --stat --oneline). Not sure how helpful the history will be, the goal was to convey a sense of how to break up this work if we need to carve off some larger tasks. Grouping tasks by which import is broken makes sense.
If the current changes are accurate, they represent a bunch of changes to lua-tree.patch. I expect this to be extremely tedious...
I have not pulled this into koko and tested yet, not sure what level of effort to expect there.
I ripped out a lot of the reporting, worker, and node logic. Those changes need to be probed and possibly carved out into discrete tasks.

javierguerragiraldez · 2022-12-12T11:15:10Z

Can I really remove the atc and node files wholesale?

yes. remember that we only use goks to validate plugins, not all of the config; much less during "real work". I did port the atc-router (#31), but then realised koko doesn't use those typedefs at all.

I'm duplicating a few things across files (table.new and table.clear, etc). These should probably be consolidated into a utils package.

full agree. right now the easiest would be to either add directly to the bunch of Lua code at internal/vm/setup.go, or create new small Lua files that would be require'd from there.

I only addressed what caused the existing tests to fail, but I'm sure I'm missing some things. How do I increase my confidence in my changes? I can add more tests? Seems like that could explode the scope pretty quickly.

again, remember we only use plugin config validation. i guess the "unit" level tests should test the typedefs, and the "functional" level should just test good and bad plugin configs. I think most of the existing tests are for the typedefs, while the plugins are mostly only for the "hard" features.

If the current changes are accurate, they represent a bunch of changes to lua-tree.patch. I expect this to be extremely tedious...

the biggest quality-of-life improvement here would be a script to recreate the patchfile completely from the changes to the Lua tree.

What I've done in the past is to use git-diff (or was it git format-patch?) and then copy/paste discrete chunks into the big patchfile. constantly applying it until it reproduced my changes with zero complains. In short, commit all the Lua changes, and then tweak the patchfile until regenerating the lua-tree results in no diffs as seen on git status (apart from the patchfile itself).

I have not pulled this into koko and tested yet, not sure what level of effort to expect there.

go work use ../goks is usually enough to test it with koko locally without fighting git too much... but there are ugly details on getting scripts right. in the end, to get CI to work you still have to tweak go.mod to pull from the unmerged branch.

I ripped out a lot of the reporting, worker, and node logic. Those changes need to be probed and possibly carved out into discrete tasks.

i wouldn't worry too much. if the plugin validators can reject/accept consistently, we're ok.

GGabriele · 2022-12-12T11:17:51Z

FYI: this will need to pull Kong 3.1.1-0 instead of 3.1.0-0

CLAassistant · 2022-12-12T22:55:38Z

All committers have signed the CLA.

omegabytes · 2022-12-14T18:35:10Z

Confirmed current changes work when imported into koko

// in go.mod
replace github.com/kong/goks => /Users/alex.gaesserkonghq.com/code/goks

All package validators tests pass. TestVersionCompatibility_310OrNewer also passes.

omegabytes · 2022-12-14T20:28:36Z

FYI: this will need to pull Kong 3.1.1-0 instead of 3.1.0-0

luarocks install kong 3.1.1-0 --tree lua-tree                                                                                                                                                                   

Error: No results matching query were found for Lua 5.4.
To check if it is available for other Lua versions, use --check-lua-versions.

omegabytes · 2022-12-14T20:39:28Z

@GGabriele @javierguerragiraldez @mikefero

I've trimmed down some of the changes and updated the patchfile. This should be ready to be merged into #35. Patch applies correctly, goks and koko tests passing.

mikefero · 2023-01-03T15:13:42Z

@omegabytes I am not sure of the state of this PR anymore now that we fixed the YAML processing for validation. Is this PR required anymore and will #35 be the only thing that still needs to be merged to complete the updates for 3.1.0?

omegabytes · 2023-01-05T18:16:01Z

Whoops! Didn't mean to close that!

GGabriele · 2023-01-25T11:24:20Z

At a high level, this looks good.
Are the newly introduced lualibs/go used on plugins? If so, it'd be nice to have some test validating these work as expected.

GGabriele · 2023-01-26T15:17:11Z

At a high level, this looks good. Are the newly introduced lualibs/go used on plugins? If so, it'd be nice to have some test validating these work as expected.

@omegabytes other than this ask, can you also sign the license CLA for this repo?

mikefero · 2023-02-07T13:15:44Z

@omegabytes With 3.2.0 on the horizon I am going to close this PR in favor of grabbing the newest updates after that release occurs.

omegabytes added 5 commits December 8, 2022 14:55

temp: ngx.decode_base64url

14256e9

replace lua resty.openssl.digest with go crypto/sha256

2f294c1

rename pkg crypto to sha256

4ac298b

replace lua resty.string.to_hex with go encoding/hex

b6e369b

replace table.nkeys with local fn

788f46c

omegabytes commented Dec 9, 2022

View reviewed changes

fmt and lint

86df807

javierguerragiraldez reviewed Dec 9, 2022

View reviewed changes

remove log_level from constants

ff8aeb1

omegabytes commented Dec 10, 2022

View reviewed changes

lua-tree/share/lua/5.1/kong/constants.lua Show resolved Hide resolved

replace table.new with local fn

ab748dd

omegabytes commented Dec 10, 2022

View reviewed changes

omegabytes added 3 commits December 9, 2022 20:26

remove/replace resty.* imports

393daa9

rework goto statements

739a308

remove ffi imports from node.lua

001809f

omegabytes force-pushed the fix/mocking branch from 5f2ce54 to 001809f Compare December 10, 2022 04:48

pdk/private/node.lua remove pl imports

884c12f

omegabytes commented Dec 10, 2022

View reviewed changes

lua-tree/share/lua/5.1/kong/pdk/private/node.lua Show resolved Hide resolved

omegabytes added 4 commits December 9, 2022 22:05

remove ngx imports from node and reports

23c7b76

override nkeys for router/utils

0345f15

remove ngx.worker.id ref

ea9bcf7

just get rid of atc already

b47d2e6

refactor: name go.hex and go.sha256 pkg

33e4b3c

omegabytes added 5 commits December 12, 2022 15:04

empty atc and node files

9798f25

remove commented code

8bd2cdc

remove debug traceback

a7b66b9

move new_tab and nkeys to kong.tools.utils

74a909b

remove node, add newlines

fc044a8

omegabytes force-pushed the fix/mocking branch 2 times, most recently from 990ddc1 to c82d5e7 Compare December 13, 2022 20:12

revert reports changes

cddf098

omegabytes force-pushed the fix/mocking branch from c82d5e7 to cddf098 Compare December 13, 2022 20:13

revert router.utils changes

821f5f7

update patchfile

4c25d00

omegabytes marked this pull request as ready for review December 14, 2022 20:37

omegabytes requested a review from a team as a code owner December 14, 2022 20:37

omegabytes requested review from javierguerragiraldez, mikefero and GGabriele December 14, 2022 20:39

omegabytes closed this Jan 5, 2023

omegabytes reopened this Jan 5, 2023

mikefero closed this Feb 7, 2023

mikefero deleted the fix/mocking branch February 7, 2023 13:15

mikefero restored the fix/mocking branch February 7, 2023 13:16

omegabytes deleted the fix/mocking branch April 13, 2023 18:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: required changes for 3.1 compatibility #36

fix: required changes for 3.1 compatibility #36

omegabytes commented Dec 9, 2022 •

edited

Loading

omegabytes Dec 9, 2022 •

edited

Loading

javierguerragiraldez Dec 9, 2022

javierguerragiraldez Dec 9, 2022

omegabytes Dec 10, 2022

omegabytes Dec 9, 2022 •

edited

Loading

javierguerragiraldez Dec 9, 2022

omegabytes Dec 9, 2022 •

edited

Loading

omegabytes Dec 13, 2022 •

edited

Loading

omegabytes Dec 9, 2022 •

edited

Loading

omegabytes Dec 9, 2022

javierguerragiraldez Dec 9, 2022

omegabytes Dec 12, 2022

omegabytes Dec 10, 2022

omegabytes commented Dec 10, 2022 •

edited

Loading

javierguerragiraldez commented Dec 12, 2022

GGabriele commented Dec 12, 2022

CLAassistant commented Dec 12, 2022 •

edited

Loading

omegabytes commented Dec 14, 2022

omegabytes commented Dec 14, 2022

omegabytes commented Dec 14, 2022

mikefero commented Jan 3, 2023

omegabytes commented Jan 5, 2023

GGabriele commented Jan 25, 2023

GGabriele commented Jan 26, 2023

mikefero commented Feb 7, 2023

		local decode_base64url = require "ngx.base64".decode_base64url
		local decode_base64url = ngx.decode_base64url

		local to_hex = require "resty.string".to_hex
		local to_hex = require "hex".to_hex

		l.PreloadModule("sha256", sha256.Loader)
		l.PreloadModule("hex", hex.Loader)

fix: required changes for 3.1 compatibility #36

fix: required changes for 3.1 compatibility #36

Conversation

omegabytes commented Dec 9, 2022 • edited Loading

omegabytes Dec 9, 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

omegabytes Dec 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omegabytes Dec 9, 2022 • edited Loading

Choose a reason for hiding this comment

omegabytes Dec 13, 2022 • edited Loading

Choose a reason for hiding this comment

omegabytes Dec 9, 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

omegabytes commented Dec 10, 2022 • edited Loading

javierguerragiraldez commented Dec 12, 2022

GGabriele commented Dec 12, 2022

CLAassistant commented Dec 12, 2022 • edited Loading

omegabytes commented Dec 14, 2022

omegabytes commented Dec 14, 2022

omegabytes commented Dec 14, 2022

mikefero commented Jan 3, 2023

omegabytes commented Jan 5, 2023

GGabriele commented Jan 25, 2023

GGabriele commented Jan 26, 2023

mikefero commented Feb 7, 2023

omegabytes commented Dec 9, 2022 •

edited

Loading

omegabytes Dec 9, 2022 •

edited

Loading

omegabytes Dec 9, 2022 •

edited

Loading

omegabytes Dec 9, 2022 •

edited

Loading

omegabytes Dec 13, 2022 •

edited

Loading

omegabytes Dec 9, 2022 •

edited

Loading

omegabytes commented Dec 10, 2022 •

edited

Loading

CLAassistant commented Dec 12, 2022 •

edited

Loading