Merge options are declared per patch, but are actually scoped to the entire MR being composed

[LCaparelli]

What happened?

I have a patch that alters an array at index 0. Then I have another patch that alters the same array, but by appending to it, using the appendSlice merge option.

This second patch sources data from an optional input field in the claim. For simplicity's sake, in the example I'll list, this input is always null/empty. This implies that the patch should be ignored altogether, but you'll see that's not entirely accurate.

Since the appendSlice merge option is only specified in the second patch, I expected only the input from that patch to be appended to the resulting array at the managed resource being composed.

Instead, the merge option also affects the first patch, causing the changes meant to be written at index 0 to actually be written to a new element of the array being appended to the original array.

Initial State at the managed resource (irrelevant fields omitted for brevity):

containers:
- name: reproducer
  image: some-image:v0.0.1

After updating the image to v0.0.2 in the claim, actual outcome:

containers:
- name: reproducer
  image: some-image:v0.0.1
- name: reproducer
  image: some-image:v0.0.2

Expected outcome:

containers:
- name: reproducer
  image: some-image:v0.0.2

How can we reproduce it?

XRD

apiVersion: apiextensions.crossplane.io/v1
kind: CompositeResourceDefinition
metadata:
  name: xworkloads.foo.com
spec:
  claimNames:
    kind: Workload
    plural: workloads
  defaultCompositeDeletePolicy: Foreground
  defaultCompositionUpdatePolicy: Automatic
  group: foo.com
  names:
    kind: XWorkload
    plural: xworkloads
  versions:
    - name: v1alpha1
      referenceable: true
      schema:
        openAPIV3Schema:
          type: object
          properties:
            spec:
              type: object
              properties:
                additionalContainers:
                  description: >-
                    List of additional containers (sidecars) belonging to the
                    pod. Cannot be updated.
                  items:
                    type: object
                    description: >-
                      A single application container that you want to run within
                      a pod.
                    properties:
                      image:
                        type: string
                      name:
                        type: string
                      ports:
                        type: array
                        items:
                          type: object
                          properties:
                            containerPort:
                              type: number
                  type: array
                image:
                  type: object
                  properties:
                    name:
                      type: string
                    tag:
                      type: string
          required:
            - spec
      served: true

Composition

apiVersion: apiextensions.crossplane.io/v1
kind: Composition
metadata:
  name: xworkloads.foo.com
spec:
  compositeTypeRef:
    apiVersion: foo.com/v1alpha1
    kind: XWorkload
  mode: Resources
  resources:
    - name: deployment
      base:
        apiVersion: kubernetes.crossplane.io/v1alpha2
        kind: Object
        spec:
          deletionPolicy: Orphan
          forProvider:
            manifest:
              apiVersion: apps/v1
              kind: Deployment
              metadata:
                name: reproducer
                namespace: default
              spec:
                replicas: 1
                selector:
                  matchLabels:
                    workloads.foo.com/name: reproducer
                template:
                  metadata:
                    labels:
                      workloads.foo.com/name: reproducer
                  spec:
                    containers:
                    - name: reproducer
                      ports:
                      - containerPort: 8080
      patches:
        - type: CombineFromComposite
          toFieldPath: 'spec.forProvider.manifest.spec.template.spec.containers[0].image'
          combine:
            strategy: string
            string:
              fmt: '%s:%s'
            variables:
              - fromFieldPath: spec.image.name
              - fromFieldPath: spec.image.tag
        - type: FromCompositeFieldPath
          fromFieldPath: spec.additionalContainers
          toFieldPath: spec.forProvider.manifest.spec.template.spec.containers
          policy:
            mergeOptions:
              appendSlice: true

You can either install the kubernetes-provider locally, that provides the Object managed resource as an API, or directly install this CRD + RBAC which I adapted from the provider's current version to make sure the provider itself has no role in this behavior:

Object CRD + RBAC

apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
  name: crossplane-admin-kubernetes.crossplane.io
rules:
- apiGroups:
  - kubernetes.crossplane.io
  resources:
  - '*'
  verbs:
  - '*'
---
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
metadata:
  name: crossplane-admin-kubernetes.crossplane.io
roleRef:
  apiGroup: rbac.authorization.k8s.io
  kind: ClusterRole
  name: crossplane-admin-objects
subjects:
- kind: ServiceAccount
  name: crossplane
  namespace: crossplane-system
---
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
  annotations:
    controller-gen.kubebuilder.io/version: v0.14.0
  name: objects.kubernetes.crossplane.io
spec:
  group: kubernetes.crossplane.io
  names:
    categories:
    - crossplane
    - managed
    - kubernetes
    kind: Object
    listKind: ObjectList
    plural: objects
    singular: object
  scope: Cluster
  versions:
  - additionalPrinterColumns:
    - jsonPath: .spec.forProvider.manifest.kind
      name: KIND
      type: string
    - jsonPath: .spec.forProvider.manifest.apiVersion
      name: APIVERSION
      priority: 1
      type: string
    - jsonPath: .spec.forProvider.manifest.metadata.name
      name: METANAME
      priority: 1
      type: string
    - jsonPath: .spec.forProvider.manifest.metadata.namespace
      name: METANAMESPACE
      priority: 1
      type: string
    - jsonPath: .spec.providerConfigRef.name
      name: PROVIDERCONFIG
      type: string
    - jsonPath: .status.conditions[?(@.type=='Synced')].status
      name: SYNCED
      type: string
    - jsonPath: .status.conditions[?(@.type=='Ready')].status
      name: READY
      type: string
    - jsonPath: .metadata.creationTimestamp
      name: AGE
      type: date
    name: v1alpha2
    schema:
      openAPIV3Schema:
        description: A Object is an provider Kubernetes API type
        properties:
          apiVersion:
            description: |-
              APIVersion defines the versioned schema of this representation of an object.
              Servers should convert recognized schemas to the latest internal value, and
              may reject unrecognized values.
              More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources
            type: string
          kind:
            description: |-
              Kind is a string value representing the REST resource this object represents.
              Servers may infer this from the endpoint the client submits requests to.
              Cannot be updated.
              In CamelCase.
              More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds
            type: string
          metadata:
            type: object
          spec:
            description: A ObjectSpec defines the desired state of a Object.
            properties:
              connectionDetails:
                items:
                  description: ConnectionDetail represents an entry in the connection
                    secret for an Object
                  properties:
                    apiVersion:
                      description: API version of the referent.
                      type: string
                    fieldPath:
                      description: |-
                        If referring to a piece of an object instead of an entire object, this string
                        should contain a valid JSON/Go field access statement, such as desiredState.manifest.containers[2].
                        For example, if the object reference is to a container within a pod, this would take on a value like:
                        "spec.containers{name}" (where "name" refers to the name of the container that triggered
                        the event) or if no container name is specified "spec.containers[2]" (container with
                        index 2 in this pod). This syntax is chosen only to have some well-defined way of
                        referencing a part of an object.
                        TODO: this design is not final and this field is subject to change in the future.
                      type: string
                    kind:
                      description: |-
                        Kind of the referent.
                        More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds
                      type: string
                    name:
                      description: |-
                        Name of the referent.
                        More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names
                      type: string
                    namespace:
                      description: |-
                        Namespace of the referent.
                        More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/namespaces/
                      type: string
                    resourceVersion:
                      description: |-
                        Specific resourceVersion to which this reference is made, if any.
                        More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#concurrency-control-and-consistency
                      type: string
                    toConnectionSecretKey:
                      type: string
                    uid:
                      description: |-
                        UID of the referent.
                        More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#uids
                      type: string
                  type: object
                  x-kubernetes-map-type: atomic
                type: array
              deletionPolicy:
                default: Delete
                description: |-
                  DeletionPolicy specifies what will happen to the underlying external
                  when this managed resource is deleted - either "Delete" or "Orphan" the
                  external resource.
                  This field is planned to be deprecated in favor of the ManagementPolicies
                  field in a future release. Currently, both could be set independently and
                  non-default values would be honored if the feature flag is enabled.
                  See the design doc for more information: https://github.com/crossplane/crossplane/blob/499895a25d1a1a0ba1604944ef98ac7a1a71f197/design/design-doc-observe-only-resources.md?plain=1#L223
                enum:
                - Orphan
                - Delete
                type: string
              forProvider:
                description: ObjectParameters are the configurable fields of a Object.
                properties:
                  manifest:
                    description: Raw JSON representation of the kubernetes object
                      to be created.
                    type: object
                    x-kubernetes-embedded-resource: true
                    x-kubernetes-preserve-unknown-fields: true
                required:
                - manifest
                type: object
              managementPolicies:
                default:
                - '*'
                description: |-
                  THIS IS A BETA FIELD. It is on by default but can be opted out
                  through a Crossplane feature flag.
                  ManagementPolicies specify the array of actions Crossplane is allowed to
                  take on the managed and external resources.
                  This field is planned to replace the DeletionPolicy field in a future
                  release. Currently, both could be set independently and non-default
                  values would be honored if the feature flag is enabled. If both are
                  custom, the DeletionPolicy field will be ignored.
                  See the design doc for more information: https://github.com/crossplane/crossplane/blob/499895a25d1a1a0ba1604944ef98ac7a1a71f197/design/design-doc-observe-only-resources.md?plain=1#L223
                  and this one: https://github.com/crossplane/crossplane/blob/444267e84783136daa93568b364a5f01228cacbe/design/one-pager-ignore-changes.md
                items:
                  description: |-
                    A ManagementAction represents an action that the Crossplane controllers
                    can take on an external resource.
                  enum:
                  - Observe
                  - Create
                  - Update
                  - Delete
                  - LateInitialize
                  - '*'
                  type: string
                type: array
              providerConfigRef:
                default:
                  name: default
                description: |-
                  ProviderConfigReference specifies how the provider that will be used to
                  create, observe, update, and delete this managed resource should be
                  configured.
                properties:
                  name:
                    description: Name of the referenced object.
                    type: string
                  policy:
                    description: Policies for referencing.
                    properties:
                      resolution:
                        default: Required
                        description: |-
                          Resolution specifies whether resolution of this reference is required.
                          The default is 'Required', which means the reconcile will fail if the
                          reference cannot be resolved. 'Optional' means this reference will be
                          a no-op if it cannot be resolved.
                        enum:
                        - Required
                        - Optional
                        type: string
                      resolve:
                        description: |-
                          Resolve specifies when this reference should be resolved. The default
                          is 'IfNotPresent', which will attempt to resolve the reference only when
                          the corresponding field is not present. Use 'Always' to resolve the
                          reference on every reconcile.
                        enum:
                        - Always
                        - IfNotPresent
                        type: string
                    type: object
                required:
                - name
                type: object
              publishConnectionDetailsTo:
                description: |-
                  PublishConnectionDetailsTo specifies the connection secret config which
                  contains a name, metadata and a reference to secret store config to
                  which any connection details for this managed resource should be written.
                  Connection details frequently include the endpoint, username,
                  and password required to connect to the managed resource.
                properties:
                  configRef:
                    default:
                      name: default
                    description: |-
                      SecretStoreConfigRef specifies which secret store config should be used
                      for this ConnectionSecret.
                    properties:
                      name:
                        description: Name of the referenced object.
                        type: string
                      policy:
                        description: Policies for referencing.
                        properties:
                          resolution:
                            default: Required
                            description: |-
                              Resolution specifies whether resolution of this reference is required.
                              The default is 'Required', which means the reconcile will fail if the
                              reference cannot be resolved. 'Optional' means this reference will be
                              a no-op if it cannot be resolved.
                            enum:
                            - Required
                            - Optional
                            type: string
                          resolve:
                            description: |-
                              Resolve specifies when this reference should be resolved. The default
                              is 'IfNotPresent', which will attempt to resolve the reference only when
                              the corresponding field is not present. Use 'Always' to resolve the
                              reference on every reconcile.
                            enum:
                            - Always
                            - IfNotPresent
                            type: string
                        type: object
                    required:
                    - name
                    type: object
                  metadata:
                    description: Metadata is the metadata for connection secret.
                    properties:
                      annotations:
                        additionalProperties:
                          type: string
                        description: |-
                          Annotations are the annotations to be added to connection secret.
                          - For Kubernetes secrets, this will be used as "metadata.annotations".
                          - It is up to Secret Store implementation for others store types.
                        type: object
                      labels:
                        additionalProperties:
                          type: string
                        description: |-
                          Labels are the labels/tags to be added to connection secret.
                          - For Kubernetes secrets, this will be used as "metadata.labels".
                          - It is up to Secret Store implementation for others store types.
                        type: object
                      type:
                        description: |-
                          Type is the SecretType for the connection secret.
                          - Only valid for Kubernetes Secret Stores.
                        type: string
                    type: object
                  name:
                    description: Name is the name of the connection secret.
                    type: string
                required:
                - name
                type: object
              readiness:
                description: |-
                  Readiness defines how the object's readiness condition should be computed,
                  if not specified it will be considered ready as soon as the underlying external
                  resource is considered up-to-date.
                properties:
                  policy:
                    default: SuccessfulCreate
                    description: Policy defines how the Object's readiness condition
                      should be computed.
                    enum:
                    - SuccessfulCreate
                    - DeriveFromObject
                    - AllTrue
                    type: string
                type: object
              references:
                items:
                  description: |-
                    Reference refers to an Object or arbitrary Kubernetes resource and optionally
                    patch values from that resource to the current Object.
                  properties:
                    dependsOn:
                      description: |-
                        DependsOn is used to declare dependency on other Object or arbitrary
                        Kubernetes resource.
                      properties:
                        apiVersion:
                          default: kubernetes.crossplane.io/v1alpha1
                          description: APIVersion of the referenced object.
                          type: string
                        kind:
                          default: Object
                          description: Kind of the referenced object.
                          type: string
                        name:
                          description: Name of the referenced object.
                          type: string
                        namespace:
                          description: Namespace of the referenced object.
                          type: string
                      required:
                      - name
                      type: object
                    patchesFrom:
                      description: |-
                        PatchesFrom is used to declare dependency on other Object or arbitrary
                        Kubernetes resource, and also patch fields from this object.
                      properties:
                        apiVersion:
                          default: kubernetes.crossplane.io/v1alpha1
                          description: APIVersion of the referenced object.
                          type: string
                        fieldPath:
                          description: |-
                            FieldPath is the path of the field on the resource whose value is to be
                            used as input.
                          type: string
                        kind:
                          default: Object
                          description: Kind of the referenced object.
                          type: string
                        name:
                          description: Name of the referenced object.
                          type: string
                        namespace:
                          description: Namespace of the referenced object.
                          type: string
                      required:
                      - fieldPath
                      - name
                      type: object
                    toFieldPath:
                      description: |-
                        ToFieldPath is the path of the field on the resource whose value will
                        be changed with the result of transforms. Leave empty if you'd like to
                        propagate to the same path as patchesFrom.fieldPath.
                      type: string
                  type: object
                type: array
              writeConnectionSecretToRef:
                description: |-
                  WriteConnectionSecretToReference specifies the namespace and name of a
                  Secret to which any connection details for this managed resource should
                  be written. Connection details frequently include the endpoint, username,
                  and password required to connect to the managed resource.
                  This field is planned to be replaced in a future release in favor of
                  PublishConnectionDetailsTo. Currently, both could be set independently
                  and connection details would be published to both without affecting
                  each other.
                properties:
                  name:
                    description: Name of the secret.
                    type: string
                  namespace:
                    description: Namespace of the secret.
                    type: string
                required:
                - name
                - namespace
                type: object
            required:
            - forProvider
            type: object
          status:
            description: A ObjectStatus represents the observed state of a Object.
            properties:
              atProvider:
                description: ObjectObservation are the observable fields of a Object.
                properties:
                  manifest:
                    description: Raw JSON representation of the remote object.
                    type: object
                    x-kubernetes-embedded-resource: true
                    x-kubernetes-preserve-unknown-fields: true
                type: object
              conditions:
                description: Conditions of the resource.
                items:
                  description: A Condition that may apply to a resource.
                  properties:
                    lastTransitionTime:
                      description: |-
                        LastTransitionTime is the last time this condition transitioned from one
                        status to another.
                      format: date-time
                      type: string
                    message:
                      description: |-
                        A Message containing details about this condition's last transition from
                        one status to another, if any.
                      type: string
                    reason:
                      description: A Reason for this condition's last transition from
                        one status to another.
                      type: string
                    status:
                      description: Status of this condition; is it currently True,
                        False, or Unknown?
                      type: string
                    type:
                      description: |-
                        Type of this condition. At most one of each condition type may apply to
                        a resource at any point in time.
                      type: string
                  required:
                  - lastTransitionTime
                  - reason
                  - status
                  - type
                  type: object
                type: array
                x-kubernetes-list-map-keys:
                - type
                x-kubernetes-list-type: map
            type: object
        required:
        - spec
        type: object
    served: true
    storage: true
    subresources:
      status: {}

