Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add cli argument to configure NAP log level #2479

Merged
merged 2 commits into from
Mar 23, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 0 additions & 10 deletions build/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -219,16 +219,6 @@ RUN --mount=target=/tmp [ -n "${BUILD_OS##*plus*}" ] && exit 0; mkdir -p etc/ngi
RUN --mount=target=/tmp [ -n "${NAP_MODULES##*waf*}" ] && exit 0; mkdir -p /etc/nginx/waf/nac-policies /etc/nginx/waf/nac-logconfs /etc/nginx/waf/nac-usersigs /var/log/app_protect /opt/app_protect \
&& chown -R nginx:0 /etc/app_protect /usr/share/ts /var/log/app_protect/ /opt/app_protect/ /var/log/nginx/ \
&& touch /etc/nginx/waf/nac-usersigs/index.conf \
&& printf "%s\n" "MODULE = ALL;" "LOG_LEVEL = TS_CRIT;" "FILE = 2;" > /etc/app_protect/bd/logger.cfg \
&& printf "%s\n" "[config_set_compiler]" "log_level=fatal" >> /etc/app_protect/tools/asm_logging.conf \
&& for v in \
asm_config_server \
lock_factory \
bd_agent \
import_export_policy \
set_active \
; do sed -i "/\[$v/a log_level=fatal" "/etc/app_protect/tools/asm_logging.conf" \
; done \
&& cp -a /tmp/build/log-default.json /etc/nginx

# run only on nap dos build
Expand Down
33 changes: 32 additions & 1 deletion cmd/nginx-ingress/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,9 @@ var (

appProtect = flag.Bool("enable-app-protect", false, "Enable support for NGINX App Protect. Requires -nginx-plus.")

appProtectLogLevel = flag.String("app-protect-log-level", appProtectLogLevelDefault,
`Sets log level for App Protect. Allowed values: fatal, error, warn, info, debug, trace. Requires -nginx-plus and -enable-app-protect.`)

appProtectDos = flag.Bool("enable-app-protect-dos", false, "Enable support for NGINX App Protect dos. Requires -nginx-plus.")

appProtectDosDebug = flag.Bool("app-protect-dos-debug", false, "Enable debugging for App Protect Dos. Requires -nginx-plus and -enable-app-protect-dos.")
Expand Down Expand Up @@ -248,6 +251,17 @@ func main() {
glog.Fatal("NGINX App Protect support is for NGINX Plus only")
}

if *appProtectLogLevel != appProtectLogLevelDefault && !*appProtect && !*nginxPlus {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that DOS is implemented the same way, but I find it a bit redundant and I think it also increases the complexity of the code

I feel something like this would be better

if *appProtect {
	if *appProtectLogLevel != appProtectLogLevelDefault {
		...
	}
	// all the other future checks
}

but maybe we can refactor both later. Do you think it adds much value to check those other flags if AP it's not enabled, instead of just ignoring it?

Anyway at this point in the code you also already checked that it's nginxPlus so there's no need to check it again

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, frankly speaking there is not much of a value in checking all flags if AP is not enabled. But It also sounds reasonable to me to refactor later in order to have the same approach for AP and DOS.

glog.Fatal("app-protect-log-level support is for NGINX Plus only and App Protect is enable")
}

if *appProtectLogLevel != appProtectLogLevelDefault && *appProtect && *nginxPlus {
logLevelValidationError := validateAppProtectLogLevel(*appProtectLogLevel)
if logLevelValidationError != nil {
glog.Fatalf("Invalid value for app-protect-log-level: %v", *appProtectLogLevel)
}
}

if *appProtectDos && !*nginxPlus {
glog.Fatal("NGINX App Protect Dos support is for NGINX Plus only")
}
Expand Down Expand Up @@ -449,7 +463,7 @@ func main() {
aPPluginDone = make(chan error, 1)
aPAgentDone = make(chan error, 1)

nginxManager.AppProtectAgentStart(aPAgentDone, *nginxDebug)
nginxManager.AppProtectAgentStart(aPAgentDone, *appProtectLogLevel)
nginxManager.AppProtectPluginStart(aPPluginDone)
}

Expand Down Expand Up @@ -783,6 +797,23 @@ func validatePort(port int) error {
return nil
}

const appProtectLogLevelDefault = "fatal"

// validateAppProtectLogLevel makes sure a given logLevel is one of the allowed values
func validateAppProtectLogLevel(logLevel string) error {
switch strings.ToLower(logLevel) {
case
"fatal",
"error",
"warn",
"info",
"debug",
"trace":
return nil
}
return fmt.Errorf("invalid App Protect log level: %v", logLevel)
}

// parseNginxStatusAllowCIDRs converts a comma separated CIDR/IP address string into an array of CIDR/IP addresses.
// It returns an array of the valid CIDR/IP addresses or an error if given an invalid address.
func parseNginxStatusAllowCIDRs(input string) (cidrs []string, err error) {
Expand Down
30 changes: 30 additions & 0 deletions cmd/nginx-ingress/main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -123,3 +123,33 @@ func TestValidateLocation(t *testing.T) {
}
}
}

