Add a validating webhook for ingress sanity check

[tjamet]

What this PR does / why we need it:

• Add a feature to reject the admission of ingresses that generates invalid configuration

Running nginx ingress controller in production for a while, we are pretty happy about it, but we occasionally have an issue where an ingress with a corrupted configuration is injected, and prevents from any other future update of the ingress nginx configuration.
Subsequently, we need to find out the ingress causing problems and fix it.
This PR increases the feedback loop by allowing, optionally, to reject the ingress, before it gets inserted and hence preserves the sanity of the configuration.

Once the patch is applied, and the controller configured, this is the user experience in case of an invalid ingress:

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #
fixes #2833
fixes #3218
fixes #3435
fixes #3459

Special notes for your reviewer:
I am aware that this PR needs testing and documentation.
In case the option of adding this feature is accepted, I will as well contribute documentation and tests.

[aledbf]

@tjamet first, thank you for working on this!
That said:

• please rebase
• to update the dependencies please run:
  • rm -rf images/grpc-fortune-teller/ (you can restore that before the commit)
  • make dep-ensure

[aledbf]

@tjamet as you said, we need tests for this :)
What's the behavior when you have multiple deployments with different classes?

[aledbf]

This is not required because ListIngresses alredy sorts the ingresses

[tjamet]

@aledbf thanks for the fast feedback
I am currently rebasing

Indeed I will then work on tests and documentation!
Regarding multiple deployments, the expected behavior is to accept ingresses that do not match the class. This said, I need further tests with multiple deployments

[aledbf]

Regarding multiple deployments, the expected behavior is to accept ingresses that do not [match the class](c07f553#diff-
86ac9ff9d75a0c5005c116e766a6127dR203).

Yes, I saw that but I am not sure about the webhook behavior with multiple deployments

[fejta-bot]

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

[ElvinEfendi]

Also fixes #3588

[ElvinEfendi]

@tjamet thanks for working on this! It'd be great if you describe your approach, discuss how much overhead this is adding to the core event loop, and why would I not want to have this always on.

[tjamet]

@aledbf I am seeing so far 2 parameters that can change the set of ingresses considered for configuration generation in a given controller:

• --ingress-class
• --watch-namespace
  I am hence suspecting multiple deployments to have 4 different ways to consider or not an ingress.

Following your comment, I added the filter based on the namespace name as well for to behave consistently with the deployment. This should handle all combinations of --ingress-class and --watch-namespace, as soon as the validating webhook is enabled for all deployments.

The approach @ElvinEfendi is pretty simple in the end, it consists of exposing, outside of the core event loop an HTTP handler, compliant with Kubernetes validating webhook interface, to ensure any ingress that is about to be inserted in the cluster would generate a syntactically valid configuration together with the rest of the known ingresses using the pre-existing features. This handler should be exposed through a service to a ValidatingWebhookConfiguration object.

As this handler is run outside of the core event loop, I have tested a benchmark that shows the following results (reported by go test -bench '.' -benchmem -run Benchmark -v within the ./bin/go-in-docker.sh):

number of pre existing ingresses	(go) memory used per request	time per validating request
0	136342 B/op	25926680 ns/op
20	637230 B/op	26366018 ns/op
50	1415960 B/op	38304690 ns/op
100	2587819 B/op	47551526 ns/op
200	5566906 B/op	71309565 ns/op
500	12790632 B/op	128302310 ns/op
1000	28805841 B/op	226466720 ns/op

Because the request is served through a service to using the ingress controller as a backend, webhook requests are load-balanced through all ingress controllers and hence reduces the load on a single pod in case multiple ingresses are inserted at the same time.

Another option to get better performances could be to drop the test with all pre-existing ingresses, which could probably be good enough to detect most configuration errors (syntax errors mainly).

Regarding the default enabling of the feature, I would tend to, as a system operator, to recommend to enable it systematically.
Although, it looks to me difficult to enable it by default directly after implementation. This would change the permissions the ingress controller would require to grant it system wide privileges to inject the ValidatingWebhookConfiguration object.
Also the way ValidatingWebhook works, they require a self signed certificate generated for the correct <service>.<namespace>.svc domain, which would be more difficult to integrate out of the box inside the ingress controller. My approach would rather be to make it easy in the chart to use such a feature to start with. Happy to read other opinions or concerns

[aledbf]

This should handle all combinations of --ingress-class and --watch-namespace, as soon as the validating webhook is enabled for all deployments.

👍

Regarding the default enabling of the feature, I would tend to, as a system operator, to recommend to enable it systematically.

This must be optional (at least for now)

Also the way ValidatingWebhook works, they require a self signed certificate generated for the correct ..svc domain, which would be more difficult to integrate out of the box inside the ingress controller. My approach would rather be to make it easy in the chart to use such a feature to start with. Happy to read other opinions or concerns

We can provide a script like https://gist.github.com/aledbf/11b3b4f882c632d021532edda8a5dc52 (I need to cleanup that) to use the feature already available in k8s to generate such certificate

[tjamet]

We can provide a script like https://gist.github.com/aledbf/11b3b4f882c632d021532edda8a5dc52 (I need to cleanup that) to use the feature already available in k8s to generate such certificate

Sure!
I can add it to this PR
We have it deployed using helm, using:

{{- $cn := printf "%s.%s.svc" ( include "nginx-ingress.validatingWebhook.fullname" . ) .Release.Namespace }}
{{- $ca := genCA (printf "%s-ca" ( include "nginx-ingress.validatingWebhook.fullname" . )) .Values.validatingWebhook.certificateValidity -}}
{{- $cert := genSignedCert $cn nil nil .Values.validatingWebhook.certificateValidity $ca -}}

This could also be integrated afterwards to the helm chart, that I could contribute to as well

Regarding impact on resources, here are graphs, with the deployment in the middle.
The previous version was 0.19.0

[zq-david-wang]

Just a silly question, would this validating webhook reject ingress resource that are valid for ingress controller of other kind? For example, the regular expression syntax for traefik is weird and would cause nginx ingress controller crash, if I deploy nginx ingress controller with this validating webhook, can I use traefik within the same cluster? (To be honest, the metrics endpoint for traefik is way more useful than nginx's)

[tjamet]

Hi @zq-david-wang

would this validating webhook reject ingress resource that are valid for ingress controller of other kind

The validating webhook is filtering on the kubernetes.io/ingress.class, ingress resources that match a different class will be accepted.
In case you run multiple ingress controllers (we do run several nginx ingress controllers) in a cluster, I would expect to find controllers deployed with different classes, hence ingresses for different ingresses kind (traefik in the case you are talking about) will be accepted.

In case both ingresses are run with the same class, both ingress controllers would generate a configuration for the ingress and hence the validating webhook will check the validity of the generated configuration

[tjamet]

Hi,
I think I addressed the concerns, are there any other one I need to address? Or is it good enough for a first version?
I will be happy to keep having a look at it and improve it with other ideas later on

[aledbf]

@tjamet please rebase and squash the commits

[tjamet]

Rebased and commits squashed

[aledbf]

@tjamet please rebase. After that, I plan to build a dev image to test and merge this PR

[ElvinEfendi]

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ElvinEfendi, tjamet

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:

• OWNERS [ElvinEfendi]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[ElvinEfendi]

Thanks @tjamet!

[towolf]

This is really cool. Thanks @tjamet.