From da61bbdd83297dbe5664afdedd4e31a24e6da697 Mon Sep 17 00:00:00 2001 From: Sokwhan Huh Date: Tue, 13 Aug 2024 11:50:22 -0700 Subject: [PATCH] Parse explanations in match blocks PiperOrigin-RevId: 662598633 --- .../main/java/dev/cel/policy/CelPolicy.java | 7 ++++ .../dev/cel/policy/CelPolicyYamlParser.java | 18 +++++++++ .../cel/policy/CelPolicyYamlParserTest.java | 38 +++++++++++++++++++ .../test/resources/nested_rule/policy.yaml | 2 +- 4 files changed, 64 insertions(+), 1 deletion(-) diff --git a/policy/src/main/java/dev/cel/policy/CelPolicy.java b/policy/src/main/java/dev/cel/policy/CelPolicy.java index 45a2c666..0f2f5d1a 100644 --- a/policy/src/main/java/dev/cel/policy/CelPolicy.java +++ b/policy/src/main/java/dev/cel/policy/CelPolicy.java @@ -159,6 +159,9 @@ public abstract static class Match { public abstract Result result(); + /** Explanation returns the explanation expression, or empty expression if output is not set. */ + public abstract Optional explanation(); + /** Encapsulates the result of this match when condition is met. (either an output or a rule) */ @AutoOneOf(Match.Result.Kind.class) public abstract static class Result { @@ -191,8 +194,12 @@ public abstract static class Builder implements RequiredFieldsChecker { public abstract Builder setResult(Result result); + public abstract Builder setExplanation(ValueString explanation); + abstract Optional result(); + abstract Optional explanation(); + @Override public ImmutableList requiredFields() { return ImmutableList.of(RequiredField.of("output or a rule", this::result)); diff --git a/policy/src/main/java/dev/cel/policy/CelPolicyYamlParser.java b/policy/src/main/java/dev/cel/policy/CelPolicyYamlParser.java index 541dfdfb..1327fbce 100644 --- a/policy/src/main/java/dev/cel/policy/CelPolicyYamlParser.java +++ b/policy/src/main/java/dev/cel/policy/CelPolicyYamlParser.java @@ -202,12 +202,30 @@ public CelPolicy.Match parseMatch( result -> ctx.reportError(tagId, "Only the rule or the output may be set")); matchBuilder.setResult(Match.Result.ofOutput(ctx.newValueString(value))); break; + case "explanation": + matchBuilder + .result() + .filter(result -> result.kind().equals(Match.Result.Kind.RULE)) + .ifPresent( + result -> + ctx.reportError( + tagId, + "Explanation can only be set on output match cases, not nested rules")); + matchBuilder.setExplanation(ctx.newValueString(value)); + break; case "rule": matchBuilder .result() .filter(result -> result.kind().equals(Match.Result.Kind.OUTPUT)) .ifPresent( result -> ctx.reportError(tagId, "Only the rule or the output may be set")); + matchBuilder + .explanation() + .ifPresent( + result -> + ctx.reportError( + result.id(), + "Explanation can only be set on output match cases, not nested rules")); matchBuilder.setResult(Match.Result.ofRule(parseRule(ctx, policyBuilder, value))); break; default: diff --git a/policy/src/test/java/dev/cel/policy/CelPolicyYamlParserTest.java b/policy/src/test/java/dev/cel/policy/CelPolicyYamlParserTest.java index 5bc90cfb..c648d3ac 100644 --- a/policy/src/test/java/dev/cel/policy/CelPolicyYamlParserTest.java +++ b/policy/src/test/java/dev/cel/policy/CelPolicyYamlParserTest.java @@ -17,6 +17,7 @@ import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.assertThrows; +import com.google.common.collect.Iterables; import com.google.testing.junit.testparameterinjector.TestParameter; import com.google.testing.junit.testparameterinjector.TestParameterInjector; import dev.cel.policy.PolicyTestHelper.K8sTagHandler; @@ -42,6 +43,21 @@ public void parseYamlPolicy_success(@TestParameter TestYamlPolicy yamlPolicy) th assertThat(policy.policySource().getDescription()).isEqualTo(description); } + @Test + public void parseYamlPolicy_withExplanation() throws Exception { + String policySource = + "rule:\n" + + " match:\n" + + " - output: 'true'\n" + + " explanation: \"'custom explanation'\""; + + CelPolicy policy = POLICY_PARSER.parse(policySource); + + assertThat(policy.rule().matches()).hasSize(1); + assertThat(Iterables.getOnlyElement(policy.rule().matches()).explanation()) + .hasValue(ValueString.of(11, "'custom explanation'")); + } + @Test public void parseYamlPolicy_errors(@TestParameter PolicyParseErrorTestCase testCase) { CelPolicyValidationException e = @@ -197,6 +213,28 @@ private enum PolicyParseErrorTestCase { "ERROR: :7:7: Only the rule or the output may be set\n" + " | output: \"world\"\n" + " | ......^"), + MATCH_NESTED_RULE_SET_THEN_EXPLANATION( + "rule:\n" + + " match:\n" + + " - condition: \"true\"\n" + + " rule:\n" + + " match:\n" + + " - output: \"hello\"\n" + + " explanation: \"foo\"", + "ERROR: :7:7: Explanation can only be set on output match cases, not nested rules\n" + + " | explanation: \"foo\"\n" + + " | ......^"), + MATCH_EXPLANATION_SET_THEN_NESTED_RULE( + "rule:\n" + + " match:\n" + + " - condition: \"true\"\n" + + " explanation: \"foo\"\n" + + " rule:\n" + + " match:\n" + + " - output: \"hello\"\n", + "ERROR: :4:21: Explanation can only be set on output match cases, not nested rules\n" + + " | explanation: \"foo\"\n" + + " | ....................^"), INVALID_ROOT_NODE_TYPE( "- rule:\n" + " id: a", "ERROR: :1:1: Got yaml node type tag:yaml.org,2002:seq, wanted type(s)" diff --git a/policy/src/test/resources/nested_rule/policy.yaml b/policy/src/test/resources/nested_rule/policy.yaml index bbbfe0fc..2fc566b8 100644 --- a/policy/src/test/resources/nested_rule/policy.yaml +++ b/policy/src/test/resources/nested_rule/policy.yaml @@ -35,4 +35,4 @@ rule: - condition: resource.origin in variables.permitted_regions output: "{'banned': false}" - output: "{'banned': true}" - + explanation: "'resource is in the banned region ' + resource.origin" \ No newline at end of file