Unmarshal and compare user objects in CompareAndSwapUser#37660
Unmarshal and compare user objects in CompareAndSwapUser#37660espadolini merged 3 commits intomasterfrom
Conversation
|
This is barely changelog-worthy, since any modification to the user params item that doesn't go through |
codingllama
left a comment
codingllama
left a comment
There was a problem hiding this comment.
Thanks for tackling this, Edoardo!
| Key: backend.Key(webPrefix, usersPrefix, existing.GetName(), paramsPrefix), | ||
| Value: existingValue, | ||
| } | ||
| const iterationLimit = 5 |
There was a problem hiding this comment.
What is the rationale behind the retries? Are all ConditionalUpdate callers expected to retry? Why 5 times exactly?
There was a problem hiding this comment.
Revisions change because something else modified the item - but from the point of view of a compare-and-swap operation we don't care about that, we only care if the value matches or not, and a concurrent modification that leaves the value identical should still result in a successful compare-and-swap. In practice this isn't actually followed in some of our CompareAndSwapFoo implementations, unfortunately: CompareAndSwapCertAuthority can fail even though it should succeed if the existing value is concurrently overwritten with a different one that is equivalent after deserialization (in that specific case it's not a big deal because all cases of CASing a cert authority are already on various timed loops to begin with).
Whether or not revisions can also spuriously change is still not fully defined - with the more recent changes to firestorebk (to do transactions and store a hard revision value in the actual document rather than rely on lastModifiedTime), the only backend for which that's a possibility is etcdbk, where the backend revision of an item is the modrevision of the underlying etcd item, and that would change if a backup is restored while Teleport is running (which would be crazy, admittedly).
Oh, and I picked 5 because humans generally have 5 fingers on one hand, although this is such a fringe scenario that I'm going to change it to 3: the initial attempt, one retry because spurious failures could actually happen, and another one because a single retry just feels weird.
There was a problem hiding this comment.
and I picked 5 because humans generally have 5 fingers on one hand (...)
That's rock-solid logic, can't argue with that.
Thanks for the explanation. So what I'm gathering is that looped ConditionalUpdates are a practice we'd like to encourage, which is good to know.
Do you think it makes sense to capture the number of retries in a more generic, reusable constant, like ConditionalUpdateDefaultRetries (or something to that effect)?
There was a problem hiding this comment.
I don't know if there's a one-size-fits-all value for such a constant, I'm only doing a retry loop very close to the backend here because I want to match the slightly stronger semantics of compare-and-swap.
Co-authored-by: Alan Parra <alan.parra@goteleport.com>
public-teleport-github-review-bot
Bot
removed request for
ibeckermayer,
jentfoo and
r0mant
|
@espadolini See the table below for backport results.
|
3 participants
This PR makes it so that
CompareAndSwapUserdoes a comparison between unmarshaled user objects rather than comparing the JSON serialization through a backendCompareAndSwap. Seeing as this requires fetching the object anyway, the new implementation uses aConditionalUpdateoperation, which is generally cheaper.This fixes the failure condition reported at #35325 (comment) , but doesn't implement broader changes (to replace all the
CompareAndSwapUsercalls that almost always already get the current value anyway with calls toUpdateUser).changelog: fix conditional user modifications (used by certain Teleport subsystems such as Device Trust) on users that have previously been locked out due to repeated recovery attempts