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

Fix/luaproto encode hooks #8297

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

git-torrent
Copy link
Contributor

@git-torrent git-torrent commented Jan 15, 2022

Summary

Transcoding between GRPC and JSON requires some types to be handled exceptionally.

Hooks provided by lua-protobuf are insufficient as they:

  1. cannot hook native types (required for bytes)
  2. are not executed recursively (required for Any)

Further, gateway must read custom JSON names from proto definitions. Conversion must also allow to keep camel_case names, enum names (instead of numbers) and to emit also JSON with default values.

Full changelog

  • added file plugins/grpc-gateway/protojson.lua handling proto->JSON formatting
  • updated rockspecs to include this new file

In tools/grpc.lua:

  • refactored to be more readable
  • add_path and parse_file methods are now exposed instead of for_each_method
  • extra hook on every field of every message included (to extract json_name option value)
  • path( "spec/fixtures/grpc" ) excluded from internally added paths as they are only intended for testing and might contain files conflicting with user code. Mixing testing code with production code is bad practice anyway.

in plugins/grpc-gateway/deco.lua

  • removed code from ancient times where include_imports directive was false.
  • adapted to new tools/grpc.lua
  • decoding/encoding of messages is now done by using protojson.lua

in plugins/grpc-web

  • adapted to new tools/grpc.lua
  • no functional changes have been made
  • schema extended to allow also additional files (useful for google.protobuf.Any conversions)
  • schema allows to add also additional import paths (useful for libraries)

Also fixed bug when shorter binding path matched longer one (e.g. /test matched also /test/something/else).

Issue reference

Fix #7469

KAG-2262(internal ticket)

Todo: more test coverage

@CLAassistant
Copy link

CLAassistant commented Jan 15, 2022

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Pavol Ostertag seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@git-torrent
Copy link
Contributor Author

@git-torrent I underestimated the amount of work and am sorry for failing to keep the promise. 3.5 is filled with other features and for personal reasons, I was off for nearly 2 weeks. This improvement cannot be fit in a gap and has to be planned. I'm trying to make this a part of 3.6.

Can I be of any help? I would still gladly contribute to unit-tests (if I know how to do them :) )

@StarlightIbuki
Copy link
Contributor

@git-torrent I underestimated the amount of work and am sorry for failing to keep the promise. 3.5 is filled with other features and for personal reasons, I was off for nearly 2 weeks. This improvement cannot be fit in a gap and has to be planned. I'm trying to make this a part of 3.6.

Can I be of any help? I would still gladly contribute to unit-tests (if I know how to do them :) )

We want to rework the PR and before the implementation is written we don't know how to unit test them. The best help we could expect from you should be a list of the improvements that we want in detail like the list we made before. To be honest, I don't fully understand the PR's intention, and the list I made is not very in-depth.

@git-torrent
Copy link
Contributor Author

git-torrent commented Oct 19, 2023

We want to rework the PR and before the implementation is written we don't know how to unit test them. The best help we could expect from you should be a list of the improvements that we want in detail like the list we made before. To be honest, I don't fully understand the PR's intention, and the list I made is not very in-depth.

Google provides automated GRPC-REST translation in their own API gateway solution. Kong provides its own, but lacks many of the translation features required by Google. The goal of this PR is to bring google-compliant translation into Kong.