After applying the provided XRD and composition, apply this workload:

apiVersion: foo.com/v1alpha1
kind: Workload
metadata:
  name: reproducer
spec:
  image:
    name: argoproj/rollouts-demo
    tag: blue

Check the resulting object, it should hold a single container (which is fine and expected). Now patch the image, simulating a bump:

kubectl patch workload reproducer --type=merge --patch '"spec": {"image": {"tag": "orange"}}'

The resulting Object (MR being composed) now has 2 containers:

containers:
- image: argoproj/rollouts-demo:blue
  name: reproducer
  ports:
  - containerPort: 8080
- image: argoproj/rollouts-demo:orange
  name: reproducer
  ports:
  - containerPort: 8080

Code Analysis

It seems we render the entire composed resource based on the patches first in this loop:

crossplane/internal/controller/apiextensions/composite/composition_pt.go

Line 200 in a2f63b3

for i := range tas {

Then apply in this loop that comes right after:

crossplane/internal/controller/apiextensions/composite/composition_pt.go

Line 261 in a2f63b3

for i := range tas {

When applying, we incorporate the merge options defined in any of the patches from the composition:

crossplane/internal/controller/apiextensions/composite/composition_pt.go

Lines 273 to 275 in a2f63b3

	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 {

This effectively means that the merge options are scoped to the entire MR being composed, not scoped to the patch where they're declared.

Additionally, because the source field for this second patch is null/empty, this also means that the merge option defined in patch that should be ignored causes side-effects.

What environment did it happen in?

Crossplane version: v1.15.2

[LCaparelli]

I'm not super familiar with the code base, please let me know if this analysis is not correct.

I think this raises a few questions:

1. Do we actually want the current behavior?
   a. If so, will we change the interface to make it clear merge options are not patch-scoped, but composed-resource-scoped?
   b. If not, many compositions out there might already depend on this behavior to work correctly, how to tackle this?
2. How can we improve the merge options documentation to illustrate its nuances?
3. How does this affect, if at all, server-side applies?

I'm willing to contribute with whatever is necessary, but at this point I think we need to further discuss what direction we should move to solve this.

[jbw976]

Great details to explain this behavior, how it can be reproduced, and some analysis of the code base - really impressive effort here @LCaparelli, that accelerates folks understanding of the issue!

I don't have the full deep context of the codebase, but I didn't catch anything obviously wrong with your analysis yet. One thing to note is that the behavior to consider the mergeOptions for all patches on the composed resource goes back at least 3 years, if not longer:
https://github.com/crossplane/crossplane/blame/master/internal/controller/apiextensions/composite/merge.go#L75

So this behavior has indeed been present for awhile - I'm anticipating there's a good reason for it, perhaps to avoid conflicting behavior across patches, but I don't have the depth of context to really know why.

That being said, a couple questions for you:

• Have you happened to try this scenario with function-patch-and-transform yet? The mergeOptions in classic patch & transform have been replaced with a policy.toFieldPath approach. I don't fully expect the behavior to differ here, but it may be useful to try.
• Have you considered a design change to your XRD/claim schema? Instead of separate spec.image and spec.additionalContainers fields, would it be possible to just make spec.image an array that can support multiple entries? e.g.:

image:
- name: argoproj/rollouts-demo
  tag: blue
# claim user can optionally add additional entries to this array
- name: container2
  tag: some-image:v0.0.1

[LCaparelli]

Thanks for taking the time to have a look at this @jbw976!

Have you considered a design change to your XRD/claim schema? Instead of separate spec.image and spec.additionalContainers fields, would it be possible to just make spec.image an array that can support multiple entries? e.g.:

We ended up setting a static limit to the number of additional containers (usually 5 sidecars is already quite a lot), so that we can have patches for specific indexes while still keeping the abstraction level we wanted for the main container (sidecars can be something we don't maintain ourselves, so we want that to be flexible and allow users to set whatever container fields they want).

        # the static patches solve temporary issues regarding containers duplication
      - type: FromCompositeFieldPath
        fromFieldPath: spec.additionalContainers[0]
        toFieldPath: spec.forProvider.manifest.spec.template.spec.containers[1]
      - type: FromCompositeFieldPath
        fromFieldPath: spec.additionalContainers[1]
        toFieldPath: spec.forProvider.manifest.spec.template.spec.containers[2]
      - type: FromCompositeFieldPath
        fromFieldPath: spec.additionalContainers[2]
        toFieldPath: spec.forProvider.manifest.spec.template.spec.containers[3]
      - type: FromCompositeFieldPath
        fromFieldPath: spec.additionalContainers[3]
        toFieldPath: spec.forProvider.manifest.spec.template.spec.containers[4]
      - type: FromCompositeFieldPath
        fromFieldPath: spec.additionalContainers[4]
        toFieldPath: spec.forProvider.manifest.spec.template.spec.containers[5]

Will probably solve this more elegantly with function-go-templating (or similar) so we can iterate indefinetely.

Have you happened to try this scenario with function-patch-and-transform yet? The mergeOptions in classic patch & transform have been replaced with a policy.toFieldPath approach. I don't fully expect the behavior to differ here, but it may be useful to try.

I'll give that a swing (hopefully) soon, still hadn't had time to do it. 😅

Replying right now just to share the workaround we found.