v1 SDK - full move #7192

johanfylling · 2024-11-25T17:35:25Z

An alternative to the v1 SDK PR and the v0 global-toggle PR.

v1 SDK: Somewhat messy, where some parts of the go API is moved to v1, but some (most) aren't.
v0 global-toggle: Contentious:
- devs need to make code changes to remain on v0 behavior
- global toggle forces lib devs to not rely on default rego-version if there is a risk of competing OPA dependencies that rely on different default
- If default rego-version is set to v0 instead of v1, OPA 1.0 doesn't fully commit to one version, and will be stuck in the past.

This 3rd option moves all (with some exceptions) of OPA proper to the v1 package, and old root packages only contains type aliases and proxy functions.

Pros:

Cleaner interface than the original v1 SDK option
No global toggle; devs select what rego-version is the default through import paths

Cons:

Most packages, types, and functions will now have two options: one in the root for v0; and one under the v1 package for v1. Devs will need to make sure they're not mixing-and-matching their imports improperly. IDE auto-complete can lead you astray.
Somewhat larger maintainer burden (because of above point)

johanfylling · 2024-11-25T17:43:20Z

Conflicting files
ast/builtins.go
ast/index.go
ast/internal/scanner/scanner.go
ast/map.go
ast/term.go
debug/debugger.go
format/format.go
rego/rego.go
storage/disk/config_test.go
storage/disk/disk_test.go
storage/disk/partition_test.go
storage/disk/paths_test.go
topdown/builtins.go
topdown/crypto_test.go
topdown/errors_test.go
topdown/eval.go
topdown/eval_test.go
topdown/exported_test.go
topdown/http.go
topdown/http_test.go
topdown/input_test.go
topdown/json_test.go
topdown/jsonschema_test.go
topdown/lineage/lineage_test.go
topdown/object.go
...

Oh bother, this'll be fun 😩 ..

johanfylling · 2024-12-02T11:08:37Z

v1/doc.go

+// Package v1 implements the v1 API for the Open Policy Agent (OPA).
+// The v1 API defaults to enforcing the v1 Rego syntax ([github.com/open-policy-agent/opa/v1/ast.RegoV1]).
+// Most packages outside the v1 API are deprecated. These constitute the older v0 API, which defaults to the v0 Rego syntax ([github.com/open-policy-agent/opa/v1/ast.RegoV0]).
+// The v0 API is provided as a means to ease transition to OPA 1.0 for 3rd party integrations, see [TODO: LINK TO V0 MIGRATION GUIDE].


The v0 migration guide isn't completed yet. Whichever makes it into main last will need to add the link.

johanfylling · 2024-12-02T11:16:15Z

v1/doc.go

johanfylling · 2024-12-02T14:10:00Z

v1/ast/parser.go

+const DefaultRegoVersion = RegoV1
+
+const (
+	RegoUndefined RegoVersion = iota


This rego-version is the default value for any ast.RegoVersion struct field, e.g. ast.ParserOptions.RegoVersion and ast.Module.regoVersion. Which allows us to select whatever rego-version we should actually default to based on the user's imports.
E.g.:

opa/sdk/opa.go

Line 33 in 31b5f17

if opts.RegoVersion == ast.RegoUndefined {

opa/v1/ast/compile.go

Line 1757 in 8b531ca

if mod.regoVersion == RegoUndefined {

johanfylling · 2024-12-02T14:28:55Z

ast/compile.go

-
-	_, errs := checker.CheckTypes(c.TypeEnv, elems, as)
-	return errs
+	return v1.NewCompiler().WithDefaultRegoVersion(DefaultRegoVersion)


So, ast.DefaultRegoVersion == ast.RegoV0 in this package. I wonder if we should use ast.RegoV0 here instead (and other places in the v0 API), as that is a bit more explicit ..

johanfylling · 2024-12-02T14:36:07Z

v1/ast/builtins.go

This GH diff makes it look like this is a completely new file, but it looks like git actually knows better:

$ git blame v1/ast/builtins.go 4dc1c56552 ast/builtins.go (Torin Sandall 2016-05-16 14:21:32 -0700 1) // Copyright 2016 The OPA Authors. All rights reserved. 4dc1c56552 ast/builtins.go (Torin Sandall 2016-05-16 14:21:32 -0700 2) // Use of this source code is governed by an Apache2 4dc1c56552 ast/builtins.go (Torin Sandall 2016-05-16 14:21:32 -0700 3) // license that can be found in the LICENSE file. 4dc1c56552 ast/builtins.go (Torin Sandall 2016-05-16 14:21:32 -0700 4) 4dc1c56552 ast/builtins.go (Torin Sandall 2016-05-16 14:21:32 -0700 5) package ast 4dc1c56552 ast/builtins.go (Torin Sandall 2016-05-16 14:21:32 -0700 6) ea2ea9b12b ast/builtins.go (Torin Sandall 2017-10-03 14:15:07 -0700 7) import ( ea2ea9b12b ast/builtins.go (Torin Sandall 2017-10-03 14:15:07 -0700 8) "strings" ea2ea9b12b ast/builtins.go (Torin Sandall 2017-10-03 14:15:07 -0700 9) 67e8954b57 v1/ast/builtins.go (Johan Fylling 2024-11-21 16:35:26 +0100 10) "github.com/open-policy-agent/opa/v1/types" ea2ea9b12b ast/builtins.go (Torin Sandall 2017-10-03 14:15:07 -0700 11) ) bad1c9a2b2 ast/builtins.go (Torin Sandall 2017-03-15 16:06:00 -0700 12) 480281741b ast/builtins.go (Torin Sandall 2016-05-25 09:39:12 -0700 13) // Builtins is the registry of built-in functions supported by OPA. 480281741b ast/builtins.go (Torin Sandall 2016-05-25 09:39:12 -0700 14) // Call RegisterBuiltin to add a new built-in. 480281741b ast/builtins.go (Torin Sandall 2016-05-25 09:39:12 -0700 15) var Builtins []*Builtin 480281741b ast/builtins.go (Torin Sandall 2016-05-25 09:39:12 -0700 16) 480281741b ast/builtins.go (Torin Sandall 2016-05-25 09:39:12 -0700 17) // RegisterBuiltin adds a new built-in function to the registry. 480281741b ast/builtins.go (Torin Sandall 2016-05-25 09:39:12 -0700 18) func RegisterBuiltin(b *Builtin) { 480281741b ast/builtins.go (Torin Sandall 2016-05-25 09:39:12 -0700 19) Builtins = append(Builtins, b) 480281741b ast/builtins.go (Torin Sandall 2016-05-25 09:39:12 -0700 20) BuiltinMap[b.Name] = b 95bdd4c824 ast/builtins.go (Torin Sandall 2017-02-09 16:59:44 -0800 21) if len(b.Infix) > 0 { 95bdd4c824 ast/builtins.go (Torin Sandall 2017-02-09 16:59:44 -0800 22) BuiltinMap[b.Infix] = b 95bdd4c824 ast/builtins.go (Torin Sandall 2017-02-09 16:59:44 -0800 23) } 480281741b ast/builtins.go (Torin Sandall 2016-05-25 09:39:12 -0700 24) } 480281741b ast/builtins.go (Torin Sandall 2016-05-25 09:39:12 -0700 25) 480281741b ast/builtins.go (Torin Sandall 2016-05-25 09:39:12 -0700 26) // DefaultBuiltins is the registry of built-in functions supported in OPA 480281741b ast/builtins.go (Torin Sandall 2016-05-25 09:39:12 -0700 27) // by default. When adding a new built-in function to OPA, update this 4dc1c56552 ast/builtins.go (Torin Sandall 2016-05-16 14:21:32 -0700 28) // list. 480281741b ast/builtins.go (Torin Sandall 2016-05-25 09:39:12 -0700 29) var DefaultBuiltins = [...]*Builtin{ ab91356d5b ast/builtins.go (Torin Sandall 2018-01-27 09:44:08 -0800 30) // Unification/equality ("=") 4dc1c56552 ast/builtins.go (Torin Sandall 2016-05-16 14:21:32 -0700 31) Equality, 6f90081ea3 ast/builtins.go (Torin Sandall 2016-11-25 10:53:21 -0800 32) ab91356d5b ast/builtins.go (Torin Sandall 2018-01-27 09:44:08 -0800 33) // Assignment (":=") ab91356d5b ast/builtins.go (Torin Sandall 2018-01-27 09:44:08 -0800 34) Assign, ab91356d5b ast/builtins.go (Torin Sandall 2018-01-27 09:44:08 -0800 35) f8286be64e ast/builtins.go (Stephan Renatus 2021-10-14 09:02:38 +0200 36) // Membership, infix "in": `x in xs` f8286be64e ast/builtins.go (Stephan Renatus 2021-10-14 09:02:38 +0200 37) Member, f8286be64e ast/builtins.go (Stephan Renatus 2021-10-14 09:02:38 +0200 38) MemberWithKey, f8286be64e ast/builtins.go (Stephan Renatus 2021-10-14 09:02:38 +0200 39) 6f90081ea3 ast/builtins.go (Torin Sandall 2016-11-25 10:53:21 -0800 40) // Comparisons 0f8c6798f2 ast/builtins.go (Torin Sandall 2018-02-16 09:22:16 -0800 41) GreaterThan, 0f8c6798f2 ast/builtins.go (Torin Sandall 2018-02-16 09:22:16 -0800 42) GreaterThanEq, 0f8c6798f2 ast/builtins.go (Torin Sandall 2018-02-16 09:22:16 -0800 43) LessThan, 0f8c6798f2 ast/builtins.go (Torin Sandall 2018-02-16 09:22:16 -0800 44) LessThanEq, 0f8c6798f2 ast/builtins.go (Torin Sandall 2018-02-16 09:22:16 -0800 45) NotEqual, 0f8c6798f2 ast/builtins.go (Torin Sandall 2018-02-16 09:22:16 -0800 46) Equal, 6f90081ea3 ast/builtins.go (Torin Sandall 2016-11-25 10:53:21 -0800 47)

johanfylling · 2024-12-02T16:26:13Z

ast/doc.go

-//	                   +--- Expression (Term | Terms | Variable Declaration)
-//
-// At query time, the policy engine expects policies to have been compiled. The compilation stage takes one or more modules and compiles them into a format that the policy engine supports.
+// Deprecated: Use [github.com/open-policy-agent/opa/v1/ast] instead.


This will put a deprecation message at the top of the doc page for the package:

and it will cause linters (such as staticcheck used in OPA) to warn about usage of deprecated functions and types when those are referenced.

However, it won't put deprecated notices on anything inside the deprecated package in the generated docs. For this, we need to explicitly tag each type, const, var, and function. We could brute force it by just adding a generic

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

everywhere. But before doing that to thousands of places, we should determine if it's necessary.

Can web perhaps add some words, like "this package isn't going to go away but new projects are advised to start with v1"? If that's accurate. Just to avoid people panicking.

You mean "web" as in what's generated from here, or as in the separate v0-migration guide we're working on for the docs?

How about:

Deprecated: This package is intended for older projects transitioning from OPA v0.x and will remain for the lifetime of OPA v1.x, but its use is not recommended. For newer features and behaviours, such as defaulting to the Rego v1 syntax, use [github.com/open-policy-agent/opa/v1/ast] instead.

Can web perhaps add some words

typing with gloves on in harsh weather, sorry 😅 "Can we perhaps add..." is what I had meant.

I've updated all package deprecation notices to:

// Deprecated: This package is intended for older projects transitioning from OPA v0.x and will remain for the lifetime of OPA v1.x, but its use is not recommended. // For newer features and behaviours, such as defaulting to the Rego v1 syntax, use the corresponding components in the [github.com/open-policy-agent/opa/v1] package instead. // See https://www.openpolicyagent.org/docs/latest/v0-compatibility/ for more information.

johanfylling · 2024-12-04T13:17:59Z

v1/ast/builtins.go

+// built-in definitions.
+var BuiltinMap map[string]*Builtin
+
+// Deprecated: Builtins can now be directly annotated with the


Now would be our chance to get rid of all deprecated functions.

If we do it, should be done as a separate PR, though; otherwise, it'll just get buried in this already monstrous diff.

ast/doc.go

netlify · 2024-12-04T17:08:18Z

✅ Deploy Preview for openpolicyagent ready!

Name	Link
🔨 Latest commit	`ded2294`
🔍 Latest deploy log	https://app.netlify.com/sites/openpolicyagent/deploys/675985296082170009cbfcea
😎 Deploy Preview	https://deploy-preview-7192--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-12-04T17:10:40Z

docs/website/scripts/live-blocks/src/preprocess/localEval.js

@@ -69,7 +69,7 @@ export default async function localEval(groups, groupName, opaVersion) {
 // Returns 1st a function that consumes a string for the output format you want and produces an array of arguments and 2nd a map of module file names to strings that should be replaced in error messages. May throw a user-friendly error.
 async function prepEval(groups, groupName, opaVersion) {
  const {module, package: pkg, query, input, included} = getGroupData(groups, groupName)
-  const base = ['eval', '--fail'] // Fail on undefined
+  const base = ['eval', '--fail', '--v0-compatible'] // Fail on undefined


Temporary measure to deal with all live blocks not being v1 compliant yet.

johanfylling · 2024-12-05T09:58:03Z

$ git blame v1/topdown/cache/cache.go
f063c90275 topdown/cache/cache.go    (Rudrakh Panigrahi 2023-11-18 12:04:58 +0530 154) func NewInterQueryCacheWithContext(ctx context.Context, config *Config) InterQueryCache {
f063c90275 topdown/cache/cache.go    (Rudrakh Panigrahi 2023-11-18 12:04:58 +0530 155)  iqCache := newCache(config)
f063c90275 topdown/cache/cache.go    (Rudrakh Panigrahi 2023-11-18 12:04:58 +0530 156)  if iqCache.staleEntryEvictionTimePeriodSeconds() > 0 {
f063c90275 topdown/cache/cache.go    (Rudrakh Panigrahi 2023-11-18 12:04:58 +0530 157)          go func() {
61c7a93363 v1/topdown/cache/cache.go (Johan Fylling     2024-12-05 10:44:12 +0100 158)                  cleanupTicker := time.NewTicker(time.Duration(iqCache.staleEntryEvictionTimePeriodSeconds()) * time.Second)
f063c90275 topdown/cache/cache.go    (Rudrakh Panigrahi 2023-11-18 12:04:58 +0530 159)                  for {
f063c90275 topdown/cache/cache.go    (Rudrakh Panigrahi 2023-11-18 12:04:58 +0530 160)                          select {
f063c90275 topdown/cache/cache.go    (Rudrakh Panigrahi 2023-11-18 12:04:58 +0530 161)                          case <-cleanupTicker.C:
61c7a93363 v1/topdown/cache/cache.go (Johan Fylling     2024-12-05 10:44:12 +0100 162)                                  // NOTE: We stop the ticker and create a new one here to ensure that applications
61c7a93363 v1/topdown/cache/cache.go (Johan Fylling     2024-12-05 10:44:12 +0100 163)                                  // get _at least_ staleEntryEvictionTimePeriodSeconds with the cache unlocked;
61c7a93363 v1/topdown/cache/cache.go (Johan Fylling     2024-12-05 10:44:12 +0100 164)                                  // see https://github.com/open-policy-agent/opa/pull/7188/files#r1855342998
f063c90275 topdown/cache/cache.go    (Rudrakh Panigrahi 2023-11-18 12:04:58 +0530 165)                                  cleanupTicker.Stop()
f063c90275 topdown/cache/cache.go    (Rudrakh Panigrahi 2023-11-18 12:04:58 +0530 166)                                  iqCache.cleanStaleValues()
f063c90275 topdown/cache/cache.go    (Rudrakh Panigrahi 2023-11-18 12:04:58 +0530 167)                                  cleanupTicker = time.NewTicker(time.Duration(iqCache.staleEntryEvictionTimePeriodSeconds()) * time.Second)
f063c90275 topdown/cache/cache.go    (Rudrakh Panigrahi 2023-11-18 12:04:58 +0530 168)                          case <-ctx.Done():
f063c90275 topdown/cache/cache.go    (Rudrakh Panigrahi 2023-11-18 12:04:58 +0530 169)                                  cleanupTicker.Stop()
f063c90275 topdown/cache/cache.go    (Rudrakh Panigrahi 2023-11-18 12:04:58 +0530 170)                                  return
f063c90275 topdown/cache/cache.go    (Rudrakh Panigrahi 2023-11-18 12:04:58 +0530 171)                          }
f063c90275 topdown/cache/cache.go    (Rudrakh Panigrahi 2023-11-18 12:04:58 +0530 172)                  }
f063c90275 topdown/cache/cache.go    (Rudrakh Panigrahi 2023-11-18 12:04:58 +0530 173)          }()
7f65b04561 topdown/cache/cache.go    (Ashutosh Narkar   2020-07-24 17:54:13 -0700 174)  }
f063c90275 topdown/cache/cache.go    (Rudrakh Panigrahi 2023-11-18 12:04:58 +0530 175)
f063c90275 topdown/cache/cache.go    (Rudrakh Panigrahi 2023-11-18 12:04:58 +0530 176)  return iqCache
7f65b04561 topdown/cache/cache.go    (Ashutosh Narkar   2020-07-24 17:54:13 -0700 177) }

This is a bit annoying: every time I need to do a manual intervention when syncing with main, I become the owner of the change. Unless anyones git-fu is stronger than mine, I suppose this is just something to live with until we get this merged 😞.

srenatus

Just realised I forgot ✅ this.

anderseknert

I haven't read through every change 😅 But a more hands-on review in the form of integrating this branch with Regal. There's a few things we've identified as needed / good to have... but let's have this merged and deal with it after.

Moving (most) source to v1 root package to prepare for v0/v1 API separation. Signed-off-by: Johan Fylling <[email protected]>

All packages, except for `cmd` and `internal`, have been moved into a new `v1` root package. Old packages are kept for backwards-compatibility reasons. All contained code is replaced with simple type aliases and proxy functions to `v1` implementations. Old packages default to the Rego v0 syntax, new `v1` packages default to the Rego v1 syntax. Signed-off-by: Johan Fylling <[email protected]>

johanfylling · 2024-12-12T14:29:02Z

The content of this PR has been merged to main through #7214 and #7215.

johanfylling force-pushed the rego-v1/sdk_full_move branch 5 times, most recently from fff4204 to 8b531ca Compare November 28, 2024 17:21

johanfylling commented Dec 2, 2024

View reviewed changes

v1/doc.go

Copy link

Contributor Author

johanfylling Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

johanfylling force-pushed the rego-v1/sdk_full_move branch from 544a26a to fdb173d Compare December 2, 2024 11:31

johanfylling commented Dec 2, 2024

View reviewed changes

johanfylling requested review from srenatus and ashutosh-narkar December 2, 2024 15:21

johanfylling commented Dec 2, 2024

View reviewed changes

johanfylling mentioned this pull request Dec 3, 2024

Update docs and server binding addr per OPA v1.0 specs #7140

Merged

johanfylling commented Dec 4, 2024

View reviewed changes

srenatus reviewed Dec 4, 2024

View reviewed changes

ast/doc.go Show resolved Hide resolved

johanfylling commented Dec 4, 2024

View reviewed changes

This was referenced Dec 6, 2024

v1 build-tag and import for side effects #7179

Closed

v1 SDK #7161

Closed

johanfylling force-pushed the rego-v1/sdk_full_move branch from e80e6c2 to 925583a Compare December 6, 2024 17:04

srenatus previously approved these changes Dec 9, 2024

View reviewed changes

johanfylling dismissed srenatus’s stale review via 32690be December 9, 2024 14:38

anderseknert previously approved these changes Dec 12, 2024

View reviewed changes

johanfylling added 2 commits December 12, 2024 13:48

Preparing for v1 API

2ba1ed7

Moving (most) source to v1 root package to prepare for v0/v1 API separation. Signed-off-by: Johan Fylling <[email protected]>

johanfylling dismissed anderseknert’s stale review via a3f090c December 12, 2024 12:56

johanfylling force-pushed the rego-v1/sdk_full_move branch from ded2294 to a3f090c Compare December 12, 2024 12:56

anderseknert approved these changes Dec 12, 2024

View reviewed changes

This was referenced Dec 12, 2024

Preparing for v1 API #7214

Merged

v1 API #7215

Merged

johanfylling closed this Dec 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

v1 SDK - full move #7192

v1 SDK - full move #7192

johanfylling commented Nov 25, 2024

johanfylling commented Nov 25, 2024

johanfylling Dec 2, 2024

johanfylling Dec 2, 2024

johanfylling Dec 2, 2024

johanfylling Dec 2, 2024

johanfylling Dec 2, 2024

johanfylling Dec 2, 2024

srenatus Dec 2, 2024

johanfylling Dec 2, 2024

srenatus Dec 2, 2024

johanfylling Dec 4, 2024

johanfylling Dec 4, 2024

netlify bot commented Dec 4, 2024 •

edited

Loading

johanfylling Dec 4, 2024

johanfylling commented Dec 5, 2024 •

edited

Loading

srenatus left a comment

anderseknert left a comment

johanfylling commented Dec 12, 2024

v1 SDK - full move #7192

v1 SDK - full move #7192

Conversation

johanfylling commented Nov 25, 2024

johanfylling commented Nov 25, 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

netlify bot commented Dec 4, 2024 • edited Loading

✅ Deploy Preview for openpolicyagent ready!

Choose a reason for hiding this comment

johanfylling commented Dec 5, 2024 • edited Loading

srenatus left a comment

Choose a reason for hiding this comment

anderseknert left a comment

Choose a reason for hiding this comment

johanfylling commented Dec 12, 2024

netlify bot commented Dec 4, 2024 •

edited

Loading

johanfylling commented Dec 5, 2024 •

edited

Loading