Merge pull request #5486 from crossplane/backport-5365-to-release-1.15

internal/controller/apiextensions/composite/composed.go

@@ -35,8 +35,14 @@ type ComposedResource struct {
 	ResourceName ResourceName
 
 	// Ready indicates whether this composed resource is ready - i.e. whether
-	// all of its readiness checks passed.
+	// all of its readiness checks passed. Setting it to false will cause the
+	// XR to be marked as not ready.
 	Ready bool
+
+	// Synced indicates whether the composition process was able to sync the
+	// composed resource with its desired state. Setting it to false will cause
+	// the XR to be marked as not synced.
+	Synced bool
 }
 
 // ComposedResourceState represents a composed resource (either desired or

internal/controller/apiextensions/composite/composition_functions.go

@@ -454,6 +454,46 @@ func (c *FunctionComposer) Compose(ctx context.Context, xr *composite.Unstructur
 		return CompositionResult{}, errors.Wrap(err, errApplyXRRefs)
 	}
 
+	// Produce our array of resources to return to the Reconciler. The
+	// Reconciler uses this array to determine whether the XR is ready.
+	resources := make([]ComposedResource, 0, len(desired))
+
+	// We apply all of our desired resources before we observe them in the loop
+	// below. This ensures that issues observing and processing one composed
+	// resource won't block the application of another.
+	for name, cd := range desired {
+		// We don't need any crossplane-runtime resource.Applicator style apply
+		// options here because server-side apply takes care of everything.
+		// 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.
+		// 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 {
+			if kerrors.IsInvalid(err) {
+				// We tried applying an invalid resource, we can't tell whether
+				// this means the resource will never be valid or it will if we
+				// run again the composition after some other resource is
+				// created or updated successfully. So, we emit a warning event
+				// and move on.
+				// We mark the resource as not synced, so that once we get to
+				// decide the XR's Synced condition, we can set it to false if
+				// any of the resources didn't sync successfully.
+				events = append(events, event.Warning(reasonCompose, errors.Wrapf(err, errFmtApplyCD, name)))
+				// NOTE(phisco): here we behave differently w.r.t. the native
+				// p&t composer, as we respect the readiness reported by
+				// functions, while there we defaulted to also set ready false
+				// in case of apply errors.
+				resources = append(resources, ComposedResource{ResourceName: name, Ready: cd.Ready, Synced: false})
+				continue
+			}
+			return CompositionResult{}, errors.Wrapf(err, errFmtApplyCD, name)
+		}
+
+		resources = append(resources, ComposedResource{ResourceName: name, Ready: cd.Ready, Synced: true})
+	}
+
 	// Our goal here is to patch our XR's status using server-side apply. We
 	// want the resulting, patched object loaded into uxr. We need to pass in
 	// only our "fully specified intent" - i.e. only the fields that we actually
@@ -475,32 +515,12 @@ func (c *FunctionComposer) Compose(ctx context.Context, xr *composite.Unstructur
 	// 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 {
+		// Note(phisco): here we are fine with this error being terminal, as
+		// there is no other resource to apply that might eventually resolve
+		// this issue.
 		return CompositionResult{}, errors.Wrap(err, errApplyXRStatus)
 	}
 
-	// Produce our array of resources to return to the Reconciler. The
-	// Reconciler uses this array to determine whether the XR is ready.
-	resources := make([]ComposedResource, 0, len(desired))
-
-	// We apply all of our desired resources before we observe them in the loop
-	// below. This ensures that issues observing and processing one composed
-	// resource won't block the application of another.
-	for name, cd := range desired {
-		// We don't need any crossplane-runtime resource.Applicator style apply
-		// options here because server-side apply takes care of everything.
-		// 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.
-		// 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)
-		}
-
-		resources = append(resources, ComposedResource{ResourceName: name, Ready: cd.Ready})
-	}
-
 	return CompositionResult{ConnectionDetails: d.GetComposite().GetConnectionDetails(), Composed: resources, Events: events}, nil
 }
 

internal/controller/apiextensions/composite/composition_functions_test.go

