Fix permission error creating socket directory for non-root instances#85
Fix permission error creating socket directory for non-root instances#85dmcgowan wants to merge 2 commits intocontainerd:mainfrom
Conversation
There was a problem hiding this comment.
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") |
There was a problem hiding this comment.
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)") |
akerouanton
left a comment
akerouanton
left a comment
There was a problem hiding this comment.
Change LGTM. Not sure what's going on with types/fieldpath.proto though.
dmcgowan
force-pushed
the
update-containerd-rootless-permission
branch
from
def128f to
7d3d6be
Compare
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>
dmcgowan
force-pushed
the
update-containerd-rootless-permission
branch
from
7d3d6be to
3f673e9
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Agreed. LGTM |
3 participants
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.