Fail on unknown METADATA attributes when using strict-mode

[johanfylling]

A danger of using custom annotations not organized into the custom annotation in a METADATA comment block is that your policy is vulnerable to future OPA updates. E.g. the following policy might be fine today:

package sample

# METADATA
# title: My rule
# severity: HIGH
deny {
  ...
}

but a future OPA version might introduce the severity annotation, which might affect policy execution.

A check in compiler strict-mode could be added, that'd cause a check command execution to fail if any unknown METADATA annotations is present.

[pauly4it]

Great idea! Before introducing this additional strict mode check, though, I would advocate for an option to disable/ignore this specific check (at least temporarily), perhaps via OPA config.

Reasoning: Some projects had implemented metadata before OPA officially introduced annotations and the custom metadata attribute. In larger projects, maintainers may still be transitioning their existing metadata to the OPA annotations format.

Once this metadata check is introduced, a project in the middle of this transition and currently running OPA in strict mode for other checks would now receive compiler errors. This would force the project maintainers to either:

1. Finish their metadata refactoring immediately (likely resulting in other planned project work being delayed), or
2. Run OPA without strict mode.

Neither of those scenarios is desirable. Providing a method to disable/ignore specific strict mode checks would allow projects more flexibility when using strict mode.

[stale[bot]]

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days.

[johanfylling]

Wouldn't the same line of arguing hold true for any new check added to strict mode? The idea behind strict mode is that it will reject any policy that isn't compatible with a future 1.0 release, which is why it's a single monolithic toggle. If we started adding sub-toggles to it, people would run the risk of never being on-par when the time comes, even though seemingly been running strict.

More fine grained checks lives more in linter space, IMO.

[anderseknert]

FWIW, this is an existing linter rule in Regal, and has been for quite some time.

One problem with the current annotation "syntax" (for lack of a better word), and one that will be excacerbated by this strictness check IMHO:

# METADATA
# title: foo
# TODO: remember to change this
# description: bar
package p

This would fail the strictness check (and by all means, the Regal rule — although that allows configuring ignores, strict mode doesn't), but it's obviously not wrong to want to comment on something in a metadata annotation.

I tried to address this by suggesting an alternative form for metadata annotations in #5402, which would also rid us of that COBOL/SQL-esque METADATA loudness:

## title: foo
#  TODO: remember to change this
## description: bar
package p

The more I've worked with metadata annotations, and it's been quite a lot in Regal and elsewhere, the more convinced I am this is a good idea.

[ashutosh-narkar]

The compact format seems nice. We would still fail on unknown metadata attributes in strict mode but now at least you can mix in comments. Looks like a good change. @johanfylling WDYT?

[johanfylling]

Alternatively, we flip it, and make double # signify a comment within metadata:

# METADATA
# title: foo
## TODO: remember to change this
# description: bar
package p

[anderseknert]

That'd solve the comment issue, but not the verbosity issue :)

See the discussion in the issue here. I liked @tsandall's suggestion of using a different comment "directive" for metadata annotations.

[johanfylling]

To accommodate projects that are already using custom 1st level annotations, we should provide a way to opt-out; as refactoring might be a big undertaking.

One way to accomplish that would be to introduce a new annotation for allowing unknown annotations outside of custom, e.g. allow-unknown-annotations (or something less verbose).

[anderseknert]

Closing this after discussion with @johanfylling. This check, and 70+ other linter rules to help enforce a more strict interpretation of Rego are already available in Regal, so adding it to strict mode would just be duplicating that logic.