Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

restart_process fails with permission denied #540

Open
thejan2009 opened this issue Dec 22, 2023 · 7 comments
Open

restart_process fails with permission denied #540

thejan2009 opened this issue Dec 22, 2023 · 7 comments

Comments

@thejan2009
Copy link
Contributor

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:

  • kubectl exec into the pod and toggle /tmp sticky bit
  • kubectl exec into the pod and chown /tmp/.restart-proc

Any other ideas on how to solve this? Could the .restart-proc file be moved to /tmp/.restart/proc and sticky bit toggled on tmp/.restart folder?

thejan2009 added a commit to thejan2009/tilt-extensions that referenced this issue Dec 22, 2023
Sometimes the container root filesystem has sticky bit enabled and
restart commands are ran with a different user. We can work around that
by creating an intermediate folder that has "0777" permissions.

Refs tilt-dev#540
thejan2009 added a commit to thejan2009/tilt-extensions that referenced this issue Dec 22, 2023
Sometimes the container root filesystem has sticky bit enabled and
restart commands are ran with a different user. We can work around that
by creating an intermediate folder that has "0777" permissions.

Refs tilt-dev#540

Signed-off-by: db <[email protected]>
nicks pushed a commit that referenced this issue Jan 2, 2024
Sometimes the container root filesystem has sticky bit enabled and
restart commands are ran with a different user. We can work around that
by creating an intermediate folder that has "0777" permissions.

Refs #540

Signed-off-by: db <[email protected]>
@nicks
Copy link
Member

nicks commented Jan 3, 2024

The fix was rolled back due to the regression reported in #544, sorry

@thejan2009
Copy link
Contributor Author

thejan2009 commented Jan 3, 2024

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
Copy link
Member

nicks commented Jan 3, 2024

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
Copy link
Contributor Author

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

thejan2009 added a commit to thejan2009/tilt-extensions that referenced this issue Jan 5, 2024
Sometimes the container root filesystem has sticky bit enabled and
restart commands are ran with a different user. We can work around that
by creating an intermediate folder that has "0777" permissions.

The folder name is parsed with `string.rfind` because os.path.dirname
won't work on Windows.

Refs tilt-dev#540

Signed-off-by: db <[email protected]>
thejan2009 added a commit to thejan2009/tilt-extensions that referenced this issue Jan 5, 2024
Sometimes the container root filesystem has sticky bit enabled and
restart commands are ran with a different user. We can work around that
by creating an intermediate folder that has "0777" permissions.

The folder name is parsed with `string.rfind` because os.path.dirname
won't work on Windows.

Refs tilt-dev#540

Signed-off-by: db <[email protected]>
nicks pushed a commit that referenced this issue Jan 5, 2024
Sometimes the container root filesystem has sticky bit enabled and
restart commands are ran with a different user. We can work around that
by creating an intermediate folder that has "0777" permissions.

The folder name is parsed with `string.rfind` because os.path.dirname
won't work on Windows.

Refs #540

Signed-off-by: db <[email protected]>
@nicks
Copy link
Member

nicks commented Jan 5, 2024

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
Copy link
Contributor Author

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
Copy link
Member

nicks commented Jan 8, 2024

eh, it happens.

how would a WORKDIR-based solution work?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants