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>
- Removing the panic recovery logic
- Removing the superfluous IsConditionReady method
- Adding Nic's unit-tests for GetCondition and IsConditionTrue
- Minor other fixes
Signed-off-by: soorena776 <javad@upbound.io>
* - Introduces Initializer hook to managed reconciler.
- ManagedNameAsExternalName is introduced and used
by default by all managed reconcilers.
Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
* GetExternalName and SetExternalName functions are implemented
Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
* - Unit tests for ManagedNameAsExternalName struct
- Move ExternalNameAnnotationKey into meta package.
Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
* Remove Establisher mechanism and port existing
establisher to Initializer.
Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
* Fix claim's external name annotation propagation to
the managed resource.
Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
This commit allows constant propagation of connection secrets from managed
resources to their bound resource claims. It does this by updating the existing
APIManagedConnectionPropagator to add 'propagation annotations', which can be
used by a new 'secret propagating reconciler' to watch both secrets for constant
propagation. The predicates and enqueue handler required to implement this are
included.
Signed-off-by: Nic Cope <negz@rk0n.org>
We currently support dynamic provisioning in the resource claim reconciler
by using a watch predicate that allows either managed resources that directly
reference a non-portable resource class of a given kind, or resource claims that
reference a non-portable resource class of a given kind indirectly via a
portable resource class.
To support static provisioning (i.e. explicitly claiming an existing managed
resource) we must also allow resource claims that explicitly reference a managed
resource. Writing one predicate to do all of this was getting cumbersome, so I
have refactored the predicate interface a little.
Signed-off-by: Nic Cope <negz@rk0n.org>
Controllers built against crossplane-runtime will currently panic if they
encounter a managed resource without a class reference. Any dynamically
provisioned managed resource will have a class reference, but this breaks
the static provisioning workflow.
Signed-off-by: Nic Cope <negz@rk0n.org>
It turns out both our watches use the same predicate and apply it to the watched
kind, not the kind that actually gets enqueued.
Signed-off-by: Nic Cope <negz@rk0n.org>
This allows us to determine whether an external resource needs updating inside
the Observe method, which must already get a fresh copy of the external resource
from the cloud provider API in order to determine whether it exists and update
the managed resource's status.
Signed-off-by: Nic Cope <negz@rk0n.org>
https://github.com/crossplaneio/crossplane/issues/719
The resource claim reconciler (and API definitions) consider
writeConnectionSecretToRef to be optional, but the managed resource reconciler
fails if it is not specified. This change aligns the codebase on the reference
being optional. Managed resources that do not specify a secret reference will
provision successfully without publishing their connection details to a secret.
Signed-off-by: Nic Cope <negz@rk0n.org>
confirmed by an Observe call.
- Reclaim polciy logic has been moved up to generic managed reconciler since we have
enough information on that level to decide whether external resource should be deleted
or not.
Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
This will allow us to set conditions as part of a generic managed resource
reconciler that operates on the Managed interface.
Signed-off-by: Nic Cope <negz@rk0n.org>
This effectively reverts https://github.com/crossplaneio/crossplane/pull/325.
I still think it would be ideal to represent BindingState as an int with a sane
zero value that marshaled to a JSON string, but it is currently impossible to
override the type of the field that is used when generating an OpenAPI spec per
https://github.com/kubernetes-sigs/controller-tools/issues/155. Until that issue
is closed it seems better to simply make this a string with a meaningless zero
value.
Signed-off-by: Nic Cope <negz@rk0n.org>
https://github.com/crossplaneio/crossplane/issues/92
This returns to our historical behaviour of creating managed resources in the
namespace of thier resource class, not that of their resource claim. I believe
this pattern to be flawed, but mostly changed it in order to leverage owner
references (which cannot cross namespaces) in order to allow a claim to watch
the resources it controls.
Instead we maintain the historical behaviour for the time being, using a custom
event handler to enqueue requests for a resource's claim (reference).
Signed-off-by: Nic Cope <negz@rk0n.org>
I had previously attempted to standardise on fooReference for an ObjectReference
to a Foo, because we had a mix of fooRef and fooReference. Turns out the API
conventions document has a position on this and says we should use fooRef. I've
kept the actual Go fields named 'FooReference' for readability, but renamed the
struct tags to 'fooRef'. I've also renamed 'writeConnectionSecretTo' to
'writeConnectionSecretToRef' since it is also a reference.
https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#object-references
Signed-off-by: Nic Cope <negz@rk0n.org>
The resource claim controller uses this binding phase to determine whether a
managed resource is available for binding to a resource claim.
Signed-off-by: Nic Cope <negz@rk0n.org>
There's _a lot_ going on in this commit. I've updated all resources to use the
embedded ResourceSpec, ResourceStatus, ResourceClaimSpec, etc structs and added
methods to ensure they satisfy the Resource and Claim interfaces.
I've also updated all controllers to suit. I've tried my best to limit the scope
of these changes to switching all controllers and their tests to:
* Use the new ConditionedStatus
* Use their WriteConnectionSecretTo field instead of generating a secret name
* Use the new ProviderReference field (which supports referencing Providers in
a different namespace)
I also updated many tests to use the '-want, +got' style annotation. This came
into scope mostly for my own sanity, as it allows me to read all test output the
same way when updating tests across many packages.
Signed-off-by: Nic Cope <negz@rk0n.org>
Nic Cope
soorena776
muvaffak onus
hasheddan
Jared Watts