github/kured
1
0
Fork 0
mirror of https://github.com/kubereboot/kured.git synced 2026-02-14 17:39:49 +00:00
Code Issues Packages Projects Releases Wiki Activity

Fix goroutine leak

Browse Source 
Without this patch, we use WriterLevel, which spawns
go routines. As we do it at every call of the util commands,
we spawn goroutines at every check.

This is a problem as it leads to memory management issues.

This fixes it by using a buffer for stdout and stderr, then
logging the results after the command was executed.

To make sure the logging happened at the same place, I inlined
the code from utils. This results in duplicated the code.

However, this is not a big problem as:
- It makes the code more readable
- The implementation between checkers and rebooters _ARE_
  different -- One definitely NEEDS privileges, while the other
  does not... Which could lead to later improvements.

Removing a "utils" package is not really a big deal (it
is kinda a win in itself, as it is an anti-pattern), as the
test coverage was kept.

Partial-Fix: #1004
Fixes: #1013
Signed-off-by: Jean-Philippe Evrard <open-source@a.spamming.party>
This commit is contained in:
Jean-Philippe Evrard
2024-11-05 21:44:53 +01:00
parent 9fbd0a2cc8
commit f20a1ddd05
9 changed files with 136 additions and 125 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
go.mod
View File

@@ -12,7 +12,6 @@ require (
github.com/sirupsen/logrus v1.9.3
github.com/spf13/pflag v1.0.5
github.com/stretchr/testify v1.9.0
gotest.tools/v3 v3.5.1
k8s.io/api v0.29.9
k8s.io/apimachinery v0.29.9
k8s.io/client-go v0.29.9
@@ -39,7 +38,6 @@ require (
github.com/golang/protobuf v1.5.4 // indirect
github.com/google/btree v1.0.1 // indirect
github.com/google/gnostic-models v0.6.8 // indirect
github.com/google/go-cmp v0.6.0 // indirect
github.com/google/gofuzz v1.2.0 // indirect
github.com/google/uuid v1.4.0 // indirect
github.com/gorilla/websocket v1.5.0 // indirect

2
go.sum
View File

@@ -295,8 +295,6 @@ gopkg.in/yaml.v2 v2.4.0/go.mod h1:RDklbk79AGWmwhnvt/jBztapEOGDOx6ZbXqjP6csGnQ=
gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=
gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA=
gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=
gotest.tools/v3 v3.5.1 h1:EENdUnS3pdur5nybKYIh2Vfgc8IUNBjxDPSjtiJcOzU=
gotest.tools/v3 v3.5.1/go.mod h1:isy3WKz7GK6uNw/sbHzfKBLvlvXwUyV06n6brMxxopU=
honnef.co/go/tools v0.0.0-20190102054323-c2f93a96b099/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4=
honnef.co/go/tools v0.0.0-20190523083050-ea95bdfd59fc/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4=
k8s.io/api v0.29.9 h1:FwdflpNsfMUYUOblMZNWJ4K/q0OSL5A4jGa0iOgcJco=

2
internal/validators.go
View File

@@ -28,7 +28,7 @@ func NewRebootChecker(rebootSentinelCommand string, rebootSentinelFile string) (
// An override of rebootSentinelCommand means a privileged command
if rebootSentinelCommand != "" {
log.Infof("Sentinel checker is (privileged) user provided command: %s", rebootSentinelCommand)
return checkers.NewCommandChecker(rebootSentinelCommand)
return checkers.NewCommandChecker(rebootSentinelCommand, 1, true)
}
log.Infof("Sentinel checker is (unprivileged) testing for the presence of: %s", rebootSentinelFile)
return checkers.NewFileRebootChecker(rebootSentinelFile)

40
pkg/checkers/checker.go
View File

@@ -1,9 +1,9 @@
package checkers
import (
"bytes"
"fmt"
"github.com/google/shlex"
"github.com/kubereboot/kured/pkg/util"
log "github.com/sirupsen/logrus"
"os"
"os/exec"
@@ -44,7 +44,7 @@ func NewFileRebootChecker(filePath string) (*FileRebootChecker, error) {
// CommandChecker is using a custom command to check
// if a reboot is required. There are two modes of behaviour,
// if Privileged is granted, the NamespacePid is used to enter
// if Privileged is granted, the NamespacePid is used to nsenter
// the given PID's namespace.
type CommandChecker struct {
CheckCommand []string
@@ -53,16 +53,15 @@ type CommandChecker struct {
}
// RebootRequired for CommandChecker runs a command without returning
// any eventual error. THis should be later refactored to remove the util wrapper
// and return the errors, instead of logging them here.
// any eventual error. This should be later refactored to return the errors,
// instead of logging and fataling them here.
func (rc CommandChecker) RebootRequired() bool {
var cmdline []string
if rc.Privileged {
cmdline = util.PrivilegedHostCommand(rc.NamespacePid, rc.CheckCommand)
} else {
cmdline = rc.CheckCommand
}
cmd := util.NewCommand(cmdline[0], cmdline[1:]...)
bufStdout := new(bytes.Buffer)
bufStderr := new(bytes.Buffer)
cmd := exec.Command(rc.CheckCommand[0], rc.CheckCommand[1:]...)
cmd.Stdout = bufStdout
cmd.Stderr = bufStderr
if err := cmd.Run(); err != nil {
switch err := err.(type) {
case *exec.ExitError:
@@ -80,19 +79,30 @@ func (rc CommandChecker) RebootRequired() bool {
log.Fatalf("Error invoking sentinel command: %v", err)
}
}
log.Info("checking if reboot is required", "cmd", cmd.Args, "stdout", bufStdout, "stderr", bufStderr)
return true
}
// NewCommandChecker is the constructor for the commandChecker, and by default
// runs new commands in a privileged fashion.
func NewCommandChecker(sentinelCommand string) (*CommandChecker, error) {
cmd, err := shlex.Split(sentinelCommand)
// Privileged means wrapping the command with nsenter.
// It allows to run a command from systemd's namespace for example (pid 1)
// This relies on hostPID:true and privileged:true to enter host mount space
// For info, rancher based need different pid, which should be user given.
// until we have a better discovery mechanism.
func NewCommandChecker(sentinelCommand string, pid int, privileged bool) (*CommandChecker, error) {
var cmd []string
if privileged {
cmd = append(cmd, "/usr/bin/nsenter", fmt.Sprintf("-m/proc/%d/ns/mnt", pid), "--")
}
parsedCommand, err := shlex.Split(sentinelCommand)
if err != nil {
return nil, fmt.Errorf("error parsing provided sentinel command: %v", err)
}
cmd = append(cmd, parsedCommand...)
return &CommandChecker{
CheckCommand: cmd,
NamespacePid: 1,
Privileged: true,
NamespacePid: pid,
Privileged: privileged,
}, nil
}

86
pkg/checkers/checker_test.go
View File

@@ -2,68 +2,86 @@ package checkers
import (
log "github.com/sirupsen/logrus"
assert "gotest.tools/v3/assert"
"reflect"
"testing"
)
func Test_nsEntering(t *testing.T) {
type args struct {
pid int
command string
privileged bool
}
tests := []struct {
name string
args args
want []string
}{
{
name: "Ensure command will run with nsenter",
args: args{pid: 1, command: "ls -Fal", privileged: true},
want: []string{"/usr/bin/nsenter", "-m/proc/1/ns/mnt", "--", "ls", "-Fal"},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
cc, _ := NewCommandChecker(tt.args.command, tt.args.pid, tt.args.privileged)
if !reflect.DeepEqual(cc.CheckCommand, tt.want) {
t.Errorf("command parsed as %v, want %v", cc.CheckCommand, tt.want)
}
})
}
}
func Test_rebootRequired(t *testing.T) {
type args struct {
sentinelCommand []string
}
tests := []struct {
name string
args args
want bool
name string
args args
want bool
fatals bool
}{
{
name: "Ensure rc = 0 means reboot required",
args: args{
sentinelCommand: []string{"true"},
},
want: true,
want: true,
fatals: false,
},
{
name: "Ensure rc != 0 means reboot NOT required",
args: args{
sentinelCommand: []string{"false"},
},
want: false,
want: false,
fatals: false,
},
{
name: "Ensure a wrong command fatals",
args: args{
sentinelCommand: []string{"./babar"},
},
want: true,
fatals: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
defer func() { log.StandardLogger().ExitFunc = nil }()
fatal := false
log.StandardLogger().ExitFunc = func(int) { fatal = true }
a := CommandChecker{CheckCommand: tt.args.sentinelCommand, NamespacePid: 1, Privileged: false}
if got := a.RebootRequired(); got != tt.want {
t.Errorf("rebootRequired() = %v, want %v", got, tt.want)
}
if tt.fatals != fatal {
t.Errorf("fatal flag is %v, want fatal %v", fatal, tt.fatals)
}
})
}
}
func Test_rebootRequired_fatals(t *testing.T) {
cases := []struct {
param []string
expectFatal bool
}{
{
param: []string{"true"},
expectFatal: false,
},
{
param: []string{"./babar"},
expectFatal: true,
},
}
defer func() { log.StandardLogger().ExitFunc = nil }()
var fatal bool
log.StandardLogger().ExitFunc = func(int) { fatal = true }
for _, c := range cases {
fatal = false
a := CommandChecker{CheckCommand: c.param, NamespacePid: 1, Privileged: false}
a.RebootRequired()
assert.Equal(t, c.expectFatal, fatal)
}
}

20
pkg/reboot/command.go
View File

@@ -1,10 +1,11 @@
package reboot
import (
"bytes"
"fmt"
"github.com/google/shlex"
"github.com/kubereboot/kured/pkg/util"
log "github.com/sirupsen/logrus"
"os/exec"
)
// CommandRebooter holds context-information for a reboot with command
@@ -15,9 +16,17 @@ type CommandRebooter struct {
// Reboot triggers the reboot command
func (c CommandRebooter) Reboot() error {
log.Infof("Invoking command: %s", c.RebootCommand)
if err := util.NewCommand(c.RebootCommand[0], c.RebootCommand[1:]...).Run(); err != nil {
bufStdout := new(bytes.Buffer)
bufStderr := new(bytes.Buffer)
cmd := exec.Command(c.RebootCommand[0], c.RebootCommand[1:]...)
cmd.Stdout = bufStdout
cmd.Stderr = bufStderr
if err := cmd.Run(); err != nil {
return fmt.Errorf("error invoking reboot command %s: %v", c.RebootCommand, err)
}
log.Info("Invoked reboot command", "cmd", cmd.Args, "stdout", bufStdout, "stderr", bufStderr)
return nil
}
@@ -28,10 +37,11 @@ func NewCommandRebooter(rebootCommand string) (*CommandRebooter, error) {
if rebootCommand == "" {
return nil, fmt.Errorf("no reboot command specified")
}
cmd, err := shlex.Split(rebootCommand)
cmd := []string{"/usr/bin/nsenter", fmt.Sprintf("-m/proc/%d/ns/mnt", 1), "--"}
parsedCommand, err := shlex.Split(rebootCommand)
if err != nil {
return nil, fmt.Errorf("error %v when parsing reboot command %s", err, rebootCommand)
}
return &CommandRebooter{RebootCommand: util.PrivilegedHostCommand(1, cmd)}, nil
cmd = append(cmd, parsedCommand...)
return &CommandRebooter{RebootCommand: cmd}, nil
}

43
pkg/reboot/command_test.go Normal file
View File

@@ -0,0 +1,43 @@
package reboot
import (
"reflect"
"testing"
)
func TestNewCommandRebooter(t *testing.T) {
type args struct {
rebootCommand string
}
tests := []struct {
name string
args args
want *CommandRebooter
wantErr bool
}{
{
name: "Ensure command is nsenter wrapped",
args: args{"ls -Fal"},
want: &CommandRebooter{RebootCommand: []string{"/usr/bin/nsenter", "-m/proc/1/ns/mnt", "--", "ls", "-Fal"}},
wantErr: false,
},
{
name: "Ensure empty command is erroring",
args: args{""},
want: nil,
wantErr: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got, err := NewCommandRebooter(tt.args.rebootCommand)
if (err != nil) != tt.wantErr {
t.Errorf("NewCommandRebooter() error = %v, wantErr %v", err, tt.wantErr)
return
}
if !reflect.DeepEqual(got, tt.want) {
t.Errorf("NewCommandRebooter() got = %v, want %v", got, tt.want)
}
})
}
}

35
pkg/util/util.go
View File

@@ -1,35 +0,0 @@
package util
import (
"fmt"
"os/exec"
log "github.com/sirupsen/logrus"
)
// NewCommand creates a new Command with stdout/stderr wired to our standard logger
func NewCommand(name string, arg ...string) *exec.Cmd {
cmd := exec.Command(name, arg...)
cmd.Stdout = log.NewEntry(log.StandardLogger()).
WithField("cmd", cmd.Args[0]).
WithField("std", "out").
WriterLevel(log.InfoLevel)
cmd.Stderr = log.NewEntry(log.StandardLogger()).
WithField("cmd", cmd.Args[0]).
WithField("std", "err").
WriterLevel(log.WarnLevel)
return cmd
}
// PrivilegedHostCommand wraps the command with nsenter.
// It allows to run a command from systemd's namespace for example (pid 1)
// This relies on hostPID:true and privileged:true to enter host mount space
// For info, rancher based need different pid, which should be user given.
// until we have a better discovery mechanism.
func PrivilegedHostCommand(pid int, command []string) []string {
cmd := []string{"/usr/bin/nsenter", fmt.Sprintf("-m/proc/%d/ns/mnt", pid), "--"}
cmd = append(cmd, command...)
return cmd
}

31
pkg/util/util_test.go
View File

@@ -1,31 +0,0 @@
package util
import (
"reflect"
"testing"
)
func Test_buildHostCommand(t *testing.T) {
type args struct {
pid int
command []string
}
tests := []struct {
name string
args args
want []string
}{
{
name: "Ensure command will run with nsenter",
args: args{pid: 1, command: []string{"ls", "-Fal"}},
want: []string{"/usr/bin/nsenter", "-m/proc/1/ns/mnt", "--", "ls", "-Fal"},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if got := PrivilegedHostCommand(tt.args.pid, tt.args.command); !reflect.DeepEqual(got, tt.want) {
t.Errorf("buildHostCommand() = %v, want %v", got, tt.want)
}
})
}
}