This switches names around from unstructured.Composite (for example) to
composite.Unstructured, mostly to allow several unstructured types to use
identically named options like WithGroupVersionKind. It also adds a few
getters and setters required for resource publications, and introduces the
resource.Requirement type that represents an application's requirement for a
published composite resource.
Signed-off-by: Nic Cope <negz@rk0n.org>
A controller engine is somewhat like a controller "sub-manager", in that it's
effectively a group of controllers. Unlike a typical controller manager, the
lifecycle of the controllers an engine manages are not coupled to the lifecycle
of the engine itself. An engine may be used by a parent controller to start and
stop child controllers in accordance with configuration provided by the custom
resource that the parent controller watches.
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>
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>
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>
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>
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>
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>
- 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
Muvaffak Onus
hasheddan
Daniel Mangum
soorena776
Jared Watts