go/sentry: simplify tm sentry flags #2560
Conversation
ptrus
changed the title
Codecov Report
@@ Coverage Diff @@
## master #2560 +/- ##
==========================================
+ Coverage 63.62% 63.78% +0.16%
==========================================
Files 347 347
Lines 32399 32401 +2
==========================================
+ Hits 20613 20667 +54
+ Misses 9218 9177 -41
+ Partials 2568 2557 -11
Continue to review full report at Codecov.
|
ptrus
force-pushed
the
ptrus/feature/simplify-tendermint-sentry
branch
3 times, most recently
from
fc27ef8 to
b099140
Compare
ptrus
changed the title
ptrus
force-pushed
the
ptrus/feature/simplify-tendermint-sentry
branch
3 times, most recently
from
3fc1db4 to
a876333
Compare
| // CfgP2PPrivatePeerID configures tendermint's private peer ID(s). | ||
| CfgP2PPrivatePeerID = "tendermint.private_peer_id" | ||
| // CfgP2PPersistentPeer configures tendermint's persistent peer(s). | ||
| CfgP2PPersistentPeer = "tendermint.persistent_peer" |
There was a problem hiding this comment.
Hm, actually having an option to configure persistent peers may still be useful (e.g., on the sentry nodes to establish connections to known good peers).
There was a problem hiding this comment.
yeah added it back, so now i'm also thinking if there's even any benefit in setting private_peer_ids on the sentry node in our case. In tendermint's case it's useful since it prevents others from knowing which nodes are behind a specific sentry. In our case this isn't really true, since this will be visible from the registry.
This would mean that there's no benefit of the (just added) tendermint.sentry.upstream_address flag, since it would do the same as the persistent_peers flag. So in that case i would be in favor of also removing this flag, so the sentry setup configuration would be:
- on sentry node:
- set sentry nodes as private_peers
- on upstream node:
worker.registration.sentry.address,worker.registration.sentry.cert_fiile(for querying addresses at registration time) (just renames of the existing flags)tendermint.disable_peer_exchange
^ which seems simple enough
Edit: Actually without the private_peer flag, the sentry will gossip the validator's IPs with others. (those IPs should most likely to be private network IPs, but it might still be better to keep the flag on). The thought about usefulness of tendermint.sentry.upstream_address flag instead of just manually setting the low level tendermint flags correctly remains.
So one place where persistent peers would IMO be useful is on the sentry nodes in order to ensure connections to some "known-good" nodes are established (if available). |
yeah agreed, i think this is actually used a lot in in cosmos/tendermint (e.g. "relay nodes"). I will leave it in, and make the added sentry flag just append to the persistent peers. |
ptrus
force-pushed
the
ptrus/feature/simplify-tendermint-sentry
branch
from
a876333 to
158442f
Compare
ptrus
force-pushed
the
ptrus/feature/simplify-tendermint-sentry
branch
from
158442f to
080a62d
Compare
|
thanks for review! |
Fixes: #2362
This changes the sentry configuration to:
--tendermint.sentry.upstream_address--worker.registration.sentry.address,--worker.registration.sentry.cert_fiile(for querying addresses at registration time)--tendermint.disable_peer_exchangeOne regression to the previous sentry setup is that the upstream node no longer sets "tendermint.persistent_peers" to sentry nodes. I think this is fine, since in any real sentry node setup, the sentry nodes are the only nodes connecting to the upstream node, so not sure if setting persistent peers is/was really needed/useful. Let me know what others think.
Also updated the E2E tests so that nodes behind sentries do not connect to the seed node, since currently the sentry setup was not really properly tested as all nodes got addresses of upstream nodes through the seed node. In future should probably enhance the sentry E2E test further, by blocking all connections to the upstream nodes not originating from sentry nodes.