[celeval] CEL Custom Tasks CRD #784

jerop · 2021-08-26T14:47:31Z

Changes

We introduced CEL Custom Tasks to experiment with using an expression language with Tekton Pipelines.

Given feedback from the past several months of usage, we have identified three main current challenges:

CEL custom tasks do not take variables for the CEL environment. As such, users cannot evaluate CEL expressions given specific variables or in specific context. For example, as described in Add ability to parse variables in expression from context for CEL #716 and Proposal: Introduce new CRD for CEL community#403, a user needed to declare runtime storage variables in the CEL environment.
CEL custom tasks are not a CRD thus making them unreusable across different Runs and PipelineRuns. Read more in TEP-0043: Common Expression Language Custom Task (CELRun) community#314 (review).
CEL custom tasks take the CEL expressions through Parameters which is misleading to some users. Read more in TEP-0043: Common Expression Language Custom Task (CELRun) community#314 (review).

To address the above challenges, this change introduces a CRD for CEL Custom Tasks, which takes CEL expressions and CEL environment variables. With this change:

CEL custom tasks now take variables for the CEL environment
CEL custom tasks are reusable across different Runs and PipelineRuns
CEL custom tasks take expressions through its own field

Closes tektoncd/community#403
Closes #716

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes docs (if user facing)
Commit messages follow commit message best practices
Commit messages includes a project tag in the subject - e.g. [OCI], [hub], [results], [task-loops]

See the contribution guide for more details.

/cc @vincent-pli @bobcatfish @imjasonh @pritidesai @bigkevmcd @vdemeester

tekton-robot · 2021-08-26T14:47:35Z

@jerop: GitHub didn't allow me to request PR reviews from the following users: vincent-pli.

Note that only tektoncd members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

Changes

We introduced CEL Custom Tasks to experiment with using an expression language with Tekton Pipelines.

Given feedback from the past several months of usage, we have identified three main current challenges:

CEL custom tasks do not take variables for the CEL environment. As such, users cannot evaluate CEL expressions given specific variables or in specific context. For example, as described in Add ability to parse variables in expression from context for CEL #716 and Proposal: Introduce new CRD for CEL community#403, a user needed to declare runtime storage variables in the CEL environment.

CEL custom tasks are not a CRD thus making them unreusable across different Runs and PipelineRuns. Read more in TEP-0043: Common Expression Language Custom Task (CELRun) community#314 (review).

CEL custom tasks take the CEL expressions through Parameters which is misleading to some users. Read more in TEP-0043: Common Expression Language Custom Task (CELRun) community#314 (review).

To address the above challenges, this change introduces a CRD for CEL Custom Tasks, which takes CEL expressions and CEL environment variables. With this change:

CEL custom tasks now take variables for the CEL environment

CEL custom tasks are reusable across different Runs and PipelineRuns

CEL custom tasks take expressions through its own field

Closes tektoncd/community#403
Closes #716

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes docs (if user facing)

Commit messages follow commit message best practices

Commit messages includes a project tag in the subject - e.g. [OCI], [hub], [results], [task-loops]

See the contribution guide for more details.

/cc @vincent-pli @bobcatfish @imjasonh @pritidesai @bigkevmcd @vdemeester

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

cel/config/201-clusterrole.yaml

cel/config/500-controller.yaml

cel/pkg/reconciler/celrun/celrun.go

wlynch has reviewed #784 where we introduce cel custom tasks crd -- so adding him as an owner so he has approval permissions

cel/README.md

jerop · 2021-09-07T22:16:37Z

/retest

bobcatfish · 2021-09-13T21:30:27Z

The last thing on my mind is renaming from CEL to something a bit more descriptive but other than that 👍 !

/approve

wlynch

Aside from the RBAC feedback, everything else is minor nits. Should be LGTM after this!

wlynch · 2021-09-13T20:37:46Z

cel/config/201-role.yaml

+    resourceNames: ["config-logging", "config-observability", "config-leader-election"]
+  - apiGroups: [""]
+    resources: ["secrets"]
+    verbs: ["list", "watch"]


