Support merging in addition to replacing objects in patches #270

ulucinar · 2021-07-08T23:36:18Z

Description of your changes

Please refer to crossplane/crossplane#2211 for more background on this PR.
This PR aims to add merge support to the fieldpath package and moves some code from the crossplane repo to the crossplane-runtime repo to implement common functionality & prevent code duplication. For merging objects (map[string]interface{}), the mergo library is used. Certain aspects of the behavior of the mergo library can be controlled via the new fieldpath.MergeOptions struct. Merging of a value at a given path of the desired runtime.Object into the actual (current) runtime.Object is achieved via a resource.ApplyOption that's passed as an argument to resource.Applicator.Apply. This new kind of ApplyOption can be obtained using the new resource.WithMergeOptions constructor. crossplane repo PR crossplane/crossplane#2409 depends on this PR.

I have:

Read and followed Crossplane's contribution process.
Run make reviewable test to ensure this PR is ready for review.
Added automated tests

How has this code been tested

Together with crossplane/crossplane#2409, I have manually tested this PR using the following manifests. I have kubectl edited resources to test & observe the behavior (e.g., when keepMapValues is false or is missing in the composition for the metadata patch, observe that claim's annotations overwrite composed's annotations with the same keys. However, when keepMapValues is true, existing values in the composed annotations are preserved even if the claim (and correspondingly the XR) has a different value for the same key):

---
apiVersion: apiextensions.crossplane.io/v1
kind: CompositeResourceDefinition
metadata:
  name: compositepostgresqlinstances.database.example.org
spec:
  group: database.example.org
  names:
    kind: CompositePostgreSQLInstance
    plural: compositepostgresqlinstances
  claimNames:
    kind: PostgreSQLInstance
    plural: postgresqlinstances
  connectionSecretKeys:
    - username
    - password
    - endpoint
    - port
  versions:
  - name: v1alpha1
    served: true
    referenceable: true
    schema:
      openAPIV3Schema:
        type: object
        properties:
          spec:
            type: object
            properties:
              parameters:
                type: object
                properties:
                  vpcSecurityGroupIDSelector:
                    description: VPCSecurityGroupIDSelector selects references to VPCSecurityGroups used to set the VPCSecurityGroupIDs.
                    properties:
                      matchControllerRef:
                        description: MatchControllerRef ensures an object with the same controller reference as the selecting object is selected.
                        type: boolean
                      matchLabels:
                        additionalProperties:
                          type: string
                        description: MatchLabels ensures an object with matching labels is selected.
                        type: object
                    type: object
                  storageGB:
                    type: integer
                required:
                  - storageGB
            required:
              - parameters
---
apiVersion: apiextensions.crossplane.io/v1
kind: Composition
metadata:
  name: composition-new
  labels:
    provider: aws
    guide: quickstart
    vpc: default
spec:
  writeConnectionSecretsToNamespace: crossplane-system
  compositeTypeRef:
    apiVersion: database.example.org/v1alpha1
    kind: CompositePostgreSQLInstance
  patchSets:
  - name: selector
    patches:
    - fromFieldPath: spec.parameters.vpcSecurityGroupIDSelector
      toFieldPath: spec.forProvider.vpcSecurityGroupIDSelector
      policy:
        mergeOptions:
          appendSlice: true
          keepMapValues: true
  - name: metadata
    patches:
    - fromFieldPath: metadata.annotations
      policy:
        mergeOptions:
          keepMapValues: true
      type: FromCompositeFieldPath
  resources:
    - name: rdsinstance
      base:
        apiVersion: database.aws.crossplane.io/v1beta1
        kind: RDSInstance
        spec:
          forProvider:
            region: us-east-1
            dbInstanceClass: db.t2.small
            masterUsername: masteruser
            engine: postgres
            engineVersion: "9.6"
            skipFinalSnapshotBeforeDeletion: true
            publiclyAccessible: true
            allocatedStorage: 20
      patches:
         - type: PatchSet
           patchSetName: selector
         - type: PatchSet
           patchSetName: metadata
         - fromFieldPath: "spec.writeConnectionSecretToRef.namespace"
           toFieldPath: "spec.writeConnectionSecretToRef.namespace"
         - fromFieldPath: "metadata.uid"
           toFieldPath: "spec.writeConnectionSecretToRef.name"
           transforms:
             - type: string
               string:
                 fmt: "%s-postgresql"
         - fromFieldPath: "spec.parameters.storageGB"
           toFieldPath: "spec.forProvider.allocatedStorage"
      connectionDetails:
        - fromConnectionSecretKey: username
        - fromConnectionSecretKey: password
        - fromConnectionSecretKey: endpoint
        - fromConnectionSecretKey: port
