Enable map key sorting in utils.FastMarshal#10070
Conversation
This enables map key sorting during JSON serialization to ensure `CompareAndSwap` opertions work consistently if the resource in question has embedded map values (e.g. labels). Note that this is hypothetically a breaking change: it makes comparisons consistent, so if it saves a variant that consistently fails a `CompareAndSwap()`, it will... fail consistently rather than sometimes work. In practice, today only `CertificateAuthority` resources use `CompareAndSwap()` and under normal circumstances they never have labels attached. In the unlikely event that some resource is affected, a load/store should save it with sorted key ordering and fix the problem.
|
I think that I am pretty sure that we have never set any labels on |
|
I might be adding a label in the metadata of |
This attempts to mitigate compare-and-swap failures if CAs have multiple labels by logging a warning instructing users to re-save their CertificateAuthority resource.
|
That's reasonable, it now logs a warning if comparisons fail and multiple labels are present. |
| // sorting to ensure CompareAndSwap checks consistently succeed. | ||
| var SafeConfig = jsoniter.Config{ | ||
| EscapeHTML: false, | ||
| MarshalFloatWith6Digits: true, // will lose precession |
There was a problem hiding this comment.
Precession is a change in the orientation of the rotational axis of a rotating body.
I realize that this is taken right from the definition of jsoniter.ConfigFastest but maybe we don't need to also mirror the misspelled comments 😅
There was a problem hiding this comment.
oh geez I'm embarrassed that I missed the typo, will fix!
| if err != nil { | ||
| if trace.IsCompareFailed(err) { | ||
| if len(existing.GetMetadata().Labels) >= 2 { | ||
| log.Warn("compare-and-swap comparison failed on certificate authority with multiple labels; it may need to be re-saved to sort map keys") |
There was a problem hiding this comment.
Any warnings or errors we log should be actionable and easy to understand by an end user, not just developers.
If a user sees this warning message, what do we expect them to do about it?
- if we don't expect them to do anything, then it's probably not a warning
- if we do expect them to do something, let's make the message more clear as to what that is
There was a problem hiding this comment.
Fair point, I've changed it to now output a message like the following:
comparison failed on certificate authority with multiple labels; if this occurs consistently, try re-saving the resource: tctl get cert_authority/user/foo.example.com --with-secrets > ca.yaml && tctl create -f ca.yaml && rm ca.yaml
fspmarshall
left a comment
fspmarshall
left a comment
There was a problem hiding this comment.
LGTM. Determinism is always good.
For those who are curious, there is a "correct" way to compare and swap under non-deterministic serialization. CAs just didn't do it for some reason:
- Load existing value.
- Compare expected value to deserialized version of existing value.
- Call
Backend.CompareAndSwapwith the value loaded in step 1.
Simply having determinism is better/faster though.
5 participants
This enables map key sorting during JSON serialization to ensure
CompareAndSwapopertions work consistently if the resource in question has embedded map values (e.g. labels).Note that this is hypothetically a breaking change: it makes comparisons consistent, so if it saves a variant that consistently fails a
CompareAndSwap(), it will... fail consistently rather than sometimes work. In practice, today onlyCertificateAuthorityresources useCompareAndSwap()and under normal circumstances they never have labels attached.In the unlikely event that some resource is affected, a load/store should save it with sorted key ordering and fix the problem.