Skip to content

Commit 92a7433

Browse files
ShaneHarveyShaneHarvey
authored
PYTHON-2580 Provide explicit guidance on handling command errors during the handshake (#571)
Command errors during the handshake MUST use SDAM error handling rules. Mark server unknown after auth failures. Test network timeout errors pre/post auth. PoolClearedError MUST NOT mark the server Unknown. Add "authEnabled" runOn requirement for SDAM integration tests.
1 parent ec6337e commit 92a7433

File tree

9 files changed

+1155
-15
lines changed

9 files changed

+1155
-15
lines changed

‎pymongo/topology.py

Lines changed: 16 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -582,7 +582,7 @@ def _handle_error(self, address, err_ctx):
582582
elif issubclass(exc_type, WriteError):
583583
# Ignore writeErrors.
584584
return
585-
elif issubclass(exc_type, NotMasterError):
585+
elif issubclass(exc_type, (NotMasterError, OperationFailure)):
586586
# As per the SDAM spec if:
587587
# - the server sees a "not master" error, and
588588
# - the server is not shutting down, and
@@ -591,14 +591,23 @@ def _handle_error(self, address, err_ctx):
591591
# as Unknown and request an immediate check of the server.
592592
# Otherwise, we clear the connection pool, mark the server as
593593
# Unknown and request an immediate check of the server.
594-
err_code = error.details.get('code', -1)
595-
is_shutting_down = err_code in helpers._SHUTDOWN_CODES
596-
# Mark server Unknown, clear the pool, and request check.
597-
self._process_change(ServerDescription(address, error=error))
598-
if is_shutting_down or (err_ctx.max_wire_version <= 7):
594+
if hasattr(error, 'code'):
595+
err_code = error.code
596+
else:
597+
err_code = error.details.get('code', -1)
598+
if err_code in helpers._NOT_MASTER_CODES:
599+
is_shutting_down = err_code in helpers._SHUTDOWN_CODES
600+
# Mark server Unknown, clear the pool, and request check.
601+
self._process_change(ServerDescription(address, error=error))
602+
if is_shutting_down or (err_ctx.max_wire_version <= 7):
603+
# Clear the pool.
604+
server.reset()
605+
server.request_check()
606+
elif not err_ctx.completed_handshake:
607+
# Unknown command error during the connection handshake.
608+
self._process_change(ServerDescription(address, error=error))
599609
# Clear the pool.
600610
server.reset()
601-
server.request_check()
602611
elif issubclass(exc_type, ConnectionFailure):
603612
# "Client MUST replace the server's description with type Unknown
604613
# ... MUST NOT request an immediate check of the server."
@@ -609,13 +618,6 @@ def _handle_error(self, address, err_ctx):
609618
# reading or writing`_, clients MUST cancel the isMaster check on
610619
# that server and close the current monitoring connection."
611620
server._monitor.cancel_check()
612-
elif issubclass(exc_type, OperationFailure):
613-
# Do not request an immediate check since the server is likely
614-
# shutting down.
615-
if error.code in helpers._NOT_MASTER_CODES:
616-
self._process_change(ServerDescription(address, error=error))
617-
# Clear the pool.
618-
server.reset()
619621

620622
def handle_error(self, address, err_ctx):
621623
"""Handle an application error.

‎test/discovery_and_monitoring_integration/auth-error.json

Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,140 @@
1+
{
2+
"runOn": [
3+
{
4+
"minServerVersion": "4.4",
5+
"authEnabled": true
6+
}
7+
],
8+
"database_name": "sdam-tests",
9+
"collection_name": "auth-error",
10+
"data": [
11+
{
12+
"_id": 1
13+
},
14+
{
15+
"_id": 2
16+
}
17+
],
18+
"tests": [
19+
{
20+
"description": "Reset server and pool after AuthenticationFailure error",
21+
"failPoint": {
22+
"configureFailPoint": "failCommand",
23+
"mode": {
24+
"times": 1
25+
},
26+
"data": {
27+
"failCommands": [
28+
"saslContinue"
29+
],
30+
"appName": "authErrorTest",
31+
"errorCode": 18
32+
}
33+
},
34+
"clientOptions": {
35+
"retryWrites": false,
36+
"appname": "authErrorTest"
37+
},
38+
"operations": [
39+
{
40+
"name": "insertMany",
41+
"object": "collection",
42+
"arguments": {
43+
"documents": [
44+
{
45+
"_id": 3
46+
},
47+
{
48+
"_id": 4
49+
}
50+
]
51+
},
52+
"error": true
53+
},
54+
{
55+
"name": "waitForEvent",
56+
"object": "testRunner",
57+
"arguments": {
58+
"event": "ServerMarkedUnknownEvent",
59+
"count": 1
60+
}
61+
},
62+
{
63+
"name": "waitForEvent",
64+
"object": "testRunner",
65+
"arguments": {
66+
"event": "PoolClearedEvent",
67+
"count": 1
68+
}
69+
},
70+
{
71+
"name": "insertMany",
72+
"object": "collection",
73+
"arguments": {
74+
"documents": [
75+
{
76+
"_id": 5
77+
},
78+
{
79+
"_id": 6
80+
}
81+
]
82+
}
83+
},
84+
{
85+
"name": "assertEventCount",
86+
"object": "testRunner",
87+
"arguments": {
88+
"event": "ServerMarkedUnknownEvent",
89+
"count": 1
90+
}
91+
},
92+
{
93+
"name": "assertEventCount",
94+
"object": "testRunner",
95+
"arguments": {
96+
"event": "PoolClearedEvent",
97+
"count": 1
98+
}
99+
}
100+
],
101+
"expectations": [
102+
{
103+
"command_started_event": {
104+
"command": {
105+
"insert": "auth-error",
106+
"documents": [
107+
{
108+
"_id": 5
109+
},
110+
{
111+
"_id": 6
112+
}
113+
]
114+
},
115+
"command_name": "insert",
116+
"database_name": "sdam-tests"
117+
}
118+
}
119+
],
120+
"outcome": {
121+
"collection": {
122+
"data": [
123+
{
124+
"_id": 1
125+
},
126+
{
127+
"_id": 2
128+
},
129+
{
130+
"_id": 5
131+
},
132+
{
133+
"_id": 6
134+
}
135+
]
136+
}
137+
}
138+
}
139+
]
140+
}

‎test/discovery_and_monitoring_integration/auth-misc-command-error.json

Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,140 @@
1+
{
2+
"runOn": [
3+
{
4+
"minServerVersion": "4.4",
5+
"authEnabled": true
6+
}
7+
],
8+
"database_name": "sdam-tests",
9+
"collection_name": "auth-misc-error",
10+
"data": [
11+
{
12+
"_id": 1
13+
},
14+
{
15+
"_id": 2
16+
}
17+
],
18+
"tests": [
19+
{
20+
"description": "Reset server and pool after misc command error",
21+
"failPoint": {
22+
"configureFailPoint": "failCommand",
23+
"mode": {
24+
"times": 1
25+
},
26+
"data": {
27+
"failCommands": [
28+
"saslContinue"
29+
],
30+
"appName": "authMiscErrorTest",
31+
"errorCode": 1
32+
}
33+
},
34+
"clientOptions": {
35+
"retryWrites": false,
36+
"appname": "authMiscErrorTest"
37+
},
38+
"operations": [
39+
{
40+
"name": "insertMany",
41+
"object": "collection",
42+
"arguments": {
43+
"documents": [
44+
{
45+
"_id": 3
46+
},
47+
{
48+
"_id": 4
49+
}
50+
]
51+
},
52+
"error": true
53+
},
54+
{
55+
"name": "waitForEvent",
56+
"object": "testRunner",
57+
"arguments": {
58+
"event": "ServerMarkedUnknownEvent",
59+
"count": 1
60+
}
61+
},
62+
{
63+
"name": "waitForEvent",
64+
"object": "testRunner",
65+
"arguments": {
66+
"event": "PoolClearedEvent",
67+
"count": 1
68+
}
69+
},
70+
{
71+
"name": "insertMany",
72+
"object": "collection",
73+
"arguments": {
74+
"documents": [
75+
{
76+
"_id": 5
77+
},
78+
{
79+
"_id": 6
80+
}
81+
]
82+
}
83+
},
84+
{
85+
"name": "assertEventCount",
86+
"object": "testRunner",
87+
"arguments": {
88+
"event": "ServerMarkedUnknownEvent",
89+
"count": 1
90+
}
91+
},
92+
{
93+
"name": "assertEventCount",
94+
"object": "testRunner",
95+
"arguments": {
96+
"event": "PoolClearedEvent",
97+
"count": 1
98+
}
99+
}
100+
],
101+
"expectations": [
102+
{
103+
"command_started_event": {
104+
"command": {
105+
"insert": "auth-misc-error",
106+
"documents": [
107+
{
108+
"_id": 5
109+
},
110+
{
111+
"_id": 6
112+
}
113+
]
114+
},
115+
"command_name": "insert",
116+
"database_name": "sdam-tests"
117+
}
118+
}
119+
],
120+
"outcome": {
121+
"collection": {
122+
"data": [
123+
{
124+
"_id": 1
125+
},
126+
{
127+
"_id": 2
128+
},
129+
{
130+
"_id": 5
131+
},
132+
{
133+
"_id": 6
134+
}
135+
]
136+
}
137+
}
138+
}
139+
]
140+
}

0 commit comments

Comments
 (0)