move logic for validating node names #2127
Conversation
this commits moves the generation of "given names" of nodes into the registration function, and adds validation of renames to RenameNode using the same logic. Fixes juanfont#2121 Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>
WalkthroughThe changes involve the removal of the Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (3)
hscontrol/grpcv1.go (1)
Impact of Removing
GivenNameAssignment:The
GivenNamefield is referenced in multiple parts of the codebase, including tests and CLI operations. Its removal from theDebugCreateNodefunction could potentially break tests or affect CLI functionality. The presence of tests that assert theGivenNamesuggests that it is an expected part of the node's attributes and likely used for identification or display purposes.
- Integration Tests: The
GivenNameis used in assertions inintegration/cli_test.go.- Node Tests: The
GivenNameis referenced inhscontrol/types/node_test.go.- CLI Commands: The
GivenNameis used incmd/headscale/cli/routes.goandcmd/headscale/cli/nodes.go.Please ensure that removing the
GivenNameassignment does not break any functionality or negatively impact the user experience. Consider reviewing the tests and CLI commands that rely onGivenNameto confirm that they still function correctly.Analysis chain
Line range hint
1-1000: Verify the impact of removing theGivenNameassignment.The removal of the
givenNamevariable and its assignment suggests a simplification of the node creation process. However, this change could lead to a loss of functionality if theGivenNamewas critical for node identification or user interaction.Please verify that removing the
GivenNameassignment does not break any functionality or negatively impact the user experience. Consider running the following script to search for any references toGivenNamein the codebase:Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for references to `GivenName` in the codebase. # Test: Search for `GivenName` in Go files. # Expect: No critical references to `GivenName`. rg --type go 'GivenName'Length of output: 6112
hscontrol/db/node.go (2)
649-656: LGTM, but fix the typo in the function name.The new
isUnqiueNamefunction looks good:
- The function correctly queries the database to check for uniqueness.
- The function correctly returns
trueif no nodes are found andfalseotherwise.- The function correctly handles the error returned by the database query.
However, there is a typo in the function name. Apply this diff to fix it:
-func isUnqiueName(tx *gorm.DB, name string) (bool, error) { +func isUniqueName(tx *gorm.DB, name string) (bool, error) {
Line range hint
659-681: LGTM, but update the function to use the fixed function name.The new
ensureUniqueGivenNamefunction looks good:
- The function correctly generates a unique name for a node.
- The function correctly handles the error returned by
generateGivenName.- The function correctly handles the error returned by
isUnqiueName.- The function correctly returns the generated name.
However, the function should be updated to use the fixed function name
isUniqueName. Apply this diff to fix it:- unique, err := isUnqiueName(tx, givenName) + unique, err := isUniqueName(tx, givenName)
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (4)
- hscontrol/auth.go (3 hunks)
- hscontrol/db/node.go (5 hunks)
- hscontrol/db/node_test.go (3 hunks)
- hscontrol/grpcv1.go (2 hunks)
Additional comments not posted (6)
hscontrol/auth.go (1)
69-69: LGTM!The change to set up logging for the
handleRegisterfunction using thelogAuthFunchelper function is approved.hscontrol/grpcv1.go (1)
376-376: LGTM!The change to wrap
request.GetNodeId()withtypes.NodeID()is a good move towards a more type-safe approach. It enhances clarity and ensures that the function receives the correct type, potentially reducing runtime errors related to type mismatches.hscontrol/db/node.go (2)
Line range hint
231-250: LGTM!The changes to the
RenameNodefunction look good:
- The function signature update to use
types.NodeIDenhances type safety and clarity.- The new uniqueness check for the node name is a necessary validation to maintain data integrity.
- The function correctly returns an error if the new name is not unique.
413-420: LGTM!The changes to the
RegisterNodefunction look good:
- The new check to ensure that the node has a unique
GivenNameproactively maintains data integrity upon registration.- The function correctly handles the error returned by
ensureUniqueGivenName.hscontrol/db/node_test.go (2)
Line range hint
1-1: LGTM!The removal of the TODO comment is approved, as it indicates that the previously missing tests have been added.
738-833: Brilliant test for the node renaming functionality!The
TestRenameNodefunction is a comprehensive test that covers all the important scenarios for node renaming:
- Registering nodes with the same hostname for different users and verifying that they are assigned unique given names.
- Renaming a node to a unique name.
- Reusing a name that is no longer in use.
- Attempting to rename a node to a name that is currently in use and expecting an error.
The test is well-structured, uses clear assertions to verify the expected behavior, and leverages database transactions to maintain data integrity during the rename operations.
Excellent work on this test! It provides great coverage for the node renaming functionality.
Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
hscontrol/db/node.go (2)
649-656: LGTM, but fix the typo in the function name.The
isUnqiueNamefunction is correctly implemented and returns the expected result.However, the function name has a typo:
-func isUnqiueName(tx *gorm.DB, name string) (bool, error) { +func isUniqueName(tx *gorm.DB, name string) (bool, error) {Please fix the typo in the function name and update all references to it.
Line range hint
659-681: LGTM, but update the function to use the corrected nameisUniqueName.The refactoring of
ensureUniqueGivenNameis approved:
- The refactoring improves code readability and maintainability by separating the concerns of generating a name and checking its uniqueness.
- The function correctly handles the error returned by
generateGivenNameandisUnqiueName.- The function ensures that a unique name is generated by appending a random suffix if necessary.
However, please update the function to use the corrected name
isUniqueName:- unique, err := isUnqiueName(tx, givenName) + unique, err := isUniqueName(tx, givenName)
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- hscontrol/db/node.go (5 hunks)
Additional comments not posted (3)
hscontrol/db/node.go (3)
Line range hint
231-250: LGTM!The changes to the
RenameNodefunction are approved:
- The
nodeIDparameter type has been updated totypes.NodeID, which enhances type safety and clarity.- The function now includes a check for the uniqueness of the new name by calling
isUnqiueName, which is a necessary validation to maintain data integrity.- The function correctly returns an error if the new name is not unique.
413-420: LGTM!The changes to the
RegisterNodefunction are approved:
- The function now includes a check to ensure that if the
GivenNamefield of a node is empty, it will be populated by callingensureUniqueGivenName.- This change proactively ensures that a node has a unique given name upon registration, which enhances data integrity.
- The function correctly handles the error returned by
ensureUniqueGivenName.
Line range hint
1-1: Verify the impact of removinglistNodesByGivenName.The removal of
listNodesByGivenNameis a significant change that affects how nodes are queried by their given names.To ensure that this change does not introduce any issues, please verify that the functionality of
listNodesByGivenNamehas been correctly replaced or refactored elsewhere in the codebase.Run the following script to search for potential usages of
listNodesByGivenNamein the codebase:If the script returns any occurrences of
listNodesByGivenName, please investigate and ensure that its functionality has been correctly replaced or refactored.
* move logic for validating node names this commits moves the generation of "given names" of nodes into the registration function, and adds validation of renames to RenameNode using the same logic. Fixes juanfont#2121 Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com> * fix double arg Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com> --------- Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>
this commits moves the generation of "given names" of nodes into the registration function, and adds validation of renames to RenameNode using the same logic.
Fixes #2121
Summary by CodeRabbit
New Features
GivenNamefield.Bug Fixes
Tests