v1 build-tag and import for side effects #7179

johanfylling · 2024-11-19T09:52:50Z

This is an alternative to the API/SDK changes in #7161.

This PR makes the following changes:

Updates the default rego-version to v1
Makes it possible to globally opt-in to v0 through
- opa_rego_v0 build tag
- _ "github.com/open-policy-agent/opa/features/rego_default_v0" import for side effects

Pros compared to alternative:

OPA 1.0 is natively v1
No effort to upgrade to v1
Single toggle to switch v0/v1
Cohesive API not fractured between v0 and v1 SDK:s.
- easier to maintain

Cons compared to alternative:

Library-makers that introduce transitive dependency to OPA need to either:
- set rego-version globally
  - vulnerable to library-user re-setting global rego-version default
- update OPA SDK usage to explicitly set rego-version
  - mostly by setting the appropriate rego-version option in factory/construction calls

For toggling which rego-version is default. This is groundwork that can be replaced/augmented with a `rego_v0` build tag in OPA v1.0. NOTE: This is a PoC proposal, not to be merged unless no other alternatives can be devised. Signed-off-by: Johan Fylling <[email protected]>

* Making `ast.DefaultRefoVersion` a func * conditioning `ast.defaultRegoVersion` on `opa_rego_v1` built tag Signed-off-by: Johan Fylling <[email protected]>

To be imported for side effects. Signed-off-by: Johan Fylling <[email protected]>