func TestValidateAppProtectLogLevel(t *testing.T) {
badLogLevels := []string{
"",
"critical",
"none",
"info;",
}
for _, badLogLevel := range badLogLevels {
err := validateAppProtectLogLevel(badLogLevel)
if err == nil {
t.Errorf("validateAppProtectLogLevel(%v) returned no error when it should have returned an error", badLogLevel)
}
}

goodLogLevels := []string{
"fatal",
"Error",
"WARN",
"info",
"debug",
"trace",
}
for _, goodLogLevel := range goodLogLevels {
err := validateAppProtectLogLevel(goodLogLevel)
if err != nil {
t.Errorf("validateAppProtectLogLevel(%v) returned an error when it should have returned no error: %v", goodLogLevel, err)
}
}
}
3 changes: 3 additions & 0 deletions deployments/helm-chart/templates/controller-daemonset.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,9 @@ spec:
- -nginx-plus={{ .Values.controller.nginxplus }}
- -nginx-reload-timeout={{ .Values.controller.nginxReloadTimeout }}
- -enable-app-protect={{ .Values.controller.appprotect.enable }}
{{- if and .Values.controller.appprotect.enable .Values.controller.appprotect.logLevel }}
- -app-protect-log-level={{ .Values.controller.appprotect.logLevel }}
{{ end }}
- -enable-app-protect-dos={{ .Values.controller.appprotectdos.enable }}
{{- if .Values.controller.appprotectdos.enable }}
- -app-protect-dos-debug={{ .Values.controller.appprotectdos.debug }}
Expand Down
3 changes: 3 additions & 0 deletions deployments/helm-chart/templates/controller-deployment.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,9 @@ spec:
- -nginx-plus={{ .Values.controller.nginxplus }}
- -nginx-reload-timeout={{ .Values.controller.nginxReloadTimeout }}
- -enable-app-protect={{ .Values.controller.appprotect.enable }}
{{- if and .Values.controller.appprotect.enable .Values.controller.appprotect.logLevel }}
- -app-protect-log-level={{ .Values.controller.appprotect.logLevel }}
{{ end }}
- -enable-app-protect-dos={{ .Values.controller.appprotectdos.enable }}
{{- if .Values.controller.appprotectdos.enable }}
- -app-protect-dos-debug={{ .Values.controller.appprotectdos.debug }}
Expand Down
2 changes: 2 additions & 0 deletions deployments/helm-chart/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ controller:
appprotect:
## Enable the App Protect module in the Ingress Controller.
enable: false
## Sets log level for App Protect. Allowed values: fatal, error, warn, info, debug, trace
# logLevel: fatal

