diff --git a/internal/controller/apiextensions/composite/composition_functions.go b/internal/controller/apiextensions/composite/composition_functions.go index a414a3a39..747100b48 100644 --- a/internal/controller/apiextensions/composite/composition_functions.go +++ b/internal/controller/apiextensions/composite/composition_functions.go @@ -17,6 +17,7 @@ package composite import ( "context" + "crypto/sha256" "fmt" "sort" @@ -79,9 +80,9 @@ const ( // resources (XR). FieldOwnerXR = "apiextensions.crossplane.io/composite" - // FieldOwnerComposed owns the fields this controller mutates on composed + // FieldOwnerComposedPrefix owns the fields this controller mutates on composed // resources. - FieldOwnerComposed = "apiextensions.crossplane.io/composed" + FieldOwnerComposedPrefix = "apiextensions.crossplane.io/composed" ) const ( @@ -390,6 +391,8 @@ func (c *FunctionComposer) Compose(ctx context.Context, xr *composite.Unstructur xr.SetName(n) xr.SetUID(u) + // NOTE(phisco): Here we are fine using a hardcoded field owner as there is + // no risk of conflict between different XRs. if err := c.client.Status().Patch(ctx, xr, client.Apply, client.ForceOwnership, client.FieldOwner(FieldOwnerXR)); err != nil { return CompositionResult{}, errors.Wrap(err, errApplyXRStatus) } @@ -407,7 +410,10 @@ func (c *FunctionComposer) Compose(ctx context.Context, xr *composite.Unstructur // Specifically it will merge rather than replace owner references (e.g. // for Usages), and will fail if we try to add a controller reference to // a resource that already has a different one. - if err := c.client.Patch(ctx, cd.Resource, client.Apply, client.ForceOwnership, client.FieldOwner(FieldOwnerComposed)); err != nil { + // NOTE(phisco): We need to set a field owner unique for each XR here, + // this prevents multiple XRs composing the same resource to be + // continuously alternated as controllers. + if err := c.client.Patch(ctx, cd.Resource, client.Apply, client.ForceOwnership, client.FieldOwner(ComposedFieldOwnerName(xr))); err != nil { return CompositionResult{}, errors.Wrapf(err, errFmtApplyCD, name) } @@ -417,6 +423,29 @@ func (c *FunctionComposer) Compose(ctx context.Context, xr *composite.Unstructur return CompositionResult{ConnectionDetails: d.GetComposite().GetConnectionDetails(), Composed: resources, Events: events}, nil } +// ComposedFieldOwnerName generates a unique field owner name +// for a given Crossplane composite resource (XR). This uniqueness is crucial to +// prevent multiple XRs, which compose the same resource, from continuously +// alternating as controllers. +// +// The function generates a deterministic hash based on the XR's name and +// GroupKind (GK), ensuring consistency even during system restores. The hash +// does not include the XR's UID (as it's not deterministic), namespace (XRs +// don't have one), or version (to allow version changes without needing to +// update the field owner name). +// +// We decided to include the GK in the hash to prevent transferring ownership of +// composed resources across XRs with whole new GK, as that should not be +// supported without manual intervention. +// +// Given that field owner names are limited to 128 characters, the function +// truncates the hash to 32 characters. A longer hash was deemed unnecessary. +func ComposedFieldOwnerName(xr *composite.Unstructured) string { + h := sha256.New() + _, _ = h.Write([]byte(xr.GetName() + xr.GroupVersionKind().GroupKind().String())) + return fmt.Sprintf("%s/%x", FieldOwnerComposedPrefix, h.Sum(nil)) +} + // An ExistingComposedResourceObserver uses an XR's resource references to load // any existing composed resources from the API server. It also loads their // connection details. diff --git a/internal/controller/apiextensions/composite/reconciler.go b/internal/controller/apiextensions/composite/reconciler.go index a7dcc0b8f..964c8e5ad 100644 --- a/internal/controller/apiextensions/composite/reconciler.go +++ b/internal/controller/apiextensions/composite/reconciler.go @@ -71,6 +71,7 @@ const ( errFetchEnvironment = "cannot fetch environment" errSelectEnvironment = "cannot select environment" errCompose = "cannot compose resources" + errInvalidResources = "some resources were invalid, check events" errRenderCD = "cannot render composed resource" reconcilePausedMsg = "Reconciliation (including deletion) is paused via the pause annotation" @@ -624,6 +625,16 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco } err = errors.Wrap(err, errCompose) r.record.Event(xr, event.Warning(reasonCompose, err)) + if kerrors.IsInvalid(err) { + // API Server's invalid errors may be unstable due to pointers in + // the string representation of invalid structs (%v), among other + // reasons. Setting these errors in conditions could cause the + // resource version to increment continuously, leading to endless + // reconciliation of the resource. To avoid this, we only log these + // errors and emit an event. The conditions' message will then just + // point to the event. + err = errors.Wrap(errors.New(errInvalidResources), errCompose) + } xr.SetConditions(xpv1.ReconcileError(err)) return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, xr), errUpdateStatus) }