For this purpose Google offers several libraries (go, c++, C#, python), but none for lua. protojson.lua in PR is this missing library for Lua (personally crafted :) ). Maybe it might easier to replace it by a dll (made of official Google C++ code) loaded into Lua by LuaBridge/LuaBind.

The rest is to use protojson library in grpc-gateway plugin.

Does this bring more light into this PR?

Pavol Ostertag added 5 commits October 23, 2023 21:05
fix: deco.lua extra newline at the end

deco.lua: remove unnecessary whitespaces

deco.lua: fix uneccessary table creation

deco.lua: unused parameter of load_json_names_info replaced with _

deco.lua: fix metatable of upstream payload

deco.lua: adopt early return style

opentelemetry: fix casual style of grpc instantiation

protojson.lua: move to tools

tools/grpc: rename parse_file() to traverse_proto_file()

Update kong/plugins/grpc-gateway/deco.lua

Co-authored-by: Xumin <[email protected]>

Update spec/01-unit/22-grpc-utils_spec.lua

Remove indent

Co-authored-by: Xumin <[email protected]>

deco.lua: remove extra spaces

fix: conflict cleanup

fix: add extra line at the end of tools/protojson.lua

add: grpc-gateway schema changes to removed_fields.lua
fix: return to good old optimization

fix: grow tail test moved upward to emit_defaults=true zone

add: default are emitted by default in protojson
@StarlightIbuki
Copy link
Contributor

StarlightIbuki commented Nov 3, 2023

@git-torrent 3.5 is delayed and we have planned so many things for 3.6. I see little hope that we can fit this improvement into the foreseeable future... In fact, we have many other possible improvements in the queue. Sorry again for this message.
I reviewed this fix and have some personal views. IMO, your effort is worth a dedicated repository. It may be easier to do as a separate functionality instead of a part of Kong.
It could be a set of functions or a module (to_JSON, from_JSON?) of lua-protobuf, or an extension to it (not necessarily an official part of the lib). That way it should also be easier to unit test (and you don't need to follow the Kong's conversion to write the tests). Then from Kong, the PR should be much smaller and easier to review.

@git-torrent
Copy link
Contributor Author

@git-torrent 3.5 is delayed and we have planned so many things for 3.6. I see little hope that we can fit this improvement into the foreseeable future... In fact, we have many other possible improvements in the queue. Sorry again for this message. I reviewed this fix and have some personal views. IMO, your effort is worth a dedicated repository. It may be easier to do as a separate functionality instead of a part of Kong. It could be a set of functions or a module (to_JSON, from_JSON?) of lua-protobuf, or an extension to it (not necessarily an official part of the lib). That way it should also be easier to unit test (and you don't need to follow the Kong's conversion to write the tests). Then from Kong, the PR should be much smaller and easier to review.

Oh, that could be. Is there any guide to write modules or extension to lua-protobuf? Or some example on the web (google did not help me much :) ). I am not the C guy, so I have to learn that first.

@StarlightIbuki
Copy link
Contributor

@git-torrent 3.5 is delayed and we have planned so many things for 3.6. I see little hope that we can fit this improvement into the foreseeable future... In fact, we have many other possible improvements in the queue. Sorry again for this message. I reviewed this fix and have some personal views. IMO, your effort is worth a dedicated repository. It may be easier to do as a separate functionality instead of a part of Kong. It could be a set of functions or a module (to_JSON, from_JSON?) of lua-protobuf, or an extension to it (not necessarily an official part of the lib). That way it should also be easier to unit test (and you don't need to follow the Kong's conversion to write the tests). Then from Kong, the PR should be much smaller and easier to review.

Oh, that could be. Is there any guide to write modules or extension to lua-protobuf? Or some example on the web (google did not help me much :) ). I am not the C guy, so I have to learn that first.

By module, I mean "Lua module". There's no such mechanism of lua-protobuf to support plugins.
To begin with, I guess you could request some features from the library maintainer, like custom handlers for build-in types or even direct fixes to those build-in types' behavior (with an option).
You could write your module in Lua to handle the proto file importing, and if the features you requested do not make it into the library, you could wrap over the original library, or make some hooks/monkey patches on it.

@StarlightIbuki
Copy link
Contributor

And in my very personal POV, there's no need to write such a library in C given LuaJIT emits quite efficient binary and cooperates with C types well.
If you still want to PR to lua-protobuf directly, you could start with Lua C APIs: https://www.lua.org/manual/5.1/manual.html#lua_Alloc

@git-torrent
Copy link
Contributor Author

@git-torrent 3.5 is delayed and we have planned so many things for 3.6. I see little hope that we can fit this improvement into the foreseeable future... In fact, we have many other possible improvements in the queue. Sorry again for this message. I reviewed this fix and have some personal views. IMO, your effort is worth a dedicated repository. It may be easier to do as a separate functionality instead of a part of Kong. It could be a set of functions or a module (to_JSON, from_JSON?) of lua-protobuf, or an extension to it (not necessarily an official part of the lib). That way it should also be easier to unit test (and you don't need to follow the Kong's conversion to write the tests). Then from Kong, the PR should be much smaller and easier to review.

Oh, that could be. Is there any guide to write modules or extension to lua-protobuf? Or some example on the web (google did not help me much :) ). I am not the C guy, so I have to learn that first.

By module, I mean "Lua module". There's no such mechanism of lua-protobuf to support plugins. To begin with, I guess you could request some features from the library maintainer, like custom handlers for build-in types or even direct fixes to those build-in types' behavior (with an option). You could write your module in Lua to handle the proto file importing, and if the features you requested do not make it into the library, you could wrap over the original library, or make some hooks/monkey patches on it.

