From b032e3bfc00044691bd1c749596d2429e06e2324 Mon Sep 17 00:00:00 2001 From: Johan Fylling Date: Fri, 24 Jan 2025 13:47:16 +0100 Subject: [PATCH] Fixing issue where bundle plugin could panic on reconfiguration (SDK use) (#7300) Fixing issue where bundle plugin would panic on reconfiguration if module rego-version is missing in bundle manifest. * Passing runtime rego-version to deactivation options * Preferring to pull rego-version from parsed modules if present This solves an edge case when using the OPA SDK, and should not affect standalone OPA. Fixes: #7297 Signed-off-by: Johan Fylling --- v1/bundle/store.go | 24 +- v1/bundle/store_test.go | 2 +- v1/plugins/bundle/plugin.go | 9 +- v1/plugins/bundle/plugin_test.go | 376 +++++++++++++++++++++++++++---- v1/tester/runner.go | 13 +- 5 files changed, 367 insertions(+), 57 deletions(-) diff --git a/v1/bundle/store.go b/v1/bundle/store.go index 86b75ee0d4..e77c052d9b 100644 --- a/v1/bundle/store.go +++ b/v1/bundle/store.go @@ -817,9 +817,23 @@ func writeEtagToStore(opts *ActivateOpts, name, etag string) error { } func writeModuleRegoVersionToStore(ctx context.Context, store storage.Store, txn storage.Transaction, b *Bundle, - bundlePath string, storagePath string, runtimeRegoVersion ast.RegoVersion) error { - if v, err := b.RegoVersionForFile(bundlePath, ast.RegoUndefined); err == nil && v != ast.RegoUndefined && v != runtimeRegoVersion { - if err := write(ctx, store, txn, moduleRegoVersionPath(storagePath), v.Int()); err != nil { + mf ModuleFile, storagePath string, runtimeRegoVersion ast.RegoVersion) error { + + var regoVersion ast.RegoVersion + if mf.Parsed != nil { + regoVersion = mf.Parsed.RegoVersion() + } + + if regoVersion == ast.RegoUndefined { + var err error + regoVersion, err = b.RegoVersionForFile(mf.Path, ast.RegoUndefined) + if err != nil { + return fmt.Errorf("failed to get rego version for module '%s' in bundle: %w", mf.Path, err) + } + } + + if regoVersion != ast.RegoUndefined && regoVersion != runtimeRegoVersion { + if err := write(ctx, store, txn, moduleRegoVersionPath(storagePath), regoVersion.Int()); err != nil { return fmt.Errorf("failed to write rego version for module '%s': %w", storagePath, err) } } @@ -853,7 +867,7 @@ func writeDataAndModules(ctx context.Context, store storage.Store, txn storage.T return err } - if err := writeModuleRegoVersionToStore(ctx, store, txn, b, mf.Path, path, runtimeRegoVersion); err != nil { + if err := writeModuleRegoVersionToStore(ctx, store, txn, b, mf, path, runtimeRegoVersion); err != nil { return err } } @@ -875,7 +889,7 @@ func writeDataAndModules(ctx context.Context, store storage.Store, txn storage.T if m := f.module; m != nil { // 'f.module.Path' contains the module's path as it relates to the bundle root, and can be used for looking up the rego-version. // 'f.Path' can differ, based on how the bundle reader was initialized. - if err := writeModuleRegoVersionToStore(ctx, store, txn, b, f.module.Path, p.String(), runtimeRegoVersion); err != nil { + if err := writeModuleRegoVersionToStore(ctx, store, txn, b, *m, p.String(), runtimeRegoVersion); err != nil { return err } } diff --git a/v1/bundle/store_test.go b/v1/bundle/store_test.go index 2bc265dcbe..5c583e4c48 100644 --- a/v1/bundle/store_test.go +++ b/v1/bundle/store_test.go @@ -1702,7 +1702,7 @@ func TestBundleLifecycle_ModuleRegoVersions(t *testing.T) { t.Fatalf("unexpected error: %s", err) } - expectedRaw := deact.expData // `{"system": {"bundles": {}, "modules": {}}}` + expectedRaw := deact.expData expected := loadExpectedSortedResult(expectedRaw) if !reflect.DeepEqual(expected, actual) { t.Errorf("expected:\n\n%s\n\ngot:\n\n%s", expectedRaw, string(util.MustMarshalJSON(actual))) diff --git a/v1/plugins/bundle/plugin.go b/v1/plugins/bundle/plugin.go index 2af14d6fae..63ad92148c 100644 --- a/v1/plugins/bundle/plugin.go +++ b/v1/plugins/bundle/plugin.go @@ -197,10 +197,11 @@ func (p *Plugin) Reconfigure(ctx context.Context, config interface{}) { params.Context = storage.NewContext() // TODO(sr): metrics? err := storage.Txn(ctx, p.manager.Store, params, func(txn storage.Transaction) error { opts := &bundle.DeactivateOpts{ - Ctx: ctx, - Store: p.manager.Store, - Txn: txn, - BundleNames: deletedBundles, + Ctx: ctx, + Store: p.manager.Store, + Txn: txn, + BundleNames: deletedBundles, + ParserOptions: p.manager.ParserOptions(), } err := bundle.Deactivate(opts) if err != nil { diff --git a/v1/plugins/bundle/plugin_test.go b/v1/plugins/bundle/plugin_test.go index 8998000c8d..1b6707cf3c 100644 --- a/v1/plugins/bundle/plugin_test.go +++ b/v1/plugins/bundle/plugin_test.go @@ -1674,23 +1674,12 @@ corge contains 1 if { t.Fatalf("Bad policy content. Exp:\n%v\n\nGot:\n\n%v", string(exp), string(bs)) } - var expData any - if tc.v1Compatible { - expData = util.MustUnmarshalJSON([]byte(`{ - "foo": {"bar": 1, "baz": "qux"}, - "system": { - "bundles": {"test-bundle": {"etag": "foo", "manifest": {"revision": "quickbrownfaux", "roots": [""]}}} - } - }`)) - } else { - expData = util.MustUnmarshalJSON([]byte(`{ - "foo": {"bar": 1, "baz": "qux"}, - "system": { - "bundles": {"test-bundle": {"etag": "foo", "manifest": {"revision": "quickbrownfaux", "roots": [""]}}}, - "modules": {"test-bundle/foo/bar.rego": {"rego_version": 1}} - } - }`)) - } + expData := util.MustUnmarshalJSON([]byte(`{ + "foo": {"bar": 1, "baz": "qux"}, + "system": { + "bundles": {"test-bundle": {"etag": "foo", "manifest": {"revision": "quickbrownfaux", "roots": [""]}}} + } + }`)) data, err := manager.Store.Read(ctx, txn, storage.Path{}) if err != nil { @@ -1988,12 +1977,6 @@ corge contains 1 if { if *tc.bundleRegoVersion != tc.managerRegoVersion { moduleRegoVersion = fmt.Sprintf(`,"modules": {"test-bundle/foo/bar.rego": {"rego_version": %d}}`, tc.bundleRegoVersion.Int()) } - } else { - manifestRegoVersion = "" - - if ast.DefaultRegoVersion != tc.managerRegoVersion { - moduleRegoVersion = fmt.Sprintf(`,"modules": {"test-bundle/foo/bar.rego": {"rego_version": %d}}`, ast.DefaultRegoVersion.Int()) - } } expData := util.MustUnmarshalJSON([]byte(fmt.Sprintf(`{ @@ -2520,23 +2503,12 @@ corge contains 2 if { } } - var expData any - if tc.v1Compatible { - expData = util.MustUnmarshalJSON([]byte(`{ - "foo": {"bar": 1, "baz": "qux"}, - "system": { - "bundles": {"test-bundle": {"etag": "", "manifest": {"revision": "quickbrownfaux", "roots": [""]}}} - } - }`)) - } else { - expData = util.MustUnmarshalJSON([]byte(`{ - "foo": {"bar": 1, "baz": "qux"}, - "system": { - "bundles": {"test-bundle": {"etag": "", "manifest": {"revision": "quickbrownfaux", "roots": [""]}}}, - "modules": {"test-bundle/foo/bar.rego": {"rego_version": 1}} - } - }`)) - } + expData := util.MustUnmarshalJSON([]byte(`{ + "foo": {"bar": 1, "baz": "qux"}, + "system": { + "bundles": {"test-bundle": {"etag": "", "manifest": {"revision": "quickbrownfaux", "roots": [""]}}} + } + }`)) data, err := manager.Store.Read(ctx, txn, storage.Path{}) if err != nil { @@ -2761,7 +2733,7 @@ corge contains 1 if { if tc.bundleRegoVersion != nil { moduleRegoVersion = tc.bundleRegoVersion.Int() } else { - moduleRegoVersion = ast.DefaultRegoVersion.Int() + moduleRegoVersion = runtimeRegoVersion } var expData any @@ -4246,6 +4218,328 @@ func TestPluginRequestVsDownloadTimestamp(t *testing.T) { } } +func TestReconfigurePlugin_OneShot_BundleDeactivation(t *testing.T) { + t.Parallel() + + tests := []struct { + note string + runtimeRegoVersion ast.RegoVersion + bundleRegoVersion ast.RegoVersion + moduleRegoVersion ast.RegoVersion + module string + }{ + { + note: "v0 runtime, v0 bundle", + runtimeRegoVersion: ast.RegoV0, + bundleRegoVersion: ast.RegoV0, + moduleRegoVersion: ast.RegoV0, + module: `package a + p[42] { true }`, + }, + { + note: "v0 runtime, v1 bundle", + runtimeRegoVersion: ast.RegoV0, + bundleRegoVersion: ast.RegoV1, + moduleRegoVersion: ast.RegoV1, + module: `package a + p contains 42 if { true }`, + }, + { + note: "v0 runtime, custom bundle", + runtimeRegoVersion: ast.RegoV0, + bundleRegoVersion: ast.RegoUndefined, + moduleRegoVersion: ast.RegoV0, + module: `package a + p[42] { true }`, + }, + { + note: "v1 runtime, v0 bundle", + runtimeRegoVersion: ast.RegoV1, + bundleRegoVersion: ast.RegoV0, + moduleRegoVersion: ast.RegoV0, + module: `package a + p[42] { true }`, + }, + { + note: "v1 runtime, v1 bundle", + runtimeRegoVersion: ast.RegoV1, + bundleRegoVersion: ast.RegoV1, + moduleRegoVersion: ast.RegoV1, + module: `package a + p contains 42 if { true }`, + }, + { + note: "v1 runtime, custom bundle", + runtimeRegoVersion: ast.RegoV1, + bundleRegoVersion: ast.RegoUndefined, + moduleRegoVersion: ast.RegoV1, + module: `package a + p contains 42 if { true }`, + }, + } + + for _, tc := range tests { + t.Run(tc.note, func(t *testing.T) { + ctx := context.Background() + manager, err := plugins.New(nil, "test-instance-id", inmemtst.New(), plugins.WithParserOptions(ast.ParserOptions{RegoVersion: tc.runtimeRegoVersion})) + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + bundleName := "test-bundle" + + plugin := Plugin{ + manager: manager, + status: map[string]*Status{}, + etags: map[string]string{}, + downloaders: map[string]Loader{}, + config: Config{ + Bundles: map[string]*Source{ + bundleName: { + Service: "s1", + }, + }, + }, + } + + plugin.status[bundleName] = &Status{Name: bundleName} + plugin.downloaders[bundleName] = download.New(download.Config{Trigger: pointTo(plugins.TriggerManual)}, plugin.manager.Client(""), bundleName) + + b := bundle.Bundle{ + Manifest: bundle.Manifest{ + Revision: "quickbrownfaux", + Roots: &[]string{"a"}, + }, + Modules: []bundle.ModuleFile{ + { + Path: "bundle/id1", + Parsed: ast.MustParseModuleWithOpts(tc.module, ast.ParserOptions{RegoVersion: tc.moduleRegoVersion}), + Raw: []byte(tc.module), + }, + }, + } + + if tc.bundleRegoVersion != ast.RegoUndefined { + b.Manifest.RegoVersion = pointTo(tc.bundleRegoVersion.Int()) + } + + b.Manifest.Init() + + plugin.oneShot(ctx, bundleName, download.Update{Bundle: &b}) + + // Ensure it has been activated + + txn := storage.NewTransactionOrDie(ctx, manager.Store) + + ids, err := manager.Store.ListPolicies(ctx, txn) + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + + expIDs := []string{"test-bundle/bundle/id1"} + + sort.Strings(ids) + sort.Strings(expIDs) + + if !slices.Equal(ids, expIDs) { + t.Fatalf("expected ids %v but got %v", expIDs, ids) + } + + manager.Store.Abort(ctx, txn) + + // reconfigure with dropped bundle + + plugin.Reconfigure(ctx, &Config{ + Bundles: map[string]*Source{}, + }) + + // bundle was removed from store + + txn = storage.NewTransactionOrDie(ctx, manager.Store) + + ids, err = manager.Store.ListPolicies(ctx, txn) + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + + expIDs = []string{} + + sort.Strings(ids) + + if !slices.Equal(ids, expIDs) { + t.Fatalf("expected ids %v but got %v", expIDs, ids) + } + + manager.Store.Abort(ctx, txn) + }) + } +} + +func TestReconfigurePlugin_ManagerInit_BundleDeactivation(t *testing.T) { + t.Parallel() + + tests := []struct { + note string + runtimeRegoVersion ast.RegoVersion + bundleRegoVersion ast.RegoVersion + moduleRegoVersion ast.RegoVersion + module string + }{ + { + note: "v0 runtime, v0 bundle", + runtimeRegoVersion: ast.RegoV0, + bundleRegoVersion: ast.RegoV0, + moduleRegoVersion: ast.RegoV0, + module: `package a + p[42] { true }`, + }, + { + note: "v0 runtime, v1 bundle", + runtimeRegoVersion: ast.RegoV0, + bundleRegoVersion: ast.RegoV1, + moduleRegoVersion: ast.RegoV1, + module: `package a + p contains 42 if { true }`, + }, + { + note: "v0 runtime, custom bundle", + runtimeRegoVersion: ast.RegoV0, + bundleRegoVersion: ast.RegoUndefined, + moduleRegoVersion: ast.RegoV0, + module: `package a + p[42] { true }`, + }, + { + note: "v1 runtime, v0 bundle", + runtimeRegoVersion: ast.RegoV1, + bundleRegoVersion: ast.RegoV0, + moduleRegoVersion: ast.RegoV0, + module: `package a + p[42] { true }`, + }, + { + note: "v1 runtime, v1 bundle", + runtimeRegoVersion: ast.RegoV1, + bundleRegoVersion: ast.RegoV1, + moduleRegoVersion: ast.RegoV1, + module: `package a + p contains 42 if { true }`, + }, + { + note: "v1 runtime, custom bundle", + runtimeRegoVersion: ast.RegoV1, + bundleRegoVersion: ast.RegoUndefined, + moduleRegoVersion: ast.RegoV1, + module: `package a + p contains 42 if { true }`, + }, + } + + for _, tc := range tests { + t.Run(tc.note, func(t *testing.T) { + bundleName := "test-bundle" + + b := bundle.Bundle{ + Manifest: bundle.Manifest{ + Revision: "quickbrownfaux", + Roots: &[]string{"a"}, + }, + Modules: []bundle.ModuleFile{ + { + Path: "bundle/id1", + Parsed: ast.MustParseModuleWithOpts(tc.module, ast.ParserOptions{RegoVersion: tc.moduleRegoVersion}), + Raw: []byte(tc.module), + }, + }, + } + + if tc.bundleRegoVersion != ast.RegoUndefined { + b.Manifest.RegoVersion = pointTo(tc.bundleRegoVersion.Int()) + } + + b.Manifest.Init() + + bundles := map[string]*bundle.Bundle{ + bundleName: &b, + } + + ctx := context.Background() + manager, err := plugins.New(nil, "test-instance-id", inmemtst.New(), + plugins.WithParserOptions(ast.ParserOptions{RegoVersion: tc.runtimeRegoVersion}), + plugins.InitBundles(bundles)) + + if err := manager.Init(ctx); err != nil { + t.Fatalf("unexpected error: %s", err) + } + + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + + plugin := Plugin{ + manager: manager, + status: map[string]*Status{}, + etags: map[string]string{}, + downloaders: map[string]Loader{}, + config: Config{ + Bundles: map[string]*Source{ + bundleName: { + Service: "s1", + }, + }, + }, + } + + plugin.status[bundleName] = &Status{Name: bundleName} + plugin.downloaders[bundleName] = download.New(download.Config{Trigger: pointTo(plugins.TriggerManual)}, plugin.manager.Client(""), bundleName) + + // Ensure it has been activated + + txn := storage.NewTransactionOrDie(ctx, manager.Store) + + ids, err := manager.Store.ListPolicies(ctx, txn) + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + + expIDs := []string{"test-bundle/bundle/id1"} + + sort.Strings(ids) + sort.Strings(expIDs) + + if !slices.Equal(ids, expIDs) { + t.Fatalf("expected ids %v but got %v", expIDs, ids) + } + + manager.Store.Abort(ctx, txn) + + // reconfigure with dropped bundle + + plugin.Reconfigure(ctx, &Config{ + Bundles: map[string]*Source{}, + }) + + // bundle was removed from store + + txn = storage.NewTransactionOrDie(ctx, manager.Store) + + ids, err = manager.Store.ListPolicies(ctx, txn) + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + + expIDs = []string{} + + sort.Strings(ids) + + if !slices.Equal(ids, expIDs) { + t.Fatalf("expected ids %v but got %v", expIDs, ids) + } + + manager.Store.Abort(ctx, txn) + }) + } +} + func TestUpgradeLegacyBundleToMuiltiBundleSameBundle(t *testing.T) { t.Parallel() diff --git a/v1/tester/runner.go b/v1/tester/runner.go index 1ddd60bfe6..63a59c9785 100644 --- a/v1/tester/runner.go +++ b/v1/tester/runner.go @@ -320,12 +320,13 @@ func (r *Runner) runTests(ctx context.Context, txn storage.Transaction, enablePr // Activate the bundle(s) to get their info and policies into the store // the actual compiled policies will overwritten later.. opts := &bundle.ActivateOpts{ - Ctx: ctx, - Store: r.store, - Txn: txn, - Compiler: r.compiler, - Metrics: metrics.New(), - Bundles: r.bundles, + Ctx: ctx, + Store: r.store, + Txn: txn, + Compiler: r.compiler, + Metrics: metrics.New(), + Bundles: r.bundles, + ParserOptions: ast.ParserOptions{RegoVersion: r.defaultRegoVersion}, } err = bundle.Activate(opts) if err != nil {