## Support for App Protect Dos
appprotectdos:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,17 @@ Requires [-nginx-plus](#cmdoption-nginx-plus).

* If the argument is set, but `nginx-plus` is set to false, the Ingress Controller will fail to start.

 
<a name="cmdoption-app-protect-log-level"></a.>

### -app-protect-log-level `<string>`

Sets log level for App Protect. Allowed values: fatal, error, warn, info, debug, trace.

Requires [-nginx-plus](#cmdoption-nginx-plus) and [-enable-app-protect](#cmdoption-enable-app-protect).

* If the argument is set, but `nginx-plus` and `enable-app-protect` are set to false, the Ingress Controller will fail to start.

&nbsp;
<a name="cmdoption-enable-app-protect-dos"></a>

Expand Down
2 changes: 1 addition & 1 deletion internal/nginx/fake_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ func (*FakeManager) SetOpenTracing(_ bool) {
}

// AppProtectAgentStart is a fake implementation of AppProtectAgentStart
func (*FakeManager) AppProtectAgentStart(_ chan error, _ bool) {
func (*FakeManager) AppProtectAgentStart(_ chan error, _ string) {
glog.V(3).Infof("Starting FakeAppProtectAgent")
}

Expand Down
28 changes: 9 additions & 19 deletions internal/nginx/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,16 +32,11 @@ const (

appProtectPluginStartCmd = "/usr/share/ts/bin/bd-socket-plugin"
appProtectAgentStartCmd = "/opt/app_protect/bin/bd_agent"
appProtectLogLevelCmd = "/opt/app_protect/bin/set_log_level"

// appPluginParams is the configuration of App-Protect plugin
appPluginParams = "tmm_count 4 proc_cpuinfo_cpu_mhz 2000000 total_xml_memory 307200000 total_umu_max_size 3129344 sys_max_account_id 1024 no_static_config"

// appProtectDebugLogConfigFileContent holds the content of the file to be written when nginx debug is enabled. It will enable NGINX App Protect debug logs
appProtectDebugLogConfigFileContent = "MODULE = IO_PLUGIN;\nLOG_LEVEL = TS_INFO | TS_DEBUG;\nFILE = 2;\nMODULE = ECARD_POLICY;\nLOG_LEVEL = TS_INFO | TS_DEBUG;\nFILE = 2;\n"

// appProtectLogConfigFileName is the location of the NGINX App Protect logging configuration file
appProtectLogConfigFileName = "/etc/app_protect/bd/logger.cfg"

appProtectDosAgentInstallCmd = "/usr/bin/adminstall"
appProtectDosAgentStartCmd = "/usr/bin/admd -d --standalone"
appProtectDosAgentStartDebugCmd = "/usr/bin/admd -d --standalone --log debug"
Expand Down Expand Up @@ -81,7 +76,7 @@ type Manager interface {
UpdateServersInPlus(upstream string, servers []string, config ServerConfig) error
UpdateStreamServersInPlus(upstream string, servers []string) error
SetOpenTracing(openTracing bool)
AppProtectAgentStart(apaDone chan error, debug bool)
AppProtectAgentStart(apaDone chan error, logLevel string)
AppProtectAgentQuit()
AppProtectPluginStart(appDone chan error)
AppProtectPluginQuit()
Expand Down Expand Up @@ -461,20 +456,15 @@ func (lm *LocalManager) SetOpenTracing(openTracing bool) {
}

// AppProtectAgentStart starts the AppProtect agent
func (lm *LocalManager) AppProtectAgentStart(apaDone chan error, debug bool) {
if debug {
glog.V(3).Info("Starting AppProtect Agent in debug mode")
err := os.Remove(appProtectLogConfigFileName)
if err != nil {
glog.Fatalf("Failed removing App Protect Log configuration file")
}
err = createFileAndWrite(appProtectLogConfigFileName, []byte(appProtectDebugLogConfigFileContent))
if err != nil {
glog.Fatalf("Failed Writing App Protect Log configuration file")
}
func (lm *LocalManager) AppProtectAgentStart(apaDone chan error, logLevel string) {
glog.V(3).Info("Setting log level for App Protect - ", logLevel)
appProtectLogLevelCmdfull := fmt.Sprintf("%v %v", appProtectLogLevelCmd, logLevel)
logLevelCmd := exec.Command("sh", "-c", appProtectLogLevelCmdfull) // #nosec G204
if err := logLevelCmd.Run(); err != nil {
glog.Fatalf("Failed to set log level for AppProtect: %v", err)
}
glog.V(3).Info("Starting AppProtect Agent")

glog.V(3).Info("Starting AppProtect Agent")
cmd := exec.Command(appProtectAgentStartCmd)
if err := cmd.Start(); err != nil {
glog.Fatalf("Failed to start AppProtect Agent: %v", err)
Expand Down