bundle: Fixing issue where `--v0-compatible` isn't respected for custom bundles #7338

johanfylling · 2025-02-04T13:33:08Z

where the bundle has been manually constructed containing v0 Rego modules and no rego_version/file_rego_versions fields are declared in the bundle manifest.

This affects bundle deactivation in the bundle store lifecycle used when for opa run in server mode (-s).

…om bundles where the bundle has been manually constructed containing v0 Rego modules and no `rego_version`/`file_rego_versions` fields are declared in the bundle manifest. This affects bundle deactivation in the bundle store lifecycle used when for `opa run` in server mode (`-s`). Signed-off-by: Johan Fylling <[email protected]>

netlify · 2025-02-04T13:34:43Z

✅ Deploy Preview for openpolicyagent ready!

Name	Link
🔨 Latest commit	`12d0f0a`
🔍 Latest deploy log	https://app.netlify.com/sites/openpolicyagent/deploys/67a3304e667472000811a3fe
😎 Deploy Preview	https://deploy-preview-7338--openpolicyagent.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

johanfylling

Some comments to aid review.

johanfylling · 2025-02-04T13:35:11Z

v1/bundle/bundle.go

@@ -1204,10 +1206,6 @@ func (b *Bundle) SetRegoVersion(v ast.RegoVersion) {
 // If there is no defined version for the given path, the default version def is returned.
 // If the version does not correspond to ast.RegoV0 or ast.RegoV1, an error is returned.
 func (b *Bundle) RegoVersionForFile(path string, def ast.RegoVersion) (ast.RegoVersion, error) {
-	if def == ast.RegoUndefined {
-		def = ast.DefaultRegoVersion
-	}


This was a "magic", undocumented behavior that even confused our own internal usage.

[comment]: I did wonder about this when I was looking at the sources recently. Thanks for addressing this!

johanfylling · 2025-02-04T13:38:38Z

v1/bundle/store.go

@@ -827,7 +827,7 @@ func writeModuleRegoVersionToStore(ctx context.Context, store storage.Store, txn

 	if regoVersion == ast.RegoUndefined {
 		var err error
-		regoVersion, err = b.RegoVersionForFile(mf.Path, ast.RegoUndefined)


This was the root cause of the issue. If no rego-version was declared for the module in the bundle, we'd always default to v1, and not the runtime rego-version (i.e. --v0-compatible enabled/disabled).

johanfylling · 2025-02-04T13:39:06Z

v1/bundle/store_test.go

+			},
+		},
+		{
+			note:               "custom bundle without rego-version, lazy, --v0-compatible",


This test-case fails without the fix.

johanfylling · 2025-02-04T13:43:44Z

v1/bundle/store_test.go

-		"modules":{
-			"example/example.rego":{
-				"rego_version":1
-			}


No explicit rego-version set for runtime and bundle, so both default to the same version (v1) and is therefore not recorded into the stored meta data.

Signed-off-by: Johan Fylling <[email protected]>

philipaconrad

Thank you for addressing this edge case for Rego v0/v1 support in bundles. 🙂

I've tested this PR branch against one of my own bundles that was failing due to a lack of the rego_version field in its manifest-- it works now!

A few comments and questions below:

philipaconrad · 2025-02-04T18:27:48Z

v1/bundle/bundle.go

+		} else if regoVersion != ast.RegoUndefined {
+			modulePopts.RegoVersion = regoVersion


[comment/question]: It is not obvious to me exactly when this condition could occur, even after diving into the sources for bundle.RegoVersionForFile. As best I can tell though, getting both an error and a regoVersion that isn't the default ast.RegoUndefined value should be pretty difficult.

If there's an obvious edge-case I'm missing, could we add a quick comment to explain it here? 😅

This is merely a safeguard against the world changing around us. You're correct that ast.RegoUndefined isn't an expected value here, but I think this is buried deeply enough that a future code change that alters this expectation could cause an issue here that is hard to detect. Also, if we get a ast.RegoUndefined returned here, we know that the correct course of action is to let the configured parser-options take precedence.

I'll add a comment 👍

philipaconrad · 2025-02-04T18:28:30Z

v1/bundle/bundle.go

@@ -1204,10 +1206,6 @@ func (b *Bundle) SetRegoVersion(v ast.RegoVersion) {
 // If there is no defined version for the given path, the default version def is returned.
 // If the version does not correspond to ast.RegoV0 or ast.RegoV1, an error is returned.
 func (b *Bundle) RegoVersionForFile(path string, def ast.RegoVersion) (ast.RegoVersion, error) {
-	if def == ast.RegoUndefined {
-		def = ast.DefaultRegoVersion
-	}


[comment]: I did wonder about this when I was looking at the sources recently. Thanks for addressing this!

philipaconrad · 2025-02-04T18:31:23Z

v1/plugins/plugins.go

+	if m.parserOptions.RegoVersion == ast.RegoUndefined {
+		m.parserOptions.RegoVersion = ast.DefaultRegoVersion
+	}


[question]: Is this where the "default to the ast.DefaultRegoVersion if none provided" logic needed to move to?

Yes. If a plugins.Manager has ast.RegoUndefined as rego-version, then any consumer of the manager would simply do the correct thing depending on what v0/v1 package it comes from, which is why this has gone undetected. Defaulting to v1 here if no rego-version has been provided through the options is however more consistent with user expectations.

Signed-off-by: Johan Fylling <[email protected]>

johanfylling commented Feb 4, 2025

View reviewed changes

johanfylling added 2 commits February 4, 2025 14:44

Adding non-lazy test cases

68b3054

Signed-off-by: Johan Fylling <[email protected]>

Merge branch 'main' into fix/bundle-store_custom-bundle_v0-compatible

564e1fd

ashutosh-narkar approved these changes Feb 4, 2025

View reviewed changes

philipaconrad approved these changes Feb 4, 2025

View reviewed changes

johanfylling added 2 commits February 5, 2025 10:31

Adding comments

c81a94c

Signed-off-by: Johan Fylling <[email protected]>

Merge branch 'main' into fix/bundle-store_custom-bundle_v0-compatible

12d0f0a

johanfylling merged commit bb12354 into open-policy-agent:main Feb 5, 2025
28 checks passed

johanfylling deleted the fix/bundle-store_custom-bundle_v0-compatible branch February 5, 2025 11:50

BrewTestBot mentioned this pull request Feb 28, 2025

opa 1.2.0 Homebrew/homebrew-core#209271

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bundle: Fixing issue where `--v0-compatible` isn't respected for custom bundles #7338

bundle: Fixing issue where `--v0-compatible` isn't respected for custom bundles #7338

johanfylling commented Feb 4, 2025

netlify bot commented Feb 4, 2025 •

edited

Loading

johanfylling left a comment

johanfylling Feb 4, 2025

philipaconrad Feb 4, 2025 •

edited

Loading

johanfylling Feb 4, 2025

johanfylling Feb 4, 2025

johanfylling Feb 4, 2025

philipaconrad left a comment •

edited

Loading

philipaconrad Feb 4, 2025 •

edited

Loading

johanfylling Feb 5, 2025

philipaconrad Feb 4, 2025 •

edited

Loading

philipaconrad Feb 4, 2025

johanfylling Feb 5, 2025

		} else if regoVersion != ast.RegoUndefined {
		modulePopts.RegoVersion = regoVersion

bundle: Fixing issue where --v0-compatible isn't respected for custom bundles #7338

bundle: Fixing issue where --v0-compatible isn't respected for custom bundles #7338

Conversation

johanfylling commented Feb 4, 2025

netlify bot commented Feb 4, 2025 • edited Loading

✅ Deploy Preview for openpolicyagent ready!

johanfylling left a comment

Choose a reason for hiding this comment

johanfylling Feb 4, 2025

Choose a reason for hiding this comment

philipaconrad Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

johanfylling Feb 4, 2025

Choose a reason for hiding this comment

johanfylling Feb 4, 2025

Choose a reason for hiding this comment

johanfylling Feb 4, 2025

Choose a reason for hiding this comment

philipaconrad left a comment • edited Loading

Choose a reason for hiding this comment

philipaconrad Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

johanfylling Feb 5, 2025

Choose a reason for hiding this comment

philipaconrad Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

philipaconrad Feb 4, 2025

Choose a reason for hiding this comment

johanfylling Feb 5, 2025

Choose a reason for hiding this comment

bundle: Fixing issue where `--v0-compatible` isn't respected for custom bundles #7338

bundle: Fixing issue where `--v0-compatible` isn't respected for custom bundles #7338

netlify bot commented Feb 4, 2025 •

edited

Loading

philipaconrad Feb 4, 2025 •

edited

Loading

philipaconrad left a comment •

edited

Loading

philipaconrad Feb 4, 2025 •

edited

Loading

philipaconrad Feb 4, 2025 •

edited

Loading