secret re-creation scenario with active informer

* RemoveRoute should not delete the informer (deletion will be handled by the plugin)
* utilize old secret name for modified event

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

pkg/cmd/infra/router/template.go

@@ -780,9 +780,6 @@ func (o *TemplateRouterOptions) Run(stopCh <-chan struct{}) error {
 		HTTPResponseHeaders:           o.HTTPResponseHeaders,
 		HTTPRequestHeaders:            o.HTTPRequestHeaders,
 	}
-	if o.AllowExternalCertificates {
-		pluginCfg.SecretManager = secretManager
-	}
 
 	svcFetcher := templateplugin.NewListWatchServiceLookup(kc.CoreV1(), o.ResyncInterval, o.Namespace)
 	templatePlugin, err := templateplugin.NewTemplatePlugin(pluginCfg, svcFetcher)

pkg/router/controller/route_secret_manager.go

@@ -3,6 +3,7 @@ package controller
 import (
 	"context"
 	"fmt"
+	"sync"
 
 	routev1 "github.com/openshift/api/route/v1"
 	"github.com/openshift/library-go/pkg/route/secretmanager"
@@ -29,17 +30,23 @@ type RouteSecretManager struct {
 	secretManager secretmanager.SecretManager
 	secretsGetter corev1client.SecretsGetter
 	sarClient     authorizationclient.SubjectAccessReviewInterface
+	// deletedSecrets tracks routes for which the associated secret was deleted after intial creation of the secret monitor.
+	// This helps to differentiate between a new secret creation and a recreation of a previously deleted secret.
+	// Populated inside DeleteFunc, and consumed or cleaned inside AddFunc and unregister().
+	// It is thread safe and "namespace/routeName" is used as it's key.
+	deletedSecrets sync.Map
 }
 
 // NewRouteSecretManager creates a new instance of RouteSecretManager.
 // It wraps the provided plugin and adds secret management capabilities.
 func NewRouteSecretManager(plugin router.Plugin, recorder RouteStatusRecorder, secretManager secretmanager.SecretManager, secretsGetter corev1client.SecretsGetter, sarClient authorizationclient.SubjectAccessReviewInterface) *RouteSecretManager {
 	return &RouteSecretManager{
-		plugin:        plugin,
-		recorder:      recorder,
-		secretManager: secretManager,
-		secretsGetter: secretsGetter,
-		sarClient:     sarClient,
+		plugin:         plugin,
+		recorder:       recorder,
+		secretManager:  secretManager,
+		secretsGetter:  secretsGetter,
+		sarClient:      sarClient,
+		deletedSecrets: sync.Map{},
 	}
 }
 
@@ -60,8 +67,20 @@ func (p *RouteSecretManager) Commit() error {
 }
 
 // HandleRoute manages the registration, unregistration, and validation of routes with external certificates.
+//
 // For Added events, it validates the route's external certificate configuration and registers it with the secret manager.
-// For Modified events, it first unregisters the route if it's already registered and then revalidates and registers it again.
+//
+// For Modified events, it checks if the route's external certificate configuration has changed and takes appropriate actions:
+//  1. Both the old and new routes have an external certificate:
+//     - If the external certificate has changed, it unregisters the old one and registers the new one.
+//     - If the external certificate has not changed, it revalidates and updates the in-memory TLS certificate and key.
+//  2. The new route has an external certificate, but the old one did not:
+//     - It registers the new route with the secret manager.
+//  3. The old route had an external certificate, but the new one does not:
+//     - It unregisters the old route from the secret manager.
+//  4. Neither the old nor the new route has an external certificate:
+//     - No action is taken.
+//
 // For Deleted events, it unregisters the route if it's registered.
 // Additionally, it delegates the handling of the event to the next plugin in the chain after performing the necessary actions.
 func (p *RouteSecretManager) HandleRoute(eventType watch.EventType, route *routev1.Route) error {
@@ -76,37 +95,91 @@ func (p *RouteSecretManager) HandleRoute(eventType watch.EventType, route *route
 			}
 		}
 
-	// For Modified events always unregister and reregister the route even if the TLS configuration did not change.
-	// Since the `HandleRoute()` method does not carry the old route spec,
-	// and there's no definite way to compare old and new TLS configurations,
-	// assume that the TLS configuration is always updated, necessitating re-registration.
-	// Additionally, always creating a new `secretHandler` ensures that there are no stale route specs
+	// TODO always creating a new `secretHandler` ensures that there are no stale route specs
 	// in the next plugin chain, especially when the referenced secret is updated or deleted.
 	// This prevents sending outdated routes to subsequent plugins, preserving expected functionality.
-	// TODO: Refer https://github.com/openshift/router/pull/565#discussion_r1596441128 for possible ways to improve the logic.
+	// TODO : we might need to add RouteLister()
 	case watch.Modified:
-		// unregister associated secret monitor, if registered
-		if p.secretManager.IsRouteRegistered(route.Namespace, route.Name) {
-			if err := p.secretManager.UnregisterRoute(route.Namespace, route.Name); err != nil {
-				log.Error(err, "failed to unregister route")
-				return err
+		// Determine if the route's external certificate configuration has changed
+		newHasExt := hasExternalCertificate(route)
+		oldSecret, oldHadExt := p.secretManager.LookupRouteSecret(route.Namespace, route.Name)
+
+		switch {
+		case newHasExt && oldHadExt:
+			// Both new and old routes have externalCertificate
+			log.V(6).Info("Both new and old routes have externalCertificate", "namespace", route.Namespace, "route", route.Name)
+			if oldSecret != route.Spec.TLS.ExternalCertificate.Name {
+				// ExternalCertificate is updated
+				log.V(4).Info("ExternalCertificate is updated", "namespace", route.Namespace, "route", route.Name)
+				// Unregister the old and register the new external certificate
+				if err := p.unregister(route); err != nil {
+					return err
+				}
+				if err := p.validateAndRegister(route); err != nil {
+					return err
+				}
+			} else {
+				// ExternalCertificate is not updated
+				// Revalidate and update the in-memory TLS certificate and key
+				// It is the responsibility of this plugin to ensure everything is synced properly.
+
+				// One Scenario: The user deletes the secret, causing the route's status to be updated to reject.
+				// This triggers the entire plugin chain again. Without re-validating the external certificate
+				// and re-syncing the secret here, the route could become active again and start serving
+				// the default certificate, even though its spec has an external certificate.
+				// Therefore, it is essential to re-sync the secret to ensure the plugin chain correctly handles the route.
+
+				log.V(4).Info("ExternalCertificate is not updated", "namespace", route.Namespace, "route", route.Name)
+				// re-validate
+				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, "ExternalCertificateValidationFailed", err.Error())
+					p.plugin.HandleRoute(watch.Deleted, route)
+					return err
+				}
+
+				// 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")
+					return err
+				}
+
+				// Update the tls.Certificate and tls.Key fields of the route with the data from the referenced secret.
+				route.Spec.TLS.Certificate = string(secret.Data["tls.crt"])
+				route.Spec.TLS.Key = string(secret.Data["tls.key"])
 			}
-		}
-		// register with secret monitor
-		if hasExternalCertificate(route) {
+
+		case newHasExt && !oldHadExt:
+			// New route has externalCertificate, old route did not
+			log.V(4).Info("New route has externalCertificate, old route did not", "namespace", route.Namespace, "route", route.Name)
+			// register with secret monitor
 			if err := p.validateAndRegister(route); err != nil {
 				return err
 			}
+
+		case !newHasExt && oldHadExt:
+			// Old route had externalCertificate, new route does not
+			log.V(4).Info("Old route had externalCertificate, new route does not", "namespace", route.Namespace, "route", route.Name)
+			// unregister with secret monitor
+			if err := p.unregister(route); err != nil {
+				return err
+			}
+
+		case !newHasExt && !oldHadExt:
+			// Neither new nor old route have externalCertificate
+			// Do nothing
 		}
 
 	case watch.Deleted:
 		// unregister associated secret monitor, if registered
