From 766a4c50946f17763b3b822414cd81e785174a21 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 Signed-off-by: chiragkyal RemoveRoute should not delete the informer (deletion will be handled by the plugin) Signed-off-by: chiragkyal --- pkg/cmd/infra/router/template.go | 3 - pkg/router/controller/route_secret_manager.go | 106 ++++++++++++++---- 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, 84 insertions(+), 49 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..08e1f57a3 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 (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,13 +247,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.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) @@ -217,7 +260,26 @@ func (p *RouteSecretManager) generateSecretHandler(route *routev1.Route) cache.R } } +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 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",