Closed hacktobeer closed 10 months ago
Good question. This is before my time but when not running in a container the worker runs as the turbinia user and needs sudo for block device access
When running eg plaso in a separate container the docket daemon mounts the volumes (input data) in a way that the default user that runs log2timeline IN the container can access the volumes. So no sudo required.
At some point it was decided to have the worker run as turbinia.Probably at that time out of security reasons, but I do not know which ones. The container runs privileged anyway.
On Fri, 29 Sep 2023 at 16:23, Juan Leaniz @.***> wrote:
@.**** approved this pull request.
LGTM - just a question on the use of 'sudo'
In turbinia/workers/artifact.py https://github.com/google/turbinia/pull/1349#discussion_r1341428935:
@@ -64,6 +64,10 @@ def run(self, evidence, result): '--artifact_filters', self.artifact_name, ] +
- if not config.DOCKER_ENABLED:
- cmd.insert(0, 'sudo')
Is sudo really required to run image_export.py? Why doesn't the docker container need to run it with root privileges? Since we are running the workers with --privileged the worker program should be able to access block devices no?
— Reply to this email directly, view it on GitHub https://github.com/google/turbinia/pull/1349#pullrequestreview-1650885472, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABP5D4EDGJCHK7KNGMZYGUTX43KX5ANCNFSM6AAAAAA5MH5OVA . You are receiving this because you authored the thread.Message ID: @.***>
What would be the plan for the worker container after this PR. To start removing the larger dependencies and instead let the DIND handle it? Would we have two separate docker workers to maintain? (one full with deps and one with just dind)
I would opt for not having 2 workers images, more maintenance is not what we want. Having one worker with currently all dependencies so people can run without DOCKER_ENABLED. With this PR we have the option to run job dependecies in DOCKER if we want that (eg plaso) to have easier dependency version management (eg switch plaso version without rebuilding/releasing worker).
Also to be honest minus a docker compose setup, I don't see us using this for the k8s deployment (open for discussion I know we chatted a bit on this for it to work for both). IMO would opt us to use k8s jobs as feel like its meant for this type of stuff and that could also allow us to have more flexibility on where they get placed/how they scale, sharing Plaso between TS and probably wont need a privileged worker (only for the one time jobs that need it). We should still be able to use the image value in the config for both cases.
I agree. I would want (and have been driving for) one uniform architecture and platform.The proposal to go for k8s Jobs makes sense but is a pretty big change. We would need to migrate all our deployments to k8s and re-write a big core of the job manager by implementing a good k8s Job manager. I am all for doing this, how about we write a proposal/design doc?
In the end this PR gives us an option to run Jobs (dependencies) in container which can pave the way for easier migration to k8s Jobs.
wdyt?
I see that makes more sense. If I'm understanding correctly, we're leaving the worker container as is including building Plaso in the container but now we also have the option to specify them through containerized jobs.
Regarding k8s jobs -> Sounds good on creating a proposal/design to see what work this will take and see if its worth it from there. I was hoping we can keep existing code for Redis/Celery to track the Task and write some code to create the k8s job but I may be over simplifying it as well.
Overall then the implementation LGTM. We can have more dependency control and it also fixes the existing docker manager code and opens the door for more types of containerized processing
@aarontp PTAL
@hacktobeer Sorry, I didn't get a chance to look at this today, but I'll make sure to check it out tomorrow.
@aarontp ptal when you have time 😊
@aarontp addressed comments PTAL. Also created issue to replace docker in long term - https://github.com/google/turbinia/issues/1383
@hacktobeer LGTM! I'll let you merge it in case you wanted to make the docker compose changes in this PR.
@aarontp I created an extended Dockerfile for the dind setup (docker-compose-dind.yml). PTAL, I think using extend is a pretty nifty feature of the Dockerfile syntax as it extends on the original file without duplicating any of the original configuration.
@aarontp I created an extended Dockerfile for the dind setup (docker-compose-dind.yml). PTAL, I think using extend is a pretty nifty feature of the Dockerfile syntax as it extends on the original file without duplicating any of the original configuration.
Cool, LGTM!
Description of the change
This PR rewrites and updates the docker code in Turbinia.
Applicable issues
1336
542
440
Checklist