[SERVER-38335] killAllSessions can kill itself if the command body contains a lsid field Created: 30/Nov/18 Updated: 29/Oct/23 Resolved: 04/Feb/19 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | Sharding |
| Affects Version/s: | 4.1.5 |
| Fix Version/s: | 4.1.8 |
| Type: | Bug | Priority: | Major - P3 |
| Reporter: | Randolph Tan | Assignee: | Randolph Tan |
| Resolution: | Fixed | Votes: | 1 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||
| Backwards Compatibility: | Fully Compatible | ||||||||||||
| Operating System: | ALL | ||||||||||||
| Sprint: | Sharding 2018-12-17, Sharding 2018-12-31, Sharding 2019-01-14, Sharding 2019-01-28, Sharding 2019-02-11 | ||||||||||||
| Participants: | |||||||||||||
| Description |
|
The command in mongos kills all sessions and then sends the same killAllSessions to all shards. If the matcher is empty and the killAllSessions command has a lsid field, it will also end up calling killOp on itself since it is technically using a session. The remote killAllSessions call to every shard will still succeed since it is using a different operation context than the original command, but the original command will get interrupted early and will not wait for the remote commands to finish. This is kind of problematic for drivers (and mongo shell) since every request is using an implicit session. As a result, there is currently no reliable way to synchronously block and wait for killAllSessions to complete before proceeding. |
| Comments |
| Comment by Githook User [ 24/May/19 ] | ||||||||
|
Author: {'name': 'Jeremy Mikola', 'email': 'jmikola@gmail.com', 'username': 'jmikola'}Message: Note that drivers can ignore killAllSessions interruptions (#550) Prior to | ||||||||
| Comment by Githook User [ 04/Feb/19 ] | ||||||||
|
Author: {'name': 'Randolph Tan', 'email': 'randolph@10gen.com', 'username': 'renctan'}Message: This is to prevent it from killing itself. | ||||||||
| Comment by Misha Tyulenev [ 10/Dec/18 ] | ||||||||
|
kaloian.manassiev I agree with adding the shouldIgnoreSession property. This will allow keeping the authentication to run but avoid adding a session to the logical cache. In this case, we can simplify the initializeOperationSessionInfo by taking requiresAuth outside so those checks will not overlap. | ||||||||
| Comment by Kaloian Manassiev [ 10/Dec/18 ] | ||||||||
|
misha.tyulenev, this is not just about implementing an allowedInTransactions property, but more like shouldIgnoreSession, which if specified as true means that the command should not check-out the session and should behave as if it was not ran with a session. And initializeOperationSessionInfo will only be called when !command->shouldIgnoreSession() && requiresAuth. Is this what you had in mind? | ||||||||
| Comment by Misha Tyulenev [ 07/Dec/18 ] | ||||||||
|
The approach is safe but I dont support the implementation suggestion. I would rather suggest implementing a virtual bool allowedInTransactions() method and override it appropriately | ||||||||
| Comment by Shane Harvey [ 06/Dec/18 ] | ||||||||
|
I was not trying to suggest removing the auth requirement for killAllSessions, merely to skip parsing/creating the OperationSessionInfoFromClient similar to what is done for commands that don't require auth. Like this:
| ||||||||
| Comment by Randolph Tan [ 06/Dec/18 ] | ||||||||
|
Hm... killAllSessions command does require auth, doesn't it? Doesn't sound right to just make it return false. | ||||||||
| Comment by Misha Tyulenev [ 06/Dec/18 ] | ||||||||
|
kaloian.manassiev this approach may introduce a security problem as this will open this method to nonauthorized users.: | ||||||||
| Comment by Kaloian Manassiev [ 05/Dec/18 ] | ||||||||
|
Ah you are proposing that we do it at the server side so that the killAllSessions operation doesn't mark itself as running under a session, by returning early here, right? | ||||||||
| Comment by Shane Harvey [ 05/Dec/18 ] | ||||||||
|
I think there's a single place in the server where it could ignore the session related arguments for a killAllSessions command. The server already does the same for all commands that don't require auth. This should remove the need to ignore a session after the fact.
Yes, drivers do this for some commands but this only the case when there's a helper method, for example pymongo's parallel_scan helper (which runs parallelCollectionScan) in | ||||||||
| Comment by Kaloian Manassiev [ 05/Dec/18 ] | ||||||||
|
The killAllSessions command was intended to do exactly what its name says and that's to kill all sessions. I understand this might be inconvenient for the drivers, but making it skip the session under which it is running is not trivial. Given the implementation difficulty and generally that this command is allowed to accept parameters where the session under which it is run is included, I think that the most prudent thing is to disallow killAllSessions from running under a session. I believe the drivers already support that for other commands too, so can you just add that one as well? | ||||||||
| Comment by Shane Harvey [ 04/Dec/18 ] | ||||||||
|
Can we get this prioritized to ease the driver testing proposed in SPEC-1140? Omitting the implicit session or using killAllSessionsByPattern may be difficult in some drivers. |