From 2f90b415e4e49434ce84728eb89bc532eb3d6f9d Mon Sep 17 00:00:00 2001 From: chiragkyal Date: Fri, 12 Jul 2024 02:01:52 +0530 Subject: [PATCH] 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 --- pkg/cmd/infra/router/template.go | 3 - pkg/router/controller/route_secret_manager.go | 204 +++++++++++++++--- pkg/router/router_test.go | 2 - pkg/router/template/fake.go | 5 - pkg/router/template/plugin.go | 3 - pkg/router/template/router.go | 12 -- pkg/router/template/router_test.go | 2 - 7 files changed, 169 insertions(+), 62 deletions(-) diff --git a/pkg/cmd/infra/router/template.go b/pkg/cmd/infra/router/template.go index 14e5b2056..7643ef818 100644 --- 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 index bbb7ff262..7b5d70ae4 100644 --- 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 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,13 +318,12 @@ 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) @@ -217,7 +331,27 @@ func (p *RouteSecretManager) generateSecretHandler(route *routev1.Route) cache.R } } +// 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) +} diff --git a/pkg/router/router_test.go b/pkg/router/router_test.go index 1b747224e..19e78e213 100644 --- 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 index 2f4560772..614db31a7 100644 --- 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 index 8d592ca28..d1633720d 100644 --- 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 index 17ccccb9f..e83363f62 100644 --- 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 index 7266957bb..9c4610d11 100644 --- 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",