From 5e38a9aa94b9f3ccf289ad63f0a81f73833c2807 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20Th=C3=B6mmes?= Date: Thu, 16 Apr 2020 12:57:50 +0200 Subject: [PATCH] [CP] Validate Ingress before adding it to the cache: Avoid duplicated domains (#39) (#1) * Validate Ingress before adding it to the cache: Avoid duplicated domains * Fix typo * Mark the ingress as failed with DuplicatedDomain reason * Improve reason and message for duplicated domain * Return error instead of handling the status here. * * Don't initialize status again. * Use ingress.status instead of copying it. * Observed generation is handled at the beginning of the reconciliation loop. * * Increase the observerGeneration at the start of the reconciliation loop. * Handle the specific error of ErrDomainConflict by marking the ingress as failed. * Define specific error for the duplicated domain error * Addressing PR comments. * Removed Errors package. * Inlined error message. * Simplified addIngressToCache logic * Moved ErrDomainConflict to generator package. * Added unit test for ValidateIngress * PR comments * Make ValidateIngress a private func * Do the validation inside AddTranslatedIngress * Make sure the validateIngress test fails due to ErrDomainConflict Co-authored-by: Joaquim Moreno Prusi --- pkg/generator/caches.go | 39 +++++++++++++++++++++++++++++-- pkg/generator/caches_test.go | 35 +++++++++++++++++++++++++-- pkg/generator/generator.go | 7 +++--- pkg/knative/ingress.go | 8 ++----- pkg/reconciler/ingress/ingress.go | 12 ++++++++-- 5 files changed, 86 insertions(+), 15 deletions(-) diff --git a/pkg/generator/caches.go b/pkg/generator/caches.go index 7ae06a8c7..f9aeeaf3c 100644 --- a/pkg/generator/caches.go +++ b/pkg/generator/caches.go @@ -17,9 +17,10 @@ limitations under the License. package generator import ( - "knative.dev/net-kourier/pkg/envoy" + "errors" "go.uber.org/zap" + "knative.dev/net-kourier/pkg/envoy" "github.com/golang/protobuf/ptypes/wrappers" @@ -32,6 +33,8 @@ import ( "knative.dev/serving/pkg/apis/networking/v1alpha1" ) +var ErrDomainConflict = errors.New("ingress has a conflicting domain with another ingress") + type Caches struct { ingresses map[string]*v1alpha1.Ingress translatedIngresses map[string]*translatedIngress @@ -58,9 +61,39 @@ func (caches *Caches) GetIngress(ingressName, ingressNamespace string) *v1alpha1 return caches.ingresses[mapKey(ingressName, ingressNamespace)] } -func (caches *Caches) AddTranslatedIngress(ingress *v1alpha1.Ingress, translatedIngress *translatedIngress) { +func (caches *Caches) validateIngress(translatedIngress *translatedIngress) error { + + // We compare the Translated Ingress to current cached Virtualhosts, and look for any domain + // clashes. If there's one clashing domain, we reject the ingress. + localVhosts := caches.clusterLocalVirtualHosts() + + // Return true early. + if len(localVhosts) == 0 { + return nil + } + + for _, vhost := range translatedIngress.internalVirtualHosts { + for _, domain := range vhost.Domains { + for _, cacheVhost := range localVhosts { + for _, cachedDomain := range cacheVhost.Domains { + if domain == cachedDomain { + return ErrDomainConflict + } + } + } + } + } + + return nil +} + +func (caches *Caches) AddTranslatedIngress(ingress *v1alpha1.Ingress, translatedIngress *translatedIngress) error { caches.logger.Debugf("adding ingress: %s/%s", ingress.Name, ingress.Namespace) + if err := caches.validateIngress(translatedIngress); err != nil { + return err + } + key := mapKey(ingress.Name, ingress.Namespace) caches.ingresses[key] = ingress caches.translatedIngresses[key] = translatedIngress @@ -68,6 +101,8 @@ func (caches *Caches) AddTranslatedIngress(ingress *v1alpha1.Ingress, translated for _, cluster := range translatedIngress.clusters { caches.AddClusterForIngress(cluster, ingress.Name, ingress.Namespace) } + + return nil } // SetOnEvicted allows to set a function that will be executed when any key on the cache expires. diff --git a/pkg/generator/caches_test.go b/pkg/generator/caches_test.go index 8d2e6a73c..d2174d7c0 100644 --- a/pkg/generator/caches_test.go +++ b/pkg/generator/caches_test.go @@ -170,8 +170,8 @@ func createTestDataForIngress(caches *Caches, ingressNamespace: ingressNamespace, routes: []*route.Route{{Name: routeName}}, clusters: []*v2.Cluster{{Name: clusterName}}, - externalVirtualHosts: []*route.VirtualHost{{Name: externalVHostName}}, - internalVirtualHosts: []*route.VirtualHost{{Name: internalVHostName}}, + externalVirtualHosts: []*route.VirtualHost{{Name: externalVHostName, Domains: []string{externalVHostName}}}, + internalVirtualHosts: []*route.VirtualHost{{Name: internalVHostName, Domains: []string{internalVHostName}}}, } caches.AddTranslatedIngress(&v1alpha1.Ingress{ @@ -185,6 +185,37 @@ func createTestDataForIngress(caches *Caches, _ = caches.SetListeners(kubeClient) } +func TestValidateIngress(t *testing.T) { + logger := zap.S() + + caches := NewCaches(logger) + kubeClient := fake.Clientset{} + + createTestDataForIngress( + caches, + "ingress_1", + "ingress_1_namespace", + "cluster_for_ingress_1", + "route_for_ingress_1", + "internal_host_for_ingress_1", + "external_host_for_ingress_1", + &kubeClient, + ) + + translatedIngress := translatedIngress{ + ingressName: "ingress_2", + ingressNamespace: "ingress_2_namespace", + routes: []*route.Route{{Name: "route_for_ingress_2"}}, + clusters: []*v2.Cluster{{Name: "cluster_for_ingress_2"}}, + externalVirtualHosts: []*route.VirtualHost{{Name: "external_host_for_ingress_2", Domains: []string{"external_host_for_ingress_2"}}}, + //This domain should clash with the cached ingress. + internalVirtualHosts: []*route.VirtualHost{{Name: "internal_host_for_ingress_2", Domains: []string{"internal_host_for_ingress_1"}}}, + } + + err := caches.validateIngress(&translatedIngress) + assert.Error(t, err, ErrDomainConflict.Error()) +} + func getVHostsNames(routeConfigs []v2.RouteConfiguration) ([]string, error) { var res []string diff --git a/pkg/generator/generator.go b/pkg/generator/generator.go index ba809c272..d66e9ff40 100644 --- a/pkg/generator/generator.go +++ b/pkg/generator/generator.go @@ -82,11 +82,12 @@ func addIngressToCaches(caches *Caches, if err != nil { return err } - if ingressTranslation != nil { - caches.AddTranslatedIngress(ingress, ingressTranslation) + + if ingressTranslation == nil { + return nil } - return nil + return caches.AddTranslatedIngress(ingress, ingressTranslation) } func listenersFromVirtualHosts(externalVirtualHosts []*route.VirtualHost, diff --git a/pkg/knative/ingress.go b/pkg/knative/ingress.go index cbe61f194..7a5f253f0 100644 --- a/pkg/knative/ingress.go +++ b/pkg/knative/ingress.go @@ -25,11 +25,9 @@ import ( ) func MarkIngressReady(ingress *networkingv1alpha1.Ingress) { - status := ingress.Status internalDomain := domainForServiceName(config.InternalServiceName) externalDomain := domainForServiceName(config.ExternalServiceName) - status.InitializeConditions() var domain string if ingress.Spec.Visibility == networkingv1alpha1.IngressVisibilityClusterLocal { @@ -38,7 +36,7 @@ func MarkIngressReady(ingress *networkingv1alpha1.Ingress) { domain = externalDomain } - status.MarkLoadBalancerReady( + ingress.Status.MarkLoadBalancerReady( []networkingv1alpha1.LoadBalancerIngressStatus{{ DomainInternal: domain, }}, @@ -49,9 +47,7 @@ func MarkIngressReady(ingress *networkingv1alpha1.Ingress) { DomainInternal: internalDomain, }}, ) - status.MarkNetworkConfigured() - status.ObservedGeneration = ingress.GetGeneration() - ingress.Status = status + ingress.Status.MarkNetworkConfigured() } func domainForServiceName(serviceName string) string { diff --git a/pkg/reconciler/ingress/ingress.go b/pkg/reconciler/ingress/ingress.go index 11e5b5981..663b8fbd8 100644 --- a/pkg/reconciler/ingress/ingress.go +++ b/pkg/reconciler/ingress/ingress.go @@ -67,8 +67,16 @@ func (reconciler *Reconciler) Reconcile(ctx context.Context, key string) error { ingress := original.DeepCopy() ingress.SetDefaults(ctx) ingress.Status.InitializeConditions() - - if err := reconciler.updateIngress(ingress); err != nil { + // This is the generation we are going to handle during this reconciliation loop. + ingress.Status.ObservedGeneration = ingress.GetGeneration() + + err = reconciler.updateIngress(ingress) + if err == generator.ErrDomainConflict { + // If we had an error due to a duplicated domain, we must mark the ingress as failed with a + // custom status. We don't want to return an error in this case as we want to update its status. + reconciler.logger.Errorw(err.Error(), ingress.Name, ingress.Namespace) + ingress.Status.MarkLoadBalancerFailed("DomainConflict", "Ingress rejected as its domain conflicts with another ingress") + } else if err != nil { return fmt.Errorf("failed to update ingress: %w", err) }