ewilliams28/releases
1
0
Fork 0
mirror of https://github.com/open-policy-agent/opa.git synced 2026-08-12 19:32:48 -06:00
Code Issues Packages Projects Releases Wiki Activity

Fixing issue where bundle plugin could panic on reconfiguration (SDK use) (#7300)

Browse Source 
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 <johan.dev@fylling.se>
This commit is contained in:
Johan Fylling
2025-01-24 13:47:16 +01:00
committed by GitHub
parent e47bd4f9a8
commit b032e3bfc0
5 changed files with 367 additions and 57 deletions
Download Patch File Download Diff File Expand all files Collapse all files
v1/bundle/store.go
+19 -5
View File
@@ -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
}
}
v1/bundle/store_test.go
+1 -1
View File
@@ -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)))
v1/plugins/bundle/plugin.go
+5 -4
View File
@@ -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 {
v1/plugins/bundle/plugin_test.go
+335 -41
View File
@@ -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()
v1/tester/runner.go
+7 -6
View File
@@ -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 {