Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: New future.strict import to prepare for future keywords being on by default #6247

Closed
tsandall opened this issue Sep 25, 2023 · 4 comments · Fixed by #6285
Closed

RFC: New future.strict import to prepare for future keywords being on by default #6247

tsandall opened this issue Sep 25, 2023 · 4 comments · Fixed by #6285

Comments

@tsandall
Copy link
Member

tsandall commented Sep 25, 2023

The if, contains, every, and in keywords currently require opt-in to prevent them from breaking existing policies. Eventually we would like to have those keywords be on by default. To get there, we could introduce a new future.strict import that by default:

  • Enables in and every
  • Enables and requires if and contains
  • Enables the strict mode checks that are defined today

Requiring if and contains solves the ambiguity between rules like a.b { true } vs. a.b.c { true }. Going forward it would mean:

  • All rules are single-value by default. When the value is omitted from the head, it defaults to true.
  • To make rules multi-value, use the contains keyword to convert the value into a set.

It'll take some time to adjust but this is conceptually simpler than what we have today with so-called complete rules, partial sets, partial objects, and differences in semantics depending on how many dots there are in the name (e.g., today x.y { ... } declares a set at at path data.x and x.y.z { ... } declares a key/value at path data.x.y.z.)

It would be nice if there was a way for users to easily migrate, e.g,. opa fmt --enable-strict ... which would rewrite the policy to be compatible (and include the import).

Once future.strict has been released for a while and we see sufficient usage, we could make the switch. One thing that would be good to know is the minimum version required to support these keywords.

I'd love to get comments/feedback on this plan.

@tsandall tsandall changed the title New future.strict import to prepare for future keywords being on by default RFC: New future.strict import to prepare for future keywords being on by default Sep 29, 2023
@anderseknert
Copy link
Member

I'm in favor!

To further accelerate the switch, it would be helpful if these constructs (contains, if) were surfaced in the AST, so that we could have tools like Regal flag when they're not used. OTOH, I suppose we could just add a rule to require the future.strict import once that exists..

Nit, but I take it every is included in this too despite not getting mentioned? :)

@johanfylling
Copy link
Contributor

In the future, when it's time for OPA 2.0, we might want to apply the same strategy; forcing you to use new keywords, syntax, etc if you import future.strict. This would however probably break older policies that had forced compliance with OPA 1.0. To avoid this, we could version the import with e.g. future.strict.v1; which then wouldn't conflict with future.strict.v2.

We could of course push this decision on the future, by letting future.strict stand for 1.0, and start this practice with 2.0.

@johanfylling
Copy link
Contributor

Nit, but I take it every is included in this too despite not getting mentioned? :)

This is a good point! I suppose what we're saying is that if you import future.strict, your policy will be OPA 1.0 compliant. For that to be true, we'd either need to keep the future.keyword.* imports in OPA 1.0 even though they'll be no-ops, or we'd need to make future.strict imply future.keyword, and such imports must be rejected.

@johanfylling johanfylling self-assigned this Oct 4, 2023
@johanfylling
Copy link
Contributor

Strict-mode enforces 5 checks:

  1. duplicate imports
  2. unused imports
  3. unused local assignments
  4. shadowing of input and data keywords
  5. use of deprecated built-ins

Letting the future.strict import enforce 1, 4, and 5 shouldn't be contentious. 2 and 3 could amount to poor developer UX, though; as you might declare imports and local variables during active development of a policy, knowing that they will eventually be used. Letting the developer to opt-in to 2 and 3 using the --strict flag even post OPA 1.0, would possibly cause less friction during development. Alternatively, we could have a means to opt-out of these checks.

johanfylling added a commit to johanfylling/opa that referenced this issue Oct 6, 2023
johanfylling added a commit to johanfylling/opa that referenced this issue Oct 6, 2023
johanfylling added a commit to johanfylling/opa that referenced this issue Oct 9, 2023
Fixes: open-policy-agent#6247
Signed-off-by: Johan Fylling <[email protected]>
johanfylling added a commit to johanfylling/opa that referenced this issue Oct 9, 2023
* duplicate imports
* `input` & `data` shadowing
* deprecated imports

Fixes: open-policy-agent#6247
Signed-off-by: Johan Fylling <[email protected]>
ashutosh-narkar pushed a commit to johanfylling/opa that referenced this issue Oct 17, 2023
ashutosh-narkar pushed a commit to johanfylling/opa that referenced this issue Oct 17, 2023
ashutosh-narkar pushed a commit to johanfylling/opa that referenced this issue Oct 17, 2023
ashutosh-narkar pushed a commit to johanfylling/opa that referenced this issue Oct 17, 2023
* duplicate imports
* `input` & `data` shadowing
* deprecated imports

Fixes: open-policy-agent#6247
Signed-off-by: Johan Fylling <[email protected]>
ashutosh-narkar pushed a commit to johanfylling/opa that referenced this issue Oct 17, 2023
ashutosh-narkar pushed a commit to johanfylling/opa that referenced this issue Oct 17, 2023
ashutosh-narkar pushed a commit to johanfylling/opa that referenced this issue Oct 17, 2023
ashutosh-narkar pushed a commit to johanfylling/opa that referenced this issue Oct 17, 2023
* duplicate imports
* `input` & `data` shadowing
* deprecated imports

Fixes: open-policy-agent#6247
Signed-off-by: Johan Fylling <[email protected]>
johanfylling added a commit that referenced this issue Oct 20, 2023
Adding `future.compat` import for enforcing strict-mode checks and additional `1.0` behavior for the module.

Fixes: #6247
Signed-off-by: Johan Fylling <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging a pull request may close this issue.

3 participants