plugins/rest: various changes re: TLS, *http.Client caching

[srenatus]

This hasn't been asked for, but it seems like a logical addition. When there's a change to the certificate or keys modification time (an FS attribute), we'll reload them.

An alternative I had considered was checking the hash of the times, which we can also do, but it'll be a little more computationally expensive, and we'd do this for every TLS handshake. I think we should then add another configurable (or good default) for "how often to compare the hash". The mod time seemed a lot simpler in comparison.

Another alternative is using a path watcher goroutine, but due to how our rest clients work, there's no natural location for a "Close()" call that would allow us to clean up the routine. So I didn't take this route.

🧹 I've moved the auth TLS code into its own file, but in a separate commit, for easier review of the diff.

[netlify]

✅ Deploy Preview for openpolicyagent ready!

Name	Link
🔨 Latest commit	3b8ce68
🔍 Latest deploy log	https://app.netlify.com/projects/openpolicyagent/deploys/699ff8705a83da0008970244
😎 Deploy Preview	https://deploy-preview-8376--openpolicyagent.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[philipaconrad]

Having looked through each commit, I like the general direction of this PR a lot, and didn't see anything obviously amiss anywhere. 🙂

My main points of concern when reviewing were around the locking (which I think is correct), and around the new branching in behavior for the plugin (cert_refresh_duration_seconds > 0 and cert_refresh_duration_seconds == 0 cases).

As far as I can tell by just eyeballing it, it looks like this is all implemented correctly, and it makes sense why we'd have different behaviors. Overall, nicely done!

When you're ready to take this PR out of Draft state, feel free to ping me, and I'll give it a more in-depth review.

[srenatus]

@philipaconrad thanks for the early review! I had seen it just now -- after having decided to switch to a simpler mod time based approach. Can you have another look? The idea here is that we have a new feature: if the certs are rotated, they'll be reloaded, without any new config or twist. The rationale being that either you don't touch your cert files, then nothing will be different; or you do and then you probably appreciate having OPA react accordingly by reloading them 😅 What do you think about that?

[philipaconrad]

I'm marking Approve, because I think this PR is in a good state to ship (possibly with a small docs update; see comments).

I think the modification time detection approach will work well, and the edge cases are fairly niche. I especially enjoy that it keeps the plugin config from sprouting additional fields.

I can't think of a good reason for mtime to update continuously for a TLS cert, but someone using touch (which updates mtime on files that already exist) in a loop could trigger the aggressive reloading behavior if the OPA instance is under load. 🤔

---

As a point of comparison to these changes, the localfile data source plugin in EOPA does something similar to your initial approach: it periodically polls the file on disk, hashes its contents, and then updates the file buffer in memory only if the hash is different.

I think we'd planned to use file modtimes over there as a heuristic to reduce disk reads, although we'd probably still hard-reload from disk every N-many polls, just to make sure the hashed contents weren't different.

[philipaconrad]

So, I did some googling around, and there are a few specific cases where even on a POSIX-compliant system you might not get a correct mtime update, even after a write that changes the file contents:

