crossplane / function-sdk-go

The Go SDK for composition functions
https://crossplane.io
Apache License 2.0
19 stars 13 forks source link

Filtering Empty Objects in SDK to Avoid Invalid Resource Generation/Apply #154

Open haarchri opened 1 month ago

haarchri commented 1 month ago

What happened?

When using for example function-kcl to create resources, the following pattern can lead to the generation of invalid resources that result in client-side crossplane beta render or server-side errors during the application:

 _items = [{
    apiVersion: "s3.aws.upbound.io/v1beta1"
    kind: "Bucket"
    metadata: _metadata("bucket")
    spec: _defaults
}]

_items += [{
    apiVersion: "s3.aws.upbound.io/v1beta1"
    kind: "BucketVersioning"
    metadata: _metadata("bucketVersioning")
    spec: _defaults | {
        forProvider: {
            bucketSelector: {
                matchControllerRef: True
            },
            versioningConfiguration: [
                {
                    status: "Enabled"
                },
            ],
        },
    }
} if get(oxr, "spec.parameters.versioning", "False") else {}]

items = _items 

This approach can produce an invalid resource object like the one shown below if the conditional logic produces an empty object ({}):

---
metadata:
  annotations:
    crossplane.io/composition-resource-name: ""
  generateName: configuration-
  labels:
    crossplane.io/composite: configuration
  ownerReferences:
  - apiVersion: example.org/v1alpha1
    blockOwnerDeletion: true
    controller: true
    kind: Template
    name: configuration
    uid: "" 

The issue arises because the condition for else produces {}, which is added to the _items list. This can lead to errors during the resource application process.

To mitigate this, every individual composition currently requires an additional filtering step:

items = [
    i for r in [_items]
    for i in r if i
]

Can the SDK be enhanced to automatically filter out these invalid empty object resources? This would simplify the composition logic and prevent potential errors during resource application.

This issue has been discussed in the KCL community. You can find the discussion here: https://cloud-native.slack.com/archives/C05TC96NWN8/p1723020486395609

How can we reproduce it?

What environment did it happen in?

Crossplane version:

negz commented 1 month ago

(I chatted with @haarchri on Slack, but capturing my thoughts here for posterity.)

My understanding of the problem is that KCL doesn't support "append X to this array if the condition is met, else append nothing". Instead it only supports "append X to this array if the condition is met, else append Y". So in the context of a composition function this means "append this resource to desired resources if the condition is met, otherwise append the empty object {}".

The SDK then sends the empty object to Crossplane, which fills in some required fields (generate name, owner refs, etc) resulting in the invalid object that can't be applied.

Fixing this at the function SDK level feels like fixing a KCL limitation at the function SDK level. As a KCL outsider it seems ideal to me to fix it at the language level. In the meantime it's possible to address this in KCL configuration using the for i in r if i workaround. I actually think it's better to do that explicitly in the KCL code than have it happen automatically at the function SDK level. Generally explicit is better than implicit/magical. Yes you need to write a little boilerplate, but that makes it really obvious what's happening to anyone who reads the config file. They can immediately see that empty resources are being filtered out.

The other concern with making a change like this at the SDK level is that there's more than on function SDK. This one's the most popular, but there's also https://github.com/crossplane/function-sdk-python/, https://github.com/crossplane/function-sdk-java and in future others. We'd need to encode this behavior into all the SDKs for consistency, or it'd be even more surprising to users.

negz commented 1 month ago

@haarchri This seems to work fine:

apiVersion: apiextensions.crossplane.io/v1
kind: Composition
metadata:
  name: function-template-go
spec:
  compositeTypeRef:
    apiVersion: example.crossplane.io/v1
    kind: XR
  mode: Pipeline
  pipeline:
  - step: normal
    functionRef:
      name: kcl-function
    input:
      apiVersion: krm.kcl.dev/v1alpha1
      kind: KCLRun
      metadata:
        name: basic
      spec:
        source: |
          import base64

          oxr = option("params").oxr
          count = oxr.spec.count or 1
          ocds = option("params").ocds

          dxr = {
              **oxr
              status.dummy = "cool-status"
          }

          _items = [{
              apiVersion: "s3.aws.upbound.io/v1beta1"
              kind: "Bucket"
              metadata: {
                annotations: {
                  "krm.kcl.dev/composition-resource-name": "a"
                }
              }
          }]

          if oxr.spec.parameters.versioning == "True":
            _items += [{
                apiVersion: "s3.aws.upbound.io/v1beta1"
                kind: "BucketVersioning"
                metadata: {
                  annotations: {
                    "krm.kcl.dev/composition-resource-name": "b"
                  }
                }
            }]

          items = _items

The BucketVersioning is appended to desired resources if spec.parameters.versioning == "True". Otherwise nothing is appended.