Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -562,7 +562,10 @@ private boolean containsEliminableFunctionOnly(CelNavigableMutableExpr navigable
.allMatch(
node -> {
if (node.getKind().equals(Kind.CALL)) {
return cseEliminableFunctions.contains(node.expr().call().function());
String function = node.expr().call().function();
return !cseOptions.nonEliminableFunctions().contains(function)
&& (cseOptions.eliminateAllFunctions()
|| cseEliminableFunctions.contains(function));
}

return true;
Expand Down Expand Up @@ -722,6 +725,10 @@ public abstract static class SubexpressionOptimizerOptions {

public abstract ImmutableSet<String> eliminableFunctions();

public abstract boolean eliminateAllFunctions();

public abstract ImmutableSet<String> nonEliminableFunctions();

/** Builder for configuring the {@link SubexpressionOptimizerOptions}. */
@AutoValue.Builder
public abstract static class Builder {
Expand All @@ -748,23 +755,48 @@ public abstract static class Builder {

/**
* Adds a collection of custom functions that will be a candidate for common subexpression
* elimination. By default, standard functions are eliminable.
* elimination. Only takes effect with {@code eliminateAllFunctions(false)}, where standard
* functions are eliminable by default.
*
* <p>Note that the implementation of custom functions must be free of side effects.
*
* @deprecated Use {@link #addNonEliminableFunctions} to exclude functions with side effects
* instead.
*/
@Deprecated
@CanIgnoreReturnValue
public Builder addEliminableFunctions(Iterable<String> functions) {
checkNotNull(functions);
this.eliminableFunctionsBuilder().addAll(functions);
return this;
}

/** See {@link #addEliminableFunctions(Iterable)}. */
/**
* @deprecated See {@link #addEliminableFunctions(Iterable)}.
*/
@Deprecated
@CanIgnoreReturnValue
public Builder addEliminableFunctions(String... functions) {
return addEliminableFunctions(Arrays.asList(functions));
}

/**
* Treats all functions, including custom ones, as candidates for common subexpression
* elimination (enabled by default). Functions with side effects must be excluded via {@link
* #addNonEliminableFunctions}.
*/
public abstract Builder eliminateAllFunctions(boolean value);

abstract ImmutableSet.Builder<String> nonEliminableFunctionsBuilder();

/** Excludes functions (e.g. those with side effects) from elimination. */
@CanIgnoreReturnValue
public Builder addNonEliminableFunctions(Iterable<String> functions) {
checkNotNull(functions);
this.nonEliminableFunctionsBuilder().addAll(functions);
return this;
}

public abstract SubexpressionOptimizerOptions build();

Builder() {}
Expand All @@ -775,7 +807,8 @@ public static Builder newBuilder() {
return new AutoValue_SubexpressionOptimizer_SubexpressionOptimizerOptions.Builder()
.iterationLimit(500)
.enableCelBlock(true)
.populateMacroCalls(false);
.populateMacroCalls(false)
.eliminateAllFunctions(true);
}

SubexpressionOptimizerOptions() {}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,8 @@ private static Cel setupCelEnv(CelBuilder celBuilder) {
private static final SubexpressionOptimizerOptions OPTIMIZER_COMMON_OPTIONS =
SubexpressionOptimizerOptions.newBuilder()
.populateMacroCalls(true)
.addEliminableFunctions("pure_custom_func")
.eliminateAllFunctions(true)
.addNonEliminableFunctions(ImmutableList.of("non_pure_custom_func"))
.build();

@Before
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -210,10 +210,6 @@ private enum CseNoOpTestCase {
// Constants and identifiers within a function
CONST_WITHIN_FUNCTION("size(\"hello\" + \"hello\" + \"hello\")"),
IDENT_WITHIN_FUNCTION("string(x + x + x)"),
// Non-standard functions that have not been explicitly added as a candidate are not
// optimized.
NON_STANDARD_FUNCTION_1("non_pure_custom_func(1) + non_pure_custom_func(1)"),
NON_STANDARD_FUNCTION_2("1 + non_pure_custom_func(1) + 1 + non_pure_custom_func(1)"),
// Duplicated but nested calls.
NESTED_FUNCTION("int(timestamp(int(timestamp(1000000000))))"),
// This cannot be optimized. Extracting the common subexpression would presence test
Expand Down Expand Up @@ -252,6 +248,55 @@ public void cse_withCelBlock_noop(@TestParameter CseNoOpTestCase testCase) throw
assertThat(CEL_UNPARSER.unparse(optimizedAst)).isEqualTo(testCase.source);
}

@Test
public void cse_customFunction_eliminatedByDefault() throws Exception {
CelAbstractSyntaxTree ast =
cel.compile("non_pure_custom_func(1) + non_pure_custom_func(1)").getAst();

CelAbstractSyntaxTree optimizedAst =
newCseOptimizer(SubexpressionOptimizerOptions.newBuilder().build()).optimize(ast);

assertThat(CEL_UNPARSER.unparse(optimizedAst))
.isEqualTo("cel.@block([non_pure_custom_func(1)], @index0 + @index0)");
}

@Test
public void cse_eliminateAllFunctionsDisabled_customFunctionRetained(
@TestParameter({
"non_pure_custom_func(1) + non_pure_custom_func(1)",
"1 + non_pure_custom_func(1) + 1 + non_pure_custom_func(1)"
})
String source)
throws Exception {
CelAbstractSyntaxTree ast = cel.compile(source).getAst();

CelAbstractSyntaxTree optimizedAst =
newCseOptimizer(
SubexpressionOptimizerOptions.newBuilder().eliminateAllFunctions(false).build())
.optimize(ast);

assertThat(CEL_UNPARSER.unparse(optimizedAst)).isEqualTo(source);
}

@Test
public void cse_eliminateAllFunctions_nonEliminableFunctionRetained() throws Exception {
CelAbstractSyntaxTree ast =
cel.compile("non_pure_custom_func(size('a')) + non_pure_custom_func(size('a'))").getAst();

CelAbstractSyntaxTree optimizedAst =
newCseOptimizer(
SubexpressionOptimizerOptions.newBuilder()
.eliminateAllFunctions(true)
.addNonEliminableFunctions(ImmutableList.of("non_pure_custom_func"))
.build())
.optimize(ast);

assertThat(CEL_UNPARSER.unparse(optimizedAst))
.isEqualTo(
"cel.@block([size(\"a\")], non_pure_custom_func(@index0) +"
+ " non_pure_custom_func(@index0))");
}

@Test
public void cse_withComprehensionStructureRetained() throws Exception {
CelAbstractSyntaxTree ast =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,14 @@ public final class CelPolicyCompilerTool implements Callable<Integer> {
description = "Maximum iteration limit for composing and optimizing the policy")
private int iterationLimit = 1000;

@Option(
names = {"--non_eliminable_functions"},
split = ",",
description =
"Comma-separated functions with side effects to exclude from common subexpression"
+ " elimination")
private String[] nonEliminableFunctions = {};

private static final CelOptions CEL_OPTIONS =
CelOptions.current()
.populateMacroCalls(true)
Expand Down Expand Up @@ -233,6 +241,9 @@ public Integer call() {
SubexpressionOptimizerOptions.newBuilder()
.iterationLimit(iterationLimit)
.populateMacroCalls(true)
// CEL functions are assumed side-effect free unless opted out.
.eliminateAllFunctions(true)
.addNonEliminableFunctions(ImmutableList.copyOf(nonEliminableFunctions))
.build()));
if (optimizeFieldSelection) {
optimizersBuilder.add(
Expand Down
9 changes: 4 additions & 5 deletions policy/src/test/java/dev/cel/policy/PolicyTestHelper.java
Original file line number Diff line number Diff line change
Expand Up @@ -87,12 +87,11 @@ enum TestYamlPolicy {
RESTRICTED_DESTINATIONS(
"restricted_destinations",
false,
"cel.@block([request.auth.claims, has(@index0.nationality), resource.labels.location in"
"cel.@block([request.auth.claims, has(@index0.nationality), locationCode(destination.ip) in"
+ " spec.restricted_destinations || resource.labels.location in"
+ " spec.restricted_destinations], (@index1 && @index0.nationality == spec.origin &&"
+ " (locationCode(destination.ip) in spec.restricted_destinations || @index2)) ? true :"
+ " ((!@index1 && locationCode(origin.ip) == spec.origin &&"
+ " (locationCode(destination.ip) in spec.restricted_destinations || @index2)) ? true :"
+ " false))"),
+ " @index2) ? true : ((!@index1 && locationCode(origin.ip) == spec.origin && @index2)"
+ " ? true : false))"),
K8S(
"k8s",
true,
Expand Down
1 change: 1 addition & 0 deletions policy/src/test/java/dev/cel/policy/tools/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ java_library(
"//common:cel_source",
"//common:proto_ast",
"//extensions:optional_library",
"//parser:unparser",
"//policy/tools",
"//runtime",
"@bazel_tools//tools/java/runfiles",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import static java.nio.charset.StandardCharsets.UTF_8;

import dev.cel.expr.CheckedExpr;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.io.Files;
import com.google.devtools.build.runfiles.AutoBazelRepository;
Expand All @@ -29,6 +30,8 @@
import dev.cel.common.CelSource;
import dev.cel.expr.conformance.proto3.TestAllTypes;
import dev.cel.extensions.CelOptionalLibrary;
import dev.cel.parser.CelUnparser;
import dev.cel.parser.CelUnparserFactory;
import dev.cel.runtime.CelRuntime;
import dev.cel.runtime.CelRuntimeFactory;
import java.io.ByteArrayOutputStream;
Expand All @@ -50,6 +53,8 @@
@AutoBazelRepository
public final class CelPolicyCompilerToolTest {

private static final CelUnparser CEL_UNPARSER = CelUnparserFactory.newUnparser();

@Rule public TemporaryFolder tempFolder = new TemporaryFolder();

private Runfiles runfiles;
Expand Down Expand Up @@ -327,6 +332,27 @@ public void compile_withIterationLimitReached_returnsError() throws Exception {
assertThat(stdErr).contains("Reason: Unexpected error while composing rules.");
}

@Test
public void compile_customFunction_eliminatedByDefault() throws Exception {
CelAbstractSyntaxTree ast = compileCustomFunctionPolicy();

assertThat(CEL_UNPARSER.unparse(ast))
.isEqualTo(
"cel.@block([expensive(\"x\")], (@index0 > 1 && @index0 < 10) ? optional.of(true) :"
+ " optional.none())");
}

@Test
public void compile_withNonEliminableFunctions_retainsCalls() throws Exception {
CelAbstractSyntaxTree ast =
compileCustomFunctionPolicy("--non_eliminable_functions", "expensive,unused");

assertThat(CEL_UNPARSER.unparse(ast))
.isEqualTo(
"(expensive(\"x\") > 1 && expensive(\"x\") < 10) ? optional.of(true) :"
+ " optional.none()");
}

@Test
public void compile_withOptimizeFieldSelection_rewritesSelectAndEvaluates() throws Exception {
String configRlocation =
Expand Down Expand Up @@ -923,6 +949,47 @@ private String createFile(String fileName, String content) throws IOException {
return file.getAbsolutePath();
}

private CelAbstractSyntaxTree compileCustomFunctionPolicy(String... extraArgs) throws Exception {
String configPath =
createFile(
"config.yaml",
"name: test-env\n"
+ "functions:\n"
+ " - name: expensive\n"
+ " overloads:\n"
+ " - id: expensive_string\n"
+ " args:\n"
+ " - type_name: string\n"
+ " return:\n"
+ " type_name: int\n");
String policyPath =
createFile(
"policy.yaml",
"name: p\n"
+ "rule:\n"
+ " variables:\n"
+ " - name: score\n"
+ " expression: expensive('x')\n"
+ " match:\n"
+ " - condition: variables.score > 1 && variables.score < 10\n"
+ " output: 'true'\n");
File outputFile = tempFolder.newFile("output.binarypb");
ImmutableList<String> args =
ImmutableList.<String>builder()
.add("--policy", policyPath, "--config", configPath)
.add("--output", outputFile.getAbsolutePath())
.add(extraArgs)
.build();

int exitCode =
new CommandLine(new CelPolicyCompilerTool()).execute(args.toArray(new String[0]));

assertThat(exitCode).isEqualTo(0);
CheckedExpr checkedExpr =
CheckedExpr.parseFrom(Files.toByteArray(outputFile), ExtensionRegistry.getEmptyRegistry());
return CelProtoAbstractSyntaxTree.fromCheckedExpr(checkedExpr).getAst();
}

private static String executeExpectingError(String... args) {
StringWriter out = new StringWriter();
PrintWriter pw = new PrintWriter(out);
Expand Down
18 changes: 12 additions & 6 deletions policy/tools/compile_cel_policy.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ def compile_cel_policy(
optimize_field_selection = False,
simple_variables = False,
iteration_limit = None,
non_eliminable_functions = [],
visibility = None):
"""Compiles a CEL policy into a CheckedExpr binarypb or textpb with optional select optimization.

Expand All @@ -54,6 +55,8 @@ def compile_cel_policy(
`- var_name: expr` instead of `- name: var_name` / `expression: expr`) in the policy (default False)
iteration_limit: (optional) int maximum iteration limit for composing and optimizing the policy
(default None, which uses the compiler default of 1000)
non_eliminable_functions: (optional) list of str function names with side effects to exclude
from common subexpression elimination. All other functions are assumed side-effect free.
visibility: (optional) visibility to use on the genrule macro (default None)
"""
if output_format not in ("binarypb", "textpb", "textproto"):
Expand All @@ -68,11 +71,11 @@ def compile_cel_policy(
args = []
srcs = [policy, config]

args.append("--policy=$(location %s)" % policy)
args.append("--config=$(location %s)" % config)
args.append("--policy=$(execpath %s)" % policy)
args.append("--config=$(execpath %s)" % config)

if base_config != None:
args.append("--base_config=$(location %s)" % base_config)
args.append("--base_config=$(execpath %s)" % base_config)
srcs.append(base_config)

if len(proto_srcs) > 0 and file_descriptor_set != None:
Expand All @@ -87,10 +90,10 @@ def compile_cel_policy(
file_descriptor_set = transitive_descriptor_set_name

if file_descriptor_set != None:
args.append("--file_descriptor_set=$(location %s)" % file_descriptor_set)
args.append("--file_descriptor_set=$(execpath %s)" % file_descriptor_set)
srcs.append(file_descriptor_set)

args.append("--output=$(location %s)" % output)
args.append("--output=$(execpath %s)" % output)
args.append("--output_format=" + output_format)
args.append("--output_version=" + output_version)

Expand All @@ -103,8 +106,11 @@ def compile_cel_policy(
if iteration_limit != None:
args.append("--iteration_limit=%d" % iteration_limit)

if non_eliminable_functions:
args.append("--non_eliminable_functions=" + ",".join(non_eliminable_functions))

cmd = (
"$(location //policy/tools:cel_policy_compiler_tool) " +
"$(execpath //policy/tools:cel_policy_compiler_tool) " +
" ".join(args)
)

Expand Down
Loading