refactor(core) expose API to obtain node_id #3234
Conversation
hishamhm
force-pushed
the
feat/node_id
branch
3 times, most recently
from
df81a9e to
1acbc5a
Compare
hishamhm
changed the title
| -- Obtain the unique node id for this node. | ||
| do | ||
| local node_id | ||
| function _M.get_node_id() |
There was a problem hiding this comment.
I'm a bit worried that we will see this utility being used in the init phase soon - and since we do nothing to guard against it, the issue would silently go unnoticed. This sort of issue has happened so many times in this project that it is one of the reasons behind my suggestion to put this in a controlled API if it is to be consumed publicly.
Until the proper API (with init and init_worker seeding and the proper initialization mechanisms are in place) - can we make this function throw an error if called in init?
if ngx.get_phase() == "init" then
error("API disabled in the context of init_by_lua", 2)
endThere was a problem hiding this comment.
A proper init function for the API would also avoid making this API return errors, and always succeed (since the shm dance is either moved to the initialization of the module, or removed altogether as discussed 1-1) - let's no go down that road yet though...
kong/api/routes/kong.lua
Outdated
| @@ -56,6 +57,7 @@ return { | |||
| tagline = tagline, | |||
| version = version, | |||
| hostname = utils.get_hostname(), | |||
| node_id = public.get_node_id(), | |||
There was a problem hiding this comment.
Should we handle the error here? Granted it is unlikely that get_node_id() will never be invoked before this particular call, but there is no safeguard against it if things ever change...
There was a problem hiding this comment.
I thought about that, but if we can't get a node_id here there's not much we can do other than just not returning the field, so returning nil naturally "erases" that field from the result (the only other thing that comes to mind would be to log the error; we could do that, but it seems overkill, and we don't for utils.get_hostname() anyway).
There was a problem hiding this comment.
True, but when raising this concern, I am mostly worried about writing reference code - another typical pattern we have seen in this project has been the copy-pasting of countless bits of Lua code around the project - spreading bad patterns all around the place...
There was a problem hiding this comment.
Knowing when to handle an error and when not to is not a bad pattern per se, but it takes experience -- so I see what you mean. Added the check here, no biggie.
another typical pattern we have seen in this project has been the copy-pasting of countless bits of Lua code around the project - spreading bad patterns all around the place...
Yeah, and ultimately, we should strive to reduce the copy-pasting rather than just providing people better bits to replicate. :)
There was a problem hiding this comment.
Yeah, and ultimately, we should strive to reduce the copy-pasting rather than just providing people better bits to replicate. :)
My struggle for the past 3 years :) Never did we get allocated time to actually work on this - glad we will soon
Move the unique identifier of the Kong node from the `kong.cluster_events` module into an internal value, accessible via `kong.tools.public.get_node_id()`. This way, the same unique identifier can be used for different features that need it, such as cluster events and identifying per-node data in health check reporting.
hishamhm
force-pushed
the
feat/node_id
branch
from
1acbc5a to
f56216c
Compare
This adds a `"node_id"` entry to the `/` status endpoint. This is a UUID generated on Kong start (each restart of Kong will result in a new `node_id`).
hishamhm
force-pushed
the
feat/node_id
branch
from
f56216c to
500b88f
Compare
Move the unique identifier of the Kong node from the
kong.cluster_eventsmodule into an internal value, accessible viakong.tools.public.get_node_id().This way, the same unique identifier can be used for different features that need it, such as cluster events and identifying per-node data in health check reporting.