integrations/operator: propagate revision + fix tests

[hugoShaka]

Depends on #34263

This PR does two things:

• ensure revision is properly propagated (this is required in v15, tests will fail if it is not. No additional coverage required, this will be covered when we'll revert Temporarily disable conditional updates for users #34255)
• fix tests causing definitive failures instead of temporary ones (failing instead of returning false in Eventually)

[github-actions]

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

[tigrato]

Shouldn't we use Upsert instead of create/update?
Upsert doesn't care about the revision which is more or less what we need here

[hugoShaka]

I asked @rosstimothy, and we likely want to propagate revision even when Upserting. IIRC Update and Upsert have different behaviours for User, and both endpoints are not implemented for all resources. We might want to support both endpoints with the same generic controller.

I don't see an issue with propagating the revision. Worst case scenario, it is not used. In the best case scenario, this prevents a conflict. For some resources we are propagating information from the existing resource, the whole process is not 100% stateless. This happens in the optional Mutate() function. For example, the "CreatedBy" field of the user spec. In this case, opportunistic locking makes sense.

[rosstimothy]

Using Upsert directly could lead to the operator overwriting any manual changes made to a resource, I'm not sure how likely that scenario is. It is generally preferred to use Create/Update where possible.

[marcoandredinis]

I think we must improve documentation.
When using IaC tools, it's not clear if users can change using other tools (web, tctl).

If they can't change, then the upsert is, imo, a better approach.
Even if they change the resource just to test something out, they can always go back to IaC flow.

If they can change the resource, the update will fail and they need to manually change the resource to merge any potential diff.

Have we decided on this?

[hugoShaka]

If they can change the resource, the update will fail and they need to manually change the resource to merge any potential diff.

With the current implementation, if a conflict happens, the operator update/upsert will fail and the resource reconciliation will be requeued. The operator will perform the update in the next reconciliation cycle, a few milliseconds later. The second reconciliation should succeed.

When using IaC tools, it's not clear if users can change using other tools (web, tctl).

We do have an origin label we are not using enough. The UI/tctl could display a warning if you are attempting to edit a resource whose origin is IaC. Things become even more confusing with resources like AccessLists where some of the information can and should be edited by users in the UI even if some parts of the resource are managed via IaC.

If they can't change, then the upsert is, imo, a better approach.
Even if they change the resource just to test something out, they can always go back to IaC flow.

AFAIK, if you do a change, TF/operator will erase it the next time, which is the intended behaviour.
My biggest issue today is not caused by users editing resources managed by IaC but caused by Teleport updating fields in the spec (which should not happen according to Kube's design, the status is here for computed values). Regardless if we use update or upsert, we need to propagate those dynamic fields that appear in the spec, or else we lose information.

Have we decided on this?

I don't think and can create an issue for this. AFAIK this PR doesn't change the current state of things, it only propagates the revision.

[public-teleport-github-review-bot]

@hugoShaka See the table below for backport results.

Branch	Result
branch/v12	Failed
branch/v13	Create PR
branch/v14	Create PR