Skip to content

Commit

Permalink
Pass on custom error log path at the time of validating config (#774)
Browse files Browse the repository at this point in the history
* add proto

* pass on custom error log path at the time of validating config

* add multi files custom error log path support

* feedback review

* add -c option only if config location
  • Loading branch information
achawla2012 authored Aug 20, 2024
1 parent f6ca97c commit ba2a6d0
Show file tree
Hide file tree
Showing 18 changed files with 654 additions and 321 deletions.
1 change: 1 addition & 0 deletions docs/proto/proto.md
Original file line number Diff line number Diff line change
Expand Up @@ -1439,6 +1439,7 @@ Represents NGINX details about a single NGINX instance
| ssl | [NginxSslMetaData](#f5-nginx-agent-sdk-NginxSslMetaData) | | NGINX SSL metadata. |
| status_url | [string](#string) | | Status URL. Example: http://localhost:8080/api |
| configure_args | [string](#string) | repeated | Command line arguments that were used when the NGINX instance was started. Example: [ "", "with-http_stub_status_module" ] |
| error_log_paths | [string](#string) | repeated | Error log path. Example: -e /home/ubuntu/nplus/var/log/error.log -e /home/ubuntu/nplus/var/log/error1.log |



Expand Down
207 changes: 133 additions & 74 deletions sdk/proto/nginx.pb.go

Large diffs are not rendered by default.

3 changes: 3 additions & 0 deletions sdk/proto/nginx.proto
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,9 @@ message NginxDetails {
// Command line arguments that were used when the NGINX instance was started.
// Example: [ "", "with-http_stub_status_module" ]
repeated string configure_args = 13 [(gogoproto.jsontag) = "configure_args"];
// Error log path.
// Example: -e /home/ubuntu/nplus/var/log/error.log -e /home/ubuntu/nplus/var/log/error1.log
repeated string error_log_paths = 14 [(gogoproto.jsontag) = "error_log_paths"];
}

// swagger:model NginxPlusMetaData
Expand Down
38 changes: 33 additions & 5 deletions src/core/nginx.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ type NginxBinary interface {
Start(nginxId, bin string) error
Stop(processId, bin string) error
Reload(processId, bin string) error
ValidateConfig(processId, bin, configLocation string, config *proto.NginxConfig, configApply *sdk.ConfigApply) error
ValidateConfig(processId, bin, configLocation string, errorLogPaths []string, config *proto.NginxConfig, configApply *sdk.ConfigApply) error
GetNginxDetailsFromProcess(nginxProcess *Process) *proto.NginxDetails
GetNginxDetailsByID(nginxID string) *proto.NginxDetails
GetNginxIDForProcess(nginxProcess *Process) string
Expand Down Expand Up @@ -209,6 +209,10 @@ func (n *NginxBinaryType) GetNginxDetailsFromProcess(nginxProcess *Process) *pro
log.Tracef("Custom conf path set: %v", path)
nginxDetailsFacade.ConfPath = path
}
if errorLogPaths := getErrorLogPathFromCommand(nginxProcess.Command); len(errorLogPaths) > 0 {
log.Tracef("Custom error log paths set: %v", errorLogPaths)
nginxDetailsFacade.ErrorLogPaths = errorLogPaths
}

n.statusUrlMutex.RLock()
urlsLength := len(n.statusUrls)
Expand Down Expand Up @@ -290,15 +294,25 @@ func (n *NginxBinaryType) Reload(processId, bin string) error {
}

// ValidateConfig tests the config with nginx -t -c configLocation.
func (n *NginxBinaryType) ValidateConfig(processId, bin, configLocation string, config *proto.NginxConfig, configApply *sdk.ConfigApply) error {
log.Debugf("Validating config, %s for nginx process, %s", configLocation, processId)
response, err := runCmd(bin, "-t", "-c", configLocation)
func (n *NginxBinaryType) ValidateConfig(processId, bin, configLocation string, errorLogPaths []string, config *proto.NginxConfig, configApply *sdk.ConfigApply) error {
log.Debugf("Validating config, %s for nginx process, %s, with custom error path %v", configLocation, processId, errorLogPaths)
cmdArgs := make([]string, 0)
cmdArgs = append(cmdArgs, "-t")
if len(configLocation) > 0 {
cmdArgs = append(cmdArgs, "-c")
cmdArgs = append(cmdArgs, configLocation)
}
for _, path := range errorLogPaths {
cmdArgs = append(cmdArgs, "-e")
cmdArgs = append(cmdArgs, path)
}
response, err := runCmd(bin, cmdArgs...)
if err != nil {
confFiles, auxFiles, getNginxConfigFilesErr := sdk.GetNginxConfigFiles(config)
if getNginxConfigFilesErr == nil {
n.writeBackup(config, confFiles, auxFiles)
}
return fmt.Errorf("error running nginx -t -c %v:\n%s", configLocation, response)
return fmt.Errorf("error running nginx %v:\n%s", strings.Join(cmdArgs, " "), response)
}

err = n.validateConfigCheckResponse(response, configLocation)
Expand Down Expand Up @@ -753,6 +767,20 @@ func getConfPathFromCommand(command string) string {
return ""
}

func getErrorLogPathFromCommand(command string) []string {
commands := strings.Split(command, " ")
errorPath := make([]string, 0)

for i, command := range commands {
if command == "-e" {
if i < len(commands)-1 {
errorPath = append(errorPath, commands[i+1])
}
}
}
return errorPath
}

func parseConfigureArguments(line string) (result map[string]interface{}, flags []string) {
// need to check for empty strings
flags = strings.Split(line[len("configure arguments:"):], " --")
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/nginx.go
Original file line number Diff line number Diff line change
Expand Up @@ -440,7 +440,7 @@ func (n *Nginx) writeConfigAndReloadNginx(correlationId string, config *proto.Ng
func (n *Nginx) validateConfig(nginx *proto.NginxDetails, correlationId string, config *proto.NginxConfig, configApply *sdk.ConfigApply) {
start := time.Now()

err := n.nginxBinary.ValidateConfig(nginx.NginxId, nginx.ProcessPath, nginx.ConfPath, config, configApply)
err := n.nginxBinary.ValidateConfig(nginx.NginxId, nginx.ProcessPath, nginx.ConfPath, nginx.ErrorLogPaths, config, configApply)
if err == nil {
_, err = n.nginxBinary.ReadConfig(nginx.GetConfPath(), config.GetConfigData().GetNginxId(), n.env.GetSystemUUID())
}
Expand Down
4 changes: 2 additions & 2 deletions src/plugins/nginx_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -541,7 +541,7 @@ func TestNginxConfigApply(t *testing.T) {

binary := tutils.NewMockNginxBinary()
binary.On("WriteConfig", mock.Anything).Return(config, nil)
binary.On("ValidateConfig", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil)
binary.On("ValidateConfig", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil)
binary.On("ReadConfig", mock.Anything, mock.Anything, mock.Anything).Return(test.config, nil)
binary.On("GetNginxDetailsByID", "12345").Return(tutils.GetDetailsMap()["12345"])
binary.On("UpdateNginxDetailsFromProcesses", env.Processes())
Expand Down Expand Up @@ -806,7 +806,7 @@ func TestNginx_validateConfig(t *testing.T) {
t.Run(test.name, func(tt *testing.T) {
env := tutils.GetMockEnvWithProcess()
binary := tutils.NewMockNginxBinary()
binary.On("ValidateConfig", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(test.validationResult)
binary.On("ValidateConfig", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(test.validationResult)
binary.On("ReadConfig", mock.Anything, mock.Anything, mock.Anything).Return(&proto.NginxConfig{}, nil)
binary.On("GetNginxDetailsMapFromProcesses", env.Processes()).Return(tutils.GetDetailsMap())
binary.On("UpdateNginxDetailsFromProcesses", env.Processes())
Expand Down
Loading

0 comments on commit ba2a6d0

Please sign in to comment.