Add the ability to wrap the http.RoundTripper from Go code

[evankanderson]

Why the changes in this PR are needed?

We use Rego (via topdown) in Minder, an OpenSSF project. We'd like to be able to customize the http.send for a number of use cases, such as automatically adding service-managed credentials (JWTs represent a specific project in a multi-tenant system, similar to GitHub Actions IDs) to outgoing requests. To do this, we'd need to inject our own http.RoundTripper, while respecting the existing flags and arguments to make it easier to transfer Rego knowledge from one environment to another (and to avoid rewriting the builtin http.send, which does a lot of useful stuff).

What are the changes in this PR?

Adds an EvalHTTPRoundTrip EvalOption and query-level WithHTTPRoundTrip option. Both use a new function type which converts an http.Transport configured by topdown to an http.RoundTripper, presumably by wrapping the underlying http.Transport.

Notes to assist PR review:

I ended up needing to adjust some of the tests; in particular, I discovered that assertTopDownWithPathAndContext was creating two query objects, but only using the supplied options for the full-eval version, and not the partial-eval version. In turn, this caused me to discover some other HTTP tests which were relying on the Query objects between the two tests having different options, and fixing them.

Further comments:

I'm not sure if there's further documentation beyond the Go docs via comments that need to be added; if so, I'm happy to add them.

[evankanderson]

I discussed this a bit on Slack, as well as with @charlieegan3 at KubeCon.

The end result ended up being a bit nicer than I anticipated in the throes of making the change, though the testing odyssey was a bit involved. (Sorry!)

[evankanderson]

I'm not sure what to make of the two test failures; neither seems obviously implicated by my code (which I built and tested on MacOS 15.1). I'm attempting 200 runs of ./topdown/cache to see if that can surface some sort of race condition.

[anderseknert]

This looks like a good, non-intrusive addition to me. I'm a bit confused by renaming "round tripper" to "round trip" though... as that had med looking at the code several times wonder how one would pass such a thing around. Can we just go with RoundTripper?

("round tripper" is IMHO questionable as well, and one part of ther larger Go NIH-terminology, but at least it's recognized for what it is here)

[charlieegan3]

Hey Evan, great to meet you last week. I am unsure if our conversation helped clear things up here (I suspect it might not have!) but this looks like a solid direction to me.

I'm a bit confused by renaming "round tripper" to "round trip"

I am inclined to agree here too, but open to being persuaded/educated otherwise.

[evankanderson]

I'm happy to rename to RoundTripper; the 3 extra characters are no real cost to me. 😁

[evankanderson]

A followup on the runs of ./topdown/cache from my Mac:

$ go test -count 200 ./topdown/cache
--- FAIL: TestInsertWithExpiryAndEviction (2.00s)
    cache_test.go:436: Didn't expect cache entry for expired_foo, found entry with value &{bar 20}
--- FAIL: TestInsertHighTTLWithStaleEntryCleanup (2.00s)
    cache_test.go:479: Expected cache entry with value &{bar 20} for high_ttl_foo, found <nil>
--- FAIL: TestInsertHighTTLWithStaleEntryCleanup (2.00s)
    cache_test.go:479: Expected cache entry with value &{bar 20} for high_ttl_foo, found <nil>
--- FAIL: TestInsertWithExpiryAndEviction (2.00s)
    cache_test.go:436: Didn't expect cache entry for expired_foo, found entry with value &{bar 20}
--- FAIL: TestInsertWithExpiryAndEviction (2.00s)
    cache_test.go:436: Didn't expect cache entry for expired_foo, found entry with value &{bar 20}
FAIL
FAIL    github.com/open-policy-agent/opa/topdown/cache  400.573s
FAIL

So I suspect that there are some 1% flakes here, but I'll dig into those separately.

[anderseknert]

Thanks! LGTM now :)

A bit of a nit, and one I should have noticed before — but as for topdown.CustomizeRoundTripper, I think the fact that it's custom is implied as it's being passed in as an option... and could just be called RoundTripper... but that's mostly just my preference.

[evankanderson]

Thanks! LGTM now :)

A bit of a nit, and one I should have noticed before — but as for topdown.CustomizeRoundTripper, I think the fact that it's custom is implied as it's being passed in as an option... and could just be called RoundTripper... but that's mostly just my preference.

I named it that way because it's not a RoundTripper, it's a transformation function.

[anderseknert]

it's not a RoundTripper, it's a transformation function.

Well, that's pretty much what a RoundTripper is :)
i.e. what any other language would call middleware

But if you think this makes its purpose more apparent, I'm fine with that.

[evankanderson]

(Note that this mechanism would allow go library users to replace allow_net with a customized roundtripper, but I don't want to break existing users by removing that functionality.)

[evankanderson]

(Do I need to do more things to get this merged? I don't have permissions to merge this myself, but I don't know what / when the merge process is.)

[anderseknert]

@evankanderson no, nothing more to do. I'm rebasing on top of main, and if the build is green, I'll merge.