-
Notifications
You must be signed in to change notification settings - Fork 16
Fix permission error creating socket directory for non-root instances #85
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
base: main
Are you sure you want to change the base?
Fix permission error creating socket directory for non-root instances #85
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Updates shim socket creation to support a caller-provided socket directory (intended to avoid permission errors for non-root instances) and bumps the containerd fork/dependency versions accordingly.
Changes:
- Switch shim socket address generation from
shim.SocketAddresstoshim.CreateSocketAddresswith an explicit socket directory/root. - Add support for
opts.SocketDir(with a default fallback) when creating shim sockets (including debug socket). - Update
go.modreplace to a newergithub.com/dmcgowan/containerd/v2pseudo-version and refresh transitive dependencies ingo.sum.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/shim/manager/manager.go | Use a socket directory (from StartOpts) and new address-creation helper to avoid permission issues. |
| go.mod | Update containerd fork replace target and indirect dependency versions. |
| go.sum | Sync sums for updated module graph after dependency changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| s, err := newShimSocket(ctx, opts.Address, grouping, false) | ||
| socketDir := opts.SocketDir | ||
| if socketDir == "" { | ||
| socketDir = filepath.Join(defaults.DefaultStateDir, "s") |
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fallback when opts.SocketDir is empty hardcodes defaults.DefaultStateDir/"s". That can (a) reintroduce the original permission issue for non-root/rootless setups when SocketDir isn’t populated, and (b) ignore a non-default configured state dir. Consider deriving the socket directory from opts.Address (preserving the previous SocketAddress behavior) or returning a clear error instructing the caller to provide SocketDir instead of silently falling back to a potentially unwritable path.
| socketDir = filepath.Join(defaults.DefaultStateDir, "s") | |
| return params, fmt.Errorf("shim socket directory must be specified in options (SocketDir)") |
| func newShimSocket(ctx context.Context, root, path, id string, debug bool) (*shimSocket, error) { | ||
| address, err := shim.CreateSocketAddress(ctx, root, path, id, debug) |
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The newShimSocket parameters root and path are ambiguous in this context (with path actually being the containerd address). Renaming them to something like socketDir and containerdAddress (and updating call sites) would reduce the chance of future misuse, especially now that the function takes both values.
| func newShimSocket(ctx context.Context, root, path, id string, debug bool) (*shimSocket, error) { | |
| address, err := shim.CreateSocketAddress(ctx, root, path, id, debug) | |
| func newShimSocket(ctx context.Context, socketDir, containerdAddress, id string, debug bool) (*shimSocket, error) { | |
| address, err := shim.CreateSocketAddress(ctx, socketDir, containerdAddress, id, debug) |
akerouanton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change LGTM. Not sure what's going on with types/fieldpath.proto though.
Adds ability to pass the socket directory to allow rootless configuration to use a directory over than /var/run/containerd. Signed-off-by: Derek McGowan <derek@mcg.dev>
Signed-off-by: Derek McGowan <derek@mcg.dev>
def128f to
7d3d6be
Compare
Allows containerd to pass through the socket directory to listen in, preventing a permission error with the default /var/run/containerd directory for non-root instances.