Conditional function execution using the Common Expression Language

[stevendborrelli]

Note: there is now a forked version of function-patch-and-transform supporting Conditionals at https://github.com/stevendborrelli/function-conditional-patch-and-transform.

---

This is a test of evaluating expressions with Common Expression Language (CEL). This is inspired by work from @ytsarev and @avalanche123 in developing a function that only executes after matching an expr conditional at https://github.com/ytsarev/function-if (see closed PR #14 for the expr implementation).

Composition authors express a CEL condition, and if it returns true, patch-and-tranfsforms defined in the input will be processed.

 mode: Pipeline
  pipeline:
  - step: if
    functionRef:
      name: function-patch-and-transform
    input:
      condition:
         expression: observed.composite.resource.spec.env == "prod" && observed.composite.resource.spec.render == true

An amd64 Function is available for testing available at:

apiVersion: pkg.crossplane.io/v1beta1
kind: Function
metadata:
  name: function-patch-and-transform
spec:
  package: index.docker.io/steve/function-patch-and-transform:v0.2.0
  packagePullPolicy: Always

Using the following XR and RunFunctionRequest inputs (click to expand):

apiVersion: nopexample.org/v1alpha1
kind: XNopResource
metadata:
  name: test-resource
spec:
  env: dev
  render: true

{
   "desired": {
      "composite": {
         "resource": {
            "apiVersion": "nopexample.org/v1alpha1",
            "kind": "XNopResource",
            "metadata": {
               "name": "test-resource"
            },
            "spec": {
               "env": "dev",
               "render": true
            }
         }
      },
      "resources": {
         "test": {
            "resource": {
               "apiVersion": "example.org/v1",
               "kind": "CD",
               "metadata": {
                  "name": "cool-42",
                  "namespace": "default"
               }
            }
         }
      }
   },
   "observed": {
      "composite": {
         "resource": {
            "apiVersion": "nopexample.org/v1alpha1",
            "kind": "XNopResource",
            "metadata": {
               "name": "test-resource"
            },
            "spec": {
               "env": "dev",
               "render": true
            },
            "status": {
               "id": "123",
               "ready": false
            }
         }
      }
   }
}

You can use the CEL Playground to test various queries.

Here are some example queries on the XR and RunFunctionRequest:

• desired.composite.resource.spec.env == "dev" evaluates to true
• desired.composite.resource.spec.render == true, evaluates to true
• desired.composite.resource.spec.render == false" evaluates to false
• observed.composite.resource.status.ready == true" evaluates to false
• size(desired.resources) == 0 evaluates to false
• "test" in desired.resourcesevaluates to true
• "bad-resource" in desired.resources evaluates to false

[stevendborrelli]

Some thoughts:

• should I shortcut the path to observed spec.XX, i.e. observed.composite.resource.spec.env => spec.env
• Does it need to be Condition: Expression: a=b, or should we just use Condition: a==b?

[negz]

@stevendborrelli what are you thinking WRT pursuing getting this merged here vs experimenting with a separate CEL function?

[phisco]

To add a bit more on what I said on slack.

I could see this go in 3 directions:

Crossplane native support

CEL feels like the right tool for the job given the wide adoption, so we could think of directly adding an if field you are proposing here to each PipelineStep, with a CEL expression, similar to what you were passing as input here.

mode: Pipeline
  pipeline:
  - step: something
    if: observed.composite.resource.spec.env == "prod" && observed.composite.resource.spec.render == true
    functionRef:
      name: function-patch-and-transform
    input:
      ...

This looks similar to what github actions do and have the benefit of completely avoiding calling the function, but on the other hand we would be adding yet another feature to Compositions, kind of overlapping with the fieldpaths syntax.

Use the Context and a well-known key apiextensions.crossplane.io/conditions, Crossplane aware

We could still add an if field to each PipelineStep, but instead of being a CEL expression, it could just be a key to be used to retrieve a bool value from the Context at a well-known key, e.g. apiextensions.crossplane.io/conditions, and then other functions could set those conditions, e.g. function-cel-conditions:

mode: Pipeline
  pipeline:
  - step: conditions
    functionRef:
      name: function-cel-conditions
    input:
      ...
        prodAndRender: observed.composite.resource.spec.env == "prod" && observed.composite.resource.spec.render == true
  - step: something
    if: prodAndRender # => effectively resulting in the bool value of Context["apiextensions.crossplane.io/conditions"]["prodAndRender"]
    functionRef:
      name: function-patch-and-transform
    input:
      ...

Again this would be similar to what github actions do, but with the benefit that we wouldn't have to start drifting function-patch-and-transform away from the in-tree implementation. Other functions could adopt this standard and implement conditions in other languages, e.g. rego, and it would still avoid the call to the "skipped" function, obviously at the cost of an initial call to the function setting the conditions.

Use the Context and a well-known key apiextensions.crossplane.io/conditions, Crossplane NOT aware

Same as above, but instead of making Crossplane aware of the conditions, functions could internally rely on this standard, without the official endorsement. This would mean function-patch-and-transform and any other function willing to support it would still need to be updated to handle these conditions on its own.

Overall I feel this would be worst option, as I feel conditionally executing steps should be a core functionality.

Get it in function-patch-and-transform [your proposal]

mode: Pipeline
  pipeline:
  - step: if
    functionRef:
      name: function-patch-and-transform
    input:
      condition:
         expression: observed.composite.resource.spec.env == "prod" && observed.composite.resource.spec.render == true

Could be fine, but as I said above it would have the drawback of being a thing only for this specific function, which I feel should not, would start the drift from the in-tree implementation (which will happen for sure at some point, but we could try pushing it back until the dust has settled a bit more), and would mean we are still calling the function from Crossplane.

My opinion

Overall I feel the Crossplane-aware option through the Context is the one I would lean more toward, but it's definitely not a hill I would die on.

[stevendborrelli]

@phisco I'm not sure what the right API is for conditionals, I lean towards having it part of the core function runtime and having CEL be part of function-sdk-go.

@negz Right now I have that fork https://github.com/stevendborrelli/function-conditional-patch-and-transform that a few people are testing, I would love to get their feedback and then have a path to get this functionality merged into the this patch-and-transform function.

[phisco]

@phisco I'm not sure what the right API is for conditionals, I lean towards having it part of the core function runtime and having CEL be part of function-sdk-go.

So, "Crossplane native support":

  mode: Pipeline
  pipeline:
  - step: something
    if: observed.composite.resource.spec.env == "prod" && observed.composite.resource.spec.render == true
    functionRef:
      name: function-patch-and-transform
    input:
      ...

Yes, I'm torn between that and giving functions control over successive functions calls through the context too. Probably native support would make it easier to understand at the cost of having to introduce CEL into the Composition API.

[haarchri]

What obstacles are preventing progress here? Resolving this will greatly benefit folks, and I don't see a need for a fork. Additionally, this is an optional feature, and I believe it will boost the use of composition functions once it's integrated into the crossplane ecosystem.

[ytsarev]

@phisco Given this is already implemented(and tested!) in the P&T function scope, and multiple customers and community members highly desire conditionals, it would be great to accept it to this function codebase and (optionally) in parallel work on Crossplane-native implementation... This functionality is highly desired to the level that we will have to run and maintain separate patch-and-transform function fork to enable people... It is obviously an unwanted possible outcome of this discussion, and we would love to converge on the same codebase.

[negz]

This functionality is highly desired to the level that we will have to run and maintain separate patch-and-transform function fork to enable people... It is obviously an unwanted possible outcome of this discussion, and we would love to converge on the same codebase.

This is working as intended. Let's try it in a fork for a little while and see how the feature works before we commit to bringing it "upstream" into this function. It's easier to drop the fork and have folks move back to this function than it is to change or remove the functionality from this function once it's been added.

[ytsarev]

@negz I generally agree with this approach but am concerned with creating more confusion in the community with multiple instances of the same stuff(we already have community/official providers, etc). "A little while" is not really defined in this case so in practice, it can lead to a long-term fork with the associated maintenance price.

[stevendborrelli]

Closing in favor of PR #84.