From e9664c2566810b316d857250439fcb990328741f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabi=C3=A1n=20Sell=C3=A9s=20Rosa?= Date: Tue, 30 Jan 2024 00:01:03 +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. Those use cases includes: - Splitting large ingresses objects in smaller ones https://github.com/kubernetes/ingress-nginx/issues/10820 - Moving ingress objects between namespaces without downtime (like when you transfer an ingress from team A that owns namespace A to team B that owns namespace B) https://github.com/kubernetes/ingress-nginx/issues/10090 ## Types of changes - [ ] Bug fix (non-breaking change which fixes an issue) - [X] New feature (non-breaking change which adds functionality) - [ ] CVE Report (Scanner found CVE and adding report) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Documentation only ## Which issue/s this PR fixes It might help with #10820 ## How Has This Been Tested? building an image and testing it in a local cluster, will update later with some real life scenarios ## Checklist: - [X] My change requires a change to the documentation. - [ ] I have updated the documentation accordingly. - [X] I've read the [CONTRIBUTION](https://github.com/kubernetes/ingress-nginx/blob/main/CONTRIBUTING.md) guide - [X] I have added unit and/or e2e tests to cover my changes. - [X] All new and existing tests passed. Change-Id: I9d4124d1c36876b06d63100cd10988eaf2f41db9 --- internal/ingress/controller/controller.go | 21 +++-- .../ingress/controller/controller_test.go | 77 +++++++++++++++++ pkg/flags/flags.go | 82 ++++++++++--------- test/e2e/admission/admission.go | 43 ++++++++++ 4 files changed, 177 insertions(+), 46 deletions(-) diff --git a/internal/ingress/controller/controller.go b/internal/ingress/controller/controller.go index aa8f4c4b92..28415e952d 100644 --- a/internal/ingress/controller/controller.go +++ b/internal/ingress/controller/controller.go @@ -122,10 +122,11 @@ type Configuration struct { IngressClassConfiguration *ingressclass.Configuration - ValidationWebhook string - ValidationWebhookCertPath string - ValidationWebhookKeyPath string - DisableFullValidationTest bool + ValidationWebhook string + ValidationWebhookCertPath string + ValidationWebhookKeyPath string + DisableFullValidationTest bool + DisablePathOverlapValidation bool GlobalExternalAuth *ngx_config.GlobalExternalAuth MaxmindEditionFiles *[]string @@ -403,10 +404,14 @@ func (n *NGINXController) CheckIngress(ing *networking.Ingress) error { startTest := time.Now().UnixNano() / 1000000 _, servers, pcfg := n.getConfiguration(ings) - err = checkOverlap(ing, servers) - if err != nil { - n.metricCollector.IncCheckErrorCount(ing.ObjectMeta.Namespace, ing.Name) - return err + if n.cfg.DisablePathOverlapValidation { + klog.Warningf("ingress %v in namespace %v not checked for path overlap since --disable-path-overlap-validation is enabled", ing.Name, ing.ObjectMeta.Namespace) + } else { + err = checkOverlap(ing, servers) + if err != nil { + n.metricCollector.IncCheckErrorCount(ing.ObjectMeta.Namespace, ing.Name) + return err + } } testedSize := len(ings) if n.cfg.DisableFullValidationTest { diff --git a/internal/ingress/controller/controller_test.go b/internal/ingress/controller/controller_test.go index 9d3fea4708..c95e34deab 100644 --- a/internal/ingress/controller/controller_test.go +++ b/internal/ingress/controller/controller_test.go @@ -357,6 +357,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 noone occurred") + } + t.Run("if 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 ce24160fdc..fe5a30b14e 100644 --- a/pkg/flags/flags.go +++ b/pkg/flags/flags.go @@ -209,6 +209,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).`) @@ -339,44 +344,45 @@ https://blog.maxmind.com/2019/12/significant-changes-to-accessing-and-using-geol ngx_config.EnableSSLChainCompletion = *enableSSLChainCompletion config := &controller.Configuration{ - APIServerHost: *apiserverHost, - KubeConfigFile: *kubeConfigFile, - UpdateStatus: *updateStatus, - ElectionID: *electionID, - ElectionTTL: *electionTTL, - EnableProfiling: *profiling, - EnableMetrics: *enableMetrics, - MetricsPerHost: *metricsPerHost, - MetricsPerUndefinedHost: *metricsPerUndefinedHost, - MetricsBuckets: histogramBuckets, - MetricsBucketFactor: *bucketFactor, - MetricsMaxBuckets: *maxBuckets, - ReportStatusClasses: *reportStatusClasses, - ExcludeSocketMetrics: *excludeSocketMetrics, - MonitorMaxBatchSize: *monitorMaxBatchSize, - DisableServiceExternalName: *disableServiceExternalName, - EnableSSLPassthrough: *enableSSLPassthrough, - DisableLeaderElection: *disableLeaderElection, - 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, - EnableTopologyAwareRouting: *enableTopologyAwareRouting, + APIServerHost: *apiserverHost, + KubeConfigFile: *kubeConfigFile, + UpdateStatus: *updateStatus, + ElectionID: *electionID, + ElectionTTL: *electionTTL, + EnableProfiling: *profiling, + EnableMetrics: *enableMetrics, + MetricsPerHost: *metricsPerHost, + MetricsPerUndefinedHost: *metricsPerUndefinedHost, + MetricsBuckets: histogramBuckets, + MetricsBucketFactor: *bucketFactor, + MetricsMaxBuckets: *maxBuckets, + ReportStatusClasses: *reportStatusClasses, + ExcludeSocketMetrics: *excludeSocketMetrics, + MonitorMaxBatchSize: *monitorMaxBatchSize, + DisableServiceExternalName: *disableServiceExternalName, + EnableSSLPassthrough: *enableSSLPassthrough, + DisableLeaderElection: *disableLeaderElection, + 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, + EnableTopologyAwareRouting: *enableTopologyAwareRouting, ListenPorts: &ngx_config.ListenPorts{ Default: *defServerPort, Health: *healthzPort, diff --git a/test/e2e/admission/admission.go b/test/e2e/admission/admission.go index 873e6719da..0d8f747ad4 100644 --- a/test/e2e/admission/admission.go +++ b/test/e2e/admission/admission.go @@ -26,6 +26,7 @@ import ( "github.com/onsi/ginkgo/v2" "github.com/stretchr/testify/assert" + appsv1 "k8s.io/api/apps/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -61,6 +62,48 @@ var _ = framework.IngressNginxDescribeSerial("[Admission] admission controller", assert.NotNil(ginkgo.GinkgoT(), err, "creating an ingress with the same host and path should return an error") }) + ginkgo.It("should allow path overlaps if disable-path-overlap-validation is enabled", func() { + host := admissionTestHost + + err := f.UpdateIngressControllerDeployment(func(deployment *appsv1.Deployment) error { + args := deployment.Spec.Template.Spec.Containers[0].Args + args = append(args, "--disable-path-overlap-validation") + deployment.Spec.Template.Spec.Containers[0].Args = args + _, err := f.KubeClientSet.AppsV1().Deployments(f.Namespace).Update(context.TODO(), deployment, metav1.UpdateOptions{}) + return err + }) + assert.Nil(ginkgo.GinkgoT(), err, "updating deployment") + defer func() { + err = f.UpdateIngressControllerDeployment(func(deployment *appsv1.Deployment) error { + args := []string{} + + for _, v := range deployment.Spec.Template.Spec.Containers[0].Args { + if strings.Contains(v, "--disable-path-overlap-validation") { + continue + } + + args = append(args, v) + } + deployment.Spec.Template.Spec.Containers[0].Args = args + _, err := f.KubeClientSet.AppsV1().Deployments(f.Namespace).Update(context.TODO(), deployment, metav1.UpdateOptions{}) + return err + }) + assert.Nil(ginkgo.GinkgoT(), err, "restoring deployment") + }() + oneIngress := framework.NewSingleIngress("a-ingress", "/", host, f.Namespace, framework.EchoService, 80, nil) + _, err = f.KubeClientSet.NetworkingV1().Ingresses(f.Namespace).Create(context.TODO(), oneIngress, metav1.CreateOptions{}) + assert.Nil(ginkgo.GinkgoT(), err, "creating ingress") + + f.WaitForNginxServer(host, + func(server string) bool { + return strings.Contains(server, fmt.Sprintf("server_name %v", host)) + }) + + anotherIngress := framework.NewSingleIngress("another-ingress", "/", host, f.Namespace, framework.EchoService, 80, nil) + _, err = f.KubeClientSet.NetworkingV1().Ingresses(f.Namespace).Create(context.TODO(), anotherIngress, metav1.CreateOptions{}) + assert.Nil(ginkgo.GinkgoT(), err, "creating an ingress with the same host and path should be allowed when disable-path-overlap-validation is true ") + }) + ginkgo.It("should allow overlaps of host and paths with canary annotation", func() { host := admissionTestHost