From 3644294aff207b77d30f1cd3517132c29e9f2527 Mon Sep 17 00:00:00 2001 From: TristonianJones Date: Mon, 10 Jun 2024 11:13:03 -0700 Subject: [PATCH 1/2] Introduce protobuf message testing to policies --- common/decls/decls.go | 4 +- common/decls/decls_test.go | 21 +++++++ policy/compiler_test.go | 50 ++++++++++------ policy/config.go | 3 +- policy/conformance.go | 11 +++- policy/helper_test.go | 10 ++++ policy/testdata/k8s/tests.yaml | 13 ++-- policy/testdata/nested_rule/tests.yaml | 9 ++- policy/testdata/pb/config.yaml | 23 +++++++ policy/testdata/pb/policy.yaml | 20 +++++++ policy/testdata/pb/tests.yaml | 34 +++++++++++ policy/testdata/required_labels/tests.yaml | 60 +++++++++++-------- .../restricted_destinations/tests.yaml | 22 ++++--- 13 files changed, 215 insertions(+), 65 deletions(-) create mode 100644 policy/testdata/pb/config.yaml create mode 100644 policy/testdata/pb/policy.yaml create mode 100644 policy/testdata/pb/tests.yaml diff --git a/common/decls/decls.go b/common/decls/decls.go index 734ebe57..2db1a027 100644 --- a/common/decls/decls.go +++ b/common/decls/decls.go @@ -162,7 +162,9 @@ func (f *FunctionDecl) AddOverload(overload *OverloadDecl) error { if oID == overload.ID() { if o.SignatureEquals(overload) && o.IsNonStrict() == overload.IsNonStrict() { // Allow redefinition of an overload implementation so long as the signatures match. - f.overloads[oID] = overload + if !o.hasBinding() || overload.hasBinding() { + f.overloads[oID] = overload + } return nil } return fmt.Errorf("overload redefinition in function. %s: %s has multiple definitions", f.Name(), oID) diff --git a/common/decls/decls_test.go b/common/decls/decls_test.go index 4a017025..eb8e6f70 100644 --- a/common/decls/decls_test.go +++ b/common/decls/decls_test.go @@ -413,6 +413,27 @@ func TestFunctionAddDuplicateOverloads(t *testing.T) { } } +func TestFunctionAddDuplicateOverloadsPreservesBinding(t *testing.T) { + f, err := NewFunction("max", + Overload("max_int", []*types.Type{types.IntType}, types.IntType), + Overload("max_int", []*types.Type{types.IntType}, types.IntType, + UnaryBinding(func(v ref.Val) ref.Val { + return v + })), + Overload("max_int", []*types.Type{types.IntType}, types.IntType), + ) + if err != nil { + t.Fatalf("NewFunction() with duplicate overload signature failed: %v", err) + } + if len(f.overloads) != 1 { + t.Fatal("Duplicate overloads were not merged") + } + o := f.overloads["max_int"] + if o.unaryOp == nil { + t.Error("Duplicate overloads purged the overload binding") + } +} + func TestFunctionAddCollidingOverloads(t *testing.T) { _, err := NewFunction("max", Overload("max_int", []*types.Type{types.IntType}, types.IntType), diff --git a/policy/compiler_test.go b/policy/compiler_test.go index d03c3e6f..59ad0375 100644 --- a/policy/compiler_test.go +++ b/policy/compiler_test.go @@ -21,6 +21,7 @@ import ( "github.com/google/cel-go/cel" "github.com/google/cel-go/common/types" + "github.com/google/cel-go/common/types/ref" ) func TestCompile(t *testing.T) { @@ -89,6 +90,11 @@ func compile(t testing.TB, name string, parseOpts []ParserOption, envOpts []cel. if err != nil { t.Fatalf("cel.NewEnv() failed: %v", err) } + // Configure any custom environment options. + env, err = env.Extend(envOpts...) + if err != nil { + t.Fatalf("env.Extend() with env options %v, failed: %v", config, err) + } // Configure declarations configOpts, err := config.AsEnvOptions(env) if err != nil { @@ -98,11 +104,6 @@ func compile(t testing.TB, name string, parseOpts []ParserOption, envOpts []cel. if err != nil { t.Fatalf("env.Extend() with config options %v, failed: %v", config, err) } - // Configure any implementations - env, err = env.Extend(envOpts...) - if err != nil { - t.Fatalf("env.Extend() with config options %v, failed: %v", config, err) - } ast, iss := Compile(env, policy) return env, ast, iss } @@ -134,22 +135,19 @@ func (r *runner) run(t *testing.T) { for _, tst := range s.Tests { tc := tst t.Run(fmt.Sprintf("%s/%s/%s", r.name, section, tc.Name), func(t *testing.T) { - out, _, err := r.prg.Eval(tc.Input) - if err != nil { - t.Fatalf("prg.Eval(tc.Input) failed: %v", err) - } - wantExpr, iss := r.env.Compile(tc.Output) - if iss.Err() != nil { - t.Fatalf("env.Compile(%q) failed :%v", tc.Output, iss.Err()) - } - testPrg, err := r.env.Program(wantExpr) - if err != nil { - t.Fatalf("env.Program(wantExpr) failed: %v", err) + input := map[string]any{} + for k, v := range tc.Input { + if v.Expr == "" { + input[k] = v.Value + continue + } + input[k] = r.eval(t, v.Expr) } - testOut, _, err := testPrg.Eval(cel.NoVars()) + out, _, err := r.prg.Eval(input) if err != nil { - t.Fatalf("testPrg.Eval() failed: %v", err) + t.Fatalf("prg.Eval(input) failed: %v", err) } + testOut := r.eval(t, tc.Output) if optOut, ok := out.(*types.Optional); ok { if optOut.Equal(types.OptionalNone) == types.True { if testOut.Equal(types.OptionalNone) != types.True { @@ -182,6 +180,22 @@ func (r *runner) bench(b *testing.B) { } } +func (r *runner) eval(t testing.TB, expr string) ref.Val { + wantExpr, iss := r.env.Compile(expr) + if iss.Err() != nil { + t.Fatalf("env.Compile(%q) failed :%v", expr, iss.Err()) + } + prg, err := r.env.Program(wantExpr) + if err != nil { + t.Fatalf("env.Program(wantExpr) failed: %v", err) + } + out, _, err := prg.Eval(cel.NoVars()) + if err != nil { + t.Fatalf("prg.Eval() failed: %v", err) + } + return out +} + func normalize(s string) string { return strings.ReplaceAll( strings.ReplaceAll( diff --git a/policy/config.go b/policy/config.go index c3397189..51a36e82 100644 --- a/policy/config.go +++ b/policy/config.go @@ -151,7 +151,8 @@ func (td *TypeDecl) AsCELType(baseEnv *cel.Env) (*cel.Type, error) { return cel.TypeParamType(td.TypeName), nil } if msgType, found := baseEnv.CELTypeProvider().FindStructType(td.TypeName); found { - return msgType, nil + // First parameter is the type name. + return msgType.Parameters()[0], nil } t, found := baseEnv.CELTypeProvider().FindIdent(td.TypeName) if !found { diff --git a/policy/conformance.go b/policy/conformance.go index e47b6160..0d1df2ec 100644 --- a/policy/conformance.go +++ b/policy/conformance.go @@ -31,7 +31,12 @@ type TestSection struct { // Note, when a test requires additional functions to be provided to execute, the test harness // must supply these functions. type TestCase struct { - Name string `yaml:"name"` - Input map[string]any `yaml:"input"` - Output string `yaml:"output"` + Name string `yaml:"name"` + Input map[string]TestInput `yaml:"input"` + Output string `yaml:"output"` +} + +type TestInput struct { + Value any `yaml:"value"` + Expr string `yaml:"expr"` } diff --git a/policy/helper_test.go b/policy/helper_test.go index 1683057a..a929b3af 100644 --- a/policy/helper_test.go +++ b/policy/helper_test.go @@ -22,6 +22,7 @@ import ( "github.com/google/cel-go/cel" "github.com/google/cel-go/common/types" "github.com/google/cel-go/common/types/ref" + "github.com/google/cel-go/test/proto3pb" "gopkg.in/yaml.v3" ) @@ -58,6 +59,15 @@ var ( optional.of((resource.origin in variables.permitted_regions) ? {"banned": false} : {"banned": true})))`, }, + { + name: "pb", + expr: `(spec.single_int32 > 10) + ? optional.of("invalid spec, got single_int32=%d, wanted <= 10".format([spec.single_int32])) + : optional.none()`, + envOpts: []cel.EnvOption{ + cel.Types(&proto3pb.TestAllTypes{}), + }, + }, { name: "required_labels", expr: ` diff --git a/policy/testdata/k8s/tests.yaml b/policy/testdata/k8s/tests.yaml index d44c90ee..9e1d8b56 100644 --- a/policy/testdata/k8s/tests.yaml +++ b/policy/testdata/k8s/tests.yaml @@ -18,11 +18,14 @@ section: tests: - name: "restricted_container" input: - resource.namespace: "dev.cel" + resource.namespace: + value: "dev.cel" resource.labels: - environment: "staging" + value: + environment: "staging" resource.containers: - - staging.dev.cel.container1 - - staging.dev.cel.container2 - - preprod.dev.cel.container3 + value: + - staging.dev.cel.container1 + - staging.dev.cel.container2 + - preprod.dev.cel.container3 output: "'only staging containers are allowed in namespace dev.cel'" diff --git a/policy/testdata/nested_rule/tests.yaml b/policy/testdata/nested_rule/tests.yaml index 3e9d7a95..48101c89 100644 --- a/policy/testdata/nested_rule/tests.yaml +++ b/policy/testdata/nested_rule/tests.yaml @@ -19,17 +19,20 @@ section: - name: "restricted_origin" input: resource: - origin: "ir" + value: + origin: "ir" output: "{'banned': true}" - name: "by_default" input: resource: - origin: "de" + value: + origin: "de" output: "{'banned': true}" - name: "permitted" tests: - name: "valid_origin" input: resource: - origin: "uk" + value: + origin: "uk" output: "{'banned': false}" diff --git a/policy/testdata/pb/config.yaml b/policy/testdata/pb/config.yaml new file mode 100644 index 00000000..bd554694 --- /dev/null +++ b/policy/testdata/pb/config.yaml @@ -0,0 +1,23 @@ +# 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: "pb" +container: "google.expr.proto3.test" +extensions: + - name: "strings" + version: 2 +variables: + - name: "spec" + type: + type_name: "google.expr.proto3.test.TestAllTypes" diff --git a/policy/testdata/pb/policy.yaml b/policy/testdata/pb/policy.yaml new file mode 100644 index 00000000..8a2723b5 --- /dev/null +++ b/policy/testdata/pb/policy.yaml @@ -0,0 +1,20 @@ +# 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: "pb" +rule: + match: + - condition: spec.single_int32 > 10 + output: | + "invalid spec, got single_int32=%d, wanted <= 10".format([spec.single_int32]) diff --git a/policy/testdata/pb/tests.yaml b/policy/testdata/pb/tests.yaml new file mode 100644 index 00000000..c06c19b1 --- /dev/null +++ b/policy/testdata/pb/tests.yaml @@ -0,0 +1,34 @@ +# 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. + +description: "Protobuf input tests" +section: + - name: "valid" + tests: + - name: "good spec" + input: + spec: + expr: > + TestAllTypes{single_int32: 10} + output: "optional.none()" + - name: "invalid" + tests: + - name: "bad spec" + input: + spec: + expr: > + TestAllTypes{single_int32: 11} + output: > + "invalid spec, got single_int32=11, wanted <= 10" + \ No newline at end of file diff --git a/policy/testdata/required_labels/tests.yaml b/policy/testdata/required_labels/tests.yaml index 2a1e8ec5..a4bf96dc 100644 --- a/policy/testdata/required_labels/tests.yaml +++ b/policy/testdata/required_labels/tests.yaml @@ -19,39 +19,45 @@ section: - name: "matching" input: spec: - labels: - env: prod - experiment: "group b" + value: + labels: + env: prod + experiment: "group b" resource: - labels: - env: prod - experiment: "group b" - release: "v0.1.0" + value: + labels: + env: prod + experiment: "group b" + release: "v0.1.0" output: "optional.none()" - name: "missing" tests: - name: "env" input: spec: - labels: - env: prod - experiment: "group b" + value: + labels: + env: prod + experiment: "group b" resource: - labels: - experiment: "group b" - release: "v0.1.0" + value: + labels: + experiment: "group b" + release: "v0.1.0" output: > "missing one or more required labels: [\"env\"]" - name: "experiment" input: spec: - labels: - env: prod - experiment: "group b" + value: + labels: + env: prod + experiment: "group b" resource: - labels: - env: staging - release: "v0.1.0" + value: + labels: + env: staging + release: "v0.1.0" output: > "missing one or more required labels: [\"experiment\"]" - name: "invalid" @@ -59,13 +65,15 @@ section: - name: "env" input: spec: - labels: - env: prod - experiment: "group b" + value: + labels: + env: prod + experiment: "group b" resource: - labels: - env: staging - experiment: "group b" - release: "v0.1.0" + value: + labels: + env: staging + experiment: "group b" + release: "v0.1.0" output: > "invalid values provided on one or more labels: [\"env\"]" diff --git a/policy/testdata/restricted_destinations/tests.yaml b/policy/testdata/restricted_destinations/tests.yaml index 1702c295..70183f7a 100644 --- a/policy/testdata/restricted_destinations/tests.yaml +++ b/policy/testdata/restricted_destinations/tests.yaml @@ -18,20 +18,26 @@ section: tests: - name: "allowed" input: - "spec.origin": "us" + "spec.origin": + value: "us" "spec.restricted_destinations": + value: - "cu" - "ir" - "kp" - "sd" - "sy" - "destination.ip": "10.0.0.1" - "origin.ip": "10.0.0.1" + "destination.ip": + value: "10.0.0.1" + "origin.ip": + value: "10.0.0.1" request: - auth: - claims: {} + value: + auth: + claims: {} resource: - name: "/company/acme/secrets/doomsday-device" - labels: - location: "us" + value: + name: "/company/acme/secrets/doomsday-device" + labels: + location: "us" output: "true" From b3c83869d33d4a8498b10e5a68d25d2ef4f28039 Mon Sep 17 00:00:00 2001 From: TristonianJones Date: Mon, 10 Jun 2024 12:47:01 -0700 Subject: [PATCH 2/2] Remove unnecessary conditional branch --- common/decls/decls.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/decls/decls.go b/common/decls/decls.go index 2db1a027..0a42f81d 100644 --- a/common/decls/decls.go +++ b/common/decls/decls.go @@ -162,7 +162,7 @@ func (f *FunctionDecl) AddOverload(overload *OverloadDecl) error { if oID == overload.ID() { if o.SignatureEquals(overload) && o.IsNonStrict() == overload.IsNonStrict() { // Allow redefinition of an overload implementation so long as the signatures match. - if !o.hasBinding() || overload.hasBinding() { + if overload.hasBinding() { f.overloads[oID] = overload } return nil