-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
reduce memory footprint and cpu usage when modsecurity and owasp rule… #4091
reduce memory footprint and cpu usage when modsecurity and owasp rule… #4091
Conversation
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
/retest |
@weltschraet: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@weltschraet please rebase |
@aledbf done |
…s are enabled globally
/ok-to-test |
Codecov Report
@@ Coverage Diff @@
## master #4091 +/- ##
=========================================
Coverage ? 57.68%
=========================================
Files ? 87
Lines ? 6450
Branches ? 0
=========================================
Hits ? 3721
Misses ? 2298
Partials ? 431 Continue to review full report at Codecov.
|
/lgtm |
@weltschraet thanks! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aledbf, weltschraet The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Hi, I think the
The controller keeps failing to reload the configuration. Controller log:
In the
My understanding is the Ingress controller failed to start since it tries to load the certificate, using a I would say disabling modsecurity with a NOTE: if I enable modsecurity, even with DetectionOnly mode, you can find the warning on SSL certificate load in the modsec audit file. |
…s are enabled globally
What this PR does / why we need it:
We have 72 ingress configurations on one cluster. The nginx ingress handles that fine, with a memory footprint of a few hundred MB. But when modsecurity and owasp rules are enabled that memory footprint increased to 3.5GB at startup and over 7GB on config reload. Additionally the CPU usage was way too high, we had long startup times and some of the times on a config reload the health checks failed.
After some research I found this issue at the ModSecurity-nginx repo. After reading it I thought maybe ModSecurity also keeps a set of rules for each location and this seems to be the case.
When enabling ModSecurity and OWASP rules globally the ingress-nginx applies the rules to each location. This pull request changes that and applies the rules to the http block.
Memory consumption went down from the numbers above back to a few hundred MB. Also the cpu usage went down noticeably.
It is still possible to apply the rules for each ingress resource separately.
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close that issue when PR gets merged):maybe fixes #4041
maybe fixes #3926
it is not clear if they have modsecurity enabled or not
Special notes for your reviewer: