Composition patch order changed with v1.14.0

[MisterMX]

What happened?

There is an undocumented breaking change introduced with #4500 to the ordering of patches, specifically patches from environment patches:

• Before v1.14.0 environment patches have been executed in order with "regular" from-XR patches.
• With v1.14.0 environment patches are no always executed after all from-XR patches have been run.

This breaks with existing patching behaviour and can make compositions that rely on patch ordering fail or even unusable with v1.14.0. For example if the environment contains the default values which might be overriden by an optional user field provided with the XR.

The breaking change has been made in https://github.com/crossplane/crossplane/pull/4500/files#diff-574bed872a28203735722ddf7a98e52ad596702e555ed0dda6525e4e578481f2R225

How can we reproduce it?

Create a composition with environment patches at index 0 and an XR patch that write to the same field at index 1. In v1.13 the field will contain the XR value. In v1.14 the field will contain the value of the environment.

What environment did it happen in?

Crossplane version: v1.14.0

[phisco]

indeed, looks like the issue comes from the refactoring we did in https://github.com/crossplane/crossplane/pull/4500/files#diff-574bed872a28203735722ddf7a98e52ad596702e555ed0dda6525e4e578481f2R213-R227 .

[phisco]

Create a composition with environment patches at index 0 and an XR patch that write to the same field at index 1. In v1.13 the field will contain the XR value. In v1.14 the field will contain the value of the environment.

just to be sure, do you mean patches at spec.environment.patches or spec.resources[*].patches?

[MisterMX]

Create a composition with environment patches at index 0 and an XR patch that write to the same field at index 1. In v1.13 the field will contain the XR value. In v1.14 the field will contain the value of the environment.

just to be sure, do you mean patches at spec.environment.patches or spec.resources[*].patches?

This is about patches in spec.resources[*].patches.

[djmcgreal-cc]

Is this what's causing me difficulties getting dynamic fields shared between resources via EnvironmentConfigs? My immediate use case is that I want to get the id of an EC2 LaunchTemplate into the EKS NodeGroup, and the new provider-aws-eks doesn't support references for the launch template, seemingly.

I extended one of the environment test cases:

kubectl apply -f https://github.com/crossplane/crossplane/raw/master/test/e2e/manifests/apiextensions/environment/setup/provider.yaml -f https://github.com/crossplane/crossplane/raw/master/test/e2e/manifests/apiextensions/environment/setup/definition.yaml

---
apiVersion: apiextensions.crossplane.io/v1alpha1
kind: EnvironmentConfig
metadata:
  name: example-environment-1
data:
  complex:
    a: b
    c:
      d: e
      f: "1"
---
apiVersion: apiextensions.crossplane.io/v1alpha1
kind: EnvironmentConfig
metadata:
  name: example-environment-2
  labels:
    stage: prod
data:
  complex:
    c:
      f: "2"
---
apiVersion: apiextensions.crossplane.io/v1
kind: Composition
metadata:
  name: nop.sqlinstances.example.org
spec:
  compositeTypeRef:
    apiVersion: example.org/v1alpha1
    kind: XSQLInstance
  environment:
    environmentConfigs:
      - ref:
          name: example-environment-1
      - type: Selector
        selector:
          matchLabels:
            - type: FromCompositeFieldPath
              key: stage
              valueFromFieldPath: metadata.labels[stage]
  resources:
    - name: nop
      base: &nop
        apiVersion: nop.crossplane.io/v1alpha1
        kind: NopResource
        metadata:
          labels:
            x: hello
        spec:
          forProvider:
            conditionAfter:
              - conditionType: Xyz
                conditionStatus: "False"
                time: 10s
      patches:
        - type: FromEnvironmentFieldPath
          fromFieldPath: complex.c.f
          toFieldPath: metadata.annotations[valueFromEnv]
        - type: ToEnvironmentFieldPath
          fromFieldPath: metadata.name
          toFieldPath: x
        - type: ToEnvironmentFieldPath
          fromFieldPath: metadata.labels.x
          toFieldPath: static
        - type: ToEnvironmentFieldPath
          fromFieldPath: status.conditions[1].type
          toFieldPath: dynamic
        - type: ToEnvironmentFieldPath
          fromFieldPath: spec.forProvider.fields.xyz
          toFieldPath: field

    - name: from-environment-with-static-field
      base: *nop
      patches:
        - type: FromEnvironmentFieldPath
          fromFieldPath: static
          toFieldPath: metadata.labels.static
        - type: FromEnvironmentFieldPath
          fromFieldPath: x
          toFieldPath: metadata.annotations['nop-name']

    - name: from-environment-with-dynamic-field
      base: *nop
      patches:
        - type: FromEnvironmentFieldPath
          fromFieldPath: dynamic
          toFieldPath: metadata.labels.dynamic
        - type: FromEnvironmentFieldPath
          fromFieldPath: field
          toFieldPath: metadata.labels.field

    - name: from-environment-with-dynamic-required
      base: *nop
      patches:
        - type: FromEnvironmentFieldPath
          fromFieldPath: dynamic
          toFieldPath: metadata.labels.dynamic
          policy:
            fromFieldPath: Required

    - name: from-environment-with-dynamic-required-field
      base: *nop
      patches:
        - type: FromEnvironmentFieldPath
          fromFieldPath: field
          toFieldPath: metadata.labels.field
          policy:
            fromFieldPath: Required
---
apiVersion: example.org/v1alpha1
kind: SQLInstance
metadata:
  name: example
  namespace: default
  labels:
    stage: prod
spec:
  parameters:
    storageGB: 10
  # This is necessary to ensure the claim's MRs are actually gone before we
  # delete the Provider - https://github.com/crossplane/crossplane/issues/4251
  compositeDeletePolicy: Foreground

And found it would cope with static fields in the environment or in a base, but struggled with dynamic ones (that I added as a condition or manually edited in a field after NopResource construction.

[djmcgreal-cc]

I guess not as didn't work in 1.13.2 either.