v1 SDK #7161

johanfylling · 2024-11-08T16:13:23Z

🚧 This is a draft PR for discussion 🚧

This PR employs two primary methods for extracting a v1 SDK while retaining old v0 behavior for existing users (no changes required to existing code):

complete move: where the full source is moved into the v1 SDK (factory functions enforce v1 behavior); and the "old" location is type aliased (factory functions enforce v0 behavior).
- v1.rego.Rego
- v1.sdk.OPA
proxy type: where the full source is moved to an internal package; the v1 SDK gets a proxy interface (factory functions enforce v1 behavior); and the "old" location is type aliased like above (factory functions enforce v0 behavior). Creating a proxy interface is more cumbersome to implement and maintain, but it has the benefit of hiding and abstracting the actual production code, making it easier for us to make future improvements and replacements without breaking the interfaces.
- ~~v1.compile.BundleCompiler~~
- v1.parser.Parser: Put outside the ast package to avoid users needing to do import aliasing when importing both ../v1/ast and ../ast.

Note: type aliasing breaks godoc generation, and requires users to click through on the aliased type to get documentation, which isn't a good developer experience. Because of this, I don't think we can have any type aliases in the v1 SDK, as that makes green field exploration clunkier; but it's something I think we can accept on the "old" v0 locations.

ast.Compiler is an exception to the above approaches: it only has a simple factory function in the v1 SDK. Similar to ast.Parser, it depends on types and fields internal to ast. The question is if we believe its used by enough users to warrant its own proxy type, or if it can be seen as "internal" enough to do without.

netlify · 2024-11-08T16:14:46Z

✅ Deploy Preview for openpolicyagent ready!

Name	Link
🔨 Latest commit	`8ff20f8`
🔍 Latest deploy log	https://app.netlify.com/sites/openpolicyagent/deploys/673603ad65020300085964e9
😎 Deploy Preview	https://deploy-preview-7161--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 · 2024-11-08T16:17:26Z

Hm, looks like I based the branch off of an old working branch. Will rebase on main.

ashutosh-narkar · 2024-11-13T01:30:02Z

ast/capabilities.go

@@ -97,6 +97,7 @@ func CapabilitiesForThisVersion() *Capabilities {
 		return f.Builtins[i].Name < f.Builtins[j].Name
 	})

+	// FIXME: In v1, this list should be empty


For ref: #6290

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

ashutosh-narkar · 2024-11-13T19:17:53Z

ast/parser_ext.go

