secret re-creation scenario with active informer

Signed-off-by: chiragkyal <[email protected]> RemoveRoute should not delete the informer (deletion will be handled by the plugin) Signed-off-by: chiragkyal <[email protected]>
openshift · Jul 23, 2024 · 766a4c5 · 766a4c5
1 parent 9a7198a
commit 766a4c5
Show file tree

Hide file tree

Showing 7 changed files with 84 additions and 49 deletions.
diff --git a/pkg/cmd/infra/router/template.go b/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)

diff --git a/pkg/router/controller/route_secret_manager.go b/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 (populated inside DeleteFunc)
+	// after intial creation of the secret monitor. This helps to differentiate between a new secret creation
+	// and a recreation of a previously deleted secret.
+	// 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{},
 	}
 }
 
@@ -85,13 +92,11 @@ func (p *RouteSecretManager) HandleRoute(eventType watch.EventType, route *route
 	// 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.
 	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
-			}
+		// unregister with secret monitor
+		if err := p.unregister(route); err != nil {
+			return err
 		}
+
 		// register with secret monitor
 		if hasExternalCertificate(route) {
 			if err := p.validateAndRegister(route); err != nil {
@@ -100,13 +105,11 @@ func (p *RouteSecretManager) HandleRoute(eventType watch.EventType, route *route
 		}
 
 	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")
-				return err
-			}
+		// unregister with secret monitor
+		if err := p.unregister(route); err != nil {
+			return err
 		}
+
 	default:
 		return fmt.Errorf("invalid eventType %v", eventType)
 	}
@@ -166,6 +169,47 @@ func (p *RouteSecretManager) generateSecretHandler(route *routev1.Route) cache.R
 		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 i.e check if the secret was deleted previously for this route.
+			// If it exists, it means the secret is being recreated. Remove the key from the map and proceed with re-registration. Else no-op (new secret creation).
+			// this helps to differentiate between a new secret creation and a recreation of a previously deleted secret.
+			key := generateKey(route.Namespace, route.Name)
+			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 apiserver route admission,
+				// we need to rely on 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())
+					// route should be already deleted in this case
+					// 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())
+					// route should be already deleted in this case
+					// 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 +247,39 @@ 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.Namespace, route.Name)
+			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)
 		},
 	}
 }
 
+func (p *RouteSecretManager) unregister(route *routev1.Route) error {
+	// 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
+		}
+		// 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.Namespace, route.Name))
+	}
+	return nil
+}
+
 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(namespace, route string) string {
+	return fmt.Sprintf("%s/%s", namespace, route)
+}
diff --git a/pkg/router/router_test.go b/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 {

diff --git a/pkg/router/template/fake.go b/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{},
 	}
 }
 

diff --git a/pkg/router/template/plugin.go b/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

diff --git a/pkg/router/template/router.go b/pkg/router/template/router.go
@@ -22,7 +22,6 @@ import (
 	"k8s.io/apimachinery/pkg/util/sets"
 
 	routev1 "github.com/openshift/api/route/v1"
-	"github.com/openshift/library-go/pkg/route/secretmanager"
 
 	logf "github.com/openshift/router/log"
 	"github.com/openshift/router/pkg/router/crl"
@@ -65,7 +64,6 @@ type templateRouter struct {
 	state            map[ServiceAliasConfigKey]ServiceAliasConfig
 	serviceUnits     map[ServiceUnitKey]ServiceUnit
 	certManager      certificateManager
-	secretManager    secretmanager.SecretManager
 	// defaultCertificate is a concatenated certificate(s), their keys, and their CAs that should be used by the underlying
 	// implementation as the default certificate if no certificate is resolved by the normal matching mechanisms.  This is
 	// usually a wildcard certificate for a cloud domain such as *.mypaas.com to allow applications to create app.mypaas.com
@@ -157,7 +155,6 @@ type templateRouterCfg struct {
 	httpHeaderNameCaseAdjustments []HTTPHeaderNameCaseAdjustment
 	httpResponseHeaders           []HTTPHeader
 	httpRequestHeaders            []HTTPHeader
-	secretManager                 secretmanager.SecretManager
 }
 
 // templateConfig is a subset of the templateRouter information that should be passed to the template for generating
@@ -256,7 +253,6 @@ func newTemplateRouter(cfg templateRouterCfg) (*templateRouter, error) {
 		state:                         make(map[ServiceAliasConfigKey]ServiceAliasConfig),
 		serviceUnits:                  make(map[ServiceUnitKey]ServiceUnit),
 		certManager:                   certManager,
-		secretManager:                 cfg.secretManager,
 		defaultCertificate:            cfg.defaultCertificate,
 		defaultCertificatePath:        cfg.defaultCertificatePath,
 		defaultCertificateDir:         cfg.defaultCertificateDir,
@@ -1134,14 +1130,6 @@ func (r *templateRouter) RemoveRoute(route *routev1.Route) {
 	defer r.lock.Unlock()
 
 	r.removeRouteInternal(route)
-
-	if r.secretManager != nil { // TODO: remove this nil check while dropping AllowExternalCertificates flag.
-		if r.secretManager.IsRouteRegistered(route.Namespace, route.Name) {
-			if err := r.secretManager.UnregisterRoute(route.Namespace, route.Name); err != nil {
-				log.Error(err, "failed to unregister route")
-			}
-		}
-	}
 }
 
 // removeRouteInternal removes the given route - internal

diff --git a/pkg/router/template/router_test.go b/pkg/router/template/router_test.go
@@ -12,7 +12,6 @@ import (
 	"github.com/google/go-cmp/cmp"
 	routev1 "github.com/openshift/api/route/v1"
 
-	fakesm "github.com/openshift/library-go/pkg/route/secretmanager/fake"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/apimachinery/pkg/util/sets"
@@ -628,7 +627,6 @@ func findCert(cert string, certs map[string]Certificate, isPrivateKey bool, t *t
 // TestRemoveRoute tests removing a ServiceAliasConfig from a ServiceUnit
 func TestRemoveRoute(t *testing.T) {
 	router := NewFakeTemplateRouter()
-	router.secretManager = &fakesm.SecretManager{IsRegistered: true}
 	route := &routev1.Route{
 		ObjectMeta: metav1.ObjectMeta{
 			Namespace: "foo",