From 466a90997518f8a6a416fe7e2a3a68b30e0c8407 Mon Sep 17 00:00:00 2001 From: chiragkyal Date: Mon, 9 Dec 2024 21:19:00 +0530 Subject: [PATCH] StatusAdmitter will have its own lock Signed-off-by: chiragkyal --- pkg/router/controller/route_secret_manager.go | 2 -- .../controller/route_secret_manager_test.go | 20 ++----------------- pkg/router/controller/status.go | 13 ++++++++++++ 3 files changed, 15 insertions(+), 20 deletions(-) diff --git a/pkg/router/controller/route_secret_manager.go b/pkg/router/controller/route_secret_manager.go index 6d68884eb..b6c819ae3 100644 --- a/pkg/router/controller/route_secret_manager.go +++ b/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) }, } } diff --git a/pkg/router/controller/route_secret_manager_test.go b/pkg/router/controller/route_secret_manager_test.go index 6bacfbb4b..67adb215c 100644 --- a/pkg/router/controller/route_secret_manager_test.go +++ b/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 diff --git a/pkg/router/controller/status.go b/pkg/router/controller/status.go index 5f20afd11..985e3cd56 100644 --- a/pkg/router/controller/status.go +++ b/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