Add unreachable checks in policies

PiperOrigin-RevId: 663120461

Author: l46kok

policy/src/main/java/dev/cel/policy/CelCompiledRule.java

@@ -30,7 +30,11 @@
  */
 @AutoValue
 public abstract class CelCompiledRule {
-  public abstract Optional<ValueString> id();
+
+  /** Source metadata identifier associated with the compiled rule. */
+  public abstract long sourceId();
+
+  public abstract Optional<ValueString> ruleId();
 
   public abstract ImmutableList<CelCompiledVariable> variables();
 
@@ -50,7 +54,7 @@ public boolean hasOptionalOutput() {
         return true;
       }
 
-      if (match.isConditionLiteral()) {
+      if (match.isConditionTriviallyTrue()) {
         return false;
       }
 
@@ -83,11 +87,14 @@ static CelCompiledVariable create(
   /** A compiled Match. */
   @AutoValue
   public abstract static class CelCompiledMatch {
+    /** Source metadata identifier associated with the compiled match. */
+    public abstract long sourceId();
+
     public abstract CelAbstractSyntaxTree condition();
 
     public abstract Result result();
 
-    public boolean isConditionLiteral() {
+    public boolean isConditionTriviallyTrue() {
       CelExpr celExpr = condition().getExpr();
       return celExpr.constantOrDefault().getKind().equals(CelConstant.Kind.BOOLEAN_VALUE)
           && celExpr.constant().booleanValue();
@@ -136,16 +143,17 @@ public static OutputValue create(long id, CelAbstractSyntaxTree ast) {
     }
 
     static CelCompiledMatch create(
-        CelAbstractSyntaxTree condition, CelCompiledMatch.Result result) {
-      return new AutoValue_CelCompiledRule_CelCompiledMatch(condition, result);
+        long sourceId, CelAbstractSyntaxTree condition, CelCompiledMatch.Result result) {
+      return new AutoValue_CelCompiledRule_CelCompiledMatch(sourceId, condition, result);
     }
   }
 
   static CelCompiledRule create(
-      Optional<ValueString> id,
+      long sourceId,
+      Optional<ValueString> ruleId,
       ImmutableList<CelCompiledVariable> variables,
       ImmutableList<CelCompiledMatch> matches,
       Cel cel) {
-    return new AutoValue_CelCompiledRule(id, variables, matches, cel);
+    return new AutoValue_CelCompiledRule(sourceId, ruleId, variables, matches, cel);
   }
 }

policy/src/main/java/dev/cel/policy/CelPolicy.java

@@ -45,7 +45,7 @@ public abstract class CelPolicy {
   public static Builder newBuilder() {
     return new AutoValue_CelPolicy.Builder()
         .setName(ValueString.of(0, ""))
-        .setRule(Rule.newBuilder().build())
+        .setRule(Rule.newBuilder(0).build())
         .setMetadata(ImmutableMap.of());
   }
 
@@ -86,8 +86,9 @@ public Builder putMetadata(Map<String, Object> map) {
    */
   @AutoValue
   public abstract static class Rule {
+    public abstract long id();
 
-    public abstract Optional<ValueString> id();
+    public abstract Optional<ValueString> ruleId();
 
     public abstract Optional<ValueString> description();
 
@@ -96,8 +97,9 @@ public abstract static class Rule {
     public abstract ImmutableSet<Match> matches();
 
     /** Builder for {@link Rule}. */
-    public static Builder newBuilder() {
+    public static Builder newBuilder(long id) {
       return new AutoValue_CelPolicy_Rule.Builder()
+          .setId(id)
           .setVariables(ImmutableSet.of())
           .setMatches(ImmutableSet.of());
     }
@@ -106,7 +108,7 @@ public static Builder newBuilder() {
     @AutoValue.Builder
     public abstract static class Builder {
 
-      public abstract Rule.Builder setId(ValueString id);
+      public abstract Rule.Builder setRuleId(ValueString id);
 
       public abstract Rule.Builder setDescription(ValueString description);
 
@@ -118,6 +120,8 @@ public abstract static class Builder {
 
       abstract ImmutableSet.Builder<Match> matchesBuilder();
 
+      abstract Builder setId(long value);
+
       @CanIgnoreReturnValue
       public Builder addVariables(Variable... variables) {
         return addVariables(Arrays.asList(variables));
@@ -159,6 +163,8 @@ public abstract static class Match {
 
     public abstract Result result();
 
+    public abstract long id();
+
     /** Explanation returns the explanation expression, or empty expression if output is not set. */
     public abstract Optional<ValueString> explanation();
 
@@ -189,13 +195,16 @@ public enum Kind {
     /** Builder for {@link Match}. */
     @AutoValue.Builder
     public abstract static class Builder implements RequiredFieldsChecker {
+      public abstract Builder setId(long value);
 
       public abstract Builder setCondition(ValueString condition);
 
       public abstract Builder setResult(Result result);
 
       public abstract Builder setExplanation(ValueString explanation);
 
+      abstract Optional<Long> id();
+
       abstract Optional<Result> result();
 
       abstract Optional<ValueString> explanation();
@@ -209,8 +218,8 @@ public ImmutableList<RequiredField> requiredFields() {
     }
 
     /** Creates a new builder to construct a {@link Match} instance. */
-    public static Builder newBuilder() {
-      return new AutoValue_CelPolicy_Match.Builder();
+    public static Builder newBuilder(long id) {
+      return new AutoValue_CelPolicy_Match.Builder().setId(id);
     }
   }
 

policy/src/main/java/dev/cel/policy/CelPolicyCompilerImpl.java

@@ -35,6 +35,7 @@
 import dev.cel.optimizer.CelOptimizerFactory;
 import dev.cel.policy.CelCompiledRule.CelCompiledMatch;
 import dev.cel.policy.CelCompiledRule.CelCompiledMatch.Result;
+import dev.cel.policy.CelCompiledRule.CelCompiledMatch.Result.Kind;
 import dev.cel.policy.CelCompiledRule.CelCompiledVariable;
 import dev.cel.policy.CelPolicy.Match;
 import dev.cel.policy.CelPolicy.Variable;
@@ -167,10 +168,39 @@ private CelCompiledRule compileRuleImpl(
           throw new IllegalArgumentException("Unexpected kind: " + match.result().kind());
       }
 
-      matchBuilder.add(CelCompiledMatch.create(conditionAst, matchResult));
+      matchBuilder.add(CelCompiledMatch.create(match.id(), conditionAst, matchResult));
     }
 
-    return CelCompiledRule.create(rule.id(), variableBuilder.build(), matchBuilder.build(), cel);
+    CelCompiledRule compiledRule =
+        CelCompiledRule.create(
+            rule.id(), rule.ruleId(), variableBuilder.build(), matchBuilder.build(), cel);
+
+    // Validate that all branches in the policy are reachable
+    checkUnreachableCode(compiledRule, compilerContext);
+
+    return compiledRule;
+  }
+
+  private void checkUnreachableCode(CelCompiledRule compiledRule, CompilerContext compilerContext) {
+    boolean ruleHasOptional = compiledRule.hasOptionalOutput();
+    ImmutableList<CelCompiledMatch> compiledMatches = compiledRule.matches();
+    int matchCount = compiledMatches.size();
+    for (int i = matchCount - 1; i >= 0; i--) {
+      CelCompiledMatch compiledMatch = compiledMatches.get(i);
+      boolean isTriviallyTrue = compiledMatch.isConditionTriviallyTrue();
+
+      if (isTriviallyTrue && !ruleHasOptional && i != matchCount - 1) {
+        if (compiledMatch.result().kind().equals(Kind.OUTPUT)) {
+          compilerContext.addIssue(
+              compiledMatch.sourceId(),
+              CelIssue.formatError(1, 0, "Match creates unreachable outputs"));
+        } else {
+          compilerContext.addIssue(
+              compiledMatch.result().rule().sourceId(),
+              CelIssue.formatError(1, 0, "Rule creates unreachable outputs"));
+        }
+      }
+    }
   }
 
   private static CelAbstractSyntaxTree newErrorAst() {

policy/src/main/java/dev/cel/policy/CelPolicyYamlParser.java

@@ -40,7 +40,7 @@ final class CelPolicyYamlParser implements CelPolicyParser {
   // Sentinel values for parsing errors
   private static final ValueString ERROR_VALUE = ValueString.newBuilder().setValue(ERROR).build();
   private static final Match ERROR_MATCH =
-      Match.newBuilder().setCondition(ERROR_VALUE).setResult(Result.ofOutput(ERROR_VALUE)).build();
+      Match.newBuilder(0).setCondition(ERROR_VALUE).setResult(Result.ofOutput(ERROR_VALUE)).build();
   private static final Variable ERROR_VARIABLE =
       Variable.newBuilder().setExpression(ERROR_VALUE).setName(ERROR_VALUE).build();
 
@@ -122,7 +122,7 @@ public CelPolicy parsePolicy(PolicyParserContext<Node> ctx, Node node) {
     public CelPolicy.Rule parseRule(
         PolicyParserContext<Node> ctx, CelPolicy.Builder policyBuilder, Node node) {
       long valueId = ctx.collectMetadata(node);
-      CelPolicy.Rule.Builder ruleBuilder = CelPolicy.Rule.newBuilder();
+      CelPolicy.Rule.Builder ruleBuilder = CelPolicy.Rule.newBuilder(valueId);
       if (!assertYamlType(ctx, valueId, node, YamlNodeType.MAP)) {
         return ruleBuilder.build();
       }
@@ -137,7 +137,7 @@ public CelPolicy.Rule parseRule(
         Node value = nodeTuple.getValueNode();
         switch (fieldName) {
           case "id":
-            ruleBuilder.setId(ctx.newValueString(value));
+            ruleBuilder.setRuleId(ctx.newValueString(value));
             break;
           case "description":
             ruleBuilder.setDescription(ctx.newValueString(value));
@@ -181,7 +181,7 @@ public CelPolicy.Match parseMatch(
       }
       MappingNode matchNode = (MappingNode) node;
       CelPolicy.Match.Builder matchBuilder =
-          CelPolicy.Match.newBuilder().setCondition(ValueString.of(ctx.nextId(), "true"));
+          CelPolicy.Match.newBuilder(nodeId).setCondition(ValueString.of(ctx.nextId(), "true"));
       for (NodeTuple nodeTuple : matchNode.getValue()) {
         Node key = nodeTuple.getKeyNode();
         long tagId = ctx.collectMetadata(key);

policy/src/main/java/dev/cel/policy/RuleComposer.java

@@ -77,7 +77,7 @@ private RuleOptimizationResult optimizeRule(Cel cel, CelCompiledRule compiledRul
       // If the condition is trivially true, none of the matches in the rule causes the result
       // to become optional, and the rule is not the last match, then this will introduce
       // unreachable outputs or rules.
-      boolean isTriviallyTrue = match.isConditionLiteral();
+      boolean isTriviallyTrue = match.isConditionTriviallyTrue();
 
       switch (match.result().kind()) {
         // For the match's output, determine whether the output should be wrapped
@@ -144,7 +144,7 @@ private RuleOptimizationResult optimizeRule(Cel cel, CelCompiledRule compiledRul
               matchAst,
               String.format(
                   "failed composing the subrule '%s' due to conflicting output types.",
-                  matchNestedRule.id().map(ValueString::value).orElse("")),
+                  matchNestedRule.ruleId().map(ValueString::value).orElse("")),
               lastOutputId);
           break;
       }

policy/src/test/java/dev/cel/policy/CelPolicyCompilerImplTest.java

@@ -308,7 +308,8 @@ private enum MultilineErrorTest {
   private enum TestErrorYamlPolicy {
     COMPILE_ERRORS("compile_errors"),
     COMPOSE_ERRORS_CONFLICTING_OUTPUT("compose_errors_conflicting_output"),
-    COMPOSE_ERRORS_CONFLICTING_SUBRULE("compose_errors_conflicting_subrule");
+    COMPOSE_ERRORS_CONFLICTING_SUBRULE("compose_errors_conflicting_subrule"),
+    ERRORS_UNREACHABLE("errors_unreachable");
 
     private final String name;
     private final String policyFilePath;

policy/src/test/resources/errors_unreachable/config.yaml

@@ -0,0 +1,54 @@
+# Copyright 2024 Google LLC
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#    https://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+name: "errors_unreachable"
+extensions:
+- name: "sets"
+- name: "strings"
+  version: "latest"
+variables:
+- name: "destination.ip"
+  type:
+    type_name: "string"
+- name: "origin.ip"
+  type:
+    type_name: "string"
+- name: "spec.restricted_destinations"
+  type:
+    type_name: "list"
+    params:
+    - type_name: "string"
+- name: "spec.origin"
+  type:
+    type_name: "string"
+- name: "request"
+  type:
+    type_name: "map"
+    params:
+    - type_name: "string"
+    - type_name: "dyn"
+- name: "resource"
+  type:
+    type_name: "map"
+    params:
+    - type_name: "string"
+    - type_name: "dyn"
+functions:
+- name: "locationCode"
+  overloads:
+  - id: "locationCode_string"
+    args:
+    - type_name: "string"
+    return:
+      type_name: "string"

policy/src/test/resources/errors_unreachable/expected_errors.baseline

@@ -0,0 +1,6 @@
+ERROR: errors_unreachable/policy.yaml:36:9: Match creates unreachable outputs
+ |       - output: |
+ | ........^
+ERROR: errors_unreachable/policy.yaml:28:7: Rule creates unreachable outputs
+ |       match:
+ | ......^

policy/src/test/resources/errors_unreachable/policy.yaml

@@ -0,0 +1,39 @@
+# Copyright 2024 Google LLC
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#    https://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+name: "errors_unreachable"
+rule:
+  variables:
+  - name: want
+    expression: request.labels
+  - name: missing
+    expression: variables.want.filter(l, !(l in resource.labels))
+  - name: invalid
+    expression: >
+      resource.labels.filter(l,
+        l in variables.want && variables.want[l] != resource.labels[l])
+  match:
+  - rule:
+      match:
+      - output: "''"
+  - condition: variables.missing.size() > 0
+    output: |
+      "missing one or more required labels: [\"" + variables.missing.join(',') + "\"]"
+  - condition: variables.invalid.size() > 0
+    rule:
+      match:
+      - output: |
+          "invalid values provided on one or more labels: [\"" + variables.invalid.join(',') + "\"]"
+      - condition: "false"
+        output: "'unreachable'"