From a714fb69dbf13171238ac17f625cc761af197c80 Mon Sep 17 00:00:00 2001 From: FBLGit Date: Wed, 8 Sep 2021 01:53:16 +0800 Subject: [PATCH] This PR: (#7514) Introduces the CLI command flag `--disable-full-test` By default, it doesn't alter the current behavior of the tests performed by the admission controller. With or Without the flag, a full checkOverlap is actioned, without any alteration and the object `pcfg` is created with the whole set of ingreses. If the flag is set to true, it does manipulate the size of `pcfg` up to the content of $this single ingress. This is achieved by overriding pcfg content by just the last slice that got recently appended to the object `ings` ``` if n.cfg.DisableFullValidationTest { _, _, pcfg = n.getConfiguration(ings[len(ings)-1:]) } ``` The following steps of generateTemplate and testTemplate are significally reduced to a signle scenario ``` content, err := n.generateTemplate(cfg, *pcfg) ... err = n.testTemplate(content) ``` This flag doesn't avoid the proper testing of collisions, neither bad syntaxis within the rendered configuration of the ingress. But it does eliminate a scenario, which I wasn't able to produce, where by for some reason even proper rendering and valid values, without collisions of host/path may end into an invalid nginx.conf The reasoning for this Feature is: - Test duration increases by the number of ingresses in the cluster. - File size grows to very important numbers 150-200Mb on clusters with just 2000~ ingresses. - Tests in that scenario, takes approximately 20s using the last 0.48.1 improvements - Produces a considerable memory consumption, as well as CPU, compute, that affects directly the containers that serve traffic. Since the flag is trully optional, and by default is disabled I fell as a good thing to have that can definitively help on large-scale scenarios that still want to have a reasonable set of tests in place at a lower cost. - [ ] Bug fix (non-breaking change which fixes an issue) - [X ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) Tested with the build kit the following scenarios on a cluster with 1000~ ingresses: - With Flag Disabled or Flag, not present (current status as per 0.48.1) collision scenario (wrong snippet content): `kubectl apply -f ../collision-syntax.yaml 0.18s user 0.05s system 3% cpu 6.639 total` collisions scenario (duplicated host): `kubectl apply -f ../collision-host.yaml 0.17s user 0.05s system 3% cpu 6.245 total` create/update: `kubectl apply -f ing-215.yaml 0.16s user 0.05s system 3% cpu 5.845 total` - With Flag Enabled (true): collision scenario (wrong snippet content): `kubectl apply -f ../collision.yaml 0.18s user 0.02s system 57% cpu 0.347 total` collision scenario (duplicated host): `kubectl apply -f ../collision.yaml 0.21s user 0.06s system 85% cpu 0.318 total` create/update: `kubectl apply -f ing-973.yaml 0.17s user 0.03s system 72% cpu 0.271 total` As part of the test, I did verified that the created nginx for the test was of a smaller size, and that it didnt affect negatively the final nginx.conf (of a much larger side) where this was merged by the next steps in place after the validation. I couldn't observe any other change in the behaviour and so far the routine looks simple and non harmful. - [x] My change requires a change to the documentation. - [x] I have updated the documentation accordingly. - [x] I've read the [CONTRIBUTION](https://github.com/kubernetes/ingress-nginx/blob/main/CONTRIBUTING.md) guide - [ ] I have added tests to cover my changes. - [ ] All new and existing tests passed. For the test part, I would need to understand the placement and test case that this would require, I wasn't able to see an existing scenario for this --- cmd/nginx/flags.go | 3 +++ docs/user-guide/cli-arguments.md | 1 + internal/ingress/controller/controller.go | 5 +++++ 3 files changed, 9 insertions(+) diff --git a/cmd/nginx/flags.go b/cmd/nginx/flags.go index 7e6db6533..c12dc7399 100644 --- a/cmd/nginx/flags.go +++ b/cmd/nginx/flags.go @@ -174,6 +174,8 @@ Takes the form ":port". If not provided, no admission controller is starte `The path of the validating webhook certificate PEM.`) validationWebhookKey = flags.String("validating-webhook-key", "", `The path of the validating webhook key PEM.`) + disableFullValidationTest = flags.Bool("disable-full-test", false, + `Disable full test of all merged ingresses at the admission stage and tests the template of the ingress being created or updated (full test of all ingresses is enabled by default)`) statusPort = flags.Int("status-port", 10246, `Port to use for the lua HTTP endpoint configuration.`) streamPort = flags.Int("stream-port", 10247, "Port to use for the lua TCP/UDP endpoint configuration.") @@ -280,6 +282,7 @@ https://blog.maxmind.com/2019/12/18/significant-changes-to-accessing-and-using-g ConfigMapName: *configMap, TCPConfigMapName: *tcpConfigMapName, UDPConfigMapName: *udpConfigMapName, + DisableFullValidationTest: *disableFullValidationTest, DefaultSSLCertificate: *defSSLCertificate, PublishService: *publishSvc, PublishStatusAddress: *publishStatusAddress, diff --git a/docs/user-guide/cli-arguments.md b/docs/user-guide/cli-arguments.md index 9c9ce0a9b..ef1c0feb2 100644 --- a/docs/user-guide/cli-arguments.md +++ b/docs/user-guide/cli-arguments.md @@ -16,6 +16,7 @@ They are set in the container spec of the `nginx-ingress-controller` Deployment | `--default-server-port` | Port to use for exposing the default server (catch-all). (default 8181) | | `--default-ssl-certificate` | Secret containing a SSL certificate to be used by the default HTTPS server (catch-all). Takes the form "namespace/name". | | `--disable-catch-all` | Disable support for catch-all Ingresses | +| `--disable-full-test` | Disable full test of all merged ingresses at the admission stage and tests the template of the ingress being created or updated (full test of all ingresses is enabled by default) | | `--election-id` | Election id to use for Ingress status updates. (default "ingress-controller-leader") | | `--enable-metrics` | Enables the collection of NGINX metrics (default true) | | `--enable-ssl-chain-completion` | Autocomplete SSL certificate chains with missing intermediate CA certificates. Certificates uploaded to Kubernetes must have the "Authority Information Access" X.509 v3 extension for this to succeed. | diff --git a/internal/ingress/controller/controller.go b/internal/ingress/controller/controller.go index 1fd22079b..38357d491 100644 --- a/internal/ingress/controller/controller.go +++ b/internal/ingress/controller/controller.go @@ -106,6 +106,7 @@ type Configuration struct { ValidationWebhook string ValidationWebhookCertPath string ValidationWebhookKeyPath string + DisableFullValidationTest bool GlobalExternalAuth *ngx_config.GlobalExternalAuth MaxmindEditionFiles *[]string @@ -274,6 +275,10 @@ func (n *NGINXController) CheckIngress(ing *networking.Ingress) error { return err } + if n.cfg.DisableFullValidationTest { + _, _, pcfg = n.getConfiguration(ings[len(ings)-1:]) + } + content, err := n.generateTemplate(cfg, *pcfg) if err != nil { n.metricCollector.IncCheckErrorCount(ing.ObjectMeta.Namespace, ing.Name)