Skip to content

Conversation

@Fricounet
Copy link
Contributor

Overview

Please briefly describe the changes your pull request makes.

Similar to what containerd allows, the socket will be chowned to a different user. The main benefit is that is makes the socket callable by users other than root.

Related Issues

Please link to the relevant issue. For example: Fix #123 or Related #456.

Change Details

Please describe your changes in detail:

Test Results

If you have any relevant screenshots or videos that can help illustrate your changes, please add them here.

I started the snapshotter from this branch:

sudo ./containerd-nydus-grpc --config ./nydus.toml
INFO[2025-10-30T17:45:12.958791294+01:00] Start nydus-snapshotter. Version: v0.15.4-10-gf6677ba.m, PID: 204002, FsDriver: fusedev, DaemonMode: multiple
INFO[2025-10-30T17:45:12.963653295+01:00] Run daemons monitor...
INFO[2025-10-30T17:45:12.967405469+01:00] Started system controller on "/containerd-local/sock/nydus-system.sock"
INFO[2025-10-30T17:45:12.967501568+01:00] Start system controller API server on /containerd-local/sock/nydus-system.sock
INFO[2025-10-30T17:45:12.968131017+01:00] setup image proxy keychain                    target-image-service=/containerd-local/sock/containerd.sock
DEBU[2025-10-30T17:45:12.968258712+01:00] Waiting for CRI service is started...
INFO[2025-10-30T17:45:12.968614159+01:00] connected to backend CRI service

using the following config

version = 1

root = "/containerd-local/io.containerd.snapshotter.v1.nydus"
# The snapshotter's GRPC server socket, containerd will connect to plugin on this socket
address = "/containerd-local/sock/image-service.sock"
uid = 1000
gid = 1000

[system]
# Snapshotter's debug and trace HTTP server interface
enable = true
uid = 1000
gid = 1000
address = "/containerd-local/sock/nydus-system.sock"

which resulted in both the snapshotter and the system socket to be created under my user:

ls /containerd-local/sock/
srwxr-x--- 0 baptiste.girardcarrabin 30 oct.  17:45 image-service.sock
srwxr-x--- 0 baptiste.girardcarrabin 30 oct.  17:45 nydus-system.sock

Change Type

Please select the type of change your pull request relates to:

  • Bug Fix
  • Feature Addition
  • Documentation Update
  • Code Refactoring
  • Performance Improvement
  • Other (please describe)

Self-Checklist

Before submitting a pull request, please ensure you have completed the following:

  • I have run a code style check and addressed any warnings/errors.
  • I have added appropriate comments to my code (if applicable).
  • I have updated the documentation (if applicable).
  • I have written appropriate unit tests.

Add a signal handler to the system socket logic to be able to clean it up on termination

Signed-off-by: Baptiste Girard-Carrabin <[email protected]>
Similar to what containerd allows, the socket will be chowned to a different user. The main benefit is that is makes the socket callable by users other than root.

Signed-off-by: Baptiste Girard-Carrabin <[email protected]>
@Fricounet Fricounet force-pushed the fricounet/socket-uid branch from 25b95b3 to 0d09129 Compare October 30, 2025 16:56
@codecov
Copy link

codecov bot commented Oct 30, 2025

Codecov Report

❌ Patch coverage is 0% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 20.89%. Comparing base (036bec7) to head (0d09129).
⚠️ Report is 20 commits behind head on main.

Files with missing lines Patch % Lines
pkg/system/system.go 0.00% 10 Missing ⚠️
cmd/containerd-nydus-grpc/snapshotter.go 0.00% 4 Missing ⚠️
snapshot/snapshot.go 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #685      +/-   ##
==========================================
- Coverage   21.03%   20.89%   -0.14%     
==========================================
  Files         122      122              
  Lines       10995    11068      +73     
==========================================
  Hits         2313     2313              
- Misses       8363     8436      +73     
  Partials      319      319              
Files with missing lines Coverage Δ
config/config.go 38.33% <ø> (ø)
snapshot/snapshot.go 0.00% <0.00%> (ø)
cmd/containerd-nydus-grpc/snapshotter.go 0.00% <0.00%> (ø)
pkg/system/system.go 5.28% <0.00%> (-0.21%) ⬇️

... and 10 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@imeoer
Copy link
Collaborator

imeoer commented Oct 31, 2025

Thanks for the PR! Can we allow configuring (call os.Chmod after net.ListenUnix) the permission 0755 of this sock? To be honest, I don't quite understand why containerd uses UID and GID instead of simpler permission bits.

@Fricounet
Copy link
Contributor Author

@imeoer just to make sure I understand, you'd prefer we just chmod 0755 the socket and not add the new uid/gid configs from this PR
OR do you want both to be present (the chmod + the uid/gid)?

I'm fine with either choice, just want to make sure I understand what you prefer

@imeoer
Copy link
Collaborator

imeoer commented Nov 3, 2025

@Fricounet Thanks for the explanation, it's also fine if the usage is consistent with containerd.

@imeoer imeoer merged commit eda72f0 into containerd:main Nov 3, 2025
16 checks passed
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.

2 participants