[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 <[email protected]>

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,16 +61,48 @@ 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
 
 	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.

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
 

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,

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 {

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)
 	}