Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
haarchri opened this issue Aug 7, 2024 · 2 comments
Labels
bug Something isn't working

Comments

@haarchri
Copy link

haarchri commented Aug 7, 2024

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:

@haarchri haarchri added the bug Something isn't working label Aug 7, 2024
@haarchri haarchri changed the title Filtering Empty Objects in SDK to Avoid Invalid Resource Generation Filtering Empty Objects in SDK to Avoid Invalid Resource Generation/Apply Aug 7, 2024
@negz
Copy link
Member

negz commented Aug 8, 2024

(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
Copy link
Member

negz commented Aug 8, 2024

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants