This will be the case when the referenced managed resource exists, but the field
from which we want to extract a value is empty. Consider for example a status
field that does not have a useful value until the resource has been created.
Blocking on the referenced resource field being non-zero is roughly equivalent
to blocking on the referenced resource being in condition Ready.
Signed-off-by: Nic Cope <negz@rk0n.org>
This commit introduces managed.APISimpleReferenceResolver, which satisfies the
managed.ReferenceResolver interface. This variant requires much less plumbing
and reflection because it expects that managed resources expose a single
ResolveReferences method that will optionally select and then resolve any
resource references. It also adds a new pkg/reference which contains a library
that managed resource authors _may_ choose to use to avoid reimplementing common
reference selection and resolution logic.
The existing managed.APIReferenceResolver implementation remains the default,
but is marked deprecated and will be removed once all managed resources use the
APISimpleReferenceResolver. Notably, the "reference resolution" condition is no
longer set by managed.Reconciler - managed resources will report reference
resolution issues via the Synced condition (i.e. as a ReconcileError).
Signed-off-by: Nic Cope <negz@rk0n.org>
Because we no longer include the UID of
the workload in a trait's workloadRef it
is necessary to pass the workload kind to
the trait reconciler so that we can get the
referenced workload on each reconcile and
use its UID for ensuring the corresponding
translation is controllable by it.
Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
In order to enable backup and restore of OAM traits
and workloads, resources they own should be able to
be regained control of if they are not already
controlled by a different owner.
Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
This allows propagation to function even when the propagating and/or propagated
secrets have been deleted and recreated, and thus allocated new UIDs.
Signed-off-by: Nic Cope <negz@rk0n.org>
We typically include the type hint at the start of enum-ish types, e.g.
AnnotationKeyFoo and ExternalResourceTagKeyBar. This was the one exception
to that pattern. Presumably this should not be noticed by most consumers of
this API, which use meta.GetExternalName to get this field.
Signed-off-by: Nic Cope <negz@rk0n.org>
This is roughly the same functionality as controllerutil.CreateOrUpdate, albeit
a little simpler. This variant is useful to us because it satisfies our
Applicator interface.
The key difference between the patching and updating applicators is that the
patching applicator will leave any existing, unset fields untouched (to the
extent that a JSON merge patch allows), while the updating applicator will
always update any existing object to exactly match the desired object.
Signed-off-by: Nic Cope <negz@rk0n.org>
This allows resource claims and managed resources to adopt existing orphaned
connection secrets without needing to be concerned with the fact that they
might be adopting an existing secret that is unrelated to Crossplane.
Signed-off-by: Nic Cope <negz@rk0n.org>
This option replaces ControllersMustMatch. It works slightly differently in that
it takes the expected controller UID explicitly instead of inferring it from the
desired object, and considers current objects with no controller reference to be
controllable. Presumable objects with no controller reference will be adopted.
Signed-off-by: Nic Cope <negz@rk0n.org>
The Apply method of the resource.Applicator interface took a client.Client
largely because it was extracted from the pre-existing resource.Apply function
to allow pluggable Apply implementations. Now that we have types that satisfy
resource.Applicator it makes more sense for those types to include a client,
rather than being passed one for every Apply call.
Signed-off-by: Nic Cope <negz@rk0n.org>
so that managed reconciler is able to call k8s functions
even though the deadline is exceeded during external client
operations.
Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
Paved objects expect their internal unstructured content to be of the types
produced by json.Unmarshal. This allows us to enforce that for any call to
p.SetValue("some.path", T) where T is any JSON serialisable type.
Signed-off-by: Nic Cope <negz@rk0n.org>
Getting, creating, or patching an object using the controller-runtime client
updates said object with the latest state from the API server. This commit
alters our resource.Apply logic to patch 'into' the supplied object, not a
deepcopy of that object. This ensures the object passed to apply is updated
with the latest view from the API server, whether it is created or patched.
Signed-off-by: Nic Cope <negz@rk0n.org>
All functions defined by this package now either live alongside their (few)
callers, or have moved into distinct crossplane-runtime packages.
Signed-off-by: Nic Cope <negz@rk0n.org>
https://github.com/sethvargo/go-password
I considered using the above library, but its API is a little overkill for our
needs. I chose not to keep the previous GeneratePassword implementation because
I felt the length argument was a unintuitive. The argument specified the length
of the randomly generated bytes, which were then base64 encoded resulting in a
~33% overhead. So for example GeneratePassword(20) generated a 27 character
password.
Signed-off-by: Nic Cope <negz@rk0n.org>
https://github.com/crossplaneio/crossplane-runtime/issues/1
Each of these functions were called by only a single package, so I've raised
PRs to move them to the packages that call them. All relevant PRs are cross
linked from the above issue
Signed-off-by: Nic Cope <negz@rk0n.org>
This allows us to plug in a no-op secret propagator for our unit tests, allowing
those tests to focus on testing the reconciler rather than 'testing through' to
the default secret propagator implementation (which has its own tests).
Signed-off-by: Nic Cope <negz@rk0n.org>
I'm hoping this will help us pass in loggers and eventers using variadic options
without awkward names like WithClaimBindingReconcilerLogger. This is a huge diff
but there's no functional changes - only moving things around and renaming them.
I was hoping we'd be able to do with without a breaking API change by using type
aliases, but doing so would create cyclic imports, because the new reconciler
packages depend on the resource package, which is where we'd need to create the
type alias to the reconciler packages.
I've broken up api.go (which contained most of the 'Kubernetes API' - i.e.
default - implementations of most of the pluggable interfaces used by our
reconcilers) into several files. My heuristic here was:
* If the implementation is used by a single reconciler, put it in that
reconciler's package.
* If the implementation is used by more than one reconciler, put it in the
resource package.
Signed-off-by: Nic Cope <negz@rk0n.org>
This commit changes the meaning of the resource claim resource policy to match
https://kubernetes.io/docs/concepts/storage/persistent-volumes/#reclaiming as
closely as possible, minus the deprecated 'Recycle' policy. Previously the
reclaim policy dictated only what happened to the external resource when its
managed resource was deleted.
Signed-off-by: Nic Cope <negz@rk0n.org>
Resource claims are namespaced, but the managed resources they dynamically
provision are cluster scoped. A namespaced resource cannot, by design, own
a cluster scoped resource. This has been a known issue for a long time, but
we recently learned that it can result in the managed resources being
unintentionally garbage collected by Kubernetes per
https://github.com/crossplaneio/stack-gcp/issues/99.
Resource claims and managed resources already reference each other via their
resourceRef and claimRef; we had set the owner reference purely to delete
dynamically provisioned managed resources along with their claim. Dynamically
provisioned managed resources will now always stick around until explicitly
cleaned up. We intend to address this by repurposing the existing reclaimPolicy
field per https://github.com/crossplaneio/crossplane-runtime/issues/21
Signed-off-by: Nic Cope <negz@rk0n.org>
These functions have been marked deprecated for as long as crossplane-runtime
has existed. As far as I can tell they are no longer used.
Signed-off-by: Nic Cope <negz@rk0n.org>
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>
Nic Cope