OCPBUGS-77056: Revert #825 "Make external cert validation asynchronous (v2 — race condition fixes)"

hack/Makefile.debug

@@ -1,7 +1,6 @@
 # -*- mode: makefile -*-
 
 export GOOS=linux
-export GOARCH ?= amd64
 
 REGISTRY ?= quay.io
 IMAGE ?= openshift/openshift-router
@@ -12,8 +11,8 @@ REMOTE_IMAGE ?= $(REGISTRY)/$(IMAGE):$(TAG)
 OPENSHIFT_ENDPOINT ?= $(shell oc config view --minify --template '{{(index .clusters 0).cluster.server}}' | grep -o '//[^ :]*' | sed 's/^..//')
 
 new-openshift-router-image:
-	GO111MODULE=on CGO_ENABLED=0 GOOS=$(GOOS) GOARCH=$(GOARCH) GOFLAGS=-mod=vendor go build -o openshift-router -gcflags=all="-N -l" ./cmd/openshift-router
-	$(IMAGEBUILDER) build --arch $(GOARCH) -t $(LOCAL_IMAGE) -f hack/Dockerfile.debug .
+	GO111MODULE=on CGO_ENABLED=0 GOFLAGS=-mod=vendor go build -o openshift-router -gcflags=all="-N -l" ./cmd/openshift-router
+	$(IMAGEBUILDER) build -t $(LOCAL_IMAGE) -f hack/Dockerfile.debug .
 
 push:
 	$(IMAGEBUILDER) tag $(LOCAL_IMAGE) $(REMOTE_IMAGE)

pkg/cmd/infra/router/clientcmd.go

@@ -60,13 +60,6 @@ func (cfg *Config) KubeConfig() (*restclient.Config, string, error) {
 	if err != nil {
 		return nil, "", err
 	}
-
-	// Increase client-side rate limiting to support higher throughput during
-	// router startup, especially when many external certificate routes are
-	// present.
-	clientConfig.QPS = 50
-	clientConfig.Burst = 100
-
 	return clientConfig, namespace, nil
 }
 

pkg/cmd/infra/router/template.go

