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

feat(conf) implement 'node_id' for configurable node ID #10385

Merged
merged 1 commit into from
Mar 29, 2023
Merged

Conversation

hbagdi
Copy link
Member

@hbagdi hbagdi commented Feb 27, 2023

Summary

Kong automatically generates node IDs for each node in a cluster. Node IDs are the only unique identifiers that exist to track a Kong node within a cluster (no assumption is made that hostname is unique across all Kong nodes).

When integrating closely with an orchestrator, a configurable node ID helps the operator to source the ID from the underlying orchestrator like Kubernetes instead of letting Kong generate one.

For most customers, letting Kong generate a Node ID will be sufficient.

Checklist

  • The Pull Request has tests
  • There's an entry in the CHANGELOG
  • There is a user-facing docs PR - Not applicable

Full changelog

  • introduce node_id configuration parameter

Copy link
Contributor

@mrwanny mrwanny left a comment

Choose a reason for hiding this comment

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

LGTM

kong.conf.default Outdated Show resolved Hide resolved
kong.conf.default Outdated Show resolved Hide resolved
Copy link
Member

@gszr gszr left a comment

Choose a reason for hiding this comment

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

LGTM. Please rebase so I can merge.

@hbagdi
Copy link
Member Author

hbagdi commented Feb 28, 2023

@gszr Rebased. Thank you for the prompt review.

@gszr
Copy link
Member

gszr commented Feb 28, 2023

At first glance test failures do not look related; I have logged the failure and restarted the job.

@hbagdi
Copy link
Member Author

hbagdi commented Feb 28, 2023

Tests seem to be consistently failing but are not related.

@gszr
Copy link
Member

gszr commented Feb 28, 2023

We will need to dig further. @hbagdi can you please run some local testing comparing your branch to master on those specific failing tests?

@hbagdi hbagdi force-pushed the feat/node-id branch 2 times, most recently from aab9a61 to adb2179 Compare March 2, 2023 21:02
@@ -1845,3 +1845,7 @@
# behavior and explicitly instructs Kong which
# OpenResty installation to use.

#node_id = # Node ID for the Kong node. Every Kong node
# in a Kong cluster must have a unique and
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# in a Kong cluster must have a unique and
# in a Kong cluster must have an unique and

Copy link
Member Author

Choose a reason for hiding this comment

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

kong/pdk/node.lua Outdated Show resolved Hide resolved
@hbagdi hbagdi force-pushed the feat/node-id branch 3 times, most recently from 5d55be2 to 264659e Compare March 23, 2023 19:51
@hbagdi hbagdi force-pushed the feat/node-id branch 2 times, most recently from f52a24e to 368f8d2 Compare March 23, 2023 20:47
@hbagdi
Copy link
Member Author

hbagdi commented Mar 23, 2023

@gszr @chronolaw This should be ready for another review.

@hbagdi hbagdi self-assigned this Mar 24, 2023
kong/pdk/node.lua Show resolved Hide resolved
end
end
-- 3. generate a new id
if not node_id then
_NODE.get_id()
Copy link
Member

Choose a reason for hiding this comment

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

i think this can be preserved in the else branch above, so the new id is logged, as it was

Copy link
Member Author

Choose a reason for hiding this comment

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

I did this to improve the readability of the code a bit. Is logging the new ID necessary?
If so, I can add another log at the end of these conditional blocks to log it unconditionally.

Copy link
Member

Choose a reason for hiding this comment

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

IMO it's questionable that it achieves the goal of improving readability -- it introduces an unnecessary conditional. But let's not bikeshed over this; I can approve / merge if you are adamant on the change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's keep it as is and move on.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a log at INFO level for node-id.

Kong automatically generates node IDs for each node in a cluster.
Node IDs are the only unique identifiers that exist to track a Kong node
within a cluster (no assumption is made that hostname is unique across
all Kong nodes).

When integrating closely with an orchestrator, a configurable node ID
helps the operator to source the ID from the underlying orchestrator
like kubernetes instead of letting Kong generating one.

For most customers, letting Kong generate a Node ID will be sufficient.
@gszr gszr merged commit ec33964 into master Mar 29, 2023
@gszr gszr deleted the feat/node-id branch March 29, 2023 20:49
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.

None yet

5 participants