…rsion` Note: can't use ast.RegoVersion values directly because of circular dependencies. `DefaultRegoVersion` could alternatively be set to int-representation (manifest representation), but the required call to `ast.RegoVersionFromInt()` from `ast.DefaultRegoVersion()` is likely somewhat more costly. Signed-off-by: Johan Fylling <[email protected]>

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

johanfylling

Some notes.

johanfylling · 2024-11-19T09:55:53Z

ast/parser.go

@@ -45,6 +45,10 @@ const (
 	RegoV1
 )

+func DefaultRegoVersion() RegoVersion {


Breaking change.
Very unlikely anyone is using this, though.

Since DefaultRegoVersion was added so recently I think we can make this change confidently.

johanfylling · 2024-11-19T09:59:56Z

Makefile

@@ -112,7 +112,7 @@ install: generate
 	$(GO) install $(GO_TAGS) -ldflags $(LDFLAGS)

 .PHONY: test
-test: go-test wasm-test
+test: go-test go-test-v0 go-test-v0-import wasm-test


go-tests are basically run twice, once for v0 and once for v1. This has impact on PR GHA execution time. Instead, we could narrow the scope of tests run for v0. The benefit of keeping it like this is we get great test-coverage, and will know if we break the v0 opt-in.

johanfylling · 2024-11-19T10:03:55Z

internal/rego_default/rego_default_v1.go

+package rego_default
+
+// DefaultRegoVersion is the default Rego version (v1).
+var DefaultRegoVersion = 3 // enum value for ast.RegoV1


Because of circular import issues, we can't assign ast.RegoV1 directly.

Alternatively, we could set this to the manifest int representation (0 for v0, 1 for v1), but that would impose a slight conversion overhead on the read side.

to avoid underscores. Signed-off-by: Johan Fylling <[email protected]>

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

ast/parser.go

charlieegan3

LGTM, I think the benefits of this approach outweigh the downsides by a considerable margin. I think the OPA SDK is super powerful and I'd hope we can make V1 the default experience for it with this route.

srenatus

✔️ code looks good, thank you for trying thisa

srenatus · 2024-11-21T10:47:01Z

ast/compile.go

+	switch mod.regoVersion {
+	case RegoUndefined:
+		switch DefaultRegoVersion() {
+		case RegoV1, RegoV0CompatV1:
+			return true
+		default:
+			return false
+		}
+	}
+
+	return mod.regoV1Compatible()


[nit] the single-case switch could be an if:

Suggested change

switch mod.regoVersion {

case RegoUndefined:

switch DefaultRegoVersion() {

case RegoV1, RegoV0CompatV1:

return true

default:

return false

}

}

return mod.regoV1Compatible()

if mod.regoVersion == RegoUndefined {

switch DefaultRegoVersion() {

case RegoV1, RegoV0CompatV1:

return true

}

return false

}

return mod.regoV1Compatible()

...but this is code golf and doesn't matter 🙃

srenatus · 2024-11-21T10:47:20Z

ast/parser.go

@@ -17,6 +17,7 @@ import (
 	"strings"
 	"unicode/utf8"

+	"github.com/open-policy-agent/opa/internal/rego/version"


[nit] let's cluster this one with the other opa imports below

srenatus · 2024-11-21T10:49:09Z

features/rego/v0/rego_default_v0.go

+//
+// Usage (import side effects only):
+//
+//	import _ "github.com/open-policy-agent/opa/features/rego/v0


Suggested change

// import _ "github.com/open-policy-agent/opa/features/rego/v0

// import _ "github.com/open-policy-agent/opa/features/rego/v0 // default to rego v0

I think it's good practice to give a reason for underscore imports; let's give a good example.

srenatus · 2024-11-21T10:50:25Z

internal/rego/version/rego_default_v0.go

+package version
+
+// DefaultRegoVersion is the default Rego version (v0).
+var DefaultRegoVersion = 1 // enum value for ast.RegoV0


We cannot import ast because of circularity?

srenatus · 2024-11-21T10:51:06Z

internal/rego/version/rego_default_v1.go

+// Use of this source code is governed by an Apache2
+// license that can be found in the LICENSE file.
+
+//go:build !opa_rego_v0


[nit] the other file has the old-style // +build comment, too. I think we only need //go:build these days, don't we? 🤔

johanfylling added 6 commits October 23, 2024 10:36

Merge branch 'main' into rego-v1/build_tag

606141c

* Adding ast.RegoUndefined

f841ea7

* Making `ast.DefaultRefoVersion` a func * conditioning `ast.defaultRegoVersion` on `opa_rego_v1` built tag Signed-off-by: Johan Fylling <[email protected]>

Adding github.com/open-policy-agent/opa/internal/rego_default_v1

70dea52

To be imported for side effects. Signed-off-by: Johan Fylling <[email protected]>

Making v1 the default rego-version

aa04bdf

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

johanfylling commented Nov 19, 2024

View reviewed changes

johanfylling added 2 commits November 19, 2024 14:20

Refactoring package names

c13780e

to avoid underscores. Signed-off-by: Johan Fylling <[email protected]>

Fixing broken tests

4d29b44

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

charlieegan3 reviewed Nov 20, 2024

View reviewed changes

ast/parser.go Show resolved Hide resolved

charlieegan3 approved these changes Nov 20, 2024

View reviewed changes

Merge branch 'main' into rego-v1/build_tag

4e96509

srenatus approved these changes Nov 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

v1 build-tag and import for side effects #7179

v1 build-tag and import for side effects #7179

johanfylling commented Nov 19, 2024 •

edited

Loading

johanfylling left a comment

johanfylling Nov 19, 2024

charlieegan3 Nov 20, 2024

johanfylling Nov 19, 2024

johanfylling Nov 19, 2024

charlieegan3 left a comment

srenatus left a comment

srenatus Nov 21, 2024

srenatus Nov 21, 2024

srenatus Nov 21, 2024

srenatus Nov 21, 2024

srenatus Nov 21, 2024

	// import _ "github.com/open-policy-agent/opa/features/rego/v0
	// import _ "github.com/open-policy-agent/opa/features/rego/v0 // default to rego v0

v1 build-tag and import for side effects #7179

Are you sure you want to change the base?

v1 build-tag and import for side effects #7179

Conversation

johanfylling commented Nov 19, 2024 • edited Loading

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

charlieegan3 left a comment

Choose a reason for hiding this comment

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

johanfylling commented Nov 19, 2024 •

edited

Loading