diff --git a/api/grpc/mpi/v1/command.pb.go b/api/grpc/mpi/v1/command.pb.go index 7c137c5e9..1c4fa9b0c 100644 --- a/api/grpc/mpi/v1/command.pb.go +++ b/api/grpc/mpi/v1/command.pb.go @@ -8,7 +8,7 @@ // Code generated by protoc-gen-go. DO NOT EDIT. // versions: -// protoc-gen-go v1.36.6 +// protoc-gen-go v1.36.7 // protoc (unknown) // source: mpi/v1/command.proto diff --git a/api/grpc/mpi/v1/common.pb.go b/api/grpc/mpi/v1/common.pb.go index 9ba42f536..26b95bf14 100644 --- a/api/grpc/mpi/v1/common.pb.go +++ b/api/grpc/mpi/v1/common.pb.go @@ -5,7 +5,7 @@ // Code generated by protoc-gen-go. DO NOT EDIT. // versions: -// protoc-gen-go v1.36.6 +// protoc-gen-go v1.36.7 // protoc (unknown) // source: mpi/v1/common.proto diff --git a/api/grpc/mpi/v1/files.pb.go b/api/grpc/mpi/v1/files.pb.go index 9ebb60f91..c68585426 100644 --- a/api/grpc/mpi/v1/files.pb.go +++ b/api/grpc/mpi/v1/files.pb.go @@ -5,7 +5,7 @@ // Code generated by protoc-gen-go. DO NOT EDIT. // versions: -// protoc-gen-go v1.36.6 +// protoc-gen-go v1.36.7 // protoc (unknown) // source: mpi/v1/files.proto diff --git a/internal/config/config.go b/internal/config/config.go index bc617c21c..dfd8d1e8f 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -379,28 +379,6 @@ func registerFlags() { DefManifestDir, "Specifies the path to the directory containing the manifest files", ) - fs.Duration( - NginxReloadMonitoringPeriodKey, - DefNginxReloadMonitoringPeriod, - "The amount of time to monitor NGINX after a reload of configuration.", - ) - fs.Bool( - NginxTreatWarningsAsErrorsKey, - DefTreatErrorsAsWarnings, - "Warning messages in the NGINX errors logs after a NGINX reload will be treated as an error.", - ) - - fs.String( - NginxApiTlsCa, - DefNginxApiTlsCa, - "The NGINX Plus CA certificate file location needed to call the NGINX Plus API if SSL is enabled.", - ) - - fs.StringSlice( - NginxExcludeLogsKey, []string{}, - "A comma-separated list of one or more NGINX log paths that you want to exclude from metrics "+ - "collection or error monitoring. This includes absolute paths or regex patterns", - ) fs.StringSlice(AllowedDirectoriesKey, DefaultAllowedDirectories(), @@ -442,6 +420,7 @@ func registerFlags() { registerAuxiliaryCommandFlags(fs) registerCollectorFlags(fs) registerClientFlags(fs) + registerDataPlaneFlags(fs) fs.SetNormalizeFunc(normalizeFunc) @@ -456,6 +435,57 @@ func registerFlags() { }) } +func registerDataPlaneFlags(fs *flag.FlagSet) { + fs.Duration( + NginxReloadMonitoringPeriodKey, + DefNginxReloadMonitoringPeriod, + "The amount of time to monitor NGINX after a reload of configuration.", + ) + fs.Bool( + NginxTreatWarningsAsErrorsKey, + DefTreatErrorsAsWarnings, + "Warning messages in the NGINX errors logs after a NGINX reload will be treated as an error.", + ) + + fs.String( + NginxApiTlsCa, + DefNginxApiTlsCa, + "The NGINX Plus CA certificate file location needed to call the NGINX Plus API if SSL is enabled.", + ) + + fs.StringSlice( + NginxExcludeLogsKey, []string{}, + "A comma-separated list of one or more NGINX log paths that you want to exclude from metrics "+ + "collection or error monitoring. This includes absolute paths or regex patterns", + ) + + // NGINX Reload Backoff Flags + fs.Duration( + NginxReloadBackoffInitialIntervalKey, + DefNginxReloadBackoffInitialInterval, + "The NGINX reload backoff initial interval, value in seconds") + + fs.Duration( + NginxReloadBackoffMaxIntervalKey, + DefNginxReloadBackoffMaxInterval, + "The NGINX reload backoff max interval, value in seconds") + + fs.Duration( + NginxReloadBackoffMaxElapsedTimeKey, + DefNginxReloadBackoffMaxElapsedTime, + "The NGINX reload backoff max elapsed time, value in seconds") + + fs.Float64( + NginxReloadBackoffRandomizationFactorKey, + DefNginxReloadBackoffRandomizationFactor, + "The NGINX reload backoff randomization factor, value float") + + fs.Float64( + NginxReloadBackoffMultiplierKey, + DefNginxReloadBackoffMultiplier, + "The NGINX reload backoff multiplier, value float") +} + func registerCommonFlags(fs *flag.FlagSet) { fs.StringToString( LabelsRootKey, @@ -906,6 +936,13 @@ func resolveDataPlaneConfig() *DataPlaneConfig { TreatWarningsAsErrors: viperInstance.GetBool(NginxTreatWarningsAsErrorsKey), ExcludeLogs: viperInstance.GetStringSlice(NginxExcludeLogsKey), APITls: TLSConfig{Ca: viperInstance.GetString(NginxApiTlsCa)}, + ReloadBackoff: &BackOff{ + InitialInterval: viperInstance.GetDuration(NginxReloadBackoffInitialIntervalKey), + MaxInterval: viperInstance.GetDuration(NginxReloadBackoffMaxIntervalKey), + MaxElapsedTime: viperInstance.GetDuration(NginxReloadBackoffMaxElapsedTimeKey), + RandomizationFactor: viperInstance.GetFloat64(NginxReloadBackoffRandomizationFactorKey), + Multiplier: viperInstance.GetFloat64(NginxReloadBackoffMultiplierKey), + }, }, } } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 60f1175d0..cce67b7ff 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1101,6 +1101,13 @@ func createConfig() *Config { ExcludeLogs: []string{"/var/log/nginx/error.log", "^/var/log/nginx/.*.log$"}, ReloadMonitoringPeriod: 30 * time.Second, TreatWarningsAsErrors: true, + ReloadBackoff: &BackOff{ + InitialInterval: 100 * time.Millisecond, + MaxInterval: 20 * time.Second, + MaxElapsedTime: 15 * time.Second, + RandomizationFactor: 1.5, + Multiplier: 1.5, + }, }, }, Collector: &Collector{ diff --git a/internal/config/defaults.go b/internal/config/defaults.go index 072052431..7a279d749 100644 --- a/internal/config/defaults.go +++ b/internal/config/defaults.go @@ -16,6 +16,13 @@ const ( DefTreatErrorsAsWarnings = false DefNginxApiTlsCa = "" + // Nginx Reload Backoff defaults + DefNginxReloadBackoffInitialInterval = 1 * time.Second + DefNginxReloadBackoffRandomizationFactor = 0.5 // the value is 0 <= and < 1 + DefNginxReloadBackoffMultiplier = 5 + DefNginxReloadBackoffMaxInterval = 10 * time.Second + DefNginxReloadBackoffMaxElapsedTime = 30 * time.Second + DefCommandServerHostKey = "" DefCommandServerPortKey = 0 DefCommandServerTypeKey = "grpc" diff --git a/internal/config/flags.go b/internal/config/flags.go index e6da27573..172879518 100644 --- a/internal/config/flags.go +++ b/internal/config/flags.go @@ -112,10 +112,16 @@ var ( LogLevelKey = pre(LogLevelRootKey) + "level" LogPathKey = pre(LogLevelRootKey) + "path" - NginxReloadMonitoringPeriodKey = pre(DataPlaneConfigRootKey, "nginx") + "reload_monitoring_period" - NginxTreatWarningsAsErrorsKey = pre(DataPlaneConfigRootKey, "nginx") + "treat_warnings_as_errors" - NginxExcludeLogsKey = pre(DataPlaneConfigRootKey, "nginx") + "exclude_logs" - NginxApiTlsCa = pre(DataPlaneConfigRootKey, "nginx") + "api_tls_ca" + NginxReloadMonitoringPeriodKey = pre(DataPlaneConfigRootKey, "nginx") + "reload_monitoring_period" + NginxTreatWarningsAsErrorsKey = pre(DataPlaneConfigRootKey, "nginx") + "treat_warnings_as_errors" + NginxReloadBackoffKey = pre(DataPlaneConfigRootKey, "nginx") + "reload_backoff" + NginxReloadBackoffInitialIntervalKey = pre(NginxReloadBackoffKey) + "initial_interval" + NginxReloadBackoffMaxIntervalKey = pre(NginxReloadBackoffKey) + "max_interval" + NginxReloadBackoffMaxElapsedTimeKey = pre(NginxReloadBackoffKey) + "max_elapsed_time" + NginxReloadBackoffRandomizationFactorKey = pre(NginxReloadBackoffKey) + "randomization_factor" + NginxReloadBackoffMultiplierKey = pre(NginxReloadBackoffKey) + "multiplier" + NginxExcludeLogsKey = pre(DataPlaneConfigRootKey, "nginx") + "exclude_logs" + NginxApiTlsCa = pre(DataPlaneConfigRootKey, "nginx") + "api_tls_ca" FileWatcherMonitoringFrequencyKey = pre(FileWatcherKey) + "monitoring_frequency" NginxExcludeFilesKey = pre(FileWatcherKey) + "exclude_files" diff --git a/internal/config/testdata/nginx-agent.conf b/internal/config/testdata/nginx-agent.conf index 1aeb11221..9df36198d 100644 --- a/internal/config/testdata/nginx-agent.conf +++ b/internal/config/testdata/nginx-agent.conf @@ -31,6 +31,12 @@ data_plane_config: exclude_logs: - /var/log/nginx/error.log - ^/var/log/nginx/.*.log$ + reload_backoff: + initial_interval: 100ms + max_interval: 20s + max_elapsed_time: 15s + randomization_factor: 1.5 + multiplier: 1.5 client: http: timeout: 15s diff --git a/internal/config/types.go b/internal/config/types.go index 059cd1515..4159c6a83 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -63,6 +63,7 @@ type ( } NginxDataPlaneConfig struct { + ReloadBackoff *BackOff `yaml:"reload_backoff" mapstructure:"reload_backoff"` APITls TLSConfig `yaml:"api_tls" mapstructure:"api_tls"` ExcludeLogs []string `yaml:"exclude_logs" mapstructure:"exclude_logs"` ReloadMonitoringPeriod time.Duration `yaml:"reload_monitoring_period" mapstructure:"reload_monitoring_period"` diff --git a/internal/datasource/nginx/process.go b/internal/datasource/nginx/process.go new file mode 100644 index 000000000..8a9f5b109 --- /dev/null +++ b/internal/datasource/nginx/process.go @@ -0,0 +1,158 @@ +// Copyright (c) F5, Inc. +// +// This source code is licensed under the Apache License, Version 2.0 license found in the +// LICENSE file in the root directory of this source tree. + +package nginx + +import ( + "bufio" + "bytes" + "context" + "fmt" + "regexp" + "strings" + + "github.com/nginx/agent/v3/internal/datasource/host/exec" + "github.com/nginx/agent/v3/internal/model" + "github.com/nginx/agent/v3/pkg/nginxprocess" +) + +const ( + keyValueLen = 2 + flagLen = 1 +) + +var versionRegex = regexp.MustCompile(`(?P\S+)\/(?P.*)`) + +func ProcessInfo(ctx context.Context, proc *nginxprocess.Process, + executer exec.ExecInterface, +) (*model.ProcessInfo, error) { + exePath := proc.Exe + + if exePath == "" { + exePath = Exe(ctx, executer) + if exePath == "" { + return nil, fmt.Errorf("unable to find NGINX exe for process %d", proc.PID) + } + } + + confPath := ConfPathFromCommand(proc.Cmd) + + var nginxInfo *model.ProcessInfo + + outputBuffer, err := executer.RunCmd(ctx, exePath, "-V") + if err != nil { + return nil, err + } + + nginxInfo = ParseNginxVersionCommandOutput(ctx, outputBuffer) + + nginxInfo.ExePath = exePath + nginxInfo.ProcessID = proc.PID + + if nginxInfo.ConfPath = model.NginxConfPath(ctx, nginxInfo); confPath != "" { + nginxInfo.ConfPath = confPath + } + + return nginxInfo, err +} + +func Exe(ctx context.Context, executer exec.ExecInterface) string { + exePath := "" + + out, commandErr := executer.RunCmd(ctx, "sh", "-c", "command -v nginx") + if commandErr == nil { + exePath = strings.TrimSuffix(out.String(), "\n") + } + + if exePath == "" { + exePath = defaultToNginxCommandForProcessPath(executer) + } + + if strings.Contains(exePath, "(deleted)") { + exePath = sanitizeExeDeletedPath(exePath) + } + + return exePath +} + +func defaultToNginxCommandForProcessPath(executer exec.ExecInterface) string { + exePath, err := executer.FindExecutable("nginx") + if err != nil { + return "" + } + + return exePath +} + +func sanitizeExeDeletedPath(exe string) string { + firstSpace := strings.Index(exe, "(deleted)") + if firstSpace != -1 { + return strings.TrimSpace(exe[0:firstSpace]) + } + + return strings.TrimSpace(exe) +} + +func ConfPathFromCommand(command string) string { + commands := strings.Split(command, " ") + + for i, command := range commands { + if command == "-c" { + if i < len(commands)-1 { + return commands[i+1] + } + } + } + + return "" +} + +func ParseNginxVersionCommandOutput(ctx context.Context, output *bytes.Buffer) *model.ProcessInfo { + nginxInfo := &model.ProcessInfo{} + + scanner := bufio.NewScanner(output) + for scanner.Scan() { + line := strings.TrimSpace(scanner.Text()) + switch { + case strings.HasPrefix(line, "nginx version"): + nginxInfo.Version = parseNginxVersion(line) + case strings.HasPrefix(line, "configure arguments"): + nginxInfo.ConfigureArgs = parseConfigureArguments(line) + } + } + + nginxInfo.Prefix = model.NginxPrefix(ctx, nginxInfo) + + return nginxInfo +} + +func parseNginxVersion(line string) string { + return strings.TrimPrefix(versionRegex.FindString(line), "nginx/") +} + +func parseConfigureArguments(line string) map[string]interface{} { + // need to check for empty strings + flags := strings.Split(line[len("configure arguments:"):], " --") + result := make(map[string]interface{}) + + for _, flag := range flags { + vals := strings.Split(flag, "=") + if isFlag(vals) { + result[vals[0]] = true + } else if isKeyValueFlag(vals) { + result[vals[0]] = vals[1] + } + } + + return result +} + +func isFlag(vals []string) bool { + return len(vals) == flagLen && vals[0] != "" +} + +func isKeyValueFlag(vals []string) bool { + return len(vals) == keyValueLen +} diff --git a/internal/datasource/nginx/process_test.go b/internal/datasource/nginx/process_test.go new file mode 100644 index 000000000..331a370ba --- /dev/null +++ b/internal/datasource/nginx/process_test.go @@ -0,0 +1,62 @@ +// Copyright (c) F5, Inc. +// +// This source code is licensed under the Apache License, Version 2.0 license found in the +// LICENSE file in the root directory of this source tree. + +package nginx + +import ( + "bytes" + "context" + "errors" + "testing" + + "github.com/nginx/agent/v3/internal/datasource/host/exec/execfakes" + "github.com/stretchr/testify/assert" +) + +func TestGetConfigPathFromCommand(t *testing.T) { + result := ConfPathFromCommand("nginx: master process nginx -c /tmp/nginx.conf") + assert.Equal(t, "/tmp/nginx.conf", result) + + result = ConfPathFromCommand("nginx: master process nginx -c") + assert.Empty(t, result) + + result = ConfPathFromCommand("") + assert.Empty(t, result) +} + +func TestNginxProcessParser_GetExe(t *testing.T) { + ctx := context.Background() + + tests := []struct { + commandError error + name string + expected string + commandOutput []byte + }{ + { + name: "Test 1: Default exe if error executing command -v nginx", + commandOutput: []byte{}, + commandError: errors.New("command error"), + expected: "/usr/bin/nginx", + }, + { + name: "Test 2: Sanitize Exe Deleted Path", + commandOutput: []byte("/usr/sbin/nginx (deleted)"), + commandError: nil, + expected: "/usr/sbin/nginx", + }, + } + for _, test := range tests { + t.Run(test.name, func(tt *testing.T) { + mockExec := &execfakes.FakeExecInterface{} + mockExec.RunCmdReturns(bytes.NewBuffer(test.commandOutput), test.commandError) + mockExec.FindExecutableReturns("/usr/bin/nginx", nil) + + result := Exe(ctx, mockExec) + + assert.Equal(tt, test.expected, result) + }) + } +} diff --git a/internal/model/process.go b/internal/model/process.go new file mode 100644 index 000000000..1e8e83b43 --- /dev/null +++ b/internal/model/process.go @@ -0,0 +1,55 @@ +// Copyright (c) F5, Inc. +// +// This source code is licensed under the Apache License, Version 2.0 license found in the +// LICENSE file in the root directory of this source tree. + +package model + +import ( + "context" + "log/slog" + "path" +) + +type ProcessInfo struct { + ConfigureArgs map[string]interface{} + Version string + Prefix string + ConfPath string + ExePath string + LoadableModules []string + DynamicModules []string + ProcessID int32 +} + +func NginxConfPath(ctx context.Context, nginxInfo *ProcessInfo) string { + var confPath string + + if nginxInfo.ConfigureArgs["conf-path"] != nil { + var ok bool + confPath, ok = nginxInfo.ConfigureArgs["conf-path"].(string) + if !ok { + slog.DebugContext(ctx, "failed to cast nginxInfo conf-path to string") + } + } else { + confPath = path.Join(nginxInfo.Prefix, "/conf/nginx.conf") + } + + return confPath +} + +func NginxPrefix(ctx context.Context, nginxInfo *ProcessInfo) string { + var prefix string + + if nginxInfo.ConfigureArgs["prefix"] != nil { + var ok bool + prefix, ok = nginxInfo.ConfigureArgs["prefix"].(string) + if !ok { + slog.DebugContext(ctx, "Failed to cast nginxInfo prefix to string") + } + } else { + prefix = "/usr/local/nginx" + } + + return prefix +} diff --git a/internal/resource/nginx_instance_operator.go b/internal/resource/nginx_instance_operator.go index fa515c081..e25d2e35d 100644 --- a/internal/resource/nginx_instance_operator.go +++ b/internal/resource/nginx_instance_operator.go @@ -11,6 +11,10 @@ import ( "errors" "fmt" "log/slog" + "time" + + "github.com/nginx/agent/v3/internal/backoff" + "github.com/nginx/agent/v3/pkg/nginxprocess" mpi "github.com/nginx/agent/v3/api/grpc/mpi/v1" "github.com/nginx/agent/v3/internal/config" @@ -18,8 +22,10 @@ import ( ) type NginxInstanceOperator struct { + agentConfig *config.Config executer exec.ExecInterface logTailer logTailerOperator + nginxProcessOperator processOperator treatWarningsAsErrors bool } @@ -29,7 +35,9 @@ func NewInstanceOperator(agentConfig *config.Config) *NginxInstanceOperator { return &NginxInstanceOperator{ executer: &exec.Exec{}, logTailer: NewLogTailerOperator(agentConfig), + nginxProcessOperator: NewNginxInstanceProcessOperator(), treatWarningsAsErrors: agentConfig.DataPlaneConfig.Nginx.TreatWarningsAsErrors, + agentConfig: agentConfig, } } @@ -53,11 +61,18 @@ func (i *NginxInstanceOperator) Validate(ctx context.Context, instance *mpi.Inst } func (i *NginxInstanceOperator) Reload(ctx context.Context, instance *mpi.Instance) error { + var reloadTime time.Time var errorsFound error + pid := instance.GetInstanceRuntime().GetProcessId() + slog.InfoContext(ctx, "Reloading NGINX PID", "pid", - instance.GetInstanceRuntime().GetProcessId()) + pid) - slog.InfoContext(ctx, "NGINX reloaded", "processid", instance.GetInstanceRuntime().GetProcessId()) + workers := i.nginxProcessOperator.NginxWorkerProcesses(ctx, pid) + + if len(workers) > 0 { + reloadTime = workers[0].Created + } errorLogs := i.errorLogs(instance) @@ -66,11 +81,21 @@ func (i *NginxInstanceOperator) Reload(ctx context.Context, instance *mpi.Instan go i.monitorLogs(ctx, errorLogs, logErrorChannel) - err := i.executer.KillProcess(instance.GetInstanceRuntime().GetProcessId()) + err := i.executer.KillProcess(pid) if err != nil { return err } + processes, procErr := i.nginxProcessOperator.FindNginxProcesses(ctx) + if procErr != nil { + slog.WarnContext(ctx, "Error finding parent process ID, unable to check if NGINX worker "+ + "processes have reloaded", "error", procErr) + } else { + i.checkWorkers(ctx, instance.GetInstanceMeta().GetInstanceId(), reloadTime, processes) + } + + slog.InfoContext(ctx, "NGINX reloaded", "process_id", pid) + numberOfExpectedMessages := len(errorLogs) for range numberOfExpectedMessages { @@ -91,6 +116,55 @@ func (i *NginxInstanceOperator) Reload(ctx context.Context, instance *mpi.Instan return nil } +func (i *NginxInstanceOperator) checkWorkers(ctx context.Context, instanceID string, reloadTime time.Time, + processes []*nginxprocess.Process, +) { + backoffSettings := &config.BackOff{ + InitialInterval: i.agentConfig.DataPlaneConfig.Nginx.ReloadBackoff.InitialInterval, + MaxInterval: i.agentConfig.DataPlaneConfig.Nginx.ReloadBackoff.MaxInterval, + MaxElapsedTime: i.agentConfig.DataPlaneConfig.Nginx.ReloadBackoff.MaxElapsedTime, + RandomizationFactor: i.agentConfig.DataPlaneConfig.Nginx.ReloadBackoff.RandomizationFactor, + Multiplier: i.agentConfig.DataPlaneConfig.Nginx.ReloadBackoff.Multiplier, + } + + slog.DebugContext(ctx, "Waiting for NGINX to finish reloading") + + newPid, findErr := i.nginxProcessOperator.FindParentProcessID(ctx, instanceID, processes, i.executer) + if findErr != nil { + slog.WarnContext(ctx, "Error finding parent process ID, unable to check if NGINX worker "+ + "processes have reloaded", "error", findErr) + + return + } + + slog.DebugContext(ctx, "Found parent process ID", "process_id", newPid) + + err := backoff.WaitUntil(ctx, backoffSettings, func() error { + slog.Info(" ============ Checking NGINX worker processes have reloaded") + currentWorkers := i.nginxProcessOperator.NginxWorkerProcesses(ctx, newPid) + if len(currentWorkers) == 0 { + return errors.New("waiting for NGINX worker processes") + } + + for _, worker := range currentWorkers { + if !worker.Created.After(reloadTime) { + return fmt.Errorf("waiting for all NGINX workers to be newer "+ + "than %v, found worker with time %v", reloadTime, worker.Created) + } + } + + return nil + }) + if err != nil { + slog.WarnContext(ctx, "Failed to check if NGINX worker processes have successfully reloaded, "+ + "timed out waiting", "error", err) + + return + } + + slog.InfoContext(ctx, "All NGINX workers have been reloaded") +} + func (i *NginxInstanceOperator) validateConfigCheckResponse(out []byte) error { if bytes.Contains(out, []byte("[emerg]")) || bytes.Contains(out, []byte("[alert]")) || diff --git a/internal/resource/nginx_instance_operator_test.go b/internal/resource/nginx_instance_operator_test.go index 186aa05bb..1f5a39f58 100644 --- a/internal/resource/nginx_instance_operator_test.go +++ b/internal/resource/nginx_instance_operator_test.go @@ -10,10 +10,19 @@ import ( "context" "errors" "fmt" + "slices" "sync" "testing" "time" + "github.com/nginx/agent/v3/internal/resource/resourcefakes" + + "github.com/nginx/agent/v3/test/stub" + + "github.com/nginx/agent/v3/pkg/nginxprocess" + + "github.com/nginx/agent/v3/internal/config" + "github.com/nginx/agent/v3/internal/datasource/host/exec/execfakes" "github.com/nginx/agent/v3/test/helpers" "github.com/nginx/agent/v3/test/protos" @@ -22,6 +31,31 @@ import ( "github.com/stretchr/testify/require" ) +const ( + exePath = "/usr/local/Cellar/nginx/1.25.3/bin/nginx" + exePath2 = "/usr/local/Cellar/nginx/1.26.3/bin/nginx" + ossConfigArgs = "--prefix=/usr/local/Cellar/nginx/1.25.3 --sbin-path=/usr/local/Cellar/nginx/1.25.3/bin/nginx " + + "--modules-path=%s --with-cc-opt='-I/usr/local/opt/pcre2/include -I/usr/local/opt/openssl@1.1/include' " + + "--with-ld-opt='-L/usr/local/opt/pcre2/lib -L/usr/local/opt/openssl@1.1/lib' " + + "--conf-path=/usr/local/etc/nginx/nginx.conf --pid-path=/usr/local/var/run/nginx.pid " + + "--lock-path=/usr/local/var/run/nginx.lock " + + "--http-client-body-temp-path=/usr/local/var/run/nginx/client_body_temp " + + "--http-proxy-temp-path=/usr/local/var/run/nginx/proxy_temp " + + "--http-fastcgi-temp-path=/usr/local/var/run/nginx/fastcgi_temp " + + "--http-uwsgi-temp-path=/usr/local/var/run/nginx/uwsgi_temp " + + "--http-scgi-temp-path=/usr/local/var/run/nginx/scgi_temp " + + "--http-log-path=/usr/local/var/log/nginx/access.log " + + "--error-log-path=/usr/local/var/log/nginx/error.log --with-compat --with-debug " + + "--with-http_addition_module --with-http_auth_request_module --with-http_dav_module " + + "--with-http_degradation_module --with-http_flv_module --with-http_gunzip_module " + + "--with-http_gzip_static_module --with-http_mp4_module --with-http_random_index_module " + + "--with-http_realip_module --with-http_secure_link_module --with-http_slice_module " + + "--with-http_ssl_module --with-http_stub_status_module --with-http_sub_module " + + "--with-http_v2_module --with-ipv6 --with-mail --with-mail_ssl_module --with-pcre " + + "--with-pcre-jit --with-stream --with-stream_realip_module --with-stream_ssl_module " + + "--with-stream_ssl_preread_module" +) + func TestInstanceOperator_ValidateConfigCheckResponse(t *testing.T) { tests := []struct { expected interface{} @@ -38,12 +72,17 @@ func TestInstanceOperator_ValidateConfigCheckResponse(t *testing.T) { out: "nginx [emerg]", expected: errors.New("error running nginx -t -c:\nnginx [emerg]"), }, + { + name: "Test 3: Warn response", + out: "nginx [warn]", + expected: errors.New("error running nginx -t -c:\nnginx [warn]"), + }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { operator := NewInstanceOperator(types.AgentConfig()) - + operator.treatWarningsAsErrors = true err := operator.validateConfigCheckResponse([]byte(test.out)) assert.Equal(t, test.expected, err) }) @@ -126,7 +165,15 @@ func TestInstanceOperator_Reload(t *testing.T) { instance := protos.NginxOssInstance([]string{}) - operator := NewInstanceOperator(types.AgentConfig()) + agentConfig := types.AgentConfig() + agentConfig.DataPlaneConfig.Nginx.ReloadBackoff = &config.BackOff{ + InitialInterval: config.DefNginxReloadBackoffInitialInterval, + MaxInterval: config.DefNginxReloadBackoffMaxInterval, + MaxElapsedTime: config.DefNginxReloadBackoffMaxElapsedTime, + RandomizationFactor: config.DefNginxReloadBackoffRandomizationFactor, + Multiplier: config.DefNginxReloadBackoffMultiplier, + } + operator := NewInstanceOperator(agentConfig) operator.executer = mockExec err := operator.Reload(ctx, instance) @@ -179,7 +226,14 @@ func TestInstanceOperator_ReloadAndMonitor(t *testing.T) { agentConfig := types.AgentConfig() agentConfig.DataPlaneConfig.Nginx.ReloadMonitoringPeriod = 10 * time.Second - operator := NewInstanceOperator(types.AgentConfig()) + agentConfig.DataPlaneConfig.Nginx.ReloadBackoff = &config.BackOff{ + InitialInterval: config.DefNginxReloadBackoffInitialInterval, + MaxInterval: config.DefNginxReloadBackoffMaxInterval, + MaxElapsedTime: config.DefNginxReloadBackoffMaxElapsedTime, + RandomizationFactor: config.DefNginxReloadBackoffRandomizationFactor, + Multiplier: config.DefNginxReloadBackoffMultiplier, + } + operator := NewInstanceOperator(agentConfig) operator.executer = mockExec var wg sync.WaitGroup @@ -201,3 +255,134 @@ func TestInstanceOperator_ReloadAndMonitor(t *testing.T) { }) } } + +func TestInstanceOperator_checkWorkers(t *testing.T) { + ctx := context.Background() + + modulePath := t.TempDir() + "/usr/lib/nginx/modules" + + configArgs := fmt.Sprintf(ossConfigArgs, modulePath) + nginxVersionCommandOutput := `nginx version: nginx/1.25.3 + built by clang 14.0.0 (clang-1400.0.29.202) + built with OpenSSL 1.1.1s 1 Nov 2022 (running with OpenSSL 1.1.1t 7 Feb 2023) + TLS SNI support enabled + configure arguments: ` + configArgs + + tests := []struct { + expectedLog string + name string + instanceID string + reloadTime time.Time + workers []*nginxprocess.Process + masterProcess []*nginxprocess.Process + }{ + { + name: "Test 1: Successful reload", + expectedLog: "All NGINX workers have been reloaded", + reloadTime: time.Date(2025, 8, 13, 8, 0, 0, 0, time.Local), + instanceID: "e1374cb1-462d-3b6c-9f3b-f28332b5f10c", + workers: []*nginxprocess.Process{ + { + PID: 567, + Created: time.Date(2025, 8, 13, 8, 1, 0, 0, time.Local), + PPID: 1234, + Name: "nginx", + Cmd: "nginx: worker process", + Exe: exePath, + }, + { + PID: 789, + PPID: 1234, + Created: time.Date(2025, 8, 13, 8, 1, 0, 0, time.Local), + Name: "nginx", + Cmd: "nginx: worker process", + Exe: exePath, + }, + }, + masterProcess: []*nginxprocess.Process{ + { + PID: 1234, + Created: time.Date(2025, 8, 13, 8, 1, 0, 0, time.Local), + PPID: 1, + Name: "nginx", + Cmd: "nginx: master process /usr/local/opt/nginx/bin/nginx -g daemon off;", + Exe: exePath, + }, + }, + }, + { + name: "Test 2: Unsuccessful reload", + expectedLog: "\"Failed to check if NGINX worker processes have successfully reloaded, timed out " + + "waiting\" error=\"waiting for NGINX worker processes\"", + reloadTime: time.Date(2025, 8, 13, 8, 0, 0, 0, time.Local), + instanceID: "e1374cb1-462d-3b6c-9f3b-f28332b5f10c", + masterProcess: []*nginxprocess.Process{ + { + PID: 1234, + Created: time.Date(2025, 8, 13, 8, 1, 0, 0, time.Local), + PPID: 1, + Name: "nginx", + Cmd: "nginx: master process /usr/local/opt/nginx/bin/nginx -g daemon off;", + Exe: exePath, + }, + }, + workers: []*nginxprocess.Process{ + { + PID: 567, + Created: time.Date(2025, 8, 13, 8, 1, 0, 0, time.Local), + PPID: 1234, + Name: "nginx", + Cmd: "nginx: worker process", + Exe: exePath, + }, + { + PID: 789, + PPID: 1234, + Created: time.Date(2025, 8, 13, 7, 1, 0, 0, time.Local), + Name: "nginx", + Cmd: "nginx: worker process", + Exe: exePath, + }, + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(tt *testing.T) { + mockExec := &execfakes.FakeExecInterface{} + mockExec.RunCmdReturnsOnCall(0, bytes.NewBufferString(nginxVersionCommandOutput), nil) + mockExec.RunCmdReturnsOnCall(1, bytes.NewBufferString(nginxVersionCommandOutput), nil) + mockExec.RunCmdReturnsOnCall(2, bytes.NewBufferString(nginxVersionCommandOutput), nil) + mockExec.RunCmdReturnsOnCall(3, bytes.NewBufferString(nginxVersionCommandOutput), nil) + + mockProcessOp := &resourcefakes.FakeProcessOperator{} + allProcesses := slices.Concat(test.workers, test.masterProcess) + mockProcessOp.FindNginxProcessesReturnsOnCall(0, allProcesses, nil) + mockProcessOp.NginxWorkerProcessesReturnsOnCall(0, test.workers) + mockProcessOp.FindParentProcessIDReturnsOnCall(0, test.masterProcess[0].PID, nil) + + logBuf := &bytes.Buffer{} + stub.StubLoggerWith(logBuf) + + agentConfig := types.AgentConfig() + agentConfig.DataPlaneConfig.Nginx.ReloadMonitoringPeriod = 10 * time.Second + agentConfig.DataPlaneConfig.Nginx.ReloadBackoff = &config.BackOff{ + InitialInterval: config.DefNginxReloadBackoffInitialInterval, + MaxInterval: config.DefNginxReloadBackoffMaxInterval, + MaxElapsedTime: config.DefNginxReloadBackoffMaxElapsedTime, + RandomizationFactor: config.DefNginxReloadBackoffRandomizationFactor, + Multiplier: config.DefNginxReloadBackoffMultiplier, + } + operator := NewInstanceOperator(agentConfig) + operator.executer = mockExec + operator.nginxProcessOperator = mockProcessOp + + operator.checkWorkers(ctx, test.instanceID, test.reloadTime, allProcesses) + + helpers.ValidateLog(t, test.expectedLog, logBuf) + + t.Logf("Logs: %s", logBuf.String()) + logBuf.Reset() + }) + } +} diff --git a/internal/resource/nginx_instance_process_operator.go b/internal/resource/nginx_instance_process_operator.go new file mode 100644 index 000000000..2bcb7816e --- /dev/null +++ b/internal/resource/nginx_instance_process_operator.go @@ -0,0 +1,84 @@ +// Copyright (c) F5, Inc. +// +// This source code is licensed under the Apache License, Version 2.0 license found in the +// LICENSE file in the root directory of this source tree. + +package resource + +import ( + "context" + "errors" + "log/slog" + + "github.com/nginx/agent/v3/internal/datasource/host/exec" + "github.com/nginx/agent/v3/internal/datasource/nginx" + "github.com/nginx/agent/v3/pkg/id" + + "github.com/nginx/agent/v3/pkg/nginxprocess" + "github.com/shirou/gopsutil/v4/process" +) + +type NginxInstanceProcessOperator struct{} + +var _ processOperator = (*NginxInstanceProcessOperator)(nil) + +func NewNginxInstanceProcessOperator() *NginxInstanceProcessOperator { + return &NginxInstanceProcessOperator{} +} + +func (p *NginxInstanceProcessOperator) FindNginxProcesses(ctx context.Context) ([]*nginxprocess.Process, error) { + processes, procErr := process.ProcessesWithContext(ctx) + if procErr != nil { + return nil, procErr + } + + nginxProcesses, err := nginxprocess.ListWithProcesses(ctx, processes) + if err != nil { + return nil, err + } + + return nginxProcesses, nil +} + +func (p *NginxInstanceProcessOperator) NginxWorkerProcesses(ctx context.Context, + masterProcessPid int32, +) []*nginxprocess.Process { + slog.DebugContext(ctx, "Getting NGINX worker processes for NGINX reload") + var workers []*nginxprocess.Process + nginxProcesses, err := p.FindNginxProcesses(ctx) + if err != nil { + slog.WarnContext(ctx, "Failed to get NGINX processes", "error", err) + return workers + } + + for _, nginxProcess := range nginxProcesses { + if nginxProcess.IsWorker() && nginxProcess.PPID == masterProcessPid { + workers = append(workers, nginxProcess) + } + } + + return workers +} + +func (p *NginxInstanceProcessOperator) FindParentProcessID(ctx context.Context, instanceID string, + nginxProcesses []*nginxprocess.Process, executer exec.ExecInterface, +) (int32, error) { + var pid int32 + + for _, proc := range nginxProcesses { + if proc.IsMaster() { + info, infoErr := nginx.ProcessInfo(ctx, proc, executer) + if infoErr != nil { + slog.WarnContext(ctx, "Failed to get NGINX process info from master process", "error", infoErr) + continue + } + processInstanceID := id.Generate("%s_%s_%s", info.ExePath, info.ConfPath, info.Prefix) + if instanceID == processInstanceID { + slog.DebugContext(ctx, "Found NGINX process ID", "process_id", processInstanceID) + return proc.PID, nil + } + } + } + + return pid, errors.New("unable to find parent process") +} diff --git a/internal/resource/nginx_instance_process_operator_test.go b/internal/resource/nginx_instance_process_operator_test.go new file mode 100644 index 000000000..65fe664ae --- /dev/null +++ b/internal/resource/nginx_instance_process_operator_test.go @@ -0,0 +1,179 @@ +// Copyright (c) F5, Inc. +// +// This source code is licensed under the Apache License, Version 2.0 license found in the +// LICENSE file in the root directory of this source tree. + +package resource + +import ( + "bytes" + "context" + "errors" + "fmt" + "testing" + "time" + + "github.com/nginx/agent/v3/internal/datasource/host/exec/execfakes" + "github.com/nginx/agent/v3/pkg/nginxprocess" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestProcessOperator_FindParentProcessID(t *testing.T) { + ctx := context.Background() + + modulePath := t.TempDir() + "/usr/lib/nginx/modules" + + configArgs := fmt.Sprintf(ossConfigArgs, modulePath) + nginxVersionCommandOutput := `nginx version: nginx/1.25.3 + built by clang 14.0.0 (clang-1400.0.29.202) + built with OpenSSL 1.1.1s 1 Nov 2022 (running with OpenSSL 1.1.1t 7 Feb 2023) + TLS SNI support enabled + configure arguments: ` + configArgs + + tests := []struct { + name string // 16 bytes + instanceID string // 16 bytes + expectErr error // 16 bytes (interface) + nginxProcesses []*nginxprocess.Process // 24 bytes (slice header) + expectedPPID int32 + }{ + { + name: "Test 1: Found parent process", + instanceID: "e1374cb1-462d-3b6c-9f3b-f28332b5f10c", + expectErr: nil, + expectedPPID: 1234, + nginxProcesses: []*nginxprocess.Process{ + { + PID: 567, + Created: time.Date(2025, 8, 13, 8, 1, 0, 0, time.Local), + PPID: 1234, + Name: "nginx", + Cmd: "nginx: worker process", + Exe: exePath, + }, + { + PID: 789, + PPID: 1234, + Created: time.Date(2025, 8, 13, 8, 1, 0, 0, time.Local), + Name: "nginx", + Cmd: "nginx: worker process", + Exe: exePath, + }, + { + PID: 1234, + Created: time.Date(2025, 8, 13, 8, 1, 0, 0, time.Local), + PPID: 1, + Name: "nginx", + Cmd: "nginx: master process /usr/local/opt/nginx/bin/nginx -g daemon off;", + Exe: exePath, + }, + }, + }, + { + name: "Test 2: unable to find parent process", + instanceID: "e1374cb1-462d-3b6c-9f3b-f28332b5f10c", + expectErr: errors.New("unable to find parent process"), + expectedPPID: 0, + nginxProcesses: []*nginxprocess.Process{ + { + PID: 567, + Created: time.Date(2025, 8, 13, 8, 1, 0, 0, time.Local), + PPID: 1234, + Name: "nginx", + Cmd: "nginx: worker process", + Exe: exePath, + }, + { + PID: 789, + PPID: 1234, + Created: time.Date(2025, 8, 13, 8, 1, 0, 0, time.Local), + Name: "nginx", + Cmd: "nginx: worker process", + Exe: exePath, + }, + { + PID: 4567, + Created: time.Date(2025, 8, 13, 8, 1, 0, 0, time.Local), + PPID: 1, + Name: "nginx", + Cmd: "nginx: master process /usr/local/opt/nginx/bin/nginx -g daemon off;", + Exe: exePath2, + }, + }, + }, + { + name: "Test 3: Found parent process, multiple NGINX processes", + instanceID: "e1374cb1-462d-3b6c-9f3b-f28332b5f10c", + expectErr: nil, + expectedPPID: 1234, + nginxProcesses: []*nginxprocess.Process{ + { + PID: 567, + Created: time.Date(2025, 8, 13, 8, 1, 0, 0, time.Local), + PPID: 1234, + Name: "nginx", + Cmd: "nginx: worker process", + Exe: exePath, + }, + { + PID: 789, + PPID: 1234, + Created: time.Date(2025, 8, 13, 8, 1, 0, 0, time.Local), + Name: "nginx", + Cmd: "nginx: worker process", + Exe: exePath, + }, + { + PID: 1234, + Created: time.Date(2025, 8, 13, 8, 1, 0, 0, time.Local), + PPID: 1, + Name: "nginx", + Cmd: "nginx: master process /usr/local/opt/nginx/bin/nginx -g daemon off;", + Exe: exePath, + }, + { + PID: 5678, + Created: time.Date(2025, 8, 13, 5, 1, 0, 0, time.Local), + PPID: 1, + Name: "nginx", + Cmd: "nginx: master process /usr/local/opt/nginx/bin/nginx -g daemon off;", + Exe: exePath2, + }, + { + PID: 567, + Created: time.Date(2025, 8, 13, 8, 1, 0, 0, time.Local), + PPID: 1234, + Name: "nginx", + Cmd: "nginx: worker process", + Exe: exePath, + }, + { + PID: 789, + PPID: 1234, + Created: time.Date(2025, 8, 13, 8, 1, 0, 0, time.Local), + Name: "nginx", + Cmd: "nginx: worker process", + Exe: exePath, + }, + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(tt *testing.T) { + processOperator := NewNginxInstanceProcessOperator() + mockExec := &execfakes.FakeExecInterface{} + mockExec.RunCmdReturnsOnCall(0, bytes.NewBufferString(nginxVersionCommandOutput), nil) + ppid, err := processOperator.FindParentProcessID(ctx, test.instanceID, test.nginxProcesses, mockExec) + + if test.expectErr != nil { + require.Error(tt, err) + } else { + require.NoError(tt, err) + } + + assert.Equal(tt, test.expectedPPID, ppid) + }) + } +} diff --git a/internal/resource/resource_service.go b/internal/resource/resource_service.go index 792c28c7a..634e920f1 100644 --- a/internal/resource/resource_service.go +++ b/internal/resource/resource_service.go @@ -19,6 +19,10 @@ import ( "strings" "sync" + "github.com/nginx/agent/v3/internal/datasource/host/exec" + + "github.com/nginx/agent/v3/pkg/nginxprocess" + parser "github.com/nginx/agent/v3/internal/datasource/config" datasource "github.com/nginx/agent/v3/internal/datasource/proto" "github.com/nginx/agent/v3/internal/model" @@ -49,6 +53,9 @@ const ( //go:generate go run github.com/maxbrunsfeld/counterfeiter/v6@v6.8.1 -generate //counterfeiter:generate . instanceOperator +//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6@v6.8.1 -generate +//counterfeiter:generate . processOperator + type resourceServiceInterface interface { AddInstances(instanceList []*mpi.Instance) *mpi.Resource UpdateInstances(ctx context.Context, instanceList []*mpi.Instance) *mpi.Resource @@ -74,6 +81,13 @@ type ( logTailerOperator interface { Tail(ctx context.Context, errorLogs string, errorChannel chan error) } + + processOperator interface { + FindNginxProcesses(ctx context.Context) ([]*nginxprocess.Process, error) + NginxWorkerProcesses(ctx context.Context, masterProcessPid int32) []*nginxprocess.Process + FindParentProcessID(ctx context.Context, instanceID string, nginxProcesses []*nginxprocess.Process, + executer exec.ExecInterface) (int32, error) + } ) type ResourceService struct { diff --git a/internal/resource/resourcefakes/fake_process_operator.go b/internal/resource/resourcefakes/fake_process_operator.go new file mode 100644 index 000000000..9774d58f7 --- /dev/null +++ b/internal/resource/resourcefakes/fake_process_operator.go @@ -0,0 +1,282 @@ +// Code generated by counterfeiter. DO NOT EDIT. +package resourcefakes + +import ( + "context" + "sync" + + "github.com/nginx/agent/v3/internal/datasource/host/exec" + "github.com/nginx/agent/v3/pkg/nginxprocess" +) + +type FakeProcessOperator struct { + FindNginxProcessesStub func(context.Context) ([]*nginxprocess.Process, error) + findNginxProcessesMutex sync.RWMutex + findNginxProcessesArgsForCall []struct { + arg1 context.Context + } + findNginxProcessesReturns struct { + result1 []*nginxprocess.Process + result2 error + } + findNginxProcessesReturnsOnCall map[int]struct { + result1 []*nginxprocess.Process + result2 error + } + FindParentProcessIDStub func(context.Context, string, []*nginxprocess.Process, exec.ExecInterface) (int32, error) + findParentProcessIDMutex sync.RWMutex + findParentProcessIDArgsForCall []struct { + arg1 context.Context + arg2 string + arg3 []*nginxprocess.Process + arg4 exec.ExecInterface + } + findParentProcessIDReturns struct { + result1 int32 + result2 error + } + findParentProcessIDReturnsOnCall map[int]struct { + result1 int32 + result2 error + } + NginxWorkerProcessesStub func(context.Context, int32) []*nginxprocess.Process + nginxWorkerProcessesMutex sync.RWMutex + nginxWorkerProcessesArgsForCall []struct { + arg1 context.Context + arg2 int32 + } + nginxWorkerProcessesReturns struct { + result1 []*nginxprocess.Process + } + nginxWorkerProcessesReturnsOnCall map[int]struct { + result1 []*nginxprocess.Process + } + invocations map[string][][]interface{} + invocationsMutex sync.RWMutex +} + +func (fake *FakeProcessOperator) FindNginxProcesses(arg1 context.Context) ([]*nginxprocess.Process, error) { + fake.findNginxProcessesMutex.Lock() + ret, specificReturn := fake.findNginxProcessesReturnsOnCall[len(fake.findNginxProcessesArgsForCall)] + fake.findNginxProcessesArgsForCall = append(fake.findNginxProcessesArgsForCall, struct { + arg1 context.Context + }{arg1}) + stub := fake.FindNginxProcessesStub + fakeReturns := fake.findNginxProcessesReturns + fake.recordInvocation("FindNginxProcesses", []interface{}{arg1}) + fake.findNginxProcessesMutex.Unlock() + if stub != nil { + return stub(arg1) + } + if specificReturn { + return ret.result1, ret.result2 + } + return fakeReturns.result1, fakeReturns.result2 +} + +func (fake *FakeProcessOperator) FindNginxProcessesCallCount() int { + fake.findNginxProcessesMutex.RLock() + defer fake.findNginxProcessesMutex.RUnlock() + return len(fake.findNginxProcessesArgsForCall) +} + +func (fake *FakeProcessOperator) FindNginxProcessesCalls(stub func(context.Context) ([]*nginxprocess.Process, error)) { + fake.findNginxProcessesMutex.Lock() + defer fake.findNginxProcessesMutex.Unlock() + fake.FindNginxProcessesStub = stub +} + +func (fake *FakeProcessOperator) FindNginxProcessesArgsForCall(i int) context.Context { + fake.findNginxProcessesMutex.RLock() + defer fake.findNginxProcessesMutex.RUnlock() + argsForCall := fake.findNginxProcessesArgsForCall[i] + return argsForCall.arg1 +} + +func (fake *FakeProcessOperator) FindNginxProcessesReturns(result1 []*nginxprocess.Process, result2 error) { + fake.findNginxProcessesMutex.Lock() + defer fake.findNginxProcessesMutex.Unlock() + fake.FindNginxProcessesStub = nil + fake.findNginxProcessesReturns = struct { + result1 []*nginxprocess.Process + result2 error + }{result1, result2} +} + +func (fake *FakeProcessOperator) FindNginxProcessesReturnsOnCall(i int, result1 []*nginxprocess.Process, result2 error) { + fake.findNginxProcessesMutex.Lock() + defer fake.findNginxProcessesMutex.Unlock() + fake.FindNginxProcessesStub = nil + if fake.findNginxProcessesReturnsOnCall == nil { + fake.findNginxProcessesReturnsOnCall = make(map[int]struct { + result1 []*nginxprocess.Process + result2 error + }) + } + fake.findNginxProcessesReturnsOnCall[i] = struct { + result1 []*nginxprocess.Process + result2 error + }{result1, result2} +} + +func (fake *FakeProcessOperator) FindParentProcessID(arg1 context.Context, arg2 string, arg3 []*nginxprocess.Process, arg4 exec.ExecInterface) (int32, error) { + var arg3Copy []*nginxprocess.Process + if arg3 != nil { + arg3Copy = make([]*nginxprocess.Process, len(arg3)) + copy(arg3Copy, arg3) + } + fake.findParentProcessIDMutex.Lock() + ret, specificReturn := fake.findParentProcessIDReturnsOnCall[len(fake.findParentProcessIDArgsForCall)] + fake.findParentProcessIDArgsForCall = append(fake.findParentProcessIDArgsForCall, struct { + arg1 context.Context + arg2 string + arg3 []*nginxprocess.Process + arg4 exec.ExecInterface + }{arg1, arg2, arg3Copy, arg4}) + stub := fake.FindParentProcessIDStub + fakeReturns := fake.findParentProcessIDReturns + fake.recordInvocation("FindParentProcessID", []interface{}{arg1, arg2, arg3Copy, arg4}) + fake.findParentProcessIDMutex.Unlock() + if stub != nil { + return stub(arg1, arg2, arg3, arg4) + } + if specificReturn { + return ret.result1, ret.result2 + } + return fakeReturns.result1, fakeReturns.result2 +} + +func (fake *FakeProcessOperator) FindParentProcessIDCallCount() int { + fake.findParentProcessIDMutex.RLock() + defer fake.findParentProcessIDMutex.RUnlock() + return len(fake.findParentProcessIDArgsForCall) +} + +func (fake *FakeProcessOperator) FindParentProcessIDCalls(stub func(context.Context, string, []*nginxprocess.Process, exec.ExecInterface) (int32, error)) { + fake.findParentProcessIDMutex.Lock() + defer fake.findParentProcessIDMutex.Unlock() + fake.FindParentProcessIDStub = stub +} + +func (fake *FakeProcessOperator) FindParentProcessIDArgsForCall(i int) (context.Context, string, []*nginxprocess.Process, exec.ExecInterface) { + fake.findParentProcessIDMutex.RLock() + defer fake.findParentProcessIDMutex.RUnlock() + argsForCall := fake.findParentProcessIDArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2, argsForCall.arg3, argsForCall.arg4 +} + +func (fake *FakeProcessOperator) FindParentProcessIDReturns(result1 int32, result2 error) { + fake.findParentProcessIDMutex.Lock() + defer fake.findParentProcessIDMutex.Unlock() + fake.FindParentProcessIDStub = nil + fake.findParentProcessIDReturns = struct { + result1 int32 + result2 error + }{result1, result2} +} + +func (fake *FakeProcessOperator) FindParentProcessIDReturnsOnCall(i int, result1 int32, result2 error) { + fake.findParentProcessIDMutex.Lock() + defer fake.findParentProcessIDMutex.Unlock() + fake.FindParentProcessIDStub = nil + if fake.findParentProcessIDReturnsOnCall == nil { + fake.findParentProcessIDReturnsOnCall = make(map[int]struct { + result1 int32 + result2 error + }) + } + fake.findParentProcessIDReturnsOnCall[i] = struct { + result1 int32 + result2 error + }{result1, result2} +} + +func (fake *FakeProcessOperator) NginxWorkerProcesses(arg1 context.Context, arg2 int32) []*nginxprocess.Process { + fake.nginxWorkerProcessesMutex.Lock() + ret, specificReturn := fake.nginxWorkerProcessesReturnsOnCall[len(fake.nginxWorkerProcessesArgsForCall)] + fake.nginxWorkerProcessesArgsForCall = append(fake.nginxWorkerProcessesArgsForCall, struct { + arg1 context.Context + arg2 int32 + }{arg1, arg2}) + stub := fake.NginxWorkerProcessesStub + fakeReturns := fake.nginxWorkerProcessesReturns + fake.recordInvocation("NginxWorkerProcesses", []interface{}{arg1, arg2}) + fake.nginxWorkerProcessesMutex.Unlock() + if stub != nil { + return stub(arg1, arg2) + } + if specificReturn { + return ret.result1 + } + return fakeReturns.result1 +} + +func (fake *FakeProcessOperator) NginxWorkerProcessesCallCount() int { + fake.nginxWorkerProcessesMutex.RLock() + defer fake.nginxWorkerProcessesMutex.RUnlock() + return len(fake.nginxWorkerProcessesArgsForCall) +} + +func (fake *FakeProcessOperator) NginxWorkerProcessesCalls(stub func(context.Context, int32) []*nginxprocess.Process) { + fake.nginxWorkerProcessesMutex.Lock() + defer fake.nginxWorkerProcessesMutex.Unlock() + fake.NginxWorkerProcessesStub = stub +} + +func (fake *FakeProcessOperator) NginxWorkerProcessesArgsForCall(i int) (context.Context, int32) { + fake.nginxWorkerProcessesMutex.RLock() + defer fake.nginxWorkerProcessesMutex.RUnlock() + argsForCall := fake.nginxWorkerProcessesArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2 +} + +func (fake *FakeProcessOperator) NginxWorkerProcessesReturns(result1 []*nginxprocess.Process) { + fake.nginxWorkerProcessesMutex.Lock() + defer fake.nginxWorkerProcessesMutex.Unlock() + fake.NginxWorkerProcessesStub = nil + fake.nginxWorkerProcessesReturns = struct { + result1 []*nginxprocess.Process + }{result1} +} + +func (fake *FakeProcessOperator) NginxWorkerProcessesReturnsOnCall(i int, result1 []*nginxprocess.Process) { + fake.nginxWorkerProcessesMutex.Lock() + defer fake.nginxWorkerProcessesMutex.Unlock() + fake.NginxWorkerProcessesStub = nil + if fake.nginxWorkerProcessesReturnsOnCall == nil { + fake.nginxWorkerProcessesReturnsOnCall = make(map[int]struct { + result1 []*nginxprocess.Process + }) + } + fake.nginxWorkerProcessesReturnsOnCall[i] = struct { + result1 []*nginxprocess.Process + }{result1} +} + +func (fake *FakeProcessOperator) Invocations() map[string][][]interface{} { + fake.invocationsMutex.RLock() + defer fake.invocationsMutex.RUnlock() + fake.findNginxProcessesMutex.RLock() + defer fake.findNginxProcessesMutex.RUnlock() + fake.findParentProcessIDMutex.RLock() + defer fake.findParentProcessIDMutex.RUnlock() + fake.nginxWorkerProcessesMutex.RLock() + defer fake.nginxWorkerProcessesMutex.RUnlock() + copiedInvocations := map[string][][]interface{}{} + for key, value := range fake.invocations { + copiedInvocations[key] = value + } + return copiedInvocations +} + +func (fake *FakeProcessOperator) 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) +} diff --git a/internal/watcher/instance/nginx_process_parser.go b/internal/watcher/instance/nginx_process_parser.go index 3a54cd914..f228dd157 100644 --- a/internal/watcher/instance/nginx_process_parser.go +++ b/internal/watcher/instance/nginx_process_parser.go @@ -6,17 +6,16 @@ package instance import ( - "bufio" - "bytes" "context" "fmt" "log/slog" "os" - "path" - "regexp" "sort" "strings" + "github.com/nginx/agent/v3/internal/datasource/nginx" + "github.com/nginx/agent/v3/internal/model" + mpi "github.com/nginx/agent/v3/api/grpc/mpi/v1" "github.com/nginx/agent/v3/internal/datasource/host/exec" "github.com/nginx/agent/v3/pkg/id" @@ -34,24 +33,9 @@ type ( NginxProcessParser struct { executer exec.ExecInterface } - - Info struct { - ConfigureArgs map[string]interface{} - Version string - Prefix string - ConfPath string - ExePath string - LoadableModules []string - DynamicModules []string - ProcessID int32 - } ) -var ( - _ processParser = (*NginxProcessParser)(nil) - - versionRegex = regexp.MustCompile(`(?P\S+)\/(?P.*)`) -) +var _ processParser = (*NginxProcessParser)(nil) func NewNginxProcessParser() *NginxProcessParser { return &NginxProcessParser{ @@ -126,34 +110,11 @@ func (npp *NginxProcessParser) Parse(ctx context.Context, processes []*nginxproc return instanceMap } -func (npp *NginxProcessParser) info(ctx context.Context, proc *nginxprocess.Process) (*Info, error) { - exePath := proc.Exe - - if exePath == "" { - exePath = npp.exe(ctx) - if exePath == "" { - return nil, fmt.Errorf("unable to find NGINX exe for process %d", proc.PID) - } - } - - confPath := confPathFromCommand(proc.Cmd) - - var nginxInfo *Info - - outputBuffer, err := npp.executer.RunCmd(ctx, exePath, "-V") +func (npp *NginxProcessParser) info(ctx context.Context, proc *nginxprocess.Process) (*model.ProcessInfo, error) { + nginxInfo, err := nginx.ProcessInfo(ctx, proc, npp.executer) if err != nil { return nil, err } - - nginxInfo = parseNginxVersionCommandOutput(ctx, outputBuffer) - - nginxInfo.ExePath = exePath - nginxInfo.ProcessID = proc.PID - - if nginxInfo.ConfPath = nginxConfPath(ctx, nginxInfo); confPath != "" { - nginxInfo.ConfPath = confPath - } - loadableModules := loadableModules(nginxInfo) nginxInfo.LoadableModules = loadableModules @@ -162,44 +123,7 @@ func (npp *NginxProcessParser) info(ctx context.Context, proc *nginxprocess.Proc return nginxInfo, err } -func (npp *NginxProcessParser) exe(ctx context.Context) string { - exePath := "" - - out, commandErr := npp.executer.RunCmd(ctx, "sh", "-c", "command -v nginx") - if commandErr == nil { - exePath = strings.TrimSuffix(out.String(), "\n") - } - - if exePath == "" { - exePath = npp.defaultToNginxCommandForProcessPath() - } - - if strings.Contains(exePath, "(deleted)") { - exePath = sanitizeExeDeletedPath(exePath) - } - - return exePath -} - -func (npp *NginxProcessParser) defaultToNginxCommandForProcessPath() string { - exePath, err := npp.executer.FindExecutable("nginx") - if err != nil { - return "" - } - - return exePath -} - -func sanitizeExeDeletedPath(exe string) string { - firstSpace := strings.Index(exe, "(deleted)") - if firstSpace != -1 { - return strings.TrimSpace(exe[0:firstSpace]) - } - - return strings.TrimSpace(exe) -} - -func convertInfoToInstance(nginxInfo Info) *mpi.Instance { +func convertInfoToInstance(nginxInfo model.ProcessInfo) *mpi.Instance { var instanceRuntime *mpi.InstanceRuntime nginxType := mpi.InstanceMeta_INSTANCE_TYPE_NGINX version := nginxInfo.Version @@ -259,87 +183,7 @@ func convertInfoToInstance(nginxInfo Info) *mpi.Instance { } } -func parseNginxVersionCommandOutput(ctx context.Context, output *bytes.Buffer) *Info { - nginxInfo := &Info{} - - scanner := bufio.NewScanner(output) - for scanner.Scan() { - line := strings.TrimSpace(scanner.Text()) - switch { - case strings.HasPrefix(line, "nginx version"): - nginxInfo.Version = parseNginxVersion(line) - case strings.HasPrefix(line, "configure arguments"): - nginxInfo.ConfigureArgs = parseConfigureArguments(line) - } - } - - nginxInfo.Prefix = nginxPrefix(ctx, nginxInfo) - - return nginxInfo -} - -func parseNginxVersion(line string) string { - return strings.TrimPrefix(versionRegex.FindString(line), "nginx/") -} - -func parseConfigureArguments(line string) map[string]interface{} { - // need to check for empty strings - flags := strings.Split(line[len("configure arguments:"):], " --") - result := make(map[string]interface{}) - - for _, flag := range flags { - vals := strings.Split(flag, "=") - if isFlag(vals) { - result[vals[0]] = true - } else if isKeyValueFlag(vals) { - result[vals[0]] = vals[1] - } - } - - return result -} - -func nginxPrefix(ctx context.Context, nginxInfo *Info) string { - var prefix string - - if nginxInfo.ConfigureArgs["prefix"] != nil { - var ok bool - prefix, ok = nginxInfo.ConfigureArgs["prefix"].(string) - if !ok { - slog.DebugContext(ctx, "Failed to cast nginxInfo prefix to string") - } - } else { - prefix = "/usr/local/nginx" - } - - return prefix -} - -func nginxConfPath(ctx context.Context, nginxInfo *Info) string { - var confPath string - - if nginxInfo.ConfigureArgs["conf-path"] != nil { - var ok bool - confPath, ok = nginxInfo.ConfigureArgs["conf-path"].(string) - if !ok { - slog.DebugContext(ctx, "failed to cast nginxInfo conf-path to string") - } - } else { - confPath = path.Join(nginxInfo.Prefix, "/conf/nginx.conf") - } - - return confPath -} - -func isFlag(vals []string) bool { - return len(vals) == flagLen && vals[0] != "" -} - -func isKeyValueFlag(vals []string) bool { - return len(vals) == keyValueLen -} - -func loadableModules(nginxInfo *Info) (modules []string) { +func loadableModules(nginxInfo *model.ProcessInfo) (modules []string) { var err error if mp, ok := nginxInfo.ConfigureArgs["modules-path"]; ok { modulePath, pathOK := mp.(string) @@ -361,7 +205,7 @@ func loadableModules(nginxInfo *Info) (modules []string) { return modules } -func dynamicModules(nginxInfo *Info) (modules []string) { +func dynamicModules(nginxInfo *model.ProcessInfo) (modules []string) { configArgs := nginxInfo.ConfigureArgs for arg := range configArgs { if strings.HasPrefix(arg, withWithPrefix) && strings.HasSuffix(arg, withModuleSuffix) { @@ -397,17 +241,3 @@ func convertToMap(processes []*nginxprocess.Process) map[int32]*nginxprocess.Pro return processesByPID } - -func confPathFromCommand(command string) string { - commands := strings.Split(command, " ") - - for i, command := range commands { - if command == "-c" { - if i < len(commands)-1 { - return commands[i+1] - } - } - } - - return "" -} diff --git a/internal/watcher/instance/nginx_process_parser_test.go b/internal/watcher/instance/nginx_process_parser_test.go index 889807960..09fcf9d85 100644 --- a/internal/watcher/instance/nginx_process_parser_test.go +++ b/internal/watcher/instance/nginx_process_parser_test.go @@ -8,13 +8,14 @@ package instance import ( "bytes" "context" - "errors" "fmt" "path/filepath" "sort" "strings" "testing" + "github.com/nginx/agent/v3/internal/model" + "google.golang.org/protobuf/proto" mpi "github.com/nginx/agent/v3/api/grpc/mpi/v1" @@ -415,7 +416,7 @@ func TestGetInfo(t *testing.T) { tests := []struct { process *nginxprocess.Process - expected *Info + expected *model.ProcessInfo name string nginxVersionCommandOutput string }{ @@ -431,7 +432,7 @@ func TestGetInfo(t *testing.T) { PID: 1123, Exe: exePath, }, - expected: &Info{ + expected: &model.ProcessInfo{ ProcessID: 1123, Version: "1.25.3", Prefix: "/usr/local/Cellar/nginx/1.25.3", @@ -499,7 +500,7 @@ func TestGetInfo(t *testing.T) { PID: 3141, Exe: exePath, }, - expected: &Info{ + expected: &model.ProcessInfo{ ProcessID: 3141, Version: "1.25.3 (nginx-plus-r31-p1)", Prefix: "/etc/nginx", @@ -580,51 +581,3 @@ func TestGetInfo(t *testing.T) { }) } } - -func TestNginxProcessParser_GetExe(t *testing.T) { - ctx := context.Background() - - tests := []struct { - commandError error - name string - expected string - commandOutput []byte - }{ - { - name: "Test 1: Default exe if error executing command -v nginx", - commandOutput: []byte{}, - commandError: errors.New("command error"), - expected: "/usr/bin/nginx", - }, - { - name: "Test 2: Sanitize Exe Deleted Path", - commandOutput: []byte("/usr/sbin/nginx (deleted)"), - commandError: nil, - expected: "/usr/sbin/nginx", - }, - } - for _, test := range tests { - t.Run(test.name, func(tt *testing.T) { - mockExec := &execfakes.FakeExecInterface{} - mockExec.RunCmdReturns(bytes.NewBuffer(test.commandOutput), test.commandError) - mockExec.FindExecutableReturns("/usr/bin/nginx", nil) - - nginxProcessParser := NewNginxProcessParser() - nginxProcessParser.executer = mockExec - result := nginxProcessParser.exe(ctx) - - assert.Equal(tt, test.expected, result) - }) - } -} - -func TestGetConfigPathFromCommand(t *testing.T) { - result := confPathFromCommand("nginx: master process nginx -c /tmp/nginx.conf") - assert.Equal(t, "/tmp/nginx.conf", result) - - result = confPathFromCommand("nginx: master process nginx -c") - assert.Empty(t, result) - - result = confPathFromCommand("") - assert.Empty(t, result) -} diff --git a/test/docker/load/Dockerfile b/test/docker/load/Dockerfile index 1d7608910..48e2071d4 100644 --- a/test/docker/load/Dockerfile +++ b/test/docker/load/Dockerfile @@ -16,23 +16,11 @@ RUN --mount=type=secret,id=nginx-crt,dst=nginx-repo.crt \ && apt-get update \ && apt-get install --no-install-recommends --no-install-suggests -y \ ca-certificates \ - gnupg1 \ + gpg \ lsb-release \ git \ wget \ make \ - && \ - NGINX_GPGKEY=573BFD6B3D8FBC641079A6ABABF5BD827BD9BF62; \ - found=''; \ - for server in \ - hkp://keyserver.ubuntu.com:80 \ - pgp.mit.edu \ - ; do \ - echo "Fetching GPG key $NGINX_GPGKEY from $server"; \ - apt-key adv --keyserver "$server" --keyserver-options timeout=10 --recv-keys "$NGINX_GPGKEY" && found=yes && break; \ - done; \ - test -z "$found" && echo >&2 "error: failed to fetch GPG key $NGINX_GPGKEY" && exit 1; \ - apt-get remove --purge --auto-remove -y gnupg1 && rm -rf /var/lib/apt/lists/* \ # Install the latest release of NGINX Plus and/or NGINX Plus modules # Uncomment individual modules if necessary # Use versioned packages over defaults to specify a release @@ -43,7 +31,8 @@ RUN --mount=type=secret,id=nginx-crt,dst=nginx-repo.crt \ && echo "Acquire::https::pkgs.nginx.com::Verify-Host \"true\";" >> /etc/apt/apt.conf.d/90nginx \ && echo "Acquire::https::pkgs.nginx.com::SslCert \"/etc/ssl/nginx/nginx-repo.crt\";" >> /etc/apt/apt.conf.d/90nginx \ && echo "Acquire::https::pkgs.nginx.com::SslKey \"/etc/ssl/nginx/nginx-repo.key\";" >> /etc/apt/apt.conf.d/90nginx \ - && printf "deb https://pkgs.nginx.com/plus/ubuntu `lsb_release -cs` nginx-plus\n" > /etc/apt/sources.list.d/nginx-plus.list \ + && printf "deb [signed-by=/usr/share/keyrings/nginx-archive-keyring.gpg] https://pkgs.nginx.com/plus/${PLUS_VERSION}/ubuntu/ `lsb_release -cs` nginx-plus\n" > /etc/apt/sources.list.d/nginx-plus.list \ + && wget -qO - https://cs.nginx.com/static/keys/nginx_signing.key | gpg --dearmor | tee /usr/share/keyrings/nginx-archive-keyring.gpg >/dev/null \ && mkdir -p /etc/ssl/nginx \ && cat nginx-repo.crt > /etc/ssl/nginx/nginx-repo.crt \ && cat nginx-repo.key > /etc/ssl/nginx/nginx-repo.key \ @@ -53,6 +42,7 @@ RUN --mount=type=secret,id=nginx-crt,dst=nginx-repo.crt \ curl \ gettext-base \ jq \ + gnupg2 \ && apt-get remove --purge -y lsb-release \ && apt-get remove --purge --auto-remove -y && rm -rf /var/lib/apt/lists/* /etc/apt/sources.list.d/nginx-plus.list \ && rm -rf /etc/apt/apt.conf.d/90nginx /etc/ssl/nginx diff --git a/test/docker/nginx-plus/deb/Dockerfile b/test/docker/nginx-plus/deb/Dockerfile index 392ed5fb7..55c5c773f 100644 --- a/test/docker/nginx-plus/deb/Dockerfile +++ b/test/docker/nginx-plus/deb/Dockerfile @@ -22,23 +22,11 @@ RUN --mount=type=secret,id=nginx-crt,dst=nginx-repo.crt \ && apt-get update \ && apt-get install --no-install-recommends --no-install-suggests -y \ ca-certificates \ - gnupg1 \ + gpg \ lsb-release \ git \ wget \ make \ - && \ - NGINX_GPGKEY=573BFD6B3D8FBC641079A6ABABF5BD827BD9BF62; \ - found=''; \ - for server in \ - hkp://keyserver.ubuntu.com:80 \ - pgp.mit.edu \ - ; do \ - echo "Fetching GPG key $NGINX_GPGKEY from $server"; \ - apt-key adv --keyserver "$server" --keyserver-options timeout=10 --recv-keys "$NGINX_GPGKEY" && found=yes && break; \ - done; \ - test -z "$found" && echo >&2 "error: failed to fetch GPG key $NGINX_GPGKEY" && exit 1; \ - apt-get remove --purge --auto-remove -y gnupg1 && rm -rf /var/lib/apt/lists/* \ # Install the latest release of NGINX Plus and/or NGINX Plus modules # Uncomment individual modules if necessary # Use versioned packages over defaults to specify a release @@ -49,7 +37,8 @@ RUN --mount=type=secret,id=nginx-crt,dst=nginx-repo.crt \ && echo "Acquire::https::pkgs.nginx.com::Verify-Host \"true\";" >> /etc/apt/apt.conf.d/90nginx \ && echo "Acquire::https::pkgs.nginx.com::SslCert \"/etc/ssl/nginx/nginx-repo.crt\";" >> /etc/apt/apt.conf.d/90nginx \ && echo "Acquire::https::pkgs.nginx.com::SslKey \"/etc/ssl/nginx/nginx-repo.key\";" >> /etc/apt/apt.conf.d/90nginx \ - && printf "deb https://pkgs.nginx.com/plus/${PLUS_VERSION}/ubuntu/ `lsb_release -cs` nginx-plus\n" > /etc/apt/sources.list.d/nginx-plus.list \ + && printf "deb [signed-by=/usr/share/keyrings/nginx-archive-keyring.gpg] https://pkgs.nginx.com/plus/${PLUS_VERSION}/ubuntu/ `lsb_release -cs` nginx-plus\n" > /etc/apt/sources.list.d/nginx-plus.list \ + && wget -qO - https://cs.nginx.com/static/keys/nginx_signing.key | gpg --dearmor | tee /usr/share/keyrings/nginx-archive-keyring.gpg >/dev/null \ && mkdir -p /etc/ssl/nginx \ && cat nginx-repo.crt > /etc/ssl/nginx/nginx-repo.crt \ && cat nginx-repo.key > /etc/ssl/nginx/nginx-repo.key \ diff --git a/test/integration/utils/config_apply_utils.go b/test/integration/utils/config_apply_utils.go index bc0c5acb5..924dfcddd 100644 --- a/test/integration/utils/config_apply_utils.go +++ b/test/integration/utils/config_apply_utils.go @@ -20,9 +20,9 @@ import ( ) const ( - RetryCount = 8 + RetryCount = 10 RetryWaitTime = 5 * time.Second - RetryMaxWaitTime = 6 * time.Second + RetryMaxWaitTime = 1 * time.Minute ) var (