Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 26 additions & 10 deletions common/libnetwork/internal/rootlessnetns/netns_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ const (
resolvConfName = "resolv.conf"
)

var errEmptyPIDFile = errors.New("PID file is empty")

type Netns struct {
// dir used for the rootless netns
dir string
Expand Down Expand Up @@ -310,18 +312,25 @@ func (n *Netns) setupSlirp4netns(nsPath string) error {

func (n *Netns) cleanupRootlessNetns() error {
pidFile := n.getPath(rootlessNetNsConnPidFile)

pid, err := readPidFile(pidFile)
// do not hard error if the file dos not exists, cleanup should be idempotent
if errors.Is(err, fs.ErrNotExist) {
logrus.Debugf("Rootless netns conn pid file does not exists %s", pidFile)
return nil
}
if err == nil {
// kill the slirp/pasta process so we do not leak it
err = unix.Kill(pid, unix.SIGTERM)
if err == unix.ESRCH {
err = nil
}
// if the pid file is empty, pasta failed to start so there is nothing to clean up
if errors.Is(err, errEmptyPIDFile) {
logrus.Debugf("Rootless netns conn pid file is empty, nothing to clean up")
return nil
}
if err != nil {
return fmt.Errorf("reading pid file: %w", err)
}
// kill the slirp/pasta process so we do not leak it
err = unix.Kill(pid, unix.SIGTERM)
if err == unix.ESRCH {
err = nil
Comment on lines +322 to +333
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO we must still return the any other pid file parsing error, so we should keep the logic as is just with the extra ErrEmptyPIDFile branch

}
return err
}
Expand Down Expand Up @@ -602,9 +611,12 @@ func (n *Netns) Run(lock *lockfile.LockFile, toRun func() error) error {
}

if count == 0 {
err = n.cleanup()
if err != nil {
return wrapError("cleanup", err)
cleanupErr := n.cleanup()
if cleanupErr != nil {
if inErr != nil {
return wrapError("cleanup", fmt.Errorf("%v: %w", inErr, cleanupErr))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inErr loses type info, consider using errors.Join.

}
return wrapError("cleanup", cleanupErr)
}
}

Expand Down Expand Up @@ -652,7 +664,11 @@ func readPidFile(path string) (int, error) {
if err != nil {
return 0, err
}
return strconv.Atoi(strings.TrimSpace(string(b)))
s := strings.TrimSpace(string(b))
if s == "" {
return 0, errEmptyPIDFile
}
return strconv.Atoi(s)
}

func (n *Netns) serializeInfo() error {
Expand Down
63 changes: 63 additions & 0 deletions common/libnetwork/internal/rootlessnetns/netns_linux_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,3 +75,66 @@ func Test_refCount(t *testing.T) {
})
}
}

func TestReadPidFile(t *testing.T) {
tests := []struct {
name string
content string
create bool
wantErr error
wantPid int
}{
{
name: "valid pid",
content: "1234\n",
create: true,
wantPid: 1234,
},
{
name: "valid pid no newline",
content: "5678",
create: true,
wantPid: 5678,
},
{
name: "empty pid file",
content: "",
create: true,
wantErr: errEmptyPIDFile,
},
{
name: "only whitespace",
content: " \n ",
create: true,
wantErr: errEmptyPIDFile,
},
{
name: "non-existent file",
create: false,
wantErr: os.ErrNotExist,
},
{
name: "invalid content",
content: "abc",
create: true,
wantErr: strconv.ErrSyntax,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
path := filepath.Join(t.TempDir(), "pidfile")
if tt.create {
err := os.WriteFile(path, []byte(tt.content), 0o600)
assert.NoError(t, err)
}
pid, err := readPidFile(path)
if tt.wantErr != nil {
assert.Error(t, err)
assert.ErrorIs(t, err, tt.wantErr)
} else {
assert.NoError(t, err)
assert.Equal(t, tt.wantPid, pid)
}
})
}
}
Loading