@@ -689,6 +691,10 @@ func parseModule(filename string, stmts []Statement, comments []*Comment, regoCo

 	// The comments slice only holds comments that were not their own statements.
 	mod.Comments = append(mod.Comments, comments...)
+	if regoCompatibilityMode == RegoUndefined {
+		// We default to v0 behaviour for backwards compatibility reasons.


We default to v0 here as we have a v1 variant of the external parser.

ashutosh-narkar · 2024-11-13T19:21:42Z

ast/term.go

@@ -50,7 +50,8 @@ type Value interface {
 	Find(path Ref) (Value, error) // Find returns value referred to by path or an error if path is not found.
 	Hash() int                    // Returns hash code of the value.
 	IsGround() bool               // IsGround returns true if this value is not a variable or contains no variables.
-	String() string               // String returns a human readable string representation of the value.
+	// FIXME: Some String() impls depend on DefaultRegoVersion. Do we need a v1/ast/ValueToString()?
+	String() string // String returns a human readable string representation of the value.


Seems like it. At least it would be consistent.

ashutosh-narkar · 2024-11-13T19:25:08Z

cmd/build.go

@@ -17,9 +17,9 @@ import (
 	"github.com/open-policy-agent/opa/ast"


Shouldn't we have a v1/ast? Otherwise wouldn't ast.DefaultRegoVersion would be v0?

I'm not sure a v1/ast lends itself to a good developer experience. Most likely, if you're using v1/ast.DefaultRegoVersion, you're probably also using other ast types that don't exist in v1. Then you'd need to do aliased imports to not have conflicts:

import ( "github.com/open-policy-agent/opa/ast" v1ast "github.com/open-policy-agent/opa/v1/ast" )

If you already have github.com/open-policy-agent/opa/ast imported, auto-complete in your IDE would most likely bring you the wrong type.

We could maybe have a const DefaultRegoVersion = ast.RegoV1 directly in the v1 package, that ties down all rego-version usage for v1 internally 🤔.

E.g.:
opa/v1/rego_version.go:

package v1 import "github.com/open-policy-agent/opa/ast" const DefaultRegoVersion = ast.RegoV1

opa/v1/parser/parser.go:

... func NewParser(opts ...Option) Parser { p := ast.NewParser() options := Options{ p: p, } for _, opt := range opts { opt(&options) } if p.ParserOptions().RegoVersion == ast.RegoUndefined { p = p.WithRegoVersion(v1.DefaultRegoVersion) } return p }

This wouldn't cause any ast import conflicts for users should they choose to use it, and the v1.DefaultRegoVersion (or v1.RegoVersion even) makes it pretty clear what it is.

ast.DefaultRegoVersion is something of a misnomer, though as this doesn't apply to the SDK components, only to cmd/services that we don't see as part of the SDK ..

Doesn't look like Intellij/Goland can handle the opa/v1 package reference from a child package like opa/v1/rego 😞.

Command-line compilation doesn't seem to have any issues, though.

So to support those IDE:s for OPA development, we'd need to put this in some child pkg if we decide this is a good route to go down. But then we'll lose the nice free v1. prefix.

I raised this since we're using ast.DefaultRegoVersion. How is this correctly set?

ashutosh-narkar · 2024-11-13T19:25:54Z

cmd/build.go

@@ -309,14 +320,17 @@ func dobuild(params buildParams, args []string) error {
 		WithPartialNamespace(params.ns).
 		WithFollowSymlinks(params.followSymlinks)

-	regoVersion := ast.DefaultRegoVersion
+	regoVersion := ast.RegoUndefined
 	if params.v0Compatible {
 		// v0 takes precedence over v1


Is this still the case for v1?

Yes, technically still true; but we'd probably drop the params.v1Compatible check in 1.0.

Come to think of it, we could probably completely drop this check, as WithRegoVersion(params.regoVersion()) already does this for us 🤔.

ashutosh-narkar · 2024-11-13T23:49:39Z

profiler/profiler_bench_test.go

@@ -33,6 +33,7 @@ func BenchmarkProfilerBigLocalVar(b *testing.B) {
 				ctx := context.Background()

 				pq, err := rego.New(
+					rego.SetRegoVersion(ast.RegoV1),


Why not use ast.DefaultRegoVersion here as we do in other places?

This is a mistype. Should be ast.RegoV0, since the generated module is v0 Rego. I'll drop this and update the code generation instead.

ashutosh-narkar · 2024-11-13T23:56:10Z

rego/rego.go

-	regoVersion                 ast.RegoVersion
-}
+//
+// Deprecated: Use [v1.Rego] instead.


Hmm do we need deprecation labels for all the aliases?

I'm not sure. Could be unnecessary. But by deprecating the type itself, users' IDE:s will tell them they're using an outdated type. wherever that type is declared, and not just at instantiation (if we only deprecate the factory function).

ashutosh-narkar · 2024-11-13T23:57:38Z

rego/rego.go


 // RegisterBuiltin1 adds a built-in function globally inside the OPA runtime.
+//
+// Deprecated: Use the Compiler option in the [github.com/open-policy-agent/opa/v1] package instead.


Not sure I understand this comment here.

Hm, this should have been [github.com/open-policy-agent/opa/v1/rego].

I wanted to be general here, to not need to be specific for each function (e.g. [github.com/open-policy-agent/opa/v1/rego.RegisterBuiltin1]

Maybe something like this is more easy to understand(?):

Deprecated: Use the equivalent Compiler option in the [github.com/open-policy-agent/opa/v1/rego] package instead.

Or we just bite the bullet end give a specific link for each option:

Deprecated: Use [github.com/open-policy-agent/opa/v1/rego.RegisterBuiltin1] instead.

👇 seems more clear.

Deprecated: Use [github.com/open-policy-agent/opa/v1/rego.RegisterBuiltin1] instead.

ashutosh-narkar · 2024-11-14T00:17:11Z

Note: type aliasing breaks godoc generation, and requires users to click through on the aliased type to get documentation, which isn't a good developer experience.

If you search for the opa package in the godoc (not sure how many do), the first hit is this. With the complete move option (which seems like a good option from a maintenance pov) the functions on this page will require users to click another link to the v1 package, correct?

johanfylling · 2024-11-14T11:02:34Z

the functions on this page will require users to click another link to the v1 package, correct?

This page will not see any changes, as it's for the ast package, where nothing has moved and we instead have proxy types/factories in v1 (v1/compile.NewCompiler(), v1/parse.Parser)

But if we look in the sdk package, where the OPA type is located, where we in 0.x have a complete description of the OPA struct and all it's methods:

In 1.0 we instead get:

You need to expand the deprecated type description to see the New() factory function and to get the link for the v1 API:

Alternatively, we could drop the deprecated annotation on the OPA type, and only have it on the New() factory function:

This makes the New() factory more discoverable in the docs, I think, but has the downside of maybe not making it clear that you're using an outdated type when inside you're IDE unless you're looking at the factory.

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

ashutosh-narkar · 2024-11-15T00:59:42Z

the functions on this page will require users to click another link to the v1 package, correct?

This page will not see any changes, as it's for the ast package, where nothing has moved and we instead have proxy types/factories in v1 (v1/compile.NewCompiler(), v1/parse.Parser)

But if we look in the sdk package, where the OPA type is located, where we in 0.x have a complete description of the OPA struct and all it's methods:
In 1.0 we instead get: You need to expand the deprecated type description to see the `New()` factory function and to get the link for the `v1` API: Alternatively, we could drop the `deprecated` annotation on the `OPA` type, and only have it on the `New()` factory function: This makes the `New()` factory more discoverable in the docs, I think, but has the downside of maybe not making it clear that you're using an outdated type when inside you're IDE unless you're looking at the factory.

The current state lgtm.

srenatus · 2024-11-15T10:20:17Z

Note: type aliasing breaks godoc generation, and requires users to click through on the aliased type to get documentation, which isn't a good developer experience. Because of this, I don't think we can have any type aliases in the v1 SDK, as that makes green field exploration clunkier; but it's something I think we can accept on the "old" v0 locations.

I think that's OK. Going forward, the DX of v1 should be the priority.

Do you two think that we should put some notes somewhere to calm people? I'm wondering about the notion of "Deprecation" and potential reactions along the lines of "OMG the function will be deleted, I need to work on this or we'll have to drop OPA now because it's so much work!" What I mean is, what does "deprecation of v0" mean for our users? Do they have a way to find out the answer themselves?

srenatus · 2024-11-15T12:36:23Z

I'm a little hesitant about the top-level v1/ directory, because it seems to overlap with how golang does major revisions of Go modules. See these docs for example. Now, everything without a "vX" branch is considered "v1", so there should be no problem with having v1/ be a top-level directory. But if there ever was a v2, and someone tried to import

import "github.com/open-policy-agent/opa/v2/sdk"

then I believe the go mod infrastructure would attempt to download code from the "v2" branch of the "github.com/open-policy-agent/opa" repo, and look for it in the top-level "sdk" folder.

There's a variant of this where you can have v1 and v2 side-by-side, as top-level folders... I'm not sure about all this; it's not someting I've tried before. But I suppose to be sure, it would make sense to have a small test program that'll import and use the new packages.

srenatus · 2024-11-15T12:52:20Z

v1/parser/parser.go

+	options := Options{
+		p: p,
+	}
+
+	for _, opt := range opts {
+		opt(&options)
+	}


So we create options, apply options to them, and then discard it, because what we wanted to happen happened to the p inside options.

This looks funny. I mean, I see what's going on, and it's a clever way to provide "shadow" options of sorts...

var RegoVersion = ast.RegoVersion

This would probably also allow us to do

p := ast.NewParser() for _, opt := range opts { opt(p) }

? But I think it's not a huge advantage over what we have here, is it?

The idea was to not have any of the option functions leak "internal" types, and thereby make the v1 parser API look contained.

I'm not sure I understand your suggestion. You're saying Option should be type Option func(*ast.Parser)?

You're saying Option should be type Option func(*ast.Parser)?

Uhm yeah I guess that's what it would amount to. But it would leave us less room for changes in the future, so it's probably a bad suggestion.

johanfylling force-pushed the rego-v1/sdk branch from a679c17 to cf4dbe3 Compare November 8, 2024 16:36

ashutosh-narkar reviewed Nov 13, 2024

View reviewed changes

Moving SDK implementations to v1 pkg

79594df

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

johanfylling force-pushed the rego-v1/sdk branch from c52b291 to 79594df Compare November 13, 2024 12:26

Fixing linting issues

d19ce42

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

ashutosh-narkar reviewed Nov 14, 2024

View reviewed changes

johanfylling added 3 commits November 14, 2024 14:39

Fixing review issues

ff577b4

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

Merge branch 'main' into rego-v1/sdk

8ff20f8

Fixing rego wasm tests

ee146a2

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

johanfylling force-pushed the rego-v1/sdk branch from 3155b49 to ee146a2 Compare November 14, 2024 14:48

srenatus reviewed Nov 15, 2024

View reviewed changes

johanfylling changed the title ~~Rego v1/sdk~~ v1 SDK Nov 19, 2024

johanfylling mentioned this pull request Nov 19, 2024

v1 build-tag and import for side effects #7179

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

v1 SDK #7161

v1 SDK #7161

johanfylling commented Nov 8, 2024 •

edited

Loading

netlify bot commented Nov 8, 2024 •

edited

Loading

johanfylling commented Nov 8, 2024

ashutosh-narkar Nov 13, 2024

ashutosh-narkar Nov 13, 2024

johanfylling Nov 14, 2024

ashutosh-narkar Nov 13, 2024

ashutosh-narkar Nov 13, 2024

johanfylling Nov 14, 2024

johanfylling Nov 14, 2024

johanfylling Nov 14, 2024

johanfylling Nov 14, 2024

ashutosh-narkar Nov 15, 2024

ashutosh-narkar Nov 13, 2024

johanfylling Nov 14, 2024

ashutosh-narkar Nov 13, 2024

johanfylling Nov 14, 2024

ashutosh-narkar Nov 13, 2024

johanfylling Nov 14, 2024

ashutosh-narkar Nov 13, 2024

johanfylling Nov 14, 2024

ashutosh-narkar Nov 15, 2024

ashutosh-narkar commented Nov 14, 2024

johanfylling commented Nov 14, 2024

ashutosh-narkar commented Nov 15, 2024

srenatus commented Nov 15, 2024

srenatus commented Nov 15, 2024

srenatus Nov 15, 2024

johanfylling Nov 15, 2024

srenatus Nov 15, 2024

		@@ -17,9 +17,9 @@ import (
		"github.com/open-policy-agent/opa/ast"

v1 SDK #7161

Are you sure you want to change the base?

v1 SDK #7161

Conversation

johanfylling commented Nov 8, 2024 • edited Loading

netlify bot commented Nov 8, 2024 • edited Loading

✅ Deploy Preview for openpolicyagent ready!

johanfylling commented Nov 8, 2024

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

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 commented Nov 14, 2024

johanfylling commented Nov 14, 2024

ashutosh-narkar commented Nov 15, 2024

srenatus commented Nov 15, 2024

srenatus commented Nov 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johanfylling commented Nov 8, 2024 •

edited

Loading

netlify bot commented Nov 8, 2024 •

edited

Loading