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.

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

GODRIVER-3054 Handshake connection should not use legacy for LB #1482

Merged
merged 11 commits into from Dec 4, 2023

Conversation

prestonvasquez
Copy link
Collaborator

@prestonvasquez prestonvasquez commented Nov 28, 2023

GODRIVER-3054

Summary

Prevent handshake connection from using legacy OP_QUERY per the specifications.

Background & Motivation

It appears that the Go Driver has never correctly used OP_MSG for load-balanced handshake connections. Prior to the proposal in this PR, the driver relied on the server description to determine if the server was load balanced for the initial handshake. However, this breaks since the topology has not been discovered yet and is defaulted to Single. Rather than using a server description, the driver should rely on the value set on the client options for LoadBalanced. This should be safe because if you attempt to set the client option for LoadBalanced on a server that is not load balanced, the driver will error:

error pinging MongoDB: connection() error occurred during connection handshake: driver attempted to initialize in load balancing mode, but the server does not support this mode

This logic was originally added in #847 , which does include a test. However, the test does not actually check the initial handshake. Should we remove it?

@prestonvasquez prestonvasquez requested a review from a team as a code owner November 28, 2023 23:00
@prestonvasquez prestonvasquez requested review from blink1073 and qingyang-hu and removed request for a team November 28, 2023 23:00
Copy link

API Change Report

No changes found!

@prestonvasquez
Copy link
Collaborator Author

Closing until we get more information from the reporter.

@@ -592,7 +592,7 @@ func (h *Hello) createOperation() driver.Operation {
ServerAPI: h.serverAPI,
}

if isLegacyHandshake(h.serverAPI, h.d) {
if isLegacyHandshake(h.serverAPI, h.loadBalanced) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

h.loadBalanced is set through the client options / URI query parameter.

blink1073
blink1073 previously approved these changes Nov 29, 2023
Copy link
Member

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

LGTM!

blink1073
blink1073 previously approved these changes Nov 30, 2023
qingyang-hu
qingyang-hu previously approved these changes Nov 30, 2023

// Per the specifications, if loadBalanced=true, drivers MUST use the hello
// command for the initial handshake and use the OP_MSG protocol.
assert.Equal(mt, "hello", handshakeMessage.CommandName)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@blink1073 @qingyang-hu The handshake specifications say that "If a server API version is requested or loadBalanced: True, drivers MUST use the hello command for the initial handshake and use the OP_MSG protocol." So we should always assert that "hello" is used as the command name in this case.

Changing this lead to the discovery that we were also setting the command name incorrectly on the initial handshake on LB'd servers. I've updated that logic in hello.go and relevant SDAM tests per DRIVERS-1929 .

blink1073
blink1073 previously approved these changes Nov 30, 2023
kevinAlbs
kevinAlbs previously approved these changes Dec 1, 2023
Copy link
Contributor

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

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

LGTM with a suggested addition to the Makefile:

This logic was originally added in #847 , which does include a test. However, the test does not actually check the initial handshake.

Running the original OP_MSG used for handshakes when loadBalanced is true test on the master branch fails with this error:

Error Trace:    /Users/kevin.albertson/code/mongo-go-driver/mongo/integration/client_test.go:776
                                        /Users/kevin.albertson/code/mongo-go-driver/mongo/integration/mongotest.go:266
Error:          Not equal: 
                expected: "hello"
                actual  : "isMaster"

I expect the test never passed, and it was undiscovered due to the load balancer Evergreen task not running the test.


I suggest: add TestLoadBalancedConnectionHandshake to the evg-test-load-balancers makefile target to run in Evergreen tasks for load balanced:

go test $(BUILD_TAGS) ./mongo/integration -run TestLoadBalancedConnectionHandshake -v -timeout $(TEST_TIMEOUT)s >> test.suite

Remove the original OP_MSG used for handshakes when loadBalanced is true test, since it is now redundant.

kevinAlbs
kevinAlbs previously approved these changes Dec 1, 2023
@prestonvasquez prestonvasquez merged commit a8fa12a into mongodb:v1 Dec 4, 2023
29 of 37 checks passed
@prestonvasquez prestonvasquez deleted the GODRIVER-3054 branch December 4, 2023 19:12
blink1073 pushed a commit to blink1073/mongo-go-driver that referenced this pull request Dec 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants