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

[TEP-0145] CEL in WhenExpression #1074

Merged
merged 1 commit into from
Oct 13, 2023
Merged

Conversation

Yongxuanzhang
Copy link
Member

This commit adds the proposal of adding CEL in WhenExpression

Signed-off-by: Yongxuan Zhang [email protected]

@tekton-robot tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 28, 2023
@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 28, 2023
@Yongxuanzhang Yongxuanzhang force-pushed the tep-144 branch 2 times, most recently from a68eaab to 6fcec4a Compare September 28, 2023 15:48
@Yongxuanzhang Yongxuanzhang changed the title [TEP-0144] CEL in WhenExpression [TEP-0145] CEL in WhenExpression Sep 28, 2023
@Yongxuanzhang Yongxuanzhang marked this pull request as ready for review September 28, 2023 15:58
@tekton-robot tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 28, 2023
@Yongxuanzhang
Copy link
Member Author

Hi @tektoncd/core-maintainers ptal at this proposal.
cc @vdemeester @pritidesai thought you might be interested

@JeromeJu
Copy link
Member

JeromeJu commented Oct 2, 2023

/assign

@jerop
Copy link
Member

jerop commented Oct 2, 2023

/kind tep

@tekton-robot tekton-robot added the kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). label Oct 2, 2023
@JeromeJu
Copy link
Member

JeromeJu commented Oct 2, 2023

/assign @pritidesai

@JeromeJu
Copy link
Member

JeromeJu commented Oct 2, 2023

/assign @vdemeester

Copy link
Member

@QuanZhang-William QuanZhang-William left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Yongxuanzhang, great TEP! I left some comments, but lgtm in general.

I believe having this feature and Param Enum can improve UX significantly.

teps/0145-cel-in-whenexpression.md Outdated Show resolved Hide resolved

The new fields will be gated by a new feature flag, `enable-cel-whenexpression`, which will defaults to `"false"` while the feature is in alpha.

The validation webhook will validate either the current when `input`+`operator`+`values` or `cel` is used, users cannot use both at the same time for one `WhenExpression`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if the validation webhook can validate if the given CEL expression is syntactically valid or not?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh this is a great suggestion! Thanks!

teps/0145-cel-in-whenexpression.md Show resolved Hide resolved
teps/0145-cel-in-whenexpression.md Outdated Show resolved Hide resolved
teps/0145-cel-in-whenexpression.md Outdated Show resolved Hide resolved
@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 10, 2023
@wlynch wlynch removed their request for review October 11, 2023 15:05
- cel: '$(params.branch)'.matches('release/.*')"
```

### Variables Substitution
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @vdemeester, I added one more section since last time you reviewed. This section will discuss how we handle variable substitution in CEL expression and how we can prevent the attack. PTAL 🙏

@Yongxuanzhang
Copy link
Member Author

Hi @pritidesai, @JeromeJu could you review this TEP? Thanks!

- input: "$(params.param1)$(params.param2)"
operator: notin
values: [""]
```
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, this is a workaround but @durera was hopping to implement more complex logic but chose to go this route since their desired conditional logic is not supported.

The desired conditional logic is:

      whenAny:
        - input: "$(params.param1)"
          operator: notin
          values: [""]
        - input: "$(params.param2)"
          operator: in
          values: ["8.5", "8.6"]

which I think can supported with a single CEL expression:

  when:
    cel: "'$(params.param1)' != '' || '$(params.param2)' == '8.5' || $(params.param2)' == '8.6'"

Do we know if there is any limitation on the length of the cel expression? If so, its worth documenting here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I missed the original one the user wants to use. Updated.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the length limitation, I have searched a bit but could not find answers...


### Variables Substitution

The CEL in When Expressions should support current Tekton’s Params and Results [string substitutions](https://github.com/tektoncd/pipeline/blob/main/docs/variables.md#variables-available-in-a-pipeline). Array and object substitutions are not supported because Tekton doesn’t support whole array/object replacements in a string.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this true? values is an array and it does honor the star notation, isn't it?

Its reasonable to implement only string params with CEL but are we supporting indexing into an array or referencing an object param using a key in CEL expressions?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I should clarify that we cannot reference to a whole array or object.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember there is an issue that we cannot reference param in "script", the case where a long string contains param reference.

I will remove this statement, and rephrase that we will support string replacements first

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed this statement

@pritidesai
Copy link
Member

Thanks a bunch @Yongxuanzhang for all the efforts on this. This will be greatly appreciated and utilized by the pipeline authors. I have left a couple of comments. Please address them before we can merge this since this is marked as implementable. Thanks again!

/approve

Copy link
Member

@JeromeJu JeromeJu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The proposal LGTM, just some small questions.

teps/0145-cel-in-whenexpression.md Outdated Show resolved Hide resolved
- Explicitly document that passing CEL expression from param is not allowed and won’t be executed.

The solution we proposed is to let CEL handle the variable substitution:
1. Add params, results, context variables to CEL's environment, similar like [Tekton Triggers](https://github.com/tektoncd/triggers/blob/main/pkg/interceptors/cel/cel.go#L104C1-L112)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the tekton trigger examples are nice to add. Would you mind also clarifying the environment here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added link to the cel environment, it would add too many details here if we want to add code snippets.

teps/0145-cel-in-whenexpression.md Show resolved Hide resolved
@Yongxuanzhang Yongxuanzhang force-pushed the tep-144 branch 6 times, most recently from ead28c8 to a035184 Compare October 13, 2023 16:06
@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JeromeJu, pritidesai, vdemeester

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

This commit adds the proposal of adding CEL in WhenExpression

Signed-off-by: Yongxuan Zhang [email protected]
Copy link
Member

@jerop jerop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merging offline as we have all the approvals 🚀

/lgtm

Thank you @Yongxuanzhang 🎉

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 13, 2023
@tekton-robot tekton-robot merged commit 3725139 into tektoncd:main Oct 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants