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

Re-fix #2767 #7012

Merged
merged 5 commits into from
Oct 18, 2024
Merged

Re-fix #2767 #7012

merged 5 commits into from
Oct 18, 2024

Conversation

xfl12345
Copy link
Contributor

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@xfl12345 xfl12345 requested a review from a team as a code owner September 28, 2024 09:33
@xfl12345
Copy link
Contributor Author

@xfl12345
Copy link
Contributor Author

Reproduce:

  1. docker compose up -d --remove-orphans
  2. Finish the WebUI setup
  3. docker restart code-server

The log will be like as follow:

[2024-09-25T09:10:10.098Z] info  code-server 4.90.3 26c763485b9101fcae2ce56fc0903f0e2d48abe3
[2024-09-25T09:10:10.126Z] info  Using user-data-dir /root/.local/share/code-server
[2024-09-25T09:10:10.274Z] info  Using config file /root/.config/code-server/config.yaml
[2024-09-25T09:10:10.275Z] info  HTTP server listening on http://0.0.0.0:8080/
[2024-09-25T09:10:10.276Z] info    - Authentication is enabled
[2024-09-25T09:10:10.276Z] info      - Using password from /root/.config/code-server/config.yaml
[2024-09-25T09:10:10.277Z] info    - Not serving HTTPS
[2024-09-25T09:10:10.278Z] info  Session server listening on /root/.local/share/code-server/code-server-ipc.sock
[23:20:31] 




[23:20:31] Extension host agent started.
[23:20:35] Deleted uninstalled extension from disk eamodio.gitlens /root/.local/share/code-server/extensions/eamodio.gitlens-15.4.0-universal
usermod: user 'coder' does not exist
usermod: user 'coder' does not exist
usermod: user 'coder' does not exist
usermod: user 'coder' does not exist
usermod: user 'coder' does not exist
usermod: user 'coder' does not exist
usermod: user 'coder' does not exist
usermod: user 'coder' does not exist
usermod: user 'coder' does not exist
usermod: user 'coder' does not exist
usermod: user 'coder' does not exist
usermod: user 'coder' does not exist
usermod: user 'coder' does not exist
usermod: user 'coder' does not exist
usermod: user 'coder' does not exist
usermod: user 'coder' does not exist
usermod: user 'coder' does not exist
usermod: user 'coder' does not exist

@code-asher
Copy link
Member

Thank you for the PR!

Do you know what exactly causes the bug? I tried to reproduce with docker compose and then docker restart but it seems to be working without errors for me. Here is what I ended up with:

# version: '3'
services:
  code_server:
    env_file:
      - path: ./default.env
        required: true
    container_name: code-server
    image: codercom/code-server:latest-xfl
    pull_policy: build
    build:
      context: ./
      dockerfile: code-server-xfl.Dockerfile
    user: "1000:1000"
    volumes:
      - /home/coder/.local:/home/coder/.local
      - /home/coder/.config:/home/coder/.config
    extra_hosts:
      - "host.docker.internal:host-gateway"
    restart: always

Maybe it is the user line? I used 1000 because I do not have a 2006 user. But I am not sure why it would matter.

It seems like the way for the bug to trigger is if whoami returned something other than code-server but it is not clear to me when or why this would happen.

@xfl12345
Copy link
Contributor Author

xfl12345 commented Oct 1, 2024

Thank you for the PR!

Do you know what exactly causes the bug? I tried to reproduce with docker compose and then docker restart but it seems to be working without errors for me. Here is what I ended up with:

# version: '3'
services:
  code_server:
    env_file:
      - path: ./default.env
        required: true
    container_name: code-server
    image: codercom/code-server:latest-xfl
    pull_policy: build
    build:
      context: ./
      dockerfile: code-server-xfl.Dockerfile
    user: "1000:1000"
    volumes:
      - /home/coder/.local:/home/coder/.local
      - /home/coder/.config:/home/coder/.config
    extra_hosts:
      - "host.docker.internal:host-gateway"
    restart: always

Maybe it is the user line? I used 1000 because I do not have a 2006 user. But I am not sure why it would matter.

It seems like the way for the bug to trigger is if whoami returned something other than code-server but it is not clear to me when or why this would happen.

You can create another normal user, one with a UID not equal to 1000.
Maybe code-server is not recommended(or not supported?) to use a UID that is different from 1000?
But it works fine with the patch applied.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@xfl12345
Copy link
Contributor Author

xfl12345 commented Oct 1, 2024

I noticed that code-server was running as root when I specified a UID different from 1000.
It seems that some function is broken after taking that action.

Perhaps code-server does not support using a custom UID.
So, I closed the PR.

@xfl12345 xfl12345 closed this Oct 1, 2024
@xfl12345
Copy link
Contributor Author

xfl12345 commented Oct 1, 2024

I found the reason for "running as root" because I defined docker-compose.yml as below.

services:
  code_server:
    extends:
      file: ./docker-compose.example.yml
      service: code_server
    user: root
    pid: host
    ipc: host
    cap_add:
      - ALL
    security_opt:
      - apparmor=unconfined

Then I disabled some features by commenting out some fields.

services:
  code_server:
    extends:
      file: ./docker-compose.example.yml
      service: code_server
    # user: root
    # pid: host
    # ipc: host
    # cap_add:
    #  - ALL
    security_opt:
      - apparmor=unconfined

Yes, the official code works fine with a custom UID.
But I'm sure the patch has solved another issue, allowing code-server to run as root successfully every time it's restarted.

@xfl12345 xfl12345 reopened this Oct 1, 2024
@code-asher
Copy link
Member

code-asher commented Oct 1, 2024

Ohh interesting! If it runs as root, is that a sign that something else is wrong?

Either way, it seems reasonable to use id to detect if the user already exists. I think we probably do not need the whoami call at all if we do it that way. Would this work?

# Rename coder user to the name in DOCKER_USER if DOCKER_USER does not already exist.
if ! id -u "$DOCKER_USER" 2>/dev/null ; then

@xfl12345
Copy link
Contributor Author

xfl12345 commented Oct 2, 2024

Ohh interesting! If it runs as root, is that a sign that something else is wrong?

Either way, it seems reasonable to use id to detect if the user already exists. I think we probably do not need the whoami call at all if we do it that way. Would this work?

# Rename coder user to the name in DOCKER_USER if DOCKER_USER does not already exist.
if ! id -u "$DOCKER_USER" 2>/dev/null ; then

Because the command will print the UID when it begins to check, I have replaced it with the code as below.

if [ -z "$(id -u "$DOCKER_USER" 2>/dev/null)" ]; then

Test case:

  1. UID[2006], name[code-server]
    • First run: OK
    • Restart: OK
  2. UID[2006], name[xfl12345]
    • First run: OK
    • Restart: OK
  3. UID[2006], name not set
    • First run: OK
    • Restart: OK
  4. UID[1000], name not set
    • First run: OK
    • Restart: OK
  5. UID[1000], name[coder]
    • First run: OK
    • Restart: OK
  6. UID[1000], name[xfl12345]
    • First run: OK
    • Restart: OK
  7. UID[0], name not set
    • First run: OK
    • Restart: OK
  8. UID[0], name[code-server]
    • First run: OK
    • Restart: OK
  9. UID[0], name[root]
    • First run: OK
    • Restart: OK

ALL PASSED! Cheers!

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@code-asher
Copy link
Member

code-asher commented Oct 18, 2024

Sorry for the delay, thank you for fixing this and for the thorough testing! This will be published soon, I think in the next few days after the release candidate has had time to sit.

@code-asher code-asher merged commit dd2e9fc into coder:main Oct 18, 2024
9 checks passed
yiliang114 pushed a commit to yiliang114/code-server that referenced this pull request Jan 23, 2025
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

Successfully merging this pull request may close these issues.

None yet

2 participants