Skip to content

Commit 93c1774

Browse files
johanfyllingjohanfylling
authored
ast: Adding rego_v1 feature to --v0-compatible capabilities (#7474)
to allow for using Rego v1 bundles in `opa build`/`check`/`eval`/`test`. Before this change, a bundle with `1` as `rego_version`/`file_rego_versions` would be rejected when evaluated with the `--v0-compatible` flag with the error: ``` rego_parse_error: illegal capabilities: rego_v1 feature required for parsing v1 Rego ``` This is fixed by adding the `rego_v1` feature to the `v0` default capabilities applied when using the `--v0-compatible` flag. Note: this allows OPA to accept Rego `v1` modules inside bundles, but modules without a specified Rego version, such as freestanding non-bundle modules or modules inside bundles with no specified Rego version, are parsed as `v0`. Signed-off-by: Johan Fylling <[email protected]>
1 parent 13831ee commit 93c1774

File tree

8 files changed

+285
-60
lines changed

8 files changed

+285
-60
lines changed

Diff for: ‎cmd/build_test.go

+130-47
Original file line numberDiff line numberDiff line change
@@ -1048,6 +1048,7 @@ func TestBuildBundleModeWithManifestRegoVersion(t *testing.T) {
10481048
expErrs []string
10491049
v0Compatible bool
10501050
v1Compatible bool
1051+
capabilities *ast.Capabilities
10511052
}{
10521053
{
10531054
note: "v0 bundle rego-version",
@@ -1181,8 +1182,47 @@ p[4] {
11811182
input.x == 1
11821183
}`,
11831184
},
1185+
expManifest: `{"revision":"","roots":["test1","test2"],"rego_version":0,"file_rego_versions":{"%ROOT%/bundle1/test2.rego":1,"%ROOT%/bundle2/test3.rego":1}}`,
1186+
},
1187+
{
1188+
note: "multiple bundles with different rego-versions, v0-compatible, no rego_v1 capabilities feature",
1189+
v0Compatible: true,
1190+
roots: []string{"bundle1", "bundle2"},
1191+
files: map[string]string{
1192+
"bundle1/.manifest": `{
1193+
"roots": ["test1"],
1194+
"rego_version": 0,
1195+
"file_rego_versions": {
1196+
"*/test2.rego": 1
1197+
}
1198+
}`,
1199+
"bundle1/test1.rego": `package test1
1200+
p[1] {
1201+
input.x == 1
1202+
}`,
1203+
"bundle1/test2.rego": `package test1
1204+
p contains 2 if {
1205+
input.x == 1
1206+
}`,
1207+
"bundle2/.manifest": `{
1208+
"roots": ["test2"],
1209+
"rego_version": 1,
1210+
"file_rego_versions": {
1211+
"*/test4.rego": 0
1212+
}
1213+
}`,
1214+
"bundle2/test3.rego": `package test2
1215+
p contains 3 if {
1216+
input.x == 1
1217+
}`,
1218+
"bundle2/test4.rego": `package test2
1219+
p[4] {
1220+
input.x == 1
1221+
}`,
1222+
},
1223+
capabilities: capsWithoutFeat(ast.RegoV0, ast.FeatureRegoV1),
11841224
expErrs: []string{
1185-
// capabilities inferred from --v0-compatible doesn't include rego_v1 feature, which must be respected
1225+
// capabilities doesn't include rego_v1 feature, which must be respected
11861226
"rego_parse_error: illegal capabilities: rego_v1 feature required for parsing v1 Rego",
11871227
},
11881228
},
@@ -1282,6 +1322,11 @@ p[4] {
12821322
params.v0Compatible = tc.v0Compatible
12831323
params.v1Compatible = tc.v1Compatible
12841324

1325+
if tc.capabilities != nil {
1326+
params.capabilities = newcapabilitiesFlag()
1327+
params.capabilities.C = tc.capabilities
1328+
}
1329+
12851330
if _, ok := tc.files["capabilities.json"]; ok {
12861331
_ = params.capabilities.Set(path.Join(root, "capabilities.json"))
12871332
}
@@ -1354,6 +1399,27 @@ p[4] {
13541399
}
13551400
}
13561401

1402+
func capsWithoutFeat(regoVersion ast.RegoVersion, feat ...string) *ast.Capabilities {
1403+
caps := ast.CapabilitiesForThisVersion(ast.CapabilitiesRegoVersion(regoVersion))
1404+
1405+
feats := make([]string, 0, len(caps.Features))
1406+
for _, f := range caps.Features {
1407+
skip := false
1408+
for _, skipF := range feat {
1409+
if f == skipF {
1410+
skip = true
1411+
break
1412+
}
1413+
}
1414+
if !skip {
1415+
feats = append(feats, f)
1416+
}
1417+
}
1418+
caps.Features = feats
1419+
1420+
return caps
1421+
}
1422+
13571423
func TestBuildBundleFromOtherBundles(t *testing.T) {
13581424
type bundleInfo map[string]string
13591425

@@ -1623,37 +1689,6 @@ p {
16231689
"policy_1.rego": `package test
16241690
q contains 1 if {
16251691
input.x == 1
1626-
}`,
1627-
},
1628-
},
1629-
expErrs: []string{
1630-
// capabilities inferred from --v0-compatible doesn't include rego_v1 feature, which must be respected
1631-
"rego_parse_error: illegal capabilities: rego_v1 feature required for parsing v1 Rego",
1632-
},
1633-
},
1634-
{
1635-
note: "single v1 bundle, v0 per-file override, --v0-compatible, rego_v1 capabilities feature",
1636-
v0Compatible: true,
1637-
capabilities: func() *ast.Capabilities {
1638-
caps := ast.CapabilitiesForThisVersion(ast.CapabilitiesRegoVersion(ast.RegoV0))
1639-
caps.Features = append(caps.Features, ast.FeatureRegoV1)
1640-
return caps
1641-
}(),
1642-
bundles: map[string]bundleInfo{
1643-
"bundle.tar.gz": {
1644-
".manifest": `{
1645-
"rego_version": 1,
1646-
"file_rego_versions": {
1647-
"/policy_0.rego": 0
1648-
}
1649-
}`,
1650-
"policy_0.rego": `package test
1651-
p {
1652-
input.x == 1
1653-
}`,
1654-
"policy_1.rego": `package test
1655-
q contains 1 if {
1656-
input.x == 1
16571692
}`,
16581693
},
16591694
},
@@ -1677,37 +1712,35 @@ q contains 1 if {
16771712
},
16781713
},
16791714
{
1680-
note: "v0 bundle + v1 bundle, --v0-compatible",
1715+
note: "single v1 bundle, v0 per-file override, --v0-compatible, no rego_v1 capabilities feature",
16811716
v0Compatible: true,
1717+
capabilities: capsWithoutFeat(ast.RegoV0, ast.FeatureRegoV1),
16821718
bundles: map[string]bundleInfo{
1683-
"bundle_v0.tar.gz": {
1684-
".manifest": `{"roots": ["test1"], "rego_version": 0}`,
1685-
"policy.rego": `package test1
1719+
"bundle.tar.gz": {
1720+
".manifest": `{
1721+
"rego_version": 1,
1722+
"file_rego_versions": {
1723+
"/policy_0.rego": 0
1724+
}
1725+
}`,
1726+
"policy_0.rego": `package test
16861727
p {
16871728
input.x == 1
16881729
}`,
1689-
},
1690-
"bundle_v1.tar.gz": {
1691-
".manifest": `{"roots": ["test2"], "rego_version": 1}`,
1692-
"policy.rego": `package test2
1730+
"policy_1.rego": `package test
16931731
q contains 1 if {
16941732
input.x == 1
16951733
}`,
16961734
},
16971735
},
16981736
expErrs: []string{
1699-
// capabilities inferred from --v0-compatible doesn't include rego_v1 feature, which must be respected
1737+
// capabilities doesn't include rego_v1 feature, which must be respected
17001738
"rego_parse_error: illegal capabilities: rego_v1 feature required for parsing v1 Rego",
17011739
},
17021740
},
17031741
{
1704-
note: "v0 bundle + v1 bundle, --v0-compatible, rego_v1 capabilities feature",
1742+
note: "v0 bundle + v1 bundle, --v0-compatible",
17051743
v0Compatible: true,
1706-
capabilities: func() *ast.Capabilities {
1707-
caps := ast.CapabilitiesForThisVersion(ast.CapabilitiesRegoVersion(ast.RegoV0))
1708-
caps.Features = append(caps.Features, ast.FeatureRegoV1)
1709-
return caps
1710-
}(),
17111744
bundles: map[string]bundleInfo{
17121745
"bundle_v0.tar.gz": {
17131746
".manifest": `{"roots": ["test1"], "rego_version": 0}`,
@@ -1743,6 +1776,31 @@ q contains 1 if {
17431776
`,
17441777
},
17451778
},
1779+
{
1780+
note: "v0 bundle + v1 bundle, --v0-compatible, no rego_v1 capabilities feature",
1781+
v0Compatible: true,
1782+
capabilities: capsWithoutFeat(ast.RegoV0, ast.FeatureRegoV1),
1783+
bundles: map[string]bundleInfo{
1784+
"bundle_v0.tar.gz": {
1785+
".manifest": `{"roots": ["test1"], "rego_version": 0}`,
1786+
"policy.rego": `package test1
1787+
p {
1788+
input.x == 1
1789+
}`,
1790+
},
1791+
"bundle_v1.tar.gz": {
1792+
".manifest": `{"roots": ["test2"], "rego_version": 1}`,
1793+
"policy.rego": `package test2
1794+
q contains 1 if {
1795+
input.x == 1
1796+
}`,
1797+
},
1798+
},
1799+
expErrs: []string{
1800+
// capabilities inferred from --v0-compatible doesn't include rego_v1 feature, which must be respected
1801+
"rego_parse_error: illegal capabilities: rego_v1 feature required for parsing v1 Rego",
1802+
},
1803+
},
17461804
{
17471805
note: "v0 bundle + v1 bundle, --v1-compatible",
17481806
v1Compatible: true,
@@ -2098,6 +2156,20 @@ p[x] {
20982156
x := 42
20992157
}`,
21002158
},
2159+
expErrs: []string{
2160+
"test.rego:2: rego_parse_error: `if` keyword is required before rule body",
2161+
"test.rego:2: rego_parse_error: `contains` keyword is required for partial set rules",
2162+
},
2163+
},
2164+
{
2165+
note: "v0 module, not v0-compatible, v0 capabilities without rego_v1 feature",
2166+
capabilities: capsWithoutFeat(ast.RegoV0, ast.FeatureRegoV1),
2167+
files: map[string]string{
2168+
"test.rego": `package test
2169+
p[x] {
2170+
x := 42
2171+
}`,
2172+
},
21012173
expErrs: []string{
21022174
"rego_parse_error: illegal capabilities: rego_v1 feature required for parsing v1 Rego",
21032175
},
@@ -2188,6 +2260,17 @@ p contains x if {
21882260
files: map[string]string{
21892261
"test.rego": `package test
21902262
2263+
p contains x if {
2264+
x := 42
2265+
}`,
2266+
},
2267+
},
2268+
{
2269+
note: "v1 module, not v0-compatible, v0 capabilities without rego_v1 feature",
2270+
capabilities: capsWithoutFeat(ast.RegoV0, ast.FeatureRegoV1),
2271+
files: map[string]string{
2272+
"test.rego": `package test
2273+
21912274
p contains x if {
21922275
x := 42
21932276
}`,

Diff for: ‎cmd/capabilities_test.go

+1
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,7 @@ func TestCapabilitiesCurrent(t *testing.T) {
9999
ast.FeatureRefHeadStringPrefixes,
100100
ast.FeatureRefHeads,
101101
ast.FeatureRegoV1Import,
102+
ast.FeatureRegoV1,
102103
},
103104
expFutureKeywords: []string{
104105
"in",

Diff for: ‎cmd/check_test.go

+20
Original file line numberDiff line numberDiff line change
@@ -545,6 +545,18 @@ a[x] {
545545
policy: `package test
546546
a[x] {
547547
x := 42
548+
}`,
549+
expErrs: []string{
550+
"test.rego:2: rego_parse_error: `if` keyword is required before rule body",
551+
"test.rego:2: rego_parse_error: `contains` keyword is required for partial set rules",
552+
},
553+
},
554+
{
555+
note: "v0 module, not v0-compatible, v0 capabilities without rego_v1 feature",
556+
capabilities: capsWithoutFeat(ast.RegoV0, ast.FeatureRegoV1),
557+
policy: `package test
558+
a[x] {
559+
x := 42
548560
}`,
549561
expErrs: []string{
550562
"rego_parse_error: illegal capabilities: rego_v1 feature required for parsing v1 Rego",
@@ -612,6 +624,14 @@ a contains x if {
612624
policy: `package test
613625
a contains x if {
614626
x := 42
627+
}`,
628+
},
629+
{
630+
note: "v1 module, not v0-compatible, v0 capabilities without rego_v1 feature",
631+
capabilities: capsWithoutFeat(ast.RegoV0, ast.FeatureRegoV1),
632+
policy: `package test
633+
a contains x if {
634+
x := 42
615635
}`,
616636
expErrs: []string{
617637
"rego_parse_error: illegal capabilities: rego_v1 feature required for parsing v1 Rego",

Diff for: ‎cmd/eval_test.go

+31-6
Original file line numberDiff line numberDiff line change
@@ -3050,6 +3050,20 @@ func TestEvalPolicyWithRegoV1Capability(t *testing.T) {
30503050
1 < 2
30513051
}`,
30523052
},
3053+
expErrs: []string{
3054+
"test.rego:2: rego_parse_error: `if` keyword is required before rule body",
3055+
},
3056+
},
3057+
{
3058+
note: "v0 module, not v0-compatible, v0 capabilities without rego_v1 feature",
3059+
v0Compatible: false,
3060+
capabilities: capsWithoutFeat(ast.RegoV0, ast.FeatureRegoV1),
3061+
modules: map[string]string{
3062+
"test.rego": `package test
3063+
allow {
3064+
1 < 2
3065+
}`,
3066+
},
30533067
expErrs: []string{
30543068
"rego_parse_error: illegal capabilities: rego_v1 feature required for parsing v1 Rego",
30553069
},
@@ -3130,6 +3144,17 @@ func TestEvalPolicyWithRegoV1Capability(t *testing.T) {
31303144
1 < 2
31313145
}`,
31323146
},
3147+
},
3148+
{
3149+
note: "v1 module, not v0-compatible, v0 capabilities without rego_v1 feature",
3150+
v0Compatible: false,
3151+
capabilities: capsWithoutFeat(ast.RegoV0, ast.FeatureRegoV1),
3152+
modules: map[string]string{
3153+
"test.rego": `package test
3154+
allow if {
3155+
1 < 2
3156+
}`,
3157+
},
31333158
expErrs: []string{
31343159
"rego_parse_error: illegal capabilities: rego_v1 feature required for parsing v1 Rego",
31353160
},
@@ -3430,22 +3455,22 @@ p contains 2 if {
34303455
},
34313456
}
34323457

3433-
v1CompatibleFlagCases := []struct {
3458+
v0CompatibleFlagCases := []struct {
34343459
note string
34353460
used bool
34363461
}{
34373462
{
3438-
"no --v1-compatible", false,
3463+
"no --v0-compatible", false,
34393464
},
34403465
{
3441-
"--v1-compatible", true,
3466+
"--v0-compatible", true,
34423467
},
34433468
}
34443469

34453470
for _, bundleType := range bundleTypeCases {
3446-
for _, v1CompatibleFlag := range v1CompatibleFlagCases {
3471+
for _, v0CompatibleFlag := range v0CompatibleFlagCases {
34473472
for _, tc := range tests {
3448-
t.Run(fmt.Sprintf("%s, %s, %s", bundleType.note, v1CompatibleFlag.note, tc.note), func(t *testing.T) {
3473+
t.Run(fmt.Sprintf("%s, %s, %s", bundleType.note, v0CompatibleFlag.note, tc.note), func(t *testing.T) {
34493474
files := map[string]string{}
34503475

34513476
if bundleType.tar {
@@ -3476,7 +3501,7 @@ p contains 2 if {
34763501
}
34773502

34783503
params := newEvalCommandParams()
3479-
params.v1Compatible = v1CompatibleFlag.used
3504+
params.v0Compatible = v0CompatibleFlag.used
34803505
if err := params.bundlePaths.Set(p); err != nil {
34813506
t.Fatal(err)
34823507
}

0 commit comments

Comments
 (0)