From https://kubernetes.io/docs/concepts/configuration/secret/#clients-that-use-the-secret-api -

For these reasons watch and list requests for secrets within a namespace are extremely powerful capabilities and should be avoided, since listing secrets allows the clients to inspect the values of all secrets that are in that namespace. The ability to watch and list all secrets in a cluster should be reserved for only the most privileged, system-level components.

Can we scope this down as well? (I assume this also applies to configmaps as well)

cel/pkg/reconciler/celrun/celrun.go

wlynch · 2021-09-13T20:50:21Z

cel/pkg/reconciler/celrun/celrun.go

+		run.ObjectMeta.Labels[key] = value
+	}
+	run.ObjectMeta.Labels[cel.GroupName+CELLabelKey] = CELMeta.Name
+	run.ObjectMeta.Labels[KindLabelKey] = cel.ControllerName


"kind" is a bit generic (and might have a confusing meaning w/ Object kind) - should we use app.kubernetes.io/managed-by instead?

wlynch · 2021-09-13T20:54:30Z

cel/pkg/reconciler/celrun/celrun.go

+
+const (
+	// CELLabelKey is the label identifier for a CEL, which is added to the Run
+	CELLabelKey = "/CEL"


Are labels case sensitive? Double checking since the README.md examples show custom.tekton.dev/cel.

wlynch · 2021-09-13T20:55:45Z

cel/pkg/reconciler/celrun/celrun.go

+	run.ObjectMeta.Labels[cel.GroupName+CELLabelKey] = CELMeta.Name
+	run.ObjectMeta.Labels[KindLabelKey] = cel.ControllerName


Let's add a section to the docs to list labels we add to the created object and what they mean.

cel/pkg/reconciler/celrun/celrun.go

cel/pkg/reconciler/celrun/celrun_test.go

wlynch · 2021-09-13T21:58:11Z

cel/pkg/reconciler/celrun/celrun_test.go

+	// The fake recorder runs in a go routine, so the timeout is here to avoid waiting
+	// on the channel forever if fewer than expected events are received.
+	// We only hit the timeout in case of failure of the test, so the actual value
+	// of the timeout is not so relevant. It's only used when tests are going to fail.


You might be able to avoid the timeout altogether if you close the channel before comparing. Since the reconciler.Reconcile call is synchronous, this should be safe.

cel/test/init_test.go

tekton-robot · 2021-10-19T13:15:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish

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:

~~OWNERS~~ [bobcatfish]

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

We introduced CEL Custom Tasks to experiment with using an expression language with Tekton Pipelines. Given feedback from the past several months of usage, we have identified three main current challenges: - CEL custom tasks do not take variables for the CEL environment. As such, users cannot evaluate CEL expressions given specific variables or in specific context. For example, as described in tektoncd#716 and tektoncd/community#403, a user needed to declare runtime storage variables in the CEL environment. - CEL custom tasks are not a CRD thus making them unreusable across different Runs and PipelineRuns. Read more in tektoncd/community#314 (review). - CEL custom tasks take the CEL expressions through Parameters which is misleading to some users. Read more in tektoncd/community#314 (review). To address the above challenges, this change introduces a CRD for CEL Custom Tasks, which takes CEL expressions and CEL environment variables. With this change: - CEL custom tasks now take variables for the CEL environment - CEL custom tasks are reusable across different Runs and PipelineRuns - CEL custom tasks take expressions through its own field Closes tektoncd/community#403 Closes tektoncd#716

tekton-robot · 2022-01-14T12:13:30Z

@jerop: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tekton-robot · 2022-04-14T13:09:46Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale with a justification.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle stale

Send feedback to tektoncd/plumbing.

jerop · 2022-05-02T21:37:54Z

/lifecycle frozen

jerop · 2022-05-02T21:38:19Z

hoping to revisit this some time 🤞🏾

closing for now

tekton-robot requested review from bobcatfish, imjasonh, pritidesai, bigkevmcd and vdemeester August 26, 2021 14:47

tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 26, 2021

jerop force-pushed the cel-crd branch from 907bf64 to b26f32f Compare August 26, 2021 14:52

jerop marked this pull request as draft August 26, 2021 14:56

jerop force-pushed the cel-crd branch 21 times, most recently from 6b79981 to eb7268c Compare August 31, 2021 20:58

jerop force-pushed the cel-crd branch from 411907a to 43c8535 Compare September 3, 2021 21:19

wlynch reviewed Sep 3, 2021

View reviewed changes

tekton-robot pushed a commit that referenced this pull request Sep 3, 2021

[cel] add wlynch as an owner

c84b196

wlynch has reviewed #784 where we introduce cel custom tasks crd -- so adding him as an owner so he has approval permissions

jerop force-pushed the cel-crd branch 2 times, most recently from 8f85ec9 to 693e075 Compare September 3, 2021 22:16

bobcatfish reviewed Sep 3, 2021

View reviewed changes

cel/README.md Outdated Show resolved Hide resolved

bobcatfish reviewed Sep 3, 2021

View reviewed changes

cel/README.md Outdated Show resolved Hide resolved

jerop force-pushed the cel-crd branch 3 times, most recently from ee11149 to 150ca7c Compare September 7, 2021 22:11

jerop force-pushed the cel-crd branch from 150ca7c to d0223e2 Compare September 8, 2021 17:21

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 13, 2021

wlynch reviewed Sep 13, 2021

View reviewed changes

jerop mentioned this pull request Oct 14, 2021

Add ability to parse variables in expression from context for CEL #716

Open

jerop force-pushed the cel-crd branch from d0223e2 to 4834b92 Compare October 19, 2021 13:15

jerop force-pushed the cel-crd branch from 4834b92 to f492a7a Compare October 19, 2021 13:16

jerop changed the title ~~[cel] CEL Custom Tasks CRD~~ [celeval] CEL Custom Tasks CRD Oct 19, 2021

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 14, 2022

tekton-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 14, 2022

tekton-robot added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels May 2, 2022

jerop closed this May 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[celeval] CEL Custom Tasks CRD #784

[celeval] CEL Custom Tasks CRD #784

jerop commented Aug 26, 2021 •

edited

Loading

tekton-robot commented Aug 26, 2021

Changes

Submitter Checklist

jerop commented Sep 7, 2021

bobcatfish commented Sep 13, 2021

wlynch left a comment

wlynch Sep 13, 2021

wlynch Sep 13, 2021

wlynch Sep 13, 2021

wlynch Sep 13, 2021

wlynch Sep 13, 2021

tekton-robot commented Oct 19, 2021

tekton-robot commented Jan 14, 2022

tekton-robot commented Apr 14, 2022

jerop commented May 2, 2022

jerop commented May 2, 2022

		run.ObjectMeta.Labels[cel.GroupName+CELLabelKey] = CELMeta.Name
		run.ObjectMeta.Labels[KindLabelKey] = cel.ControllerName

[celeval] CEL Custom Tasks CRD #784

[celeval] CEL Custom Tasks CRD #784

Conversation

jerop commented Aug 26, 2021 • edited Loading

Changes

Submitter Checklist

tekton-robot commented Aug 26, 2021

Changes

Submitter Checklist

jerop commented Sep 7, 2021

bobcatfish commented Sep 13, 2021

wlynch left a comment

Choose a reason for hiding this comment

wlynch Sep 13, 2021

Choose a reason for hiding this comment

wlynch Sep 13, 2021

Choose a reason for hiding this comment

wlynch Sep 13, 2021

Choose a reason for hiding this comment

wlynch Sep 13, 2021

Choose a reason for hiding this comment

wlynch Sep 13, 2021

Choose a reason for hiding this comment

tekton-robot commented Oct 19, 2021

tekton-robot commented Jan 14, 2022

tekton-robot commented Apr 14, 2022

jerop commented May 2, 2022

jerop commented May 2, 2022

jerop commented Aug 26, 2021 •

edited

Loading