From 1cc78b03cae28372f57776b0c2a72bec9f416fa3 Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Thu, 17 Mar 2022 16:51:11 -0700 Subject: [PATCH 1/6] Add configuration file and runtime management of NGINX Problem: (1) Add/update/delete NGINX configuration files to/from the file system. (2) Reload NGINX to apply changes in the configuration files. Solution: For (1): Add a new component - file.Manager - that allows writing and removing configuration files for a server. The Manager writes server configuration files to /etc/nginx/conf.d folder. This folder is available to both gateway and NGINX containers as part of the shared volume /etc/nginx For (2): Add a new component - runtime.Manager - that allows reload NGINX. The Manager reloads NGINX by sending a HUP signal to the NGINX master process. Because the gateway process is running under user 1001 (rather than root), during the image build we add the CAP_KILL capability to the gateway binary. --- build/Dockerfile | 17 +- deploy/manifests/nginx-gateway.yaml | 13 +- internal/events/loop.go | 60 ++++-- internal/events/loop_test.go | 38 +++- internal/manager/manager.go | 6 +- internal/nginx/file/filefakes/fake_manager.go | 192 ++++++++++++++++++ internal/nginx/file/manager.go | 63 ++++++ internal/nginx/file/manager_test.go | 12 ++ internal/nginx/runtime/manager.go | 93 +++++++++ internal/nginx/runtime/manager_test.go | 98 +++++++++ .../runtime/runtimefakes/fake_manager.go | 112 ++++++++++ 11 files changed, 672 insertions(+), 32 deletions(-) create mode 100644 internal/nginx/file/filefakes/fake_manager.go create mode 100644 internal/nginx/file/manager.go create mode 100644 internal/nginx/file/manager_test.go create mode 100644 internal/nginx/runtime/manager.go create mode 100644 internal/nginx/runtime/manager_test.go create mode 100644 internal/nginx/runtime/runtimefakes/fake_manager.go diff --git a/build/Dockerfile b/build/Dockerfile index 3b82d3f3b5..7b7e52170f 100644 --- a/build/Dockerfile +++ b/build/Dockerfile @@ -11,14 +11,25 @@ RUN go mod download COPY cmd /go/src/github.com/nginxinc/nginx-gateway-kubernetes/cmd COPY internal /go/src/github.com/nginxinc/nginx-gateway-kubernetes/internal COPY pkg /go/src/github.com/nginxinc/nginx-gateway-kubernetes/pkg -RUN CGO_ENABLED=0 GOOS=linux go build -trimpath -a -ldflags "-s -w -X main.version=${VERSION} -X main.commit=${GIT_COMMIT} -X main.date=${DATE}" -o ./build/.out/gateway . +RUN CGO_ENABLED=0 GOOS=linux go build -trimpath -a -ldflags "-s -w -X main.version=${VERSION} -X main.commit=${GIT_COMMIT} -X main.date=${DATE}" -o gateway . + +FROM alpine:3.15 as capabilizer +RUN apk add --no-cache libcap + +FROM capabilizer as local-capabilizer +COPY ./build/.out/gateway /usr/bin/ +RUN setcap 'cap_kill=+ep' /usr/bin/gateway + +FROM capabilizer as container-capabilizer +COPY --from=builder /go/src/github.com/nginxinc/nginx-gateway-kubernetes/cmd/gateway/gateway /usr/bin/ +RUN setcap 'cap_kill=+ep' /usr/bin/gateway FROM scratch as common USER 1001:1001 ENTRYPOINT [ "/usr/bin/gateway" ] FROM common as container -COPY --from=builder /go/src/github.com/nginxinc/nginx-gateway-kubernetes/cmd/gateway/gateway /usr/bin/ +COPY --from=container-capabilizer /usr/bin/gateway /usr/bin/ FROM common as local -COPY ./build/.out/gateway /usr/bin/ +COPY --from=local-capabilizer /usr/bin/gateway /usr/bin/ \ No newline at end of file diff --git a/deploy/manifests/nginx-gateway.yaml b/deploy/manifests/nginx-gateway.yaml index defad3a0ff..67ed2c4f18 100644 --- a/deploy/manifests/nginx-gateway.yaml +++ b/deploy/manifests/nginx-gateway.yaml @@ -72,14 +72,15 @@ spec: labels: app: nginx-gateway spec: + shareProcessNamespace: true serviceAccountName: nginx-gateway volumes: - name: nginx-config emptyDir: { } initContainers: - - image: busybox:1.34 + - image: busybox:1.34 # FIXME(pleshakov): use gateway container to init the Config with proper main config name: nginx-config-initializer - command: [ 'sh', '-c', 'echo "events {} http { server { default_type text/plain; return 200 \"hello from \$hostname\n\"; } }" > /etc/nginx/nginx.conf' ] + command: [ 'sh', '-c', 'echo "events {} http { include /etc/nginx/conf.d/*.conf; server { default_type text/html; return 404; } }" > /etc/nginx/nginx.conf && mkdir /etc/nginx/conf.d && chown 1001:0 /etc/nginx/conf.d' ] volumeMounts: - name: nginx-config mountPath: /etc/nginx @@ -87,6 +88,14 @@ spec: - image: nginx-gateway:0.0.1 imagePullPolicy: IfNotPresent name: nginx-gateway + volumeMounts: + - name: nginx-config + mountPath: /etc/nginx + securityContext: + runAsUser: 1001 + # FIXME(pleshakov) - figure out which capabilities are required + # dropping ALL and adding only CAP_KILL doesn't work + # Note: CAP_KILL is needed for sending HUP signal to NGINX master args: - --gateway-ctlr-name=k8s-gateway.nginx.org/nginx-gateway/gateway - image: nginx:1.21.3 diff --git a/internal/events/loop.go b/internal/events/loop.go index 47a3e9e1de..0ef6c7de63 100644 --- a/internal/events/loop.go +++ b/internal/events/loop.go @@ -6,6 +6,8 @@ import ( "github.com/go-logr/logr" "github.com/nginxinc/nginx-gateway-kubernetes/internal/nginx/config" + "github.com/nginxinc/nginx-gateway-kubernetes/internal/nginx/file" + "github.com/nginxinc/nginx-gateway-kubernetes/internal/nginx/runtime" "github.com/nginxinc/nginx-gateway-kubernetes/internal/state" "github.com/nginxinc/nginx-gateway-kubernetes/internal/status" apiv1 "k8s.io/api/core/v1" @@ -14,12 +16,14 @@ import ( // EventLoop is the main event loop of the Gateway. type EventLoop struct { - conf state.Configuration - serviceStore state.ServiceStore - generator config.Generator - eventCh <-chan interface{} - logger logr.Logger - statusUpdater status.Updater + conf state.Configuration + serviceStore state.ServiceStore + generator config.Generator + eventCh <-chan interface{} + logger logr.Logger + statusUpdater status.Updater + nginxFileMgr file.Manager + nginxRuntimeMgr runtime.Manager } // NewEventLoop creates a new EventLoop. @@ -30,14 +34,18 @@ func NewEventLoop( eventCh <-chan interface{}, statusUpdater status.Updater, logger logr.Logger, + nginxFileMgr file.Manager, + nginxRuntimeMgr runtime.Manager, ) *EventLoop { return &EventLoop{ - conf: conf, - serviceStore: serviceStore, - generator: generator, - eventCh: eventCh, - statusUpdater: statusUpdater, - logger: logger.WithName("eventLoop"), + conf: conf, + serviceStore: serviceStore, + generator: generator, + eventCh: eventCh, + statusUpdater: statusUpdater, + logger: logger.WithName("eventLoop"), + nginxFileMgr: nginxFileMgr, + nginxRuntimeMgr: nginxRuntimeMgr, } } @@ -118,14 +126,8 @@ func (el *EventLoop) processChangesAndStatusUpdates(ctx context.Context, changes el.logger.Info("Processing a change", "host", c.Host.Value) - // TO-DO: This code is temporary. We will remove it once we have a component that processes changes. - fmt.Printf("%+v\n", c) - if c.Op == state.Upsert { cfg, warnings := el.generator.GenerateForHost(c.Host) - // TO-DO: for now, we only print the generated config, without writing it on the file system - // and reloading NGINX. - fmt.Println(string(cfg)) for obj, objWarnings := range warnings { for _, w := range objWarnings { @@ -137,6 +139,28 @@ func (el *EventLoop) processChangesAndStatusUpdates(ctx context.Context, changes "warning", w) } } + + el.logger.Info("Writing configuration", + "host", c.Host.Value) + + err := el.nginxFileMgr.WriteServerConfig(c.Host.Value, cfg) + if err != nil { + el.logger.Error(err, "Failed to write configuration", + "host", c.Host.Value) + } + } else { + err := el.nginxFileMgr.DeleteServerConfig(c.Host.Value) + if err != nil { + el.logger.Error(err, "Failed to delete configuration", + "host", c.Host.Value) + } + } + } + + if len(changes) > 0 { + err := el.nginxRuntimeMgr.Reload(ctx) + if err != nil { + el.logger.Error(err, "Failed to reload NGINX") } } diff --git a/internal/events/loop_test.go b/internal/events/loop_test.go index 0b2b2d2129..05186ece50 100644 --- a/internal/events/loop_test.go +++ b/internal/events/loop_test.go @@ -4,7 +4,10 @@ import ( "context" "github.com/nginxinc/nginx-gateway-kubernetes/internal/events" + "github.com/nginxinc/nginx-gateway-kubernetes/internal/nginx/config" "github.com/nginxinc/nginx-gateway-kubernetes/internal/nginx/config/configfakes" + "github.com/nginxinc/nginx-gateway-kubernetes/internal/nginx/file/filefakes" + "github.com/nginxinc/nginx-gateway-kubernetes/internal/nginx/runtime/runtimefakes" "github.com/nginxinc/nginx-gateway-kubernetes/internal/state" "github.com/nginxinc/nginx-gateway-kubernetes/internal/state/statefakes" "github.com/nginxinc/nginx-gateway-kubernetes/internal/status/statusfakes" @@ -37,6 +40,8 @@ var _ = Describe("EventLoop", func() { var fakeUpdater *statusfakes.FakeUpdater var fakeServiceStore *statefakes.FakeServiceStore var fakeGenerator *configfakes.FakeGenerator + var fakeNginxFimeMgr *filefakes.FakeManager + var fakeNginxRuntimeMgr *runtimefakes.FakeManager var cancel context.CancelFunc var eventCh chan interface{} var errorCh chan error @@ -47,7 +52,9 @@ var _ = Describe("EventLoop", func() { fakeUpdater = &statusfakes.FakeUpdater{} fakeServiceStore = &statefakes.FakeServiceStore{} fakeGenerator = &configfakes.FakeGenerator{} - ctrl = events.NewEventLoop(fakeConf, fakeServiceStore, fakeGenerator, eventCh, fakeUpdater, zap.New()) + fakeNginxFimeMgr = &filefakes.FakeManager{} + fakeNginxRuntimeMgr = &runtimefakes.FakeManager{} + ctrl = events.NewEventLoop(fakeConf, fakeServiceStore, fakeGenerator, eventCh, fakeUpdater, zap.New(), fakeNginxFimeMgr, fakeNginxRuntimeMgr) var ctx context.Context @@ -71,8 +78,10 @@ var _ = Describe("EventLoop", func() { It("should process upsert event", func() { fakeChanges := []state.Change{ { - Op: state.Upsert, - Host: state.Host{}, + Op: state.Upsert, + Host: state.Host{ + Value: "example.com", + }, }, } fakeStatusUpdates := []state.StatusUpdate{ @@ -83,6 +92,9 @@ var _ = Describe("EventLoop", func() { } fakeConf.UpsertHTTPRouteReturns(fakeChanges, fakeStatusUpdates) + fakeCfg := []byte("fake") + fakeGenerator.GenerateForHostReturns(fakeCfg, config.Warnings{}) + hr := &v1alpha2.HTTPRoute{} eventCh <- &events.UpsertEvent{ @@ -102,13 +114,22 @@ var _ = Describe("EventLoop", func() { Eventually(fakeGenerator.GenerateForHostCallCount).Should(Equal(1)) Expect(fakeGenerator.GenerateForHostArgsForCall(0)).Should(Equal(fakeChanges[0].Host)) + + Eventually(fakeNginxFimeMgr.WriteServerConfigCallCount).Should(Equal(1)) + host, cfg := fakeNginxFimeMgr.WriteServerConfigArgsForCall(0) + Expect(host).Should(Equal("example.com")) + Expect(cfg).Should(Equal(fakeCfg)) + + Eventually(fakeNginxRuntimeMgr.ReloadCallCount).Should(Equal(1)) }) It("should process delete event", func() { fakeChanges := []state.Change{ { - Op: state.Delete, - Host: state.Host{}, + Op: state.Delete, + Host: state.Host{ + Value: "example.com", + }, }, } fakeStatusUpdates := []state.StatusUpdate{ @@ -137,9 +158,10 @@ var _ = Describe("EventLoop", func() { return updates }).Should(Equal(fakeStatusUpdates)) - // TO-DO: - // once we have a component that processes host deletion, ensure that - // its corresponding method is eventually called + Eventually(fakeNginxFimeMgr.DeleteServerConfigCallCount).Should(Equal(1)) + Expect(fakeNginxFimeMgr.DeleteServerConfigArgsForCall(0)).Should(Equal("example.com")) + + Eventually(fakeNginxRuntimeMgr.ReloadCallCount).Should(Equal(1)) }) }) diff --git a/internal/manager/manager.go b/internal/manager/manager.go index 0c09d0e1c9..70c67ce30b 100644 --- a/internal/manager/manager.go +++ b/internal/manager/manager.go @@ -12,6 +12,8 @@ import ( hr "github.com/nginxinc/nginx-gateway-kubernetes/internal/implementations/httproute" svc "github.com/nginxinc/nginx-gateway-kubernetes/internal/implementations/service" ngxcfg "github.com/nginxinc/nginx-gateway-kubernetes/internal/nginx/config" + "github.com/nginxinc/nginx-gateway-kubernetes/internal/nginx/file" + ngxruntime "github.com/nginxinc/nginx-gateway-kubernetes/internal/nginx/runtime" "github.com/nginxinc/nginx-gateway-kubernetes/internal/state" "github.com/nginxinc/nginx-gateway-kubernetes/internal/status" nginxgwv1alpha1 "github.com/nginxinc/nginx-gateway-kubernetes/pkg/apis/gateway/v1alpha1" @@ -78,7 +80,9 @@ func Start(cfg config.Config) error { serviceStore := state.NewServiceStore() reporter := status.NewUpdater(mgr.GetClient(), cfg.Logger) configGenerator := ngxcfg.NewGeneratorImpl(serviceStore) - eventLoop := events.NewEventLoop(conf, serviceStore, configGenerator, eventCh, reporter, cfg.Logger) + nginxFileMgr := file.NewManagerImpl() + nginxRuntimeMgr := ngxruntime.NewManagerImpl() + eventLoop := events.NewEventLoop(conf, serviceStore, configGenerator, eventCh, reporter, cfg.Logger, nginxFileMgr, nginxRuntimeMgr) err = mgr.Add(eventLoop) if err != nil { diff --git a/internal/nginx/file/filefakes/fake_manager.go b/internal/nginx/file/filefakes/fake_manager.go new file mode 100644 index 0000000000..d33e31188f --- /dev/null +++ b/internal/nginx/file/filefakes/fake_manager.go @@ -0,0 +1,192 @@ +// Code generated by counterfeiter. DO NOT EDIT. +package filefakes + +import ( + "sync" + + "github.com/nginxinc/nginx-gateway-kubernetes/internal/nginx/file" +) + +type FakeManager struct { + DeleteServerConfigStub func(string) error + deleteServerConfigMutex sync.RWMutex + deleteServerConfigArgsForCall []struct { + arg1 string + } + deleteServerConfigReturns struct { + result1 error + } + deleteServerConfigReturnsOnCall map[int]struct { + result1 error + } + WriteServerConfigStub func(string, []byte) error + writeServerConfigMutex sync.RWMutex + writeServerConfigArgsForCall []struct { + arg1 string + arg2 []byte + } + writeServerConfigReturns struct { + result1 error + } + writeServerConfigReturnsOnCall map[int]struct { + result1 error + } + invocations map[string][][]interface{} + invocationsMutex sync.RWMutex +} + +func (fake *FakeManager) DeleteServerConfig(arg1 string) error { + fake.deleteServerConfigMutex.Lock() + ret, specificReturn := fake.deleteServerConfigReturnsOnCall[len(fake.deleteServerConfigArgsForCall)] + fake.deleteServerConfigArgsForCall = append(fake.deleteServerConfigArgsForCall, struct { + arg1 string + }{arg1}) + stub := fake.DeleteServerConfigStub + fakeReturns := fake.deleteServerConfigReturns + fake.recordInvocation("DeleteServerConfig", []interface{}{arg1}) + fake.deleteServerConfigMutex.Unlock() + if stub != nil { + return stub(arg1) + } + if specificReturn { + return ret.result1 + } + return fakeReturns.result1 +} + +func (fake *FakeManager) DeleteServerConfigCallCount() int { + fake.deleteServerConfigMutex.RLock() + defer fake.deleteServerConfigMutex.RUnlock() + return len(fake.deleteServerConfigArgsForCall) +} + +func (fake *FakeManager) DeleteServerConfigCalls(stub func(string) error) { + fake.deleteServerConfigMutex.Lock() + defer fake.deleteServerConfigMutex.Unlock() + fake.DeleteServerConfigStub = stub +} + +func (fake *FakeManager) DeleteServerConfigArgsForCall(i int) string { + fake.deleteServerConfigMutex.RLock() + defer fake.deleteServerConfigMutex.RUnlock() + argsForCall := fake.deleteServerConfigArgsForCall[i] + return argsForCall.arg1 +} + +func (fake *FakeManager) DeleteServerConfigReturns(result1 error) { + fake.deleteServerConfigMutex.Lock() + defer fake.deleteServerConfigMutex.Unlock() + fake.DeleteServerConfigStub = nil + fake.deleteServerConfigReturns = struct { + result1 error + }{result1} +} + +func (fake *FakeManager) DeleteServerConfigReturnsOnCall(i int, result1 error) { + fake.deleteServerConfigMutex.Lock() + defer fake.deleteServerConfigMutex.Unlock() + fake.DeleteServerConfigStub = nil + if fake.deleteServerConfigReturnsOnCall == nil { + fake.deleteServerConfigReturnsOnCall = make(map[int]struct { + result1 error + }) + } + fake.deleteServerConfigReturnsOnCall[i] = struct { + result1 error + }{result1} +} + +func (fake *FakeManager) WriteServerConfig(arg1 string, arg2 []byte) error { + var arg2Copy []byte + if arg2 != nil { + arg2Copy = make([]byte, len(arg2)) + copy(arg2Copy, arg2) + } + fake.writeServerConfigMutex.Lock() + ret, specificReturn := fake.writeServerConfigReturnsOnCall[len(fake.writeServerConfigArgsForCall)] + fake.writeServerConfigArgsForCall = append(fake.writeServerConfigArgsForCall, struct { + arg1 string + arg2 []byte + }{arg1, arg2Copy}) + stub := fake.WriteServerConfigStub + fakeReturns := fake.writeServerConfigReturns + fake.recordInvocation("WriteServerConfig", []interface{}{arg1, arg2Copy}) + fake.writeServerConfigMutex.Unlock() + if stub != nil { + return stub(arg1, arg2) + } + if specificReturn { + return ret.result1 + } + return fakeReturns.result1 +} + +func (fake *FakeManager) WriteServerConfigCallCount() int { + fake.writeServerConfigMutex.RLock() + defer fake.writeServerConfigMutex.RUnlock() + return len(fake.writeServerConfigArgsForCall) +} + +func (fake *FakeManager) WriteServerConfigCalls(stub func(string, []byte) error) { + fake.writeServerConfigMutex.Lock() + defer fake.writeServerConfigMutex.Unlock() + fake.WriteServerConfigStub = stub +} + +func (fake *FakeManager) WriteServerConfigArgsForCall(i int) (string, []byte) { + fake.writeServerConfigMutex.RLock() + defer fake.writeServerConfigMutex.RUnlock() + argsForCall := fake.writeServerConfigArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2 +} + +func (fake *FakeManager) WriteServerConfigReturns(result1 error) { + fake.writeServerConfigMutex.Lock() + defer fake.writeServerConfigMutex.Unlock() + fake.WriteServerConfigStub = nil + fake.writeServerConfigReturns = struct { + result1 error + }{result1} +} + +func (fake *FakeManager) WriteServerConfigReturnsOnCall(i int, result1 error) { + fake.writeServerConfigMutex.Lock() + defer fake.writeServerConfigMutex.Unlock() + fake.WriteServerConfigStub = nil + if fake.writeServerConfigReturnsOnCall == nil { + fake.writeServerConfigReturnsOnCall = make(map[int]struct { + result1 error + }) + } + fake.writeServerConfigReturnsOnCall[i] = struct { + result1 error + }{result1} +} + +func (fake *FakeManager) Invocations() map[string][][]interface{} { + fake.invocationsMutex.RLock() + defer fake.invocationsMutex.RUnlock() + fake.deleteServerConfigMutex.RLock() + defer fake.deleteServerConfigMutex.RUnlock() + fake.writeServerConfigMutex.RLock() + defer fake.writeServerConfigMutex.RUnlock() + copiedInvocations := map[string][][]interface{}{} + for key, value := range fake.invocations { + copiedInvocations[key] = value + } + return copiedInvocations +} + +func (fake *FakeManager) recordInvocation(key string, args []interface{}) { + fake.invocationsMutex.Lock() + defer fake.invocationsMutex.Unlock() + if fake.invocations == nil { + fake.invocations = map[string][][]interface{}{} + } + if fake.invocations[key] == nil { + fake.invocations[key] = [][]interface{}{} + } + fake.invocations[key] = append(fake.invocations[key], args) +} + +var _ file.Manager = new(FakeManager) diff --git a/internal/nginx/file/manager.go b/internal/nginx/file/manager.go new file mode 100644 index 0000000000..e1f6a05b52 --- /dev/null +++ b/internal/nginx/file/manager.go @@ -0,0 +1,63 @@ +package file + +import ( + "fmt" + "os" + "path/filepath" +) + +const confdFolder = "/etc/nginx/conf.d" + +//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 . Manager + +// Manager manages NGINX configuration files. +type Manager interface { + // WriteServerConfig writes the server config on the file system. + // The name distinguishes this server config among all other server configs. For that, it must be unique. + // Note that name is not the name of the corresponding configuration file. + WriteServerConfig(name string, cfg []byte) error + // DeleteServerConfig deletes the corresponding configuration file for the server from the file system. + DeleteServerConfig(name string) error +} + +// ManagerImpl is an implementation of Manager. +type ManagerImpl struct { +} + +// NewManagerImpl creates a new NewManagerImpl. +func NewManagerImpl() *ManagerImpl { + return &ManagerImpl{} +} + +func (m *ManagerImpl) WriteServerConfig(name string, cfg []byte) error { + path := getPathForServerConfig(name) + + file, err := os.Create(path) + if err != nil { + return fmt.Errorf("failed to create server config %s: %w", path, err) + } + + defer file.Close() + + _, err = file.Write(cfg) + if err != nil { + return fmt.Errorf("failed to write server config %s: %w", path, err) + } + + return nil +} + +func (m *ManagerImpl) DeleteServerConfig(name string) error { + path := getPathForServerConfig(name) + + err := os.Remove(path) + if err != nil { + return fmt.Errorf("failed to remove server config %s: %w", path, err) + } + + return nil +} + +func getPathForServerConfig(name string) string { + return filepath.Join(confdFolder, name+".conf") +} diff --git a/internal/nginx/file/manager_test.go b/internal/nginx/file/manager_test.go new file mode 100644 index 0000000000..add900b3cf --- /dev/null +++ b/internal/nginx/file/manager_test.go @@ -0,0 +1,12 @@ +package file + +import "testing" + +func TestGetPathForServerConfig(t *testing.T) { + expected := "/etc/nginx/conf.d/test.example.com.conf" + + result := getPathForServerConfig("test.example.com") + if result != expected { + t.Errorf("getPathForServerConfig() returned %q but expected %q", result, expected) + } +} diff --git a/internal/nginx/runtime/manager.go b/internal/nginx/runtime/manager.go new file mode 100644 index 0000000000..ebf253f7e8 --- /dev/null +++ b/internal/nginx/runtime/manager.go @@ -0,0 +1,93 @@ +package runtime + +import ( + "bytes" + "context" + "errors" + "fmt" + "os" + "strconv" + "syscall" + "time" +) + +//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 . Manager + +// Manager manages the runtime of NGINX. +type Manager interface { + // Reload reloads NGINX configuration. It is a blocking operation. + Reload(ctx context.Context) error +} + +// ManagerImpl implements Manager. +type ManagerImpl struct { +} + +// NewManagerImpl creates a new ManagerImpl. +func NewManagerImpl() Manager { + return &ManagerImpl{} +} + +func (m *ManagerImpl) Reload(ctx context.Context) error { + // FIXME(pleshakov): Before reload attempt, make sure NGINX is running. + // If the gateway container starts before NGINX container (which is possible), then it is possible that a reload can be attempted + // when NGINX is not running yet. Make sure to prevent this case, so we don't get an error. + + // We find the NGINX master PID on every reload because it will change if the NGINX container is restarted. + pid, err := findMasterProcess(readProcDir, os.ReadFile) + if err != nil { + return fmt.Errorf("failed to find NGINX master process: %w", err) + } + + // send HUP signal to the NGINX master process reload configuration + // See https://nginx.org/en/docs/control.html + err = syscall.Kill(pid, syscall.SIGHUP) + if err != nil { + return fmt.Errorf("failed to send the HUP signal to NGINX master: %w", err) + } + + // FIXME(pleshakov) + // (1) ensure the reload actually happens. + // (2) ensure that in case of an error, the error message can be seen by the admins. + + // for now, to prevent a subsequent reload starting before the in-flight reload finishes, we simply sleep. + // Fixing (1) will make the sleep unnecessary. + + select { + case <-ctx.Done(): + return nil + case <-time.After(1 * time.Second): + } + + return nil +} + +func readProcDir() ([]os.DirEntry, error) { + return os.ReadDir("/proc") +} + +func findMasterProcess(readProcDir func() ([]os.DirEntry, error), readFile func(string) ([]byte, error)) (int, error) { + procFolders, err := readProcDir() + if err != nil { + return 0, fmt.Errorf("unable to read process directory: %w", err) + } + + for _, folder := range procFolders { + pid, err := strconv.Atoi(folder.Name()) + if err != nil { + continue + } + + cmdlineFile := fmt.Sprintf("/proc/%v/cmdline", folder.Name()) + content, err := readFile(cmdlineFile) + if err != nil { + return 0, fmt.Errorf("unable to read file %s: %w", cmdlineFile, err) + } + + if bytes.HasPrefix(content, []byte("nginx: master process")) { + return pid, nil + } + } + + return 0, errors.New("NGINX master is not running") +} diff --git a/internal/nginx/runtime/manager_test.go b/internal/nginx/runtime/manager_test.go new file mode 100644 index 0000000000..64e6a9e084 --- /dev/null +++ b/internal/nginx/runtime/manager_test.go @@ -0,0 +1,98 @@ +package runtime + +import ( + "errors" + "io/fs" + "os" + "testing" +) + +type fakeEntry struct { + name string +} + +func (e *fakeEntry) Name() string { + return e.name +} +func (*fakeEntry) IsDir() bool { return false } +func (*fakeEntry) Type() fs.FileMode { return 0 } +func (*fakeEntry) Info() (fs.FileInfo, error) { return nil, nil } + +func TestFindMasterProcess(t *testing.T) { + readProcDir := func() ([]os.DirEntry, error) { + return []os.DirEntry{ + &fakeEntry{name: "x"}, + &fakeEntry{name: "1"}, + &fakeEntry{name: "2"}, + }, nil + } + readProcDirError := func() ([]os.DirEntry, error) { + return nil, errors.New("error") + } + + readFile := func(name string) ([]byte, error) { + results := map[string][]byte{ + "/proc/x/cmdline": []byte("test"), + "/proc/1/cmdline": []byte("nginx: worker process"), + "/proc/2/cmdline": []byte("nginx: master process /usr/sbin/nginx -c /etc/nginx/nginx.conf"), + } + + res, exits := results[name] + if !exits { + return nil, errors.New("error") + } + + return res, nil + } + readFileError := func(string) ([]byte, error) { + return nil, errors.New("error") + } + + tests := []struct { + readProcDirFunc func() ([]os.DirEntry, error) + readFileFunc func(string) ([]byte, error) + expected int + expectError bool + msg string + }{ + { + readProcDirFunc: readProcDir, + readFileFunc: readFile, + expected: 2, + expectError: false, + msg: "normal case", + }, + { + readProcDirFunc: readProcDirError, + readFileFunc: readFile, + expected: 0, + expectError: true, + msg: "failed to read proc dir", + }, + { + readProcDirFunc: readProcDir, + readFileFunc: readFileError, + expected: 0, + expectError: true, + msg: "failed to read file", + }, + } + + for _, test := range tests { + result, err := findMasterProcess(test.readProcDirFunc, test.readFileFunc) + + if result != test.expected { + t.Errorf("findMasterProcess() returned %d but expected %d for case %q", result, test.expected, test.msg) + } + + if test.expectError { + if err == nil { + t.Errorf("findMasterProcess() didn't return error for case %q", test.msg) + } + } else { + if err != nil { + t.Errorf("findMasterProcess() returned unexpected error %v for case %q", err, test.msg) + } + } + } +} diff --git a/internal/nginx/runtime/runtimefakes/fake_manager.go b/internal/nginx/runtime/runtimefakes/fake_manager.go new file mode 100644 index 0000000000..8afa4f2517 --- /dev/null +++ b/internal/nginx/runtime/runtimefakes/fake_manager.go @@ -0,0 +1,112 @@ +// Code generated by counterfeiter. DO NOT EDIT. +package runtimefakes + +import ( + "context" + "sync" + + "github.com/nginxinc/nginx-gateway-kubernetes/internal/nginx/runtime" +) + +type FakeManager struct { + ReloadStub func(context.Context) error + reloadMutex sync.RWMutex + reloadArgsForCall []struct { + arg1 context.Context + } + reloadReturns struct { + result1 error + } + reloadReturnsOnCall map[int]struct { + result1 error + } + invocations map[string][][]interface{} + invocationsMutex sync.RWMutex +} + +func (fake *FakeManager) Reload(arg1 context.Context) error { + fake.reloadMutex.Lock() + ret, specificReturn := fake.reloadReturnsOnCall[len(fake.reloadArgsForCall)] + fake.reloadArgsForCall = append(fake.reloadArgsForCall, struct { + arg1 context.Context + }{arg1}) + stub := fake.ReloadStub + fakeReturns := fake.reloadReturns + fake.recordInvocation("Reload", []interface{}{arg1}) + fake.reloadMutex.Unlock() + if stub != nil { + return stub(arg1) + } + if specificReturn { + return ret.result1 + } + return fakeReturns.result1 +} + +func (fake *FakeManager) ReloadCallCount() int { + fake.reloadMutex.RLock() + defer fake.reloadMutex.RUnlock() + return len(fake.reloadArgsForCall) +} + +func (fake *FakeManager) ReloadCalls(stub func(context.Context) error) { + fake.reloadMutex.Lock() + defer fake.reloadMutex.Unlock() + fake.ReloadStub = stub +} + +func (fake *FakeManager) ReloadArgsForCall(i int) context.Context { + fake.reloadMutex.RLock() + defer fake.reloadMutex.RUnlock() + argsForCall := fake.reloadArgsForCall[i] + return argsForCall.arg1 +} + +func (fake *FakeManager) ReloadReturns(result1 error) { + fake.reloadMutex.Lock() + defer fake.reloadMutex.Unlock() + fake.ReloadStub = nil + fake.reloadReturns = struct { + result1 error + }{result1} +} + +func (fake *FakeManager) ReloadReturnsOnCall(i int, result1 error) { + fake.reloadMutex.Lock() + defer fake.reloadMutex.Unlock() + fake.ReloadStub = nil + if fake.reloadReturnsOnCall == nil { + fake.reloadReturnsOnCall = make(map[int]struct { + result1 error + }) + } + fake.reloadReturnsOnCall[i] = struct { + result1 error + }{result1} +} + +func (fake *FakeManager) Invocations() map[string][][]interface{} { + fake.invocationsMutex.RLock() + defer fake.invocationsMutex.RUnlock() + fake.reloadMutex.RLock() + defer fake.reloadMutex.RUnlock() + copiedInvocations := map[string][][]interface{}{} + for key, value := range fake.invocations { + copiedInvocations[key] = value + } + return copiedInvocations +} + +func (fake *FakeManager) recordInvocation(key string, args []interface{}) { + fake.invocationsMutex.Lock() + defer fake.invocationsMutex.Unlock() + if fake.invocations == nil { + fake.invocations = map[string][][]interface{}{} + } + if fake.invocations[key] == nil { + fake.invocations[key] = [][]interface{}{} + } + fake.invocations[key] = append(fake.invocations[key], args) +} + +var _ runtime.Manager = new(FakeManager) From cc1100a7c51dffba95f54284f7254d60aba43a3a Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Wed, 30 Mar 2022 13:36:47 -0700 Subject: [PATCH 2/6] Add a test for reloading once for multiple changes --- internal/events/loop_test.go | 52 ++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/internal/events/loop_test.go b/internal/events/loop_test.go index 05186ece50..f272c406f1 100644 --- a/internal/events/loop_test.go +++ b/internal/events/loop_test.go @@ -202,6 +202,58 @@ var _ = Describe("EventLoop", func() { }) }) + Describe("Processing events common cases", func() { + AfterEach(func() { + cancel() + + var err error + Eventually(errorCh).Should(Receive(&err)) + Expect(err).To(BeNil()) + }) + + It("should reload once in case of multiple changes", func() { + fakeChanges := []state.Change{ + { + Op: state.Delete, + Host: state.Host{ + Value: "one.example.com", + }, + }, + { + Op: state.Upsert, + Host: state.Host{ + Value: "two.example.com", + }, + }, + } + fakeConf.DeleteHTTPRouteReturns(fakeChanges, nil) + + fakeCfg := []byte("fake") + fakeGenerator.GenerateForHostReturns(fakeCfg, config.Warnings{}) + + nsname := types.NamespacedName{Namespace: "test", Name: "route"} + + // the exact event doesn't matter. what matters is the generated changes return by DeleteHTTPRouteReturns + eventCh <- &events.DeleteEvent{ + NamespacedName: nsname, + Type: &v1alpha2.HTTPRoute{}, + } + + Eventually(fakeConf.DeleteHTTPRouteCallCount).Should(Equal(1)) + Expect(fakeConf.DeleteHTTPRouteArgsForCall(0)).Should(Equal(nsname)) + + Eventually(fakeNginxFimeMgr.WriteServerConfigCallCount).Should(Equal(1)) + host, cfg := fakeNginxFimeMgr.WriteServerConfigArgsForCall(0) + Expect(host).Should(Equal("two.example.com")) + Expect(cfg).Should(Equal(fakeCfg)) + + Eventually(fakeNginxFimeMgr.DeleteServerConfigCallCount).Should(Equal(1)) + Expect(fakeNginxFimeMgr.DeleteServerConfigArgsForCall(0)).Should(Equal("one.example.com")) + + Eventually(fakeNginxRuntimeMgr.ReloadCallCount).Should(Equal(1)) + }) + }) + Describe("Edge cases", func() { AfterEach(func() { cancel() From cffee5fd2a8dc3bdb47e4a3a9b2349f449702bf7 Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Wed, 30 Mar 2022 13:38:23 -0700 Subject: [PATCH 3/6] empty struct declarations should be one line type ManagerImpl struct{} --- internal/nginx/file/manager.go | 3 +-- internal/nginx/runtime/manager.go | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/internal/nginx/file/manager.go b/internal/nginx/file/manager.go index e1f6a05b52..947cde4ede 100644 --- a/internal/nginx/file/manager.go +++ b/internal/nginx/file/manager.go @@ -21,8 +21,7 @@ type Manager interface { } // ManagerImpl is an implementation of Manager. -type ManagerImpl struct { -} +type ManagerImpl struct{} // NewManagerImpl creates a new NewManagerImpl. func NewManagerImpl() *ManagerImpl { diff --git a/internal/nginx/runtime/manager.go b/internal/nginx/runtime/manager.go index ebf253f7e8..1352db130b 100644 --- a/internal/nginx/runtime/manager.go +++ b/internal/nginx/runtime/manager.go @@ -20,8 +20,7 @@ type Manager interface { } // ManagerImpl implements Manager. -type ManagerImpl struct { -} +type ManagerImpl struct{} // NewManagerImpl creates a new ManagerImpl. func NewManagerImpl() Manager { From bd0f4f03bf39f89f238e79f20717a2598737b72f Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Wed, 30 Mar 2022 13:44:18 -0700 Subject: [PATCH 4/6] Use main instead of master when referring to NGINX master process --- deploy/manifests/nginx-gateway.yaml | 2 +- internal/nginx/runtime/manager.go | 14 +++++++------- internal/nginx/runtime/manager_test.go | 10 +++++----- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/deploy/manifests/nginx-gateway.yaml b/deploy/manifests/nginx-gateway.yaml index 67ed2c4f18..c924b85b94 100644 --- a/deploy/manifests/nginx-gateway.yaml +++ b/deploy/manifests/nginx-gateway.yaml @@ -95,7 +95,7 @@ spec: runAsUser: 1001 # FIXME(pleshakov) - figure out which capabilities are required # dropping ALL and adding only CAP_KILL doesn't work - # Note: CAP_KILL is needed for sending HUP signal to NGINX master + # Note: CAP_KILL is needed for sending HUP signal to NGINX main process args: - --gateway-ctlr-name=k8s-gateway.nginx.org/nginx-gateway/gateway - image: nginx:1.21.3 diff --git a/internal/nginx/runtime/manager.go b/internal/nginx/runtime/manager.go index 1352db130b..e878228983 100644 --- a/internal/nginx/runtime/manager.go +++ b/internal/nginx/runtime/manager.go @@ -32,17 +32,17 @@ func (m *ManagerImpl) Reload(ctx context.Context) error { // If the gateway container starts before NGINX container (which is possible), then it is possible that a reload can be attempted // when NGINX is not running yet. Make sure to prevent this case, so we don't get an error. - // We find the NGINX master PID on every reload because it will change if the NGINX container is restarted. - pid, err := findMasterProcess(readProcDir, os.ReadFile) + // We find the main NGINX PID on every reload because it will change if the NGINX container is restarted. + pid, err := findMainProcess(readProcDir, os.ReadFile) if err != nil { - return fmt.Errorf("failed to find NGINX master process: %w", err) + return fmt.Errorf("failed to find NGINX main process: %w", err) } - // send HUP signal to the NGINX master process reload configuration + // send HUP signal to the NGINX main process reload configuration // See https://nginx.org/en/docs/control.html err = syscall.Kill(pid, syscall.SIGHUP) if err != nil { - return fmt.Errorf("failed to send the HUP signal to NGINX master: %w", err) + return fmt.Errorf("failed to send the HUP signal to NGINX main: %w", err) } // FIXME(pleshakov) @@ -65,7 +65,7 @@ func readProcDir() ([]os.DirEntry, error) { return os.ReadDir("/proc") } -func findMasterProcess(readProcDir func() ([]os.DirEntry, error), readFile func(string) ([]byte, error)) (int, error) { +func findMainProcess(readProcDir func() ([]os.DirEntry, error), readFile func(string) ([]byte, error)) (int, error) { procFolders, err := readProcDir() if err != nil { return 0, fmt.Errorf("unable to read process directory: %w", err) @@ -88,5 +88,5 @@ func findMasterProcess(readProcDir func() ([]os.DirEntry, error), readFile func( } } - return 0, errors.New("NGINX master is not running") + return 0, errors.New("NGINX main process is not running") } diff --git a/internal/nginx/runtime/manager_test.go b/internal/nginx/runtime/manager_test.go index 64e6a9e084..8618165cfb 100644 --- a/internal/nginx/runtime/manager_test.go +++ b/internal/nginx/runtime/manager_test.go @@ -18,7 +18,7 @@ func (*fakeEntry) IsDir() bool { return false } func (*fakeEntry) Type() fs.FileMode { return 0 } func (*fakeEntry) Info() (fs.FileInfo, error) { return nil, nil } -func TestFindMasterProcess(t *testing.T) { +func TestFindMainProcess(t *testing.T) { readProcDir := func() ([]os.DirEntry, error) { return []os.DirEntry{ &fakeEntry{name: "x"}, @@ -79,19 +79,19 @@ func TestFindMasterProcess(t *testing.T) { } for _, test := range tests { - result, err := findMasterProcess(test.readProcDirFunc, test.readFileFunc) + result, err := findMainProcess(test.readProcDirFunc, test.readFileFunc) if result != test.expected { - t.Errorf("findMasterProcess() returned %d but expected %d for case %q", result, test.expected, test.msg) + t.Errorf("findMainProcess() returned %d but expected %d for case %q", result, test.expected, test.msg) } if test.expectError { if err == nil { - t.Errorf("findMasterProcess() didn't return error for case %q", test.msg) + t.Errorf("findMainProcess() didn't return error for case %q", test.msg) } } else { if err != nil { - t.Errorf("findMasterProcess() returned unexpected error %v for case %q", err, test.msg) + t.Errorf("findMainProcess() returned unexpected error %v for case %q", err, test.msg) } } } From ef405b1d1ab791d7fd31edbd9f589fc70c3826bc Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Wed, 30 Mar 2022 13:45:59 -0700 Subject: [PATCH 5/6] Return concrete type instead of interface --- internal/nginx/runtime/manager.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/nginx/runtime/manager.go b/internal/nginx/runtime/manager.go index e878228983..c834d44e92 100644 --- a/internal/nginx/runtime/manager.go +++ b/internal/nginx/runtime/manager.go @@ -23,7 +23,7 @@ type Manager interface { type ManagerImpl struct{} // NewManagerImpl creates a new ManagerImpl. -func NewManagerImpl() Manager { +func NewManagerImpl() *ManagerImpl { return &ManagerImpl{} } From 6c884cff220a6895fbad620880b52e92337d1470 Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Wed, 30 Mar 2022 15:09:43 -0700 Subject: [PATCH 6/6] Find main NGINX process pid using the pid file --- deploy/manifests/nginx-gateway.yaml | 2 +- internal/nginx/runtime/manager.go | 39 ++++-------- internal/nginx/runtime/manager_test.go | 86 +++++++++----------------- 3 files changed, 43 insertions(+), 84 deletions(-) diff --git a/deploy/manifests/nginx-gateway.yaml b/deploy/manifests/nginx-gateway.yaml index c924b85b94..d1c435a547 100644 --- a/deploy/manifests/nginx-gateway.yaml +++ b/deploy/manifests/nginx-gateway.yaml @@ -80,7 +80,7 @@ spec: initContainers: - image: busybox:1.34 # FIXME(pleshakov): use gateway container to init the Config with proper main config name: nginx-config-initializer - command: [ 'sh', '-c', 'echo "events {} http { include /etc/nginx/conf.d/*.conf; server { default_type text/html; return 404; } }" > /etc/nginx/nginx.conf && mkdir /etc/nginx/conf.d && chown 1001:0 /etc/nginx/conf.d' ] + command: [ 'sh', '-c', 'echo "events {} pid /etc/nginx/nginx.pid; http { include /etc/nginx/conf.d/*.conf; server { default_type text/html; return 404; } }" > /etc/nginx/nginx.conf && mkdir /etc/nginx/conf.d && chown 1001:0 /etc/nginx/conf.d' ] volumeMounts: - name: nginx-config mountPath: /etc/nginx diff --git a/internal/nginx/runtime/manager.go b/internal/nginx/runtime/manager.go index c834d44e92..8608bbad4d 100644 --- a/internal/nginx/runtime/manager.go +++ b/internal/nginx/runtime/manager.go @@ -1,16 +1,19 @@ package runtime import ( - "bytes" "context" - "errors" "fmt" "os" "strconv" + "strings" "syscall" "time" ) +const pidFile = "/etc/nginx/nginx.pid" + +type readFileFunc func(string) ([]byte, error) + //go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 . Manager // Manager manages the runtime of NGINX. @@ -33,7 +36,7 @@ func (m *ManagerImpl) Reload(ctx context.Context) error { // when NGINX is not running yet. Make sure to prevent this case, so we don't get an error. // We find the main NGINX PID on every reload because it will change if the NGINX container is restarted. - pid, err := findMainProcess(readProcDir, os.ReadFile) + pid, err := findMainProcess(os.ReadFile) if err != nil { return fmt.Errorf("failed to find NGINX main process: %w", err) } @@ -61,32 +64,16 @@ func (m *ManagerImpl) Reload(ctx context.Context) error { return nil } -func readProcDir() ([]os.DirEntry, error) { - return os.ReadDir("/proc") -} - -func findMainProcess(readProcDir func() ([]os.DirEntry, error), readFile func(string) ([]byte, error)) (int, error) { - procFolders, err := readProcDir() +func findMainProcess(readFile readFileFunc) (int, error) { + content, err := readFile(pidFile) if err != nil { - return 0, fmt.Errorf("unable to read process directory: %w", err) + return 0, err } - for _, folder := range procFolders { - pid, err := strconv.Atoi(folder.Name()) - if err != nil { - continue - } - - cmdlineFile := fmt.Sprintf("/proc/%v/cmdline", folder.Name()) - content, err := readFile(cmdlineFile) - if err != nil { - return 0, fmt.Errorf("unable to read file %s: %w", cmdlineFile, err) - } - - if bytes.HasPrefix(content, []byte("nginx: master process")) { - return pid, nil - } + pid, err := strconv.Atoi(strings.TrimSpace(string(content))) + if err != nil { + return 0, fmt.Errorf("invalid pid file content %q: %w", content, err) } - return 0, errors.New("NGINX main process is not running") + return pid, nil } diff --git a/internal/nginx/runtime/manager_test.go b/internal/nginx/runtime/manager_test.go index 8618165cfb..21ac7d5a87 100644 --- a/internal/nginx/runtime/manager_test.go +++ b/internal/nginx/runtime/manager_test.go @@ -2,84 +2,56 @@ package runtime import ( "errors" - "io/fs" - "os" "testing" ) -type fakeEntry struct { - name string -} - -func (e *fakeEntry) Name() string { - return e.name -} -func (*fakeEntry) IsDir() bool { return false } -func (*fakeEntry) Type() fs.FileMode { return 0 } -func (*fakeEntry) Info() (fs.FileInfo, error) { return nil, nil } - func TestFindMainProcess(t *testing.T) { - readProcDir := func() ([]os.DirEntry, error) { - return []os.DirEntry{ - &fakeEntry{name: "x"}, - &fakeEntry{name: "1"}, - &fakeEntry{name: "2"}, - }, nil - } - readProcDirError := func() ([]os.DirEntry, error) { - return nil, errors.New("error") - } - - readFile := func(name string) ([]byte, error) { - results := map[string][]byte{ - "/proc/x/cmdline": []byte("test"), - "/proc/1/cmdline": []byte("nginx: worker process"), - "/proc/2/cmdline": []byte("nginx: master process /usr/sbin/nginx -c /etc/nginx/nginx.conf"), - } - - res, exits := results[name] - if !exits { - return nil, errors.New("error") + readFileFuncGen := func(content []byte) readFileFunc { + return func(name string) ([]byte, error) { + if name != pidFile { + return nil, errors.New("error") + } + return content, nil } - - return res, nil } readFileError := func(string) ([]byte, error) { return nil, errors.New("error") } tests := []struct { - readProcDirFunc func() ([]os.DirEntry, error) - readFileFunc func(string) ([]byte, error) - expected int - expectError bool - msg string + readFile readFileFunc + expected int + expectError bool + msg string }{ { - readProcDirFunc: readProcDir, - readFileFunc: readFile, - expected: 2, - expectError: false, - msg: "normal case", + readFile: readFileFuncGen([]byte("1\n")), + expected: 1, + expectError: false, + msg: "normal case", + }, + { + readFile: readFileFuncGen([]byte("")), + expected: 0, + expectError: true, + msg: "empty file content", }, { - readProcDirFunc: readProcDirError, - readFileFunc: readFile, - expected: 0, - expectError: true, - msg: "failed to read proc dir", + readFile: readFileFuncGen([]byte("not a number")), + expected: 0, + expectError: true, + msg: "bad file content", }, { - readProcDirFunc: readProcDir, - readFileFunc: readFileError, - expected: 0, - expectError: true, - msg: "failed to read file", + readFile: readFileError, + expected: 0, + expectError: true, + msg: "cannot read file", }, } for _, test := range tests { - result, err := findMainProcess(test.readProcDirFunc, test.readFileFunc) + result, err := findMainProcess(test.readFile) if result != test.expected { t.Errorf("findMainProcess() returned %d but expected %d for case %q", result, test.expected, test.msg)