fix: run patches in order for composed resources

Signed-off-by: Philippe Scorsolini <p.scorsolini@gmail.com>

Author: phisco

fn.go

@@ -196,58 +196,19 @@ func (f *Function) RunFunction(ctx context.Context, req *fnv1beta1.RunFunctionRe
 			log.Debug("Found corresponding observed resource",
 				"ready", ready,
 				"name", ocd.Resource.GetName())
-
-			// TODO(negz): Should failures to patch the XR be terminal? It could
-			// indicate a required patch failed. A required patch means roughly
-			// "this patch has to succeed before you mutate the resource". This
-			// is useful to make sure we never create a composed resource in the
-			// wrong state. It's less clear how useful it is for the XR, given
-			// we'll only ever be updating it, not creating it.
-
-			// We want to patch the XR from observed composed resources, not
-			// from desired state. This is because folks will typically be
-			// patching from a field that is set once the observed resource is
-			// applied such as its status.
-			if err := RenderToCompositePatches(dxr.Resource, ocd.Resource, t.Patches); err != nil {
-				response.Warning(rsp, errors.Wrapf(err, "cannot render ToComposite patches for composed resource %q", t.Name))
-				log.Info("Cannot render ToComposite patches for composed resource", "warning", err)
-				warnings++
-			}
-
-			// TODO(negz): Same as above, but for the Environment. What does it
-			// mean for a required patch to the environment to fail? Should it
-			// be terminal?
-
-			// Run all patches that are from the (observed) composed resource to
-			// the environment.
-			if err := RenderToEnvironmentPatches(env, ocd.Resource, t.Patches); err != nil {
-				response.Warning(rsp, errors.Wrapf(err, "cannot render ToEnvironment patches for composed resource %q", t.Name))
-				log.Info("Cannot render ToEnvironment patches for composed resource", "warning", err)
-				warnings++
-			}
 		}
 
