cel: Add canonical CEL (dev.cel.expr) fields #89
Conversation
sergiitk
force-pushed
the
canonical-cel
branch
2 times, most recently
from
ee39ead to
5034e4d
Compare
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 <[email protected]>
Signed-off-by: Sergii Tkachenko <[email protected]>
Signed-off-by: Sergii Tkachenko <[email protected]>
|
For posterity, the fix for the go build (besides the domain setup) was implemented in google/cel-spec#342. |
|
@sergiitk im not a maintainer of this repo cc @adisuissa |
| @@ -29,10 +29,14 @@ EXTERNAL_PROTO_GO_BAZEL_DEP_MAP = { | |||
| # go_googleapis in https://github.com/bazelbuild/rules_go/blob/master/go/dependencies.rst#overriding-dependencies | |||
| "@com_google_googleapis//google/api/expr/v1alpha1:checked_proto": "@go_googleapis//google/api/expr/v1alpha1:expr_go_proto", | |||
| "@com_google_googleapis//google/api/expr/v1alpha1:syntax_proto": "@go_googleapis//google/api/expr/v1alpha1:expr_go_proto", | |||
| "@dev_cel//proto/cel/expr:checked_proto": "@dev_cel//proto/cel/expr:checked_go_proto", | |||
There was a problem hiding this comment.
IIUC these are introduced in addition to the com_googleapis variant to avoid introducing a breaking API change. However, as this is a superset of the com_googleapis project, then I think that conceptually it needs to replace it completely (to avoid violating ODR).
I'm a bit concerned what will happen if these two repos diverge. but as we don't have versioning in place, I don't think we can do much about it.
cc @htuch if there are other ways here.
There was a problem hiding this comment.
@adisuissa Canonical CEL (cel.dev) is meant to replace v1alpha1 CEL from google APIs package. Canonical CEL will always be extended in a backward-compatible way (as protos normally should be), so there isn't a divergence problem. I'm CCing CEL folks here for more context: @l46kok, @TristonianJones.
Also, @tyxia has more context from the Envoy side.
There was a problem hiding this comment.
From Envoy perspective, cel.dev and v1alpha1 will co-exist. They are guaranteed to be in sync by CEL team.
New users in Envoy are recommended to use cel.dev while existing users can still use v1alpha1
Thus, v1alpha1 is deprecated but can not be removed.
There was a problem hiding this comment.
I think this makes sense. When the Envoy PR lands, which shoudl be adding implementation level support (not just API import), it might make sense to audit any remaining references to CEL that goes direct to google.api package namespace in the API protos in envoyproxy/envoy, e.g. RBAC.
There was a problem hiding this comment.
Looks like nothing is blocking this PR from being merged, right?
|
Hi @sergiitk , |
|
Thanks @mmorel-35. AFAIK this repo is not using bazel modules yet. Could you please let CEL folks know about it? They'll likely to be interested in including it into their docs. |
|
Ok, I created an issue so they can have a look. |
This PR adds canonical CEL (https://github.com/google/cel-spec/tree/master/proto/cel/expr) fields to
xds.type.v3.CelExpression. Canonical CELcel.exprwas created identical to thegoogle.api.expr.v1alpha1, but may be extended in a backward-compatible way.Nuances:
cel_expr_parsedandcel_expr_checkedare added outside ofoneof expr_specifierper updated policy change: API: change style guide to discourage use of "oneof" envoyproxy/envoy#30851option (validate.required) = trueis removed from theoneof expr_specifier, so the users may not presume one of theparsed_expr,checked_exprwill be set.Related:
go buildCI job #78.go.modand protobuf published in Add golang module for canonical protos. Move existing go module under tests google/cel-spec#342 / v1.15.0.com_github_cncf_xdsto cncf/xds@0c46c01 envoyproxy/envoy#33160. cncf/xds version will be updated once this PR is merged.