fix discovery overwriting dynamic resources

[GavinFrazar]

Changelog: Prevent discovery service from overwriting Teleport dynamic resources that have the same name as discovered resources.

Closes #29828

This fix relies on checking the existing resource to see if we can update it - when we add optimistic locking for db/kube_cluster/app then we can do a conditional update. It's very unlikely that a user will update the resource during such a small time window anyway. I also consider using conditional updates out of scope to fix the linked issue because it's a fundamentally different problem.

[rosstimothy]

when we add optimistic locking for db/kube_cluster/app then we can do a conditional update. It's very unlikely that a user will update the resource during such a small time window anyway. I also consider using conditional updates out of scope to fix the linked issue because it's a fundamentally different problem.

There are no plans to extend optimistic locking to presence related resources. Conditional writes are a more expensive operation than a plain write and at scale can(and have) taken down a backend. Optimistic locking is only intended to be leveraged for cluster configuration(roles, users, auth preference, auth connectors, etc.) that may accidentally be simultaneously updated by humans.

[rosstimothy]

How come we opted not to proceed with your suggestion from #29828? I think the Create, Get, Update sequence introduced here could pose other issues since none of the operations are atomic and we may be operating on outdated state by the time the Get on creation failure occurs.

[GavinFrazar]

How come we opted not to proceed with your suggestion from #29828? I think the Create, Get, Update sequence introduced here could pose other issues since none of the operations are atomic and we may be operating on outdated state by the time the Get on creation failure occurs.

I opted to not do that because the reconciler would have to be changed to avoid deleting current resources in a different discovery_group or of a different origin - it seemed to me like that behavior was too specific to put in reconciler code and it would affect a ton of things that use the reconciler.

If I did do that then we would still be operating on potentially outdated state, right?
Reconciler could call GetCurrentResources(), then one of those resources is updated before it takes some action like create/delete/update.

edit:
Another motivating reason that I forgot to include before, is that if I change the reconciler config such that it gets unfiltered resources, then it could still have a resource created during the time window between fetching current resources and creating a new resource. If that happens, then it needs to have special handling in "OnCreate" to handle AlreadyExists, and then we're right back to this solution but with a bunch of extra code to handle the unfiltered resources and avoid deleting resources that aren't managed by discovery.

[public-teleport-github-review-bot]

@GavinFrazar See the table below for backport results.

Branch	Result
branch/v14	Failed
branch/v15	Create PR
branch/v16	Create PR

[milos-teleport]

Example error if someone stumbles upon this with v14 or earlier

2024-01-01T15:20:00Z WARN [DISCOVERY] Unable to reconcile database resources. error:[
ERROR REPORT:
Original Error: trace.aggregate failed to create db dbname
database "dbname" doesn't exist

Solution: upgrade to at least v15.4.12