From 7dd1dd88529ca32fd77ee6befb45d3f50795cec6 Mon Sep 17 00:00:00 2001 From: Francisco Rodrigues Date: Tue, 9 Apr 2024 18:23:32 -0300 Subject: [PATCH 1/3] sdk: fix bug while activating the bundle plugin For some reason the bundle plugin activation with multiple optimized bundles, builded in v1 syntax, results in parsing error. During the activation of the bundle, when we need to parse the rego modules, the ParserOptions is not forwarded to the `bundle.ActivateOps`, so the v1 syntax does not seem to be properly parsed. Signed-off-by: Francisco Rodrigues --- plugins/bundle/plugin.go | 15 ++++----- sdk/opa_test.go | 51 +++++++++++++++++++++++++++++++ sdk/testdata/Makefile | 6 ++++ sdk/testdata/policy1.tar.gz | Bin 0 -> 434 bytes sdk/testdata/policy1/.manifest | 3 ++ sdk/testdata/policy1/policy.rego | 24 +++++++++++++++ sdk/testdata/policy2.tar.gz | Bin 0 -> 262 bytes sdk/testdata/policy2/.manifest | 3 ++ sdk/testdata/policy2/policy.rego | 11 +++++++ 9 files changed, 106 insertions(+), 7 deletions(-) create mode 100644 sdk/testdata/policy1.tar.gz create mode 100644 sdk/testdata/policy1/.manifest create mode 100644 sdk/testdata/policy1/policy.rego create mode 100644 sdk/testdata/policy2.tar.gz create mode 100644 sdk/testdata/policy2/.manifest create mode 100644 sdk/testdata/policy2/policy.rego diff --git a/plugins/bundle/plugin.go b/plugins/bundle/plugin.go index 818952d9a9..2a8aedfd0b 100644 --- a/plugins/bundle/plugin.go +++ b/plugins/bundle/plugin.go @@ -597,13 +597,14 @@ func (p *Plugin) activate(ctx context.Context, name string, b *bundle.Bundle) er var activateErr error opts := &bundle.ActivateOpts{ - Ctx: ctx, - Store: p.manager.Store, - Txn: txn, - TxnCtx: params.Context, - Compiler: compiler, - Metrics: p.status[name].Metrics, - Bundles: map[string]*bundle.Bundle{name: b}, + Ctx: ctx, + Store: p.manager.Store, + Txn: txn, + TxnCtx: params.Context, + Compiler: compiler, + Metrics: p.status[name].Metrics, + Bundles: map[string]*bundle.Bundle{name: b}, + ParserOptions: p.manager.ParserOptions(), } if p.manager.Info != nil { diff --git a/sdk/opa_test.go b/sdk/opa_test.go index c4985674f8..74ba4edd16 100644 --- a/sdk/opa_test.go +++ b/sdk/opa_test.go @@ -2844,3 +2844,54 @@ func toMetricMap(metrics []*promdto.MetricFamily) map[string]bool { } return metricMap } + +func TestActivateOptimizedV1Bundles(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), time.Millisecond*100) + defer cancel() + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.FileServer(http.Dir("testdata")).ServeHTTP(w, r) + })) + + config := fmt.Sprintf(`{ + "services": { + "test": { + "url": %q + } + }, + "bundles": { + "policy1": { + "resource": "/policy1.tar.gz" + }, + "policy2": { + "resource": "/policy2.tar.gz" + } + } + }`, server.URL) + + opa, err := sdk.New(ctx, sdk.Options{ + ID: "sdk-id-0", + Config: strings.NewReader(config), + V1Compatible: true, + }) + + defer opa.Stop(ctx) + + if err != nil { + t.Fatal(err) + } + + d, err := opa.Decision(context.Background(), sdk.DecisionOptions{ + Path: "policy2/authz", + Input: map[string]interface{}{ + "role": "admin", + }, + }) + if err != nil { + t.Fatal(err) + } + + if d.Result != true { + t.Errorf("expected result to be true, got %v", d.Result) + } +} diff --git a/sdk/testdata/Makefile b/sdk/testdata/Makefile index b2e96e07c1..a29403de90 100644 --- a/sdk/testdata/Makefile +++ b/sdk/testdata/Makefile @@ -1,3 +1,9 @@ disco.tar.gz: bundle/data.json opa build bundle mv bundle.tar.gz disco.tar.gz + +policy1.tar.gz: + opa build --partial-namespace policy1_partial --optimize=1 --v1-compatible -o policy1.tar.gz -b policy1/ + +policy2.tar.gz: + opa build --partial-namespace policy2_partial --optimize=1 --v1-compatible -o policy2.tar.gz -b policy2/ diff --git a/sdk/testdata/policy1.tar.gz b/sdk/testdata/policy1.tar.gz new file mode 100644 index 0000000000000000000000000000000000000000..0357fcd4c29366dce14884238d6bcbda7f26f3aa GIT binary patch literal 434 zcmV;j0ZslNiwFP!00000|Lm4gZ<{a>fb&}a3d_$GIYY>#X!#}6bh?4ldNnpOrdu23 zzfU93F;XoLYZ4*(d+|lK?;Nb}U|CY2*tO2?aPDcR9MlmcvO33Ac!{v_j zhqeji7x3S;rhjfG><{eLM~K{zrK^Lk^s6eDwKKYSk%?Ci?n7&w^FL2l6aTYZev1BI zgPMvbdQiN-e8$Ka_k-&l-?KaG0`J}r{VdkxgC>(8S{G6T%n;duu~O}*HGxwb9$)!p z%XefN#n|!pG1)j#a;&oD7?iJ2B1m>Oj{jX*qQrctY+w2U+SYAIyfcb#x%g&W@uW)e zdy&zV4yYUSii$^7_8Sn5E64!iif{RGd(nEYYjUTfPgm=S|9P6NqW{-m;{R~@jEzNm&)`nHLEwBOa2Fr?1bvTqFTshFL$VAZ z9g=nNyo{O`%cLT!cd7}Ot8w=Ge?9eI!a9%sUxU5y>RC71S+Nn~UU=t1BQ{?|uc^2f clhz=#p+7@b>Mt40`AOwZ^&pD|L62|8=d_Iscd7Y5PJu zEf0?`M5lWiO_DUGr@d@9vL_R@5MSTi%Sqg@^XC)%WeuQ%Fn(9&*yNEX$VfBLD#Z M|AVy#O#l!80KE2olmGw# literal 0 HcmV?d00001 diff --git a/sdk/testdata/policy2/.manifest b/sdk/testdata/policy2/.manifest new file mode 100644 index 0000000000..41f21d76b4 --- /dev/null +++ b/sdk/testdata/policy2/.manifest @@ -0,0 +1,3 @@ +{ + "roots": ["policy2"] +} \ No newline at end of file diff --git a/sdk/testdata/policy2/policy.rego b/sdk/testdata/policy2/policy.rego new file mode 100644 index 0000000000..5a5debfe81 --- /dev/null +++ b/sdk/testdata/policy2/policy.rego @@ -0,0 +1,11 @@ +package policy2 + +import rego.v1 + +default authz := false + +# METADATA +# entrypoint: true +authz if { + input.role == "admin" +} From 18954383e4bd7652bc456d843dc303d45bba959e Mon Sep 17 00:00:00 2001 From: Johan Fylling Date: Thu, 11 Apr 2024 11:20:22 +0200 Subject: [PATCH 2/3] Updating `TestLoadAndActivateBundlesFromDiskV1Compatible` to assert the function of multiple consecutive bundle updates. Signed-off-by: Johan Fylling --- plugins/bundle/plugin_test.go | 228 +++++++++++++++++++++++----------- 1 file changed, 156 insertions(+), 72 deletions(-) diff --git a/plugins/bundle/plugin_test.go b/plugins/bundle/plugin_test.go index 9fe5e85845..766d80af0e 100644 --- a/plugins/bundle/plugin_test.go +++ b/plugins/bundle/plugin_test.go @@ -1374,50 +1374,126 @@ func TestLoadAndActivateBundlesFromDisk(t *testing.T) { } func TestLoadAndActivateBundlesFromDiskV1Compatible(t *testing.T) { + type update struct { + modules map[string]string + expErrs []string + } // Note: modules are parsed before passed to plugin, so any expected errors must be triggered by the compiler stage. tests := []struct { note string v1Compatible bool - module string - expErrs []string + updates []update }{ { note: "v0.x", - module: `package foo + updates: []update{ + { + modules: map[string]string{ + "/foo/bar.rego": `package foo import future.keywords corge contains 1 if { input.x == 2 }`, + }, + }, + }, }, { note: "v0.x, shadowed import (no error)", - module: `package foo + updates: []update{ + { + modules: map[string]string{ + "/foo/bar.rego": `package foo import future.keywords import data.foo import data.bar as foo corge contains 1 if { input.x == 2 }`, + }, + }, + }, }, { note: "v1.0", v1Compatible: true, - module: `package foo + updates: []update{ + { + modules: map[string]string{ + "/foo/bar.rego": `package foo corge contains 1 if { input.x == 2 }`, + }, + }, + }, }, { note: "v1.0, shadowed import", v1Compatible: true, - module: `package foo + updates: []update{ + { + modules: map[string]string{ + "/foo/bar.rego": `package foo import data.foo import data.bar as foo corge contains 1 if { input.x == 2 }`, - expErrs: []string{ - "rego_compile_error: import must not shadow import data.foo", + }, + expErrs: []string{ + "rego_compile_error: import must not shadow import data.foo", + }, + }, + }, + }, + { + note: "v1.0, module updated", + v1Compatible: true, + updates: []update{ + { + modules: map[string]string{ + "/foo/bar.rego": `package foo +corge contains 1 if { + input.x == 2 +}`, + }, + }, + { + modules: map[string]string{ + "/foo/bar.rego": `package foo +corge contains 2 if { + input.x == 3 +}`, + }, + }, + }, + }, + { + note: "v1.0, module updated, shadowed import", + v1Compatible: true, + updates: []update{ + { + modules: map[string]string{ + "/foo/bar.rego": `package foo +corge contains 1 if { + input.x == 2 +}`, + }, + }, + { + modules: map[string]string{ + "/foo/bar.rego": `package foo +import data.foo +import data.bar as foo +corge contains 2 if { + input.x == 3 +}`, + }, + expErrs: []string{ + "rego_compile_error: import must not shadow import data.foo", + }, + }, }, }, } @@ -1455,80 +1531,88 @@ corge contains 1 if { plugin.loadAndActivateBundlesFromDisk(ctx) - // persist a bundle to disk and then load it - b := bundle.Bundle{ - Manifest: bundle.Manifest{Revision: "quickbrownfaux"}, - Data: util.MustUnmarshalJSON([]byte(`{"foo": {"bar": 1, "baz": "qux"}}`)).(map[string]interface{}), - Modules: []bundle.ModuleFile{ - { - URL: "/foo/bar.rego", - Path: "/foo/bar.rego", - Parsed: ast.MustParseModuleWithOpts(tc.module, popts), - Raw: []byte(tc.module), - }, - }, - } - - b.Manifest.Init() + for _, update := range tc.updates { + // persist a bundle to disk and then load it + b := bundle.Bundle{ + Manifest: bundle.Manifest{Revision: "quickbrownfaux"}, + Data: util.MustUnmarshalJSON([]byte(`{"foo": {"bar": 1, "baz": "qux"}}`)).(map[string]interface{}), + } + for url, module := range update.modules { + b.Modules = append(b.Modules, bundle.ModuleFile{ + URL: url, + Path: url, + Parsed: ast.MustParseModuleWithOpts(module, popts), + Raw: []byte(module), + }) + } - var buf bytes.Buffer - if err := bundle.NewWriter(&buf).UseModulePath(true).Write(b); err != nil { - t.Fatal("unexpected error:", err) - } + b.Manifest.Init() - err = plugin.saveBundleToDisk(bundleName, &buf) - if err != nil { - t.Fatalf("unexpected error %v", err) - } + var buf bytes.Buffer + if err := bundle.NewWriter(&buf).UseModulePath(true).Write(b); err != nil { + t.Fatal("unexpected error:", err) + } - plugin.loadAndActivateBundlesFromDisk(ctx) + err = plugin.saveBundleToDisk(bundleName, &buf) + if err != nil { + t.Fatalf("unexpected error %v", err) + } - if tc.expErrs != nil { - if status, ok := plugin.status[bundleName]; !ok { - t.Fatalf("Expected to find status for %s, found nil", bundleName) - } else if status.Type != bundle.SnapshotBundleType { - t.Fatalf("expected snapshot bundle but got %v", status.Type) - } else if errs := status.Errors; len(errs) != len(tc.expErrs) { - t.Fatalf("expected errors:\n\n%v\n\nbut got:\n\n%v", tc.expErrs, errs) - } else { - for _, expErr := range tc.expErrs { - found := false - for _, err := range errs { - if strings.Contains(err.Error(), expErr) { - found = true - break + plugin.loadAndActivateBundlesFromDisk(ctx) + + if update.expErrs != nil { + if status, ok := plugin.status[bundleName]; !ok { + t.Fatalf("Expected to find status for %s, found nil", bundleName) + } else if status.Type != bundle.SnapshotBundleType { + t.Fatalf("expected snapshot bundle but got %v", status.Type) + } else if errs := status.Errors; len(errs) != len(update.expErrs) { + t.Fatalf("expected errors:\n\n%v\n\nbut got:\n\n%v", update.expErrs, errs) + } else { + for _, expErr := range update.expErrs { + found := false + for _, err := range errs { + if strings.Contains(err.Error(), expErr) { + found = true + break + } + } + if !found { + t.Fatalf("expected error:\n\n%v\n\nbut got:\n\n%v", expErr, errs) } - } - if !found { - t.Fatalf("expected error:\n\n%v\n\nbut got:\n\n%v", expErr, errs) } } - } - } else { - txn := storage.NewTransactionOrDie(ctx, manager.Store) - defer manager.Store.Abort(ctx, txn) + } else { + txn := storage.NewTransactionOrDie(ctx, manager.Store) + fatal := func(args ...any) { + manager.Store.Abort(ctx, txn) + t.Fatal(args...) + } - ids, err := manager.Store.ListPolicies(ctx, txn) - if err != nil { - t.Fatal(err) - } else if len(ids) != 1 { - t.Fatal("Expected 1 policy") - } + ids, err := manager.Store.ListPolicies(ctx, txn) + if err != nil { + fatal(err) + } + for _, id := range ids { + bs, err := manager.Store.GetPolicy(ctx, txn, id) + p, _ := strings.CutPrefix(id, bundleName) + module := update.modules[p] + exp := []byte(module) + if err != nil { + fatal(err) + } else if !bytes.Equal(bs, exp) { + fatal("Bad policy content. Exp:\n%v\n\nGot:\n\n%v", string(exp), string(bs)) + } + } - bs, err := manager.Store.GetPolicy(ctx, txn, ids[0]) - exp := []byte(tc.module) - if err != nil { - t.Fatal(err) - } else if !bytes.Equal(bs, exp) { - t.Fatalf("Bad policy content. Exp:\n%v\n\nGot:\n\n%v", string(exp), string(bs)) - } + data, err := manager.Store.Read(ctx, txn, storage.Path{}) + expData := util.MustUnmarshalJSON([]byte(`{"foo": {"bar": 1, "baz": "qux"}, "system": {"bundles": {"test-bundle": {"etag": "", "manifest": {"revision": "quickbrownfaux", "roots": [""]}}}}}`)) + if err != nil { + fatal(err) + } else if !reflect.DeepEqual(data, expData) { + fatal("Bad data content. Exp:\n%v\n\nGot:\n\n%v", expData, data) + } - data, err := manager.Store.Read(ctx, txn, storage.Path{}) - expData := util.MustUnmarshalJSON([]byte(`{"foo": {"bar": 1, "baz": "qux"}, "system": {"bundles": {"test-bundle": {"etag": "", "manifest": {"revision": "quickbrownfaux", "roots": [""]}}}}}`)) - if err != nil { - t.Fatal(err) - } else if !reflect.DeepEqual(data, expData) { - t.Fatalf("Bad data content. Exp:\n%v\n\nGot:\n\n%v", expData, data) + manager.Store.Abort(ctx, txn) } } }) From 2d4a6ff3fb94864559c732234b9b86dc944bfe7d Mon Sep 17 00:00:00 2001 From: Francisco Rodrigues Date: Thu, 11 Apr 2024 07:13:33 -0300 Subject: [PATCH 3/3] sdk: refactor test The bug in activating bundles in the SDK is not necessarily related to optimized bundles, but specifically for v1 bundles. So the test was renamed to `TestActivateV1Bundles` and the test structure simplified to better describe the scenario. Signed-off-by: Francisco Rodrigues --- sdk/opa_test.go | 12 ++++----- sdk/testdata/Makefile | 6 ++--- sdk/testdata/policy1.tar.gz | Bin 434 -> 0 bytes sdk/testdata/policy1/.manifest | 3 --- sdk/testdata/policy1/policy.rego | 24 ------------------ sdk/testdata/policy2.tar.gz | Bin 262 -> 0 bytes sdk/testdata/policy2/.manifest | 3 --- sdk/testdata/v1bundle.tar.gz | Bin 0 -> 281 bytes sdk/testdata/v1bundle/.manifest | 3 +++ .../{policy2 => v1bundle}/policy.rego | 4 +-- 10 files changed, 11 insertions(+), 44 deletions(-) delete mode 100644 sdk/testdata/policy1.tar.gz delete mode 100644 sdk/testdata/policy1/.manifest delete mode 100644 sdk/testdata/policy1/policy.rego delete mode 100644 sdk/testdata/policy2.tar.gz delete mode 100644 sdk/testdata/policy2/.manifest create mode 100644 sdk/testdata/v1bundle.tar.gz create mode 100644 sdk/testdata/v1bundle/.manifest rename sdk/testdata/{policy2 => v1bundle}/policy.rego (74%) diff --git a/sdk/opa_test.go b/sdk/opa_test.go index 74ba4edd16..0fa1ccc8bf 100644 --- a/sdk/opa_test.go +++ b/sdk/opa_test.go @@ -2845,7 +2845,7 @@ func toMetricMap(metrics []*promdto.MetricFamily) map[string]bool { return metricMap } -func TestActivateOptimizedV1Bundles(t *testing.T) { +func TestActivateV1Bundles(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), time.Millisecond*100) defer cancel() @@ -2860,11 +2860,8 @@ func TestActivateOptimizedV1Bundles(t *testing.T) { } }, "bundles": { - "policy1": { - "resource": "/policy1.tar.gz" - }, - "policy2": { - "resource": "/policy2.tar.gz" + "v1bundle": { + "resource": "/v1bundle.tar.gz" } } }`, server.URL) @@ -2872,6 +2869,7 @@ func TestActivateOptimizedV1Bundles(t *testing.T) { opa, err := sdk.New(ctx, sdk.Options{ ID: "sdk-id-0", Config: strings.NewReader(config), + Logger: logging.New(), V1Compatible: true, }) @@ -2882,7 +2880,7 @@ func TestActivateOptimizedV1Bundles(t *testing.T) { } d, err := opa.Decision(context.Background(), sdk.DecisionOptions{ - Path: "policy2/authz", + Path: "v1bundle/authz", Input: map[string]interface{}{ "role": "admin", }, diff --git a/sdk/testdata/Makefile b/sdk/testdata/Makefile index a29403de90..1268401628 100644 --- a/sdk/testdata/Makefile +++ b/sdk/testdata/Makefile @@ -2,8 +2,6 @@ disco.tar.gz: bundle/data.json opa build bundle mv bundle.tar.gz disco.tar.gz -policy1.tar.gz: - opa build --partial-namespace policy1_partial --optimize=1 --v1-compatible -o policy1.tar.gz -b policy1/ +v1bundle.tar.gz: v1bundle/.manifest v1bundle/policy.rego + opa build --v1-compatible -o v1bundle.tar.gz -b v1bundle/ -policy2.tar.gz: - opa build --partial-namespace policy2_partial --optimize=1 --v1-compatible -o policy2.tar.gz -b policy2/ diff --git a/sdk/testdata/policy1.tar.gz b/sdk/testdata/policy1.tar.gz deleted file mode 100644 index 0357fcd4c29366dce14884238d6bcbda7f26f3aa..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 434 zcmV;j0ZslNiwFP!00000|Lm4gZ<{a>fb&}a3d_$GIYY>#X!#}6bh?4ldNnpOrdu23 zzfU93F;XoLYZ4*(d+|lK?;Nb}U|CY2*tO2?aPDcR9MlmcvO33Ac!{v_j zhqeji7x3S;rhjfG><{eLM~K{zrK^Lk^s6eDwKKYSk%?Ci?n7&w^FL2l6aTYZev1BI zgPMvbdQiN-e8$Ka_k-&l-?KaG0`J}r{VdkxgC>(8S{G6T%n;duu~O}*HGxwb9$)!p z%XefN#n|!pG1)j#a;&oD7?iJ2B1m>Oj{jX*qQrctY+w2U+SYAIyfcb#x%g&W@uW)e zdy&zV4yYUSii$^7_8Sn5E64!iif{RGd(nEYYjUTfPgm=S|9P6NqW{-m;{R~@jEzNm&)`nHLEwBOa2Fr?1bvTqFTshFL$VAZ z9g=nNyo{O`%cLT!cd7}Ot8w=Ge?9eI!a9%sUxU5y>RC71S+Nn~UU=t1BQ{?|uc^2f clhz=#p+7@b>Mt40`AOwZ^&pD|L62|8=d_Iscd7Y5PJu zEf0?`M5lWiO_DUGr@d@9vL_R@5MSTi%Sqg@^XC)%WeuQ%Fn(9&*yNEX$VfBLD#Z M|AVy#O#l!80KE2olmGw# diff --git a/sdk/testdata/policy2/.manifest b/sdk/testdata/policy2/.manifest deleted file mode 100644 index 41f21d76b4..0000000000 --- a/sdk/testdata/policy2/.manifest +++ /dev/null @@ -1,3 +0,0 @@ -{ - "roots": ["policy2"] -} \ No newline at end of file diff --git a/sdk/testdata/v1bundle.tar.gz b/sdk/testdata/v1bundle.tar.gz new file mode 100644 index 0000000000000000000000000000000000000000..9d7cce5ef2e707437eaeb9bb99c76520a6c98358 GIT binary patch literal 281 zcmV+!0p|W6iwFP!00000|LoPVPQx$|2H>ptDNgQ+k~3{OM9L5eMz(Hx#cIW_U0r^`lL!`S%`R|HWlFXliv)O165$A~S6%Q*SPQN^3hC*N|GQjah z`V<36pW(NA?SI@KF}$*KkC2>Eu@Mi^wni1bGkUqry;`{+?r|*tqMYsJe=1V`PotO1 zXSq_a`wbXtRVSk%kdYo=psJyhCMd=xaC@_8uA4=}CZH_&ZSSwWxLjvv$SY_qat5Rt2-5FydFpI|ExT?|MRPq|5F&aR~sF)v%KQ`l6&VU f@am5576QLd!A+7R`G?*D00960>5&Kr01yBG11^We literal 0 HcmV?d00001 diff --git a/sdk/testdata/v1bundle/.manifest b/sdk/testdata/v1bundle/.manifest new file mode 100644 index 0000000000..aa0f6db355 --- /dev/null +++ b/sdk/testdata/v1bundle/.manifest @@ -0,0 +1,3 @@ +{ + "roots": ["v1bundle"] +} \ No newline at end of file diff --git a/sdk/testdata/policy2/policy.rego b/sdk/testdata/v1bundle/policy.rego similarity index 74% rename from sdk/testdata/policy2/policy.rego rename to sdk/testdata/v1bundle/policy.rego index 5a5debfe81..fce2a45888 100644 --- a/sdk/testdata/policy2/policy.rego +++ b/sdk/testdata/v1bundle/policy.rego @@ -1,6 +1,4 @@ -package policy2 - -import rego.v1 +package v1bundle default authz := false