From 9bc026001d603850e87081b96f4ad639e0ea5c72 Mon Sep 17 00:00:00 2001
From: aphralG <108004222+aphralG@users.noreply.github.com>
Date: Mon, 7 Oct 2024 15:26:57 +0100
Subject: [PATCH] Agent Config Changes: rename & update config_dirs & update
 exclude_logs  (#882)

---
 internal/config/config.go                     | 27 +++++++++----------
 internal/config/config_test.go                |  8 +++---
 internal/config/defaults.go                   | 15 ++++++++---
 internal/config/flags.go                      |  2 +-
 internal/config/testdata/nginx-agent.conf     | 11 ++++++--
 internal/config/types.go                      |  5 ++--
 .../watcher/instance/nginx_config_parser.go   |  2 +-
 .../instance/nginx_config_parser_test.go      | 20 +++++++-------
 nginx-agent.conf                              |  9 +++++--
 scripts/packages/postinstall.sh               |  7 +++--
 test/config/agent/nginx-agent-otel-load.conf  |  6 ++++-
 .../agent/nginx-config-with-grpc-client.conf  |  5 +++-
 test/mock/collector/nginx-agent.conf          |  6 ++++-
 test/types/config.go                          |  3 +--
 14 files changed, 79 insertions(+), 47 deletions(-)

diff --git a/internal/config/config.go b/internal/config/config.go
index b971671b92..22abcc97e5 100644
--- a/internal/config/config.go
+++ b/internal/config/config.go
@@ -70,9 +70,11 @@ func RegisterConfigFile() error {
 
 func ResolveConfig() (*Config, error) {
 	// Collect allowed directories, so that paths in the config can be validated.
-	configDir := viperInstance.GetString(ConfigDirectoriesKey)
+	directories := viperInstance.GetStringSlice(AllowedDirectoriesKey)
 	allowedDirs := make([]string, 0)
-	for _, dir := range strings.Split(configDir, ":") {
+
+	// Check directories in allowed_directories are valid
+	for _, dir := range directories {
 		if dir != "" && filepath.IsAbs(dir) {
 			allowedDirs = append(allowedDirs, dir)
 		} else {
@@ -96,7 +98,6 @@ func ResolveConfig() (*Config, error) {
 		Log:                resolveLog(),
 		DataPlaneConfig:    resolveDataPlaneConfig(),
 		Client:             resolveClient(),
-		ConfigDir:          configDir,
 		AllowedDirectories: allowedDirs,
 		Collector:          collector,
 		Command:            resolveCommand(),
@@ -147,18 +148,16 @@ 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.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",
 	)
 
 	fs.Duration(ClientTimeoutKey, time.Minute, "Client timeout")
-	fs.String(ConfigDirectoriesKey,
-		DefConfigDirectories,
-		"Defines the paths that you want to grant NGINX Agent read/write access to."+
-			" This key is formatted as a string and follows Unix PATH format")
+	fs.StringSlice(AllowedDirectoriesKey,
+		DefaultAllowedDirectories(),
+		"A comma-separated list of paths that you want to grant NGINX Agent read/write access to")
 
 	fs.String(
 		CommandServerHostKey,
@@ -239,7 +238,7 @@ func registerFlags() {
 
 	fs.StringSlice(
 		FeaturesKey,
-		GetDefaultFeatures(),
+		DefaultFeatures(),
 		"A comma-separated list of features enabled for the agent.",
 	)
 
@@ -358,7 +357,7 @@ func resolveDataPlaneConfig() *DataPlaneConfig {
 		Nginx: &NginxDataPlaneConfig{
 			ReloadMonitoringPeriod: viperInstance.GetDuration(NginxReloadMonitoringPeriodKey),
 			TreatWarningsAsErrors:  viperInstance.GetBool(NginxTreatWarningsAsErrorsKey),
-			ExcludeLogs:            viperInstance.GetString(NginxExcludeLogsKey),
+			ExcludeLogs:            viperInstance.GetStringSlice(NginxExcludeLogsKey),
 		},
 	}
 }
diff --git a/internal/config/config_test.go b/internal/config/config_test.go
index b907f15bf6..b49609c1a4 100644
--- a/internal/config/config_test.go
+++ b/internal/config/config_test.go
@@ -63,7 +63,8 @@ func TestResolveConfig(t *testing.T) {
 
 	assert.Equal(t, 30*time.Second, actual.DataPlaneConfig.Nginx.ReloadMonitoringPeriod)
 	assert.False(t, actual.DataPlaneConfig.Nginx.TreatWarningsAsErrors)
-	assert.Equal(t, "/var/log/nginx/error.log:/var/log/nginx/access.log", actual.DataPlaneConfig.Nginx.ExcludeLogs)
+	assert.Equal(t, []string{"/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)
@@ -75,8 +76,8 @@ func TestResolveConfig(t *testing.T) {
 	assert.Equal(t, 10*time.Second, actual.Client.Timeout)
 
 	assert.Equal(t,
-		"/etc/nginx:/usr/local/etc/nginx:/var/run/nginx:/usr/share/nginx/modules:/var/log/nginx:invalid/path",
-		actual.ConfigDir,
+		allowedDir,
+		actual.AllowedDirectories,
 	)
 
 	assert.Equal(t, allowedDir, actual.AllowedDirectories)
@@ -298,7 +299,6 @@ func getAgentConfig() *Config {
 			MaxMessageRecieveSize: 20,
 			MaxMessageSendSize:    40,
 		},
-		ConfigDir: "",
 		AllowedDirectories: []string{
 			"/etc/nginx", "/usr/local/etc/nginx", "/var/run/nginx", "/var/log/nginx", "/usr/share/nginx/modules",
 		},
diff --git a/internal/config/defaults.go b/internal/config/defaults.go
index 6c8ed93610..733645ed84 100644
--- a/internal/config/defaults.go
+++ b/internal/config/defaults.go
@@ -14,12 +14,11 @@ import (
 const (
 	DefGracefulShutdownPeriod      = 5 * time.Second
 	DefNginxReloadMonitoringPeriod = 10 * time.Second
-	DefTreatErrorsAsWarnings       = true
+	DefTreatErrorsAsWarnings       = false
 
 	DefCollectorConfigPath  = "/etc/nginx-agent/opentelemetry-collector-agent.yaml"
 	DefCollectorLogLevel    = "INFO"
 	DefCollectorLogPath     = "/var/log/nginx-agent/opentelemetry-collector-agent.log"
-	DefConfigDirectories    = "/etc/nginx:/usr/local/etc/nginx:/usr/share/nginx/modules"
 	DefCollectorTLSCertPath = "/var/lib/nginx-agent/cert.pem"
 	DefCollectorTLSKeyPath  = "/var/lib/nginx-agent/key.pem"
 	DefCollectorTLSCAPath   = "/var/lib/nginx-agent/ca.pem"
@@ -57,7 +56,7 @@ const (
 	DefCollectorBatchProcessorTimeout          = 200 * time.Millisecond
 )
 
-func GetDefaultFeatures() []string {
+func DefaultFeatures() []string {
 	return []string{
 		pkg.FeatureConfiguration,
 		pkg.FeatureConnection,
@@ -65,3 +64,13 @@ func GetDefaultFeatures() []string {
 		pkg.FeatureFileWatcher,
 	}
 }
+
+func DefaultAllowedDirectories() []string {
+	return []string{
+		"/etc/nginx",
+		"/usr/local/etc/nginx",
+		"/usr/share/nginx/modules",
+		"/var/run/nginx",
+		"/var/log/nginx",
+	}
+}
diff --git a/internal/config/flags.go b/internal/config/flags.go
index 4cbc6ca5ce..b8c0800a30 100644
--- a/internal/config/flags.go
+++ b/internal/config/flags.go
@@ -10,7 +10,7 @@ import (
 
 const (
 	ClientRootKey                               = "client"
-	ConfigDirectoriesKey                        = "config_dirs"
+	AllowedDirectoriesKey                       = "allowed_directories"
 	ConfigPathKey                               = "path"
 	CommandRootKey                              = "command"
 	DataPlaneConfigRootKey                      = "data_plane_config"
diff --git a/internal/config/testdata/nginx-agent.conf b/internal/config/testdata/nginx-agent.conf
index 398d75d9cf..70fa0aa3cb 100644
--- a/internal/config/testdata/nginx-agent.conf
+++ b/internal/config/testdata/nginx-agent.conf
@@ -14,7 +14,9 @@ data_plane_config:
     nginx:
         reload_monitoring_period: 30s
         treat_warnings_as_errors: false
-        exclude_logs: "/var/log/nginx/error.log:/var/log/nginx/access.log"
+        exclude_logs: 
+            - /var/log/nginx/error.log
+            - /var/log/nginx/access.log
 client: 
   timeout: 10s
 
@@ -61,4 +63,9 @@ collector:
         host: "127.0.0.1"
         port: 1234
 
-config_dirs: "/etc/nginx:/usr/local/etc/nginx:/var/run/nginx:/usr/share/nginx/modules:/var/log/nginx:invalid/path"
+allowed_directories:
+    - /etc/nginx
+    - /usr/local/etc/nginx
+    - /var/run/nginx
+    - /usr/share/nginx/modules
+    - /var/log/nginx
diff --git a/internal/config/types.go b/internal/config/types.go
index a9d71fd98e..d40eec22b9 100644
--- a/internal/config/types.go
+++ b/internal/config/types.go
@@ -42,9 +42,8 @@ type (
 		Watchers           *Watchers        `yaml:"-"`
 		Version            string           `yaml:"-"`
 		Path               string           `yaml:"-"`
-		ConfigDir          string           `yaml:"-" mapstructure:"config-dirs"`
 		UUID               string           `yaml:"-"`
-		AllowedDirectories []string         `yaml:"-"`
+		AllowedDirectories []string         `yaml:"-" mapstructure:"allowed_directories"`
 		Features           []string         `yaml:"-"`
 	}
 
@@ -58,7 +57,7 @@ type (
 	}
 
 	NginxDataPlaneConfig struct {
-		ExcludeLogs            string        `yaml:"-" mapstructure:"exclude_logs"`
+		ExcludeLogs            []string      `yaml:"-" mapstructure:"exclude_logs"`
 		ReloadMonitoringPeriod time.Duration `yaml:"-" mapstructure:"reload_monitoring_period"`
 		TreatWarningsAsErrors  bool          `yaml:"-" mapstructure:"treat_warnings_as_errors"`
 	}
diff --git a/internal/watcher/instance/nginx_config_parser.go b/internal/watcher/instance/nginx_config_parser.go
index 966d34f9d1..ab147e2bd5 100644
--- a/internal/watcher/instance/nginx_config_parser.go
+++ b/internal/watcher/instance/nginx_config_parser.go
@@ -160,7 +160,7 @@ func (ncp *NginxConfigParser) ignoreLog(logPath string) bool {
 		return true
 	}
 
-	for _, path := range strings.Split(ncp.agentConfig.DataPlaneConfig.Nginx.ExcludeLogs, ":") {
+	for _, path := range ncp.agentConfig.DataPlaneConfig.Nginx.ExcludeLogs {
 		ok, err := filepath.Match(path, logPath)
 		if err != nil {
 			slog.Error("Invalid path for excluding log", "log_path", path)
diff --git a/internal/watcher/instance/nginx_config_parser_test.go b/internal/watcher/instance/nginx_config_parser_test.go
index 6f8c22dc18..e4c493e530 100644
--- a/internal/watcher/instance/nginx_config_parser_test.go
+++ b/internal/watcher/instance/nginx_config_parser_test.go
@@ -664,70 +664,70 @@ func TestNginxConfigParser_ignoreLog(t *testing.T) {
 	tests := []struct {
 		name        string
 		logPath     string
-		excludeLogs string
 		expectedLog string
+		excludeLogs []string
 		expected    bool
 	}{
 		{
 			name:        "Test 1: allowed log path",
 			logPath:     "/tmp/var/log/nginx/access.log",
-			excludeLogs: "",
+			excludeLogs: []string{},
 			expected:    false,
 			expectedLog: "",
 		},
 		{
 			name:        "Test 2: syslog",
 			logPath:     "syslog:server=unix:/var/log/nginx.sock,nohostname;",
-			excludeLogs: "",
+			excludeLogs: []string{},
 			expected:    true,
 			expectedLog: "",
 		},
 		{
 			name:        "Test 3: log off",
 			logPath:     "off",
-			excludeLogs: "",
+			excludeLogs: []string{},
 			expected:    true,
 			expectedLog: "",
 		},
 		{
 			name:        "Test 4: log /dev/stderr",
 			logPath:     "/dev/stderr",
-			excludeLogs: "",
+			excludeLogs: []string{},
 			expected:    true,
 			expectedLog: "",
 		},
 		{
 			name:        "Test 5: log /dev/stdout",
 			logPath:     "/dev/stdout",
-			excludeLogs: "",
+			excludeLogs: []string{},
 			expected:    true,
 			expectedLog: "",
 		},
 		{
 			name:        "Test 6: log /dev/null",
 			logPath:     "/dev/null",
-			excludeLogs: "",
+			excludeLogs: []string{},
 			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]*",
+			excludeLogs: []string{"/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]*",
+			excludeLogs: []string{"/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]*",
+			excludeLogs: []string{"/var/log/nginx/[^ace]*", "/var/log/nginx/a[^c]*"},
 			expected:    false,
 			expectedLog: "Log being read is outside of allowed directories",
 		},
diff --git a/nginx-agent.conf b/nginx-agent.conf
index 2109917eb6..a90be6a9eb 100644
--- a/nginx-agent.conf
+++ b/nginx-agent.conf
@@ -10,7 +10,12 @@ log:
   # set log path. if empty, don't log to file.
   path: /var/log/nginx-agent/
 
-config_dirs: "/etc/nginx:/usr/local/etc/nginx:/usr/share/nginx/modules:/var/run/nginx:/var/log/nginx"
+allowed_directories: 
+    - /etc/nginx
+    - /usr/local/etc/nginx
+    - /usr/share/nginx/modules
+    - /var/run/nginx
+    - /var/log/nginx
 
 ## command server settings 
 # command:
@@ -28,4 +33,4 @@ config_dirs: "/etc/nginx:/usr/local/etc/nginx:/usr/share/nginx/modules:/var/run/
 #      server:
 #        host: "127.0.0.1"         # OTLP exporter server host
 #        port: 5643                # OTLP exporter server port
-#      tls: {}
\ No newline at end of file
+#      tls: {}
diff --git a/scripts/packages/postinstall.sh b/scripts/packages/postinstall.sh
index c44686b32f..95dcb38545 100755
--- a/scripts/packages/postinstall.sh
+++ b/scripts/packages/postinstall.sh
@@ -239,8 +239,11 @@ metrics:
 
 # OSS NGINX default config path
 # path to aux file dirs can also be added
-config_dirs: "/etc/nginx:/usr/local/etc/nginx:/usr/share/nginx/modules:/etc/nms"
-
+allowed_directories: 
+    - /etc/nginx
+    - /usr/local/etc/nginx
+    - /usr/share/nginx/modules
+    - /etc/nms
 api:
   # default port for NGINX Agent API, this is for the server configuration of the REST API
   port: 8081
diff --git a/test/config/agent/nginx-agent-otel-load.conf b/test/config/agent/nginx-agent-otel-load.conf
index 0b8ff73178..8154721024 100644
--- a/test/config/agent/nginx-agent-otel-load.conf
+++ b/test/config/agent/nginx-agent-otel-load.conf
@@ -10,7 +10,11 @@ log:
   # set log path. if empty, don't log to file.
   path: /var/log/nginx-agent/
 
-config_dirs: "/etc/nginx:/usr/local/etc/nginx:/usr/share/nginx/modules:/var/run/nginx"
+allowed_directories: 
+    - /etc/nginx
+    - /usr/local/etc/nginx
+    - /usr/share/nginx/modules
+    - /var/run/nginx
 
 client:
   timeout: 10s
diff --git a/test/config/agent/nginx-config-with-grpc-client.conf b/test/config/agent/nginx-config-with-grpc-client.conf
index 70ce2f7ab7..11319ea6d5 100644
--- a/test/config/agent/nginx-config-with-grpc-client.conf
+++ b/test/config/agent/nginx-config-with-grpc-client.conf
@@ -16,7 +16,10 @@ command:
 process_monitor:
   monitoring_frequency: 30s
 
-config_dirs: "/etc/nginx:/usr/local/etc/nginx:/usr/share/nginx/modules"
+allowed_directories: 
+    - /etc/nginx
+    - /usr/local/etc/nginx
+    - /usr/share/nginx/modules
 
 client:
   timeout: 10s
diff --git a/test/mock/collector/nginx-agent.conf b/test/mock/collector/nginx-agent.conf
index eadeece887..d19bcfc7af 100644
--- a/test/mock/collector/nginx-agent.conf
+++ b/test/mock/collector/nginx-agent.conf
@@ -21,7 +21,11 @@ data_plane_config:
     reload_monitoring_period: 5s
     treat_warnings_as_errors: true
 
-config_dirs: "/etc/nginx:/usr/local/etc/nginx:/usr/share/nginx/modules:/var/run/nginx"
+allowed_directories: 
+    - /etc/nginx
+    - /usr/local/etc/nginx
+    - /usr/share/nginx/modules
+    - /var/run/nginx
 
 client:
   timeout: 10s
diff --git a/test/types/config.go b/test/types/config.go
index 424f483ab3..f3abd391a4 100644
--- a/test/types/config.go
+++ b/test/types/config.go
@@ -45,7 +45,6 @@ func AgentConfig() *config.Config {
 			Time:                clientTime,
 			PermitWithoutStream: clientPermitWithoutStream,
 		},
-		ConfigDir:          "",
 		AllowedDirectories: []string{"/tmp/"},
 		Collector: &config.Collector{
 			ConfigPath: "/etc/nginx-agent/nginx-agent-otelcol.yaml",
@@ -126,7 +125,7 @@ func AgentConfig() *config.Config {
 			Nginx: &config.NginxDataPlaneConfig{
 				TreatWarningsAsErrors:  true,
 				ReloadMonitoringPeriod: reloadMonitoringPeriod,
-				ExcludeLogs:            "",
+				ExcludeLogs:            []string{},
 			},
 		},
 		Watchers: &config.Watchers{