[FAB-17675] Prevent gossip probe from registering as a connection #925
Conversation
Part of FAB-17672: Gossip bootstrap peer and anchor peer connection establishments are a two step process: I) Connect to the remote endpoint, and figure out its organization association II) Connect to the remote endpoint once again, this time sending membership information according to what it is eligible to. Unfortunately, if the responding peer connects to the initiator peer at the same time, the connection of (1) will be overwritten by the connection (gossip keeps a single connection among every pair of peers) from the responder peer and the initiator peer will consider the responder peer as dead. In production, where peers run for a long time and constnatly retry, this is not a problem. However, in integration tests that expect data to be disseminated in a timely manner, this might cause flakes. this change set adds a flag that indicates this connection is not a long lasting one, and thus there is no point to add it to the connection store. Change-Id: Id8f4ee12145748527233301356af830c3401ee02 Signed-off-by: yacovm <yacovm@il.ibm.com>
| @@ -37,6 +37,10 @@ const ( | |||
| DefSendBuffSize = 20 | |||
| ) | |||
|
|
|||
| var ( | |||
| errProbe = errors.New("probe") | |||
There was a problem hiding this comment.
Can't we have a more descriptive error message for better readability?
Correct me if I have misunderstood: this error would be returned when the peer tries to authenticate a stream which is not a probe (by passing an appropriate flag) but it ends up being a probe. As a result, we don't store info in the connection store. If my understanding is correct, I am not sure whether it should be an error. Maybe a bool can be used instead as I mentioned in the other comment.
There was a problem hiding this comment.
You misunderstood. This error just means that the remote peer probed us, so there is no point in doing anything with the returned first value.
There was a problem hiding this comment.
Okay. That's what I meant may be my wordings didn't convey that. That's why I suggested isProbe bool later in the comment.
There was a problem hiding this comment.
I see a function that returns an object and an error in the following way:
If the error is nil, use the first return value.
Else, use the other return value.
This perfectly fits my needs here.
| @@ -163,7 +167,7 @@ func (c *commImpl) createConnection(endpoint string, expectedPKIID common.PKIidT | |||
|
|
|||
| ctx, cancel = context.WithCancel(context.Background()) | |||
| if stream, err = cl.GossipStream(ctx); err == nil { | |||
| connInfo, err = c.authenticateRemotePeer(stream, true) | |||
| connInfo, err = c.authenticateRemotePeer(stream, true, false) | |||
| if err == nil { | |||
There was a problem hiding this comment.
Unrelated to this CR. Can be err != nil and return early to avoid the extra indentation.
There was a problem hiding this comment.
This is by design.
I need to close the gRPC connection, and also to cancel the gRPC stream.
So it's not just an early return. I want cleanup operations to be at the bottom of the function so that I manage them at the same place.
| connInfo, err := c.authenticateRemotePeer(stream, false) | ||
| connInfo, err := c.authenticateRemotePeer(stream, false, false) | ||
|
|
||
| if err == errProbe { |
There was a problem hiding this comment.
It can be confusing to the code reader. Some questions might arise like why we are ignoring an error. If we are trying to not store info in the conn store for probs, can't the return value be an isProbe bool?
There was a problem hiding this comment.
No, 3 return values is ugly, and this is an un-exported function.
It's perfectly fine to have a specific error that indicates something.
Look at:
if e, ok := err.(*QueryError); ok && e.Err == ErrPermission {
// query failed because of a permission problem
}
There was a problem hiding this comment.
Thanks for the clarification. Agree that 3 return value is ugly.
|
This PR helps to fix the test flake in FAB-17611. The changes have been included in #895 for verification purpose. |
[FAB-17675] Prevent gossip probe from registering as a connection (hyperledger#925)
Part of FAB-17672:
Gossip bootstrap peer and anchor peer connection establishments are a two step process:
I) Connect to the remote endpoint, and figure out its organization association
II) Connect to the remote endpoint once again, this time sending membership information according to what it is eligible to.
Unfortunately, if the responding peer connects to the initiator peer at the same time, the connection of (1) will be overwritten by the connection (gossip keeps a single connection among every pair of peers) from the responder peer and the initiator peer will consider the responder peer as dead.
In production, where peers run for a long time and constnatly retry, this is not a problem.
However, in integration tests that expect data to be disseminated in a timely manner, this might cause flakes.
this change set adds a flag that indicates this connection is not a long lasting one, and thus there is no point to add it to the connection store.
Change-Id: Id8f4ee12145748527233301356af830c3401ee02
Signed-off-by: yacovm yacovm@il.ibm.com