@@ -37,13 +37,13 @@ import (
 	routelisters "github.com/openshift/client-go/route/listers/route/v1"
 	"github.com/openshift/library-go/pkg/crypto"
 	"github.com/openshift/library-go/pkg/proc"
+	"github.com/openshift/library-go/pkg/route/secretmanager"
 
 	"github.com/openshift/router/pkg/router"
 	"github.com/openshift/router/pkg/router/client"
 	"github.com/openshift/router/pkg/router/controller"
 	"github.com/openshift/router/pkg/router/metrics"
 	"github.com/openshift/router/pkg/router/metrics/haproxy"
-
 	"github.com/openshift/router/pkg/router/shutdown"
 	templateplugin "github.com/openshift/router/pkg/router/template"
 	haproxyconfigmanager "github.com/openshift/router/pkg/router/template/configmanager/haproxy"
@@ -770,7 +770,7 @@ func (o *TemplateRouterOptions) Run(stopCh <-chan struct{}) error {
 		return err
 	}
 
-	secretManager := controller.NewSharedSecretManager(kc, nil)
+	secretManager := secretmanager.NewManager(kc, nil)
 
 	pluginCfg := templateplugin.TemplatePluginConfig{
 		AppCtx:                        ctx,
@@ -816,7 +816,7 @@ func (o *TemplateRouterOptions) Run(stopCh <-chan struct{}) error {
 	informer := factory.CreateRoutesSharedInformer()
 	routeLister := routelisters.NewRouteLister(informer.GetIndexer())
 	if o.UpdateStatus {
-		lease := writerlease.New(time.Minute, 3*time.Second, 1)
+		lease := writerlease.New(time.Minute, 3*time.Second)
 		go lease.Run(stopCh)
 		tracker := controller.NewSimpleContentionTracker(informer, o.RouterName, o.ResyncInterval/10)
 		tracker.SetConflictMessage(fmt.Sprintf("The router detected another process is writing conflicting updates to route status with name %q. Please ensure that the configuration of all routers is consistent. Route status will not be updated as long as conflicts are detected.", o.RouterName))

pkg/router/controller/contention.go

@@ -46,7 +46,6 @@ var (
 		ExtCrtStatusReasonSecretRecreated,
 		ExtCrtStatusReasonSecretUpdated,
 		ExtCrtStatusReasonSecretDeleted,
-		ExtCrtStatusReasonSARCompleted,
 	)
 )
 
@@ -295,10 +294,8 @@ func ingressConditionsEqual(a, b []routev1.RouteIngressCondition) bool {
 
 // conditionsEqual compares two RouteIngressConditions, ignoring LastTransitionTime and any reason in ignoreIngressConditionReason.
 func conditionsEqual(a, b *routev1.RouteIngressCondition) bool {
-	if a.Type == b.Type && a.Status == b.Status {
-		if ignoreIngressConditionReason.Has(a.Reason) || ignoreIngressConditionReason.Has(b.Reason) {
-			return true
-		}
+	if ignoreIngressConditionReason.Has(a.Reason) || ignoreIngressConditionReason.Has(b.Reason) {
+		return true
 	}
 	return a.Type == b.Type &&
 		a.Status == b.Status &&

pkg/router/controller/factory/factory_endpointslices_test.go

@@ -2,6 +2,7 @@ package factory_test
 
 import (
 	"context"
+	"os"
 	"testing"
 	"time"
 
@@ -76,15 +77,15 @@ func protocolPtr(p kapi.Protocol) *kapi.Protocol {
 	return &p
 }
 
-func newEndpointSliceTestSetup(t *testing.T, plugin router.Plugin, initialObjects ...runtime.Object) (*fakekubeclient.Clientset, chan struct{}) {
+func newEndpointSliceTestSetup(plugin router.Plugin, initialObjects ...runtime.Object) (*fakekubeclient.Clientset, chan struct{}) {
 	stopCh := make(chan struct{})
 	client := fakekubeclient.NewSimpleClientset(initialObjects...)
 	fakeProject := &fakeproject.FakeProjectV1{}
 
 	// WatchListClient featuregate is enabled by default since v0.35. Fake client does not support
 	// initializing its cache from Watch, so falling back to use List instead. The envvar below
 	// configures the featuregate state.
-	t.Setenv("KUBE_FEATURE_"+string(features.WatchListClient), "False")
+	os.Setenv("KUBE_FEATURE_"+string(features.WatchListClient), "False")
 
 	factory.NewDefaultRouterControllerFactory(
 		fakerouterclient.NewSimpleClientset(),
@@ -103,7 +104,7 @@ func TestEndpointSlicesAdd(t *testing.T) {
 		handleEndpointsCh: make(chan handleEndpointsEvent),
 	}
 
-	client, stopCh := newEndpointSliceTestSetup(t, plugin)
+	client, stopCh := newEndpointSliceTestSetup(plugin)
 	defer close(stopCh)
 
 	type testCase struct {
@@ -338,7 +339,7 @@ func TestEndpointSlicesDelete(t *testing.T) {
 		handleEndpointsCh: make(chan handleEndpointsEvent),
 	}
 
-	client, stopCh := newEndpointSliceTestSetup(t, plugin)
+	client, stopCh := newEndpointSliceTestSetup(plugin)
 	defer close(stopCh)
 
 	for _, eps := range []discoveryv1.EndpointSlice{eps1, eps2} {

pkg/router/controller/route_secret_manager.go

@@ -25,7 +25,6 @@ const (
 	ExtCrtStatusReasonSecretUpdated    = "ExternalCertificateSecretUpdated"
 	ExtCrtStatusReasonSecretDeleted    = "ExternalCertificateSecretDeleted"
 	ExtCrtStatusReasonGetFailed        = "ExternalCertificateGetFailed"
-	ExtCrtStatusReasonSARCompleted     = "ExternalCertificateSARCompleted"
 )
 
 // RouteSecretManager implements the router.Plugin interface to register
@@ -108,15 +107,6 @@ func (p *RouteSecretManager) Commit() error {
 func (p *RouteSecretManager) HandleRoute(eventType watch.EventType, route *routev1.Route) error {
 	log.V(10).Info("HandleRoute: RouteSecretManager", "eventType", eventType)
 
-	// DeepCopy the route before any mutation. The route pointer may come from
-	// the informer cache (via the lister), which is shared across goroutines.
-	// populateRouteTLSFromSecret writes Certificate and Key in-place, which
-	// would race with informer goroutines that read the same object (e.g.,
-	// secret handler UpdateFunc calling route.DeepCopy()).
-	if hasExternalCertificate(route) {
-		route = route.DeepCopy()
-	}
-
 	switch eventType {
 	case watch.Added:
 		// register with secret monitor
@@ -167,15 +157,14 @@ func (p *RouteSecretManager) HandleRoute(eventType watch.EventType, route *route
 				// Therefore, it is essential to re-sync the secret to ensure the plugin chain correctly handles the route.
 
 				log.V(4).Info("Re-validating existing external certificate", "namespace", route.Namespace, "secret", oldSecret, "route", route.Name)
-				// re-validate (synchronous, throttled by semaphore)
+				// re-validate
 				if err := p.validate(route); err != nil {
 					return err
 				}
 				// read referenced secret and update TLS certificate and key
 				if err := p.populateRouteTLSFromSecret(route); err != nil {
 					return err
 				}
-
 			}
 
 		case newHasExt && !oldHadExt:
@@ -209,27 +198,13 @@ func (p *RouteSecretManager) HandleRoute(eventType watch.EventType, route *route
 	}
 
 	// call next plugin
-	err := p.plugin.HandleRoute(eventType, route)
-
-	// If the route was accepted by the downstream plugin chain and it has an external certificate
-	// that we successfully validated in this pass, emit the SARCompleted status — but only if the
-	// route doesn't already have an ext-cert admitted reason. Without this guard, every HandleRoute
-	// writes SARCompleted, which triggers a status update, which re-enqueues the route, which
-	// triggers another HandleRoute — a feedback loop that doubles cert writes and HAProxy reloads.
-	if err == nil && hasExternalCertificate(route) && (eventType == watch.Added || eventType == watch.Modified) {
-		if !hasExtCertAdmittedReason(route, p.routerName) {
-			msg := fmt.Sprintf("SAR check and secret load completed for secret %q", route.Spec.TLS.ExternalCertificate.Name)
-			p.recorder.RecordRouteUpdate(route, ExtCrtStatusReasonSARCompleted, msg)
-		}
-	}
-
-	return err
+	return p.plugin.HandleRoute(eventType, route)
 }
 
 // validateAndRegister validates the route's externalCertificate configuration and registers it with the secret manager.
 // It also updates the in-memory TLS certificate and key after reading from secret informer's cache.
 func (p *RouteSecretManager) validateAndRegister(route *routev1.Route) error {
-	// validate (synchronous, throttled by semaphore)
+	// validate
 	if err := p.validate(route); err != nil {
 		return err
 	}
@@ -238,7 +213,6 @@ func (p *RouteSecretManager) validateAndRegister(route *routev1.Route) error {
 	if err := p.secretManager.RegisterRoute(context.TODO(), route.Namespace, route.Name, route.Spec.TLS.ExternalCertificate.Name, handler); err != nil {
 		return fmt.Errorf("failed to register router: %w", err)
 	}
-
 	// read referenced secret and update TLS certificate and key
 	if err := p.populateRouteTLSFromSecret(route); err != nil {
 		return err
@@ -286,7 +260,6 @@ func (p *RouteSecretManager) generateSecretHandler(namespace, routeName string)
 		AddFunc: func(obj interface{}) {
 			secret := obj.(*kapi.Secret)
 			log.V(4).Info("Secret added for route", "namespace", namespace, "secret", secret.Name, "route", routeName)
-			routeapihelpers.InvalidateAsyncSARCache(namespace, secret.Name)
 
 			// Secret re-creation scenario
 			// Check if the route key exists in the deletedSecrets map, indicating that the secret was previously deleted for this route.
@@ -297,14 +270,12 @@ func (p *RouteSecretManager) generateSecretHandler(namespace, routeName string)
 			if _, deleted := p.deletedSecrets.LoadAndDelete(key); deleted {
 				log.V(4).Info("Secret recreated for route", "namespace", namespace, "secret", secret.Name, "route", routeName)
 
-				// Ensure fetching the updated route and DeepCopy to avoid
-				// reading/writing the shared informer cache object.
+				// Ensure fetching the updated route
 				route, err := p.routelister.Routes(namespace).Get(routeName)
 				if err != nil {
 					log.Error(err, "failed to get route", "namespace", namespace, "route", routeName)
 					return
 				}
-				route = route.DeepCopy()
 
 				// The route should *remain* rejected until it's re-evaluated
 				// by all the plugins (including this plugin). Once passes, the route will become active again.
@@ -318,61 +289,43 @@ func (p *RouteSecretManager) generateSecretHandler(namespace, routeName string)
 			secretNew := new.(*kapi.Secret)
 			key := generateKey(namespace, routeName)
 			log.V(4).Info("Secret updated for route", "namespace", namespace, "secret", secretNew.Name, "oldSecretVersion", secretOld.ResourceVersion, "newSecretVersion", secretNew.ResourceVersion, "route", routeName)
-			routeapihelpers.InvalidateAsyncSARCache(namespace, secretNew.Name)
 
-			// Ensure fetching the updated route and DeepCopy to avoid
-			// reading/writing the shared informer cache object.
+			// Ensure fetching the updated route
 			route, err := p.routelister.Routes(namespace).Get(routeName)
 			if err != nil {
 				log.Error(err, "failed to get route", "namespace", namespace, "route", routeName)
 				return
 			}
-			route = route.DeepCopy()
 
 			msg := fmt.Sprintf("secret %q updated for route %q (oldSecretVersion=%v, newSecretVersion=%v)", secretNew.Name, key, secretOld.ResourceVersion, secretNew.ResourceVersion)
 			// Update the route status to notify plugins, including this plugin, for re-evaluation.
 			// - If the route is admitted (Admitted=True), record an update event.
 			// - If the route is not admitted, record a rejection event (keep it rejected).
-			if isRouteAdmittedTrue(route, p.routerName) {
+			if isRouteAdmittedTrue(route.DeepCopy(), p.routerName) {
 				p.recorder.RecordRouteUpdate(route, ExtCrtStatusReasonSecretUpdated, msg)
 			} else {
 				p.recorder.RecordRouteRejection(route, ExtCrtStatusReasonSecretUpdated, msg)
 			}
 		},
 
 		DeleteFunc: func(obj interface{}) {
-			secret, ok := obj.(*kapi.Secret)
-			if !ok {
-				tombstone, ok := obj.(cache.DeletedFinalStateUnknown)
-				if !ok {
-					log.Error(nil, "Couldn't get object from tombstone", "type", fmt.Sprintf("%T", obj))
-					return
-				}
-				secret, ok = tombstone.Obj.(*kapi.Secret)
-				if !ok {
-					log.Error(nil, "Tombstone contained object that is not a secret", "type", fmt.Sprintf("%T", tombstone.Obj))
-					return
-				}
-			}
+			secret := obj.(*kapi.Secret)
 			key := generateKey(namespace, routeName)
-			msg := fmt.Sprintf("external certificate validation failed: secret %q deleted for route %q", secret.Name, key)
+			msg := fmt.Sprintf("secret %q deleted for route %q", secret.Name, key)
 			log.V(4).Info(msg)
-			routeapihelpers.InvalidateAsyncSARCache(namespace, secret.Name)
 
 			// keep the secret monitor active and mark the secret as deleted for this route.
 			p.deletedSecrets.Store(key, true)
 
-			// Ensure fetching the updated route and DeepCopy to avoid
-			// reading/writing the shared informer cache object.
+			// Ensure fetching the updated route
 			route, err := p.routelister.Routes(namespace).Get(routeName)
 			if err != nil {
 				log.Error(err, "failed to get route", "namespace", namespace, "route", routeName)
 				return
 			}
-			route = route.DeepCopy()
 
 			// Reject this route
-			p.recorder.RecordRouteRejection(route, ExtCrtStatusReasonValidationFailed, msg)
+			p.recorder.RecordRouteRejection(route, ExtCrtStatusReasonSecretDeleted, msg)
 		},
 	}
 }
@@ -381,14 +334,10 @@ func (p *RouteSecretManager) generateSecretHandler(namespace, routeName string)
 // If the validation fails, it records the route rejection and triggers
 // the deletion of the route by calling the HandleRoute method with a watch.Deleted event.
 //
-// This function is synchronous: it blocks until the SAR check completes.
-// Concurrency is throttled by the semaphore in ValidateTLSExternalCertificate.
-//
 // NOTE: TLS data validation and sanitization are handled by the next plugin `ExtendedValidator`,
 // by reading the "tls.crt" and "tls.key" added by populateRouteTLSFromSecret.
 func (p *RouteSecretManager) validate(route *routev1.Route) error {
 	fldPath := field.NewPath("spec").Child("tls").Child("externalCertificate")
-
 	if err := routeapihelpers.ValidateTLSExternalCertificate(route, fldPath, p.sarClient, p.secretsGetter).ToAggregate(); err != nil {
 		log.Error(err, "skipping route due to invalid externalCertificate configuration", "namespace", route.Namespace, "route", route.Name)
 		p.recorder.RecordRouteRejection(route, ExtCrtStatusReasonValidationFailed, err.Error())
@@ -403,9 +352,7 @@ func (p *RouteSecretManager) validate(route *routev1.Route) error {
 // the deletion of the route by calling the HandleRoute method with a watch.Deleted event.
 // Note: This function performs an in-place update of the route. The caller should be aware that the route's TLS configuration will be modified directly.
 func (p *RouteSecretManager) populateRouteTLSFromSecret(route *routev1.Route) error {
-	// read referenced secret from the informer cache.
-	// GetSecret attempts to read from the cache and falls back to a direct API
-	// call if the cache is not synced or the secret is not found.
+	// read referenced secret
 	secret, err := p.secretManager.GetSecret(context.TODO(), route.Namespace, route.Name)
 	if err != nil {
 		log.Error(err, "failed to get referenced secret")
@@ -449,29 +396,6 @@ func generateKey(namespace, routeName string) string {
 	return fmt.Sprintf("%s/%s", namespace, routeName)
 }
 
-// hasExtCertAdmittedReason returns true if the route already has an
-// Admitted=True condition with an external-certificate reason (e.g.,
-// SARCompleted, SecretUpdated). Used to avoid redundant SARCompleted
-// status writes that would create a re-enqueue feedback loop.
-func hasExtCertAdmittedReason(route *routev1.Route, routerName string) bool {
-	for _, ingress := range route.Status.Ingress {
-		if ingress.RouterName != routerName {
-			continue
-		}
-		for _, condition := range ingress.Conditions {
-			if condition.Type == routev1.RouteAdmitted && condition.Status == kapi.ConditionTrue {
-				switch condition.Reason {
-				case ExtCrtStatusReasonSARCompleted,
-					ExtCrtStatusReasonSecretUpdated,
-					ExtCrtStatusReasonSecretRecreated:
-					return true
-				}
-			}
-		}
-	}
-	return false
-}
-
 // isRouteAdmittedTrue returns true if the given route has been admitted
 // by the current router, otherwise false.
 func isRouteAdmittedTrue(route *routev1.Route, routerName string) bool {