Add HTTP2 as option in configmap and annotations

[thetechnick]

Hi Nginx Team,

I have added default http2 support to the ingress template.
Is there a reason, that the nginx ingress controller does not enable http2 for ssl connections by default?
The only obstacle I had, was that the environment in the debian container (openssl 1.0.1f) does not support http2. That is why I changed the base container to alpine. (wich should also solve #11)

The changed image is uploaded to: (Tag: 0.5.0)
https://quay.io/repository/nico_schieder/nginxinc-kubernetes-ingress

Please keep in mind that I could not test the nginx plus config.

[pleshakov]

I don't recommend enabling http/2 by default. It is a new protocol and some browsers still have problems fully implementing it, which leads to problems in some cases.

What we can do is to introduce an option to enable http/2. We can put such an option into ConfigMap and annotation.

Regarding the base image, we can switch from debian to ubuntu:16.04, which has openssl with support for ALPN required for Chrome users.
I don't know how many people are using alpine linux so that we can switch to it by default.

We can add support for http/2. Let me know if you're interested in helping.

[thetechnick]

Hi,
sure I am interested in helping!
I will make http2 configurable in the configmap and individually on each ingress object via annotations.

Does this look good to you, in terms of annotation naming?

annotations:
  nginx.org/http2: 'True'

# configmap
http2: 'True'

Edit: changed annotation name from "enable-http2" to only "http2"
Edit2: About alpine linux vs. debian/ubuntu:

• The base image contains as little as possible, which helps to keep the attack surface of such a container minimal.
• The image does already contain vi, cat, less and curl, so you are able to quickly debug things without having to install additional packages
• Smaller size safes bandwidth, speeds up updates and developer time
• Frequent updates of packages further increases the security

The only real downside of alpine IMHO, is that coreos/clair is not able to analyse it for vulnerabilities, because CVEs at alpine are not yet tracked in a format that clair can use.
Overall we (@domainfactory) try to use alpine as much as possible.

We also have some more pull requests incoming for:

• ipv6 binding
• log_format
• forward secrecy
• auth_request
• proxy_set_headers
  Do you prefer small pull requests per option/feature or do you prefer the pull requests to be larger?

[pleshakov]

@thetechnick thx for the pull request!

The naming of nginx.org/http2: 'True' looks good to me.

About alpine linux vs. debian/ubuntu:

The arguments you provided for alpine make a good case for alpine. However, I would stick to debian as a default and as an option provide the alpine based image, similar to what we're providing on DockerHub https://hub.docker.com/_/nginx/

We also have some more pull requests incoming for:
ipv6 binding
log_format
forward secrecy
auth_request
proxy_set_headers Do you prefer small pull requests per option/feature or do you prefer the pull requests to be larger?

That's awesome! Pull requests per option are preferable. If you have an idea how to implement a particular feature, I suggest we discuss it via a GitHub issue

Regarding this pull request:

• Since you're parsing the value of the annotation -- https://github.com/TheTechNick/kubernetes-ingress/blob/914fa992db02e5f353331a14bc287f225a5d1da7/nginx-controller/controller/controller.go#L326 -- it makes sense to report an error in case of a parsing error, similar to https://github.com/nginxinc/kubernetes-ingress/blob/master/nginx-controller/nginx/configurator.go#L198
• Another place where a server is created is here -- https://github.com/TheTechNick/kubernetes-ingress/blob/914fa992db02e5f353331a14bc287f225a5d1da7/nginx-controller/nginx/configurator.go#L147 Http2 must be added
• Let's stick to the debian base image, we will add an option to create an alpine base image.
• Please remove http2 from the nginx-plus ingress.tmpl.

[thetechnick]

The arguments you provided for alpine make a good case for alpine. However, I would stick to debian as a default and as an option provide the alpine based image, similar to what we're providing on DockerHub https://hub.docker.com/_/nginx/

Sounds great!

That's awesome! Pull requests per option are preferable. If you have an idea how to implement a particular feature, I suggest we discuss it via a GitHub issue

Watch out for some issues :)
Do I somehow interrupt your plans if I add unittests for some of the config mapping functions?
I would like to be sure new config options do work without having to test them on a kubernetes cluster.

[pleshakov]

Thx! Looks good to me!
Before I merge, could you squash all commits into a single commit?

Do I somehow interrupt your plans if I add unittests for some of the config mapping functions?
I would like to be sure new config options do work without having to test them on a kubernetes cluster.

That would be great if you add unittests for that!

[thetechnick]

I have squashed the commits into one and I have also added an entry to examples/customization/nginx-config.yaml, so the configmap entry is documented. Should I also create a examples/http2 folder with the annotations like ssl-services?

[pleshakov]

Thx! No need for the examples/http2 folder.

We'll make a new release (0.5.1) this Friday that will include this feature. We'll also create a new tag on our DockerHub to support the alpine distro.

[pleshakov]

We'll make a new release next week.