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

cel: Add canonical CEL (dev.cel.expr) fields #75

Merged
merged 1 commit into from
Nov 17, 2023

Conversation

sergiitk
Copy link
Contributor

@sergiitk sergiitk commented Nov 11, 2023

This PR adds canonical CEL (https://github.com/google/cel-spec/tree/master/proto/cel/expr) fields to xds.type.v3.CelExpression. Canonical CEL cel.expr was created identical to the google.api.expr.v1alpha1, but may be extended in a backward-compatible way.

Nuances:

  1. The new fields cel_expr_parsed and cel_expr_checked are added outside of oneof expr_specifier per updated policy change: API: change style guide to discourage use of "oneof" envoyproxy/envoy#30851
  2. option (validate.required) = true is removed from the oneof expr_specifier, so the users may not presume one of the parsed_expr, checked_expr will be set.

@sergiitk sergiitk changed the title Add canonical CEL support: dev.cel.expr (com_github_grpc_grpc approach) Add canonical CEL fields support Nov 15, 2023
@sergiitk sergiitk changed the title Add canonical CEL fields support Add canonical CEL fields to CelExpression Nov 15, 2023
@sergiitk sergiitk changed the title Add canonical CEL fields to CelExpression cel: Add canonical CEL fields Nov 15, 2023
@sergiitk sergiitk changed the title cel: Add canonical CEL fields cel: Add canonical CEL (dev.cel.expr) fields Nov 15, 2023
Signed-off-by: Sergii Tkachenko <sergiitk@google.com>
@sergiitk sergiitk marked this pull request as ready for review November 16, 2023 19:49
@sergiitk
Copy link
Contributor Author

@tyxia @markdroth Please review.

@sergiitk
Copy link
Contributor Author

Here's how generated Envoy doc looks:
image

Copy link

@tyxia tyxia left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

xds/type/v3/cel.proto Show resolved Hide resolved
Copy link

@tyxia tyxia left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks!

xds/type/v3/cel.proto Show resolved Hide resolved
@markdroth markdroth merged commit 0f5e0d9 into cncf:main Nov 17, 2023
2 checks passed
@sergiitk sergiitk deleted the canonical-cel branch November 20, 2023 18:34
sergiitk added a commit to sergiitk/xds that referenced this pull request Nov 21, 2023
sergiitk added a commit to sergiitk/xds that referenced this pull request Nov 21, 2023
This reverts commit 0f5e0d9.
Temporarily reverting the PR so that folks aren't affected by
cncf#76.
Will be brought back once CEL folks solve the domain issue.

Signed-off-by: Sergii Tkachenko <sergiitk@google.com>
adisuissa pushed a commit that referenced this pull request Nov 21, 2023
Signed-off-by: Sergii Tkachenko <sergiitk@google.com>
sergiitk added a commit to sergiitk/xds that referenced this pull request Nov 27, 2023
sergiitk added a commit to sergiitk/xds that referenced this pull request Nov 27, 2023
…ncf#79)"

This reverts commit 5b9bca5.

Reverts cncf#79.
Now that cncf#78 `go build` CI is in place; this PR brings back the
CEL fields.

Once `del.dev` domain is fixed, we'll see the CI passing and will be
able to merge this PR.

Original comment
---

This PR adds canonical CEL
(https://github.com/google/cel-spec/tree/master/proto/cel/expr) fields
to `xds.type.v3.CelExpression`. Canonical CEL `cel.expr` was created
identical to the `google.api.expr.v1alpha1`, but may be extended in a
backward-compatible way.

Nuances: 1. The new fields `cel_expr_parsed` and `cel_expr_checked` are
added outside of `oneof expr_specifier` per updated policy change:
envoyproxy/envoy#30851 2. `option
(validate.required) = true` is removed from the `oneof expr_specifier`,
so the users may not presume one of the `parsed_expr`, `checked_expr`
will be set.

---

Signed-off-by: Sergii Tkachenko <sergiitk@google.com>
sergiitk added a commit to sergiitk/xds that referenced this pull request Jan 24, 2024
…ncf#79)"

This reverts commit 5b9bca5.

Reverts cncf#79.
Now that cncf#78 `go build` CI is in place; this PR brings back the
CEL fields.

Once `del.dev` domain is fixed, we'll see the CI passing and will be
able to merge this PR.

Original comment

Signed-off-by: Sergii Tkachenko <sergiitk@google.com>
---

This PR adds canonical CEL
(https://github.com/google/cel-spec/tree/master/proto/cel/expr) fields
to `xds.type.v3.CelExpression`. Canonical CEL `cel.expr` was created
identical to the `google.api.expr.v1alpha1`, but may be extended in a
backward-compatible way.

Nuances: 1. The new fields `cel_expr_parsed` and `cel_expr_checked` are
added outside of `oneof expr_specifier` per updated policy change:
envoyproxy/envoy#30851 2. `option
(validate.required) = true` is removed from the `oneof expr_specifier`,
so the users may not presume one of the `parsed_expr`, `checked_expr`
will be set.

---

Signed-off-by: Sergii Tkachenko <sergiitk@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants