Required patch policy aborts rendering of entire composite

[benagricola]

Regarding discussion in Slack around the Required patch policy in combination with bidirectional patching.

This appears to be intentional behaviour instead of an actual bug (and I suspect this is how Compositions have always behaved) but seems somewhat counter-intuitive to me when used with bi-directional patching and the Required policy.

What happened?

I was attempting to use a bi-directional patch within a single composition so I could use the ARN of one base resource in the body of another base resource (Bucket ARN in an IAMPolicy document).

My belief was that I would be able to use the following XRD / Composition:

---
apiVersion: apiextensions.crossplane.io/v1
kind: CompositeResourceDefinition
metadata:
  annotations:
  name: compositeobjectstoragebuckets.resources.company.systems
spec:
  group: resources.company.systems
  names:
    kind: CompositeObjectStorageBucket
    plural: compositeobjectstoragebuckets
  claimNames:
    kind: ObjectStorageBucket
    plural: objectstoragebuckets
  defaultCompositionRef:
    name: object-storage-bucket-aws
  versions:
    - name: v1alpha1
      referenceable: true
      served: true
      schema:
        openAPIV3Schema:
          type: object
          properties:
            status:
              type: object
              properties:
                uid:
                  description: >
                    UID of the underlying bucket. On AWS for example,
                    this would contain the ARN of the generated bucket.
                  type: string

---
apiVersion: apiextensions.crossplane.io/v1
kind: Composition
metadata:
  name: object-storage-bucket-aws
spec:
  compositeTypeRef:
    apiVersion: resources.company.systems/v1alpha1
    kind: CompositeObjectStorageBucket

  patchSets:
    - name: BucketSyncStatus
      patches:
        # Sync the bucket ARN back to the UID field on the composite
        - type: ToCompositeFieldPath
          fromFieldPath: status.atProvider.arn
          toFieldPath: status.uid
          # Optional because status.atProvider won't exist until the bucket
          # is reconciled.
          policy:
            fromFieldPath: Optional

    - name: S3ObjectRwPolicy
      patches:
        # Use the synced bucket UID to generate a valid policyDocument
        - fromFieldPath: status.uid
          toFieldPath: spec.forProvider.document
          # Required because without a source ARN, the policy document is invalid
          policy:
            fromFieldPath: Required
          transforms:
            - type: string
              string:
                fmt: |
                  {
                    "Version": "2012-10-17",
                    "Statement": [
                        {
                            "Action": [
                                "s3:GetObject*",
                                "s3:DeleteObject*",
                                "s3:PutObject*"
                            ],
                            "Resource": "%s/*",
                            "Effect": "Allow"
                        }
                    ]
                  }

  resources:
    - name: Bucket
      base:
        apiVersion: s3.aws.crossplane.io/v1beta1
        kind: Bucket
        spec:
          deletionPolicy: Delete
          forProvider:
            acl: private
            locationConstraint: us-east-1
      patches:
        - type: PatchSet
          patchSetName: BucketSyncStatus

    - name: S3RwPolicy
      base:
        apiVersion: identity.aws.crossplane.io/v1alpha1
        kind: IAMPolicy
        spec:
          deletionPolicy: Delete
      patches:
        - type: PatchSet
          patchSetName: S3ObjectRwPolicy

I thought what would happen here is that the patch on the Bucket would be a no-op because the fromField doesn't exist, the Bucket resource would be created, and then the patch on the Policy would be rejected (and only the Policy resource would not be created) as the fromField does not exist yet and is set Required.

What actually happens is the error from the Required patch on the Policy (cannot render composed resource from resource template at index 1: cannot apply the patch at index 0: status: no such field) causes the entire composition to be aborted and requeued, with no resources applied to the cluster - the Bucket is never created, so can never populate its' status field for the Policy patch to work on.

Removing the Required policy from the patch allows both resources to be created but until the Bucket is ready and its' value is patched back, the Policy resource is invalid (in this case, contains an empty format string placeholder from the string transform).

If this invalid resource were rejected by the provider (e.g. there was a policy check before creation on the provider-aws side that threw an error) then I don't think it would be possible to bypass and the resources would never be created.

How can we reproduce it?

Create a Composition and XRD based on the above code, and create an instance of it. Watch as the Composition never completes first reconciliation, instead aborting due to the patching failure of the IAMPolicy.

What environment did it happen in?

Crossplane version: v1.2.0-rc.0.5.ge4491ffd

cc @negz @muvaf @mcavoyk

[muvaf]

I think Crossplane should deploy the ones it can all the time, providing an eventual consistency without imposing an order requirement in the resources array. I was thinking whether this should be a release blocker but it seems this might require a risky change, so I'd say let's fix it in v1.2

[hasheddan]

This thread points to some of the things we are seeing here I think, but it was focused on patching back to the composite resource and that motivated the fix. I imagine we could do a similar fix patching to managed resources but I need to look a little closer.

[hasheddan]

Looks like if we fail to render any resources then we don't apply any of them:

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

Line 418 in ecfb6ce

// We want to ensure we can render all of our composed resources before we

	// We want to ensure we can render all of our composed resources before we
	// apply any of them. We prefer to avoid creating or updating any composed
	// resources if we know we won't be able to create or update all of them.
	refs := make([]corev1.ObjectReference, len(tas))
	cds := make([]resource.Composed, len(tas))
	for i, ta := range tas {
		cd := composed.New(composed.FromReference(ta.Reference))
		if err := r.composed.Render(ctx, cr, cd, ta.Template); err != nil {
			log.Debug(errRenderCD, "error", err, "index", i)
			r.record.Event(cr, event.Warning(reasonCompose, errors.Wrapf(err, errFmtRender, i)))
			return reconcile.Result{RequeueAfter: shortWait}, nil
		}

		cds[i] = cd
		refs[i] = *meta.ReferenceTo(cd, cd.GetObjectKind().GroupVersionKind())
	}

Guessing we need to skip on something like:

crossplane/apis/apiextensions/v1/composition_types.go

Line 355 in ecfb6ce

func IsOptionalFieldPathNotFound(err error, s *PatchPolicy) bool {

[negz]

It seems like there's a couple of cases in which folks would want to use required patches:

1. To require an XR 'field' that can't be marked required via its OpenAPI schema - e.g. to require that a particular label or annotation exist before resources are composed.
2. To require an XR field that is late initialized by something outside the Composition - e.g. to require that an XR's spec.claimRef.namespace is set by the claim binding machinery before resources are composed.
3. To patch from a late initialized (or status) composite resource field back to the XR.
4. To patch from a late initialized (or status) XR field to a composite resource.

If I follow correctly cases 1, 2, and 3 are working but 4 is not. I agree we should fix this to enable case 4, but I don't think we need to consider this a release blocker. We could address this in 1.2 or even in a 1.1.1 patch release.

[benagricola]

Yep, I think that's accurate, but depending on how case 4 is fixed it could also enable a 5th case by circumstance - where an optional field on the XR that is set fromFieldPath: Required might conditionally enable or disable the rendering of a single base resource.