Do not block composed resources on failed neighbor

[hasheddan]

Description of your changes

Updates the composite resource reconciler to render all resources it is
able to even if it is not able to render all resources in the specified
composition. This is a change in behavior from refusing to render any
composed resources if not able to render all.

Signed-off-by: hasheddan georgedanielmangum@gmail.com

Fixes #2175

I have:

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

How has this code been tested

Using the manifests in #2175:

1. Create an ObjectStorageBucket

🤖 (crossplane) k get objectstoragebucket
NAME   READY   CONNECTION-SECRET   AGE
test   False                       119s

2. Observed that only the Bucket was created

🤖 (crossplane) k get bucket
NAME               READY   SYNCED   AGE
test-j4jhd-7kvk8   True    True     60m

3. See that both references are create but IAMPolicy does not have a name

- apiVersion: resources.company.systems/v1alpha1
  kind: CompositeObjectStorageBucket
  metadata:
    annotations:
      crossplane.io/external-name: ""
      kubectl.kubernetes.io/last-applied-configuration: |
        {"apiVersion":"resources.company.systems/v1alpha1","kind":"ObjectStorageBucket","metadata":{"annotations":{},"name":"test","namespace":"default"},"spec":{}}
    creationTimestamp: "2021-06-03T22:14:21Z"
    generateName: test-
    generation: 4
    labels:
      crossplane.io/claim-name: test
      crossplane.io/claim-namespace: default
      crossplane.io/composite: test-j4jhd
   ...
   name: test-j4jhd
  spec:
    claimRef:
      apiVersion: resources.company.systems/v1alpha1
      kind: ObjectStorageBucket
      name: test
      namespace: default
    compositionRef:
      name: object-storage-bucket-aws
    resourceRefs:
    - apiVersion: s3.aws.crossplane.io/v1beta1
      kind: Bucket
      name: test-j4jhd-7kvk8
    - apiVersion: identity.aws.crossplane.io/v1alpha1
      kind: IAMPolicy
  status:
    conditions:
    - lastTransitionTime: "2021-06-03T22:16:51Z"
      reason: Creating
      status: "False"
      type: Ready
    uid: arn:aws:s3:::test-j4jhd-7kvk8

4. Now that uid is populated in status, we can create the IAMPolicy

🤖 (crossplane-runtime) k get iampolicy
NAME               ARN                                           READY   SYNCED   AGE
test-j4jhd-5phzf   arn:aws:iam::609897127049:policy/test-j4jhd   True    True     6m23s

5. Name of IAMRole is now present in resource refs on the composite

- apiVersion: resources.company.systems/v1alpha1
  kind: CompositeObjectStorageBucket
  metadata:
    annotations:
      crossplane.io/external-name: ""
      kubectl.kubernetes.io/last-applied-configuration: |
        {"apiVersion":"resources.company.systems/v1alpha1","kind":"ObjectStorageBucket","metadata":{"annotations":{},"name":"test","namespace":"default"},"spec":{}}
    creationTimestamp: "2021-06-03T22:14:21Z"
    generateName: test-
    generation: 5
    labels:
      crossplane.io/claim-name: test
      crossplane.io/claim-namespace: default
      crossplane.io/composite: test-j4jhd
    ...
    name: test-j4jhd
    ...
  spec:
    claimRef:
      apiVersion: resources.company.systems/v1alpha1
      kind: ObjectStorageBucket
      name: test
      namespace: default
    compositionRef:
      name: object-storage-bucket-aws
    resourceRefs:
    - apiVersion: s3.aws.crossplane.io/v1beta1
      kind: Bucket
      name: test-j4jhd-7kvk8
    - apiVersion: identity.aws.crossplane.io/v1alpha1
      kind: IAMPolicy
      name: test-j4jhd-5phzf
  status:
    conditions:
    - lastTransitionTime: "2021-06-03T23:16:37Z"
      reason: Available
      status: "True"
      type: Ready
    uid: arn:aws:s3:::test-j4jhd-7kvk8

6. Deleting the claim cleans up the composite and all managed resources.

[hasheddan]

check-diff is know bug that is fixed in #2166

[hasheddan]

I removed the BucketPolicy from the composition and that resulted in the cannot access object metadata error going away, but the UID field was not being patched back to the XR or XRC correctly 🤔

[muvaf]

Are we feeling OK with including this in v1.2.0?

[hasheddan]

@muvaf yep! Need to do a bit more testing, but I'll get this fixed up and ready to go!

[muvaf]

The append will mess up with the ordered association code here because it is coupled with the order very tightly.

As we talked offline, we need either an empty entry or placeholder. My preference would be towards empty entry. It seems like we need to remove the requirements here and there is also a workaround here in the code that sets the references (I don't remember why it existed precisely, but I think it might have to do with using ObjectReference schema, which we don't do now even though the struct in Go code is ObjectReference).

I think it's worth testing these changes. I can't really see a reason why it'd not be able to have an array entry with all the values being "". But if it can't, maybe having apiVersion and kind but leaving name empty could be helpful as placeholder.

A different approach could be that we could mark the entry with the name or index of the template that's used, i.e. adding a baseName to ref struct here. But I guess that ship has sailed in #2131 where we decided to use an annotation on the composed resource.

[negz]

A different approach could be that we could mark the entry with the name or index of the template that's used, i.e. adding a baseName to ref struct here. But I guess that ship has sailed in #2131 where we decided to use an annotation on the composed resource.

Just dropping some breadcrumbs to this specific comment - #2131 (comment), which touches on why I didn't take that option.

[hasheddan]

@muvaf @negz I have updated here to only reference composed resources by GVK when we are unable to render them (see updated testing results in PR body) and everything appears to be behaving correctly. I think this is ready for another review, but I am still testing out a few scenarios -- I am specifically concerned with situations where we fail to render a resource after we have already rendered it successfully in the past, but I think we should still be fine in that case.

[negz]

One question and a nit, but this broadly LGTM.

[muvaf]

LGTM! Thanks @hasheddan !