Avoid non-requeue for MRs with determinstic external names.

[nolancon]

Description of your changes

• Add a new Annotation crossplane.io/external-name-is-deterministic WithDeterministicExternalName reconciler option which allows the user to declare that a given resource is deterministically named by the external system.
• For MRs which are deterministically named, ignore the safeguard against leaked resources.

These changes enables the user to essentially "opt-out" of the current behaviour which blocks re-queuing of MRs in a scenario where creation is deemed incomplete. This safeguard is perfectly justified for MRs which are given non-deterministic names by the external system, preventing leaked resources (this scenario is described in detail here).

However, for resources which are deterministically named by the external system, the decision to not re-queue the MR is unnecessary as these resources are not in danger of leaking. For instance, provider-ceph creates S3 buckets by reconciling Bucket MRs - these s3 buckets are deterministically named and created idempotently. Therefore there is no danger in re-queuing after an initial "incomplete creation" failure (in fact it is expected). This kind of failure can (and does) happen regularly and in a system with 1000s of buckets leads to dozens of MRs which are not re-queued and therefore left in this pending state requiring manual intervention.

I have tested this patched version of the runtime against provider ceph. More details can be seen on this demo PR demo PR which also contains details on the existing workaround we use to attempt to mitigate the above scenario (but this is only best-effort and does not eradicate the problem completely).

Also, I'm happy to open a docs PR on this but would like to get feedback on this approach first, thanks.

Fixes #
#340 (maybe not fixes, but allows an "opt-out")

I have:

• Read and followed Crossplane's contribution process.
• Run earthly +reviewable to ensure this PR is ready for review.
• Added or updated unit tests.
• Linked a PR or a docs tracking issue to document this change.
• Added backport release-x.y labels to auto-backport this PR.

Need help with this checklist? See the cheat sheet.

[negz]

Thanks for taking a shot at this! I was thinking about something similar.

Could this be configured at the entire-type level though? I assume the if one kind: Widget has a deterministic external name, that'll be true for all kind: Widget resources, right? Ideally you wouldn't need to add this annotation to all of them.

Configuring this at the type level feels like the right way to go, but it's harder. It seems like a good use for the ManagedResourceDefinition type proposed in crossplane/crossplane#2262.

[negz]

Actually I guess this could just be handled at the code level via a new option to the MR reconciler. That'd mean provider authors would need to adopt it rather than being something end-users could specify though.

[nolancon]

Hi @negz, thanks for taking a look.

Could this be configured at the entire-type level though? I assume the if one kind: Widget has a deterministic external name, that'll be true for all kind: Widget resources, right? Ideally you wouldn't need to add this annotation to all of them.

Yes to both. The annotation approach is suitable for our use case because MRs are never really created by our users. They are created by a separate piece of software that interprets the user's "create" request. So adding the annotation will never be the burden of the user, instead it's just an extra line of code when we create the MR on the user's behalf (by machine - hence the annotation instead of a label). But I can appreciate this may not be the case for other providers.

Actually I guess this could just be handled at the code level via a new option to the MR reconciler. That'd mean provider authors would need to adopt it rather than being something end-users could specify though.

I like this approach and it had crossed my mind also, but I felt like using an annotation was less disruptive. However, reflecting on the above, maybe this would be a happy medium as it solves the problem and also removes the burden of adding annotations for end users.

[nolancon]

@negz I reverted the earlier changes and added a new commit with the ReconcilerOption approach to make the diff more readable. Here is an example PR for how this can be implemented by a downstream provider linode/provider-ceph#333

[negz]

One suggestion but this LGTM. Thank you!