-		if p.secretManager.IsRouteRegistered(route.Namespace, route.Name) {
-			if err := p.secretManager.UnregisterRoute(route.Namespace, route.Name); err != nil {
-				log.Error(err, "failed to unregister route")
+		if _, exists := p.secretManager.LookupRouteSecret(route.Namespace, route.Name); exists {
+			if err := p.unregister(route); err != nil {
 				return err
 			}
 		}
+
 	default:
 		return fmt.Errorf("invalid eventType %v", eventType)
 	}
@@ -151,21 +224,63 @@ func (p *RouteSecretManager) validateAndRegister(route *routev1.Route) error {
 }
 
 // generateSecretHandler creates ResourceEventHandlerFuncs to handle Add, Update, and Delete events on secrets.
-// AddFunc: Invoked when a new secret is added. It logs the addition of the secret.
+//
+// AddFunc: Invoked when a new secret is added. It logs the addition of the secret. This function also handles the
+// re-creation scenario where a previously deleted secret is added again. In such cases, it revalidates the route's
+// external certificate configuration, updates the route's TLS certificate and key, and calls the next plugin's HandleRoute method with a watch.Modified event, and then commits the changes by calling the next plugin's Commit() method.
+//
 // UpdateFunc: Invoked when an existing secret is updated. It performs validation of the route's external certificate configuration.
-// 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.
-// If the validation succeeds, it updates the route's TLS certificate and key with the new secret data and calls the next plugin's HandleRoute method with a watch.Modified event, and then the next plugin's Commit() method.
-// DeleteFunc: Invoked when the secret is deleted. It unregisters the associated route, records the route rejection, and triggers the deletion of the route by calling the HandleRoute method with a watch.Deleted event.
+// 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.
+// If the validation succeeds, it updates the route's TLS certificate and key with the new secret data and calls the next plugin's HandleRoute method with a watch.Modified event, and then commits the changes by calling the next plugin's Commit() method.
+//
+// DeleteFunc: Invoked when the secret is deleted. It logs the deletion, marks the secret as deleted for this route in the deletedSecrets map,
+// records the route rejection, and triggers the deletion of the route by calling the HandleRoute method with a watch.Deleted event. NOTE: It doesn't unregister the route.
 func (p *RouteSecretManager) generateSecretHandler(route *routev1.Route) cache.ResourceEventHandlerFuncs {
 	// secret handler
 	return cache.ResourceEventHandlerFuncs{
 
-		// AddFunc is intentionally left empty (only logs the event) because this handler is generated only after ensuring the existence of the secret.
-		// By leaving this empty, we prevent unnecessary triggering for the addition of the secret again. Additionally, GetSecret() method is called
-		// immediately after registering with the secretManager, to read the secret from the cache.
 		AddFunc: func(obj interface{}) {
 			secret := obj.(*kapi.Secret)
 			log.V(4).Info("secret added for route", "namespace", route.Namespace, "secret", secret.Name, "route", route.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.
+			// If it exists, it means the secret is being recreated. Remove the key from the map and proceed with re-registration.
+			// Otherwise, no-op (new secret creation scenario and no race condition with that flow)
+			// This helps to differentiate between a new secret creation and a re-creation of a previously deleted secret.
+			key := generateKey(route)
+			if _, deleted := p.deletedSecrets.LoadAndDelete(key); deleted {
+				log.V(4).Info("secret recreated for route", "namespace", route.Namespace, "secret", secret.Name, "route", route.Name)
+
+				// re-validate
+				// since secret re-creation will not trigger the apiserver route admission,
+				// we need to rely on the router controller for this validation.
+				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, "ExternalCertificateValidationFailed", err.Error())
+					p.plugin.HandleRoute(watch.Deleted, route)
+					return
+				}
+
+				// read the re-created secret
+				reCreatedSecret, err := p.secretManager.GetSecret(context.TODO(), route.Namespace, route.Name)
+				if err != nil {
+					log.Error(err, "failed to get referenced secret")
+					p.recorder.RecordRouteRejection(route, "ExternalCertificateGetFailed", err.Error())
+					p.plugin.HandleRoute(watch.Deleted, route)
+					return
+				}
+
+				// update tls.Certificate and tls.Key
+				route.Spec.TLS.Certificate = string(reCreatedSecret.Data["tls.crt"])
+				route.Spec.TLS.Key = string(reCreatedSecret.Data["tls.key"])
+
+				// call the next plugin with watch.Modified
+				p.plugin.HandleRoute(watch.Modified, route)
+				// commit the changes
+				p.plugin.Commit()
+			}
 		},
 
 		UpdateFunc: func(old interface{}, new interface{}) {
@@ -203,21 +318,40 @@ func (p *RouteSecretManager) generateSecretHandler(route *routev1.Route) cache.R
 
 		DeleteFunc: func(obj interface{}) {
 			secret := obj.(*kapi.Secret)
-			msg := fmt.Sprintf("secret %s deleted for route %s/%s", secret.Name, route.Namespace, route.Name)
+			key := generateKey(route)
+			msg := fmt.Sprintf("secret %s deleted for route %s", secret.Name, key)
 			log.V(4).Info(msg)
 
-			// unregister associated secret monitor
-			if err := p.secretManager.UnregisterRoute(route.Namespace, route.Name); err != nil {
-				log.Error(err, "failed to unregister route")
-			}
+			// keep the secret monitor active and mark the secret as deleted for this route.
+			p.deletedSecrets.Store(key, true)
 
 			p.recorder.RecordRouteRejection(route, "ExternalCertificateSecretDeleted", msg)
 			p.plugin.HandleRoute(watch.Deleted, route)
 		},
 	}
 }
 
+// unregister removes the route's registration with the secret manager and ensures
+// that any references to the deletedSecrets are cleaned up.
+func (p *RouteSecretManager) unregister(route *routev1.Route) error {
+	// unregister associated secret monitor
+	if err := p.secretManager.UnregisterRoute(route.Namespace, route.Name); err != nil {
+		log.Error(err, "failed to unregister route")
+		return err
+	}
+	// clean the route if present inside deletedSecrets
+	// this is required for the scenario when the associated secret is deleted, before unregistering with secretManager
+	p.deletedSecrets.Delete(generateKey(route))
+	return nil
+}
+
+// hasExternalCertificate checks whether the given route has an externalCertificate specified.
 func hasExternalCertificate(route *routev1.Route) bool {
 	tls := route.Spec.TLS
 	return tls != nil && tls.ExternalCertificate != nil && len(tls.ExternalCertificate.Name) > 0
 }
+
+// generateKey creates a unique identifier for a route
+func generateKey(route *routev1.Route) string {
+	return fmt.Sprintf("%s/%s", route.Namespace, route.Name)
+}

pkg/router/router_test.go

@@ -14,7 +14,6 @@ import (
 	"time"
 
 	routev1 "github.com/openshift/api/route/v1"
-	fakesm "github.com/openshift/library-go/pkg/route/secretmanager/fake"
 
 	projectfake "github.com/openshift/client-go/project/clientset/versioned/fake"
 	routeclient "github.com/openshift/client-go/route/clientset/versioned"
@@ -161,7 +160,6 @@ u3YLAbyW/lHhOCiZu2iAI8AbmXem9lW6Tr7p/97s0w==
 			Value:  `'"shouldn'\''t break"'`,
 			Action: "Set",
 		}},
