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

qingyang-hu · 2022-10-28T01:15:33Z

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/url package 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.

matthewdale · 2022-10-28T02:29:29Z

x/mongo/driver/topology/polling_srv_records_test.go

@@ -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"


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?

matthewdale · 2022-10-28T02:36:15Z

x/mongo/driver/topology/topology.go

@@ -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)


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?

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.

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

matthewdale · 2022-10-28T02:36:39Z

x/mongo/driver/topology/topology.go

+			// 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])


This refactor is a great idea!

benjirewis

LGTM % Matt's comment. We could just add a separate TestPollingSRVRecordsUserPass test and keep the existing test as-is. Thanks! 🧑‍🔧

…username/password.

prestonvasquez

LGTM

x/mongo/driver/topology/polling_srv_records_test.go

prestonvasquez · 2022-10-28T17:55:39Z

x/mongo/driver/topology/topology.go

+		if len(parsedHosts) != 1 {
+			return fmt.Errorf("URI with SRV must include one and only one hostname")
+		}
+		_, _, err = net.SplitHostPort(parsedHosts[0])


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.

prestonvasquez · 2022-10-28T17:56:04Z

x/mongo/driver/topology/topology.go

+			// 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])


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.

Co-authored-by: Preston Vasquez <24281431+prestonvasquez@users.noreply.github.com>

kevinAlbs

LGTM, great find!

x/mongo/driver/topology/polling_srv_records_test.go

Co-authored-by: Kevin Albertson <kevin.albertson@10gen.com>

matthewdale

Looks good 👍

matthewdale · 2022-10-28T19:16:43Z

x/mongo/driver/topology/topology.go

@@ -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)


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 <24281431+prestonvasquez@users.noreply.github.com> Co-authored-by: Kevin Albertson <kevin.albertson@10gen.com>

GODRIVER-2620 Fix hostname parsing for SRV polling.

cdaa715

qingyang-hu requested review from prestonvasquez, matthewdale, benjirewis and kevinAlbs October 28, 2022 02:11

matthewdale reviewed Oct 28, 2022

View reviewed changes

benjirewis approved these changes Oct 28, 2022

View reviewed changes

GODRIVER-2620 Enhance test cases to cover URIs both with and without …

7ad50fe

…username/password.

prestonvasquez approved these changes Oct 28, 2022

View reviewed changes

qingyang-hu and others added 2 commits October 28, 2022 14:27

GODRIVER-2620 Minor polish.

e675205

Label test helper.

5b180ca

Co-authored-by: Preston Vasquez <24281431+prestonvasquez@users.noreply.github.com>

kevinAlbs approved these changes Oct 28, 2022

View reviewed changes

x/mongo/driver/topology/polling_srv_records_test.go Show resolved Hide resolved

qingyang-hu and others added 2 commits October 28, 2022 14:33

Comment non-specification-required test.

19a4154

Co-authored-by: Kevin Albertson <kevin.albertson@10gen.com>

Indent code

eae97a7

matthewdale approved these changes Oct 28, 2022

View reviewed changes

qingyang-hu merged commit e4853fb into mongodb:master Oct 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

qingyang-hu commented Oct 28, 2022

matthewdale Oct 28, 2022

matthewdale Oct 28, 2022

qingyang-hu Oct 28, 2022 •

edited

matthewdale Oct 28, 2022

matthewdale Oct 28, 2022

benjirewis left a comment •

edited

prestonvasquez left a comment

prestonvasquez Oct 28, 2022

prestonvasquez Oct 28, 2022

kevinAlbs left a comment

matthewdale left a comment

matthewdale Oct 28, 2022

	_, _, err = net.SplitHostPort(parsedHosts[0])
	_, _, err = net.SplitHostPort(uri.Host)

	go t.pollSRVRecords(parsedHosts[0])
	go t.pollSRVRecords(uri.Host)

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

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

Conversation

qingyang-hu commented Oct 28, 2022

Summary

Background & Motivation

matthewdale Oct 28, 2022

Choose a reason for hiding this comment

matthewdale Oct 28, 2022

Choose a reason for hiding this comment

qingyang-hu Oct 28, 2022 • edited

Choose a reason for hiding this comment

matthewdale Oct 28, 2022

Choose a reason for hiding this comment

matthewdale Oct 28, 2022

Choose a reason for hiding this comment

benjirewis left a comment • edited

Choose a reason for hiding this comment

prestonvasquez left a comment

Choose a reason for hiding this comment

prestonvasquez Oct 28, 2022

Choose a reason for hiding this comment

prestonvasquez Oct 28, 2022

Choose a reason for hiding this comment

kevinAlbs left a comment

Choose a reason for hiding this comment

matthewdale left a comment

Choose a reason for hiding this comment

matthewdale Oct 28, 2022

Choose a reason for hiding this comment

qingyang-hu Oct 28, 2022 •

edited

benjirewis left a comment •

edited