Add minimum compatible version computation to compiler #6348

tsandall · 2023-10-26T14:58:16Z

This PR adds support for computing the minimum compatible OPA version for a given policy. See commit messages for details.

netlify · 2023-10-26T14:59:38Z

✅ Deploy Preview for openpolicyagent ready!

Name	Link
🔨 Latest commit	`06387df`
🔍 Latest deploy log	https://app.netlify.com/sites/openpolicyagent/deploys/653fd59fbb0bb50008395e13
😎 Deploy Preview	https://deploy-preview-6348--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.

srenatus

Looks good to me! Just minor comments and questions.

ast/capabilities.go

srenatus · 2023-10-27T07:56:07Z

ast/compile.go

+		for _, rule := range c.Modules[name].Rules {
+			if len(rule.Head.Reference) >= 3 {
+				features[FeatureRefHeadStringPrefixes] = struct{}{}
+			}


#6334 ⚡ that'll need an update when we get the new feature string in.

Yep, I figured that'll be a good first test of the system.

srenatus · 2023-10-27T07:57:12Z

ast/compile.go

 	for name := range c.Modules {
+		c.imports[name] = c.Modules[name].Imports


Couldn't we just let them in the modules? 🤔

I couldn't find the reason for why we're stripping the imports out. I stopped looking after I found tsandall@c622662 from @johanfylling. @johanfylling do you recall why we're doing this?

It looks like this was previously part of the compile_stage_resolve_refs stage, and was probably moved into its own stage because the compile_stage_check_duplicate_imports stage still needed the imports to be present. If I disable the compile_stage_remove_imports stage, I see failing tests, so later stages seems to rely on the imports being removed ..

ast/compile.go

srenatus · 2023-10-27T08:06:38Z

ast/version_index.json

I feel like there's some overlap with the builtin metadata's "introduced in" field -- https://github.com/open-policy-agent/opa/blob/main/internal/cmd/genbuiltinmetadata/main.go#L62. Not a blocker, of course.

Hrmmm, there's definitely overlap. I didn't look at the details of the builtin_metadata.json file and assumed the index wouldn't be in the right format. I guess the latter is missing the keywords and features index. I can go either way on this.

Let's keep it decoupled. It's all build-time stuff, right?

srenatus · 2023-10-27T08:14:19Z

ast/compile.go

@@ -345,6 +350,7 @@ func NewCompiler() *Compiler {
 		{"CheckDeprecatedBuiltins", "compile_state_check_deprecated_builtins", c.checkDeprecatedBuiltins},
 		{"BuildRuleIndices", "compile_stage_rebuild_indices", c.buildRuleIndices},
 		{"BuildComprehensionIndices", "compile_stage_rebuild_comprehension_indices", c.buildComprehensionIndices},
+		{"BuildRequiredCapabilities", "compile_stage_build_required_capabilities", c.buildRequiredCapabilities},


Can you add this one to 544fd03#diff-067a842cc5e1a348551f4c7656120105acedafb2faac6e7b7ff57b75c5349570R1494? I don't think we'll care about the min req caps for wasm (and IR)

I can add it though I wonder if knowing the built-ins required by the policy may be useful in some cases?

Yeah it might be time to expose the desired stages in some way. 🤔

ashutosh-narkar

LGTM

anderseknert

How does this work when custom capabilities have been provided, as is often the case with tools built on top of OPA? If I've disabled e.g. http.send via capabilities, this function will show it as if it never was supported? If I've added a capability like a custom built-in, this function won't work as it's built on generated OPA-provided capabilities? Both are OK I guess. I'd like to understand nonetheless :)

Also — if the fixes to print are unrelated to this, should that not go in a separate PR? I suspect this could fix some open issue on __local__s emerging in print calls, but there's nothing linked.

tsandall · 2023-10-30T15:58:51Z

How does this work when custom capabilities have been provided, as is often the case with tools built on top of OPA? If I've disabled e.g. http.send via capabilities, this function will show it as if it never was supported?

Nothing has changed in the way the allowed capabilities are handled by the compiler. So I would just expect an instance of http.send in the policy to generate a compile error like it does today.

If I've added a capability like a custom built-in, this function won't work as it's built on generated OPA-provided capabilities? Both are OK I guess. I'd like to understand nonetheless :)

Right, the MinimalCompatibleVersion function won't be able to find a version. Besides forking OPA and adding your own capabilities files, we don't have a way to provide custom ones at build-time. That's probably OK.

Also — if the fixes to print are unrelated to this, should that not go in a separate PR? I suspect this could fix some open issue on __local__s emerging in print calls, but there's nothing linked.

I only included that in this PR because I needed that fix to properly implement the logic that determines whether a comprehension was modified. I could have submitted the fix beforehand and then based this PR off that one but it feels unnecessary IMO. It's mentioned in the commit log if anyone needs the context down the road.

I don't think this will fix whatever issues you've seen w/ generated vars emerging in print calls. That sounds like we're returning errors without reversing the var w/ the RewrittenVars map.

anderseknert · 2023-10-30T16:02:35Z

Thanks, Torin 👍

This commit adds support for computing the required capabilities on compile. Built-in function requirements are mostly handled by the type checker with the exception of special builtins that are rewritten by the compiler. Each of those built-ins have a dedicated test case. Future keywords and features are handled by a new stage on the compiler. Also, fix error handling around print rewriting safety check as it was silently dropping errors if there were multiple comprehensions in the expression and the last one was valid. The error would have eventually been caught by the type checker, so this just improves the error message in that case. message in that case. message in that case. message in that case. Signed-off-by: Torin Sandall <[email protected]>

This commit adds the required capabilities ot the output of the inspect operation. This allows users who are determined enough to lookup the capabilities for the bundle. This is just the MVP so only the JSON format will show the capabilities. Signed-off-by: Torin Sandall <[email protected]>

This commit adds the ability to determine the minimum compatible OPA version for a set of capabilities. This can be coupled with the capabilities generated by the compiler to determine the min. compatible version of a policy/bundle. The build has been extended to generate the version index that lets us quickly check the required version for each builtin/feature/keyword in the capabilities. Signed-off-by: Torin Sandall <[email protected]>

Signed-off-by: Torin Sandall <[email protected]>

tsandall force-pushed the min-compat branch from b288b74 to 3345278 Compare October 26, 2023 16:49

tsandall requested review from johanfylling, srenatus, philipaconrad and ashutosh-narkar and removed request for johanfylling and srenatus October 26, 2023 19:55

tsandall force-pushed the min-compat branch 4 times, most recently from 5dfb0b6 to 1e8cfc3 Compare October 26, 2023 21:46

srenatus approved these changes Oct 27, 2023

View reviewed changes

ashutosh-narkar approved these changes Oct 27, 2023

View reviewed changes

anderseknert reviewed Oct 27, 2023

View reviewed changes

tsandall added 4 commits October 30, 2023 09:10

cmd: fix inspect to omit unnecessary whitespace

06387df

Signed-off-by: Torin Sandall <[email protected]>

tsandall force-pushed the min-compat branch from 1e8cfc3 to 06387df Compare October 30, 2023 16:11

tsandall merged commit 8a8dd09 into open-policy-agent:main Oct 30, 2023
24 checks passed

tsandall mentioned this pull request Oct 31, 2023

ast: Adding rule_head_general_refs capabilities feature flag #6346

Merged

BrewTestBot mentioned this pull request Nov 30, 2023

opa 0.59.0 Homebrew/homebrew-core#156009

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add minimum compatible version computation to compiler #6348

Add minimum compatible version computation to compiler #6348

tsandall commented Oct 26, 2023

netlify bot commented Oct 26, 2023 •

edited

Loading

srenatus left a comment

srenatus Oct 27, 2023

tsandall Oct 27, 2023

srenatus Oct 27, 2023

tsandall Oct 27, 2023

johanfylling Oct 30, 2023

srenatus Oct 27, 2023

tsandall Oct 27, 2023

srenatus Oct 27, 2023

srenatus Oct 27, 2023

tsandall Oct 27, 2023

srenatus Oct 27, 2023

ashutosh-narkar left a comment

anderseknert left a comment

tsandall commented Oct 30, 2023

anderseknert commented Oct 30, 2023

		for name := range c.Modules {
		c.imports[name] = c.Modules[name].Imports

Add minimum compatible version computation to compiler #6348

Add minimum compatible version computation to compiler #6348

Conversation

tsandall commented Oct 26, 2023

netlify bot commented Oct 26, 2023 • edited Loading

✅ Deploy Preview for openpolicyagent ready!

srenatus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashutosh-narkar left a comment

Choose a reason for hiding this comment

anderseknert left a comment

Choose a reason for hiding this comment

tsandall commented Oct 30, 2023

anderseknert commented Oct 30, 2023

netlify bot commented Oct 26, 2023 •

edited

Loading