hashicorp / hashicorp/consul

api.Lock fails to notice lock has been lost for minutes when partitioned

Open
#18,271 1 comment 0 reactions 0 assignees View on GitHub
type/bug
Dominant language
Go
Stars
30.1k
Forks
4.6k
Avg merge
2d 6h
Merged PRs (30d)
43

Description

I found this while testing Vault's usage of `api.Lock` specifically but it is not doing anything unusual compared with our API.

If the node holding the lock is partitioned from Consul in such a way that all packets are dropped (i.e. it doesn't see TCP RST) then despite setting TTL or LockDelay to small numbers of seconds, the node will not notice it is no longer holding the lock for multiple minutes on most operating systems.

I'll use an example with `DefaultLockSessionTTL=15s` and `DefaultLockWaitTime=15s.`. My expectation is that if the node was partitioned from consul for more than 15 seconds it would realise it could no longer hold the lock and release it.

What actually happens is:

1. `RenewSession` is attempted every 7.5 seconds (`DefaultLockSessionTTL/2`). Lets assume there is no idle connection to Consul in the connection pool (best case) and so this will start failing within 7.5 seconds (assuming CPU is able to schedule fast enough etc.). However we don't do anything except log these errors! They are not checked by `monitorLock` and despite knowing after two failures that we can't possible still hold the lock, this is not signalled to the application.
2. `monitorLock` meanwhile will most likely be sat waiting on the KV blocking query to return. The return of this blocking query is the only signal that currently causes us to close the leader lock. There are no read deadlines in use so if the node is just not receiving any packets then this read will block without noticing the partition until Go's http client default TCP keepalive detects the TCP connection is unresponsive. If you work out how long that will take (`30s` set by go http client * `TCP_KEEPCNT` which is 9, 8 and 5 by default on linux, mac and windows respectively) it's about 5m on linux, 4.5 on mac and 3 on windows best case (idle timers also factor in). I corroborated that calculation roughly on my Mac measuring 5m 40s between inducing a partition and the application being notified that the lock was lost.

## Proposal

I think the simplest and lowest risk fix would be to actually use the failures to `RenewSession` as a signal to step down.

It would be possible to consider setting shorter timeouts on the blocking query but they would need to actually unblock the query and have consul return a response every 15s or less to have equivalent effect - simply setting read deadlines of 15s on a blocking query of 5 mins wouldn't work. This is likely to significantly increase cost of locks in terms of consul CPU and bandwidth, especially if the application writes a large value (say 10KiB) into the lock KV and then effectively re-fetches it every 15 seconds or less.

If `RenewSession` maintained a conservative estimate of the last time it was able renew the session, then it seems reasonable that `monitorLock` should somehow check that periodically and close the leader channel to signal loss if we ever exceed the session TTL without a successful update. In almost all cases Consul will have already released the lock due to not getting timely TTL updates anyway around this time. There is a small chance that a session renew request actually did make it to Consul but was lost on the return journey which could cause the application to step down earlier than Consul release it's lock but that seems like a more reasonable tradeoff than unbounded delay where the lock holder assumes it is still holding the lock even though it hasn't been able to maintain the session for many times the session TTL.

Testing this in unit tests will be somewhat difficult as it requires control over the network stack to some degree. I am working on integration tests in Vault that validate this scenario though so we can potentially use those as a way to verify the change in behaviour.

## Meta

I've labelled this issue as a "bug" although it's always worked this way because it seems surprising given the documentation of Session TTL and LockDelay that there are cases where a node would think it was the leader for many times longer than either setting even when it's network partitioned. But feel free to change that label if you disagree!

This might be something I can contribute a PR for soon but I'd also be very happy to help someone else contribute.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.