feat: store and detect Rego language version per rule type (Phase 1)#6317
feat: store and detect Rego language version per rule type (Phase 1)#6317dakshhhhh16 wants to merge 1 commit intomindersec:mainfrom
Conversation
|
Coverage is 59.353% — dakshhhhh16:feat/rego-v1-dual-parse into mindersec:main. No base build found for |
dakshhhhh16
force-pushed
the
feat/rego-v1-dual-parse
branch
from
bb06f8c to
38914ef
Compare
evankanderson
left a comment
evankanderson
left a comment
There was a problem hiding this comment.
Thanks, this is looking pretty close, though I'll want to do some manual testing with flag on/off if you haven't already. I suspect that this is a one-way flag -- once you've enabled "allow rego v1 dual-parse", some (enumerable but not easily known) set of your ruletypes in the Minder database will stop evaluating correctly when the flag is disabled.
| @@ -313,7 +313,7 @@ func TestRuleType_Definition_Eval_Rego_Validate(t *testing.T) { | |||
There was a problem hiding this comment.
Can you change the name of this test to something like "valid v1 rego expression"?
dakshhhhh16
force-pushed
the
feat/rego-v1-dual-parse
branch
from
38914ef to
148f6cd
Compare
|
Damn! Even the security check didn't fail this time!. |
| // Try parsing as Rego V1 first; if that fails, fall back to V0. | ||
| // This allows both V0 and V1 policies to pass validation. | ||
| _, err := ast.ParseModuleWithOpts("minder-ruletype-def.rego", rego.Def, | ||
| ast.ParserOptions{RegoVersion: ast.RegoV0}) | ||
| ast.ParserOptions{RegoVersion: ast.RegoV1}) | ||
| if err != nil { | ||
| _, err = ast.ParseModuleWithOpts("minder-ruletype-def.rego", rego.Def, | ||
| ast.ParserOptions{RegoVersion: ast.RegoV0}) | ||
| } | ||
| if err != nil { | ||
| return fmt.Errorf("%w: rego definition is invalid: %s", ErrInvalidRuleTypeDefinition, err) | ||
| } |
There was a problem hiding this comment.
We should move this validation to ruletypes/service.go, rather than having it happen on the client as well as the server.
Both CreateRuleType and UpdateRuleType should return an error if the rego_v1_dual_parse flag is false and the user supplies a string that can only be parsed in the V1 dialect. (We should add a test for this, either in ruletypes/services_test.go or in controlplane/handlers_ruletype_test.go)
We want to move the validation of the rego language out of the protobuf validator, because doing the validation on the client as well as the server means that users will need to update their minder binary to take advantage of new features -- we'd prefer to simply be able to light them up from the server side. (We've had this problem before for other features, like not needing to specify def: {} on each rule in a policy.)
dakshhhhh16
force-pushed
the
feat/rego-v1-dual-parse
branch
2 times, most recently
from
748c2bb to
a8fd600
Compare
…ec#5262) Phase 1 of the Rego V1 migration plan: - Add rego_version column to rule_type table (migration 000117) - Implement DetectRegoVersion() using parse-attempt detection (try V1 parser first, fall back to V0) - Store detected version in DB on rule type create/update - Wire stored version into Rego evaluator via WithRegoVersion option - Update validators to dual-parse (accept both V0 and V1 syntax) - Add rego_v1_dual_parse feature flag for gated rollout - Add comprehensive unit tests for version detection Signed-off-by: Daksh Pathak <daksh.pathak.ug24@nsut.ac.in> Signed-off-by: dakshhhhh16 <daksh.pathak.ug24@nsut.ac.in>
dakshhhhh16
force-pushed
the
feat/rego-v1-dual-parse
branch
from
a8fd600 to
7893de6
Compare
|
Updated! |
evankanderson
left a comment
evankanderson
left a comment
There was a problem hiding this comment.
This probably deserves a quick manual end-to-end test pass. Feel free to either post results, or ask me to run it when I get sufficient connectivity. (Beginning of next week -- I'm on a camping trip this week.)
| @@ -400,3 +456,5 @@ func getAvailableDataSources( | |||
|
|
|||
| return datasources, nil | |||
| } | |||
There was a problem hiding this comment.
Lint is complaining about an extra trailing newline.
3 participants
Parent Issue #5262
Sub-issue #6316
Problem
Minder's Rego evaluator unconditionally compiles all policies with
ast.RegoV0, even on OPA v1.14+ where the recommended default is V1. This blocks users from writing modern Rego (if,import rego.v1,every, etc.) and means there is no foundation for a gradual V1 migration.Approach
Instead of a big-bang migration, this phase establishes the necessary infrastructure:
rego_versioncolumn to therule_typetable (default'v0').CreateRuleType/UpdateRuleType, the server tries the V1 parser first. If it succeeds, stores'v1'; otherwise stores'v0'. No user action required.Validate()tries V1 first and falls back to V0, so both syntaxes pass.rego_v1_dual_parseis enabled, the evaluator reads the stored version instead of hardcoding V0.No existing behaviour changes without the feature flag.
Out of scope (future phases)
rego_versionin the API / CLIrego_v1_warn_v0)rego_v1_refuse_v0)