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

constraint: Update OPA to v1 #517

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

charlieegan3
Copy link

@charlieegan3 charlieegan3 commented Jan 23, 2025

This is an initial PoC to show what rego.v1 support might look.

The main thing I am unsure of is the regorewriter. This could have some issues even with v0 as I understand it. For example, some x in data.lib will not be rewritten correctly. Anyone with more knowledge on this component, it'd be interesting to better know how this is used and how it should work when supporting both v0 and v1.

Next steps, after this PR

The next obvious thing to consider here is that attempting parsing in more than one rego version is not ideal. This is done here since we will not know the source version of the rego until we try to parse it. We might consider something like this to allow users to force GK to use a given version.

apiVersion: templates.gatekeeper.sh/v1beta1
kind: ConstraintTemplate
metadata:
  name: k8srequiredlabels
spec:
  crd:
    spec:
      names:
        kind: K8sRequiredLabels
      validation:
        # Schema for the `parameters` field
        openAPIV3Schema:
          properties:
            labels:
              type: array
              items:
                type: string
  targets:
    - target: admission.k8s.gatekeeper.sh
      version: v1 <------ this, or something like it.
      rego: |
        package k8srequiredlabels

        violation contains {"msg": msg, "details": {"foo": "bar"}} if {
          ...
        }

@codecov-commenter
Copy link

codecov-commenter commented Jan 23, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 53.80%. Comparing base (76869f8) to head (8a35958).
Report is 43 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #517      +/-   ##
==========================================
- Coverage   54.68%   53.80%   -0.89%     
==========================================
  Files          71       97      +26     
  Lines        5241     6141     +900     
==========================================
+ Hits         2866     3304     +438     
- Misses       2073     2502     +429     
- Partials      302      335      +33     
Flag Coverage Δ
unittests 53.80% <100.00%> (-0.89%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

This is an initial PoC to show what rego.v1 support might look.

The main thing I am unsure of is the regorewriter. This could have some issues even with v0 as I understand it. For example, `some x in data.lib` will not be rewritten correctly. Anyone with more knowledge on this component, it'd be interesting to better know how this is used and how it should work when supporting both v0 and v1.

The next obvious thing to consider here is that attempting parsing in more than one rego version is not ideal. This is done here since we will not know the source version of the rego until we try to parse it. We might consider something like this to allow users to force GK to use a given version.

```
apiVersion: templates.gatekeeper.sh/v1beta1
kind: ConstraintTemplate
metadata:
  name: k8srequiredlabels
spec:
  crd:
    spec:
      names:
        kind: K8sRequiredLabels
      validation:
        # Schema for the `parameters` field
        openAPIV3Schema:
          properties:
            labels:
              type: array
              items:
                type: string
  targets:
    - target: admission.k8s.gatekeeper.sh
      version: v1
      rego: |
        package k8srequiredlabels

        violation contains {"msg": msg, "details": {"foo": "bar"}} if {
          ...
        }
```

Signed-off-by: Charlie Egan <[email protected]>
@charlieegan3 charlieegan3 marked this pull request as ready for review January 29, 2025 10:11
@charlieegan3 charlieegan3 force-pushed the rego-v1 branch 2 times, most recently from 166e9c1 to 84962bb Compare January 29, 2025 10:18
@maxsmythe
Copy link
Contributor

Thanks for the PR!

To answer your question about regorewriter:

First some background:

regorewriter was written when all constraint templates were compiled into the same OPA environment. It was intended to provide hermeticity. Basically, we wanted to be sure that no constraint template could modify the behavior of any other constraint template, so we rewrote the module paths to ensure non-overlap. We also limited the sub-fields of data that could be accessed in order to prevent cross-querying. This has the side effect of providing static verification to users (e.g. catching mis-types like data.inventry instead of data.inventory.

Since that time, we have moved on to compiling each constraint template into a separate Rego environment (though the data cache is shared). This was mainly to reduce the O(n^2) complexity growth of compiling templates, but had the effect of strengthening sandboxing as well.

As such, regorewriter is less load bearing than it used to be, but still useful.

Key features of regorewriter (I may be missing some, did a brief survey of the code):

  • ensure all library modules have the expected prefix (data.libs) so they can be imported
  • ensure that only recognized subfields of data are used -- this helps avoid typos, but more importantly it gives us room to add future subfields without breaking backwards compatibility
  • make sure data cannot be reassigned to a new variable to escape static analysis (data must always have a sub-reference)
  • do not let any package clobber a root field (e.g. data.libs is not valid, data.libs.toolbox is)

It does look like some portions of regorewriter may be redundant -- if we require library packages to start with data.libs, why are we also rewriting those references? But a close reading of the code would be required to determine that for sure.

WRT adding the version string...

I like it! Though we should probably nudge people to using the newer format, which lives under spec.targets[].engine.source, example:

https://github.com/open-policy-agent/gatekeeper-library/blob/99a8a79972adcaf4c521e5d3ba87c5a6a1643049/library/pod-security-policy/forbidden-sysctls/template.yaml#L78-L150

I'd make it a sibling of the rego field, defined here:

type Source struct {
// Rego holds the main code for the constraint template. The `Violations` rule is the entry point.
Rego string `json:"rego,omitempty"`
// Libs holds supporting code for the main rego library. Modules can be imported from `data.libs`.
Libs []string `json:"libs,omitempty"`
}

for backwards compatibility, undefined -> v0, and using spec.targets[].rego would imply v0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants