Ensure OPA strict mode compliance #429

anderseknert · 2023-10-30T19:23:49Z

Hi Gatekeeper friends! 👋😃

In order to ensure the Gatekeeper library works with future updates of OPA, and to better catch some common mistakes in Rego policy, this PR aims to fix all errors reported from opa check --strict src, and to make sure future changes are compliant with the OPA strict mode checks.

The errors raised from strict mode, and which are fixed with this PR, include:

input must not be shadowed/assigned
Unused function args — use wildcards (_) in their place
Unused local assignment
Deprecated built-in functions
- re_match -> regex.match
- all -> these were not needed
- any -> strings.any_prefix_match and strings.any_suffix_match
  as these were always used together with startswith/endswith

The by far most noisy fix was the first one, i.e. input getting shadowed in basically all tests. Changed to use inp for a name as in is of course another name we don't want to shadow 😅

The changes here do not change semantics of the code. I did however also make some additional fixes where e.g. the unused function args error showed that a function wasn't needed in the first place, and so on. Naturally, all tests pass as before.

Two things I'm not sure about:

The code introduces the strings.any_prefix_match and strings.any_suffix_match built-in functions in one policy. These were introduced in OPA v0.44.0, which is quite a few OPA and Gatekeeper versions ago. I could not find information on what the minimum version of OPA or Gatekeeper must be supported in the Rego code in this library. If needed, that change can be reverted.
https://github.com/open-policy-agent/gatekeeper-library mentions Versioning. Should I update the versions of each policy changed with this PR?

To make sure the code stays compliant, I've added the opa check --strict command to execute with the test.sh script, before the unit tests run.

anderseknert · 2023-10-30T20:02:25Z

Generating artifact hub content for  /home/runner/work/gatekeeper-library/gatekeeper-library/library/general/allowedrepos
panic: looks like template.yaml is updated but the version is not. Please update the 'metadata.gatekeeper.sh/version' annotation in the template.yaml source

I guess that answered my question about whether versioning was needed 😄
Am I correct to think a patch version bump would be needed for these changes?

.github/workflows/workflow.yaml

apeabody · 2023-10-30T20:16:55Z

Generating artifact hub content for  /home/runner/work/gatekeeper-library/gatekeeper-library/library/general/allowedrepos
panic: looks like template.yaml is updated but the version is not. Please update the 'metadata.gatekeeper.sh/version' annotation in the template.yaml source
I guess that answered my question about whether versioning was needed 😄 Am I correct to think a patch version bump would be needed for these changes?

Hi @anderseknert - Yup, assuming these are all backward compatible with no change in behavior, they can just be a patch version bumps.

anderseknert · 2023-10-30T20:51:03Z

@apeabody all versions now bumped accordingly, and the tests seemingly pleased.

artifacthub/library/general/allowedrepos/1.0.0/artifacthub-pkg.yml

apeabody

Thanks for the contribution @anderseknert!

A second maintainer will also need to review.

maxsmythe · 2023-10-30T23:04:35Z

@anderseknert

Quick question about not being able to override input. Gatekeeper's Rego driver relies on this behavior to get constraint arguments in a performant manner. We could switch to specifying the input directly for each individual constraint, but in (admittedly old) performance testing, that was found to be slower.

The question is... will the below linked Rego code eventually break?

https://github.com/open-policy-agent/frameworks/blob/3eb381ce6cbedf3c1adedf2bfb20aa5e491c5baa/constraint/pkg/client/drivers/rego/rego.go#L27-L37

anderseknert · 2023-10-30T23:23:30Z

@maxsmythe no, that's fine! You can still override input using with. You just can't shadow the global input variable itself, i.e. not input := { ... } but inp := { ... } (or any other name). Saying with input as inp or with input as { ... } is just doing what with is there for 🙂

maxsmythe · 2023-10-30T23:25:01Z

@anderseknert Awesome, thanks for clarifying!

.github/workflows/workflow.yaml

apeabody · 2023-11-03T19:53:04Z

@sozercan - We are now testing with both OPA versions, when convenient PTAL

apeabody · 2023-11-27T22:32:25Z

Hi @sozercan or @ritazh Please take a look when convenient - Thanks!

nilekhc

@anderseknert

Looks like unit tests are failing. Could you PTAL?

apeabody · 2023-12-06T19:33:34Z

@anderseknert

Looks like unit tests are failing. Could you PTAL?

Ahh, looks like it is the k8sdisallowinteractivetty template added in 2d99845

src/general/disallowinteractive/src_test.rego:4: rego_compile_error: variables must not shadow input (use a different variable name)
src/general/disallowinteractive/src_test.rego:9: rego_compile_error: variables must not shadow input (use a different variable name)
src/general/disallowinteractive/src_test.rego:14: rego_compile_error: variables must not shadow input (use a different variable name)
src/general/disallowinteractive/src_test.rego:19: rego_compile_error: variables must not shadow input (use a different variable name)
src/general/disallowinteractive/src_test.rego:24: rego_compile_error: variables must not shadow input (use a different variable name)
src/general/disallowinteractive/src_test.rego:29: rego_compile_error: variables must not shadow input (use a different variable name)
src/general/disallowinteractive/src_test.rego:34: rego_compile_error: variables must not shadow input (use a different variable name)
src/general/disallowinteractive/src_test.rego:39: rego_compile_error: variables must not shadow input (use a different variable name)
src/general/disallowinteractive/src_test.rego:44: rego_compile_error: variables must not shadow input (use a different variable name)

Hi Gatekeeper friends! 👋😃 In order to ensure the Gatekeeper library works with future updates of OPA, and to better catch some common mistakes in Rego policy in the future, this PR aims to fix all errors reported from `opa check --strict src`, and to make sure future changes are compliant with the OPA [strict mode](https://www.openpolicyagent.org/docs/latest/policy-language/#strict-mode) checks. The errors raised from strict mode, and which are fixed with this PR, include: * `input` must not be shadowed/assigned * Unused function args — use wildcards (`_`) in their place * Unused local assignment * Deprecated built-in functions * `re_match` -> `regex.match` * `all` -> these were not needed * `any` -> `strings.any_prefix_match` and `strings.any_suffix_match` as these were always used together with `startswith`/`endswith` The by far most noisy fix was the first one, i.e. `input` getting shadowed in basically all tests. Changed to use `inp` for a name as `in` is of course another name we don't want to shadow 😅 The changes here do not change semantics of the code. I did however also make some additional fixes where e.g. the unused function args error showed that a function wasn't needed in the first place, and so on. Naturally, all tests pass as before. Two things I'm not sure about: 1. The code introduces the `strings.any_prefix_match` and `strings.any_suffix_match` built-in functions in one policy. These were introduced in OPA v0.44.0, which is quite a few OPA and Gatekeeper versions ago. I could not find information on what the minimum version of OPA or Gatekeeper must be supported in the Rego code in this library. If needed, that change can be reverted. 2. https://github.com/open-policy-agent/gatekeeper-library mentions Versioning. Should I update the versions of each policy changed with this PR? To make sure the code stays compliant, I've added the `opa check --strict` command to execute with the `test.sh` script, before the unit tests run. Signed-off-by: Anders Eknert <[email protected]>

anderseknert · 2023-12-06T19:34:55Z

Yes, some new tests overloading input, as is now verboten. I just fixed that and pushed.

anderseknert requested a review from a team as a code owner October 30, 2023 19:23

anderseknert force-pushed the check-strict branch 3 times, most recently from 1839c6e to 1de0f97 Compare October 30, 2023 19:50

apeabody reviewed Oct 30, 2023

View reviewed changes

.github/workflows/workflow.yaml Outdated Show resolved Hide resolved

anderseknert force-pushed the check-strict branch from 1de0f97 to 1c8907a Compare October 30, 2023 20:36

apeabody reviewed Oct 30, 2023

View reviewed changes

artifacthub/library/general/allowedrepos/1.0.0/artifacthub-pkg.yml Outdated Show resolved Hide resolved

anderseknert force-pushed the check-strict branch from 1c8907a to 8de9c2e Compare October 30, 2023 21:00

apeabody approved these changes Oct 30, 2023

View reviewed changes

maxsmythe requested review from sozercan and ritazh October 30, 2023 22:57

sozercan reviewed Nov 1, 2023

View reviewed changes

.github/workflows/workflow.yaml Outdated Show resolved Hide resolved

apeabody reviewed Nov 3, 2023

View reviewed changes

.github/workflows/workflow.yaml Outdated Show resolved Hide resolved

anderseknert force-pushed the check-strict branch from 9a5dc4a to 348e7dc Compare November 3, 2023 19:33

apeabody requested a review from sozercan November 8, 2023 00:29

nilekhc approved these changes Dec 6, 2023

View reviewed changes

nilekhc reviewed Dec 6, 2023

View reviewed changes

anderseknert force-pushed the check-strict branch from 5a6a6cc to 5b44a5a Compare December 6, 2023 19:34

nilekhc approved these changes Dec 6, 2023

View reviewed changes

nilekhc merged commit 3f72d58 into open-policy-agent:master Dec 6, 2023
16 checks passed

anderseknert deleted the check-strict branch December 6, 2023 19:48

charlieegan3 mentioned this pull request Nov 6, 2024

OPA 1.0 Preparatory Work open-policy-agent/gatekeeper#3681

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure OPA strict mode compliance #429

Ensure OPA strict mode compliance #429

anderseknert commented Oct 30, 2023

anderseknert commented Oct 30, 2023

apeabody commented Oct 30, 2023 •

edited

Loading

anderseknert commented Oct 30, 2023

apeabody left a comment

maxsmythe commented Oct 30, 2023

anderseknert commented Oct 30, 2023

maxsmythe commented Oct 30, 2023

apeabody commented Nov 3, 2023

apeabody commented Nov 27, 2023

nilekhc left a comment

apeabody commented Dec 6, 2023

anderseknert commented Dec 6, 2023

Ensure OPA strict mode compliance #429

Ensure OPA strict mode compliance #429

Conversation

anderseknert commented Oct 30, 2023

anderseknert commented Oct 30, 2023

apeabody commented Oct 30, 2023 • edited Loading

anderseknert commented Oct 30, 2023

apeabody left a comment

Choose a reason for hiding this comment

maxsmythe commented Oct 30, 2023

anderseknert commented Oct 30, 2023

maxsmythe commented Oct 30, 2023

apeabody commented Nov 3, 2023

apeabody commented Nov 27, 2023

nilekhc left a comment

Choose a reason for hiding this comment

apeabody commented Dec 6, 2023

anderseknert commented Dec 6, 2023

apeabody commented Oct 30, 2023 •

edited

Loading