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

fix(pluginsocket.proto): type for set_upstream #12727

Merged
merged 1 commit into from
Mar 13, 2024
Merged

fix(pluginsocket.proto): type for set_upstream #12727

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
3 changes: 3 additions & 0 deletions changelog/unreleased/kong/pluginsocket-proto-wrong-type.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
message: |
Fix an issue where external plugins using the protobuf-based protocol would fail to call the `kong.Service.SetUpstream` method with an error `bad argument #2 to 'encode' (table expected, got boolean)`.
type: bugfix
2 changes: 1 addition & 1 deletion kong/include/kong/pluginsocket.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,7 @@ service Kong {
rpc Router_GetRoute(google.protobuf.Empty) returns (Route);
rpc Router_GetService(google.protobuf.Empty) returns (Service);

rpc Service_SetUpstream(String) returns (google.protobuf.Empty);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it work with only Bool ? The general PDK says that there are 2 values returned: (bool, string) - https://docs.konghq.com/gateway/latest/plugin-development/pdk/kong.service/#kongserviceset_upstreamhost

Copy link
Member Author

@gszr gszr Mar 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Good question!
I have verified the change works for both cases.

Longer answer:
Although it fixes the behavior described in Kong/go-pdk#114, the go-pdk does not currently handle consistently the nil, err protocol adopted in the Lua PDK -- ie, most methods do not return the err on the Go PDK side (only other errors are reported, like connection errors, protobuf errors, etc). Also, on the Go PDK side the SetUpstream method does not return a boolean, only an error. Fixing this means an API breaking change has to be introduced, so this really needs to be done before we release v1.0.0, but not in the scope of this fix. I will add this to my future improvements notes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, ok! That makes sense. Thanks 👍 !

rpc Service_SetUpstream(String) returns (Bool);
rpc Service_SetTarget(Target) returns (google.protobuf.Empty);

rpc Service_Request_SetScheme(String) returns (google.protobuf.Empty);
Expand Down
Loading