tilt-dev / tilt-extensions

Extensions for Tilt
https://tilt.dev/
Apache License 2.0
201 stars 159 forks source link

`restart_process` fails with permission denied #540

Open thejan2009 opened 8 months ago

thejan2009 commented 8 months ago

docker_build_with_restart has been failing for me recently, but I've found out what's the issue - even though /tmp/.restart-proc has permissions 666, the /tmp folder has sticky bit enabled and the containers run with root user instead of 1001 that's the owner of /tmp/.restart-proc.

Will copy 1 file(s) to container: [api-fb9658fb7-pg8pd/api]
- '~/code/go/build/api' --> '/bin/api'
[CMD 1/1] sh -c date > /tmp/.restart-proc
sh: can't create /tmp/.restart-proc: Permission denied
  → Failed to update container api-fb9658fb7-pg8pd/api: executing on container 818f0243f0: command "date > /tmp/.restart-proc" failed with exit code: 1
Build Failed: executing on container 818f0243f0: command "date > /tmp/.restart-proc" failed with exit code: 1

I've found two temporary workarounds, but they won't scale:

nicks commented 8 months ago

The fix was rolled back due to the regression reported in https://github.com/tilt-dev/tilt-extensions/issues/544, sorry

thejan2009 commented 8 months ago

Hey, I took a look at the issue there, not sure how to proceed.

       → /bin/sh: 1: [mkdir,: not found
...
process "/bin/sh -c [\"mkdir\", \"-p\", \"\\tmp\\.restart\"]"

It seems for some reason docker builder garbles the parameters for this RUN command in exec form. And that path is also not correct. We can work around missing mkdir by using WORKDIR dockerfile instruction.

But the os.path.dirname output will still be a windows-style path that will, in the best case, create a useless directory. I think here we'll always need a linux-style dirname function. Any idea how to get that in Tiltfile? In golang, this would mean using path.Dir instead of filepath.Dir.

nicks commented 8 months ago

oh ya, good call... i would probably use string manipulation, something like:

index = path.rfind('/')
if index != -1:
  dir = path[0:index]

https://github.com/bazelbuild/starlark/blob/master/spec.md#string%C2%B7rfind

thejan2009 commented 8 months ago

Thanks, this works and it also seems like a safe solution, because / character is not allowed in filenames.

nicks commented 8 months ago

unfortunately, this had to be rolled back again.

a container with a sticky /tmp seems pretty rare. is there some reason why you can't solve this problem with the existing restart_file param?

thejan2009 commented 8 months ago

Sad. I think you should consider banning me from any future contributions, given the track record :)

a container with a sticky /tmp seems pretty rare.

[~] docker run -t --rm busybox:latest ls -ld /tmp
drwxrwxrwt    2 root     root          4096 Dec 19 21:10 /tmp
[~] docker run -t --rm alpine:latest ls -ld /tmp
drwxrwxrwt    2 root     root          4096 Dec  7 09:45 /tmp
[~] docker run -t --rm ubuntu:latest ls -ld /tmp
drwxrwxrwt 2 root root 4096 Dec 11 14:34 /tmp

Found the root cause for my issues, though. I use colima instead of docker desktop and in a recent update it switched the base VM to ubuntu. And there is this new kernel parameter fs.protected_regular that's set to 2 by default and inherited by all containers. In my case it prevents the root user from overwriting regular files created by other users when sticky bit is set in the folder. In docker for desktop, this parameter seems to be set to 0 by default.

I've now set the parameter to 0, but still, would you accept a WORKDIR-based solution that I outlined in the second PR? It doesn't require mkdir nor sh in the base container image.

nicks commented 8 months ago

eh, it happens.

how would a WORKDIR-based solution work?