negz
commented
1 year ago Thanks for spotting this @sttts. At first glance I agreed with your assessment, but after digging a little deeper I think we're okay. Or more specifically, I don't think we're breaking optimistic concurrency anywhere that it matters much.
As described in https://github.com/crossplane/crossplane/issues/4047 we have two applicator implementations - APIUpdatingApplicator and APIPatchingApplicator. I just did a search to refresh my memory on all the places we use these.
APIUpdatingApplicator
https://github.com/search?q=org%3Acrossplane%20NewAPIUpdatingApplicator&type=code
The APIUpdatingApplicator definitely does break optimistic concurrency. This is dangerous, but I believe we only use it in places today where that's inconsequential. We use it to intentionally clobber other state, in cases where a resource Crossplane owns should always be synced from the state of another resource and where we don't expect other writers. In all cases we don't use a get-mutate-apply pattern - we only use it to build desired state (a "sparse object") and apply it.
I found the following uses of APIUpdatingApplicator:
- RBAC manager controllers - use it to render and apply RBAC Roles and ClusterRoles.
- XRD controller - uses it to render and apply the CRDs that power XRs and XRCs.
- ProviderConfig controller - uses it to render and apply a ProviderConfigUsage.
- Claim controller - uses it to render and apply a connection secret.
In these cases I wouldn't expect other clients or controllers to be writing to the resources Crossplane applies, and if they did I think it's reasonable that Crossplane would undo their changes.
APIPatchingApplicator
https://github.com/search?q=org%3Acrossplane+NewAPIPatchingApplicator&type=code
The example you point to in the claim controller uses the APIPatchingApplicator. I think this applicator does actually respect the resource version of the applied resource though, per https://github.com/crossplane/crossplane-runtime/pull/258. We send a "merge patch" which is really just our entire serialized desired state as the HTTP PATCH request body to the API server. So if we did a get-mutate-apply, I believe our HTTP request would contain the metadata.resourceVersion that we read and be rejected by the API server if it was stale. So, I think we're safe from data-loss in this much more widely used applicator.
That said, it will lead to a lot of retries and noise (per https://github.com/crossplane/crossplane/issues/2472), and can't delete fields that we no longer want (per https://github.com/crossplane/crossplane/issues/4047). I'd still like to replace it with something better.
I found the following uses of APIPatchingApplicator:
- The core init container - uses it to render and apply CRDs, package Lock, and optionally packages.
- Package reconciler - Uses it to render and apply package revisions.
- Kubernetes secret stores - Use it to render and apply connection secrets.
- Managed resource reconciler - Uses it to render and apply connection secrets.
- XR controller - Uses it to render and apply the XR and any composed resources during Composition.
- Claim controller - Uses it to render and apply the XR and claim.
github-actions[bot]
What happened?
Looking at the claim controller, updating the XR (
cpin the code), the following race with other actors in the system (controllers, user with kubectl) exists:cpcpwhatever it wants to updatecpcp.RV = current.RVAssuming between 1 and 3a some client updates the XR out of band, e.g. some other controller, or kubectl. 3c will wipe out that change, won't it?
Above is just one example for that issue. The apply func is widely used in Crossplane.
How can we reproduce it?
This will likely show up as subtle bugs as this is some kind of time travel, or controllers fighting for their desired state of the world and racing with their informers.
What environment did it happen in?
Crossplane version: main