In a setup with a cluster of servers to which 2 different leaf nodes
attach to, and queue subs are attached to one of the leaf, if the
leaf server is restarted and reconnects to another server in the
cluster, there was a risk for an infinite message loop between
some servers in the "hub" cluster.
Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
Currently, we use ReadyForConnections in server tests to wait for the
server to be ready. However, when this fails we don't get a clue about
why it failed.
This change adds a new unexported method called readyForConnections that
returns an error describing which check failed. The exported
ReadyForConnections version works exactly as before. The unexported
version gets used in internal tests only.
This was caused by not sending subs across leaf node connections in some
cases but sending unsub in all cases. This imbalance caused
subscriptions to go away too soon. (ref count was off)
Signed-off-by: Matthias Hanel <mh@synadia.com>
If the subscription was foo. > but the server also had an import deny of foo.bar
It was legal to send the subscription. But the other server was unaware
of the restriction and sent the message anyway. The check of the
incoming message did not happen.
Fixing by ignoring messages the server is not supposed to receive.
And exchange deny_import so that the non soliciting leaf node knows to not
send these messages in the first place.
NB. merging of deny_ export/import with perms from INFO happens in processLeafnodeInfo
Signed-off-by: Matthias Hanel <mh@synadia.com>
On connect all subscription where sent by the soliciting leaf node.
If creds contains sub deny permissions, the leaf node would be
disconnected.
This waits for the permissions to be exchanged and checks permissions
before sending subscriptions.
Signed-off-by: Matthias Hanel <mh@synadia.com>
We were setting the ping timer in the accepting server as soon
as the leafnode connection is created, just after sending
the INFO and setting the auth timer.
Sending a PING too soon may cause the solicit side to process
this PING and send a PONG in response, possibly before sending
the CONNECT, which the accepting side would fail as an authentication
error, since first protocol is expected to be a CONNECT.
Since LeafNode always expect a CONNECT, we always set the auth
timer. So now on accept, instead of starting the ping timer just
after sending the INFO, we will delay setting this timer only
after receiving the CONNECT.
The auth timer will take care of a stale connection in the time
it takes to receives the CONNECT.
Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
If leafnodes from a cluster were to reconnect to a server in
a different cluster, it was possible for that server to send
to the leafnodes some their own subscriptions that could cause
an inproper loop detection error.
There was also a defect that would cause subscriptions over route
for leafnode subscriptions to be registered under the wrong key,
which would lead to those subscriptions not being properly removed
on route disconnect.
Finally, during route disconnect, the leafnodes map was not updated.
This PR fixes that too.
Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
- It was possible that when the server was sending frames to a
webbrowser, it would send empty frames. While technically not wrong,
prevent that from happening.
- Not copying enqueued buffers could cause corruption with LN+WS.
Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
This also applies to times that end up in that json.
Where applicable moved time.Now() to where it is used.
Moved calls to .UTC() to where time is created it that time is converted
later anyway.
Signed-off-by: Matthias Hanel <mh@synadia.com>
The issue was introduced by PR #1858.
Key points:
- Sec-WebSocket-Extensions must contain approved headers, so moving
the "no-masking" private extension to its own header "Nats-No-Masking".
- The format of the permessage-deflate negotiation response became
invalid, I have fixed that.
- For leaf nodes, if `permessage-deflate` extension is not at all
present in the response, then simply disable compression, however
if it is present but there is no server/client no context take over,
then we have to fail the connection.
- A leafnode test was not setting the "NoMasking" option so the
test TestLeafNodeWSNoMaskingRejected was not capturing possible
error if negotiation failed.
Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
This will allow a better experience if there is a load balancer
in between and expects websocket frames to be masked.
Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
Added two options in the remote leaf node configuration
- compress, for websocket only at the moment
- ws_masking, to force remote leafnode connections to mask websocket
frames (default is no masking since it is communication between
server to server)
Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
The LeafNode connect protocol's Name field had json tag "name" but
was changed to "server_name" in the JetStream cluster branch.
Changing it back to "name" to not have to deal with different
places where to get the name from.
Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
The server that solicits a LeafNode connection does not send an
INFO, so the accepting side had no way to know if the remote
supports headers or not. The solicit side will now send the headers
support capability in the CONNECT protocol so that the receiving
side can mark the inbound connection with headers support based
on that and its own support for headers.
Resolves#1781
Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
Presence of TLS config in any remote gateway or leafnode would
cause the config reload to fail (because TLS config internal
content may change which fails the DeepEqual check).
This PR excludes the TLS configs in such case to check for
changes in gateways and leafnodes.
Although GW and LN config reload is technically supported, this
PR updates the internal remotes' TLS configuration so that
changes/updates to TLS certificates would take effect after
a configuration reload.
Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
A newly introduced test (TestLeafNodeTwoRemotesBindToSameAccount)
had a server creating two remotes to the same server/account.
This test quite often show the data race:
```
go test -race -v -run=TestLeafNodeTwoRemotesBindToSameAccount ./server -count 100 --failfast
=== RUN TestLeafNodeTwoRemotesBindToSameAccount
==================
WARNING: DATA RACE
Write at 0x00c000168790 by goroutine 34:
github.com/nats-io/nats-server/v2/server.(*client).processLeafNodeConnect()
/Users/ivan/dev/go/src/github.com/nats-io/nats-server/server/leafnode.go:1177 +0x314
github.com/nats-io/nats-server/v2/server.(*client).processConnect()
/Users/ivan/dev/go/src/github.com/nats-io/nats-server/server/client.go:1719 +0x9e4
github.com/nats-io/nats-server/v2/server.(*client).parse()
/Users/ivan/dev/go/src/github.com/nats-io/nats-server/server/parser.go:870 +0xf88
github.com/nats-io/nats-server/v2/server.(*client).readLoop()
/Users/ivan/dev/go/src/github.com/nats-io/nats-server/server/client.go:1052 +0x7a5
github.com/nats-io/nats-server/v2/server.(*Server).createLeafNode.func4()
/Users/ivan/dev/go/src/github.com/nats-io/nats-server/server/leafnode.go:872 +0x52
Previous read at 0x00c000168790 by goroutine 32:
github.com/nats-io/nats-server/v2/server.(*client).remoteCluster()
/Users/ivan/dev/go/src/github.com/nats-io/nats-server/server/leafnode.go:1203 +0x42d
github.com/nats-io/nats-server/v2/server.(*Server).updateLeafNodes()
/Users/ivan/dev/go/src/github.com/nats-io/nats-server/server/leafnode.go:1375 +0x2cf
github.com/nats-io/nats-server/v2/server.(*client).processLeafSub()
/Users/ivan/dev/go/src/github.com/nats-io/nats-server/server/leafnode.go:1619 +0x858
github.com/nats-io/nats-server/v2/server.(*client).parse()
/Users/ivan/dev/go/src/github.com/nats-io/nats-server/server/parser.go:624 +0x5031
github.com/nats-io/nats-server/v2/server.(*client).readLoop()
/Users/ivan/dev/go/src/github.com/nats-io/nats-server/server/client.go:1052 +0x7a5
github.com/nats-io/nats-server/v2/server.(*Server).createLeafNode.func4()
/Users/ivan/dev/go/src/github.com/nats-io/nats-server/server/leafnode.go:872 +0x52
Goroutine 34 (running) created at:
github.com/nats-io/nats-server/v2/server.(*Server).startGoRoutine()
/Users/ivan/dev/go/src/github.com/nats-io/nats-server/server/server.go:2627 +0xc7
github.com/nats-io/nats-server/v2/server.(*Server).createLeafNode()
/Users/ivan/dev/go/src/github.com/nats-io/nats-server/server/leafnode.go:872 +0xf7a
github.com/nats-io/nats-server/v2/server.(*Server).startLeafNodeAcceptLoop.func1()
/Users/ivan/dev/go/src/github.com/nats-io/nats-server/server/leafnode.go:474 +0x5e
github.com/nats-io/nats-server/v2/server.(*Server).acceptConnections.func1()
/Users/ivan/dev/go/src/github.com/nats-io/nats-server/server/server.go:1784 +0x57
Goroutine 32 (running) created at:
github.com/nats-io/nats-server/v2/server.(*Server).startGoRoutine()
/Users/ivan/dev/go/src/github.com/nats-io/nats-server/server/server.go:2627 +0xc7
github.com/nats-io/nats-server/v2/server.(*Server).createLeafNode()
/Users/ivan/dev/go/src/github.com/nats-io/nats-server/server/leafnode.go:872 +0xf7a
github.com/nats-io/nats-server/v2/server.(*Server).startLeafNodeAcceptLoop.func1()
/Users/ivan/dev/go/src/github.com/nats-io/nats-server/server/leafnode.go:474 +0x5e
github.com/nats-io/nats-server/v2/server.(*Server).acceptConnections.func1()
/Users/ivan/dev/go/src/github.com/nats-io/nats-server/server/server.go:1784 +0x57
==================
testing.go:965: race detected during execution of test
--- FAIL: TestLeafNodeTwoRemotesBindToSameAccount (0.05s)
```
This is because as soon as a LEAF is registered with the account, it is available
in the account's lleafs map, even before the CONNECT for this connectio is processed.
If another LEAF connection is processing a LSUB, the code goes over all leaf connections
for the account and may find the new connection that is in the process of connecting.
The check accesses c.leaf.remoteCluster unlocked which is also set unlocked during
the CONNECT. The fix is to have the set and check on that particular location using
the client's lock.
Ideally I believe that the connection should not have been in the account's lleafs,
or at least not used until the CONNECT for this leaf connection is fully processed.
Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
There was a test to prevent an errorneous loop detection when a
remote would reconnect (due to a stale connection) while the accepting
side did not detect the bad connection yet.
However, this test was racy because the test was done prior to add
the connections to the map.
In the case of a misconfiguration where the remote creates 2 different
remote connections that end-up binding to the same account in the
accepting side, then it was possible that this would not be detected.
And when it was, the remote side would be unaware since the disconnect/
reconnect attempts would not show up if not running in debug mode.
This change makes sure that the detection is no longer racy and returns
an error to the remote so at least the log/console of the remote will
show the "duplicate connection" error messages.
Resolves#1730
Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
This is an addition to PR #1652. I have simply added a check but
at this point in time there is no risk that connection is closed
this early.
I also renamed the small helper function and fixed a test that
had an improper `s.mu.Unlock()` in an error condition.
Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
If the soliciting side detects the disconnect and attempts to
reconnect but the accepting side did not yet close the connection,
a "loop detected" error would be reported and the soliciting server
would not try to reconnect for 30 seconds.
Made a change so that the accepting server checks for existing
leafnode connection for the same server and same account, and if
it is found, close the "old" connection so it is replaced by
the "new" one.
Resolves#1606
Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
We previously simply called DialTimeout() on a route's url when
soliciting. If it resolved to the IP of the host, it would create
a route to self, which server detects, but then would not try again
with other IPs that would have allowed to form a cluster with
other servers running on the other IPs.
This PR keeps track of local IPs + cluster port and exclude them
from the list of IPs returned by LookupHost API. This even prevent
solicitation of routes to self. Only non-local IPs will be tried.
Resolves#1586
Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
Inhibit Go's default TCP keepalive settings for NATS
Go 1.13 changed the semantics of the tuning parameters for TCP keepalives, including the default value. This affects all TCP listeners. The NATS protocol has its own L7 keepalive system (PING/PONG) and the Go defaults are not a good fit for some valid deployment scenarios, while Go doesn't directly expose a working API for tuning these.
Rather than add a configuration knob and pull in another dependency (with portability issues) just disable TCP keepalives for all listeners used for speaking the NATS protocol.
Change the tests so we test the same logic. Do not change HTTP monitoring, profiling, or the websocket API listeners.
Change KeepAlive on client connections too.
This change allows the removal of the connection and update of
the server state to be done "in place" but still delay the flushing
of and close of tcp connection to the writeLoop. With ref counting
we ensure that the reconnect happens after the flushing but not
before the state has been updated.
Had to fix some places where we may have called closeConnection()
from under the server lock since it now would deadlock for sure.
Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
We cannot call c.closeConnection() under the server lock because
closeConnection() can invoke server lock in some cases.
Created a test that should run without `-race` to reproduce the deadlock
(which it does) but sometimes would fail because cluster would not be
formed. This unconvered an issue with conflict resolution which
test TestRouteClusterNameConflictBetweenStaticAndDynamic() can reproduce
easily. The issue was that we were not updating a dynamic name with
the remote if the remote was non dynamic.
Resolves#1543
Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
If some servers in the cluster have the same connect URLs (due
to the use of client advertise), then it would be possible to
have a server sends the connect_urls INFO update to clients with
missing URLs.
Resolves#1515
Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
The call sendProtoNow() should not normally be used (only when
setting up a connection when the writeloop is not yet started and
server needs to send something before being able to start the
writeLoop.
Instead, code should use enqueueProto(). For this particular
case though, use queueOutbound() directly and add to the
producer's pcd map.
Also fixed other places where we were using queueOutbound() +
flushSignal() which is what enqueueProto is doing.
Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
This was discovered with the test TestLeafNodeWithGatewaysServerRestart
that was sometimes failing. Investigation showed that when cluster B
was shutdown, one of the server on A that had a connection from B
that just broke tried to reconnect (as part of reconnect retries of
implicit gateways) to a server in B that was in the process of shuting down.
The connection had been accepted but createGateway not called because
the server's running boolean had been set to false as part of the shutdown.
However, the connection was not closed so the server on A had a valid
connection to a dead server from cluster B. When the B cluster (now single
server) was restarted and a LeafNode connection connected to it, then
the gateway from B to A was created, that server on A did not create outbound
connection to that B server because it already had one (the zombie one).
So this PR strengthens the starting of accept loops and also make sure
that if a connection (all type of connections) is not accepted because
the server is shuting down, that connection is properly closed.
Since all accept loops had almost same code, made a generic function
that accept functions to call specific create connection functions.
Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
Leafnodes that formed clusters were partially supported. This adds proper support for origin cluster, subscription suppression and data message no echo for the origin cluster.
Signed-off-by: Derek Collison <derek@nats.io>
When a leafnode would connect with credentials that had permissions the spoke did not have a way of knowing what those were.
This could lead to being disconnected when sending subscriptions or messages to the hub which were not allowed.
Signed-off-by: Derek Collison <derek@nats.io>
This was found due to a recent test that was flapping. The test
was not checking the correct server for leafnode connection, but
that uncovered the following bug:
When a leafnode connection is solicited, the read/write loops are
started. Then, the connection lock is released and several
functions invoked to register the connection with an account and
add to the connection leafs map.
The problem is that the readloop (for instance) could get a read
error and close the connection *before* the above said code
executes, which would lead to a connection incorrectly registered.
This could be fixed either by delaying the start of read/write loops
after the registration is done, or like in this PR, check the
connection close status after registration, and if closed, manually
undoing the registration with account/leafs map.
Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
There is a race between the time the processing of a subscription
and the init/send of subscriptions when accepting a leaf node
connection that may cause internally a subscription's subject
to be counted many times, which would then prevent the send of
an LS- when the subscription's interest goes away.
Imagine this sequence of events, each side represents a "thread"
of execution:
```
client readLoop leaf node readLoop
----------------------------------------------------------
recv SUB foo 1
sub added to account's sublist
recv CONNECT
auth, added to acc.
updateSmap
smap["foo"]++ -> 1
no LS+ because !allSubsSent
init smap
finds sub in acc sl
smap["foo"]++ -> 2
sends LS+ foo
allSubsSent == true
recv UNSUB 1
updateSmap
smap["foo"]-- -> 1
no LS- because count != 0
----------------------------------------------------------
```
Equivalent result but with slightly diffent execution:
```
client readLoop leaf node readLoop
----------------------------------------------------------
recv SUB foo 1
sub added to account's sublist
recv CONNECT
auth, added to acc.
init smap
finds sub in acc sl
smap["foo"]++ -> 1
sends LS+ foo
allSubsSent == true
updateSmap
smap["foo"]++ -> 2
no LS+ because count != 1
recv UNSUB 1
updateSmap
smap["foo"]-- -> 1
no LS- because count != 0
----------------------------------------------------------
```
The approach for the fix is delay the creation of the smap
until we actually initialize the map and send the subs on processing
of the CONNECT.
In the meantime, as soon as the LN connection is registered
and available in updateSmap, we check that smap is nil or
not. If nil, we do nothing.
In "init smap" we keep track of the subscriptions that have been
added to smap. This map will be short lived, just enough to
protect against races above.
In updateSmap, when smap is not nil, we need to checki, if we
are adding, that the subscription has not already been handled.
The tempory subscription map will be ultimately emptied/set to
nil with the use of a timer (if not emptied in place when
processing smap updates).
Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
If a leafnode connection is accepted but the server is shutdown
before the connection is fully registered, the shutdown would
stall because read and write loop go routine would not be
stopped.
Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
