fix: add check for node name duplication#4
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds validation to prevent duplicate node names when generating cluster join tokens. When a join token is requested for a node name that already exists in the cluster, the API now returns an error instead of allowing the duplicate.
Changes:
- Added a check in the join token creation endpoint to verify node name uniqueness against existing cluster nodes
- Created comprehensive test coverage for the duplicate node name validation scenario
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/k8sd/api/cluster_tokens.go | Added Kubernetes client check to validate node name doesn't already exist before creating join token |
| pkg/k8sd/api/cluster_tokens_test.go | Added test case and mock provider implementation to verify duplicate node name detection |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
louiseschmidtgen
left a comment
There was a problem hiding this comment.
Nice work Ethan! Thanks for following up on the previous PR with this token check.
|
Please also add what this PR solves and how to the PR description. |
|
Thanks for the review @louiseschmidtgen! I've added your requests to the PR |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
louiseschmidtgen
left a comment
There was a problem hiding this comment.
Thank you for your work Ethan!
|
|
||
| // getOrCreateJoinTokenFn exists to allow tests to stub out microcluster join-token | ||
| // creation without having to spin up a real microcluster instance. | ||
| var getOrCreateJoinTokenFn = getOrCreateJoinToken |
There was a problem hiding this comment.
Let's try and avoid something like this.
Can we rather move the check into a function and test it?
It would also be nice to have a full e2e test to validate Microcluster's behaviour for these scenarios.
There was a problem hiding this comment.
I refactored the check to be outside the method which made unit testing easier. As for e2e tests, they're in the k8s-snap repo so I can have those run once this PR is merged in
louiseschmidtgen
left a comment
There was a problem hiding this comment.
Two small nits for the sake of improving the code quality from great to excellent:
I don't see any complaints? Maybe I'm not looking in the right place but no complains are shown in the files changed section outside of your comments |
louiseschmidtgen
left a comment
There was a problem hiding this comment.
LGTM, please address my little comment and then feel free to merge.
Thank you for the hard work Ethan!
added a check in the get-join-token command to verify that there are no other nodes (CP or worker) that share the same name before providing the token. This way, microcluster and kubernetes have internally consistent stores of node data.
This is split off from this PR (see for context on issue: canonical/k8s-snap#2263 ) which was created before k8sd was moved into this repo
Currently, if a CP node attempts to join the cluster with the same name as an existing worker node, it will fail in Kubernetes but persist in the microcluster, creating a mismatch in data. This PR adds a check in the get-join-token command to verify that there are no other nodes (CP or worker) that share the same name before providing the token. This way, microcluster and kubernetes have internally consistent stores of node data.