Tag deleted from claim spec is added back

[ulucinar]

What happened?

We have observed an issue with @stevendborrelli where a tag is deleted from a map in a claim spec parameter, and it gets added back to the claim. Please see below for an example XRD, claim and composition.

After you remove the tag key from the claim’s spec.parameters.tags map (and expect first the XR’s corresponding map to be updated, and then the MR’s spec.forProvider.tags map to be replaced because of the composition’s relevant patch), the removed key gets reinserted by the claim.APIClaimConfigurator, which configures a claim using the associated XR. At this point, the tag key is missing (from spec.parameters.tags) in the claim but it still exists in the XR. The APIClaimConfigurator merges the spec of the XR into the claim’s spec using mergo.Merge. And mergo.Merge puts the missing key in the claim’s spec.parameters.tags from the XR’s map (which still contains it) while merging these maps.

There is no race condition involved. The claim reconciler uses a resource.APIPatchingApplicator to patch the XR via a JSON-merge patch but this can not remove the key (map not replaced, there is now a missing key in a map, which has no effect). However, resource.APIPatchingApplicator fetches the current spec of the XR and puts it into the same object that will later be used to configure the claim (via the APIClaimConfigurator as explained above).
Thus the claim acquires back the deleted key.

How can we reproduce it?

Using the following XRD, claim and composition, please delete the test key from claim's (AWSGlueTrigger) spec.parameters.tags:

apiVersion: dataservices.lmig.com/v1alpha1
kind: AWSGlueTrigger
metadata:
  namespace: upbound-system
  name: ateam-glue-test-trigger-instance1
  annotations:
    crossplane.io/external-name: ateam-glue-trigger-tags-test
spec:
  parameters:
    description: "test trigger for A-team test 2"
    workflowName: "a-team-workflow1"
    type: ON_DEMAND
    actions:
      - jobName: "ateam-glue-name1"
    tags:
      deployment_guid: "0000-9-9-99999"
      lm-troux_uid: "0000-9-9-99999"
      test: new_tag
  compositionSelector:
    matchLabels:
      purpose: glue-trigger
      provider: aws

---
apiVersion: apiextensions.crossplane.io/v1
kind: Composition
metadata:
  name: gds-glue-trigger-pattern
  labels:
    crossplane.io/xrd: xawsgluetriggers.dataservices.lmig.com
    purpose: glue-trigger
    provider: aws
spec:
  compositeTypeRef:
    apiVersion: dataservices.lmig.com/v1alpha1
    kind: XAWSGlueTrigger
  writeConnectionSecretsToNamespace: default
  resources:
  - name: glue-trigger
    base:
      apiVersion: glue.aws.upbound.io/v1beta1
      kind: Trigger
      spec:
        forProvider:
          region: us-east-1
          tags: {}
    patches:
      - fromFieldPath: "metadata.annotations[crossplane.io/external-name]"
        toFieldPath: "metadata.annotations[crossplane.io/external-name]"
      - type: FromCompositeFieldPath
        fromFieldPath: spec.parameters.description
        toFieldPath: spec.forProvider.description
      - type: FromCompositeFieldPath
        fromFieldPath: spec.parameters.workflowName
        toFieldPath: spec.forProvider.workflowName
      - type: FromCompositeFieldPath
        fromFieldPath: spec.parameters.type
        toFieldPath: spec.forProvider.type
      - type: FromCompositeFieldPath
        fromFieldPath: spec.parameters.actions
        toFieldPath: spec.forProvider.actions
      - type: FromCompositeFieldPath
        fromFieldPath: spec.parameters.predicate
        toFieldPath: spec.forProvider.predicate
      - type: FromCompositeFieldPath
        fromFieldPath: spec.parameters.startOnCreation
        toFieldPath: spec.forProvider.startOnCreation
      - type: FromCompositeFieldPath
        fromFieldPath: spec.parameters.tags
        toFieldPath: spec.forProvider.tags


---

apiVersion: apiextensions.crossplane.io/v1
kind: CompositeResourceDefinition
metadata:
  name: xawsgluetriggers.dataservices.lmig.com
  labels:
    purpose: glue-trigger
    provider: aws
spec:
  defaultCompositionRef:
    name: gds-glue-trigger-pattern
  group: dataservices.lmig.com
  names:
    kind: XAWSGlueTrigger
    plural: xawsgluetriggers
  claimNames:
    kind: AWSGlueTrigger
    plural: awsgluetriggers
  versions:
    - name: v1alpha1
      served: true
      referenceable: true
      schema:
        openAPIV3Schema:
          type: object
          properties:
            spec:
              type: object
              properties:
                parameters:
                  type: object
                  properties:
                    description:
                      type: string
                    workflowName:
                      type: string
                    startOnCreation:
                      type: boolean
                    type:
                      type: string
                      enum: [CONDITIONAL, ON_DEMAND, SCHEDULED]
                    actions:
                      type: array
                      items:
                        type: object
                        properties:
                          jobName:
                            type: string
                          arguments:
                            type: object
                            additionalProperties:
                              type: string
                    predicate:
                      type: array
                      items:
                        type: object
                        properties:
                          logical:
                            type: string
                            enum: [AND, ANY]
                          conditions:
                            type: array
                            items:
                              type: object
                              properties:
                                operator:
                                  type: string
                                jobName:
                                  type: string
                                state:
                                  type: string
                                  enum: [SUCCEEDED, STOPPED, TIMEOUT, FAILED]
                        required:
                          - conditions
                    tags:
                      description: Tags map required for comliance
                      type: object
                      additionalProperties:
                          type: string
                  required:
                    - actions
                    - tags
                    - type
              required:
                - parameters

What environment did it happen in?

Crossplane version: v1.10.0

[haarchri]

If you stop crossplane and remove and start again it should working - did you tried ?

[github-actions]

Crossplane does not currently have enough maintainers to address every issue and pull request. This issue has been automatically marked as stale because it has had no activity in the last 90 days. It will be closed in 7 days if no further activity occurs. Leaving a comment starting with /fresh will mark this issue as not stale.

[ytsarev]

/fresh looks still relevant

[turkenh]

I did run some experiments to see whether server side apply can help here, my findings so far:

• Server Side Apply resolves this issue with the changes in this branch.
• However, even though the tag removed from the claim was removed from the composite, it was still present in the MR.
• According to the Composition documentation and this PR’s description, the default behavior should actually replace instead of a merge. That part could be a regression.

Overall, the server-side seems promising, and we can at least change the claim controllers applicator from patching to server-side apply.

Switching to server-side apply for applying composed resources is definitely a broader topic. We need to spend more time on that and ensure we do enough testing.

One idea could be switching only to NewPTFComposer, which is used when the composition functions are enabled.

[LCaparelli]

However, even though the tag removed from the claim was removed from the composite, it was still present in the MR.

This is in line with the behavior I'm observing in a composition of our own to manage AWS RDS Aurora DBClusters.

Different from this case, it's not a tag that is never removed, but a simple field.

What is also different, is that we're able to remove it from the claim with no problem, but it never gets removed from the composite, as you've described with server-side apply:

❯ kubectl get dbcluster database-sample-serverlessv2 -o yaml | egrep "^\s+deletionProtection"
❯ kubectl get xdbcluster database-sample-serverlessv2-htx7l -o yaml | egrep "^\s+deletionProtection"
  deletionProtection: true

Unfortunately, our composition and XRD are quite large, using some CRDs of our own as well, so I doubt it will be of much help in investigating due to all the noise. I was not able to create a small reproducer, all my attempts lead to the behavior described in this issue: I'm unable to remove it from the claim itself.

[pedjak]

Fixed via #4896.