Ok, let's write it in Lua. Do you suggest I should agree with author of lua-protobuf to add here https://github.com/starwing/lua-protobuf a protojson.lua file that does the job for us (and required protoc.lua?).

Btw, I've already reported a missing support for hooking data-types and it was rejected as too laborious. I would not expect anything more :( (it seems nobody wants to be compatible with google).

@StarlightIbuki
Copy link
Contributor

Do you suggest I should agree with author of lua-protobuf to add here starwing/lua-protobuf a protojson.lua file that does the job for us (and required protoc.lua?).

That would be ideal. If they do not like the idea, you could try make it a library which imports lua-protobuf.

Btw, I've already reported a missing support for hooking data-types and it was rejected as too laborious. I would not expect anything more :( (it seems nobody wants to be compatible with google).

Lua is not a mainstream user of the protobuf and gRPC and people seem to tolerate those glitches in their use. After all, with some workarounds, they can still use it.

@quantverse
Copy link

Hi, any chance this PR will solve the issue where int64 types are transcoded to JSON like floats with lost digits? According to the specs these should be transcoded as strings.

@git-torrent
Copy link
Contributor Author

Hi, any chance this PR will solve the issue where int64 types are transcoded to JSON like floats with lost digits? According to the specs these should be transcoded as strings.

yes, this PR solves this by enabling int64_as_string option https://github.com/starwing/lua-protobuf?tab=readme-ov-file#options

@quantverse
Copy link

Great news! Any chance this PR will be merged soon? Without this fix, the gRPC-gateway plugin is not really much usable for anyone who uses 64-bit integers like identifiers...

I think I will try to rebase this PR against the latest master and test if it works as expected...

@quantverse
Copy link

FYI I rebased that branch onto the current master and it builds fine, unfortunately, the int64 types don't really transcode well - they are now wrapped as strings but still processed as floats.

1718291265706240595 becomes "1.7182912657062e+18"

but almost there!

@quantverse
Copy link

quantverse commented Jun 13, 2024

Made it working if anyone is interested...

diff --git a/kong/tools/protojson.lua b/kong/tools/protojson.lua
--- a/kong/tools/protojson.lua	(revision 2ea36c87bb42c0a2555f8c9f791baf9654de2c97)
+++ b/kong/tools/protojson.lua	(date 1718294556881)
@@ -243,13 +243,12 @@
   elseif ( f_type == "bytes" ) then
     return ngx.encode_base64( f ) 
   elseif ( is_int[ f_type ] ) then -- parse numeric value
-    if ( type(f) ~= "number" or f ~= f or f ~= math.floor( f ) ) then -- possibly remove
-      error( ("invalid integer value at %s, got: %s"):format(path, f), 0 )
-    end
-
     if is_int64[ f_type ] then
       return string.gsub(f, "^#", "")
     else
+      if ( type(f) ~= "number" or f ~= f or f ~= math.floor( f ) ) then -- possibly remove
+        error( ("invalid integer value at %s, got: %s"):format(path, f), 0 )
+      end
       return f
     end
   elseif ( is_decimal[ f_type ] ) then
@@ -585,7 +584,7 @@
     pb.option( "enum_as_value" )
   end
 
-  pb.option( "int64_as_number" )
+  pb.option( "int64_as_string" )
 
   return message_to_json( pb.decode( msg_type, msg ), msg_type, path )
 end

@quantverse
Copy link

quantverse commented Jun 14, 2024

Unfortunately, there is another problem in the opposite direction (JSON to proto for instance when transcoding a POST body) - 64-bit integers are transcoded wrong without warning!

An example: 1718291265706240595 (or "1718291265706240595" passed as a string, does not matter) in JSON is transcoded to 1718291265706240512 at the proto side (note the last two digits) - there is apparently some precision loss during the transcoding process...

This is especially dangerous if this is an ID of something because these IDs get silently modified without any warning. Troubleshooting this could be quite a nightmare.

Any idea where to look to fix this one?

@git-torrent
Copy link
Contributor Author

Thank you for your input. I am leaving for a short vacation, let me fix it upon arrival.

@quantverse
Copy link

Any ETA for the fix?

@quantverse
Copy link

Related issues/PRs to the problem I have reported above:

mpx/lua-cjson#37
mpx/lua-cjson#36

@StarlightIbuki
Copy link
Contributor

StarlightIbuki commented Sep 12, 2024

A good news is that we are working on https://github.com/Kong/lua-resty-simdjson, which may solve the issues this PR aims to solve
Sorry for the misleading information, the simdJSON library is not applied to the json-protobuf decoding/encoding yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants