[FIXED] Some Leafnode issues #1106
Conversation
server/leafnode.go
Outdated
| @@ -499,7 +515,7 @@ func (s *Server) createLeafNode(conn net.Conn, remote *leafNodeCfg) *client { | |||
| // FIXME(dlc) - Make this resolve at startup. | |||
There was a problem hiding this comment.
If this is at startup we can remove, but if server starts and then can fail at a later time still need to update this IMO.
server/leafnode.go
Outdated
| @@ -499,7 +515,7 @@ func (s *Server) createLeafNode(conn net.Conn, remote *leafNodeCfg) *client { | |||
| // FIXME(dlc) - Make this resolve at startup. | |||
| acc, err := s.LookupAccount(remote.LocalAccount) | |||
| if err != nil { | |||
| c.Debugf("No local account %q for leafnode", remote.LocalAccount) | |||
| s.Fatalf("No local account %q for leafnode: %v", remote.LocalAccount, err) | |||
server/leafnode.go
Outdated
| @@ -499,7 +515,7 @@ func (s *Server) createLeafNode(conn net.Conn, remote *leafNodeCfg) *client { | |||
| // FIXME(dlc) - Make this resolve at startup. | |||
| acc, err := s.LookupAccount(remote.LocalAccount) | |||
| if err != nil { | |||
| c.Debugf("No local account %q for leafnode", remote.LocalAccount) | |||
| s.Fatalf("No local account %q for leafnode: %v", remote.LocalAccount, err) | |||
There was a problem hiding this comment.
Is this only for local config? Meaning can this fail on operator mode lookup?
There was a problem hiding this comment.
That's what we discussed yesterday! My initial thought was that we should ERR and retry but you said that the account should always be avail otherwise we should not start (arguably, we could then check the account availability at startup instead of here, but..).
If you are asking if LookupAccount() can fail, of course it can, if it has to fetch and connection is not avail, etc..
There was a problem hiding this comment.
We are in createLeafnode() here, which is invoked when soliciting or accepting a LN connection.
There was a problem hiding this comment.
I think per my comment if we are in local config mode we should lookup any account binding for the leafnode on startup and fail if it does not exist.
There was a problem hiding this comment.
For solicited ones its essentially on startup, but not for accepted ones.
There was a problem hiding this comment.
My concern was that in operator mode we may not be able to lookup the account at first connect, but may later. But that may be wrong assumption.
Regardless of local/operator mode, lookup on Server A can fail at runtime:
- Account removed and config reload issued
- Account expired
If after any of those 2 events Server B is restarted, Server A would try to recreate connection to B and lookup will fail (just tried with removing account + config reload + restart of B).
There was a problem hiding this comment.
For Server A that has a solicited connection to Server B, agree that the account can expire, or be removed via config reload. We need to define our desired behavior when that happens. Server should not fail or exit obviously, but probably should not aimlessly try to recreate the connection without a valid local account IMO.
There was a problem hiding this comment.
All my discussion has been about Server A here.. the one that has a "remote" configuration in "leafnodes{}".
Let's discuss then later, because I am not sure what is the alternative between [FTL] (exit) and [ERR] and try again... except prevent removal/expiration of such account? (not sure how we would do that because that would leave the config file in different state than the runtime)..
There was a problem hiding this comment.
On startup if a solicited leafnode can not resolve/lookup its local account the server should fail to start IMO. If we subsequently get an error on the account we should shut the leafnode down. We can keep trying to reconnect and failing on account lookup to see if it resolves, or be smarter about it but that feels like we can do that later on.
There was a problem hiding this comment.
Ok, updated the PR (push forced). Not sure I have captured what you wanted. Happy to iterate if needed or remove the connect/account part and just leave the fix for username/password.
- On startup, verify that local account in leafnode (if specified can be found otherwise fail startup). - At runtime, print error and continue trying to reconnect. Will need to decide a better approach. - When using basic auth (user/password), it was possible for a solicited Leafnode connection to not use user/password when trying an URL that was discovered through gossip. The server now saves the credentials of a configured URL to use with the discovered ones. Updated RouteRTT test in case RTT does not seem to be updated because getting always the same value. Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
When soliciting a Leafnode connection, if there is an account
specified and the lookup fails, the server would only print
as a debug and the connection would never been retried. After
discussion, it seems that proper behavior would be to produce
a fatal error.
When using basic auth (user/password), it was possible for a
solicited Leafnode connection to not use user/password when
trying an URL that was discovered through gossip. The server
now saves the credentials of a configured URL to use with
the discovered ones.
Updated RouteRTT test in case RTT does not seem to be updated
because getting always the same value.
Signed-off-by: Ivan Kozlovic ivan@synadia.com