Merge branch 'v3' into fix-nginx-response-status-metrics

internal/command/command_plugin_test.go

@@ -159,7 +159,6 @@ func TestMonitorSubscribeChannel(t *testing.T) {
 
 	// Verify the logger was called
 	if s := logBuf.String(); !strings.Contains(s, "Received management plane request") {
-		// defer wg.Done()
 		t.Errorf("Unexpected log %s", s)
 	}
 

internal/config/config.go

@@ -144,6 +144,13 @@ func registerFlags() {
 		"Warning messages in the NGINX errors logs after a NGINX reload will be treated as an error.",
 	)
 
+	fs.String(
+		NginxExcludeLogsKey,
+		"",
+		"One or more NGINX log paths that you want to exclude from metrics collection or error monitoring "+
+			"This key is formatted as a string and follows Unix PATH format",
+	)
+
 	fs.Duration(ClientTimeoutKey, time.Minute, "Client timeout")
 	fs.String(ConfigDirectoriesKey,
 		DefConfigDirectories,
@@ -319,7 +326,8 @@ func resolveDataPlaneConfig() *DataPlaneConfig {
 	return &DataPlaneConfig{
 		Nginx: &NginxDataPlaneConfig{
 			ReloadMonitoringPeriod: viperInstance.GetDuration(NginxReloadMonitoringPeriodKey),
-			TreatWarningsAsError:   viperInstance.GetBool(NginxTreatWarningsAsErrorsKey),
+			TreatWarningsAsErrors:  viperInstance.GetBool(NginxTreatWarningsAsErrorsKey),
+			ExcludeLogs:            viperInstance.GetString(NginxExcludeLogsKey),
 		},
 	}
 }

internal/config/config_test.go

@@ -62,7 +62,8 @@ func TestResolveConfig(t *testing.T) {
 	assert.Equal(t, "./", actual.Log.Path)
 
 	assert.Equal(t, 30*time.Second, actual.DataPlaneConfig.Nginx.ReloadMonitoringPeriod)
-	assert.False(t, actual.DataPlaneConfig.Nginx.TreatWarningsAsError)
+	assert.False(t, actual.DataPlaneConfig.Nginx.TreatWarningsAsErrors)
+	assert.Equal(t, "/var/log/nginx/error.log:/var/log/nginx/access.log", actual.DataPlaneConfig.Nginx.ExcludeLogs)
 
 	require.NotNil(t, actual.Collector)
 	assert.Equal(t, "/etc/nginx-agent/nginx-agent-otelcol.yaml", actual.Collector.ConfigPath)

internal/config/flags.go

@@ -54,7 +54,8 @@ var (
 	LogLevelKey                    = pre(LogLevelRootKey) + "level"
 	LogPathKey                     = pre(LogLevelRootKey) + "path"
 	NginxReloadMonitoringPeriodKey = pre(DataPlaneConfigRootKey, "nginx") + "reload_monitoring_period"
-	NginxTreatWarningsAsErrorsKey  = pre(DataPlaneConfigRootKey, "nginx") + "treat_warnings_as_error"
+	NginxTreatWarningsAsErrorsKey  = pre(DataPlaneConfigRootKey, "nginx") + "treat_warnings_as_errors"
+	NginxExcludeLogsKey            = pre(DataPlaneConfigRootKey, "nginx") + "exclude_logs"
 	OTLPExportURLKey               = pre(CollectorRootKey) + "otlp_export_url"
 	OTLPReceiverURLKey             = pre(CollectorRootKey) + "otlp_receiver_url"
 )

internal/config/testdata/nginx-agent.conf

@@ -13,7 +13,8 @@ watchers:
 data_plane_config:
     nginx:
         reload_monitoring_period: 30s
-        treat_warnings_as_error: false
+        treat_warnings_as_errors: false
+        exclude_logs: "/var/log/nginx/error.log:/var/log/nginx/access.log"
 client: 
   timeout: 10s
 

internal/config/types.go

@@ -69,8 +69,9 @@ type (
 	}
 
 	NginxDataPlaneConfig struct {
+		ExcludeLogs            string        `yaml:"-" mapstructure:"exclude_logs"`
 		ReloadMonitoringPeriod time.Duration `yaml:"-" mapstructure:"reload_monitoring_period"`
-		TreatWarningsAsError   bool          `yaml:"-" mapstructure:"treat_warnings_as_error"`
+		TreatWarningsAsErrors  bool          `yaml:"-" mapstructure:"treat_warnings_as_errors"`
 	}
 
 	Client struct {

internal/file/file_plugin.go

@@ -191,23 +191,41 @@ func (fp *FilePlugin) handleConfigApplyRequest(ctx context.Context, msg *bus.Mes
 			"instance_id", configApplyRequest.GetOverview().GetConfigVersion().GetInstanceId(),
 			"error", err,
 		)
+
 		response = fp.createDataPlaneResponse(
 			correlationID,
 			mpi.CommandResponse_COMMAND_STATUS_ERROR,
 			"Config apply failed, rolling back config",
 			configApplyRequest.GetOverview().GetConfigVersion().GetInstanceId(),
 			err.Error(),
 		)
+		fp.messagePipe.Process(ctx, &bus.Message{Topic: bus.DataPlaneResponseTopic, Data: response})
+
+		rollbackErr := fp.fileManagerService.Rollback(
+			ctx,
+			configApplyRequest.GetOverview().GetConfigVersion().GetInstanceId(),
+		)
+		if rollbackErr != nil {
+			rollbackResponse := fp.createDataPlaneResponse(
+				correlationID,
+				mpi.CommandResponse_COMMAND_STATUS_FAILURE,
+				"Config apply failed, rollback failed",
+				configApplyRequest.GetOverview().GetConfigVersion().GetInstanceId(),
+				rollbackErr.Error())
+			fp.messagePipe.Process(ctx, &bus.Message{Topic: bus.DataPlaneResponseTopic, Data: rollbackResponse})
+			fp.fileManagerService.ClearCache()
+
+			return
+		}
 
+		response = fp.createDataPlaneResponse(
+			correlationID,
+			mpi.CommandResponse_COMMAND_STATUS_FAILURE,
+			"Config apply failed, rollback successful",
+			configApplyRequest.GetOverview().GetConfigVersion().GetInstanceId(),
+			err.Error())
 		fp.messagePipe.Process(ctx, &bus.Message{Topic: bus.DataPlaneResponseTopic, Data: response})
-		fp.handleConfigApplyFailedRequest(ctx, &bus.Message{
-			Topic: bus.ConfigApplyFailedTopic,
-			Data: &model.ConfigApplyMessage{
-				CorrelationID: correlationID,
-				InstanceID:    configApplyRequest.GetOverview().GetConfigVersion().GetInstanceId(),
-				Error:         err,
-			},
-		})
+		fp.fileManagerService.ClearCache()
 
 		return
 	case model.OK:

internal/file/file_plugin_test.go

@@ -181,6 +181,12 @@ func TestFilePlugin_Process_ConfigApplyRequestTopic(t *testing.T) {
 				assert.Equal(t, "Config apply failed, rolling back config",
 					dataPlaneResponse.GetCommandResponse().GetMessage())
 				assert.Equal(t, test.configApplyReturnsErr.Error(), dataPlaneResponse.GetCommandResponse().GetError())
+				dataPlaneResponse, ok = messages[1].Data.(*mpi.DataPlaneResponse)
+				assert.True(t, ok)
+				assert.Equal(t, "Config apply failed, rollback successful",
+					dataPlaneResponse.GetCommandResponse().GetMessage())
+				assert.Equal(t, mpi.CommandResponse_COMMAND_STATUS_FAILURE,
+					dataPlaneResponse.GetCommandResponse().GetStatus())
 			case test.configApplyStatus == model.NoChange:
 				assert.Len(t, messages, 2)
 				dataPlaneResponse, ok := messages[0].Data.(*mpi.DataPlaneResponse)

internal/resource/nginx_log_tailer_operator.go

@@ -71,7 +71,7 @@ func (l *NginxLogTailerOperator) Tail(ctx context.Context, errorLog string, erro
 }
 
 func (l *NginxLogTailerOperator) doesLogLineContainError(line string) bool {
-	if l.agentConfig.DataPlaneConfig.Nginx.TreatWarningsAsError && warningRegex.MatchString(line) {
+	if l.agentConfig.DataPlaneConfig.Nginx.TreatWarningsAsErrors && warningRegex.MatchString(line) {
 		return true
 	}
 

internal/watcher/instance/nginx_config_parser.go

@@ -16,6 +16,7 @@ import (
 	"net/http"
 	"os"
 	"path/filepath"
+	"slices"
 	"strconv"
 	"strings"
 
@@ -83,7 +84,7 @@ func (ncp *NginxConfigParser) Parse(ctx context.Context, instance *mpi.Instance)
 	return ncp.createNginxConfigContext(ctx, instance, payload)
 }
 
-// nolint: cyclop,revive
+// nolint: cyclop,revive,gocognit
 func (ncp *NginxConfigParser) createNginxConfigContext(
 	ctx context.Context,
 	instance *mpi.Instance,
@@ -103,11 +104,16 @@ func (ncp *NginxConfigParser) createNginxConfigContext(
 				case "log_format":
 					formatMap = ncp.formatMap(directive)
 				case "access_log":
-					accessLog := ncp.accessLog(directive.Args[0], ncp.accessLogDirectiveFormat(directive), formatMap)
-					nginxConfigContext.AccessLogs = append(nginxConfigContext.AccessLogs, accessLog)
+					if !ncp.ignoreLog(directive.Args[0]) {
+						accessLog := ncp.accessLog(directive.Args[0], ncp.accessLogDirectiveFormat(directive),
+							formatMap)
+						nginxConfigContext.AccessLogs = append(nginxConfigContext.AccessLogs, accessLog)
+					}
 				case "error_log":
-					errorLog := ncp.errorLog(directive.Args[0], ncp.errorLogDirectiveLevel(directive))
-					nginxConfigContext.ErrorLogs = append(nginxConfigContext.ErrorLogs, errorLog)
+					if !ncp.ignoreLog(directive.Args[0]) {
+						errorLog := ncp.errorLog(directive.Args[0], ncp.errorLogDirectiveLevel(directive))
+						nginxConfigContext.ErrorLogs = append(nginxConfigContext.ErrorLogs, errorLog)
+					}
 				case "root":
 					rootFiles := ncp.rootFiles(ctx, directive.Args[0])
 					nginxConfigContext.Files = append(nginxConfigContext.Files, rootFiles...)
@@ -146,6 +152,31 @@ func (ncp *NginxConfigParser) createNginxConfigContext(
 	return nginxConfigContext, nil
 }
 
+func (ncp *NginxConfigParser) ignoreLog(logPath string) bool {
+	logLower := strings.ToLower(logPath)
+	ignoreLogs := []string{"off", "/dev/stderr", "/dev/stdout", "/dev/null"}
+
+	if strings.HasPrefix(logLower, "syslog:") || slices.Contains(ignoreLogs, logLower) {
+		return true
+	}
+
+	for _, path := range strings.Split(ncp.agentConfig.DataPlaneConfig.Nginx.ExcludeLogs, ":") {
+		ok, err := filepath.Match(path, logPath)
+		if err != nil {
+			slog.Error("Invalid path for excluding log", "log_path", path)
+		} else if ok {
+			slog.Info("Excluding log as specified in config", "log_path", logPath)
+			return true
+		}
+	}
+
+	if !ncp.agentConfig.IsDirectoryAllowed(logLower) {
+		slog.Warn("Log being read is outside of allowed directories", "log_path", logPath)
+	}
+
+	return false
+}
+
 func (ncp *NginxConfigParser) formatMap(directive *crossplane.Directive) map[string]string {
 	formatMap := make(map[string]string)
 

internal/watcher/instance/nginx_config_parser_test.go

@@ -6,13 +6,17 @@
 package instance
 
 import (
+	"bytes"
 	"context"
 	"fmt"
 	"net/http"
 	"net/http/httptest"
 	"os"
+	"strings"
 	"testing"
 
+	"github.com/nginx/agent/v3/test/stub"
+
 	"github.com/google/go-cmp/cmp"
 	mpi "github.com/nginx/agent/v3/api/grpc/mpi/v1"
 	"github.com/nginx/agent/v3/pkg/files"
@@ -646,3 +650,94 @@ Reading: 0 Writing: 1 Waiting: 1
 		})
 	}
 }
+
+func TestNginxConfigParser_ignoreLog(t *testing.T) {
+	tests := []struct {
+		name        string
+		logPath     string
+		excludeLogs string
+		expectedLog string
+		expected    bool
+	}{
+		{
+			name:        "Test 1: allowed log path",
+			logPath:     "/tmp/var/log/nginx/access.log",
+			excludeLogs: "",
+			expected:    false,
+			expectedLog: "",
+		},
+		{
+			name:        "Test 2: syslog",
+			logPath:     "syslog:server=unix:/var/log/nginx.sock,nohostname;",
+			excludeLogs: "",
+			expected:    true,
+			expectedLog: "",
+		},
+		{
+			name:        "Test 3: log off",
+			logPath:     "off",
+			excludeLogs: "",
+			expected:    true,
+			expectedLog: "",
+		},
+		{
+			name:        "Test 4: log /dev/stderr",
+			logPath:     "/dev/stderr",
+			excludeLogs: "",
+			expected:    true,
+			expectedLog: "",
+		},
+		{
+			name:        "Test 5: log /dev/stdout",
+			logPath:     "/dev/stdout",
+			excludeLogs: "",
+			expected:    true,
+			expectedLog: "",
+		},
+		{
+			name:        "Test 6: log /dev/null",
+			logPath:     "/dev/null",
+			excludeLogs: "",
+			expected:    true,
+			expectedLog: "",
+		},
+		{
+			name:        "Test 7: exclude logs set, log path should be excluded",
+			logPath:     "/tmp/var/log/nginx/alert.log",
+			excludeLogs: "/tmp/var/log/nginx/[^ace]*:/tmp/var/log/nginx/a[^c]*",
+			expected:    true,
+			expectedLog: "",
+		},
+		{
+			name:        "Test 8: exclude logs set, log path is allowed",
+			logPath:     "/tmp/var/log/nginx/access.log",
+			excludeLogs: "/tmp/var/log/nginx/[^ace]*:/tmp/var/log/nginx/a[^c]*",
+			expected:    false,
+			expectedLog: "",
+		},
+		{
+			name:        "Test 9: log path outside allowed dir",
+			logPath:     "/var/log/nginx/access.log",
+			excludeLogs: "/var/log/nginx/[^ace]*:/var/log/nginx/a[^c]*",
+			expected:    false,
+			expectedLog: "Log being read is outside of allowed directories",
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			logBuf := &bytes.Buffer{}
+			stub.StubLoggerWith(logBuf)
+
+			agentConfig := types.AgentConfig()
+			agentConfig.DataPlaneConfig.Nginx.ExcludeLogs = test.excludeLogs
+
+			ncp := NewNginxConfigParser(agentConfig)
+			assert.Equal(t, test.expected, ncp.ignoreLog(test.logPath))
+
+			if s := logBuf.String(); !strings.Contains(s, test.expectedLog) {
+				t.Errorf("Expected to receive log: %s", test.expectedLog)
+			}
+		})
+	}
+}

test/config/nginx/nginx-with-server-block-access-log.conf

@@ -0,0 +1,54 @@
+worker_processes  1;
+error_log  /var/log/nginx/error.log;
+
+events {
+    worker_connections  1024;
+}
+
+http {
+    include       mime.types;
+    default_type  application/octet-stream;
+
+    log_format  main  '$remote_addr - $remote_user [$time_local] "$request" '
+                  '$status $body_bytes_sent "$http_referer" '
+                  '"$http_user_agent" "$http_x_forwarded_for" '
+                  '"$bytes_sent" "$request_length" "$request_time" '
+                  '"$gzip_ratio" $server_protocol ';
+
+    access_log  /var/log/nginx/access.log  main;
+
+    sendfile        on;
+    keepalive_timeout  65;
+
+    server {
+        listen       8080;
+        server_name  localhost;
+
+        access_log  /var/log/nginx/server.log  main;
+
+        location / {
+            root   /usr/share/nginx/html;
+            index  index.html index.htm;
+        }
+
+        ##
+        # Enable Metrics
+        ##
+        location /api {
+            stub_status;
+            allow 127.0.0.1;
+            deny all;
+        }
+
+        location /test {
+            return 200 "Test Success";
+        }
+
+        # redirect server error pages to the static page /50x.html
+        #
+        error_page   500 502 503 504  /50x.html;
+        location = /50x.html {
+            root   /usr/share/nginx/html;
+        }
+    }
+}