Islandora / Crayfish

A collection of Islandora microservices, lovingly known as Crayfish.
MIT License
9 stars 29 forks source link

3x: Allow homarus to use faststart #170

Closed bibliophileaxe closed 2 years ago

bibliophileaxe commented 2 years ago

Rebase with 3.x Original Issue: https://github.com/Islandora/documentation/issues/2162

seth-shaw-asu commented 2 years ago

I spun up the new islandora-playbook is Crayfish 3.x and then applied this PR. The video derivative didn't work and the logs show:

[2022-10-17 12:49:51] request.INFO: Matched route "convert_get". {"route":"convert_get","route_parameters":{"_route":"convert_get","_controller":"App\\Islandora\\Homarus\\Controller\\HomarusController::convert"},"request_uri":"http://127.0.0.1:8000/homarus/convert","method":"GET"} []
...skipping authentication stuff...
[2022-10-17 12:49:52] app.INFO: Ffmpeg Convert request. [] []
[2022-10-17 12:49:52] app.DEBUG: Tempfile: /var/www/html/Crayfish/Homarus/src/Controller/../../static/file_example_mov_1920_2_2mb.mov.mp4 [] []
[2022-10-17 12:49:52] app.DEBUG: X-Islandora-Args: {"args":" -loglevel error"} []
[2022-10-17 12:49:52] app.DEBUG: Ffmpeg Command: {"cmd":"ffmpeg -headers 'Authorization:  Bearer eyJ0eXAiOiJKV1QiLCJhbGciOiJSUzI1NiJ9.eyJpYXQiOjE2NjYwMjg5ODksImV4cCI6MTY2NjAzNjE4OSwid2ViaWQiOiIxIiwiaXNzIjoiaHR0cHM6XC9cL2xvY2FsaG9zdDo4MDAwIiwic3ViIjoiYWRtaW4iLCJyb2xlcyI6WyJhdXRoZW50aWNhdGVkIiwiZmVkb3JhYWRtaW4iXSwiYXVkIjpbImlzbGFuZG9yYSJdfQ.racbxnihkudPadE8o0T_7coId-yOmAgVN_w0QmO3OOBrmyEghz4QUxww5NzFdiAwaNbecBzQpcgPYamsyPmazWYeN9RlAOmGqptVzUIO7Rfytc04lzrmg6BgBm8YEYYeiz2GCtVJMTs0Wp4t5mOUzJLJP7zdHBDk2qwfpb_0I-QVaVuNN_czfU00vyHD5I2KXXsgc9tKCnSRdIZXMLCGilvuUTMFQZXZF1NFhsX5GzeIxU7m_3Juq1tmxpvb140p8TANj_B0sp0MB9Lbrv_wgGIKfyZh4MsWndW1pZcHfmHpxImgIHUcB4wpn2hMoCw7C6cxK8usMkDZm8TTZ6enWQ' -i http://localhost:8000/_flysystem/fedora/2022-10/file_example_mov_1920_2_2mb.mov   -loglevel error  -vcodec libx264 -preset medium -acodec aac -strict -2 -ab 128k -ac 2 -async 1 -movflags faststart -y -f mp4 /var/www/html/Crayfish/Homarus/src/Controller/../../static/file_example_mov_1920_2_2mb.mov.mp4"} []
[2022-10-17 12:49:54] app.ERROR: Process exited with non-zero code. {"exit_code":1,"stderr":"/var/www/html/Crayfish/Homarus/src/Controller/../../static/file_example_mov_1920_2_2mb.mov.mp4: No such file or directory\n"} []
[2022-10-17 12:49:54] app.ERROR: RuntimeException: {"exception":"[object] (RuntimeException(code: 500): /var/www/html/Crayfish/Homarus/src/Controller/../../static/file_example_mov_1920_2_2mb.mov.mp4: No such file or directory\n at /var/www/html/Crayfish/Homarus/vendor/islandora/crayfish-commons/CmdExecuteService.php:120)"} []
seth-shaw-asu commented 2 years ago

Shouldn't we be using a temp directory instead? Or, even better, something configurable?

codecov[bot] commented 2 years ago

Codecov Report

Base: 76.67% // Head: 75.55% // Decreases project coverage by -1.12% :warning:

Coverage data is based on head (2b2e18b) compared to base (aed6511). Patch has no changes to coverable lines.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## 3.x #170 +/- ## ============================================ - Coverage 76.67% 75.55% -1.13% - Complexity 158 159 +1 ============================================ Files 6 6 Lines 583 589 +6 ============================================ - Hits 447 445 -2 - Misses 136 144 +8 ``` | [Impacted Files](https://codecov.io/gh/Islandora/Crayfish/pull/170?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Islandora) | Coverage Δ | | |---|---|---| | [...d\_dir/Homarus/src/Controller/HomarusController.php](https://codecov.io/gh/Islandora/Crayfish/pull/170/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Islandora#diff-YnVpbGRfZGlyL0hvbWFydXMvc3JjL0NvbnRyb2xsZXIvSG9tYXJ1c0NvbnRyb2xsZXIucGhw) | `84.31% <0.00%> (-15.69%)` | :arrow_down: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Islandora). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Islandora)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

seth-shaw-asu commented 2 years ago

So, question for the @Islandora/committers, am I being paranoid with my discomfort that we are dropping these processing files in a directory that it publicly accessible to anyone who can reach the Crayfish webserver?

I mean, sure, Isle doesn't expose the Crayfish containers to the public web, but that doesn't mean others won't, and someone can keep pinging crayfish.example:8000/homarus/static/ to see what files are in process, even if they will be removed once they get shipped back to Drupal. Normally our derivatives will be publicly available anyway, but some will be subject to restrictions and that just feels like a security gap.

Let me know if I'm being overly paranoid and I'll smash the green button.

jordandukart commented 2 years ago

Honestly, probably worth bringing up on the tech call.

seth-shaw-asu commented 2 years ago

Any reason why we shouldn't use a system temp directory instead of a public one? We are sending the binary back in the response, not a download link...

adam-vessey commented 2 years ago

Ideally, I think, we would be using unnamed/anonymous files similar to what tmpfile() returns (that the OS would garbage collect/mark free after it detects that there are no references to it remaining in the system, thereby avoiding the possibility of files being left around if the PHP process crashes before deleting the file); however, I think the ideal implementation would require reworking the command executor in order to pass in the file resource instead of having stdout of the command go to a pipe (https://github.com/Islandora/Crayfish-Commons/blob/01e65d6182601834a94283aeacd8ef1ec10dc4e0/src/CmdExecuteService.php#L63-L66)... (really, would it make sense for it to deal with streams more explicitly? Seems like it already buffers things in one, could be just something like: https://github.com/Islandora/Crayfish-Commons/compare/3.x...adam-vessey:Crayfish-Commons:feature/more-streaming).

... though that's not to say that having the target file system used as configuration could not be useful, and may be something of a moot point if we do indeed need to pass a file name to ffmpeg (which... may not be 100% necessary? For example, running things on CLI, redirecting things to/from files can be handled more or less transparently, behind the scenes managing to deal with the files directly... whether or not things via proc_open() would work the same way would have to be tested out)...

bibliophileaxe commented 2 years ago

I think making the directory configurable makes sense. That way, people can decide if they need it to be a public or a private directory and handle it themselves.

seth-shaw-asu commented 2 years ago

Is this ready to test again, @bibliophileaxe?

bibliophileaxe commented 2 years ago

This is ready for review now.