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.

Sign up for GitHub

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

Jump to bottom

Ensure not 2 nodes can have the same name with different case #4413

Closed
pierresouchay opened this issue Jul 18, 2018 · 1 comment
Closed

Ensure not 2 nodes can have the same name with different case #4413

pierresouchay opened this issue Jul 18, 2018 · 1 comment
Labels
theme/internal-cleanup Used to identify tech debt, testing improvements, code refactoring, and non-impactful optimization type/enhancement Proposed improvement or new feature

Comments

@pierresouchay
Copy link
Contributor

Consul Version: 1.1.0 (server and client side)

Overview of the Issue

When renaming a node, ensure the name is not taken by another node.

Since DNS is case insensitive and DB as issues when similar names with different
cases are added, check for unicity based on case insensitivity.

Following another big incident we had in our cluster, we also validate
that adding/renaming a not does not conflicts with case insensitive
matches.

We had the following error once:

  • one node called: mymachine.MYDC.mydomain was shut off
  • another node (different ID) was added with name: mymachine.mydc.mydomain before
    72 hours

When restarting the consul server of domain, the consul server restarted failed
to start since it detected an issue in RAFT database because
mymachine.MYDC.mydomain and mymachine.mydc.mydomain had the same names.

Checking at registration time with case insensitivity should definitly fix
those issues and avoid Consul DB corruption.
pierresouchay added a commit to pierresouchay/consul that referenced this issue Jul 18, 2018
@pierresouchay
Allow to rename nodes with IDs, will fix hashicorp#3974 and hashicorp…
5aca4e5 
…#4413

This change allow to rename any well behaving recent agent with an
ID to be renamed safely, ie: without taking the name of another one
with case insensitive comparison.

Deprecated behaviour warning
----------------------------

Due to asceding compatibility, it is still possible however to
"take" the name of another name by not providing any ID.

Note that when not providing any ID, it is possible to have 2 nodes
having similar names with case differences, ie: myNode and mynode
which might lead to DB corruption on Consul server side and
lead to server not properly restarting.

See hashicorp#3983 and hashicorp#4399 for Context about this change.

Disabling registration of nodes without IDs as specified in hashicorp#4414
should probably be the way to go eventually.
@pierresouchay pierresouchay mentioned this issue Jul 18, 2018
Merged
@mkeeler mkeeler added type/enhancement Proposed improvement or new feature theme/internal-cleanup Used to identify tech debt, testing improvements, code refactoring, and non-impactful optimization labels Jul 26, 2018
mkeeler pushed a commit that referenced this issue Aug 10, 2018
@pierresouchay @mkeeler
Allow to rename nodes with IDs, will fix #3974 and #4413 (#4415)
ef3b81a 
* Allow to rename nodes with IDs, will fix #3974 and #4413

This change allow to rename any well behaving recent agent with an
ID to be renamed safely, ie: without taking the name of another one
with case insensitive comparison.

Deprecated behaviour warning
----------------------------

Due to asceding compatibility, it is still possible however to
"take" the name of another name by not providing any ID.

Note that when not providing any ID, it is possible to have 2 nodes
having similar names with case differences, ie: myNode and mynode
which might lead to DB corruption on Consul server side and
lead to server not properly restarting.

See #3983 and #4399 for Context about this change.

Disabling registration of nodes without IDs as specified in #4414
should probably be the way to go eventually.

* Removed the case-insensitive search when adding a node within the else
block since it breaks the test TestAgentAntiEntropy_Services

While the else case is probably legit, it will be fixed with #4414 in
a later release.

* Added again the test in the else to avoid duplicated names, but
enforce this test only for nodes having IDs.

Thus most tests without any ID will work, and allows us fixing

* Added more tests regarding request with/without IDs.

`TestStateStore_EnsureNode` now test registration and renaming with IDs

`TestStateStore_EnsureNodeDeprecated` tests registration without IDs
and tests removing an ID from a node as well as updated a node
without its ID (deprecated behaviour kept for backwards compatibility)

* Do not allow renaming in case of conflict, including when other node has no ID

* Fixed function GetNodeID that was not working due to wrong type when searching node from its ID

Thus, all tests about renaming were not working properly.

Added the full test cas that allowed me to detect it.

* Better error messages, more tests when nodeID is not a valid UUID in GetNodeID()

* Added separate TestStateStore_GetNodeID to test GetNodeID.

More complete test coverage for GetNodeID

* Added new unit test `TestStateStore_ensureNoNodeWithSimilarNameTxn`

Also fixed comments to be clearer after remarks from @banks

* Fixed error message in unit test to match test case

* Use uuid.ParseUUID to parse Node.ID as requested by @mkeeler
@pierresouchay
Copy link
Contributor Author

Fixed in 1.2.3 by #4415

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
theme/internal-cleanup Used to identify tech debt, testing improvements, code refactoring, and non-impactful optimization type/enhancement Proposed improvement or new feature
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@mkeeler @pierresouchay