open-policy-agent / opa

Open Policy Agent (OPA) is an open source, general-purpose policy engine.
https://www.openpolicyagent.org
Apache License 2.0
9.64k stars 1.34k forks source link

Allow user-defined zero-argument functions in Rego #6315

Open johanfylling opened 1 year ago

johanfylling commented 1 year ago

It's possible to declare functions with zero arguments in Rego:

package example

foo() := 1

these are however treated as regular rules, and there is no semantic difference between the above policy and:

package example

foo := 1

This is cause for confusion, as the user likely expects foo() to behave as a function (e.g. not contribute to output document), but it will actually behave as a rule.

Alternatively, it should be a parser error to declare a zero-arg rule.

anderseknert commented 1 year ago

Alternatively, it should be a parser error to declare a zero-arg rule.

I would suggest taking a stand here instead. The title of this issue suggests as much :)

ashutosh-narkar commented 1 year ago

IMO unless we think there is an actual need for user-defined zero-argument functions, this should be a parser error.

johanfylling commented 1 year ago

I would suggest taking a stand here instead. The title of this issue suggests as much :)

My intention was to bring this up for discussion. But in retrospect, I should have done that as a comment, and not as part of the description.

anderseknert commented 1 year ago

Yeah, no worries @johanfylling 🙂

@ashutosh-narkar seeing these used in policy libraries working on Regal integrations is what brought this up. Longer discussion on Slack, but since that's only retained for 90 days, motivation summarized for posterity. The arguments for allowing zero-arity functions:

  1. Not having to explain why zero-arity functions won't work, as there isn't really a technical reason.
  2. Consistency with built-in functions (opa.runtime(), rego.metadata.rule(), etc)
  3. Allows “masking”, or making values "private" when evaluated as part of larger document.
ashutosh-narkar commented 1 year ago

Thanks for the context @anderseknert! This list looks reasonable.

anderseknert commented 11 months ago

Make sure to add #6314 as a testcase for when this gets implemented.