-
Notifications
You must be signed in to change notification settings - Fork 38.6k
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
Fix bug where CEL listOfString.join() results in unexpected error #117593
Conversation
@@ -339,6 +339,17 @@ func TestValidationExpressions(t *testing.T) { | |||
"self.val1 + [4, 5] == [1, 2, 3, 4, 5]", | |||
}, | |||
}, | |||
{name: "string lists", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need a negative test that obj: objs([]interface{}{"a", 1, "c"}),
errors when join is attempted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CEL compiler prevents mixed type lists via https://github.com/google/cel-go/blob/d39523c445fcf548ef6529b63c1a1045cc373d37/cel/options.go#L129, which we have enabled.
I could test that non-string lists are not eligible to be joined, but it seems excessive to test that all functions are only available on the receivers that they are defined for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, if we guard against it and test for that guard, that's fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SG. I've added the tests for the guard (for both lists and maps).
/lgtm |
LGTM label has been added. Git tree hash: 28fc3035a7c6c8d8fffda29e464bc584f21652ae
|
#117596 backports to 1.27, which is where this is needed most, since it helps a lot for |
/triage accepted |
/lgtm |
LGTM label has been added. Git tree hash: b353f8a42d4be0335a16c19f1bba6716239b57a4
|
Co-authored-by: Alvaro Aleman <[email protected]>
/lgtm |
LGTM label has been added. Git tree hash: 22f16fde1442cdddf86ac43ee02ae1e108309c0f
|
/retest |
1 similar comment
/retest |
/priority important-soon |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jpbetz, lavalamp 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 |
/retest |
…93-origin-release-1.27 Automated cherry pick of #117593: Fix bug where CEL listOfString.join() results in unexpected
What type of PR is this?
/kind bug
What this PR does / why we need it:
Without string.join() it can be difficult to leverage the
messageExpression
field added to CRD validation rules and ValidatingAdmissionPolicy in 1.27.Which issue(s) this PR fixes:
Fixes #117590
Special notes for your reviewer:
We may be able to simplify this in the future depending on the outcome of google/cel-go#688. For now, this introduces an isolated workaround.
Does this PR introduce a user-facing change?