GODRIVER-3054 Handshake connection should not use legacy for LB#1482
GODRIVER-3054 Handshake connection should not use legacy for LB#1482prestonvasquez merged 11 commits intomongodb:v1from
Conversation
API Change ReportNo changes found! |
|
Closing until we get more information from the reporter. |
| } | ||
|
|
||
| if isLegacyHandshake(h.serverAPI, h.d) { | ||
| if isLegacyHandshake(h.serverAPI, h.loadBalanced) { |
There was a problem hiding this comment.
h.loadBalanced is set through the client options / URI query parameter.
|
Ah, it looks like there is a relevant failure: |
d99bc98
|
|
||
| // 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) |
There was a problem hiding this comment.
@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 .
kevinAlbs
left a comment
There was a problem hiding this comment.
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.suiteRemove the original OP_MSG used for handshakes when loadBalanced is true test, since it is now redundant.
f2c80b8
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: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?