Charles/eopa engine #1000

charlesdaniels · 2024-08-20T16:19:51Z

This PR adds initial support for an EOPA engine. It depends on the upstream PR #92, and should not be merged before that one is.

It includes 3 main components:

The config package has been changed so that if the eopa engine is used, the new helpers in the upstream PR are used to look up the capabilities that are attached to the config.Config type. The engine and version are saved in the config for later use.
The internal/lsp package has been modified to inspect the engine and version from the configuration, and to look up capabilities for use in the internal/lsp/rego package.
A new capabilities package has been added to provide a general-purpose way for Regal to access OPA capabilities. It unifies access to local, remote, and embedded capabilities using URLs. Both config and internal/lsp have been refactored to use this new system.

In summary, capabilities.Lookup() supports loading OPA capabilities in the following ways:

file:// URLs are loaded as JSON from disk, just like specifying from.file.
`http{s}:// URLs are loaded as JSON from a web request, which was not previously supported but has been discussed.
regal:///capabilities/default loads the capabilities using ast.CapabilitiesForThisVersion()
regal:///capabilities/{engine} loads the latest (according to SemVer) capabilities version for the engine
regal:///capabilities/{engine}/{version} loads the specified capabilities version for the engine

Semantic versioning support is implemented using github.com/coreos/go-semver/semver, which is the same library OPA uses for its semver builtins.

The existing from.engine, from.file, and from.version will continue to work, but internally they are translated to URLs so that access to capabilities is consistent everywhere in Regal. from.url has been added as well.

These changes do appear to work, and enable Enterprise OPA specific builtins to appear in the VSCode plugin with only a config change.

Remaining work:

implement additional LSP tests to ensure capabilities loading works as expected
implement additional LSP tests to ensure capabilities hot-reload works as expected
document new capabilities.Lookup functionality
switch to fetching EOPA capabilities JSON during build
refactor LSP to avoid global lock for the rego package

srenatus

Looks good so far 👍

pkg/capabilities/capabilities.go

internal/lsp/rego/builtins.go

build/do.rq

srenatus · 2024-08-23T08:23:03Z

build/do.rq

+	# similar setups locally. Setting GIT_CONFIG=/dev/null in this case
+	# prevents this.
+
+	fetch_cmd := rq.template("sh -c 'GIT_CONFIG=/dev/null git archive --remote={{.remote | quote}} {{.ref}} {{.srcdir | quote}} | tar -x -C {{.destdir | quote}}'", templ)


No idea what's preferable, but if you'd consider it less of a hassle, you could grab the zip:
https://github.com/StyraInc/enterprise-opa/archive/refs/heads/main.zip

I was trying to find a good way to only have to download the subset of the repo that is needed, rather than the entire thing. If all else fails, I'll just do that, and/or clone the whole repo into /tmp or something.

I don't know why the git archive thing doesn't work. StackOverflow says it should. The man page leads me to believe it should. But run() is complaining about a mismatched ', and on top of that when I run it manually, git complains about trying to clone a git:// URL with the ssh:// protocol, which is just bizarre.

internal/lsp/rego/builtins.go

anderseknert

Looking good! Just some minor remarks. There were other changes in the past though that I'm curious to know more about. Were they not needed?

anderseknert · 2024-08-23T20:11:02Z

build/do.rq

+
+	{ rq.error(sprintf("\nstdout: %s\nstderr: %s\n", [eopa_tags_result.stdout, eopa_tags_result.stderr])) | eopa_tags_result.exitcode != 0 }
+
+	eopa_tags := {r[2] | r := eopa_tags_result.stdout[_]}


Can we use some ... in iteration instead? Here and on line 257. This file isn't lintable at this point in time, as we'd need something similar for rq as this PR provides for eopa. But that doesn't mean should ignore the best practices and idioms Regal recommends.

I'm not actually sure how to use some ... in, can that handle additional clauses? I ended up having to filter the tag list down with further clauses.

hmm, yeah after 646a37d I'm not sure some ... in can work, based on reading the docs

It works anywhere:

eopa_tags := {t | some r in eopa_tags_result.stdout; t := r[2] ; not known_bad_tags[t]}

Ahh I see, I didn't realize you could use it in that way. I'll switch it over.

pkg/capabilities/embedded/eopa/v0.100.5.json

build/do.rq

anderseknert

Great work on this!

I left a couple of comments. You can ignore the style-related ones if you wish, as Regal itself will tend to most of them once we can lint .rq files :)

The rest should be considered however.

Very nice contribution!

README.md

build/do.rq

internal/capabilities/capabilities_test.go

Still needs some refactoring, but we can properly use the embedded capabilities in the LSP now.

Rather than using an option, we upgrade the existing rego package so that the builtins can be updated at runtime.

We have the capabilities embedded into Regal now instead.

We want the regal:// URI scheme to be a hidden implementation detail.

Moving the rq.write() to the LHS of the | seems more intuitive, and it avoids the possibility that OPA may one day optimize comprehensions with constants on the LHS.

The CI indicates that this is not allowed.

charlesdaniels · 2024-08-29T15:15:19Z

I'm not sure why the tests are suddenly failing in CI? They pass on my machine, and none of the code the failing tests exercise was changed since the last commit where they passed.

I think server_test.go:405: timed out waiting for file diagnostics to be sent is also kinda interesting - 405 is a blank line, so I'm not sure how the test could be failing there.

charlesdaniels · 2024-08-29T15:25:40Z

Interestingly, I do see a similar test failure when I run in an Alpine docker container (ARM64). This smells like some concurrency problem to me.

This reverts commit d6a4e98.

anderseknert

Thanks a lot for this Charles! I'll merge this as it is, and will have a look at the build issues reported when merged.

srenatus approved these changes Aug 22, 2024

View reviewed changes

pkg/capabilities/capabilities.go Outdated Show resolved Hide resolved

internal/lsp/rego/builtins.go Outdated Show resolved Hide resolved

srenatus reviewed Aug 23, 2024

View reviewed changes

build/do.rq Outdated Show resolved Hide resolved

srenatus reviewed Aug 23, 2024

View reviewed changes

internal/lsp/rego/builtins.go Outdated Show resolved Hide resolved

anderseknert reviewed Aug 23, 2024

View reviewed changes

charlesdaniels force-pushed the charles/eopa-engine branch from b6d2f9e to 34b26a9 Compare August 26, 2024 21:30

srenatus reviewed Aug 27, 2024

View reviewed changes

build/do.rq Outdated Show resolved Hide resolved

srenatus reviewed Aug 27, 2024

View reviewed changes

build/do.rq Outdated Show resolved Hide resolved

srenatus reviewed Aug 27, 2024

View reviewed changes

build/do.rq Outdated Show resolved Hide resolved

charlesdaniels marked this pull request as ready for review August 27, 2024 22:04

charlesdaniels force-pushed the charles/eopa-engine branch 2 times, most recently from e1d4aed to ec8c3a6 Compare August 28, 2024 19:54

anderseknert reviewed Aug 28, 2024

View reviewed changes

charlesdaniels added 17 commits August 29, 2024 11:11

check in PoC with capabilities

149ce1e

remove eopa capabilities

88f1ab0

use enterprise-opa's capabilities package

655a373

try to fix go.mod

2c89dd7

LSP+capabilities working correctly

eeb16a7

Still needs some refactoring, but we can properly use the embedded capabilities in the LSP now.

refactor LSP capabilities handling

ceae653

Rather than using an option, we upgrade the existing rego package so that the builtins can be updated at runtime.

implement URL-based caps lookup

f92073f

unify config + LSP on new capabilities.Lookup()

70e1991

unbreak tests

b666348

remove global lock

247ebc6

simplify http access in capabilities package

4cb276a

WiP: initial work to fetch eopa caps

327cffc

fully implement do.rq fetch

804d850

fix broken caps, add tests to embedded

d159426

remove enterprise-opa dep

0e60495

We have the capabilities embedded into Regal now instead.

move capabilities to be internal

c43422f

We want the regal:// URI scheme to be a hidden implementation detail.

move docs into README

95604b4

charlesdaniels added 20 commits August 29, 2024 11:11

add LSP test for new embedded EOPA caps

1358083

remove debug statements

aaa18e7

drop pre-1.0.0 eopa tags

19fbb99

fix lint errors

0cd5e4f

fix typo in do.rq

e94e99c

improve rq.write comprehension

1098d12

Moving the rq.write() to the LHS of the | seems more intuitive, and it avoids the possibility that OPA may one day optimize comprehensions with constants on the LHS.

fix incorrect test

3713d41

apply golangci fixes

8689de7

fix some linter issues

35a1e74

use some ... in in fetch task

d11047e

fix remaining lint issues

a8d9442

fix lint issue with main.rego

4d0b937

remove scope:document from report

723c057

The CI indicates that this is not allowed.

format server_test

eda0a23

restore metadata for report

fc39b64

fix lint issue

52dc471

format server_test

caf0860

use nolint to make golint stop complaining

03376bc

explicitly wait for builtins map to update

dbfd68c

implement PR review suggestions

e893548

charlesdaniels force-pushed the charles/eopa-engine branch from b5cb5c5 to e893548 Compare August 29, 2024 15:11

charlesdaniels added 2 commits August 29, 2024 11:31

temporarily increase server_test timeout

d6a4e98

Revert "temporarily increase server_test timeout"

d3f1f15

This reverts commit d6a4e98.

anderseknert approved these changes Aug 29, 2024

View reviewed changes

anderseknert merged commit 9cd3b05 into main Aug 29, 2024
0 of 3 checks passed

anderseknert deleted the charles/eopa-engine branch August 29, 2024 16:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Charles/eopa engine #1000

Charles/eopa engine #1000

charlesdaniels commented Aug 20, 2024 •

edited

Loading

srenatus left a comment

srenatus Aug 23, 2024

charlesdaniels Aug 23, 2024

anderseknert left a comment

anderseknert Aug 23, 2024

charlesdaniels Aug 23, 2024

charlesdaniels Aug 23, 2024

anderseknert Aug 28, 2024

charlesdaniels Aug 28, 2024

anderseknert left a comment

charlesdaniels commented Aug 29, 2024 •

edited

Loading

charlesdaniels commented Aug 29, 2024

anderseknert left a comment


		{ rq.error(sprintf("\nstdout: %s\nstderr: %s\n", [eopa_tags_result.stdout, eopa_tags_result.stderr])) \| eopa_tags_result.exitcode != 0 }

		eopa_tags := {r[2] \| r := eopa_tags_result.stdout[_]}

Charles/eopa engine #1000

Charles/eopa engine #1000

Conversation

charlesdaniels commented Aug 20, 2024 • edited Loading

srenatus left a comment

Choose a reason for hiding this comment

srenatus Aug 23, 2024

Choose a reason for hiding this comment

charlesdaniels Aug 23, 2024

Choose a reason for hiding this comment

anderseknert left a comment

Choose a reason for hiding this comment

anderseknert Aug 23, 2024

Choose a reason for hiding this comment

charlesdaniels Aug 23, 2024

Choose a reason for hiding this comment

charlesdaniels Aug 23, 2024

Choose a reason for hiding this comment

anderseknert Aug 28, 2024

Choose a reason for hiding this comment

charlesdaniels Aug 28, 2024

Choose a reason for hiding this comment

anderseknert left a comment

Choose a reason for hiding this comment

charlesdaniels commented Aug 29, 2024 • edited Loading

charlesdaniels commented Aug 29, 2024

anderseknert left a comment

Choose a reason for hiding this comment

charlesdaniels commented Aug 20, 2024 •

edited

Loading

charlesdaniels commented Aug 29, 2024 •

edited

Loading