allow non-standard listen ports

[seletskiy]

PR introduces two new annotations:

• nginx.org/listen-ports
• nginx.org/listen-ports-ssl

When using ingress controller with hostNetwork: true it is possible to allow
to have non-standard ports (not 80 and 443) served by nginx. It's useful
for dynamic ports in ingress/port-based routing.

Fixes #98.

[pleshakov]

@seletskiy Thanks!

A few suggestions:

• Documentation: I think it is better to document those annotation in the table here, than having an example with yaml files. Please remove those yaml files. I can add documentation later after the PR is merged.
• It is important to redirect to a different SSL port here, while for this case it is ok -- that one is for the case when NGINX is sitting behind a load balancer and SSL termination is done by that load balancer.
• It is better to add Ports and SSLPorts to Config and parse annotations in createConfig

[seletskiy]

@pleshakov:

Documentation: I think it is better to document those annotation in the table here, than having an example with yaml files. Please remove those yaml files. I can add documentation later after the PR is merged.

Done.

It is important to redirect to a different SSL port here, while for this case it is ok -- that one is for the case when NGINX is sitting behind a load balancer and SSL termination is done by that load balancer.

Done: f1b118b#diff-394b0af448cb4ab4c28989f3aecae208L31

It is better to add Ports and SSLPorts to Config and parse annotations in createConfig

This will not work, because Config is not passed to the nginx.ingress.tmpl, so we can't extract ports from it here:

kubernetes-ingress/nginx-controller/nginx/nginx.go

Lines 151 to 154 in 6065c9e

	func (nginx *NginxController) AddOrUpdateIngress(name string, config IngressNginxConfig) {
	glog.V(3).Infof("Updating NGINX configuration")
	filename := nginx.getIngressNginxConfigFileName(name)
	nginx.templateIt(config, filename)

[pleshakov]

@seletskiy
Thanks. I've added additional suggestions.

This will not work, because Config is not passed to the nginx.ingress.tmpl, so we can't extract ports from it here:

It be great if we could have ports and ssl ports defined in the Config, with default values set to [80] and [443]. When parsing the annotations in createConfig , we could overwrite those default values based on the annotations values, but always making sure that there is at least one port for regular and ssl ports.

[pleshakov]

1. len(sslPorts) > 1 leads to a bug, as it only enables SSL termination when we have at least 2 SSL ports.
2. this check is also unnecessary: we should always have at least 1 SSL port if SSL is enabled (if pemFile, ok := pems[serverName];)

[seletskiy]

Yes, you're right, sorry.

[seletskiy]

That check is simple a safe guard for case when annotation specifies empty value.

[pleshakov]

This redirect is for the case when SSL termination is done at a load balancer in front of NGINX. It can be enabled for Ingress resources without SSL termination. Thus, it is a bug and the config generation will fail.

[seletskiy]

Reverted.

[pleshakov]

what is this for?

[seletskiy]

It's to ignore nginx-controller binary which appears when you run go build inside nginx-controller directory.

[seletskiy]

It be great if we could have ports and ssl ports defined in the Config, with default values set to [80] and [443]. When parsing the annotations in createConfig , we could overwrite those default values based on the annotations values, but always making sure that there is at least one port for regular and ssl ports.

OK, I've moved default values to Config struct. Check it out now.

[pleshakov]

@seletskiy Thanks so much!
There is one thing left. This check is unnecessary, as we always make sure that there is at least one element in ingCfg.SSLPorts.

[seletskiy]

@pleshakov: I've removed excessive if, rebased and resolved conflicts against new commits in upstream. Check it out again.

[pleshakov]

@seletskiy Thanks!!

[seletskiy]

@pleshakov: Thanks for assistance. Do you plan to release new version of image on the Docker Hub?

[pleshakov]

@seletskiy yes, we will release a new version today

[CodeSwimBikeRunner]

can anyone provide a working example?

[pleshakov]

@ChristopherLClark here is an example. instead of 80 and 443, NGINX will listen on 9080 and 9443

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: cafe-ingress
  annotations:
     kubernetes.io/ingress.class: "nginx"
     nginx.org/listen-ports: "9080"
     nginx.org/listen-ports-ssl: "9443"
spec:
. . .

Also see https://github.com/nginxinc/kubernetes-ingress/blob/master/docs/configmap-and-annotations.md

[CodeSwimBikeRunner]

@pleshakov Hmm, this still doesnt quite meet my needs. I need to have kubernetes, in the spec, rules, portion be forwarding routes with ports.

So for example:

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: ingress-iot-admin
  annotations: 
    kubernetes.io/ingress.class: nginx
    nginx.org/listen-ports: "30000"
  labels:
    app: iot-admin
spec:
  rules:
  - host: localhost:30000
    http:
      paths:
      - path: /
        backend:
          serviceName: service-iot-admin
          servicePort: 8080

In local development we can't use dns entries. So I would like to mimic the production cluster with localhost:30000 as the domain and then all requests go through that.

[pleshakov]

@ChristopherLClark that should work OK, if you change localhost:30000 to localhost in your Ingress

[CodeSwimBikeRunner]

@pleshakov yes I ultimately ended up running kubernetes under port 80, and killed my other processes, but I was looking for a way where I could run kubernetes under a different port. This ended up being easier just switching to 80.