Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Policy nested rule fix #1092

Merged
merged 4 commits into from
Dec 12, 2024
Merged
Changes from 2 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
3 changes: 2 additions & 1 deletion policy/compiler_test.go
Original file line number Diff line number Diff line change
@@ -184,9 +184,10 @@ func compile(t testing.TB, name string, parseOpts []ParserOption, envOpts []cel.
}

func (r *runner) setup(t testing.TB) {
t.Helper()
env, ast, iss := compile(t, r.name, r.parseOpts, r.envOpts, r.compilerOpts)
if iss.Err() != nil {
t.Fatalf("Compile() failed: %v", iss.Err())
t.Fatalf("Compile(%s) failed: %v", r.name, iss.Err())
}
pExpr, err := cel.AstToString(ast)
if err != nil {
237 changes: 197 additions & 40 deletions policy/composer.go
Original file line number Diff line number Diff line change
@@ -88,42 +88,29 @@ func (opt *ruleComposerImpl) Optimize(ctx *cel.OptimizerContext, a *ast.AST) *as
}

func (opt *ruleComposerImpl) optimizeRule(ctx *cel.OptimizerContext, r *CompiledRule) ast.Expr {
matchExpr := ctx.NewCall("optional.none")
matches := r.Matches()
matchCount := len(matches)
// Visitor to rewrite variables-prefixed identifiers with index names.
vars := r.Variables()
for _, v := range vars {
opt.registerVariable(ctx, v)
}

optionalResult := true
matches := r.Matches()
matchCount := len(matches)
var output outputStep = nil
if r.HasOptionalOutput() {
output = newOptionalOutputStep(ctx, ctx.NewLiteral(types.True), ctx.NewCall("optional.none"))
}
// Build the rule subgraph.
for i := matchCount - 1; i >= 0; i-- {
m := matches[i]
cond := ctx.CopyASTAndMetadata(m.Condition().NativeRep())
// If the condition is trivially true, not 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.
triviallyTrue := m.ConditionIsLiteral(types.True)

// If the output is non-nil, then determine whether the output should be wrapped
// into an optional value, a conditional, or both.
TristonianJones marked this conversation as resolved.
Show resolved Hide resolved
if m.Output() != nil {
out := ctx.CopyASTAndMetadata(m.Output().Expr().NativeRep())
if triviallyTrue {
matchExpr = out
optionalResult = false
continue
}
if optionalResult {
out = ctx.NewCall("optional.of", out)
}
matchExpr = ctx.NewCall(
operators.Conditional,
cond,
out,
matchExpr)
step := newNonOptionalOutputStep(ctx, cond, out)
output = step.combine(output)
continue
}

@@ -132,29 +119,16 @@ func (opt *ruleComposerImpl) optimizeRule(ctx *cel.OptimizerContext, r *Compiled
child := m.NestedRule()
nestedRule := opt.optimizeRule(ctx, child)
nestedHasOptional := child.HasOptionalOutput()
if optionalResult && !nestedHasOptional {
nestedRule = ctx.NewCall("optional.of", nestedRule)
}
if !optionalResult && nestedHasOptional {
matchExpr = ctx.NewCall("optional.of", matchExpr)
optionalResult = true
}
// If either the nested rule or current condition output are optional then
// use optional.or() to specify the combination of the first and second results
// Note, the argument order is reversed due to the traversal of matches in
// reverse order.
if optionalResult && triviallyTrue {
matchExpr = ctx.NewMemberCall("or", nestedRule, matchExpr)
if nestedHasOptional {
step := newOptionalOutputStep(ctx, cond, nestedRule)
output = step.combine(output)
continue
}
matchExpr = ctx.NewCall(
operators.Conditional,
cond,
nestedRule,
matchExpr,
)
step := newNonOptionalOutputStep(ctx, cond, nestedRule)
output = step.combine(output)
}

matchExpr := output.expr()
identVisitor := opt.rewriteVariableName(ctx)
ast.PostOrderVisit(matchExpr, identVisitor)

@@ -177,6 +151,8 @@ func (opt *ruleComposerImpl) rewriteVariableName(ctx *cel.OptimizerContext) ast.
})
}

// registerVariable creates an entry for a variable name within the cel.@block used to enumerate
// variables within composed policy expression.
func (opt *ruleComposerImpl) registerVariable(ctx *cel.OptimizerContext, v *CompiledVariable) {
varName := fmt.Sprintf("variables.%s", v.Name())
indexVar := fmt.Sprintf("@index%d", opt.nextVarIndex)
@@ -192,6 +168,187 @@ func (opt *ruleComposerImpl) registerVariable(ctx *cel.OptimizerContext, v *Comp
opt.nextVarIndex++
}

// sortedVariables returns the variables ordered by their declaration index.
func (opt *ruleComposerImpl) sortedVariables() []varIndex {
return opt.varIndices
}

// outputStep interface represents a policy output expression.
type outputStep interface {
TristonianJones marked this conversation as resolved.
Show resolved Hide resolved
TristonianJones marked this conversation as resolved.
Show resolved Hide resolved
// isOptional indicates whether the output step has an optional result.
//
// Individual conditional attributes are not optional; however, rules and subrules can have optional output.
isOptional() bool

// condition returns the condition associated with the output.
condition() ast.Expr

// isConditional returns true if the condition expression is not trivially true.
isConditional() bool

// expr returns the output expression for the step.
expr() ast.Expr

// combine assembles two output expressions into a single output step.
combine(other outputStep) outputStep
}

// baseOutputStep encapsulates the common features of an outputStep implementation.
type baseOutputStep struct {
ctx *cel.OptimizerContext
cond ast.Expr
out ast.Expr
}

func (b baseOutputStep) condition() ast.Expr {
return b.cond
}

func (b baseOutputStep) isConditional() bool {
c := b.cond
return c.Kind() != ast.LiteralKind || c.AsLiteral() != types.True
}

func (b baseOutputStep) expr() ast.Expr {
return b.out
}

// newNonOptionalOutputStep returns an output step whose output is not optional.
func newNonOptionalOutputStep(ctx *cel.OptimizerContext, cond, out ast.Expr) nonOptionalOutputStep {
return nonOptionalOutputStep{
baseOutputStep: &baseOutputStep{
ctx: ctx,
cond: cond,
out: out,
},
}
}

type nonOptionalOutputStep struct {
*baseOutputStep
}

// isOptional returns false
func (nonOptionalOutputStep) isOptional() bool {
return false
}

// combine assembles a new outputStep from the target output step an an input output step.
//
// non-optional.combine(non-optional) // non-optional
// (non-optional && conditional).combine(optional) // optional
// (non-optional && unconditional).combine(optional) // non-optional
//
// The last combination case is unusual, but effectively it means that the non-optional value prunes away
// the potential optional output.
func (s nonOptionalOutputStep) combine(step outputStep) outputStep {
if step == nil {
// The input `step`` may be nil if this is the first outputStep
TristonianJones marked this conversation as resolved.
Show resolved Hide resolved
return s
}
ctx := s.ctx
trueCondition := ctx.NewLiteral(types.True)
if step.isOptional() {
// If the step is optional, convert the non-optional value to an optional one and return a ternary
if s.isConditional() {
return newOptionalOutputStep(ctx,
trueCondition,
ctx.NewCall(operators.Conditional,
s.condition(),
ctx.NewCall("optional.of", s.expr()),
step.expr()),
)
}
// The `step` is pruned away by a unconditional non-optional step `s`.
return s
}
return newNonOptionalOutputStep(ctx,
trueCondition,
ctx.NewCall(operators.Conditional,
s.condition(),
s.expr(),
step.expr()))
}

// newOptionalOutputStep returns an output step with an optional policy output.
func newOptionalOutputStep(ctx *cel.OptimizerContext, cond, out ast.Expr) optionalOutputStep {
return optionalOutputStep{
baseOutputStep: &baseOutputStep{
ctx: ctx,
cond: cond,
out: out,
},
}
}

type optionalOutputStep struct {
*baseOutputStep
}

// isOptional returns true.
func (optionalOutputStep) isOptional() bool {
return true
}

// combine assembles a new outputStep from the target output step an an input output step.
//
// optional.combine(optional) // optional
// (optional && conditional).combine(non-optional) // optional
// (optional && unconditional).combine(non-optional) // non-optional
//
// The last combination case indicates that an optional value in one case should be resolved
// to a non-optional value as
func (s optionalOutputStep) combine(step outputStep) outputStep {
if step == nil {
// This is likely unreachable for an optional step, but worth adding as a safeguard
return s
}
ctx := s.ctx
trueCondition := ctx.NewLiteral(types.True)
if step.isOptional() {
// Introduce a ternary to capture the conditional return when combining a
// conditional optional with another optional.
if s.isConditional() {
return newOptionalOutputStep(ctx,
trueCondition,
ctx.NewCall(operators.Conditional,
s.condition(),
s.expr(),
step.expr()),
)
}
// When an optional is unconditionally combined with another optional, rely
// on the optional 'or' to fall-through from one optional to another.
if !isOptionalNone(step.expr()) {
return newOptionalOutputStep(ctx,
trueCondition,
ctx.NewMemberCall("or", s.expr(), step.expr()))
}
// Otherwise, the current step 's' is unconditional and effectively prunes away
// the other input 'step'.
return s
}
if s.isConditional() {
// Introduce a ternary to capture the conditional return while wrapping the
// non-optional result from a lower step into an optional value.
return newOptionalOutputStep(ctx,
trueCondition,
ctx.NewCall(operators.Conditional,
s.condition(),
s.expr(),
ctx.NewCall("optional.of", step.expr())))
}
// If the current step is unconditional and the step is non-optional, attempt
// to convert to the optional step 's' to a non-optional value using `orValue`
// with the 'step' expression value.
return newNonOptionalOutputStep(ctx,
trueCondition,
ctx.NewMemberCall("orValue", s.expr(), step.expr()),
)
}

func isOptionalNone(e ast.Expr) bool {
return e.Kind() == ast.CallKind &&
e.AsCall().FunctionName() == "optional.none" &&
len(e.AsCall().Args()) == 0
}
32 changes: 29 additions & 3 deletions policy/helper_test.go
Original file line number Diff line number Diff line change
@@ -57,9 +57,8 @@ var (
["us", "uk", "es"],
{"us": false, "ru": false, "ir": false}],
((resource.origin in @index1 && !(resource.origin in @index0))
? optional.of({"banned": true}) : optional.none()).or(
optional.of((resource.origin in @index0)
? {"banned": false} : {"banned": true})))`,
? optional.of({"banned": true}) : optional.none()).orValue(
(resource.origin in @index0) ? {"banned": false} : {"banned": true}))`,
},
{
name: "nested_rule2",
@@ -86,6 +85,33 @@ var (
: (!(resource.origin in @index0)
? optional.of({"banned": "unconfigured_region"}) : optional.none()))`,
},
{
name: "nested_rule4",
expr: `(x > 0) ? true : false`,
},
{
name: "nested_rule5",
expr: `
(x > 0)
? ((x > 2) ? optional.of(true) : optional.none())
: ((x > 1)
? ((x >= 2) ? optional.of(true) : optional.none())
: optional.of(false))`,
},
{
name: "nested_rule6",
expr: `
((x > 2) ? optional.of(true) : optional.none())
.orValue(((x > 3) ? optional.of(true) : optional.none())
.orValue(false))`,
},
{
name: "nested_rule7",
expr: `
((x > 2) ? optional.of(true) : optional.none())
.or(((x > 3) ? optional.of(true) : optional.none())
.or((x > 1) ? optional.of(false) : optional.none()))`,
},
{
name: "context_pb",
expr: `
19 changes: 19 additions & 0 deletions policy/testdata/nested_rule4/config.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# 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: "nested_rule4"
variables:
- name: x
type:
type_name: int
24 changes: 24 additions & 0 deletions policy/testdata/nested_rule4/policy.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# 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: nested_rule4
rule:
match:
- condition: x > 0
rule:
match:
- rule:
match:
- output: "true"
- output: "false"
Loading