open-policy-agent / opa

Open Policy Agent (OPA) is an open source, general-purpose policy engine.
https://www.openpolicyagent.org
Apache License 2.0
9.3k stars 1.29k forks source link

cmd/exec: adds --stdin-input (-I) flag for input piping or manual entry #6822

Closed colinjlacy closed 5 days ago

colinjlacy commented 2 weeks ago

What are the changes in this PR?

Notes to assist PR review:

Can test with input piping, e.g. echo "{\"foo\": 8}" | opa exec ... or manual entry via Stdin.

Fixes #6538

netlify[bot] commented 2 weeks ago

Deploy Preview for openpolicyagent ready!

Name Link
Latest commit e5615d7d9d1421a6c6171a1841a0c35c67246f81
Latest deploy log https://app.netlify.com/sites/openpolicyagent/deploys/667c10134506130008a404a9
Deploy Preview https://deploy-preview-6822--openpolicyagent.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

colinjlacy commented 1 week ago

Hey folks, I'm stuck on one aspect of this and I could use some guidance. In offline convos with @srenatus it was pointed out that trying to set a timeout on the goroutine that spawns the stdin reader creates a blocking thread that will continue to block until the main process exits. That is to say that even with the timeout in the main goroutine, prompting it to continue and not wait for the child goroutine to send back data, the child will continue to block and wait for input from stdin. That's not great as it's a known memory leak and unintentional block on further input (if further input is ever needed). So the timeout has some less-than ideal side effects.

On the other hand, as was discussed in the original issue, I'm concerned about the overall UX of adding functionality that could potentially hang indefinitely while the process awaits user input.

In order to get something in, I'm more partial to the idea of preventing the leak/block and omitting the timeout, as it doesn't look like it's possible to get around blocking reads on stdin. Any objections?

srenatus commented 1 week ago

I think it's OK to not have a timeout, and use the simplest possible thing here. It's also going to be on-par with the opa eval -I. 👍

srenatus commented 5 days ago

@johanfylling can you have a quick look and merge when it's convenient, please?