---
apiVersion: database.example.org/v1alpha1
kind: PostgreSQLInstance
metadata:
  name: my-db
  namespace: default
  annotations:
    a: a-from-claim
    b: b-from-claim
spec:
  parameters:
    vpcSecurityGroupIDSelector:
      matchLabels:
        l1: l1-from-claim
        l2: l2-from-claim
    storageGB: 20
  compositionSelector:
    matchLabels:
      provider: aws
      vpc: default

pkg/fieldpath/merge.go

negz · 2021-07-30T00:13:16Z

pkg/fieldpath/merge.go

+	KeepMapValues bool `json:"keepMapValues,omitempty"`
+	// Specifies that already existing elements in a merged slice should be preserved
+	AppendSlice bool `json:"appendSlice,omitempty"`
+}


Is the intent that this type would be embedded in API types, as in https://github.com/crossplane/crossplane/pull/2409/files#diff-40b5c705a7ba08a514b14b156d26120fcd0c6a8b4326096ceecbb7d3bd971f29R283?

If so, I would prefer it to be defined along with all other API types in https://github.com/crossplane/crossplane-runtime/tree/master/apis/common/v1. I'd also suggest we make sure it complies with API conventions. Specifically:

Are we sure boolean fields are the best option here? API conventions warns to think twice about them.

Are these fields optional? If so they should be marked // +optional and be *bool.

Yes, it's embedded in API types as you mentioned. Moved it to the suggested package.

Regarding API conventions, we could go with a string type alias MergePolicy and define policy constants with it but we may add other options that control merge behavior and we will (assuming they are not mutually exclusive) need cartesian product of these options. So I preferred a bitmap-like structure.

Added the +optional marker and converted the types from bool to *bool. Asking to learn: FWIW, here for these two fields, we have the same semantics for the zero-values of both bool and *bool. Do we have another reason for the conversion apart from being probably a better practice?

negz · 2021-07-30T00:15:15Z

pkg/fieldpath/merge.go

+		return nil, false, err
+	}
+	return Pave(oMap), true, nil
+}


Is merge.go the right place for this function? It does not seem specific to merging.

Moved it to another package (fieldpath/object). Please see below for a discussion.

negz · 2021-07-30T00:18:53Z

pkg/resource/api.go

@@ -152,6 +153,15 @@ func (a *APIPatchingApplicator) Apply(ctx context.Context, o client.Object, ao .
 	return errors.Wrap(a.client.Patch(ctx, o, &patch{desired}), "cannot patch object")
 }

+// WithMergeOptions returns an ApplyOption for merging the value at the given
+// fieldPath of desired object onto the current object with
+// the given merge options.


I wonder whether we should consider moving to server side apply rather than building our own merge implementation client side?

https://kubernetes.io/docs/reference/using-api/server-side-apply/#merge-strategy

I will take a look.

Hi @negz,
Took a look at server-side apply. My understanding is that it is mainly addressing a somewhat different kind of a problem than the feature we are trying to implement, though we can employ conflict resolution mechanics of server-side apply to achieve merging/replacing fields as you suggested. And this does not mean that we cannot change the feature formulation (or that my understanding does not have flaws).

My understanding is that server-side apply primarily aims a "more stable object lifecycle" by keeping track of appliers (aka fieldManagers) as the managers of a set of fields, and giving a conflict if a field is being modified by another applier than the current fieldManager. We can control how field ownership will be defined, at "compile" time, via merge strategies, e.g., define a merge-strategy of granular for a map if we would like to allow different appliers to control different keys of a map, or a merge-strategy of atomic if would like the map to be managed by a single applier with all its keys.

With the current implementation here, we are allowing a composition to specify the merge strategy at runtime and it works bidirectionally, i.e., while merging from composite into composed or from composed into composite. One composition can request a different merge strategy than another.