-		// If either of the below renderings return an error, most likely a
-		// required FromComposite or FromEnvironment patch failed. A required
-		// patch means roughly "this patch has to succeed before you mutate the
-		// resource." This is useful to make sure we never create a composed
-		// resource in the wrong state. To that end, we don't want to add this
-		// resource to our accumulated desired state.
-		if err := RenderFromCompositePatches(dcd.Resource, oxr.Resource, t.Patches); err != nil {
-			response.Warning(rsp, errors.Wrapf(err, "cannot render FromComposite patches for composed resource %q", t.Name))
-			log.Info("Cannot render FromComposite patches for composed resource", "warning", err)
+		errs, store := RenderComposedPatches(ocd.Resource, dcd.Resource, oxr.Resource, dxr.Resource, env, t.Patches)
+		for _, err := range errs {
+			response.Warning(rsp, errors.Wrapf(err, "cannot render patches for composed resource %q", t.Name))
+			log.Info("Cannot render patches for composed resource", "warning", err)
 			warnings++
-			continue
-		}
-		if err := RenderFromEnvironmentPatches(dcd.Resource, env, t.Patches); err != nil {
-			response.Warning(rsp, errors.Wrapf(err, "cannot render FromEnvironment patches for composed resource %q", t.Name))
-			log.Info("Cannot render FromEnvironment patches for composed resource", "warning", err)
-			warnings++
-			continue
 		}
 
-		// Add or replace our desired resource.
-		desired[resource.Name(t.Name)] = dcd
+		if store {
+			// Add or replace our desired resource.
+			desired[resource.Name(t.Name)] = dcd
+		}
 	}
 
 	if err := response.SetDesiredCompositeResource(rsp, dxr); err != nil {

fn_test.go

@@ -447,7 +447,7 @@ func TestRunFunction(t *testing.T) {
 					Results: []*fnv1beta1.Result{
 						{
 							Severity: fnv1beta1.Severity_SEVERITY_WARNING,
-							Message:  fmt.Sprintf("cannot render FromComposite patches for composed resource %q: cannot apply the %q patch at index 1: spec.doesNotExist: no such field", "cool-resource", "FromCompositeFieldPath"),
+							Message:  fmt.Sprintf("cannot render patches for composed resource %q: cannot apply the %q patch at index 1: spec.doesNotExist: no such field", "cool-resource", "FromCompositeFieldPath"),
 						},
 					},
 					Context: &structpb.Struct{Fields: map[string]*structpb.Value{fncontext.KeyEnvironment: structpb.NewStructValue(nil)}},

render.go

@@ -9,6 +9,9 @@ import (
 	"github.com/crossplane/crossplane-runtime/pkg/errors"
 	"github.com/crossplane/crossplane-runtime/pkg/resource"
 
+	"github.com/crossplane/function-sdk-go/resource/composed"
+	"github.com/crossplane/function-sdk-go/resource/composite"
+
 	"github.com/crossplane-contrib/function-patch-and-transform/input/v1beta1"
 )
 
@@ -102,3 +105,71 @@ func RenderToEnvironmentPatches(env *unstructured.Unstructured, o runtime.Object
 	}
 	return nil
 }
+
+// RenderComposedPatches renders the supplied composed resource by applying all
+// patches that are to or from the supplied composite resource and environment
+// in the order they were defined. Properly selecting the right source or
+// destination between observed and desired resources.
+func RenderComposedPatches( //nolint:gocyclo // just a switch
+	ocd *composed.Unstructured,
+	dcd *composed.Unstructured,
+	oxr *composite.Unstructured,
+	dxr *composite.Unstructured,
+	env *unstructured.Unstructured,
+	ps []v1beta1.Patch,
+) (errs []error, store bool) {
+	for i, p := range ps {
+		switch t := p.Type; t {
+		case v1beta1.PatchTypeToCompositeFieldPath, v1beta1.PatchTypeCombineToComposite:
+			// TODO(negz): Should failures to patch the XR be terminal? It could
+			// indicate a required patch failed. A required patch means roughly
+			// "this patch has to succeed before you mutate the resource". This
+			// is useful to make sure we never create a composed resource in the
+			// wrong state. It's less clear how useful it is for the XR, given
+			// we'll only ever be updating it, not creating it.
+
+			// We want to patch the XR from observed composed resources, not
+			// from desired state. This is because folks will typically be
+			// patching from a field that is set once the observed resource is
+			// applied such as its status.
+			if ocd == nil {
+				continue
+			}
+			if err := ApplyToObjects(p, dxr, ocd); err != nil {
+				errs = append(errs, errors.Wrapf(err, errFmtPatch, t, i))
+			}
+		case v1beta1.PatchTypeToEnvironmentFieldPath, v1beta1.PatchTypeCombineToEnvironment:
+			// TODO(negz): Same as above, but for the Environment. What does it
+			// mean for a required patch to the environment to fail? Should it
+			// be terminal?
+
+			// Run all patches that are from the (observed) composed resource to
+			// the environment.
+			if ocd == nil {
+				continue
+			}
+			if err := ApplyToObjects(p, env, ocd); err != nil {
+				errs = append(errs, errors.Wrapf(err, errFmtPatch, t, i))
+			}
+		// If either of the below renderings return an error, most likely a
+		// required FromComposite or FromEnvironment patch failed. A required
+		// patch means roughly "this patch has to succeed before you mutate the
+		// resource." This is useful to make sure we never create a composed
+		// resource in the wrong state. To that end, we don't want to add this
+		// resource to our accumulated desired state.
+		case v1beta1.PatchTypeFromCompositeFieldPath, v1beta1.PatchTypeCombineFromComposite:
+			if err := ApplyToObjects(p, oxr, dcd); err != nil {
+				errs = append(errs, errors.Wrapf(err, errFmtPatch, t, i))
+				return errs, false
+			}
+		case v1beta1.PatchTypeFromEnvironmentFieldPath, v1beta1.PatchTypeCombineFromEnvironment:
+			if err := ApplyToObjects(p, env, dcd); err != nil {
+				errs = append(errs, errors.Wrapf(err, errFmtPatch, t, i))
+				return errs, false
+			}
+		case v1beta1.PatchTypePatchSet:
+			// Already resolved - nothing to do.
+		}
+	}
+	return errs, true
+}