Allow expressed url parameters to influence implicit servers. #409
Conversation
When a connected server returns information on other server, we can selectively use the information. Two cases here. 1. When the connected server was expressed as a hostname and we are doing TLS but the other implicit servers are bare IPs. Use the hostname during TLS. 2. When a connected server url had username and password, we can re-use those on implicit servers if it was not already defined. Signed-off-by: Derek Collison <[email protected]>
nats.go
Outdated
| } | ||
| // If its blank we will override it with the current host | ||
| if tlsCopy.ServerName == _EMPTY_ { | ||
| s := nc.srvPool[0] |
There was a problem hiding this comment.
0 for reconnects correspond indeed to the current url in the pool, but not in the case of initial connect. I wonder if instead of having nc.url for the current url we should have stored nc.srv for the current *srv, from which we can then access url or tlsName.
There was a problem hiding this comment.
Might be a good idea. Let me look into that.
There was a problem hiding this comment.
Confused. You did save current and replaced everywhere but still are using srvPool[0] here?
There was a problem hiding this comment.
uug, too early in the morning. Will fix.
nats.go
Outdated
| // the current hostname we are connected to. | ||
| if nc.Opts.Secure { | ||
| if addr := net.ParseIP(u.Hostname()); addr != nil { | ||
| if caddr := net.ParseIP(nc.url.Hostname()); caddr == nil { |
There was a problem hiding this comment.
Not big of a deal, but addURLToPool is called with implicit == true from the processing of INFO with an array of urls. So it could be that we do this parseIP check many times for the same nc.url. Wonder if this could be moved to processInfo instead?
There was a problem hiding this comment.
You mean the one about the current connected correct? We could move it but we need the tlsName for when we create the *srv struct.
There was a problem hiding this comment.
processInfo() is the only one to call addURLToPool with implicit == true. processInfo runs under connection lock (when processing async INFO). The current nc.url cannot change while this processing is taking place, so I was suggesting that this net.ParseIP(nc.url.Hostname() be done once before the loop that calls addURLToPool(). But again, no big deal.
There was a problem hiding this comment.
Well I may look to hoist *srv instead of just url to nc.
| @@ -0,0 +1,21 @@ | |||
| # TLS config file | |||
| # Cert has no IPs, so we use an IP here to simulate a single hostname cluster. | |||
There was a problem hiding this comment.
Travis also has support to set custom hostnames which may help to simulate setup further https://docs.travis-ci.com/user/hosts/
There was a problem hiding this comment.
Nice I did not know that! Would prefer a test that can be run locally as well. What is best way to have a test understand if it is running on Travis?
There was a problem hiding this comment.
I think we could check whether os.Getenv("TRAVIS") == "true" which is one of the default environment variables and skip locally if not defined for example.
There was a problem hiding this comment.
Good point. For now I think we are ok, we are using cert with localhost and no IPs defined, so is testing what we want.
Signed-off-by: Derek Collison <[email protected]>
nats.go
Outdated
| } | ||
| // If its blank we will override it with the current host | ||
| if tlsCopy.ServerName == _EMPTY_ { | ||
| s := nc.srvPool[0] |
There was a problem hiding this comment.
Confused. You did save current and replaced everywhere but still are using srvPool[0] here?
test/reconnect_test.go
Outdated
| @@ -649,3 +652,73 @@ func TestReconnectBufSize(t *testing.T) { | |||
| } | |||
| nc.Buffered() | |||
| } | |||
|
|
|||
| // When a cluster is fronted by a single DNS name (desired) but communicates IPs to clients (also desired), | |||
| // and we TLS, we want to make sure we do the right thing connecting to an IP directly for TLS to work. | |||
Signed-off-by: Derek Collison <[email protected]>
When a connected server returns information on other server, we can selectively use the information.
Two cases:
the other implicit servers are bare IPs. Use the hostname during TLS.
on implicit servers if it was not already defined.
Signed-off-by: Derek Collison [email protected]