Adding `--v1-compatible` flag to `opa build` and `opa eval` #6478

johanfylling · 2023-12-13T15:07:25Z

This PR implements a small subset of the commands in #6463; namely build and eval. I'm not including more commands here to keep the scope of the PR tight, in part to make it easier to review, but also so that implementation of the remaining commands can be parallelized between multiple developers.

When applied, the --rego-v1 flag will cause opa build and opa eval to behave strictly as they would in OPA 1.0 in regards to module parsing, compilation, and PE. The difference to the behavior of e.g. opa fmt --rego-v1 is that this strict 1.0 behavior allows for policies using the future keywords without first importing future.keywords or rego.v1. E.g.:

package example

allow if {
  input.x == 42
}

An open question is what this flag should be named. A couple of options:

--rego-v1: The current naming in this PR. This was selected because this flag already exists on the fmt and check commands. I'm now reevaluating this choice, as --rego-v1 on e.g. fmt means the policy will be formatted to be compatible with both Rego in OPA 0.x and 1.0, whereas for eval it enforces compatibility with only 1.0. In addition to this behavioral difference, reusing the --rego.v1 flag means that we can't provide exactly OPA 1.0 behavior for fmt and check in addition to this current v0/v1 compatibility.
--v1-compatible: This flag already exists in opa run, and will apply configuration default values as if OPA 1.0. E.g., the server defaults to listen to localhost instead of 0.0.0.0 (currently the only change). It could be argued that module processing is part of this "OPA 1.0 compatibility mode".
--rego-version=<version>: Allow the user to explicitly select what version/flavor of Rego to use. In this PR, we have internally have three flavors of Rego (we could allow the user to either select between no1 and no3; or all three, but that might become confusing):
1. rego-v0: The current, default Rego version.
2. rego-v0-compat-v1: A flavor of Rego compatible with both v0 and v1. What is currently enforced in opa check --rego-v1 and opa fmt --rego-v1.
3. rego-v1: The future Rego version of OPA 1.0.

netlify · 2023-12-13T15:08:46Z

✅ Deploy Preview for openpolicyagent ready!

Name	Link
🔨 Latest commit	`839fe01`
🔍 Latest deploy log	https://app.netlify.com/sites/openpolicyagent/deploys/6582b686953a7c0008f91940
😎 Deploy Preview	https://deploy-preview-6478--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 notes.

johanfylling · 2023-12-13T15:09:20Z

ast/parser.go

@@ -105,8 +114,8 @@ type ParserOptions struct {
 	FutureKeywords     []string
 	SkipRules          bool
 	JSONOptions        *astJSON.Options
+	RegoVersion        RegoVersion


This is a breaking change. We could also keep the old RegoV1Compatible parameter, but I think that'll just be confusing. It's unlikely anyone is using this yet, so I feel this isn't a huge transgression.

Should we deprecate the old one and remove it in a future release? I agree not many must be using this.

I've added it back and added a deprecation notice.

johanfylling · 2023-12-13T15:11:19Z

loader/loader.go

@@ -103,7 +103,11 @@ type FileLoader interface {
 	WithCapabilities(*ast.Capabilities) FileLoader
 	WithJSONOptions(*astJSON.Options) FileLoader

+	// WithRegoV1Compatible
+	// Deprecated: use WithRegoVersion instead


I think it's unlikely many users are using this already, so we can probably remove the WithRegoV1Compatible func now.

ashutosh-narkar · 2023-12-14T23:29:16Z

In terms of naming the flag --v1-compatible seems like a good option as we're clear that this strict OPA v1.0 behavior. Also we have a proposed --v0-compatible flag.

bundle/bundle.go

compile/compile.go

ast/parser.go

ashutosh-narkar · 2023-12-15T00:11:06Z

cmd/eval.go

+	if params.regoV1 {
+		regoArgs = append(regoArgs, rego.SetRegoVersion(ast.RegoV1))
+	}
+


For both eval and build, we need to document how the new flag works either in docs or the help text.

For sure 👍 . I'm thinking we should update the OPA 1.0 page.
It's not included here as I didn't want to spend time on writing that text before we locked down the flag name.

johanfylling · 2023-12-15T14:00:49Z

Let's go with --v1-compatible for flag name. It'll allow us to also add this functionality to fmt and check.

Fixes: open-policy-agent#6463 Signed-off-by: Johan Fylling <johan.dev@fylling.se>

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

…at-v1) Signed-off-by: Johan Fylling <johan.dev@fylling.se>

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

@ashutosh-narkar

…goVersion()` Suggested by @ashutosh-narkar Signed-off-by: Johan Fylling <johan.dev@fylling.se>

@ashutosh-narkar

Requested by @ashutosh-narkar Signed-off-by: Johan Fylling <johan.dev@fylling.se>

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

Adding `--v1-compatible` flag to `opa test` Fixes: open-policy-agent#6463 Signed-off-by: Johan Fylling <johan.dev@fylling.se>

Fixes: open-policy-agent#6463 Signed-off-by: Johan Fylling <johan.dev@fylling.se>

johanfylling · 2023-12-19T18:12:59Z

Added support for commands: test, fmt, and check.

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

ashutosh-narkar

This looks good. The test coverage is also nice. Few comments 👇 .

ashutosh-narkar · 2023-12-19T19:24:57Z

ast/parser.go

+	// RegoV1Compatible additionally affects the Rego version. Use EffectiveRegoVersion to get the effective Rego version.
+	RegoVersion RegoVersion
+	// RegoV1Compatible is equivalent to setting RegoVersion to RegoV0CompatV1.
+	// Setting RegoV1Compatible only has effect if RegoVersion is RegoV0.


Aren't we kind of changing the meaning of RegoV1Compatible? It's fine I guess since I don't expect this has much usage.

True. I've changed this to make RegoV1Compatible take precedence.

ashutosh-narkar · 2023-12-19T20:07:04Z

cmd/build_test.go

+`,
+			},
+			// Note: the rego.v1 import isn't added to the optimized module.
+			// This is ok, as the bundle was built with the --rego-v1 flag,


Do we mean the --v1-compatible flag or --rego-v1 flag?

ashutosh-narkar · 2023-12-19T20:17:05Z

cmd/fmt.go

@@ -206,6 +223,8 @@ func init() {
 	formatCommand.Flags().BoolVarP(&fmtParams.list, "list", "l", false, "list all files who would change when formatted")
 	formatCommand.Flags().BoolVarP(&fmtParams.diff, "diff", "d", false, "only display a diff of the changes")
 	formatCommand.Flags().BoolVar(&fmtParams.fail, "fail", false, "non zero exit code on reformat")
-	formatCommand.Flags().BoolVar(&fmtParams.regoV1, "rego-v1", false, "format as Rego v1")
+	addRegoV1FlagWithDescription(formatCommand.Flags(), &fmtParams.regoV1, false, "format as Rego v1")


Similar to opa check the resulting policy will also be compatible with the current OPA version?

Yes. Changing description.

ashutosh-narkar · 2023-12-19T20:28:34Z

format/format.go

@@ -26,7 +26,7 @@ type Opts struct {
 	// carry along their original source locations.
 	IgnoreLocations bool

-	RegoV1 bool
+	RegoVersion ast.RegoVersion


Probably need to deprecate RegoV1

ashutosh-narkar · 2023-12-19T20:31:52Z

rego/rego_test.go

@@ -1167,6 +1167,83 @@ func TestPrepareAndPartial(t *testing.T) {
 	}
 }

+func TestPartialWitRegoV1(t *testing.T) {


Nit: TestPartialWithRegoV1

ashutosh-narkar · 2023-12-19T20:33:01Z

rego/rego_test.go

+				t.Fatal(err)
+			}
+
+			fmt.Println(partialQuery)


Nit: We can remove this

ashutosh-narkar · 2023-12-19T20:34:15Z

rego/rego_test.go

+				"policy.rego": `package test
+				x contains y if { y := 1 }`,
+			},
+			expectedErr: "rego_parse_error: var cannot be used for rule name", // FIXME: Improve error message


Is this something to address in this PR?

I believe this might require a larger set of changes in the parser. Let's not delay this PR and risk it not getting in this release, and fix this in the next release instead.

ashutosh-narkar · 2023-12-19T20:38:33Z

docs/content/opa-1.md

+* `check`*
+* `fmt`*
+* `eval`
+* `test`


Do you think it would be helpful to add a short note about what the --v1-compatible flag does in each command. For example, in test it will parse modules/bundles per v1 syntax.

* Changing `ParserOptions.RegoV1Compatible` take precedence over `ParserOptions.RegoVersion` * Fixing comment in test * Updating `fmt --rego-v1` CLI description Signed-off-by: Johan Fylling <johan.dev@fylling.se>

* Changing `ParserOptions.RegoV1Compatible` take precedence over `ParserOptions.RegoVersion` * Fixing comment in test * Updating `fmt --rego-v1` CLI description * Adding back `Opts.RegoV1` and deprecating. * Making `Opts.RegoV1` take precedence over `Opts.RegoVersion` Signed-off-by: Johan Fylling <johan.dev@fylling.se>

* Changing `ParserOptions.RegoV1Compatible` take precedence over `ParserOptions.RegoVersion` * Fixing comment in test * Updating `fmt --rego-v1` CLI description * Adding back `Opts.RegoV1` and deprecating. * Making `Opts.RegoV1` take precedence over `Opts.RegoVersion` * `TestPartialWitRegoV1` -> `TestPartialWithRegoV1` Signed-off-by: Johan Fylling <johan.dev@fylling.se>

* Changing `ParserOptions.RegoV1Compatible` take precedence over `ParserOptions.RegoVersion` * Fixing comment in test * Updating `fmt --rego-v1` CLI description * Adding back `Opts.RegoV1` and deprecating. * Making `Opts.RegoV1` take precedence over `Opts.RegoVersion` * `TestPartialWitRegoV1` -> `TestPartialWithRegoV1` * removing `Println` in test Signed-off-by: Johan Fylling <johan.dev@fylling.se>

* Changing `ParserOptions.RegoV1Compatible` take precedence over `ParserOptions.RegoVersion` * Fixing comment in test * Updating `fmt --rego-v1` CLI description * Adding back `Opts.RegoV1` and deprecating. * Making `Opts.RegoV1` take precedence over `Opts.RegoVersion` * `TestPartialWitRegoV1` -> `TestPartialWithRegoV1` * removing `Println` in test * Updating docs with per-command behavioural descriptions for `--v1-compatible`. Signed-off-by: Johan Fylling <johan.dev@fylling.se>

johanfylling commented Dec 13, 2023

View reviewed changes

johanfylling requested a review from ashutosh-narkar December 14, 2023 14:59

ashutosh-narkar reviewed Dec 15, 2023

View reviewed changes

johanfylling changed the title ~~Adding --rego-v1 flag to opa build and opa eval~~ Adding --v1-compatible flag to opa build and opa eval Dec 15, 2023

johanfylling force-pushed the rego-v1/flag_on_all_commands branch 3 times, most recently from ecd0189 to ee3291d Compare December 18, 2023 15:23

johanfylling added 10 commits December 18, 2023 16:13

ast+cmd+rego: Adding --rego-v1 flag to opa eval

6808e80

Fixes: open-policy-agent#6463 Signed-off-by: Johan Fylling <johan.dev@fylling.se>

Adding --rego-v1 flag to opa build

56b1e54

Fixes: open-policy-agent#6463 Signed-off-by: Johan Fylling <johan.dev@fylling.se>

Formatting PE support modules to comply with rego-v1 when required

45e5fdb

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

Removing rego.v1 import when formatting for rego-v1 (not rego-v0-comp…

44e6772

…at-v1) Signed-off-by: Johan Fylling <johan.dev@fylling.se>

touch up

3da495e

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

Fixing linting issues

e38eb8a

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

Consolidating Bundle.FormatModules() and `Bundle.FormatModulesForRe…

46db9c2

…goVersion()` Suggested by @ashutosh-narkar Signed-off-by: Johan Fylling <johan.dev@fylling.se>

Adding descriptions to RegoVersion

1b2e632

Requested by @ashutosh-narkar Signed-off-by: Johan Fylling <johan.dev@fylling.se>

Using --v1-compatible flag instead of --rego-v1

a8e033b

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

Updating docs

b811b7b

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

ashutosh-narkar force-pushed the rego-v1/flag_on_all_commands branch from ee3291d to b811b7b Compare December 19, 2023 00:13

Reintroducing ParserOptions.RegoV1Compatible to avoid breaking change

b9559ad

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

johanfylling requested a review from ashutosh-narkar December 19, 2023 09:53

johanfylling added 3 commits December 19, 2023 15:35

cmd & tester

326e873

Adding `--v1-compatible` flag to `opa test` Fixes: open-policy-agent#6463 Signed-off-by: Johan Fylling <johan.dev@fylling.se>

Adding --v1-compatible flag to opa fmt

d818bd7

Fixes: open-policy-agent#6463 Signed-off-by: Johan Fylling <johan.dev@fylling.se>

Adding --v1-compatible flag to opa check

fdc4db6

Fixes: open-policy-agent#6463 Signed-off-by: Johan Fylling <johan.dev@fylling.se>

Making linter happy

93462a9

Signed-off-by: Johan Fylling <johan.dev@fylling.se>

ashutosh-narkar reviewed Dec 19, 2023

View reviewed changes

ashutosh-narkar previously approved these changes Dec 19, 2023

View reviewed changes

johanfylling added 5 commits December 20, 2023 10:19

Review modifications suggested by @ashutosh-narkar

d248201

* Changing `ParserOptions.RegoV1Compatible` take precedence over `ParserOptions.RegoVersion` * Fixing comment in test * Updating `fmt --rego-v1` CLI description Signed-off-by: Johan Fylling <johan.dev@fylling.se>

johanfylling dismissed ashutosh-narkar’s stale review via 839fe01 December 20, 2023 09:40

Merge branch 'main' into rego-v1/flag_on_all_commands

e5a5558

johanfylling merged commit 38c2f0c into open-policy-agent:main Dec 20, 2023
24 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding `--v1-compatible` flag to `opa build` and `opa eval` #6478

Adding `--v1-compatible` flag to `opa build` and `opa eval` #6478

johanfylling commented Dec 13, 2023

netlify bot commented Dec 13, 2023 •

edited

johanfylling left a comment

johanfylling Dec 13, 2023

ashutosh-narkar Dec 19, 2023

johanfylling Dec 19, 2023

johanfylling Dec 13, 2023

ashutosh-narkar commented Dec 14, 2023 •

edited

ashutosh-narkar Dec 15, 2023

johanfylling Dec 15, 2023

johanfylling commented Dec 15, 2023

johanfylling commented Dec 19, 2023

ashutosh-narkar left a comment

ashutosh-narkar Dec 19, 2023

johanfylling Dec 20, 2023

ashutosh-narkar Dec 19, 2023

ashutosh-narkar Dec 19, 2023

johanfylling Dec 20, 2023

ashutosh-narkar Dec 19, 2023

ashutosh-narkar Dec 19, 2023

ashutosh-narkar Dec 19, 2023

ashutosh-narkar Dec 19, 2023

johanfylling Dec 20, 2023

ashutosh-narkar Dec 19, 2023

Adding --v1-compatible flag to opa build and opa eval #6478

Adding --v1-compatible flag to opa build and opa eval #6478

Conversation

johanfylling commented Dec 13, 2023

netlify bot commented Dec 13, 2023 • edited

✅ Deploy Preview for openpolicyagent ready!

johanfylling 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

ashutosh-narkar commented Dec 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johanfylling commented Dec 15, 2023

johanfylling commented Dec 19, 2023

ashutosh-narkar 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

Adding `--v1-compatible` flag to `opa build` and `opa eval` #6478

Adding `--v1-compatible` flag to `opa build` and `opa eval` #6478

netlify bot commented Dec 13, 2023 •

edited

ashutosh-narkar commented Dec 14, 2023 •

edited