Use v3/dev/performance of ModSecurity because of performance

[Stono]

Fixes: #1995
fixes #1958

[k8s-reviewable]

This change is 

[coveralls]

Coverage increased (+0.2%) to 39.742% when pulling c1b7880 on Stono:master into b020686 on kubernetes:master.

[Stono]

Hey @aledbf - any chance of merging this?

[Stono]

In fact, do NOT merge this. Just found it doesn't work :'(

-------------------------------------------------------------------------------
W0129 20:14:16.662979       7 queue.go:113] requeuing default/at-consumer-platform-service, err
-------------------------------------------------------------------------------
Error: exit status 1
2018/01/29 20:14:16 [emerg] 84#84: "modsecurity_rules_file" directive Rules error. File: /etc/nginx/owasp-modsecurity-crs/rules/REQUEST-920-PROTOCOL-ENFORCEMENT.conf. Line: 169. Column: 390. Invalid variable:  MULTIPART_SEMICOLON_MISSING},\ in /tmp/nginx-cfg735831326:214
nginx: [emerg] "modsecurity_rules_file" directive Rules error. File: /etc/nginx/owasp-modsecurity-crs/rules/REQUEST-920-PROTOCOL-ENFORCEMENT.conf. Line: 169. Column: 390. Invalid variable:  MULTIPART_SEMICOLON_MISSING},\ in /tmp/nginx-cfg735831326:214
nginx: configuration file /tmp/nginx-cfg735831326 test failed

ModSecurity is turning out to be a bloody nightmare.

[aledbf]

@Stono I need to test manually because the tests do not cover the nginx docker image. At the end of the day, I will test this and the brotli rollback publishing an image in quay repo for testing.

[aledbf]

@Stono merging this does not update the image the nginx ingress controller will use.

[Stono]

@aledbf yeah that's me testing locally, and rebuilding the nginx base image - as you can see it doesn't work :'( so i wouldn't bother with this PR for now. Sorry.

[Stono]

@aledbf I cracked it!
I found very specific versions of modsecurity 3 and the core rule set without the performance degradation.

I have a fully enabled WAF now which isn't user impacting! I'd really suggest testing and merging this quickly as the current build of modsecurity is crippling.

[Stono]

/assign @aledbf

[pieterlange]

If mod_security stable doesn't perform the solution is not to use their development branch. The maintenance overhead for this project will simply too large currently.

[Stono]

@pieterlange i see your point however modsecurity with ingress-nginx in its current state is unusable. I would event suggest you rolling back to a main release <= 3.0.0 of modsecurity than stay where you are.

And also as it stood each build of the image was just pulling in the latest master anyway git clone --depth 1 -b v3/master --single-branch https://github.com/SpiderLabs/ModSecurity. So you weren't exactly locking down to a specific release and could introduce regression without knowing.

[aledbf]

@Stono please test quay.io/aledbf/nginx-ingress-controller:0.322

[Stono]

sure, can do. What changes have you made in that build (so i know what i'm looking for?)

[aledbf]

@Stono contains this PR + the brotli version rollback #1958
I'm trying to fix 2 issues with this PR. If the brotli fix works I will ask you to make two changes to this PR so we can publish a new version of the nginx image

[Stono]

@aledbf no problem, leave it with me and i'll get back to you shortly.

[Stono]

Hi dude,
It doesn't even start on my cluster, I get a strange error to do with a log for a pod that doesn't exist.

❯ kn get pods
NAME                                                 READY     STATUS             RESTARTS   AGE
default-http-backend-ccf9cd97d-d6htj                 1/1       Running            0          5h
ingress-nginx-external-controller-8c5c5d7bd-b9hx6    2/3       CrashLoopBackOff   2          1m

❯ kn logs ingress-nginx-external-controller-8c5c5d7bd-b9hx6 nginx-ingress-controller
failed to open log file "/var/log/pods/6bf8ceea-06b3-11e8-8781-42010a8401f8/nginx-ingress-controller_1.log": open /var/log/pods/6bf8ceea-06b3-11e8-8781-42010a8401f8/nginx-ingress-controller_1.log: no such file or directory%

[Stono]

Update:
I think theres something up with that image, this is me running 0.3.1 on my host:

❯ docker run --rm -it quay.io/aledbf/nginx-ingress-controller:0.321
-------------------------------------------------------------------------------
NGINX Ingress controller
  Release:    0.10.1
  Build:      git-5dc261dd
  Repository: https://github.com/aledbf/ingress
-------------------------------------------------------------------------------

F0131 18:28:56.609626       7 main.go:59] Please specify --default-backend-service
/Users/karl.stoney/git/autotrader/charts/helm-cmp-signals-platform git/master

