GODRIVER-2620 Fix hostname parsing for SRV polling. #1112
Conversation
qingyang-hu
requested review from
prestonvasquez,
matthewdale,
benjirewis and
kevinAlbs
| @@ -127,7 +127,7 @@ func compareHosts(t *testing.T, received []description.Server, expected []string | |||
| func TestPollingSRVRecordsSpec(t *testing.T) { | |||
| for _, tt := range srvPollingTests { | |||
| t.Run(tt.name, func(t *testing.T) { | |||
| uri := "mongodb+srv://test1.test.build.10gen.cc/?heartbeatFrequencyMS=100" | |||
| uri := "mongodb+srv://user:pass@test1.test.build.10gen.cc/?heartbeatFrequencyMS=100" | |||
There was a problem hiding this comment.
Only testing with a URI containing a username/password may cause the same lack of test coverage as there is currently, but for URIs with no username/password.
Is there a way we can test with multiple URI formats? Could we include the URI in the test struct and add test cases for both with and without username/password?
| @@ -234,7 +236,22 @@ func (t *Topology) Connect() error { | |||
|
|
|||
| t.serversLock.Unlock() | |||
| if t.pollingRequired { | |||
| go t.pollSRVRecords() | |||
| uri, err := url.Parse(t.cfg.URI) | |||
There was a problem hiding this comment.
I'm concerned we don't have enough test coverage of MongoDB connection string formats in the SRV polling tests to assert that it's safe to use url.Parse.
Is every mongodb+srv:// connection string parseable with url.Parse?
There was a problem hiding this comment.
The package doc says url.Parse parses [scheme:][//[userinfo@]host][/]path[?query][#fragment].
It has been proven that mongodb+srv is parseable as a legal scheme, and covered in the test cases.
Mongo host in the connection string may be different from ordinary hostnames, e.g. a comma-separated host list. However, it is illegal for SRV, and should be sanity checked by topology.NewConfig before being passed in Connect().
Also, we don't need to parse the path string in this case so it doesn't impact us even though we may have special cases in the query parameters.
There was a problem hiding this comment.
Sounds good. I also tested out using url.Parse to parse a bunch of connection string formats and they all worked correctly (see here).
x/mongo/driver/topology/topology.go
Outdated
| // but should not be able to when using SRV | ||
| return fmt.Errorf("URI with srv must not include a port number") | ||
| } | ||
| go t.pollSRVRecords(parsedHosts[0]) |
There was a problem hiding this comment.
This refactor is a great idea!
…username/password.
x/mongo/driver/topology/topology.go
Outdated
| if len(parsedHosts) != 1 { | ||
| return fmt.Errorf("URI with SRV must include one and only one hostname") | ||
| } | ||
| _, _, err = net.SplitHostPort(parsedHosts[0]) |
There was a problem hiding this comment.
| _, _, err = net.SplitHostPort(parsedHosts[0]) | |
| _, _, err = net.SplitHostPort(uri.Host) |
[optional]: IMO this code reads better by using the uri to get the host, not the slice used in the sanity check.
x/mongo/driver/topology/topology.go
Outdated
| // but should not be able to when using SRV | ||
| return fmt.Errorf("URI with srv must not include a port number") | ||
| } | ||
| go t.pollSRVRecords(parsedHosts[0]) |
There was a problem hiding this comment.
| go t.pollSRVRecords(parsedHosts[0]) | |
| go t.pollSRVRecords(uri.Host) |
[optional]: IMO this code reads better by using the uri to get the host, not the slice used in the sanity check.
Co-authored-by: Preston Vasquez <[email protected]>
Co-authored-by: Kevin Albertson <[email protected]>
| @@ -234,7 +236,22 @@ func (t *Topology) Connect() error { | |||
|
|
|||
| t.serversLock.Unlock() | |||
| if t.pollingRequired { | |||
| go t.pollSRVRecords() | |||
| uri, err := url.Parse(t.cfg.URI) | |||
There was a problem hiding this comment.
Sounds good. I also tested out using url.Parse to parse a bunch of connection string formats and they all worked correctly (see here).
* GODRIVER-2620 Fix hostname parsing for SRV polling. Co-authored-by: Preston Vasquez <[email protected]> Co-authored-by: Kevin Albertson <[email protected]>
* GODRIVER-2620 Fix hostname parsing for SRV polling. Co-authored-by: Preston Vasquez <[email protected]> Co-authored-by: Kevin Albertson <[email protected]>
* GODRIVER-2620 Fix hostname parsing for SRV polling. Co-authored-by: Preston Vasquez <[email protected]> Co-authored-by: Kevin Albertson <[email protected]>
GODRIVER-2620
Summary
Fix hostname parsing for SRV polling.
Background & Motivation
The original code parses the SRV hostname incorrectly for any URI with a username and password. This PR extracts the hostname from a URI using the
net/urlpackage instead of manual parsing and finishes all sanity checks before passing the hostname.However, I couldn't find a decent way to pass the errors from inside the polling out.