-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Conversation
@galitskiy looks like there's a security error |
HI @lucacome . not sure ho to avoid this warning... prbl have a harcoded loglevel instead of using variable value directly (smth like if var=x, fullCmd = cmd +x) |
128bec1
to
c04e002
Compare
Codecov Report
@@ Coverage Diff @@
## master #2479 +/- ##
==========================================
+ Coverage 53.48% 53.66% +0.17%
==========================================
Files 49 48 -1
Lines 14306 14228 -78
==========================================
- Hits 7652 7635 -17
+ Misses 6411 6352 -59
+ Partials 243 241 -2
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
@@ -248,6 +251,17 @@ func main() { | |||
glog.Fatal("NGINX App Protect support is for NGINX Plus only") | |||
} | |||
|
|||
if *appProtectLogLevel != appProtectLogLevelDefault && !*appProtect && !*nginxPlus { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Proposed changes
implement NAP log level configuration via cli argument. Replacing manual preparing of logger configuration file (in Dockerfile and internally when using nginxdebug) with invoking perl tool (installed with app-protect).
Checklist
Before creating a PR, run through this checklist and mark each as complete.