I'm not sure but server-side apply sounded to me like something of a broader context than adding this feature. For example, I think we may want to reconsider resource.APIPatchingApplicator if we decide to employ apply patches and the reconcilers themselves (though we may also choose to use server-side apply only in specific contexts).

pkg/fieldpath/merge_test.go

pkg/fieldpath/merge.go

negz · 2021-07-30T00:30:17Z

pkg/fieldpath/merge.go

+
+// MergePath merges the value at the given field path of the src object into
+// the dst object.
+func MergePath(fieldPath string, dst, src runtime.Object, mergeOptions *MergeOptions) error {


I'm not sure how I feel about making this package deal more directly with runtime.Object - i.e. taking them as an argument. So far the established pattern has been to convert a runtime.Object (or anything else) to a *Paved, then operate on that. What do you think about implementing MergePath as a method on *Paved similar to the current SetValue? For example:

func (p *Paved) MergeValue(path string, value interface{}, o ...MergeOption) error

Added Paved.MergeValue as suggested.

I see benefit in introducing a layer which can handle runtime.Objects so that as demonstrated in the dependent PR crossplane/crossplane/pull/2409, so I moved those newly introduced functions handling with runtime.Object into the fieldpath/object package. We could also move them to core crossplane. What do you think?

Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>

- Add MergeReplace in fieldpath package for ease of writing tests. Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>

- Add "fieldpath/object" package that deals with runtime.Objects - Move MergeOptions struct to package "apis/common/v1". Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>

negz · 2021-08-02T18:09:14Z

pkg/fieldpath/object/object.go

+// ToPaved tries to convert a runtime.Object into a *Paved via
+// runtime.DefaultUnstructuredConverter if needed. Returns the paved if
+// the conversion is successful along with whether the
+// runtime.DefaultUnstructuredConverter has been employed during the
+// conversion.


I see that when you call this function you name the bool return value copied. It would be useful to include this information in the docstring. From my reader's perspective it is not obvious what the implications of runtime.DefaultUnstructuredConverter being employed is - I can only infer from the function below that it means the returned value is a copy when it would otherwise not be.

Also, note that there is already a fieldpath.PaveObject function that has very similar functionality to this one. The key difference being that it always invokes the DefaultUnstructuredConverter. Do we need both?

Yes, I also do not like that we have two functions with exactly the same duty. The newly added one (ToPaved), as you see, has a shortcut for types that provide an UnstructuredContent() method, a performance optimization that we can live without. We can remove ToPaved and always copy back to the runtime.Object at the call site via runtime.DefaultUnstructuredConverter.FromUnstructured.

Because fieldpath.PaveObject is already part of the public API, I did not want to remove it. Maybe we can declare it as deprecated in favor of ToPaved. I don't have a strong opinion here.

If we decide to keep ToPaved, I will update the documentation as you suggested above.

Thanks! I don't have a strong opinion either. Usually I would bias for API simplicity over performance optimisations though, at least until we experience noticeable performance issues. In this case I'm happy for you to choose - we can either deprecate the old function or not add the new one.

Removed fieldpath.ToPaved and kept fieldpath.PaveObject.

negz · 2021-08-02T18:22:15Z

pkg/fieldpath/object/object.go

+	return fieldpath.Pave(oMap), true, nil
+}
+
+// PatchFieldValueToObject applies the value to the "to" object at the given


I'm still a little uncomfortable with the fact that we're adding what appear to be convenience functions that operate on runtime.Object for patching, but not for any other part of the fieldpath package.

My concern is not in thinking that runtime.Object is the wrong level to operate at, but rather that merging now appears to be a special case that is asymmetrical from the rest of the API. If the caller wants to set or get a value they handle conversion to a *Paved and use SetValue and GetValue. However if the caller wants to patch, they use this special package that operates on runtime.Object.

Perhaps this makes sense if you imagine this package would be expanded to handle more than just merging with time?

I agree. That's why I had also suggested that we can maybe move these convenience functions to core crossplane. The Paved.MergeValue is coherent with the rest of the fieldpath package, but we also need some higher level functions that will extract the value to be merged into the destination from a source runtime.Object, as you also acknowledge.

What do you think about moving these convenience functions to crossplane/crossplane (I'm calling this as "core crossplane", not sure if it conveys the right meaning)? I'm also fine if they stay under fieldpath/object as they act as a bridge between runtime.Objects and Paveds. Maybe a little less true for object.MergeReplace, but then even object.MergeReplace accepts a path parameter.

Hrm I can see a case either way. Perhaps we should start with these in core Crossplane - we can move them here if we find our library of functions that operate on objects grows.

I've moved the functions in fieldpath.object to core Crossplane including resource.WithMergeOptions and unexported them. If we need to use them elsewhere, we can consider to export them but for now, no need to export them. Please see the dependent PR.

…is set - Move resource.WithMergeOptions to core Crossplane and unexport - Move fieldpath.object functions to core Crossplane and unexport - Move fieldpath.MergeValue & related functions to its own file - Add tests for fieldpath.MergeValue Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>

ulucinar · 2021-08-04T18:56:12Z

Hi @negz,
Thank you very much for the review. I really appreciate it. I see it as a great learning opportunity.

I realized that when MergeOptions.AppendSlice is set and the intersection between the destination and the source slices is not empty, we get duplicate items in the destination slice. I believe this is not the UX we would want and I've added fieldpath.removeSourceDuplicates to handle duplicates by default. I have also added test cases in pkg/fieldpath/merge_test.go for this case and various others.

Do you think we had better add a new MergeOption that allows duplicates when used together with MergeOptions.AppendSlice? My opinion is that we do not need unless a good use-case pops up.

negz

Thanks for persisting with this @ulucinar! One last thing, but otherwise this looks good to me.

My opinion is that we do not need unless a good use-case pops up.

I agree - let's wait for now.

pkg/fieldpath/merge.go

Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>

ulucinar mentioned this pull request Jul 9, 2021

Support merging instead of replacing in patches crossplane/crossplane#2409

Merged

3 tasks

negz reviewed Jul 30, 2021

View reviewed changes

ulucinar added 3 commits August 2, 2021 14:18

Support merging in addition to replacing objects in patches

6f106aa

Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>

Add tests for merge.go

fcbfd04

- Add MergeReplace in fieldpath package for ease of writing tests. Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>

Add Paved.MargeValue method

e7b4a22

- Add "fieldpath/object" package that deals with runtime.Objects - Move MergeOptions struct to package "apis/common/v1". Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>

ulucinar force-pushed the aru/fix-2211 branch from 8f0fbbc to e7b4a22 Compare August 2, 2021 11:20

negz reviewed Aug 2, 2021

View reviewed changes

ulucinar force-pushed the aru/fix-2211 branch from efccf99 to 4d6ec58 Compare August 4, 2021 18:38

negz approved these changes Aug 4, 2021

View reviewed changes

pkg/fieldpath/merge.go Show resolved Hide resolved

Improve comments on fieldpath.merge

0127cd0

Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>

negz merged commit 047d938 into crossplane:master Aug 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support merging in addition to replacing objects in patches #270

Support merging in addition to replacing objects in patches #270

ulucinar commented Jul 8, 2021 •

edited

Loading

negz Jul 30, 2021

ulucinar Jul 30, 2021

negz Jul 30, 2021

ulucinar Aug 2, 2021

negz Jul 30, 2021

ulucinar Aug 2, 2021

ulucinar Aug 2, 2021

negz Jul 30, 2021

ulucinar Aug 2, 2021

negz Aug 2, 2021

negz Aug 2, 2021

ulucinar Aug 2, 2021

ulucinar Aug 2, 2021

negz Aug 3, 2021

ulucinar Aug 4, 2021

negz Aug 2, 2021

ulucinar Aug 2, 2021 •

edited

Loading

negz Aug 3, 2021

ulucinar Aug 4, 2021

ulucinar commented Aug 4, 2021 •

edited

Loading

negz left a comment •

edited

Loading

Support merging in addition to replacing objects in patches #270

Support merging in addition to replacing objects in patches #270

Conversation

ulucinar commented Jul 8, 2021 • edited Loading

Description of your changes

How has this code been tested

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ulucinar Aug 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ulucinar commented Aug 4, 2021 • edited Loading

negz left a comment • edited Loading

Choose a reason for hiding this comment

ulucinar commented Jul 8, 2021 •

edited

Loading

ulucinar Aug 2, 2021 •

edited

Loading

ulucinar commented Aug 4, 2021 •

edited

Loading

negz left a comment •

edited

Loading