xds: Implementation of Unified Matcher and CEL Integration#12640
xds: Implementation of Unified Matcher and CEL Integration#12640shivaspeaks wants to merge 14 commits intogrpc:masterfrom
Conversation
shivaspeaks
force-pushed
the
xds-unified-matcher-and-cel
branch
from
082243d to
f472c63
Compare
l46kok
left a comment
l46kok
left a comment
There was a problem hiding this comment.
Sorry about the drive-by -- I happened to notice this while putting together an unrelated PR in CEL-Java. I'll hold off on submitting it on our end.
gradle/libs.versions.toml
Outdated
| # checkstyle 10.0+ requires Java 11+ | ||
| # See https://checkstyle.sourceforge.io/releasenotes_old_8-35_10-26.html#Release_10.0 | ||
| # checkForUpdates: checkstylejava8:9.+ | ||
| cel-runtime = "dev.cel:runtime:0.11.1" |
There was a problem hiding this comment.
If you plan on using both the compilation and runtime packages, you could consider just taking a dependency on dev.cel:cel instead, which includes both. It will likely make your dependency management much easier.
On a separate note -- let me know if you'd like us to cut a new release.
There was a problem hiding this comment.
We purposefully don't want a dependency on the cel parser. Also, it seems dev.cel:cel is busted, in that it re-defines dev.cel.runtime instead of depending on the other artifact. That will cause hard-to-debug duplicate classes in the class path.
There was a problem hiding this comment.
For artifact issue -- it's tracked, we can look into prioritizing it: (b/328302799).
The PR here is referencing CelCompiler, so I think you'll need the compiler dependency (unless if it shouldn't have been included here).
There was a problem hiding this comment.
The problem is pretty serious, as it prevents dependency management from working properly and it is not something regular people are able to debug. It's the sort of thing where everything looks fine, but then you make an unrelated change and somehow suddenly breaks CEL. So the concern is in support costs.
Really, I'm wary about merging this with the broken cel jars; it's best not to have them in our dependency tree. Even with the code disabled they can still harm users. Maybe it is best to make them compileOnly until things are fixed, which also means parts of testing and feature stabilization would be delayed until it is fixed.
The PR is only using CelCompiler in tests. It will need to get reorganized to avoid the dependency for production.
@shivaspeaks, you may want to glance at go/grpc-cel-integration. It was supposed to have been superseded by the gRFC, but I'm seeing the gRFC doesn't make a point of calling out some of the goals (like not depending on the CEL compiler). It's longer than you probably want to read in full, but you may just skim it to find the "interesting" parts.
There was a problem hiding this comment.
I'll look into this next week. I was actually under the impression this wasn't a hard blocker. Thanks for surfacing it up.
| .enableStringConversion(false) | ||
| .enableStringConcatenation(false) | ||
| .enableListConcatenation(false) |
There was a problem hiding this comment.
Consider subsetting the environment via CelStandardDeclarations (compiler) and CelStandardFunctions (runtime) instead. Examples:
- https://github.com/google/cel-java/blob/b3ee6a6599b20814488a83bb660cfaace158c0d2/checker/src/test/java/dev/cel/checker/CelStandardDeclarationsTest.java#L135
- https://github.com/google/cel-java/blob/b3ee6a6599b20814488a83bb660cfaace158c0d2/runtime/src/test/java/dev/cel/runtime/CelStandardFunctionsTest.java#L116
This is the preferred mechanism, as it gives you more custom control over what functions/overloads are exposed. It's the direction cel-go went too, so this is likely desirable from a consistency standpoint as well.
| * Compiles the CEL expression string into a CelMatcher. | ||
| */ | ||
| @VisibleForTesting | ||
| public static CelMatcher compile(String expression) |
There was a problem hiding this comment.
Move this to the testing code so we don't need a dependency on the Cel compiler. It looks unused for production.
gradle/libs.versions.toml
Outdated
| # checkstyle 10.0+ requires Java 11+ | ||
| # See https://checkstyle.sourceforge.io/releasenotes_old_8-35_10-26.html#Release_10.0 | ||
| # checkForUpdates: checkstylejava8:9.+ | ||
| cel-runtime = "dev.cel:runtime:0.11.1" |
There was a problem hiding this comment.
The problem is pretty serious, as it prevents dependency management from working properly and it is not something regular people are able to debug. It's the sort of thing where everything looks fine, but then you make an unrelated change and somehow suddenly breaks CEL. So the concern is in support costs.
Really, I'm wary about merging this with the broken cel jars; it's best not to have them in our dependency tree. Even with the code disabled they can still harm users. Maybe it is best to make them compileOnly until things are fixed, which also means parts of testing and feature stabilization would be delayed until it is fixed.
The PR is only using CelCompiler in tests. It will need to get reorganized to avoid the dependency for production.
@shivaspeaks, you may want to glance at go/grpc-cel-integration. It was supposed to have been superseded by the gRFC, but I'm seeing the gRFC doesn't make a point of calling out some of the goals (like not depending on the CEL compiler). It's longer than you probably want to read in full, but you may just skim it to find the "interesting" parts.
| String id = over.toString(); | ||
| return !id.equals("ADD_STRING") && !id.equals("ADD_LIST"); |
There was a problem hiding this comment.
You should be able to compare directly against the overload enum instead of the stringified form (ex: over != AddOverload.ADD_STRING).
3 participants
Implements gRFC A106: xDS Unified Matcher and CEL Integration
grpc/proposal#520