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

Support user functions to be used in CEL context #697

Open
pugangxa opened this issue Aug 13, 2021 · 15 comments
Open

Support user functions to be used in CEL context #697

pugangxa opened this issue Aug 13, 2021 · 15 comments

Comments

@pugangxa
Copy link
Contributor

/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
Copy link
Contributor Author

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

@Tomcli
Copy link
Member

Tomcli commented Aug 13, 2021

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

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

Tomcli commented Aug 17, 2021

@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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

@pugangxa
Copy link
Contributor Author

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

Tomcli commented Aug 31, 2021

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

Tomcli commented Aug 31, 2021

@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
Copy link
Contributor Author

pugangxa commented Sep 1, 2021

Thanks Tommy, it looks fine to me. I created an issue tektoncd/experimental#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
Copy link
Contributor Author

pugangxa commented Sep 9, 2021

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

@Tomcli
Copy link
Member

Tomcli commented Sep 9, 2021

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

Tomcli commented Sep 14, 2021

@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
Copy link
Contributor Author

pugangxa commented Sep 14, 2021

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

stale bot commented Mar 2, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants