search

kubernetes / ingress-nginx

Ingress-NGINX Controller for Kubernetes
https://kubernetes.github.io/ingress-nginx/
Apache License 2.0
17.26k stars 8.2k forks source link

curly brace in ingress path breaks NGINX template #3579

Closed ghouscht closed 5 years ago

ghouscht commented 5 years ago

https://github.com/kubernetes/ingress-nginx/issues/3155 describes the same issue, but is closed without a soultion. I hope it's ok to open a new issue.

Is this a BUG REPORT or FEATURE REQUEST? (choose one): BUG

NGINX Ingress controller version: 0.21.0

Kubernetes version (use kubectl version):

Client Version: version.Info{Major:"1", Minor:"12", GitVersion:"v1.12.3", GitCommit:"435f92c719f279a3a67808c80521ea17d5715c66", GitTreeState:"clean", BuildDate:"2018-11-26T12:57:14Z", GoVersion:"go1.10.4", Compiler:"gc", Platform:"linux/amd64"}
Server Version: version.Info{Major:"1", Minor:"12", GitVersion:"v1.12.3", GitCommit:"435f92c719f279a3a67808c80521ea17d5715c66", GitTreeState:"clean", BuildDate:"2018-11-26T12:46:57Z", GoVersion:"go1.10.4", Compiler:"gc", Platform:"linux/amd64"}

Environment:

What happened: Nginx config is broken if someone uses curly braces in the path directive, resulting in the following error:

-------------------------------------------------------------------------------
W1219 05:49:14.001609       9 queue.go:130] requeuing scratch-gostelit/testdata-ingress, err
-------------------------------------------------------------------------------
Error: exit status 1
2018/12/19 05:49:13 [notice] 1011#1011: ModSecurity-nginx v1.0.0
2018/12/19 05:49:13 [emerg] 1011#1011: unknown directive "1}/(.*)" in /tmp/nginx-cfg473894624:1117                                                                                              nginx: [emerg] unknown directive "1}/(.*)" in /tmp/nginx-cfg473894624:1117
nginx: configuration file /tmp/nginx-cfg473894624 test failed

-------------------------------------------------------------------------------

rendered config:

1117     location /testdata/rds/([a-zA-Z]*)/(t1){1}/(.*) {
   1
   2         set $namespace      "scratch-gostelit";
   3         set $ingress_name   "testdata-ingress";
   4         set $service_name   "remotedataservice-t1";
   5         set $service_port   "80";
   6     set $location_path  "/testdata/rds/([a-zA-Z]*)/(t1){1}/(.*)";

What you expected to happen: Not to break the ingress of a whole cluster simply by using curly braces in the path.

How to reproduce it (as minimally and precisely as possible): Apply the following ingress definition and watch the logs of your ingress:

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  annotations:
    kubernetes.io/ingress.class: nginx
    nginx.ingress.kubernetes.io/rewrite-target: /rds/$1/$2/$3
    nginx.ingress.kubernetes.io/use-regex: true
  name: testdata-ingress
spec:
  rules:
  - host: testdata.t1.test.local
    http:
      paths:
      - path: "/testdata/rds/([a-zA-Z]*)/(t1){1}/(.*)"
        backend:
          serviceName: remotedataservice-t1
          servicePort: 80
  tls:
  - hosts:
    - testdata.t1.test.local

Reloading of the ingress now fails forever (or at least until the created ingress is removed), which will sooner or later disturbe other things running in the same cluster.

Anything else we need to know: Putting single quotes around the location directive in the nginx template L1016 seems to solve the problem.
Nginx docs quote:

 If a regular expression includes the “}” or “;” characters, the whole expressions should be enclosed in single or double quotes.

I'm happy to help out with a PR here, for either putting quotes around the directives in the nginx template or to handle this somewhere in the code (maybe also with proper escaping of special chars to handle this kind of problems better).

zrdaley commented 5 years ago

This should have been fixed here: https://github.com/kubernetes/ingress-nginx/pull/3182 which was released in 0.20.0. Are you sure that you're running this version or later?

ghouscht commented 5 years ago

Thanks for the hint, I somehow missed that. Version should be 0.21.0, but now I'm a bit concerned if it really is. I'm out of the office now, will check again tomorrow.

ghouscht commented 5 years ago

We're definitly using version 0.21.0 of nginx-ingress-controller, see details below:

nginx-ingress-controller:
    Container ID:  docker://7c00cfcc65272672ddfe34521d48d19c81a65cb23cbd9a3f7f672d0b18987f33
    Image:         quay-docker-remote.repo.pnet.ch/kubernetes-ingress-controller/nginx-ingress-controller:0.21.0
    Image ID:      docker-pullable://quay-docker-remote.repo.pnet.ch/kubernetes-ingress-controller/nginx-ingress-controller@sha256:617076c3e3d4d0638a4927702530d8456bda64c67194f6daed272a59e93b992f

Image name is a bit confusing, it's just a simple pull-through our artifactory instance from quay.io, that's why the image name is different. Hash of the image matches the one from quay.io for version 0.21.0: sha256:617076c3e3d4d0638a4927702530d8456bda64c67194f6daed272a59e93b992f

I ran the reproduction on two different installations (one with version 0.20.0 and the other with version 0.21.0), the behaviour is the same as described in my first post. Looks like https://github.com/kubernetes/ingress-nginx/pull/3182 doesn't fix the problem. I'll have a look at the code and try to understand why this happens.

ghouscht commented 5 years ago

Looks like I did a small, but important mistake.... As per use-regex docs the annotation expects a string in this form:

nginx.ingress.kubernetes.io/use-regex: "true"

I used a bool in the following form:

nginx.ingress.kubernetes.io/use-regex: true

A simple kubectl get ingress -o yaml shows, that the annotation is lost if the second form is used. On first sight thats not really obvious for a normal user and can be missed easily.
If you're a bit familiar with the k8s api, this should be quite obvious (however I also missed it on first sight), because the api defines an annotation as map[string]string, which in turn explains that the annotation is lost if the value is not a string.

The only thing which is probably worth a discousion, is that a "normal" user with permissions to create an ingress, can possibly break the ingress of an entire cluster by simply putting a curly brace in the path field either

I can handle all those cases by writing a simple validating admission controller but probably it's worth to go the safe way and handle this for all users in the nginx ingress directly. I don't know if this is the "way to go" if so, please let me know, I'm happy to help out here with a PR and some opinions on how to do that, if not I'll close this issue. Thank you for your help.

ElvinEfendi commented 5 years ago

The only thing which is probably worth a discousion, is that a "normal" user with permissions to create an ingress, can possibly break the ingress of an entire cluster

@ghouscht this is a known issue and it sucks. I'll create a Github issue to make it explicit and discuss possible solutions. Here's another report of this: https://github.com/kubernetes/ingress-nginx/issues/3435

ghouscht commented 5 years ago

Sounds good. I'll close this issue.