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

colinjlacy · 2024-06-17T11:11:05Z

What are the changes in this PR?

adds --std-in flag to exec command
adds test coverage to cmd/internal/exec package
updates exec command docs with new flag

Notes to assist PR review:

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

netlify · 2024-06-17T11:12:13Z

✅ Deploy Preview for openpolicyagent ready!

Name	Link
🔨 Latest commit	`e5615d7`
🔍 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... Use your smartphone camera to open QR code link.

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

srenatus · 2024-06-17T11:46:10Z

cmd/exec.go

@@ -78,6 +79,7 @@ e.g., opa exec --decision /foo/bar/baz ...`,
 	cmd.Flags().VarP(params.LogLevel, "log-level", "l", "set log level")
 	cmd.Flags().Var(params.LogFormat, "log-format", "set log format")
 	cmd.Flags().StringVar(&params.LogTimestampFormat, "log-timestamp-format", "", "set log timestamp format (OPA_LOG_TIMESTAMP_FORMAT environment variable)")
+	cmd.Flags().BoolVarP(&params.StdIn, "std-in", "", false, "read input from os.Stdin rather than a static file; read timeout is 30 seconds")


Let's use the same flag as opa eval uses, please:

-I, --stdin-input read input document from stdin

srenatus · 2024-06-17T11:48:31Z

cmd/internal/exec/std_in_reader.go

+}
+
+func (sr *stdInReader) ReadInput() (string, error) {
+	input := make(chan string, 1)


Who closes that channel? 🤔

srenatus · 2024-06-19T10:56:19Z

cmd/internal/exec/std_in_reader.go

+				lines = append(lines, line)
+			}
+		}
+	}(done)


💭 I think the scoping rules would let you safely use done inside the go func() { ... }() body, wouldn't they?

srenatus · 2024-06-19T10:57:05Z

cmd/internal/exec/std_in_reader.go

+		case i := <-input:
+			return i, nil
+		case <-time.After(sr.Timeout):
+			done <- true


I think close(done) would be more slightly more idiomatic? Closed channels return their type's zero value; but we don't care for that value anyhow.

colinjlacy · 2024-06-21T10:23:22Z

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 · 2024-06-21T10:34:19Z

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

Quite an overhaul of our tests there, but I think it's for the better. Thanks!

There's one nitpick that you can safely ignore (t.Cleanup) but the docs should probably be regenerated to match the flag.

👏 Thanks again!

srenatus · 2024-06-26T09:24:42Z

cmd/exec_test.go

+						t.Fatalf("unexpected error when rewinding temp file: %q", err.Error())
+					}
+					oldStdin := os.Stdin
+					defer func() {


[nit][ We could use t.Cleanup() here. But it doesn't make a difference, I believe.

srenatus · 2024-06-26T09:26:11Z

docs/content/cli.md

@@ -618,6 +618,7 @@ opa exec <path> [<path> [...]] [flags]
      --log-timestamp-format string          set log timestamp format (OPA_LOG_TIMESTAMP_FORMAT environment variable)
      --set stringArray                      override config values on the command line (use commas to specify multiple values)
      --set-file stringArray                 override config values with files on the command line (use commas to specify multiple values)
+      --std-in                               read input from `os.Stdin` rather than a static file


This needs to be regenerated, I think?

Signed-off-by: Colin Lacy <[email protected]>

srenatus · 2024-06-26T09:59:12Z

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

johanfylling

Thank you! 😃

srenatus reviewed Jun 17, 2024

View reviewed changes

colinjlacy force-pushed the issue-6538-stdin-input branch from b582237 to 294b6f3 Compare June 19, 2024 10:50

srenatus reviewed Jun 19, 2024

View reviewed changes

colinjlacy force-pushed the issue-6538-stdin-input branch 3 times, most recently from 6af67c8 to e25b93b Compare June 25, 2024 08:07

srenatus changed the title ~~cmd/exec: adds --std-in flag for input piping or manual entry~~ cmd/exec: adds --stdin-input (-I) flag for input piping or manual entry Jun 26, 2024

srenatus previously approved these changes Jun 26, 2024

View reviewed changes

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

190ecd9

Signed-off-by: Colin Lacy <[email protected]>

colinjlacy dismissed srenatus’s stale review via 190ecd9 June 26, 2024 09:42

colinjlacy force-pushed the issue-6538-stdin-input branch from e25b93b to 190ecd9 Compare June 26, 2024 09:42

johanfylling approved these changes Jun 26, 2024

View reviewed changes

Merge branch 'main' into issue-6538-stdin-input

e5615d7

johanfylling merged commit 96800d7 into open-policy-agent:main Jun 26, 2024
28 checks passed

colinjlacy deleted the issue-6538-stdin-input branch June 26, 2024 16:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

colinjlacy commented Jun 17, 2024

netlify bot commented Jun 17, 2024 •

edited

Loading

srenatus Jun 17, 2024

srenatus Jun 17, 2024

colinjlacy Jun 17, 2024

srenatus Jun 19, 2024

srenatus Jun 19, 2024

colinjlacy Jun 19, 2024

colinjlacy commented Jun 21, 2024

srenatus commented Jun 21, 2024

srenatus left a comment

srenatus Jun 26, 2024

srenatus Jun 26, 2024

srenatus commented Jun 26, 2024 •

edited

Loading

johanfylling left a comment

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

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

Conversation

colinjlacy commented Jun 17, 2024

What are the changes in this PR?

Notes to assist PR review:

netlify bot commented Jun 17, 2024 • edited Loading

✅ Deploy Preview for openpolicyagent ready!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colinjlacy commented Jun 21, 2024

srenatus commented Jun 21, 2024

srenatus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srenatus commented Jun 26, 2024 • edited Loading

johanfylling left a comment

Choose a reason for hiding this comment

netlify bot commented Jun 17, 2024 •

edited

Loading

srenatus commented Jun 26, 2024 •

edited

Loading