opa icon indicating copy to clipboard operation
opa copied to clipboard

Parser does not respect capabilities file when using `opa eval`

Open johanfylling opened this issue 2 years ago • 8 comments
trafficstars

Given the policy:

package test

import future.keywords.if

p if {
   true
}

and a modified capabilities file, where we've removed the if keyword:

{
  "builtins": [
    {
      "name": "eq",
      "decl": {
        "args": [
          {
            "type": "any"
          },
          {
            "type": "any"
          }
        ],
        "result": {
          "type": "boolean"
        },
        "type": "function"
      },
      "infix": "="
    }
  ],
  "future_keywords": [
    "contains",
    "every",
    "in"
  ],
  "features": [
    "rule_head_ref_string_prefixes"
  ]
}

When running opa eval -d policy.rego --capabilities capabilities.json -fpretty 'data', we expect the error message:

1 error occurred: policy.rego:3: rego_parse_error: unexpected keyword, must be one of [contains every in]
	import future.keywords.if

but instead we get a successful evaluation:

{
  "test": {
    "p": true
  }
}

johanfylling avatar Nov 03 '23 09:11 johanfylling

Is this a bug, or by design?

The opa build command will respect the passed capabilities file. One could reason that it's only during commands like check and build we want to impose parser limitations on OPA, and the reason for eval taking a --capabilities flag is to inform it about what built-ins are available, not to restrict its parsing.

johanfylling avatar Nov 03 '23 09:11 johanfylling

If this isn't a bug, and should be corrected, this is where the capabilities are dropped.

johanfylling avatar Nov 03 '23 10:11 johanfylling

the reason for eval taking a --capabilities flag is to inform it about what built-ins are available, not to restrict its parsing.

If this was by design it should have been documented, no? Seems like a bug.

ashutosh-narkar avatar Nov 03 '23 16:11 ashutosh-narkar

Yes, it's a peculiar limitation if by design. And I'd honestly be surprised if anyone expected eval to behave differently than build and check. Especially since it's undocumented.

johanfylling avatar Nov 03 '23 16:11 johanfylling

I know I’m probably the only user of “opa parse”, but I guess it makes sense for that to have a capabilities flag added if that has an impact on parsing?

anderseknert avatar Nov 03 '23 17:11 anderseknert

it makes sense for that to have a capabilities flag added if that has an impact on parsing?

yes, I suppose so. All it'd do with the current set of capabilities we have is to stop it in its tracks and output an error. But it'd probably be a good thing to have it behave as the other commands.

johanfylling avatar Nov 03 '23 17:11 johanfylling

Agreed, but fine to ignore until someone else asks about it.

anderseknert avatar Nov 03 '23 17:11 anderseknert

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days. Although currently inactive, the issue could still be considered and actively worked on in the future. More details about the use-case this issue attempts to address, the value provided by completing it or possible solutions to resolve it would help to prioritize the issue.

stale[bot] avatar Dec 03 '23 21:12 stale[bot]