Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

GODRIVER-2620 Fix hostname parsing for SRV polling. #1112

Merged
merged 6 commits into from Oct 28, 2022
Merged

GODRIVER-2620 Fix hostname parsing for SRV polling. #1112

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cdaa715
GODRIVER-2620 Fix hostname parsing for SRV polling.
qingyang-hu Oct 28, 2022
7ad50fe
GODRIVER-2620 Enhance test cases to cover URIs both with and without …
qingyang-hu Oct 28, 2022
e675205
GODRIVER-2620 Minor polish.
qingyang-hu Oct 28, 2022
5b180ca
Label test helper.
qingyang-hu Oct 28, 2022
19a4154
Comment non-specification-required test.
qingyang-hu Oct 28, 2022
eae97a7
Indent code
qingyang-hu Oct 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion x/mongo/driver/topology/polling_srv_records_test.go
View file
Open in desktop
Expand Up @@ -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"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

cfg, err := NewConfig(options.Client().ApplyURI(uri), nil)
require.NoError(t, err, "error constructing topology configs: %v", err)

Expand Down
28 changes: 19 additions & 9 deletions x/mongo/driver/topology/topology.go
View file
Open in desktop
Expand Up @@ -14,6 +14,8 @@ import (
"context"
"errors"
"fmt"
"net"
"net/url"
"strings"
"sync"
"sync/atomic"
Expand Down Expand Up @@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. I also tested out using url.Parse to parse a bunch of connection string formats and they all worked correctly (see here).

if err != nil {
return err
}
// sanity check before passing the hostname to resolver
parsedHosts := strings.Split(uri.Host, ",")
if len(parsedHosts) != 1 {
return fmt.Errorf("URI with SRV must include one and only one hostname")
}
_, _, err = net.SplitHostPort(parsedHosts[0])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
_, _, 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.

if err == nil {
// we were able to successfully extract a port from the host,
// 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])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This refactor is a great idea!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

t.pollingwg.Add(1)
}

Expand Down Expand Up @@ -556,7 +573,7 @@ func (t *Topology) selectServerFromDescription(desc description.Topology,
return suitable, nil
}

func (t *Topology) pollSRVRecords() {
func (t *Topology) pollSRVRecords(hosts string) {
defer t.pollingwg.Done()

serverConfig := newServerConfig(t.cfg.ServerOpts...)
Expand All @@ -573,13 +590,6 @@ func (t *Topology) pollSRVRecords() {
}
}()

// remove the scheme
uri := t.cfg.URI[14:]
hosts := uri
if idx := strings.IndexAny(uri, "/?@"); idx != -1 {
hosts = uri[:idx]
}

for {
select {
case <-pollTicker.C:
Expand Down