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.

Sign up for GitHub

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

Jump to bottom

chore(*) Bundling proto files inside the luarock #8918

Closed
wants to merge 1 commit into from
Closed

chore(*) Bundling proto files inside the luarock #8918

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .requirements
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,4 @@ RESTY_EVENTS_VERSION=0.1.0
LIBYAML_VERSION=0.2.5
KONG_BUILD_TOOLS_VERSION=4.30.0
KONG_NGINX_MODULE_BRANCH=0.2.1
PROTOBUF_VERSION=3.19.0
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depends on Kong/kong-build-tools#484

29 changes: 28 additions & 1 deletion kong-2.8.0-0.rockspec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -500,5 +500,32 @@ build = {

["kong.tracing.instrumentation"] = "kong/tracing/instrumentation.lua",
["kong.tracing.propagation"] = "kong/tracing/propagation.lua",
}
},
install = {
lua = {
-- Proto files. We fake that they are lua modules so they will be included in the luarock, but don't require them.
-- Every '.' character is turned into a directory seperator by luarocks so we cannot have 'name.proto', it must be 'name_proto'
-- to match the original path when bundled into the luarock
["kong.include.opentelemetry.proto.trace.v1.trace_proto"] = "kong/include/opentelemetry/proto/trace/v1/trace.proto",
["kong.include.opentelemetry.proto.collector.trace.v1.trace_service_proto"] = "kong/include/opentelemetry/proto/collector/trace/v1/trace_service.proto",
["kong.include.opentelemetry.proto.common.v1.common_proto"] = "kong/include/opentelemetry/proto/common/v1/common.proto",
["kong.include.opentelemetry.proto.resource.v1.resource_proto"] = "kong/include/opentelemetry/proto/resource/v1/resource.proto",
["kong.include.kong.pluginsocket_proto"] = "kong/include/kong/pluginsocket.proto",
["kong.include.kong.model.plugin_entities_proto"] = "kong/include/kong/model/plugin_entities.proto",
["kong.include.kong.model.route_proto"] = "kong/include/kong/model/route.proto",
["kong.include.kong.model.plugin_proto"] = "kong/include/kong/model/plugin.proto",
["kong.include.kong.model.consumer_proto"] = "kong/include/kong/model/consumer.proto",
["kong.include.kong.model.sni_proto"] = "kong/include/kong/model/sni.proto",
["kong.include.kong.model.ca_certificate_proto"] = "kong/include/kong/model/ca_certificate.proto",
["kong.include.kong.model.service_proto"] = "kong/include/kong/model/service.proto",
["kong.include.kong.model.upstream_proto"] = "kong/include/kong/model/upstream.proto",
["kong.include.kong.model.parameter_proto"] = "kong/include/kong/model/parameter.proto",
["kong.include.kong.model.certificate_proto"] = "kong/include/kong/model/certificate.proto",
["kong.include.kong.model.target_proto"] = "kong/include/kong/model/target.proto",
["kong.include.kong.model.workspace_proto"] = "kong/include/kong/model/workspace.proto",
["kong.include.kong.model.config_proto"] = "kong/include/kong/model/config.proto",
["kong.include.kong.services.config.v1.config_proto"] = "kong/include/kong/services/config/v1/config.proto",
["kong.include.wrpc.wrpc_proto"] = "kong/include/wrpc/wrpc.proto",
kikito marked this conversation as resolved.
Show resolved Hide resolved
}
},
}
9 changes: 8 additions & 1 deletion kong/tools/grpc.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ local protoc = require "protoc"
local pb = require "pb"
local pl_path = require "pl.path"
local date = require "date"
local loader = require("luarocks.loader")
kikito marked this conversation as resolved.
Show resolved Hide resolved

local bpack = lpack.pack
local bunpack = lpack.unpack
Expand Down Expand Up @@ -65,9 +66,15 @@ end

function _M.new()
local protoc_instance = protoc.new()
-- Look for the kong/init.lua file because the 'include' folder is a sibling of that.
-- Currently there is not a luarock function to 'give me the root of a luarock', onl
-- 'give me the path to a module in a luarock'.
local kong_init_file, _, _ = loader.which("kong")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

loader.which returns the path to a specified module

local kong_include_dir = kong_init_file:gsub("init%.lua$", "include")
Copy link
Member

@kikito kikito Jun 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overprotective code is not my cup of tea, but this worries me. Two concerns:

  • The GRPC module shouldn't "know" about kong internal structure (like that our "main luarock uses a file called init.lua")
  • If we for some reason decide to rename init.lua to something else (unlikely, but it might happen if we decide to change how we load modules in the far future), this will fail with an obscure message.

My first impulse would be moving this logic to a function in kong.tools.utils.get_include_module and also write a test for it, so it breaks the tests if we change things. However I believe the grpc module gets required before utils, so this might not be possible. I don't know where else I would put this function. Kong.global or even kong.constants, perhaps? The path shouldn't change once kong starts after all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • The GRPC module needs to know that from the root of the installed luarock (or source code directory), there is a folder named kong/include that stores all the proto files to be loaded. I agree that looking for kong/init.lua isn't the best approach but there isn't a luarock API right now for "give me the install root of a rock", only "give me the path to a file in a luarock").
  • I'm against moving tiny pieces of code that are only used once into utility modules. I think that just obfuscates the code and makes it harder to read. I did add a new comment here to (hopefully) better explain why I am calling the current API.
  • I think if we renamed kong/init.lua the tests I wrote would break trying to call :gsub on a nil table. I'm open to adding an assert with a better message like assert(kong_init_file, "Could not find the 'init.lua' file in the root of the kong luarock"). Thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's go with the assert then.

Suggested change
local kong_include_dir = kong_init_file:gsub("init%.lua$", "include")
local kong_include_dir = kong_init_file:gsub("init%.lua$", "include")
assert(kong_include_dir, "Could not find the 'init.lua' file in the root of the kong luarock")

assert(kong_include_dir)
-- order by priority
for _, v in ipairs {
"/usr/local/kong/include",
kong_include_dir,
"/usr/local/opt/protobuf/include/", -- homebrew
"/usr/include",
"kong/include",
Expand Down
18 changes: 18 additions & 0 deletions spec/01-unit/22-grpc-utils_spec.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,4 +42,22 @@ describe("grpc tools", function()
"Added.Final",
}, methods)
end)

it("includes kong/include in the load path", function()
local grpc_tools_instance = grpc_tools.new()
assert.matches("kong/include$", grpc_tools_instance.protoc_instance.paths[3])
end)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a test to load the .proto file with pure luarocks loader.

Suggested change
end)
end)
it("loads .proto files with luarocks loader", function()
local loader = require "luarocks.loader"
local kong_init_file, _, _ = loader.which("kong")
local kong_include_dir = kong_init_file:gsub("init%.lua$", "include")
local protoc = require "protoc"
local p = protoc.new()
p.include_imports = true
p:addpath(kong_include_dir)
assert(p:loadfile("opentelemetry/proto/collector/trace/v1/trace_service.proto"))
end)


-- duplicates how we load .proto files so we have a finer grain test pointing out if that breaks
it("loads .proto files with luarocks loader", function()
local loader = require "luarocks.loader"
local kong_init_file, _, _ = loader.which("kong")
local kong_include_dir = kong_init_file:gsub("init%.lua$", "include")

local protoc = require "protoc"
local p = protoc.new()
p.include_imports = true
p:addpath(kong_include_dir)
assert(p:loadfile("opentelemetry/proto/collector/trace/v1/trace_service.proto"))
end)
end)