And this is me running 0.3.2:

❯ docker run --rm -it quay.io/aledbf/nginx-ingress-controller:0.322
^C%

0.3.2 just hangs and i have to SIGKILL it

[aledbf]

@Stono sorry about that. Use quay.io/aledbf/nginx-ingress-controller:0.323

[Stono]

@aledbf that worked a treat, i can confirm modsec is working beautifully without any performance degradation :-)

[aledbf]

@Stono lets wait for the feedback on the brotli issue until the end of the day, if there's no feedback I will just merge this PR
Thanks for fixing this issue.

[Stono]

Sure thing, and no problem. We're going to be making very heavy use of this ingress controller so you'll be hearing a lot more from me!

[aledbf]

@Stono this is the diff:

diff --git a/Makefile b/Makefile
index fa895736..617b4f13 100644
--- a/Makefile
+++ b/Makefile
@@ -50,7 +50,7 @@ IMAGE = $(REGISTRY)/$(IMGNAME)
 MULTI_ARCH_IMG = $(IMAGE)-$(ARCH)
 
 # Set default base image dynamically for each arch
-BASEIMAGE?=quay.io/kubernetes-ingress-controller/nginx-$(ARCH):0.32
+BASEIMAGE?=quay.io/kubernetes-ingress-controller/nginx-$(ARCH):0.33
 
 ifeq ($(ARCH),arm)
        QEMUARCH=arm
diff --git a/images/nginx/Makefile b/images/nginx/Makefile
index e6f2f2f8..15c39ddf 100644
--- a/images/nginx/Makefile
+++ b/images/nginx/Makefile
@@ -13,7 +13,7 @@
 # limitations under the License.
 
 # 0.0.0 shouldn't clobber any released builds
-TAG ?= 0.32
+TAG ?= 0.33
 REGISTRY ?= quay.io/kubernetes-ingress-controller
 ARCH ?= $(shell go env GOARCH)
 DOCKER ?= gcloud docker --
diff --git a/images/nginx/build.sh b/images/nginx/build.sh
index f4d44c06..4669079e 100755
--- a/images/nginx/build.sh
+++ b/images/nginx/build.sh
@@ -181,8 +181,8 @@ make install
 
 # Get Brotli source and deps
 cd "$BUILD_PATH"
-git clone --depth=1 https://github.com/eustas/ngx_brotli.git
-cd ngx_brotli 
+git clone --depth=1 https://github.com/google/ngx_brotli.git
+cd ngx_brotli
 git submodule init
 git submodule update

[Stono]

@aledbf just pushed, sorry for the delay!

[aledbf]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, Stono

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

• images/OWNERS [aledbf]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

[aledbf]

@Stono thanks!

[kinghrothgar]

Merging this broke building for me because as it bumped the TAG to a version that hasn't been uploaded yet for arm: manifest for quay.io/kubernetes-ingress-controller/nginx-arm:0.33 not found

NVM, I think this PR will actually fix the problem if someone pushes the image. #2019 is what broke it for me actually.

[aledbf]

@kinghrothgar I am checking why the image is not published