@@ -614,8 +614,8 @@ func TestFunctionCompose(t *testing.T) {
 			want: want{
 				res: CompositionResult{
 					Composed: []ComposedResource{
-						{ResourceName: "desired-resource-a"},
-						{ResourceName: "observed-resource-a", Ready: true},
+						{ResourceName: "desired-resource-a", Synced: true},
+						{ResourceName: "observed-resource-a", Ready: true, Synced: true},
 					},
 					ConnectionDetails: managed.ConnectionDetails{
 						"from": []byte("function-pipeline"),
@@ -815,8 +815,8 @@ func TestFunctionCompose(t *testing.T) {
 			want: want{
 				res: CompositionResult{
 					Composed: []ComposedResource{
-						{ResourceName: "desired-resource-a"},
-						{ResourceName: "observed-resource-a", Ready: true},
+						{ResourceName: "desired-resource-a", Synced: true},
+						{ResourceName: "observed-resource-a", Ready: true, Synced: true},
 					},
 					ConnectionDetails: managed.ConnectionDetails{
 						"from": []byte("function-pipeline"),

internal/controller/apiextensions/composite/composition_pt.go

@@ -273,6 +273,21 @@ func (c *PTComposer) Compose(ctx context.Context, xr *composite.Unstructured, re
 		o := []resource.ApplyOption{resource.MustBeControllableBy(xr.GetUID()), usage.RespectOwnerRefs()}
 		o = append(o, mergeOptions(filterPatches(t.Patches, patchTypesFromXR()...))...)
 		if err := c.client.Apply(ctx, cd, o...); err != nil {
+			if kerrors.IsInvalid(err) {
+				// We tried applying an invalid resource, we can't tell whether
+				// this means the resource will never be valid or it will if we
+				// run again the composition after some other resource is
+				// created or updated successfully. So, we emit a warning event
+				// and move on.
+				events = append(events, event.Warning(reasonCompose, errors.Wrap(err, errApplyComposed)))
+				// We unset the cd here so that we don't try to observe it
+				// later. This will also mean we report it as not ready and not
+				// synced. Resulting in the XR being reported as not ready nor
+				// synced too.
+				cds[i] = nil
+				continue
+			}
+
 			// TODO(negz): Include the template name (if any) in this error.
 			// Including the rendered resource's kind may help too (e.g. if the
 			// template is anonymous).
@@ -298,7 +313,7 @@ func (c *PTComposer) Compose(ctx context.Context, xr *composite.Unstructured, re
 		// to observe it. We still want to return it to the Reconciler so that
 		// it knows that this desired composed resource is not ready.
 		if cd == nil {
-			resources[i] = ComposedResource{ResourceName: name, Ready: false}
+			resources[i] = ComposedResource{ResourceName: name, Synced: false, Ready: false}
 			continue
 		}
 
@@ -328,7 +343,7 @@ func (c *PTComposer) Compose(ctx context.Context, xr *composite.Unstructured, re
 			return CompositionResult{}, errors.Wrapf(err, errFmtCheckReadiness, name)
 		}
 
-		resources[i] = ComposedResource{ResourceName: name, Ready: ready}
+		resources[i] = ComposedResource{ResourceName: name, Ready: ready, Synced: true}
 	}
 
 	// Call Apply so that we do not just replace fields on existing XR but

internal/controller/apiextensions/composite/composition_pt_test.go

@@ -392,6 +392,7 @@ func TestPTCompose(t *testing.T) {
 					Composed: []ComposedResource{{
 						ResourceName: "cool-resource",
 						Ready:        true,
+						Synced:       true,
 					}},
 					ConnectionDetails: details,
 				},
@@ -457,10 +458,12 @@ func TestPTCompose(t *testing.T) {
 						{
 							ResourceName: "cool-resource",
 							Ready:        true,
+							Synced:       true,
 						},
 						{
 							ResourceName: "uncool-resource",
 							Ready:        false,
+							Synced:       false,
 						},
 					},
 					ConnectionDetails: details,

internal/controller/apiextensions/composite/reconciler.go

@@ -73,6 +73,7 @@ const (
 	errCompose                = "cannot compose resources"
 	errInvalidResources       = "some resources were invalid, check events"
 	errRenderCD               = "cannot render composed resource"
+	errSyncResources          = "cannot sync composed resources"
 
 	reconcilePausedMsg = "Reconciliation (including deletion) is paused via the pause annotation"
 )
@@ -651,6 +652,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco
 	}
 
 	var unready []ComposedResource
+	var unsynced []ComposedResource
 	for i, cd := range res.Composed {
 		// Specifying a name for P&T templates is optional but encouraged.
 		// If there was no name, fall back to using the index.
@@ -659,38 +661,62 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco
 			id = strconv.Itoa(i)
 		}
 
+		if !cd.Synced {
+			log.Debug("Composed resource is not yet valid", "id", id)
+			unsynced = append(unsynced, cd)
+			r.record.Event(xr, event.Normal(reasonCompose, fmt.Sprintf("Composed resource %q is not yet valid", id)))
+		}
+
 		if !cd.Ready {
 			log.Debug("Composed resource is not yet ready", "id", id)
 			unready = append(unready, cd)
 			r.record.Event(xr, event.Normal(reasonCompose, fmt.Sprintf("Composed resource %q is not yet ready", id)))
-			continue
 		}
 	}
 
-	xr.SetConditions(xpv1.ReconcileSuccess())
-
-	// TODO(muvaf): If a resource becomes Unavailable at some point, should we
-	// still report it as Creating?
-	if len(unready) > 0 {
-		// We want to requeue to wait for our composed resources to
-		// become ready, since we can't watch them.
-		names := make([]string, len(unready))
-		for i, cd := range unready {
-			names[i] = string(cd.ResourceName)
-		}
-		// sort for stable condition messages. With functions, we don't have a
-		// stable order otherwise.
-		xr.SetConditions(xpv1.Creating().WithMessage(fmt.Sprintf("Unready resources: %s", resource.StableNAndSomeMore(resource.DefaultFirstN, names))))
+	if updateXRConditions(xr, unsynced, unready) {
+		// This requeue is subject to rate limiting. Requeues will exponentially
+		// backoff from 1 to 30 seconds. See the 'definition' (XRD) reconciler
+		// that sets up the ratelimiter.
 		return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, xr), errUpdateStatus)
 	}
 
 	// We requeue after our poll interval because we can't watch composed
 	// resources - we can't know what type of resources we might compose
 	// when this controller is started.
-	xr.SetConditions(xpv1.Available())
 	return reconcile.Result{RequeueAfter: r.pollInterval}, errors.Wrap(r.client.Status().Update(ctx, xr), errUpdateStatus)
 }
 
+// updateXRConditions updates the conditions of the supplied composite resource
+// based on the supplied composed resources. It returns true if the XR should be
+// requeued immediately.
+func updateXRConditions(xr *composite.Unstructured, unsynced, unready []ComposedResource) (requeueImmediately bool) {
+	readyCond := xpv1.Available()
+	syncedCond := xpv1.ReconcileSuccess()
+	if len(unsynced) > 0 {
+		// We want to requeue to wait for our composed resources to
+		// become ready, since we can't watch them.
+		syncedCond = xpv1.ReconcileError(errors.New(errSyncResources)).WithMessage(fmt.Sprintf("Invalid resources: %s", resource.StableNAndSomeMore(resource.DefaultFirstN, getComposerResourcesNames(unsynced))))
+		requeueImmediately = true
+	}
+	if len(unready) > 0 {
+		// We want to requeue to wait for our composed resources to
+		// become ready, since we can't watch them.
+		readyCond = xpv1.Creating().WithMessage(fmt.Sprintf("Unready resources: %s", resource.StableNAndSomeMore(resource.DefaultFirstN, getComposerResourcesNames(unready))))
+		requeueImmediately = true
+	}
+	xr.SetConditions(syncedCond, readyCond)
+	return requeueImmediately
+}
+
+func getComposerResourcesNames(cds []ComposedResource) []string {
+	names := make([]string, len(cds))
+	for i, cd := range cds {
+		names[i] = string(cd.ResourceName)
+	}
+	return names
+}
+
 // EnqueueForCompositionRevisionFunc returns a function that enqueues (the
 // related) XRs when a new CompositionRevision is created. This speeds up
 // reconciliation of XRs on changes to the Composition by not having to wait for

internal/controller/apiextensions/composite/reconciler_test.go

@@ -517,21 +517,27 @@ func TestReconcile(t *testing.T) {
 							Composed: []ComposedResource{{
 								ResourceName: "elephant",
 								Ready:        false,
+								Synced:       true,
 							}, {
 								ResourceName: "cow",
 								Ready:        false,
+								Synced:       true,
 							}, {
 								ResourceName: "pig",
 								Ready:        true,
+								Synced:       true,
 							}, {
 								ResourceName: "cat",
 								Ready:        false,
+								Synced:       true,
 							}, {
 								ResourceName: "dog",
 								Ready:        true,
+								Synced:       true,
 							}, {
 								ResourceName: "snake",
 								Ready:        false,
+								Synced:       true,
 							}},
 						}, nil
 					})),

test/e2e/apiextensions_test.go

@@ -105,6 +105,50 @@ func TestCompositionMinimal(t *testing.T) {
 	)
 }
 
+// TestCompositionInvalidComposed tests Crossplane's Composition functionality,
+// checking that although a composed resource is invalid, i.e. it didn't apply
+// successfully.
+func TestCompositionInvalidComposed(t *testing.T) {
+	manifests := "test/e2e/manifests/apiextensions/composition/invalid-composed"
+
+	xrList := composed.NewList(composed.FromReferenceToList(corev1.ObjectReference{
+		APIVersion: "example.org/v1alpha1",
+		Kind:       "XParent",
+	}), composed.FromReferenceToList(corev1.ObjectReference{
+		APIVersion: "example.org/v1alpha1",
+		Kind:       "XChild",
+	}))
+
+	environment.Test(t,
+		features.New(t.Name()).
+			WithLabel(LabelArea, LabelAreaAPIExtensions).
+			WithLabel(LabelSize, LabelSizeSmall).
+			WithLabel(config.LabelTestSuite, config.TestSuiteDefault).
+			WithSetup("PrerequisitesAreCreated", funcs.AllOf(
+				funcs.ApplyResources(FieldManager, manifests, "setup/*.yaml"),
+				funcs.ResourcesCreatedWithin(30*time.Second, manifests, "setup/*.yaml"),
+				funcs.ResourcesHaveConditionWithin(1*time.Minute, manifests, "setup/definition.yaml", apiextensionsv1.WatchingComposite()),
+				funcs.ResourcesHaveConditionWithin(2*time.Minute, manifests, "setup/provider.yaml", pkgv1.Healthy(), pkgv1.Active()),
+			)).
+			Assess("CreateXR", funcs.AllOf(
+				funcs.ApplyResources(FieldManager, manifests, "xr.yaml"),
+				funcs.InBackground(funcs.LogResources(xrList)),
+				funcs.InBackground(funcs.LogResources(nopList)),
+				funcs.ResourcesCreatedWithin(30*time.Second, manifests, "xr.yaml"),
+			)).
+			Assess("XRStillAnnotated", funcs.AllOf(
+				// Check the XR it has metadata.annotations set
+				funcs.ResourcesHaveFieldValueWithin(1*time.Minute, manifests, "xr.yaml", "metadata.annotations[exampleVal]", "foo"),
+			)).
+			WithTeardown("DeleteXR", funcs.AllOf(
+				funcs.DeleteResources(manifests, "xr.yaml"),
+				funcs.ResourcesDeletedWithin(2*time.Minute, manifests, "xr.yaml"),
+			)).
+			WithTeardown("DeletePrerequisites", funcs.ResourcesDeletedAfterListedAreGone(3*time.Minute, manifests, "setup/*.yaml", nopList)).
+			Feature(),
+	)
+}
+
 // TestCompositionPatchAndTransform tests Crossplane's Composition functionality,
 // checking that a claim using patch-and-transform Composition will become
 // available when its composed resources do, and have a field derived from