improve ca cmp #10351
improve ca cmp #10351
Conversation
| } | ||
|
|
||
| if !services.CertAuthoritiesEquivalent(actual, expected) { | ||
| return trace.CompareFailed("cluster %v settings have been updated, try again", new.GetName()) |
There was a problem hiding this comment.
I realize that it's the exact same message as before, but this is only accurate if the rotation was indeed the only thing different between actual and expected. Would it be too much of a layer violation if we checked for that here?
There was a problem hiding this comment.
Not sure I follow. Are you suggesting we return a different error message based on which elements of the CAs differ?
There was a problem hiding this comment.
E.g. check rotation state and return something like "rotation state concurrently updated"?
There was a problem hiding this comment.
Yes, and maybe leave the "try again" as that message is displayed verbatim by tctl on a manual auth - whereas if something bad has happened and we are never actually going to recover by waiting, there's no point in telling the user to try again.
There was a problem hiding this comment.
Admittedly the fix could be to actually catch and replace the error on one of the layers between this and the user instead
fspmarshall
force-pushed
the
fspmarshall/ca-cmp
branch
from
1611641 to
99067f2
Compare
|
@fspmarshall Looks like this is approved, can we merge? |
* improve ca cmp (#10351) * Fix panic in `CertAuthority.Clone` because of non-UTC times. (#12057) Co-authored-by: Forrest <[email protected]>
* improve ca cmp (#10351) * Fix panic in `CertAuthority.Clone` because of non-UTC times. (#12057) Co-authored-by: Forrest <[email protected]>
* improve ca cmp (#10351) * Fix panic in `CertAuthority.Clone` because of non-UTC times. (#12057) Co-authored-by: Forrest <[email protected]>
The
CompareAndSwapCertAuthoritiesmethod currently relies upon deterministic serialization to function correctly. Until #10070 it was possible that serialization wouldn't actually be deterministic. After that PR was merged, @espadolini discovered another potential failure case. If a new field is added to the ca resource and the auth server must be downgraded for some reason, the comparison fails due to the new field being omitted.This PR changes
CompareAndSwapCertAuthoritiesto instead load the value that actually exists at the target key, perform comparison using deserialized values (avoiding downgrade issues), and pass the originalbackend.Itemback toBackend.CompareAndSwap(further mitigating the risk of non-deterministic serialization).