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.

Sign up for GitHub

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

Add resource constraints for CEL #3144

Merged
merged 20 commits into from
Feb 3, 2022
Merged

Add resource constraints for CEL #3144

Changes from 12 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
ae09cbb
Add initial info on CEL resource constraints.
DangerOnTheRanger Jan 13, 2022
16283e1
Reword to imply depth limiting is just an option.
DangerOnTheRanger Jan 13, 2022
11be065
Move resource constraint timeout to the top.
DangerOnTheRanger Jan 13, 2022
eea4fd8
Add link to CEL cost subsystem code.
DangerOnTheRanger Jan 13, 2022
ff38644
Change timeout from per-expression to per-request.
DangerOnTheRanger Jan 13, 2022
efda13f
Move paragraph on cost subsystem to the end.
DangerOnTheRanger Jan 13, 2022
fa0b979
Change CEL expression timeout to 5 seconds.
DangerOnTheRanger Jan 13, 2022
bf2b434
Change text that was out-of-date after reformatting.
DangerOnTheRanger Jan 13, 2022
76e4605
Note that the timeout message will include evaluation time.
DangerOnTheRanger Jan 13, 2022
e4b49ba
Specifcy that Beta timeout will be 100ms-1s.
DangerOnTheRanger Jan 14, 2022
8bd777a
Run hack/update-toc.sh.
DangerOnTheRanger Jan 14, 2022
e541ae3
Clarify that Beta performance will be tested against benchmarks.
DangerOnTheRanger Jan 14, 2022
6a550cb
Prepare for final review.
DangerOnTheRanger Feb 1, 2022
ac9e0e7
Run hack/update-toc.sh
DangerOnTheRanger Feb 1, 2022
4e440b0
Update in response to feedback.
DangerOnTheRanger Feb 3, 2022
3e36b22
Remove per-expression timeout, make per-request timeout sourced from …
DangerOnTheRanger Feb 3, 2022
8736077
Add runtime cost check.
DangerOnTheRanger Feb 3, 2022
49fe807
Run hack/update-toc.sh
DangerOnTheRanger Feb 3, 2022
5f96b1c
Minor fixes with APF wording.
DangerOnTheRanger Feb 3, 2022
5f44379
Keep cost bounds as future work.
DangerOnTheRanger Feb 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions keps/sig-api-machinery/2876-crd-validation-expression-language/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
- [Type Checking](#type-checking)
- [Type System Integration](#type-system-integration)
- [Why not represent associative lists as maps in CEL?](#why-not-represent-associative-lists-as-maps-in-cel)
- [Resource constraints](#resource-constraints)
- [Test Plan](#test-plan)
- [Graduation Criteria](#graduation-criteria)
- [Alpha](#alpha)
Expand Down Expand Up @@ -589,6 +590,29 @@ associativeList.all(e, e.key1 == 'a' && e.key2 == 'b' && e.val == 100)
associativeList.all(e, e.val == 100)
```

### Resource constraints

For Beta, per-request execution time will be constrained via a context-passed timeout. For Beta, we are looking at a range between 100 milliseconds and 1 second for the timeout. We want to see how
CEL performs in benchmarks once our Beta optimizations are in place before deciding what specific
duration to set the timeout to. If the timeout is exceeded, the error message will include how
long each expression took to evaluate.
Copy link
Member

Choose a reason for hiding this comment

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

this has the downside of permitting costly CEL expressions that time out to be persisted in the CRD, and fail at CR request time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely - only having a timeout doesn't provide nearly as strong guarantees as estimating expression cost. I think if we can guarantee good cost estimation, we should go with it. I'm a little concerned about accuracy, since I feel there could be cases where an incorrect estimate is given, and an expression that should be allowed isn't permitted (or vice versa).


We will provide time and space benchmarks to show what can be expected in typical and worst-case
scenarios for CEL expressions, and that for most use cases, the timeout alone will be sufficient.
The alternatives listed below would be more aimed at malformed/malicious expressions.
Copy link
Member

Choose a reason for hiding this comment

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

guarding against malformed/malicious expressions is definitely in scope


If the context timeout proves to be insufficient, we have other alternatives to explore, such as
limiting nested list comprehensions to a depth of 2. According to the performance section of
[the CEL spec](https://github.com/google/cel-spec/blob/master/doc/langdef.md#performance), the
time complexity of these operations are all O(n), so by limiting nesting to 2 deep, we limit
expression complexity to O(n<sup>2</sup>). We can limit regular expression complexity
as well.

CEL also provides a [cost subsystem](https://github.com/google/cel-go/blob/dfef54b359b05532fb9695bc88937aa8530ab055/cel/program.go#L309) that could be used in the future,
but the cost subsystem would need to know the length of any relevant lists in order to be useful.
That information can be supplied using `maxLength`, but this is an optional field, and if not
passed, CEL would not be able to provide a useful figure.
Copy link
Member

@liggitt liggitt Jan 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure if it's useful, but we could set a max length of a list based on the 3MB request limit and the size of the most compact encoding possible:

  • int / float lists = 3*1024*1024 / 2 (0,0,0,0,0,0,0,0,0,... encoding)
  • string lists = 3*1024*1024 / 3 ("","","","","",... encoding)
  • list lists = 3*1024*1024 / 3 ([],[],[],[],[],... encoding)
  • object lists = 3*1024*1024 / 3 ({},{},{},{},{},... encoding)

I'm not sure if passing such big list sizes to CEL would make the cost subsystem go nuts or not, but we could check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'll give passing lists of that size through CEL a shot and see what happens. I think that list might work well to provide maxLength with a default value, though I'm concerned that there will be a large discrepancy between what we'd estimate resource usage to be versus what we'd see in practice if we actually ran the expressions. I suspect any solution that works without maxLength being explicitly set will run into the same issue, though.


### Test Plan

We will extend both the unit test suite and the integration test suite to cover the CRD validation rule described in this KEP.
Expand Down