StatusAdmitter will have its own lock

Signed-off-by: chiragkyal <[email protected]>

pkg/router/controller/route_secret_manager.go

@@ -315,8 +315,6 @@ func (p *RouteSecretManager) generateSecretHandler(namespace, routeName string)
 
 			// Reject this route
 			p.recorder.RecordRouteRejection(route, "ExternalCertificateSecretDeleted", msg)
-			// Stop serving this route
-			p.plugin.HandleRoute(watch.Deleted, route)
 		},
 	}
 }

pkg/router/controller/route_secret_manager_test.go

@@ -1290,11 +1290,8 @@ func TestSecretDelete(t *testing.T) {
 	recorder := &statusRecorder{
 		doneCh: make(chan struct{}),
 	}
-	p := &fakePluginDone{
-		doneCh: make(chan struct{}),
-	}
 	lister := &routeLister{items: []*routev1.Route{route}}
-	rsm := NewRouteSecretManager(p, recorder, &fake.SecretManager{}, &testSecretGetter{}, lister, &testSARCreator{})
+	rsm := NewRouteSecretManager(&fakePlugin{}, recorder, &fake.SecretManager{}, &testSecretGetter{}, lister, &testSARCreator{})
 
 	// Create a fakeSecret and start an informer for it
 	secret := fakeSecret("sandbox", "tls-secret", corev1.SecretTypeTLS, map[string][]byte{})
@@ -1317,19 +1314,10 @@ func TestSecretDelete(t *testing.T) {
 	}
 
 	<-recorder.doneCh // wait until the route's status is updated
-	<-p.doneCh        // wait until p.plugin.HandleRoute() is completed
 
-	expectedRoute := route
-	expectedEventType := watch.Deleted
 	expectedRejections := []string{"sandbox-route-test:ExternalCertificateSecretDeleted"}
 	expectedDeletedSecrets := true
 
-	if !reflect.DeepEqual(expectedRoute, p.route) {
-		t.Fatalf("expected route for next plugin %v, but got %v", expectedRoute, p.route)
-	}
-	if expectedEventType != p.eventType {
-		t.Fatalf("expected %s event for next plugin, but got %s", expectedEventType, p.eventType)
-	}
 	if !reflect.DeepEqual(expectedRejections, recorder.rejections) {
 		t.Fatalf("expected rejections %v, but got %v", expectedRejections, recorder.rejections)
 	}
@@ -1356,11 +1344,8 @@ func TestSecretRecreation(t *testing.T) {
 	recorder := &statusRecorder{
 		doneCh: make(chan struct{}),
 	}
-	p := &fakePluginDone{
-		doneCh: make(chan struct{}),
-	}
 	lister := &routeLister{items: []*routev1.Route{route}}
-	rsm := NewRouteSecretManager(p, recorder, &fake.SecretManager{}, &testSecretGetter{}, lister, &testSARCreator{})
+	rsm := NewRouteSecretManager(&fakePlugin{}, recorder, &fake.SecretManager{}, &testSecretGetter{}, lister, &testSARCreator{})
 
 	// Create a fakeSecret and start an informer for it
 	secret := fakeSecret("sandbox", "tls-secret", corev1.SecretTypeTLS, map[string][]byte{})
@@ -1383,7 +1368,6 @@ func TestSecretRecreation(t *testing.T) {
 	}
 
 	<-recorder.doneCh // wait until the route's status is updated (deletion)
-	<-p.doneCh        // wait until p.plugin.HandleRoute() is completed (deletion)
 
 	// re-create the secret
 	recorder.doneCh = make(chan struct{}) // need a new doneCh for re-creation

pkg/router/controller/status.go

@@ -3,6 +3,7 @@ package controller
 import (
 	"context"
 	"fmt"
+	"sync"
 	"time"
 
 	corev1 "k8s.io/api/core/v1"
@@ -64,6 +65,7 @@ func (logRecorder) RecordRouteUnservableInFutureVersionsClear(route *routev1.Rou
 
 // StatusAdmitter ensures routes added to the plugin have status set.
 type StatusAdmitter struct {
+	lock   sync.Mutex
 	plugin router.Plugin
 	client client.RoutesGetter
 	lister routelisters.RouteLister
@@ -81,6 +83,7 @@ type StatusAdmitter struct {
 // with differing configurations are writing updates at the same time.
 func NewStatusAdmitter(plugin router.Plugin, client client.RoutesGetter, lister routelisters.RouteLister, name, hostName string, lease writerlease.Lease, tracker ContentionTracker) *StatusAdmitter {
 	return &StatusAdmitter{
+		lock:   sync.Mutex{},
 		plugin: plugin,
 		client: client,
 		lister: lister,
@@ -104,6 +107,8 @@ var nowFn = getRfc3339Timestamp
 
 // HandleRoute attempts to admit the provided route on watch add / modifications.
 func (a *StatusAdmitter) HandleRoute(eventType watch.EventType, route *routev1.Route) error {
+	a.lock.Lock()
+	defer a.lock.Unlock()
 	log.V(10).Info("HandleRoute: StatusAdmitter")
 	switch eventType {
 	case watch.Added, watch.Modified:
@@ -135,6 +140,8 @@ func (a *StatusAdmitter) Commit() error {
 // This function is intended to be used to signal route updates, keeping `Admitted=True` to
 // provide information about the change.
 func (a *StatusAdmitter) RecordRouteUpdate(route *routev1.Route, reason, message string) {
+	a.lock.Lock()
+	defer a.lock.Unlock()
 	performIngressConditionUpdate("admit", a.lease, a.tracker, a.client, a.lister, route, a.routerName, a.routerCanonicalHostname, routev1.RouteIngressCondition{
 		Type:    routev1.RouteAdmitted,
 		Status:  corev1.ConditionTrue,
@@ -145,6 +152,8 @@ func (a *StatusAdmitter) RecordRouteUpdate(route *routev1.Route, reason, message
 
 // 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) {
+	a.lock.Lock()
+	defer a.lock.Unlock()
 	performIngressConditionUpdate("reject", a.lease, a.tracker, a.client, a.lister, route, a.routerName, a.routerCanonicalHostname, routev1.RouteIngressCondition{
 		Type:    routev1.RouteAdmitted,
 		Status:  corev1.ConditionFalse,
@@ -156,6 +165,8 @@ func (a *StatusAdmitter) RecordRouteRejection(route *routev1.Route, reason, mess
 // RecordRouteUnservableInFutureVersions attempts to update the route status with a
 // reason for a route being unservable in future versions.
 func (a *StatusAdmitter) RecordRouteUnservableInFutureVersions(route *routev1.Route, reason, message string) {
+	a.lock.Lock()
+	defer a.lock.Unlock()
 	expectedCondition := routev1.RouteIngressCondition{
 		Type:    routev1.RouteUnservableInFutureVersions,
 		Status:  corev1.ConditionTrue,
@@ -179,6 +190,8 @@ func (a *StatusAdmitter) RecordRouteUnservableInFutureVersions(route *routev1.Ro
 
 // RecordRouteUnservableInFutureVersionsClear clears the UnservableInFutureVersions status back to an unset state.
 func (a *StatusAdmitter) RecordRouteUnservableInFutureVersionsClear(route *routev1.Route) {
+	a.lock.Lock()
+	defer a.lock.Unlock()
 	// First, verify if updates are required by checking if the route ingress status matches expected values.
 	// This helps avoid unnecessary tasks in the writerlease queue and prevents unneeded lease extensions.
 	// We only do this in for the new UnservableInFutureVersions condition to avoid perturbing existing logic for the