From e8a10e188d48049a5022d53b6b36104eaa31a0b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabi=C3=A1n=20Sell=C3=A9s?= Date: Thu, 1 Feb 2024 13:05:55 +0100 Subject: [PATCH] Add disable path overlap validation flag ## What this PR does / why we need it: In https://github.com/kubernetes/ingress-nginx/issues/5651 there was a request to throw an error when there are two ingresses defining the same host and path, which was implemented as part of the validation webhook. Despite of this there are clear rules on the ingress controller that describes what the controller would do in [such situation (the oldest rule wins)](https://github.com/kubernetes/ingress-nginx/blob/main/docs/how-it-works.md?plain=1#L27) Some users are relying on this validation behaviour to prevent misconfigurations, but there are use cases where allowing it, maybe temporarily, is helpful. Change-Id: I9d4124d1c36876b06d63100cd10988eaf2f41db9 --- internal/ingress/controller/controller.go | 20 +++-- .../ingress/controller/controller_test.go | 77 +++++++++++++++++++ pkg/flags/flags.go | 68 ++++++++-------- 3 files changed, 126 insertions(+), 39 deletions(-) diff --git a/internal/ingress/controller/controller.go b/internal/ingress/controller/controller.go index f60f7a0533..5d9d9121f9 100644 --- a/internal/ingress/controller/controller.go +++ b/internal/ingress/controller/controller.go @@ -111,10 +111,11 @@ type Configuration struct { IngressClassConfiguration *ingressclass.IngressClassConfiguration - ValidationWebhook string - ValidationWebhookCertPath string - ValidationWebhookKeyPath string - DisableFullValidationTest bool + ValidationWebhook string + ValidationWebhookCertPath string + ValidationWebhookKeyPath string + DisableFullValidationTest bool + DisablePathOverlapValidation bool GlobalExternalAuth *ngx_config.GlobalExternalAuth MaxmindEditionFiles *[]string @@ -319,11 +320,14 @@ func (n *NGINXController) CheckIngress(ing *networking.Ingress) error { startTest := time.Now().UnixNano() / 1000000 _, servers, pcfg := n.getConfiguration(ings) - err := checkOverlap(ing, allIngresses, servers) - if err != nil { - n.metricCollector.IncCheckErrorCount(ing.ObjectMeta.Namespace, ing.Name) - return err + if !n.cfg.DisablePathOverlapValidation { + err := checkOverlap(ing, allIngresses, servers) + if err != nil { + n.metricCollector.IncCheckErrorCount(ing.ObjectMeta.Namespace, ing.Name) + return err + } } + testedSize := len(ings) if n.cfg.DisableFullValidationTest { _, _, pcfg = n.getConfiguration(ings[len(ings)-1:]) diff --git a/internal/ingress/controller/controller_test.go b/internal/ingress/controller/controller_test.go index da9f10e454..836b2d2329 100644 --- a/internal/ingress/controller/controller_test.go +++ b/internal/ingress/controller/controller_test.go @@ -339,6 +339,83 @@ func TestCheckIngress(t *testing.T) { t.Errorf("with a new ingress without error, no error should be returned") } }) + + t.Run("When there is a duplicated ingress with same host and path it should error", func(t *testing.T) { + ing := &networking.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-ingress", + Namespace: "test-namespace", + Annotations: map[string]string{ + "kubernetes.io/ingress.class": "nginx", + }, + }, + Spec: networking.IngressSpec{ + Rules: []networking.IngressRule{ + { + Host: "example.com", + IngressRuleValue: networking.IngressRuleValue{ + HTTP: &networking.HTTPIngressRuleValue{ + Paths: []networking.HTTPIngressPath{ + { + Path: "/", + PathType: &pathTypePrefix, + Backend: networking.IngressBackend{ + Service: &networking.IngressServiceBackend{ + Name: "http-svc", + Port: networking.ServiceBackendPort{ + Number: 80, + Name: "http", + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + } + + nginx.store = &fakeIngressStore{ + ingresses: []*ingress.Ingress{ + { + Ingress: *ing, + ParsedAnnotations: &annotations.Ingress{}, + }, + }, + } + duplicatedIngress := ing.DeepCopy() + duplicatedIngress.ObjectMeta.Name = "duplicated-ingress" + + nginx.cfg.DisablePathOverlapValidation = false + nginx.command = testNginxTestCommand{ + t: t, + err: nil, + expected: "_,example.com", + } + + err = nginx.CheckIngress(duplicatedIngress) + if err == nil { + t.Errorf("expected errors but none occurred") + } + t.Run("when disablePathOverlap is enabled should not throw any error", func(t *testing.T) { + duplicatedIngress := ing.DeepCopy() + duplicatedIngress.ObjectMeta.Name = "duplicated-ingress" + + nginx.cfg.DisablePathOverlapValidation = true + nginx.command = testNginxTestCommand{ + t: t, + err: nil, + expected: "_,example.com", + } + + err = nginx.CheckIngress(duplicatedIngress) + if err != nil { + t.Errorf("expected no errors but one %+v occurred", err) + } + }) + }) }) t.Run("When the ingress is marked as deleted", func(t *testing.T) { diff --git a/pkg/flags/flags.go b/pkg/flags/flags.go index 65b5fbcdc1..81137778da 100644 --- a/pkg/flags/flags.go +++ b/pkg/flags/flags.go @@ -194,6 +194,11 @@ 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.`) + disablePathOverlapValidation = flags.Bool("disable-path-overlap-validation", false, + `Disable path overlap validation. Validation webhook blocks creating ingresses with the same hostname and path in the same ingressClass. + These flags turn off this validation as it helps split ingresses or move them between namespaces as it relies on the existing model rules: + 'If the same path for the same host is defined in more than one Ingress, the oldest rule wins'`) + 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).`) @@ -315,37 +320,38 @@ https://blog.maxmind.com/2019/12/18/significant-changes-to-accessing-and-using-g ngx_config.EnableSSLChainCompletion = *enableSSLChainCompletion config := &controller.Configuration{ - APIServerHost: *apiserverHost, - KubeConfigFile: *kubeConfigFile, - UpdateStatus: *updateStatus, - ElectionID: *electionID, - EnableProfiling: *profiling, - EnableMetrics: *enableMetrics, - MetricsPerHost: *metricsPerHost, - MetricsBuckets: histogramBuckets, - ReportStatusClasses: *reportStatusClasses, - MonitorMaxBatchSize: *monitorMaxBatchSize, - DisableServiceExternalName: *disableServiceExternalName, - EnableSSLPassthrough: *enableSSLPassthrough, - ResyncPeriod: *resyncPeriod, - DefaultService: *defaultSvc, - Namespace: *watchNamespace, - WatchNamespaceSelector: namespaceSelector, - ConfigMapName: *configMap, - TCPConfigMapName: *tcpConfigMapName, - UDPConfigMapName: *udpConfigMapName, - DisableFullValidationTest: *disableFullValidationTest, - DefaultSSLCertificate: *defSSLCertificate, - DeepInspector: *deepInspector, - PublishService: *publishSvc, - PublishStatusAddress: *publishStatusAddress, - UpdateStatusOnShutdown: *updateStatusOnShutdown, - ShutdownGracePeriod: *shutdownGracePeriod, - PostShutdownGracePeriod: *postShutdownGracePeriod, - UseNodeInternalIP: *useNodeInternalIP, - SyncRateLimit: *syncRateLimit, - HealthCheckHost: *healthzHost, - DynamicConfigurationRetries: *dynamicConfigurationRetries, + APIServerHost: *apiserverHost, + KubeConfigFile: *kubeConfigFile, + UpdateStatus: *updateStatus, + ElectionID: *electionID, + EnableProfiling: *profiling, + EnableMetrics: *enableMetrics, + MetricsPerHost: *metricsPerHost, + MetricsBuckets: histogramBuckets, + ReportStatusClasses: *reportStatusClasses, + MonitorMaxBatchSize: *monitorMaxBatchSize, + DisableServiceExternalName: *disableServiceExternalName, + EnableSSLPassthrough: *enableSSLPassthrough, + ResyncPeriod: *resyncPeriod, + DefaultService: *defaultSvc, + Namespace: *watchNamespace, + WatchNamespaceSelector: namespaceSelector, + ConfigMapName: *configMap, + TCPConfigMapName: *tcpConfigMapName, + UDPConfigMapName: *udpConfigMapName, + DisableFullValidationTest: *disableFullValidationTest, + DisablePathOverlapValidation: *disablePathOverlapValidation, + DefaultSSLCertificate: *defSSLCertificate, + DeepInspector: *deepInspector, + PublishService: *publishSvc, + PublishStatusAddress: *publishStatusAddress, + UpdateStatusOnShutdown: *updateStatusOnShutdown, + ShutdownGracePeriod: *shutdownGracePeriod, + PostShutdownGracePeriod: *postShutdownGracePeriod, + UseNodeInternalIP: *useNodeInternalIP, + SyncRateLimit: *syncRateLimit, + HealthCheckHost: *healthzHost, + DynamicConfigurationRetries: *dynamicConfigurationRetries, ListenPorts: &ngx_config.ListenPorts{ Default: *defServerPort, Health: *healthzPort,