• NFS / Samba filesystems (updates may be delayed or set in the server's timezone, among other weirdness from being on a network)
• FAT family filesystems (mtime has a 2 second resolution. Writes happening within 2 seconds of each other won't be detected.)
  • See the 0x0E entry in the linked table for a description of the time format used for both create/modify time records.
• mmap()'d files may have delayed updates.
• Device and virtual files (/dev, /proc, and so on) may not get mtime updates when they change.

I think mtime detection should work fine, 99%+ of the time. In all but the NFS-related cases above, the timestamp will usually only be off by a few seconds at most, which won't affect the average user.

I feel that we should still document somewhere that we're relying on mtime as the refresh trigger, or someone will get surprised by it. 🤔

Maybe adding a sentence or two to the Configuration page entry for the client TLS config items might do the job?

[srenatus]

Taken back to "draft" status. I'd like to think about this some more, thanks for the input @philipaconrad, I appreciate it a lot! 🔍 👀

[netlify]

✅ Deploy Preview for openpolicyagent ready!

Name	Link
🔨 Latest commit	420da4d
🔍 Latest deploy log	https://app.netlify.com/projects/openpolicyagent/deploys/69a808eab8eebc00085aea13
😎 Deploy Preview	https://deploy-preview-8376--openpolicyagent.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[srenatus]

OK I took a bit of a detour here, went on the scenic route and fixed what whatever I found.

1. We're not caching the http.Client. Comments about this for custom plugins were added to the CHANGELOG. Existing in-tree plugins should be OK (as of this PR)
2. There's a re-read interval config for our TLS auth plugin. By default it'll re-read on every handshake, more or less doing what it did before. But it'll check the hash sums first to avoid parsing certs when it's unnecessary.
3. Min TLS versions should now be propagated from the server to the REST plugins. If nothing was set, we'll enforce TLS v1.2.

[netlify]

✅ Deploy Preview for openpolicyagent ready!

Name	Link
🔨 Latest commit	2423816
🔍 Latest deploy log	https://app.netlify.com/projects/openpolicyagent/deploys/69b293e53405e20008e56955
😎 Deploy Preview	https://deploy-preview-8376--openpolicyagent.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[charlieegan3]

This is declared in config but it only used in the rest package, for now. Do we have plans to use this elsewhere that might make it make sense to keep here?

The defaulting also seems to happen in the rest package.

[srenatus]

The server package uses it, too. We could move it... but since it's a constant, I didn't think it mattered too much 🤔

[srenatus]

https://github.com/open-policy-agent/opa/pull/8376/changes#diff-bede818503e8e0efdaf932d4c6479a9f7e34b435a9f50871833f88e4117a91bb

[charlieegan3]

is it not PK that's encrypted here? rather than the cert.

[srenatus]

Moved code I hadn't looked at this closely, but you are right, I think. Will update!

[charlieegan3]

Suggested change

	return nil, err
	return nil, fmt.Errorf("failed to parse public/private key pair: %v", err)

[charlieegan3]

Some of this might have been my handiwork, but we are getting a number of top level TLS configs here. TLS, AllowInsecureTLS, minTLSVersion etc.

[srenatus]

We're sharing one struct for both unmarshalling config and for storing derivates of it... I think that's part of why it's like it is.

[alex60217101990]

Could we consider using fasthttp instead of net/http? The standard library is great, but fasthttp would be more efficient for high-load scenarios.

[srenatus]

Could we consider using fasthttp instead of net/http? The standard library is great, but fasthttp would be more efficient for high-load scenarios.

This would be tricky, as it's impossible to do without breaking the interfaces for custom auth plugin implementations. So, we'd need a very convincing case to pull this off -- like a real world problem with decision logs (the only thing that would be affected in high-load scenarios) that would be solved by swapping out net/http with fasthttp.

[alex60217101990]

With the PR caching *http.Client per rest.Client instance, each OPA service still gets its own transport (and therefore its own connection pool). If multiple services point to the same host (e.g., a single backend serving both bundles and decision log ingestion), connections aren't shared between them.

Consider allowing an opt-in shared *http.Transport keyed by (host, TLS fingerprint) so that services targeting the same endpoint can reuse the underlying TCP/TLS connections. The *http.Client can remain per-service for auth isolation, but the transport underneath could be shared.

This matters in practice: a typical deployment might configure 3-5 services (bundles, decision logs, status) all hitting the same control plane. Each one maintaining its own idle connection pool means 1-1,5x the memory for connection state and TLS session data that could otherwise be amortized.

[srenatus]

This sounds good on paper, but the extra complexity needs to be warranted. Let's keep it in mind in case someone brings up a case where measurements reveal this as a bottleneck.

[alex60217101990]

oauth2ClientCredentialsAuthPlugin.requestToken() creates a throwaway *http.Client on every token refresh:

// v1/plugins/rest/auth.go:717

client := DefaultRoundTripperClient(&tls.Config{InsecureSkipVerify: ap.tlsSkipVerify}, 10)

response, err := client.Do(r)

Even with the PR caching the main *http.Client, this path is untouched. Every token refresh allocates a new transport, does a TLS handshake from scratch, and then discards everything. For short-lived tokens or high-frequency refreshes, this defeats connection reuse entirely.

The fix is straightforward: cache the token endpoint client alongside the main client, or reuse the main client for token requests (since Prepare() is where auth headers are set, using the main client for token fetches won't cause recursion as long as the token endpoint request skips Prepare).

[alex60217101990]

Decision logs are uploaded as gzip-compressed chunks (default limit: 32KB, max: ~4GB). The current DefaultRoundTripperClient inherits Go's default transport settings, which are conservative:

• WriteBufferSize: 4KB (default) — for a 32KB chunk this means multiple write syscalls per upload

• ReadBufferSize: 4KB (default)

• MaxIdleConnsPerHost: 2 (default) — if decision logs back up and multiple uploads happen concurrently, only 2 connections are reused

For large batch uploads (which is common in high-throughput deployments), it would help to expose transport-level tunables in the service config, or at least set better defaults:

tr := http.DefaultTransport.(*http.Transport).Clone()

tr.WriteBufferSize = 32 * 1024  // match the default upload chunk size

tr.ReadBufferSize = 8 * 1024

tr.MaxIdleConnsPerHost = 4       // allow more connection reuse under load

This is especially relevant now that the client is cached — the transport settings persist for the lifetime of the service, so getting them right matters more than before.