kubeflow / kfp-tekton

Kubeflow Pipelines on Tekton
https://developer.ibm.com/blogs/kubeflow-pipelines-with-tekton-and-watson/
Apache License 2.0
167 stars 118 forks source link

Support user functions to be used in CEL context #697

Open pugangxa opened 2 years ago

pugangxa commented 2 years ago

/kind feature

Description: User can use CEL expressions in condition now, see https://github.com/kubeflow/kfp-tekton/blob/master/guides/advanced_user_guide.md Now only the CEL builtin functions can be used. It's not possible for user to implement cel functions and used in CEL context. For example want to transform the input parameter and then output and implemented a simple function to be CEL ext function and want to use it in the expression.

Additional information: Investigated and can let the controller load these user functions as plugin. See PoC here: https://github.com/pugangxa/cel-plugin-poc

pugangxa commented 2 years ago

Should log an issue and implement in https://github.com/tektoncd/experimental/tree/main/cel but we can discuss here firstly.

Tomcli commented 2 years ago

Thanks, do you have a plan for how to add this plugin to the CEL controller?

cc @pritidesai @afrittoli Do we need to create a TEP or present at the API group if we are trying to change an experimental feature? I remember @vincent-pli was trying to do some enhancement on CEL, but that get dropped because it was not brought up in the community call.

pritidesai commented 2 years ago

I would recommend to demonstrate it in the API WG to get attention from the folks, but that does not block from implementing it.

Tomcli commented 2 years ago

@pugangxa Do you have an example to make it work as part of the CEL controller? If you have it, you or our team can start take it forward to present in the Tekton API WG and get some feedbacks from the community.

pugangxa commented 2 years ago

@pugangxa Do you have an example to make it work as part of the CEL controller? If you have it, you or our team can start take it forward to present in the Tekton API WG and get some feedbacks from the community.

Thanks Tommy, will work out an example in 2~3 days and then move forward. Will update you then, thanks.

pugangxa commented 2 years ago

Sorry I have other stuff and will postpone this a little bit.

pugangxa commented 2 years ago

@Tomcli The example is available https://github.com/pugangxa/experimental/tree/cel-plugin-poc, please take a look. Also drafted a document about it. CEL User Functions.docx

Tomcli commented 2 years ago

thanks @pugangxa, I will create a google docs based on your documents to present to the community and we also need to discuss how to workaround on the security part later on. But this is a good start to show the community what we want to have.

Tomcli commented 2 years ago

@pugangxa I created this google doc for your proposal and updated some wordings and orders. Let me know if it looks good to you because I will use this doc to open the community issue and present in the API WG. https://docs.google.com/document/d/1xJGl_GCOi1QtIShHUBiA4V9FoGw5roU9DfaEkpJ9dyw/edit?usp=sharing

pugangxa commented 2 years ago

Thanks Tommy, it looks fine to me. I created an issue https://github.com/tektoncd/experimental/issues/789 and agree we can discuss how to workaround the security part later on. Can send the PR after discussion and your present in the API WG.

pugangxa commented 2 years ago

@Tomcli Seems you are on vacation but any update on this? When will it be presented in the API WG?

Tomcli commented 2 years ago

This week's Tekton API WG was cancelled because we have the US labor day on Monday. I will be presenting it in the next API WG on Sep 13.

Tomcli commented 2 years ago

@pugangxa I presented the POC to the Tekton API WG today and here are the feedbacks

  1. We need a more specific use case for why we want to use the go plugins for adding user functions. Some counter arguments are:
    • How does the user aware of the dependencies it should use in the go plugins? (I was arguing we can have a service that help generate the code for them)
    • Why we are limiting user functions in Golang with specific dependencies? (We need to provide a more specific example on DataStage where it cannot achieve with CEL today, but can be done with simple Golang to justify this feature)
  2. Can we use the static links for this case? Tekton trigger was adding its own static link class to extend their use case. (I argue that we don't know what kind of functions users might provide and rolling out new images for function updates is very expensive. However, we can do it in a hybrid mode where the common functions can be statically embedded and have the plugins as the additional extension.)

We can discuss tomorrow during our call to clarify some of these questions as well. The next step for us is to create a TEP in Tekton to answer those questions, so we can add our code to the CEL controller.

pugangxa commented 2 years ago

Thanks Tommy. 1: I understand these counter arguments and in fact I also thought about them, but just seems there's no other better solution for this.

- Yes I think have a service to generate the code is a good idea. Though still need keep the added dependencies the same. I think as in the PoC it does not impact the normal process. So the current behavior will be kept, just up to user if they want to add new functions dynamically, then follow the requirements. - Yes datastage just want to extract knowledge from the CEL expression, but they allow user to define their own functions to do this. I am not sure whether this is their document but I can discuss with them too. https://www.ibm.com/docs/en/iis/11.7?topic=jobs-custom-resources

2: In fact we added some static functions to the controller already. But for user's functions if we still follow this way then each time user need build the controller again after new functions added and roll it out. It's not allowed in managed environment or companies with lots of departments.

Yeah we can talk tomorrow and brainstorming whether there's better solution for this kind of requirements.

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.