-		SecretManager: &fakesm.SecretManager{},
 	}
 	plugin, err = templateplugin.NewTemplatePlugin(pluginCfg, svcFetcher)
 	if err != nil {

pkg/router/template/fake.go

@@ -1,9 +1,5 @@
 package templaterouter
 
-import (
-	fakesm "github.com/openshift/library-go/pkg/route/secretmanager/fake"
-)
-
 // NewFakeTemplateRouter provides an empty template router with a simple certificate manager
 // backed by a fake cert writer for testing
 func NewFakeTemplateRouter() *templateRouter {
@@ -13,7 +9,6 @@ func NewFakeTemplateRouter() *templateRouter {
 		serviceUnits:              make(map[ServiceUnitKey]ServiceUnit),
 		certManager:               fakeCertManager,
 		rateLimitedCommitFunction: nil,
-		secretManager:             &fakesm.SecretManager{},
 	}
 }
 

pkg/router/template/plugin.go

@@ -17,7 +17,6 @@ import (
 
 	routev1 "github.com/openshift/api/route/v1"
 
-	"github.com/openshift/library-go/pkg/route/secretmanager"
 	unidlingapi "github.com/openshift/router/pkg/router/unidling"
 )
 
@@ -69,7 +68,6 @@ type TemplatePluginConfig struct {
 	HTTPHeaderNameCaseAdjustments []HTTPHeaderNameCaseAdjustment
 	HTTPResponseHeaders           []HTTPHeader
 	HTTPRequestHeaders            []HTTPHeader
-	SecretManager                 secretmanager.SecretManager
 }
 
 // RouterInterface controls the interaction of the plugin with the underlying router implementation
@@ -166,7 +164,6 @@ func NewTemplatePlugin(cfg TemplatePluginConfig, lookupSvc ServiceLookup) (*Temp
 		httpHeaderNameCaseAdjustments: cfg.HTTPHeaderNameCaseAdjustments,
 		httpResponseHeaders:           cfg.HTTPResponseHeaders,
 		httpRequestHeaders:            cfg.HTTPRequestHeaders,
-		secretManager:                 cfg.SecretManager,
 	}
 	router, err := newTemplateRouter(templateRouterCfg)
 	return newDefaultTemplatePlugin(router, cfg.IncludeUDP, lookupSvc), err