Per the comment, there's too high a chance we'll get stuck and not process the
delete because we reference resources that are also being deleted. I considered:
* Processing references at delete time but not blocking on accessor errors. I
felt this introduced too much complexity for little gain compared to just not
trying resolution at all.
* Making AttributeReferencers no-ops when the field they would set already had a
value. I think this is an avenue we should investigate (issue forthcoming) but
it is awkward to implement with the current AttributeReferencer interface.
* Enforcing ordered deletes, such that a referenced resource cannot be deleted
(at least not by Crossplane) until its referencers have all been deleted. This
is the most bulletproof, but also the most complicated solution and would
require further design investigation to pursue.
Signed-off-by: Nic Cope <negz@rk0n.org>
All resource claims use the status subresource, but at the time the claim
reconciler was introduced most managed resources did not. This is no longer
true - all managed resources use the status subresource with the exception of a
few stragglers.
Signed-off-by: Nic Cope <negz@rk0n.org>
Previously we set the binding phase to unbound, then called Update(), which
reset the binding status to the API server value before we called
Status().Update() to persist it.
Signed-off-by: Nic Cope <negz@rk0n.org>
Unless someone else added a finalizer (and didn't yet remove it) the claim will
cease to exist as soon as the finalizer is removed, so there's nothing to update.
Signed-off-by: Nic Cope <negz@rk0n.org>
The claim reconciler uses the finalizer to unbind the managed resource. If we
never bound to the resource there's nothing to do. Keep in mind we currently
rely on garbage collection to ensure dynamically provisioned managed resources
are deleted when the claim is deleted.
Signed-off-by: Nic Cope <negz@rk0n.org>
This commit moves where we set the finalizer for managed resources to right
before creating them, not at the beginning of the reconcile. This means we'll be
less likely to encounter issues where we can't delete a managed resource because
we could never create it in the first place, but we added a finalizer.
* By the time we get here we know our Observe call worked. If (for example) our
cloud provider credentials were completely wrong, we'd never proceed far
enough to add the finalizer.
* If Observe works but Create fails (for example because we had RO cloud
provider credentials) we would already have added the finalizer, but...
* When the managed resource was deleted we'd be able to Observe that the
external resource does not exist (because we were never able to Create it) and
thus would not call Delete on the external resource and go straight to
unpublishing credentials and removing the finalizer.
This commit also renames and refactors a bunch of our interfaces to use less
obtuse names. Previously sometimes a "finalize" method unbound a managed
resource, while at other times it removed the finalizer. Similarly, finalizers
were added in "initialize". We now have a 'Binder' interface with bind and
unbind methods, and two 'Finalizer' interfaces (one for Claim, and one for
Managed) that add and remove finalizers, as you would expect.
Signed-off-by: Nic Cope <negz@rk0n.org>
Clarify the purpose of a few types, and make their documentation a little more
similar to the documentation of existing similar patterns.
Signed-off-by: Nic Cope <negz@rk0n.org>
This status update is only useful when:
* Some other controller also added a finalizer to this managed resource.
* The other controller has not yet processed the delete and removed its finalizer.
Given that this is a rare (or non-existent) edge case, and given that attempting
to update the status of a non-existent managed resource results in spurious
errors in the logs, I think it's safe to remove this logic.
Signed-off-by: Nic Cope <negz@rk0n.org>
We no longer need CanReference types to satisfy the metav1.Object interface. It
was used only to determine the namespace of the referencing object before all
such objects became cluster scoped.
Signed-off-by: Nic Cope <negz@rk0n.org>
Reference resolution is now a no-op if nothing changes, so we run it on every
reconcile. We also run it after delete has been handled, so unresolved
references will only block creates and updates.
This commit means we'll make more get calls to the cache (or API) in order to
resolve our references each reconcile, and also risk potentially changing the
values of 'immutable' fields automatically if and when our references resources
change. I believe we should address this by having referencers be no-ops when
the field value they would set is already set.
I attempted to move reference resolution to right before we call create or
update (i.e. after observe and delete), but it turns out certain resources
(specifically GCP Connections) could need references to be resolved in order to
observe the external resource.
Signed-off-by: Nic Cope <negz@rk0n.org>
The mock binding status was identical to the real one, while the mock
conditioned status set only the most recent condition, leading to a few
slightly broken managed resource reconciler tests.
Signed-off-by: Nic Cope <negz@rk0n.org>
the established convention is for the managed resource reconciler to requeue
after a short wait (typically 30 seconds) when it knows it is waiting for an
operation.
Signed-off-by: Nic Cope <negz@rk0n.org>
This commit refactors ResolveReferencers to allow the code that finds types
within a struct that satisfy AttributeReferencer to be swapped out. It also
updates the default AttributeReferencerFinder to avoid checking struct tags.
Previously errors were returned when:
1. A struct field tagged as a referencer did not satisfy AttributeReferencer
2. A struct field not tagged as a referencer satisfied AttributeReferencer
If either of these scenarios occurred, ResolveReferences would panic with the
returned error the first time it encountered an incorrectly written API type.
My feeling is that both of these conditions are testing for programmer errors
that would be better caught at build time than at runtime.
Signed-off-by: Nic Cope <negz@rk0n.org>
No functional updates to the code here, just tidying up the tests a bit to
ensure they all mock out their various moving parts via options.
Signed-off-by: Nic Cope <negz@rk0n.org>
Just wrapping a comment and updating the linter override to reflect that we've
moved beyond "slightly" over our cyclomatic complexity goal. :(
Signed-off-by: Nic Cope <negz@rk0n.org>
We already set this reference at dynamic provisioning time, but we need it set
for (bound) statically provisioned managed resources too, so we set it
(potentially again) at binding time.
We must still set the reference at dynamic provisioning time in order to ensure
reconciles are queued for dynamically provisioned managed resources when they
become available.
Signed-off-by: Nic Cope <negz@rk0n.org>
Despite past negz's optimistic comment to the contrary, we can't assume the
class reference is set by the time we get to the dynamic provisioning stage.
We are queued for managed resources that reference a claim, so we could reach
the dynamic provisioning stage without a class reference being set if a managed
resource referenced a claim that had no class reference or resource reference.
Signed-off-by: Nic Cope <negz@rk0n.org>
If a resource claim specifies a managed resource reference to a non-existent
managed resource we'll panic because we proceed and try to operate on our
managed resource's nil class reference.
Signed-off-by: Nic Cope <negz@rk0n.org>
200ms seems low enough that GCP consistently beats Azure when scheduling
RedisCluster claims in my experiments.
Signed-off-by: Nic Cope <negz@rk0n.org>
This allows us to require that name (and namespace where appropriate) are set at
the CRD level. In the case of cluster scoped resources that reference secrets
this is less surprising than defaulting to the `default` namespace when the
namespace is omitted.
Signed-off-by: Nic Cope <negz@rk0n.org>
This commit renames "non portable resource class" back to "resource class", and
requires that resource claims reference a (non portable) resource class in any
namespace.
Signed-off-by: Nic Cope <negz@rk0n.org>
Muvaffak Onus