From 93668a7ff5514d55d3a8b26cabd1915ce91a3332 Mon Sep 17 00:00:00 2001 From: Konrad `ktoso` Malawski Date: Thu, 3 Sep 2020 21:45:03 +0900 Subject: [PATCH] !swim disallow addMember of non UID peers --- Sources/SWIM/SWIMInstance.swift | 15 ++++++++++++++- Tests/SWIMTests/SWIMInstanceTests.swift | 10 ++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/Sources/SWIM/SWIMInstance.swift b/Sources/SWIM/SWIMInstance.swift index 40b2016..6634844 100644 --- a/Sources/SWIM/SWIMInstance.swift +++ b/Sources/SWIM/SWIMInstance.swift @@ -393,10 +393,17 @@ extension SWIM { } ) - /// Take care about peers with UID and without (!), some shells may not be quite good about this. + /// Note that peers without UID (in their `Node`) will NOT be added to the membership. + /// + /// This is because a cluster member must be a _specific_ peer instance, and not some arbitrary "some peer on that host/port", + /// which a Node without UID represents. The only reason we allow for peers and nodes without UID, is to simplify making + /// initial contact with a node - i.e. one can construct a peer to "there should be a peer on this host/port" to send an initial ping, + /// however in reply a peer in gossip must ALWAYS include it's unique identifier in the node - such that we know it from + /// any new instance of a process on the same host/port pair. internal func addMember(_ peer: SWIMPeer, status: SWIM.Status) -> [AddMemberDirective] { var directives: [AddMemberDirective] = [] + // Guard 1) protect against adding already known dead members if self.hasTombstone(peer.node) { // We saw this member already and even confirmed it dead, it shall never be added again self.log.debug("Attempt to re-add already confirmed dead peer \(peer), ignoring it.") @@ -404,6 +411,12 @@ extension SWIM { return directives } + // Guard 2) protect against adding non UID members + guard peer.node.uid != nil else { + self.log.warning("Ignoring attempt to add peer representing node without UID: \(peer)") + return directives + } + let maybeExistingMember = self.member(for: peer) if let existingMember = maybeExistingMember, existingMember.status.supersedes(status) { // we already have a newer state for this member diff --git a/Tests/SWIMTests/SWIMInstanceTests.swift b/Tests/SWIMTests/SWIMInstanceTests.swift index ba41896..6e75232 100644 --- a/Tests/SWIMTests/SWIMInstanceTests.swift +++ b/Tests/SWIMTests/SWIMInstanceTests.swift @@ -906,6 +906,16 @@ final class SWIMInstanceTests: XCTestCase { XCTAssertNil(swim.nextPeerToPing()) } + func test_addMember_shouldNotAddPeerWithoutUID() { + let swim = SWIM.Instance(settings: .init(), myself: self.myself) + + let other = TestPeer(node: .init(protocol: "test", host: "127.0.0.1", port: 111, uid: nil)) + let directives = swim.addMember(other, status: .alive(incarnation: 0)) + XCTAssertEqual(directives.count, 0) + XCTAssertFalse(swim.isMember(other)) + XCTAssertNil(swim.nextPeerToPing()) + } + func test_addMember_shouldReplaceMemberIfDifferentUID() { let swim = SWIM.Instance(settings: .init(), myself: self.myself) _ = swim.addMember(self.second, status: .alive(incarnation: 0))