crossplane-contrib / function-kcl

Crossplane Composition Functions using KCL Programming Language
Apache License 2.0
26 stars 10 forks source link

[Enhancement] setting composition resource status #60

Closed markphillips100 closed 3 months ago

markphillips100 commented 3 months ago

What happened?

I want to provide values to custom composition resource status fields, similar to how patch and transform do it using ToCompositeFieldPath. I am defining extra properties on my CompositeResourceDefinition's status property and attempting to set the property value using the below code:

_dxr = option("params").dxr
_dxr.status.myExtraProp = "sample"

However, the function fails to compile complaining of an UndefinedType:

    Message:               cannot compose resources: pipeline step "normal" returned a fatal result: failed to run kcl function pipelines: failed to compile the kcl package
EvaluationError
failed to update the dict. An iterable of key-value pairs was expected, but got UndefinedType. Check if the syntax for updating the dictionary with the attribute 'myExtraProp' is correct

How can we reproduce it?

Define a custom status property called myprop in xrd and attempt to set its value on dxr.status.myprop.

What environment did it happen in?

Function version: xpkg.upbound.io/crossplane-contrib/function-kcl:v0.3.4

Peefy commented 3 months ago

Maybe the status is Undefined. I think you can do this:`

_dxr = option("params").dxr
_dxr = _dxr | {status.myExtraProp = "sample"}
markphillips100 commented 3 months ago

It doesn't error at least :-) but no status properties other than the usual "conditions" property are shown.

Peefy commented 3 months ago

Do you mean to set the status to take effect in the cluster? Currently, option("params") is readonly.

markphillips100 commented 3 months ago

On the composite, same as function-patch-and-transform does here

markphillips100 commented 3 months ago

Just for clarity, a crossplane native way of what I mean is described here. The "secondResource" in that example is what I am trying to set from the function.

Peefy commented 3 months ago

Make sense. Thank you! Would you consider combining patch function with kcl function, or would I integrate the functionality of patch function into kcl function and use kcl to describe it uniformly

markphillips100 commented 3 months ago

I'd much prefer if it were possible with kcl function entirely. I have no intention of using patch and transform moving ahead as I'd prefer to encapsulate all resource code into one kcl function package for a given composition. Using more than one function containing resource-aware logic seems like it will introduce friction for maintenance imo.

markphillips100 commented 3 months ago

KCL function can already do an equivalent of P&T's MR field path mapping, and composite field to MR field. This pseudo field-to-composite status mapping completes the set.

Peefy commented 3 months ago

Thank you, in response to your recent needs and issues, I will design and implement it recently.

markphillips100 commented 3 months ago

FYI, I believe the status property is the only property that should be allowed to be updated. Metadata and spec are not allowed I'm pretty sure. I have a feeling crossplane would either error or ignore such an update but just thought I'd mention.

Peefy commented 3 months ago

Thank you! I will pay attention to this point when designing and consider the use of crossplane and other functions of crossplane at the same time.

Peefy commented 3 months ago

Hello @markphillips100 I've tried this composition resource and set XR status. Is this what you want?

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: normal1
    functionRef:
      name: kcl-function
    input:
      apiVersion: krm.kcl.dev/v1alpha1
      kind: KCLRun
      metadata:
        name: basic1
      spec:
        target: XR # Note set 'XR' here
        source: |
          {
              status.atProvider.bala = "cc"
          }
markphillips100 commented 3 months ago

I assume target: XR is necessary in your example, whereas I would be using target: Resources? I need to be able to set the value from within the same function package else I would be leaking knowledge of a specific resource to a separate function just to set a property on the composite.

To clarify, I don't want the composition yaml to know anything except which resources package to use hence need the required functionality available to the Resources target if possible.

Peefy commented 3 months ago

Thank you! Make sense.

Peefy commented 3 months ago

I've opened a PR to show set the xr status and pass connection details using the meta resource. https://github.com/crossplane-contrib/function-kcl/pull/61

markphillips100 commented 3 months ago

Wonderful. Do you wish me to try the PR or wait for next release? Assumes I can access a PR build of course :-)

Peefy commented 3 months ago

When the PR is merged, you can try the next release v0.4.0. Of course, if possible, could you also help me review the example and code to prevent any misunderstandings in my understanding of your description. ❤️

markphillips100 commented 3 months ago

Yes I will certainly review. Might have to be tomorrow. Getting late. :-)

zargor commented 3 months ago

I have tried your example locally and it is working. However, in my case, I am struggling to achieve something that works perfectly when using standard composition with patch sets and resources.

patches:
    - type: ToCompositeFieldPath
      fromFieldPath: "status.atProvider.arn"
      toFieldPath: "spec.status.arn"
    - type: ToCompositeFieldPath
      fromFieldPath: "status.atProvider.id"
      toFieldPath: "spec.status.id"

From my perspective this should be still simple to achieve with KCL function(s), but at least in my case, it is not.

I would love to see an example covering only such case, where XR status field is updated by some atProviderdata once MR is provisioned. Like in example given, from status.atProvider to composite field.

markphillips100 commented 3 months ago

@zargor composite status properties can now be set using the Default target. See here for an example. And you can read any resource's atProvider property using ocds["<resource-name>"].Resource.status.atProvider.myProp.

I notice in your patches example you are writing to a spec.status property. I'm pretty sure status on a composite is a sibling to spec rather than a child property so just use status.id. I don't believe spec can be written to.

zargor commented 3 months ago

@markphillips100 I can confirm it works when testing locally, using beta render. However, in k8s cluster this does not work. What happens is that MR is never created and Status block of XR is not presented at all. So this kind of tweak will spoil provisioning of the resource:

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

Would like to hear how to set it up to work in a real k8s cluster, not only using local test tools.

markphillips100 commented 3 months ago

I hate to say it, but it works for me in a k8 cluster. Are you definitely using the Default target? It's the only target that implements the feature (I think).

Maybe @Peefy can comment more?

Peefy commented 3 months ago

Thank you! @markphillips100

Hello @zargor , could you give me with more configuration information for KCLRun resources? So that I can replicate it in the cluster. The implementation of this function is referenced from the Crossplane Go template function, and I confirm that there should be no problem. Or you can give a try on XR target like the example? https://github.com/crossplane-contrib/function-kcl/blob/main/examples/xr/patching/composition.yaml Please ensure that the target field of KCLRun is set to default or XR.

zargor commented 3 months ago

First I wanna thank you both for your efforts. Next is I had to be sure whether this works in some realistic enviroment since I did not have any issues with local testing but in cluster.

This is basically what I try to do with https://marketplace.upbound.io/providers/upbound/provider-aws-dynamodb/v1.3.1/resources/dynamodb.aws.upbound.io/Table/v1beta1 :

    # inserts from pipeline
    - step: xrd-parameters-mapping
      functionRef:
        name: kcl-function
      input:
        apiVersion: krm.kcl.dev/v1alpha1
        kind: KCLRun
        metadata:
          name: basic
        spec:
          target: Resources
          source: |
            oxr = option("params").oxr

            name = oxr.metadata.name
            labels = oxr.metadata.labels or {}
            annotations = oxr.metadata.annotations or {}

            # ...

            table = {
                apiVersion = "dynamodb.aws.upbound.io/v1beta1"
                kind = "Table"
                metadata.name = name
                metadata.labels = labels
                metadata.annotations = annotations
                spec.forProvider = {
                    attribute = attribute
                    billingMode = billingMode
                    region = region
                    streamEnabled = streamEnabled
               # ...
            }

            items = [table]

    - step: readiness
      functionRef:
        name: function-auto-ready

    - step: resource-status
       functionRef:
         name: kcl-function
       input:
         apiVersion: krm.kcl.dev/v1alpha1
         kind: KCLRun
         metadata:
           name: basic
         spec:
           target: XR
           source: |
             {
                 status.is_ok = True
             }        

Would be awesome to learn also about target set eligible in terms of KCLRun.

zargor commented 3 months ago

I think I got it finally. It is not only about one issue.

This example is fine but in my case I did not realise the target field to patch has to be defined in XRD in the first place.

Otherwise the pipeline just does nothing, not expressing any reason/event/status why is not successful.

Secondly, if one wants to patch XR with ocds["<resource-name>"].Resource.status.atProvider.myProp this will not work, at least as long as there is no such resource provisioned/available at the time. Meaning that the resource(s) must be created, so to be able to reach out to atProvider set.

Now the question is, how we can manage to do it in patch XR step, like to have the pipeline smart enough to wait for provisioning to complete. Is this anyhow addressed anywhere? Or differently, do we need a completely separate composition just to be able to set some status properties back to XR?

The most interesting thing is that XR status patching works like a charm when using function-patch-and-transform within a pipeline.

markphillips100 commented 3 months ago

I don't use the XR target for patching the composite status. I do mine in the Default target function along with the creation of the resources. The function will execute many times during each reconciliation loop so eventually the ocds will contain resources with their atProvider data set and any assignment to composite status will be resolved accordingly. This is how normal patch and transform function works too

zargor commented 3 months ago

@markphillips100 that is very good information. Would be great to share some brief snippet around that.

markphillips100 commented 3 months ago

@zargor I do all my kcl function compositions using oci packages so don't have an inline example off hand but I'll add the relevant bits here.

You are correct in that any property you want to surface on the XR needs to be defined on the XRD. In this example we are surfacing myProp both as a status property on the XR and as a printed column. In addition, we've added the property to the composite's secret.

apiVersion: apiextensions.crossplane.io/v1
kind: CompositeResourceDefinition
metadata:
  name: mycomposites.example.com
spec:
  group: example.com
  names:
    kind: MyComposite
    plural: mycomposites
  claimNames:
    kind: MyClaim
    plural: myclaims
  connectionSecretKeys:
# Add list of props to be surfaced by composite secret - NOTE: this is separate to surfacing properties on the XR.
    - myProp
  versions:
  - name: v1alpha1
    served: true
    referenceable: true
    schema:
      openAPIV3Schema:
        type: object
        properties:
          spec:
            type: object
            properties:
              id:
                type: string
                description: ID of this composite that other objects can refer.
              parameters:
                type: object
                description: Parameters used to customize the cluster
                properties:
                  inputA:
                    description: A description.
                    type: string
            required:
              - id
              - parameters
# This is required in order to surface properties on the XR.
          status:
            type: object
            properties:
              myProp:
                description: The myProp of my resource.
                type: string
# Surface some of XR's properties so user sees them when executing: kubectl get mycomposite
    additionalPrinterColumns:
    - name: myProp
      type: string
      jsonPath: ".status.myProp"

The composition should only need a single kcl function with target "Default" to create resources, set the composite secret data, and also the XR status property. You will need to follow this up with function-auto-ready function in order to have the XR ready status be resolved.

I haven't executed this code so just take it as guidance please:

apiVersion: apiextensions.crossplane.io/v1
kind: Composition
metadata:
  name: my-composite
spec:
  compositeTypeRef:
    apiVersion: example.com/v1alpha1
    kind: MyComposite
  writeConnectionSecretsToNamespace: crossplane-system
  mode: Pipeline
  pipeline:
  - step: normal
    functionRef:
      name: kcl-function
    input:
      apiVersion: krm.kcl.dev/v1alpha1
      kind: KCLRun
      metadata:
        name: basic
      spec:
# Note: this can be omitted and Default will be used.
# Do not use "Resources" as this does not support the features we need.
        target: Default
        source: |
            import base64

            oxr = option("params").oxr

            name = oxr.metadata.name
            labels = oxr.metadata.labels or {}
            annotations = oxr.metadata.annotations or {}

            # ...

            table = {
                apiVersion = "dynamodb.aws.upbound.io/v1beta1"
                kind = "Table"
                metadata.name = name
                metadata.labels = labels
                metadata.annotations = annotations
                spec.forProvider = {
                    attribute = attribute
                    billingMode = billingMode
                    region = region
                    streamEnabled = streamEnabled
               # ...
            }

            # We must use a null referencing syntax as the resource will not exist in ocds on first pass, 
            # and highly unlikely any atProvider data is provided for a while too.
            # We presume this code will execute many times and 
            # eventually atProvider will contain the data we want to surface.

            # This code assumes "name" is the name given to the metadata.name property of the table resource, and
            # there will eventually be a property called "myProp" on the table resource's atProvider data.

            # Get myProp data.
            tableResource = option("params").ocds[name]?.Resource 
            myProp = tableResource?.status?.atProvider?.myProp or ""

            # Apply myProp data to the desired XR.
            # NOTE: It's important to include the **oxr else kcl will not know this object is to update the XR.
            dxr = {
                **oxr
                 # We are setting the value always to either its final value or "".
                 # We could use a condition as used below in the details.
                status.myProp = myProp
            }

            # As a bonus, let's add myProp to the composite's secret.
            details = {
                apiVersion: "meta.krm.kcl.dev/v1alpha1"
                kind: "CompositeConnectionDetails"
                if myProp != "":
                    data: {
                        myProp = base64.encode(myProp)
                    }
                else:
                    data: {}
            }

            # Finally we must return not only our resources to be created but also the details and dxr.
            items = [table, details, dxr]

  - step: automatically-detect-ready-composed-resources
    functionRef:
      name: function-auto-ready        

Hope that helps.

Peefy commented 3 months ago

Closed by #61