Feat/wrpc

[javierguerragiraldez]

wRPC protocol in CP/DP communications

• connect via WebSockets, preserve existing authentication schemes.
• bidirectional RPC, passing data/args/results/errors in both directions.
• some error handling
• request/response/pending expiration
  • drop expired packets on reception
  • drop expired waiters on response
  • drop expired waiters that never receive response
• encapsulate receive thread within protocol management
• encapsulate send thread within protocol management, only for "server" sockets.
• send periodic PINGs
• detect and act on missing pings or pongs.
• send config data from CP to every DP on any change.
• handle hash or version of config data, to ignore unneeded config messages.
• transform config data to fit protobuf spec
  • wrap plugins config in Struct
  • allow foreign keys to be passed in expanded form
  • ensure "absent" fields don't get materialized with "default" or "empty" values on decoding.
  • ...
• define inter-worker communication channel
• permit calls from any worker
• pass existing hybrid mode tests

Note: there's still some code duplication between the existing (json) protocol and this. This is to ensure the feature flag (cluster_protocol=json|wrpc) doesn't activate any new code. Once the version negotiation feature (#8397 ) is merged in, this will be refactored on top of that and repeated code will be factored away.

[mikefero]

Looks great so far; some nits and suggestions for now; feel free to punt some of them during another iteration.

First pass; need to look over kong/tools/wrpc.lua more though.

[mikefero]

should we make the version a configuration item; conf.wrpc_version? This will allow us to switch between versions in the future either during development or a forced downgrade by the user.

[javierguerragiraldez]

a new protocol version would require new code. i think it's best that each version of the code hardcodes the appropriate version of the .proto; in fact, .proto files are part of the code, even if loaded dynamically instead of generating static code as on Go

[mikefero]

There is a lot of duplicate code; similar to the other control plane protocol variants. We may want to outside of this PR clean those up for maintenance purposes.

[mikefero]

Great work! Let's get this in and then iterate on the remaining items in your list and then we can focus on other issues as them come in. With this code being completely isolated from the JSON payload we will be in good shape.

[zhongchen]

the type should be int32 instead of double? it is defined as int32 in all other places.

[javierguerragiraldez]

i've forwarded the fix to the original repo (these .proto files are copied from there, ideally should be copied during build time and not on the repo. we'll get there soon)

[StarlightIbuki]

And maybe more refactoring(extracting common codes) also helps when implementing the version negotiation.

[javierguerragiraldez]

rebased on top of master

[StarlightIbuki]

It seems of no effect?

[javierguerragiraldez]

right. seems a leftover from debugging code

[StarlightIbuki]

Could we just use one dictionary key, and put the name of the channel together into the value? So we could fetch only once.
After all, post keeps blocked if NAME_KEY is not free.

[javierguerragiraldez]

this is one of the reasons to move to a socket implementation, it's hard to follow which concepts tie 1:1 into which and which are 1:n
in this case, i think the purpose was to reduce serialization of processes, each client pushes their value without blocking each other, and only block on making the "server" pull the notification. at higher level i think there's still the same contention anyway... but it's important to be able for each client to fire the message and not wait for all the others to be served.

[StarlightIbuki]

Could we just do this?

local id_assigner = {}
id_assigner.__index = id_assigner
setmetatable(id_assigner, id_assigner)

function id_assigner.new()
  return setmetatable({
    [0] = 0,
  }, id_assigner)
end

function id_assigner:get_id(name)
  if self[name] == nil then
    self[name] = self[0]
    self[0] = self[0] + 1
  end
  return self[name]
end

-- ....

function wrpc_service:add(service_name)
  local service_ids = id_assigner_new()
  local rpc_ids = {}
  local service_fname = assert(proto_searchpath(service_name))

  grpc.each_method(service_fname, function(_, srvc, mthd)
    assert(srvc.name)
    assert(mthd.name)
    local rpc_name = srvc.name .. "." .. mthd.name

    local service_id = service_ids:get_id(srvc.name)
    if rpc_ids[service_id] == nil then
      rpc_ids[service_id] = id_assigner_new()
    end
    local rpc_id = rpc_ids[service_id]:get_id(rpc_name)
    local rpc = {
      name = rpc_name,
      service_id = tonumber(service_id),
      rpc_id = tonumber(rpc_id),
      input_type = mthd.input_type,
      output_type = mthd.output_type,
    }
    self.methods[service_id .. ":" .. rpc_id] = rpc
    self.methods[rpc_name] = rpc
  end, true)
end

[javierguerragiraldez]

you mean dynamically assign ids to methods and rpcs? no, we must respect those in the .proto file

[StarlightIbuki]

tags seem not used

[javierguerragiraldez]

they're part of the annotation structure, it's put into type_annotations, which is a subtable of annotations. after scanning the file, it's read to get the service_id and rpc_id on the .each_method() loop

[StarlightIbuki]

It seems that all call to call and call_wait uses a table to contain arguments.

[javierguerragiraldez]

not sure what do you mean. the specific case i had in mind is the CP sending the same config to several DP nodes. instead of repeating the call with the unencoded config, it calls service:encode_args() once, and then peer:send_encoded_call() for each DP node

[StarlightIbuki]

I mean like this:

local resp, err = peer:call_wait("ConfigService.ReportMetadata", { plugins = self.plugins_list })
assert(peer:call("ConfigService.PingCP", { hash = hash }))

This comment is related to the comment below.

[StarlightIbuki]

Encode every arguments with the same input_type seems a bug.

[javierguerragiraldez]

the RPC definitions in .proto files only specify one input type. still, the frame structure supports repeated payloads. a single call could specify several items of the same time instead of using an array.

[StarlightIbuki]

Maybe I misinterpreted the comment in the wrpc.proto:

  // payloads is an array representing the request or response data of an RPC.
  // A request message MAY contain multiple elements where each element represents
  // an argument to the RPC call. The order of the elements MUST correspond to
  // the order of arguments in the function call.
  // A response message MUST contain a single payload.
  // Use a wrapper type for RPCs which contain multiple responses.
  // Unless otherwise specified by the Service or RPC, the encoding method in
  // use is PROTO3.
  //
  // Note: This results in double proto3 encoding. Once encoding of payloads,
  // and then encoding of the entire message. We acknowledge that there are
  // some overheads here and they will be addressed in a future iteration.
  repeated bytes payloads = 9;

By "each element represents an argument" I thought it was for every member of the request message (like "hash" and "plugins" in the previous comment).
Also, if an RPC call receives repeated messages, it cannot use repeat like this:

  rpc SomeCall(repeated SomeCallRequest) returns (SomeCallResponse);

But to use something like:

Message RepeatedSomeCallRequest {
    repeated SomeCallRequest arguments = 1;
}

So I don't think it makes sense to encode an array of payloads...

[javierguerragiraldez]

Also, if an RPC call receives repeated messages, it cannot use repeat like this:

rpc SomeCall(repeated SomeCallRequest) returns (SomeCallResponse);

right, the syntax doesn't support this. but in the current implementation, the "repeated" is implied, so this could be done:

rpc SomeCall(SomeCallRequest) returns (someCallResponse);

message SomeCallRequest {
    string name = 1;
    string value = 2;
}

and in Lua:

   peer:call("serv.SomeCall", {name="one", value=1}, {name="two", value=2}, {name="three", value=3})

and it would result in a single call with three payload elements of the same type.

But I agree that it's not an important usecase, and the structure you describe would be just as good.

[StarlightIbuki]

@hbagdi May you clarify this? Do we really want to implement it this way?