[WIP] OCPBUGS-1689: Router clear admitted status on label changes

go.mod

@@ -19,6 +19,7 @@ require (
 	github.com/prometheus/common v0.37.0
 	github.com/spf13/cobra v1.6.0
 	github.com/spf13/pflag v1.0.5
+	github.com/stretchr/testify v1.8.1
 	k8s.io/api v0.27.2
 	k8s.io/apimachinery v0.27.2
 	k8s.io/apiserver v0.27.2
@@ -68,6 +69,7 @@ require (
 	github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect
 	github.com/pkg/errors v0.9.1 // indirect
 	github.com/pkg/profile v1.3.0 // indirect
+	github.com/pmezard/go-difflib v1.0.0 // indirect
 	github.com/prometheus/procfs v0.8.0 // indirect
 	github.com/sirupsen/logrus v1.9.0 // indirect
 	github.com/stoewer/go-strcase v1.2.0 // indirect

pkg/router/controller/extended_validator.go

@@ -51,7 +51,7 @@ func (p *ExtendedValidator) HandleRoute(eventType watch.EventType, route *routev
 		log.Error(err, "skipping route due to invalid configuration", "route", routeName)
 
 		p.recorder.RecordRouteRejection(route, "ExtendedValidationFailed", err.Error())
-		p.plugin.HandleRoute(watch.Deleted, route)
+		p.plugin.HandleRoute(watch.Error, route)
 		return fmt.Errorf("invalid route configuration")
 	}
 

pkg/router/controller/host_admitter.go

@@ -127,15 +127,15 @@ func (p *HostAdmitter) HandleEndpoints(eventType watch.EventType, endpoints *kap
 // HandleRoute processes watch events on the Route resource.
 func (p *HostAdmitter) HandleRoute(eventType watch.EventType, route *routev1.Route) error {
 	if p.allowedNamespaces != nil && !p.allowedNamespaces.Has(route.Namespace) {
-		// Ignore routes we don't need to "service" due to namespace
-		// restrictions (ala for sharding).
-		return nil
+		// Routes that we don't need to "service" due to namespace
+		// restrictions (ala for sharding) should be deleted/cleared.
+		return p.plugin.HandleRoute(watch.Deleted, route)
 	}
 
 	if err := p.admitter(route); err != nil {
 		log.V(4).Info("route not admitted", "namespace", route.Namespace, "name", route.Name, "error", err.Error())
 		p.recorder.RecordRouteRejection(route, "RouteNotAdmitted", err.Error())
-		p.plugin.HandleRoute(watch.Deleted, route)
+		p.plugin.HandleRoute(watch.Error, route)
 		return err
 	}
 

pkg/router/controller/host_admitter_test.go

@@ -3,11 +3,12 @@ package controller
 import (
 	"fmt"
 	"math/rand"
-	"reflect"
 	"strings"
 	"testing"
 	"time"
 
+	"github.com/stretchr/testify/assert"
+
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
@@ -1037,83 +1038,88 @@ func TestHandleNamespaceProcessing(t *testing.T) {
 	}
 
 	tests := []struct {
-		name      string
-		namespace string
-		host      string
-		policy    routev1.WildcardPolicyType
-		expected  bool
+		routeName   string
+		namespace   string
+		host        string
+		policy      routev1.WildcardPolicyType
+		expectEvent watch.EventType
+		expectErr   bool
 	}{
+		// WARNING: These test cases MUST be run together; otherwise, some will fail.
 		{
-			name:      "expected",
-			namespace: "ns1",
-			host:      "update.expected.test",
-			policy:    routev1.WildcardPolicyNone,
-			expected:  true,
+			routeName:   "expected",
+			namespace:   "ns1",
+			host:        "update.expected.test",
+			policy:      routev1.WildcardPolicyNone,
+			expectEvent: watch.Added,
 		},
 		{
-			name:      "not-expected",
-			namespace: "updatemenot",
-			host:      "no-update.expected.test",
-			policy:    routev1.WildcardPolicyNone,
-			expected:  false,
+			routeName:   "not-expected",
+			namespace:   "updatemenot",
+			host:        "no-update.expected.test",
+			policy:      routev1.WildcardPolicyNone,
+			expectEvent: watch.Deleted,
 		},
 		{
-			name:      "expected-wild",
-			namespace: "ns1",
-			host:      "update.wild.expected.test",
-			policy:    routev1.WildcardPolicySubdomain,
-			expected:  true,
+			routeName:   "expected-wild",
+			namespace:   "ns1",
+			host:        "update.wild.expected.test",
+			policy:      routev1.WildcardPolicySubdomain,
+			expectEvent: watch.Added,
 		},
 		{
-			name:      "not-expected-wild-not-owner",
-			namespace: "nsx",
-			host:      "second.wild.expected.test",
-			policy:    routev1.WildcardPolicySubdomain,
-			expected:  false,
+			routeName:   "not-expected-wild-not-owner",
+			namespace:   "nsx",
+			host:        "second.wild.expected.test",
+			policy:      routev1.WildcardPolicySubdomain,
+			expectEvent: watch.Added,
+			expectErr:   true,
 		},
 		{
-			name:      "not-expected-wild",
-			namespace: "otherns",
-			host:      "noupdate.wild.expected.test",
-			policy:    routev1.WildcardPolicySubdomain,
-			expected:  false,
+			routeName:   "not-expected-wild",
+			namespace:   "otherns",
+			host:        "noupdate.wild.expected.test",
+			policy:      routev1.WildcardPolicySubdomain,
+			expectEvent: watch.Deleted,
 		},
 		{
-			name:      "expected-wild-other-subdomain",
-			namespace: "nsx",
-			host:      "host.third.wild.expected.test",
-			policy:    routev1.WildcardPolicySubdomain,
-			expected:  true,
+			routeName:   "expected-wild-other-subdomain",
+			namespace:   "nsx",
+			host:        "host.third.wild.expected.test",
+			policy:      routev1.WildcardPolicySubdomain,
+			expectEvent: watch.Added,
 		},
 		{
-			name:      "not-expected-plain-2",
-			namespace: "notallowed",
-			host:      "not.allowed.expected.test",
-			policy:    routev1.WildcardPolicyNone,
-			expected:  false,
+			routeName:   "not-expected-plain-2",
+			namespace:   "notallowed",
+			host:        "not.allowed.expected.test",
+			policy:      routev1.WildcardPolicyNone,
+			expectEvent: watch.Deleted,
 		},
 		{
-			name:      "not-expected-blocked",
-			namespace: "nsx",
-			host:      "blitz.domain.blocked.test",
-			policy:    routev1.WildcardPolicyNone,
-			expected:  false,
+			routeName:   "not-expected-blocked",
+			namespace:   "nsx",
+			host:        "blitz.domain.blocked.test",
+			policy:      routev1.WildcardPolicyNone,
+			expectEvent: watch.Error,
+			expectErr:   true,
 		},
 		{
-			name:      "not-expected-blocked-wildcard",
-			namespace: "ns2",
-			host:      "wild.blocked.domain.blocked.test",
-			policy:    routev1.WildcardPolicySubdomain,
-			expected:  false,
+			routeName:   "not-expected-blocked-wildcard",
+			namespace:   "ns2",
+			host:        "wild.blocked.domain.blocked.test",
+			policy:      routev1.WildcardPolicySubdomain,
+			expectEvent: watch.Error,
+			expectErr:   true,
 		},
 	}
 
 	for _, tc := range tests {
 		route := &routev1.Route{
 			ObjectMeta: metav1.ObjectMeta{
-				Name:      tc.name,
+				Name:      tc.routeName,
 				Namespace: tc.namespace,
-				UID:       types.UID(tc.name),
+				UID:       types.UID(tc.routeName),
 			},
 			Spec: routev1.RouteSpec{
 				Host:           tc.host,
@@ -1132,15 +1138,11 @@ func TestHandleNamespaceProcessing(t *testing.T) {
 		}
 
 		err := admitter.HandleRoute(watch.Added, route)
-		if tc.expected {
-			if err != nil {
-				t.Fatalf("test case %s unexpected error: %v", tc.name, err)
-			}
-			if !reflect.DeepEqual(p.route, route) {
-				t.Fatalf("test case %s expected route to be processed: %+v", tc.name, route)
-			}
-		} else if err == nil && reflect.DeepEqual(p.route, route) {
-			t.Fatalf("test case %s did not expected route to be processed: %+v", tc.name, route)
+		assert.Equal(t, p.event, tc.expectEvent, "route: "+tc.routeName)
+		if tc.expectErr && err == nil {
+			t.Fatalf("route %s expected error, but did not receive one", tc.routeName)
+		} else if !tc.expectErr && err != nil {
+			t.Fatalf("route %s unexpected error: %v", tc.routeName, err)
 		}
 	}
 }

pkg/router/controller/router_controller.go

@@ -125,11 +125,11 @@ func (c *RouterController) processNamespace(eventType watch.EventType, ns *kapi.
 					}
 				}
 			}
+		}
 
-			if routeMap, ok := c.NamespaceRoutes[ns.Name]; ok {
-				for _, route := range routeMap {
-					c.processRoute(watch.Modified, route)
-				}
+		if routeMap, ok := c.NamespaceRoutes[ns.Name]; ok {
+			for _, route := range routeMap {
+				c.processRoute(watch.Modified, route)
 			}
 		}
 	}

pkg/router/controller/status.go

@@ -78,10 +78,12 @@ var nowFn = getRfc3339Timestamp
 func (a *StatusAdmitter) HandleRoute(eventType watch.EventType, route *routev1.Route) error {
 	switch eventType {
 	case watch.Added, watch.Modified:
-		performIngressConditionUpdate("admit", a.lease, a.tracker, a.client, a.lister, route, a.routerName, a.routerCanonicalHostname, routev1.RouteIngressCondition{
+		performIngressConditionUpdate("admit", eventType, a.lease, a.tracker, a.client, a.lister, route, a.routerName, a.routerCanonicalHostname, &routev1.RouteIngressCondition{
 			Type:   routev1.RouteAdmitted,
 			Status: corev1.ConditionTrue,
 		})
+	case watch.Deleted:
+		performIngressConditionUpdate("delete", eventType, a.lease, a.tracker, a.client, a.lister, route, a.routerName, a.routerCanonicalHostname, nil)
 	}
 	return a.plugin.HandleRoute(eventType, route)
 }
@@ -104,7 +106,7 @@ func (a *StatusAdmitter) Commit() error {
 
 // RecordRouteRejection attempts to update the route status with a reason for a route being rejected.
 func (a *StatusAdmitter) RecordRouteRejection(route *routev1.Route, reason, message string) {
-	performIngressConditionUpdate("reject", a.lease, a.tracker, a.client, a.lister, route, a.routerName, a.routerCanonicalHostname, routev1.RouteIngressCondition{
+	performIngressConditionUpdate("reject", watch.Error, a.lease, a.tracker, a.client, a.lister, route, a.routerName, a.routerCanonicalHostname, &routev1.RouteIngressCondition{
 		Type:    routev1.RouteAdmitted,
 		Status:  corev1.ConditionFalse,
 		Reason:  reason,
@@ -113,15 +115,29 @@ func (a *StatusAdmitter) RecordRouteRejection(route *routev1.Route, reason, mess
 }
 
 // performIngressConditionUpdate updates the route to the appropriate status for the provided condition.
-func performIngressConditionUpdate(action string, lease writerlease.Lease, tracker ContentionTracker, oc client.RoutesGetter, lister routelisters.RouteLister, route *routev1.Route, routerName, hostName string, condition routev1.RouteIngressCondition) {
+func performIngressConditionUpdate(action string, eventType watch.EventType, lease writerlease.Lease, tracker ContentionTracker, oc client.RoutesGetter, lister routelisters.RouteLister, route *routev1.Route, routerName, hostName string, condition *routev1.RouteIngressCondition) {
 	key := string(route.UID)
 	routeNamespace, routeName := route.Namespace, route.Name
 
 	lease.Try(key, func() (writerlease.WorkResult, bool) {
-		route, err := lister.Routes(routeNamespace).Get(routeName)
-		if err != nil {
-			return writerlease.None, false
+		var routeUpdated *routev1.Route
+		var err error
+		switch eventType {
+		case watch.Deleted:
+			// If route has been deleted from the cache, then we can't use the cache to retrieve it, but we still want
+			// to process it further to remove the admitted status.
+			if routeUpdated, err = oc.Routes(route.Namespace).Get(context.TODO(), routeName, metav1.GetOptions{}); err != nil {
+				return writerlease.None, false
+			}
+		default:
+			// Otherwise, use the cache for all other events.
+			routeUpdated, err = lister.Routes(routeNamespace).Get(routeName)
+			if err != nil {
+				return writerlease.None, false
+			}
 		}
+		route = routeUpdated
+
 		if string(route.UID) != key {
 			log.V(4).Info("skipped update due to route UID changing (likely delete and recreate)", "action", action, "namespace", route.Namespace, "name", route.Name)
 			return writerlease.None, false
@@ -167,16 +183,22 @@ func performIngressConditionUpdate(action string, lease writerlease.Lease, track
 	})
 }
 
-// recordIngressCondition updates the matching ingress on the route (or adds a new one) with the specified
+// recordIngressCondition adds, updates, or removes the matching ingress on the route with the specified
 // condition, returning whether the route was updated or created, the time assigned to the condition, and
 // a pointer to the current ingress record.
-func recordIngressCondition(route *routev1.Route, name, hostName string, condition routev1.RouteIngressCondition) (changed, created bool, at time.Time, latest, original *routev1.RouteIngress) {
+func recordIngressCondition(route *routev1.Route, name, hostName string, condition *routev1.RouteIngressCondition) (changed, created bool, at time.Time, latest, original *routev1.RouteIngress) {
 	for i := range route.Status.Ingress {
 		existing := &route.Status.Ingress[i]
 		if existing.RouterName != name {
 			continue
 		}
 
+		// If condition is nil, remove the ingress condition.
+		if condition == nil {
+			route.Status.Ingress = append(route.Status.Ingress[:i], route.Status.Ingress[i+1:]...)
+			return true, false, time.Time{}, nil, nil
+		}
+
 		// check whether the ingress is out of date without modifying it
 		changed := existing.Host != route.Spec.Host ||
 			existing.WildcardPolicy != route.Spec.WildcardPolicy ||
@@ -185,7 +207,7 @@ func recordIngressCondition(route *routev1.Route, name, hostName string, conditi
 		existingCondition := findCondition(existing, condition.Type)
 		if existingCondition != nil {
 			condition.LastTransitionTime = existingCondition.LastTransitionTime
-			if *existingCondition != condition {
+			if *existingCondition != *condition {
 				changed = true
 			}
 		}
@@ -202,25 +224,30 @@ func recordIngressCondition(route *routev1.Route, name, hostName string, conditi
 		existing.WildcardPolicy = route.Spec.WildcardPolicy
 		existing.RouterCanonicalHostname = hostName
 		if existingCondition == nil {
-			existing.Conditions = append(existing.Conditions, condition)
+			existing.Conditions = append(existing.Conditions, *condition)
 			existingCondition = &existing.Conditions[len(existing.Conditions)-1]
 		} else {
-			*existingCondition = condition
+			*existingCondition = *condition
 		}
 		now := nowFn()
 		existingCondition.LastTransitionTime = &now
 
 		return true, false, now.Time, existing, &original
 	}
 
+	// If we didn't find a matching ingress, but condition was nil, don't do anything.
+	if condition == nil {
+		return false, false, time.Time{}, nil, nil
+	}
+
 	// add a new ingress
 	route.Status.Ingress = append(route.Status.Ingress, routev1.RouteIngress{
 		RouterName:              name,
 		Host:                    route.Spec.Host,
 		WildcardPolicy:          route.Spec.WildcardPolicy,
 		RouterCanonicalHostname: hostName,
 		Conditions: []routev1.RouteIngressCondition{
-			condition,
+			*condition,
 		},
 	})
 	ingress := &route.Status.Ingress[len(route.Status.Ingress)-1]