p2p: prevent connections from same ip by ebuchman · Pull Request #1520 · tendermint/tendermint

ebuchman · 2018-04-28T20:40:00Z

TODO: fix and write tests

Updated all relevant documentation in docs
Updated all code comments where relevant
Wrote tests
Updated CHANGELOG.md

odeke-em · 2018-04-29T07:47:46Z

p2p/peer_set.go

 func (ps *PeerSet) Remove(peer Peer) {
 	ps.mtx.Lock()
 	defer ps.mtx.Unlock()
+	delete(ps.lookupIP[peer.RemoteIP()])


Does this compile?

melekes

🥇 🏎 🍰

melekes · 2018-04-29T16:37:34Z

p2p/peer_set.go


+// HasIP returns true iff the PeerSet contains
+// the peer referred to by this IP address.
+func (ps *PeerSet) HasIP(peerIP string) bool {


maybe we should use net.IP here?

xla

Does this mean we have to overhaul the majority of our switch tets, as we use netpipe excessively which reports pipe as the remote addr?

xla · 2018-04-30T14:02:00Z

p2p/peer_set.go


+// HasIP returns true iff the PeerSet contains
+// the peer referred to by this IP address.
+func (ps *PeerSet) HasIP(peerIP string) bool {


xla · 2018-04-30T14:02:21Z

p2p/peer.go

+func (pc peerConn) RemoteIP() string {
+	host, _, err := net.SplitHostPort(pc.conn.RemoteAddr().String())
+	if err != nil {
+		panic(err)


Is this case worth a panic?

xla · 2018-04-30T14:03:01Z

p2p/peer.go

 }

+// Return the IP from the connection RemoteAddr
+func (pc peerConn) RemoteIP() string {


If we truly want to return an ip address I think we should do a lookup in case RemoteAddr() is a dns name.

ebuchman · 2018-05-21T20:17:21Z

:o same test failure as anton is having in #1574 (comment)

ebuchman · 2018-05-21T20:32:02Z

Oh well, here at least presumably it's becuase they all have the same IP, so they disconnect when they realize thaT?

ebuchman · 2018-05-21T20:37:00Z

maybe what we need is some kind of "filter" object that we can set that determines what peers we disconnect to ... that way in tests we don't need to apply the same level of filters so we can not worry about connecting to self when we're trying to test something else ... ?

ebuchman · 2018-05-21T20:41:23Z

p2p/switch.go

 		return err
 	}

+	// dont connect to multiple peers on the same IP


out of place ?

ebuchman · 2018-05-21T20:41:52Z

p2p/peer.go

+		return pc.ips[0]
+	}
+
+	if pc.conn == nil || pc.conn.RemoteAddr().String() == "pipe" {


is this only nil during test? we should add comment either way that this little section is just for tests.

ebuchman · 2018-05-21T20:42:43Z

p2p/peer.go

+		return pc.ips[0]
+	}
+
+	host, _, err := net.SplitHostPort(pc.conn.RemoteAddr().String())


Does RemoteAddr return host names or only IPs ?

ebuchman · 2018-05-21T20:43:20Z

p2p/peer.go

 	persistent bool
 	config     *PeerConfig
 	conn       net.Conn // source connection
+	ips        []net.IP


are you thinking we'll use this for something ? looks like we only ever use the [0]

ebuchman · 2018-05-21T20:44:42Z

p2p/switch.go

+	}
+
+	// check ips for both the connection addr and the self reported addr
+	if sw.peers.HasIP(pc.RemoteIP()) {


this is the connection address. Maybe we should check the self-reported address from the NodeInfo as well ?

ebuchman · 2018-05-21T20:46:15Z

p2p/peer_set_test.go

 	assert.Equal(t, wantNilErrCount, gotNilErrCount, "invalid nil errCount")
 }

+func TestPeerSetAddDuplicateIP(t *testing.T) {


ebuchman · 2018-05-21T20:49:03Z

Looks good. Some minor comments, but mostly just need to address the test issue before merge.

Note the test failure here actually has a switch with no peers and makes sense why its not working ( I think ). The fail in #1574 might be different

xla · 2018-05-22T14:35:03Z

@ebuchman Thanks for the thorough review.

codecov-io · 2018-05-23T00:51:39Z

Codecov Report

Merging #1520 into develop will increase coverage by 3.3%.
The diff coverage is 60.97%.

@@            Coverage Diff             @@
##           develop    #1520     +/-   ##
==========================================
+ Coverage    60.62%   63.93%   +3.3%     
==========================================
  Files          115      103     -12     
  Lines         9971     9571    -400     
==========================================
+ Hits          6045     6119     +74     
+ Misses        3356     2891    -465     
+ Partials       570      561      -9

Impacted Files	Coverage Δ
p2p/errors.go	`0% <0%> (ø)`	⬆️
p2p/peer_set.go	`100% <100%> (ø)`	⬆️
p2p/test_util.go	`56.47% <100%> (ø)`	⬆️
p2p/switch.go	`53.03% <28.57%> (+0.53%)`	⬆️
p2p/peer.go	`66.45% <71.42%> (+0.49%)`	⬆️
state/execution.go	`70.39% <0%> (-0.47%)`	⬇️
consensus/wal_generator.go	`79.2% <0%> (-0.41%)`	⬇️
rpc/client/httpclient.go	`69.3% <0%> (-0.31%)`	⬇️
node/node.go	`64.37% <0%> (-0.12%)`	⬇️
config/config.go	`81.28% <0%> (-0.11%)`	⬇️
... and 26 more

xla · 2018-05-23T01:13:03Z

@ebuchman @melekes green at last and ready for a final review.

ebuchman · 2018-05-24T00:57:13Z

LGTM!

…ndermint#1520) Bumps [bufbuild/buf-setup-action](https://github.com/bufbuild/buf-setup-action) from 1.27.0 to 1.27.1. - [Release notes](https://github.com/bufbuild/buf-setup-action/releases) - [Commits](bufbuild/buf-setup-action@v1.27.0...v1.27.1) --- updated-dependencies: - dependency-name: bufbuild/buf-setup-action dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

ebuchman requested a review from melekes as a code owner April 28, 2018 20:40

ebuchman mentioned this pull request Apr 29, 2018

p2p: prevent multiple connections from same IP #1264

Closed

odeke-em reviewed Apr 29, 2018

View reviewed changes

melekes reviewed Apr 29, 2018

View reviewed changes

xla reviewed Apr 30, 2018

View reviewed changes

ebuchman changed the base branch from bucky/p2p-cleanup to develop April 30, 2018 22:30

ebuchman assigned xla Apr 30, 2018

ebuchman and others added 4 commits May 16, 2018 19:21

p2p: prevent connections from same ip

1fe41be

Move to ne.IP

77f09f5

Use remotePeer for test switch

c5f4527

Remove double locking in HasIP

b698a9f

xla force-pushed the bucky/p2p-same-ip branch from 4382dbe to b698a9f Compare May 16, 2018 17:25

xla added 5 commits May 18, 2018 16:27

Let peerConn handle IPs in for tests

d596ed1

Fix race in test suffix

0cd92a4

Add RemoteIP to test implementation

7b02b5b

Return fake IP even when there is no conn

20e9dd0

Do not set address for self error

91b6d3f

ebuchman commented May 21, 2018

View reviewed changes

xla added 3 commits May 23, 2018 00:24

Fix persistent peer switch test

4848e88

Test duplicate IP guard in peer set

7d98cfd

Fix pex reactor test

e11f316

xla force-pushed the bucky/p2p-same-ip branch 2 times, most recently from 2dee41b to 01fd102 Compare May 22, 2018 23:54

xla added 2 commits May 23, 2018 01:56

Incoporate review feedback

01fd102

Use different loopback addresses for test switch

186d38d

ebuchman merged commit caf5afc into develop May 24, 2018

ebuchman deleted the bucky/p2p-same-ip branch May 24, 2018 00:57

xla mentioned this pull request May 24, 2018

[wal] small fixes in SearchEndHeight & replay logic #1607

Merged

Conversation

ebuchman commented Apr 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

melekes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xla left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebuchman commented May 21, 2018

Uh oh!

ebuchman commented May 21, 2018

Uh oh!

ebuchman commented May 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebuchman commented May 21, 2018

Uh oh!

xla commented May 22, 2018

Uh oh!

codecov-io commented May 23, 2018

Codecov Report

Uh oh!

xla commented May 23, 2018

Uh oh!

ebuchman commented May 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ebuchman commented Apr 28, 2018 •

edited

Loading