Use ExternalAddr attribute for inter-node communication
#1817
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1817 +/- ##
==========================================
- Coverage 31.06% 30.95% -0.12%
==========================================
Files 374 372 -2
Lines 26377 25882 -495
==========================================
- Hits 8195 8012 -183
+ Misses 17492 17194 -298
+ Partials 690 676 -14
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| @@ -17,12 +17,15 @@ func (x Node) PublicKey() []byte { | |||
| // IterateAddresses iterates over all announced network addresses | |||
| // and passes them into f. Handler MUST NOT be nil. | |||
| func (x Node) IterateAddresses(f func(string) bool) { | |||
| for _, addr := range (netmap.NodeInfo)(x).ExternalAddresses() { | |||
There was a problem hiding this comment.
So firstly we are going to process external addresses everywhere, is it expected for intra-container communication? I thought "old" addresses are preferred for this purpose.
|
@fyrchik are you sure you've covered all the needed places? I see a few more places of
|
|
IIRC, the ExternalAddr attribute should be used for client (sdk?) connections to nodes. For inter-node communication we have to use addressed from netmap and use ExternalAddr as a fallback only, if there is an option allowing such behaviour. |
|
There are 3 different approaches:
I decided to go with the first, because this way there are fewer places with filtering logic, so less bugs. Also, it will be easier to change |
| @@ -6,7 +6,7 @@ Changelog for NeoFS Node | |||
| ### Added | |||
| - Serving `NetmapService.NetmapSnapshot` RPC (#1793) | |||
| - `netmap snapshot` command of NeoFS CLI (#1793) | |||
|
|
|||
| - `apiclient.allow_external` config flag to fallback to node external addresses (#1817) | |||
There was a problem hiding this comment.
do we need to add that to the Update from... section too?
There was a problem hiding this comment.
I think no, it is a completely new feature, old nodes are expected to continue working.
There was a problem hiding this comment.
yes, but how will an admin know about that? i try to write any config changes to that section. but ok
There was a problem hiding this comment.
I suggest adding this to Update section. With new behaviour node admins can explicitly declare external addresses for their node, not only listen corresponding addresses.
There was a problem hiding this comment.
Yes, but do we need Added section in this case? Every added feature can be used in some way, I've thought Updating from was for things requiring special care.
Fixed anyway.
There was a problem hiding this comment.
If we need Ops attention, we add/duplicate something in Updating from. For this particular case we need to highlight it.
Signed-off-by: Evgenii Stratonikov <evgeniy@morphbits.ru>
…rator Signed-off-by: Evgenii Stratonikov <evgeniy@morphbits.ru>
Signed-off-by: Evgenii Stratonikov <evgeniy@morphbits.ru>
…rator Signed-off-by: Evgenii Stratonikov <evgeniy@morphbits.ru>
No description provided.