Merge pull request #91 from phisco/respect-merge-options

feat: toFieldPath policies, replacing upstream MergeOptions

Author: negz

README.md

@@ -128,11 +128,12 @@ These fields are now required. This makes P&T configuration less ambiguous:
 * `resources[i].patches[i].transforms[i].math.type`
 
 Also, the `resources[i].patches[i].policy.mergeOptions` field is no longer
-supported.
-
-Composition functions use Kubernetes server-side apply to intelligently merge
-arrays and objects. This requires merge configuration to be specified at the
-composed resource schema level (i.e. in CRDs) per [#4617].
+supported. The same capability can be achieved by setting
+`resources[i].patches[i].policy.toFieldPath` to:
+- `MergeObject` - equivalent to
+  `resources[i].patches[i].policy.mergeOptions.keepMapValues: true`
+- `AppendArray` - equivalent to
+  `resources[i].patches[i].policy.mergeOptions.appendSlice: false`
 
 ## Developing this function
 

fn_test.go

@@ -465,7 +465,7 @@ func TestRunFunction(t *testing.T) {
 							},
 							{
 								Name: "existing-resource",
-								Base: &runtime.RawExtension{Raw: []byte(`{"apiVersion":"example.org/v1","kind":"CD","spec":{}}`)},
+								Base: &runtime.RawExtension{Raw: []byte(`{"apiVersion":"example.org/v1","kind":"CD","spec":{"targetObject": {"keep": "me"}}}`)},
 								Patches: []v1beta1.ComposedPatch{
 									{
 										// This patch should work.
@@ -475,6 +475,17 @@ func TestRunFunction(t *testing.T) {
 											ToFieldPath:   ptr.To[string]("spec.watchers"),
 										},
 									},
+									{
+										// This patch should work too and properly handle mergeOptions.
+										Type: v1beta1.PatchTypeFromCompositeFieldPath,
+										Patch: v1beta1.Patch{
+											FromFieldPath: ptr.To[string]("spec.sourceObject"),
+											ToFieldPath:   ptr.To[string]("spec.targetObject"),
+											Policy: &v1beta1.PatchPolicy{
+												ToFieldPath: ptr.To(v1beta1.ToFieldPathPolicyMergeObject),
+											},
+										},
+									},
 									{
 										// This patch will fail because the path
 										// is not found.
@@ -492,7 +503,7 @@ func TestRunFunction(t *testing.T) {
 					}),
 					Observed: &fnv1beta1.State{
 						Composite: &fnv1beta1.Resource{
-							Resource: resource.MustStructJSON(`{"apiVersion":"example.org/v1","kind":"XR","spec":{"widgets":"10"}}`),
+							Resource: resource.MustStructJSON(`{"apiVersion":"example.org/v1","kind":"XR","spec":{"widgets":"10", "sourceObject": {"me": "too"}}}`),
 						},
 						Resources: map[string]*fnv1beta1.Resource{
 							// "existing-resource" exists.
@@ -520,7 +531,7 @@ func TestRunFunction(t *testing.T) {
 							// Note that the first patch did work. We only
 							// skipped the patch from the required field path.
 							"existing-resource": {
-								Resource: resource.MustStructJSON(`{"apiVersion":"example.org/v1","kind":"CD","spec":{"watchers":"10"}}`),
+								Resource: resource.MustStructJSON(`{"apiVersion":"example.org/v1","kind":"CD","spec":{"watchers":"10", "targetObject": {"me": "too", "keep": "me"}}}`),
 							},
 
 							// Note "new-resource" doesn't appear here.
@@ -534,7 +545,7 @@ func TestRunFunction(t *testing.T) {
 						},
 						{
 							Severity: fnv1beta1.Severity_SEVERITY_WARNING,
-							Message:  `cannot render composed resource "existing-resource" "FromCompositeFieldPath" patch at index 1: ignoring 'policy.fromFieldPath: Required' because 'to' resource already exists: spec.doesNotExist: no such field`,
+							Message:  `cannot render composed resource "existing-resource" "FromCompositeFieldPath" patch at index 2: ignoring 'policy.fromFieldPath: Required' because 'to' resource already exists: spec.doesNotExist: no such field`,
 						},
 					},
 				},

go.mod

@@ -47,7 +47,6 @@ require (
 	github.com/mailru/easyjson v0.7.7 // indirect
 	github.com/mattn/go-colorable v0.1.13 // indirect
 	github.com/mattn/go-isatty v0.0.20 // indirect
-	github.com/matttproud/golang_protobuf_extensions v1.0.4 // indirect
 	github.com/matttproud/golang_protobuf_extensions/v2 v2.0.0 // indirect
 	github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
 	github.com/modern-go/reflect2 v1.0.2 // indirect

go.sum

@@ -32,6 +32,16 @@ const (
 	FromFieldPathPolicyRequired FromFieldPathPolicy = "Required"
 )
 
+// A ToFieldPathPolicy determines how to patch to a field path.
+type ToFieldPathPolicy string
+
+// ToFieldPath patch policies.
+const (
+	ToFieldPathPolicyReplace     ToFieldPathPolicy = "Replace"
+	ToFieldPathPolicyMergeObject ToFieldPathPolicy = "MergeObject"
+	ToFieldPathPolicyAppendArray ToFieldPathPolicy = "AppendArray"
+)
+
 // A PatchPolicy configures the specifics of patching behaviour.
 type PatchPolicy struct {
 	// FromFieldPath specifies how to patch from a field path. The default is
@@ -41,6 +51,15 @@ type PatchPolicy struct {
 	// +kubebuilder:validation:Enum=Optional;Required
 	// +optional
 	FromFieldPath *FromFieldPathPolicy `json:"fromFieldPath,omitempty"`
+
+	// ToFieldPath specifies how to patch to a field path. The default is
+	// 'Replace', which means the patch will completely replace the target field,
+	// or create it if it does not exist. Use 'MergeObject' to merge the patch
+	// object with the target object, or 'AppendArray' to append the patch array
+	// to the target array.
+	// +kubebuilder:validation:Enum=Replace;MergeObject;AppendArray
+	// +optional
+	ToFieldPath *ToFieldPathPolicy `json:"toFieldPath,omitempty"`
 }
 
 // GetFromFieldPathPolicy returns the FromFieldPathPolicy for this PatchPolicy, defaulting to FromFieldPathPolicyOptional if not specified.
@@ -51,6 +70,14 @@ func (pp *PatchPolicy) GetFromFieldPathPolicy() FromFieldPathPolicy {
 	return *pp.FromFieldPath
 }
 
+// GetToFieldPathPolicy returns the ToFieldPathPolicy for this PatchPolicy, defaulting to ToFieldPathPolicyReplace if not specified.
+func (pp *PatchPolicy) GetToFieldPathPolicy() ToFieldPathPolicy {
+	if pp == nil || pp.ToFieldPath == nil {
+		return ToFieldPathPolicyReplace
+	}
+	return *pp.ToFieldPath
+}
+
 // Environment represents the Composition environment.
 type Environment struct {
 	// Patches is a list of environment patches that are executed before a

input/v1beta1/resources_patches.go

@@ -117,6 +117,18 @@ spec:
                           - Optional
                           - Required
                           type: string
+                        toFieldPath:
+                          description: |-
+                            ToFieldPath specifies how to patch to a field path. The default is
+                            'Replace', which means the patch will completely replace the target field,
+                            or create it if it does not exist. Use 'MergeObject' to merge the patch
+                            object with the target object, or 'AppendArray' to append the patch array
+                            to the target array.
+                          enum:
+                          - Replace
+                          - MergeObject
+                          - AppendArray
+                          type: string
                       type: object
                     toFieldPath:
                       description: |-
@@ -448,6 +460,18 @@ spec:
                             - Optional
                             - Required
                             type: string
+                          toFieldPath:
+                            description: |-
+                              ToFieldPath specifies how to patch to a field path. The default is
+                              'Replace', which means the patch will completely replace the target field,
+                              or create it if it does not exist. Use 'MergeObject' to merge the patch
+                              object with the target object, or 'AppendArray' to append the patch array
+                              to the target array.
+                            enum:
+                            - Replace
+                            - MergeObject
+                            - AppendArray
+                            type: string
                         type: object
                       toFieldPath:
                         description: |-
@@ -843,6 +867,18 @@ spec:
                             - Optional
                             - Required
                             type: string
+                          toFieldPath:
+                            description: |-
+                              ToFieldPath specifies how to patch to a field path. The default is
+                              'Replace', which means the patch will completely replace the target field,
+                              or create it if it does not exist. Use 'MergeObject' to merge the patch
+                              object with the target object, or 'AppendArray' to append the patch array
+                              to the target array.
+                            enum:
+                            - Replace
+                            - MergeObject
+                            - AppendArray
+                            type: string
                         type: object
                       toFieldPath:
                         description: |-

input/v1beta1/zz_generated.deepcopy.go

@@ -7,7 +7,9 @@ import (
 	"github.com/pkg/errors"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/utils/ptr"
 
+	xpv1 "github.com/crossplane/crossplane-runtime/apis/common/v1"
 	"github.com/crossplane/crossplane-runtime/pkg/fieldpath"
 
 	"github.com/crossplane/function-sdk-go/resource/composed"
@@ -20,11 +22,11 @@ const (
 	errPatchSetType = "a patch in a PatchSet cannot be of type PatchSet"
 
 	errFmtUndefinedPatchSet           = "cannot find PatchSet by name %s"
-	errFmtInvalidPatchType            = "patch type %s is unsupported"
 	errFmtCombineStrategyNotSupported = "combine strategy %s is not supported"
 	errFmtCombineConfigMissing        = "given combine strategy %s requires configuration"
 	errFmtCombineStrategyFailed       = "%s strategy could not combine"
 	errFmtExpandingArrayFieldPaths    = "cannot expand ToFieldPath %s"
+	errFmtInvalidPatchPolicy          = "invalid patch policy %s"
 )
 
 // A PatchInterface is a patch that can be applied between resources.
@@ -80,7 +82,35 @@ func ApplyFromFieldPathPatch(p PatchInterface, from, to runtime.Object) error {
 		return patchFieldValueToMultiple(p.GetToFieldPath(), out, to)
 	}
 
-	return errors.Wrap(patchFieldValueToObject(p.GetToFieldPath(), out, to), "cannot patch to object")
+	mo, err := toMergeOption(p)
+	if err != nil {
+		return err
+	}
+
+	return errors.Wrap(patchFieldValueToObject(p.GetToFieldPath(), out, to, mo), "cannot patch to object")
+}
+
+// toMergeOption returns the MergeOptions from the PatchPolicy's ToFieldPathPolicy, if defined.
+func toMergeOption(p PatchInterface) (mo *xpv1.MergeOptions, err error) {
+	if p == nil {
+		return nil, nil
+	}
+	pp := p.GetPolicy()
+	if pp == nil {
+		return nil, nil
+	}
+	switch pp.GetToFieldPathPolicy() {
+	case v1beta1.ToFieldPathPolicyReplace:
+		// nothing to do, this is the default
+	case v1beta1.ToFieldPathPolicyAppendArray:
+		mo = &xpv1.MergeOptions{AppendSlice: ptr.To(true)}
+	case v1beta1.ToFieldPathPolicyMergeObject:
+		mo = &xpv1.MergeOptions{KeepMapValues: ptr.To(true)}
+	default:
+		// should never happen
+		return nil, errors.Errorf(errFmtInvalidPatchPolicy, pp.GetToFieldPathPolicy())
+	}
+	return mo, nil
 }
 
 // ApplyCombineFromVariablesPatch patches the "to" resource, taking a list of
@@ -127,7 +157,7 @@ func ApplyCombineFromVariablesPatch(p PatchInterface, from, to runtime.Object) e
 		return err
 	}
 
-	return errors.Wrap(patchFieldValueToObject(p.GetToFieldPath(), out, to), "cannot patch to object")
+	return errors.Wrap(patchFieldValueToObject(p.GetToFieldPath(), out, to, nil), "cannot patch to object")
 }
 
 // ApplyEnvironmentPatch applies a patch to or from the environment. Patches to
@@ -298,13 +328,15 @@ func ComposedTemplates(pss []v1beta1.PatchSet, cts []v1beta1.ComposedTemplate) (
 
 // patchFieldValueToObject applies the value to the "to" object at the given
 // path, returning any errors as they occur.
-func patchFieldValueToObject(fieldPath string, value any, to runtime.Object) error {
+// If no merge options is supplied, then destination field is replaced
+// with the given value.
+func patchFieldValueToObject(fieldPath string, value any, to runtime.Object, mo *xpv1.MergeOptions) error {
 	paved, err := fieldpath.PaveObject(to)
 	if err != nil {
 		return err
 	}
 
-	if err := paved.SetValue(fieldPath, value); err != nil {
+	if err := paved.MergeValue(fieldPath, value, mo); err != nil {
 		return err
 	}
 

package/input/pt.fn.crossplane.io_resources.yaml

@@ -199,7 +199,23 @@ func ValidatePatch(p PatchInterface) *field.Error { //nolint: gocyclo // This is
 			return WrapFieldError(err, field.NewPath("transforms").Index(i))
 		}
 	}
-
+	if pp := p.GetPolicy(); pp != nil {
+		switch pp.GetToFieldPathPolicy() {
+		case v1beta1.ToFieldPathPolicyReplace,
+			v1beta1.ToFieldPathPolicyAppendArray,
+			v1beta1.ToFieldPathPolicyMergeObject:
+			// ok
+		default:
+			return field.Invalid(field.NewPath("policy", "toFieldPathPolicy"), pp.GetToFieldPathPolicy(), "unknown toFieldPathPolicy")
+		}
+		switch pp.GetFromFieldPathPolicy() {
+		case v1beta1.FromFieldPathPolicyRequired,
+			v1beta1.FromFieldPathPolicyOptional:
+			// ok
+		default:
+			return field.Invalid(field.NewPath("policy", "fromFieldPathPolicy"), pp.GetFromFieldPathPolicy(), "unknown fromFieldPathPolicy")
+		}
+	}
 	return nil
 }