-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
chore(cluster) head request to check wrpc endpoint #8834
Conversation
this has the same issue that forced us to move version negotiation as a wRPC service and not as a HTTP request: the websocket and http client libraries won't use the same connection, so there's no guarantee both will go to the same server. PR #8775 implements the inline negotiation and does the equivalent of this PR: "try the wRPC connection and fall back to old protocol if it can't connect" |
@javierguerragiraldez What is the advantage of "keeping the connection on the same backend server". It seems to be very fragile and I don't know why would anyone want to deploy their CP cluster this way. |
Otherwise it requires shared state even before establishing the connection, as the CP has to carry the negotiation result from one event to another. If there's no guarantee that it's the same CP node, it has to be in a database. When it's part of the wRPC itself, on the other hand, it seamlessly activates the right services (and version) for the connection, without any convoluted state management. |
5717f2f
to
65ab467
Compare
65ab467
to
df6da6b
Compare
kong/clustering/init.lua
Outdated
--- detect '/v1/wrpc' endpoint | ||
--- if there is no '/v1/wrpc', fallback to websocket + json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This documentation is not 100% accurate, it describes the goal, but not what this function does.
--- detect '/v1/wrpc' endpoint | |
--- if there is no '/v1/wrpc', fallback to websocket + json | |
--- Return the highest supported Hybrid mode protocol version. |
Also, does this needs to be inside init.lua
? The CP shouldn't need this functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could it be moved to wrpc_data_plane.lua
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is not suitable for wrpc_data_plane.lua
, because at this point we have not decided which module will be required.
kong/clustering/init.lua
Outdated
return "v1" -- wrpc | ||
end | ||
|
||
|
||
function _M:request_version_negotiation() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed with @chronolaw offline, this should probably be removed as the name is misleading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
… into chore/test_version_negotiation
Summary
We do not use
/version-handshake
endpoint, just send a simpleHEAD
request to/v1/wrpc
,If it works then we use wrpc, else we fall back to websocket + json.
If this PR is merged, I will remove the code and test case of
version_negotiation
.Now I just keep it for compatibility.
Full changelog
head_request_version
inversion_negotiation/init.lua
request_version_negotiation
init_worker