switch trusted/remote cluster management to atomic write #48009
Conversation
fspmarshall
added
no-changelog
fspmarshall
force-pushed
the
fspmarshall/trust-atomics
branch
from
b0603a5 to
dd4ce9e
Compare
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
fspmarshall
force-pushed
the
fspmarshall/trust-atomics
branch
4 times, most recently
from
1051970 to
3bfc519
Compare
fspmarshall
force-pushed
the
fspmarshall/trust-atomics
branch
from
3bfc519 to
9b0530b
Compare
| if active { | ||
| if currentlyActive { |
There was a problem hiding this comment.
An explicit truth table like switch might make this a bit clearer and reduce some of the indentation
There was a problem hiding this comment.
Attempted. The reduced indentation was nice, but I found it overall a bit less readable, so leaving as-is for now.
| return condacts, nil | ||
| } | ||
|
|
||
| func updateCertAuthoritiesCondActs(cas []types.CertAuthority, active bool, currentlyActive bool) ([]backend.ConditionalAction, error) { |
There was a problem hiding this comment.
Are all of the possible conditional actions taken here covered in tests?
There was a problem hiding this comment.
added a separate dedicated test just for covering each case with and without a conflict.
public-teleport-github-review-bot
bot
removed request for
espadolini and
fheinecke
fspmarshall
force-pushed
the
fspmarshall/trust-atomics
branch
from
9b0530b to
65f7014
Compare
fspmarshall
force-pushed
the
fspmarshall/trust-atomics
branch
from
65f7014 to
f58c589
Compare
|
@fspmarshall See the table below for backport results.
|
| if existingCluster == nil { | ||
| return a.createTrustedCluster(ctx, tc) |
There was a problem hiding this comment.
Concurrent calls to UpsertTrustedCluster can result in an AlreadyExists error, which at the very least feels at least a bit wrong.
Are we already retrying every operation in the terraform provider no matter what the error is? Will we remember to do so for UpsertTrustedCluster if and when that comes to be automatable?
There was a problem hiding this comment.
So I tried adding retry to this logic, but ended up discarding it. Once you start to try to distinguish between AlreadyExists due to concurrent tc creation vs AlreadyExists due to conflicting CAs and handling CompareFailed from failed updates, it all starts to get a bit messy. My (potentially weak) justification for leaving as-is is that anything currently updating these in a racy fashion is already broken because it is likely to cause conflicting state (e.g. role map on CA disagreeing with role map in TC). Unexpected error seems like a strictly better outcome. I can take another stab at if you feel strongly about it, but right now I'm leaning toward just accepting that this method might yield unexpected errors during concurrent use.
There was a problem hiding this comment.
If you find yourself around the code maybe we should replace the AlreadyExists with a CompareFailed here in UpsertTrustedCluster? Otherwise it's not even worth the mental effort in opening a PR, I agree.
| var err error | ||
| existingCluster, err = a.GetTrustedCluster(ctx, trustedCluster.GetName()) | ||
| existingCluster, err = a.GetTrustedCluster(ctx, tc.GetName()) |
There was a problem hiding this comment.
Not wrong in the current implementation of (*Server).GetTrustedCluster but the typical error contract is that the return value must not be inspected in any way if the err is not nil - GetTrustedCluster might be accidentally updated to return a concrete type, which would make existingCluster always non-nil (but invalid) even in case of an error.
There was a problem hiding this comment.
Fixed in #48176
This PR switches creation/update/delete of trusted and remote clusters (and their associated resources) to use the
AtomicWriteAPI. Previously, the various checks and writes associated with these operations were performed sequentially, leading to various possible inconsistent states in the event of crashes, errors, or concurrent writes. With these changes, all relevant trust resources are now updated as a single atomic operation.