Add support for crun hook stdout/stderr annotations in precreate hooks - #1089
Conversation
bda30fb to
d459229
Compare
Luap99
left a comment
Luap99
left a comment
There was a problem hiding this comment.
LGTM overall, but looks like you need to rebase
d459229 to
dc75571
Compare
| preExistingStdout string | ||
| checkStdoutMode bool |
There was a problem hiding this comment.
These are not really independent test variants; they can be always used if a stdout annotation is used.
Similarly preExistingStdout is not a test parameter, it can be a constant, and exercised whenever stdout is recorded.
If these checks are worth doing, they might also be worth doing for stderr.
bd9671f to
e0c69ce
Compare
| } | ||
| } | ||
|
|
||
| func TestRuntimeConfigFilterCreatesStdoutFileWithCorrectMode(t *testing.T) { |
There was a problem hiding this comment.
I don’t think this is really worth a separate function (both for the cognitive/maintenance load and for the time to start the shell) … add the stat after the ReadFile content check, both for stdout and stderr (this test checks stdout only).
Absolutely non-blocking: It might be convenient to use a assertHookFileMatches(path, expectedContents) function to share the stdout+stderr code, I’m not immediately sure whether it would be worth it. Either two copies, or a shared helper, is fine with me.
There was a problem hiding this comment.
don’t think this is really worth a separate function
… oh, right, this was separate because we only set mode when creating a new file, wasn’t it? And now, with always writing the preExisting…Content data, we never create a new file. So we do need either this separate function, or a separate preExistingStdout test parameter; without that, we never actually test what permissions we create the file with.
I’m afraid I overlooked that completely.
There was a problem hiding this comment.
np, just restored the separate function
e0c69ce to
50608b1
Compare
|
A late design question: Are we fine with creating the stdout/stderr files if there are stderr/stdout annotations, but no hooks? It feels a bit unclean in principle, but it also probably makes little difference in practice. |
Precreate hooks run directly by Podman before the OCI runtime is invoked, so they never benefit from crun's run.oci.hooks.stdout/ run.oci.hooks.stderr annotations. This adds the same support to RuntimeConfigFilterWithOptions, opening the annotated file(s) in append mode (creating if missing, mode 0700 to match crun) and wiring hook stdout/stderr into them. Signed-off-by: Joshua Arrevillaga <2004jarrevillaga@gmail.com>
50608b1 to
04b8f4c
Compare
|
In practice doesn't make sense to create the files if there's no hooks to actually redirect so I added an early return for the case |
3 participants
OCI hooks can redirect their output to a file via 2 crun annotations
run.oci.hooks.stdout=FILE/run.oci.hooks.stderr=FILEThis works for every hook type except precreatehooks, since those are executed directly by podman before the OCI runtime is invoked so crun doesnt get a chance to honor the annotations. This adds the same annotation support toRuntimeConfigFilterWithOptionsso precreate hook output anc be redirected too. PR in podman will be needed to bump vendor dependency.Test
go test ./pkg/hooks/exec/... -run TestRuntimeConfigFilterOutputRedirection -v