Skip to content

Commit

Permalink
secret re-creation scenario with active informer
Browse files Browse the repository at this point in the history
* 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]>
  • Loading branch information
chiragkyal committed Aug 27, 2024
1 parent 9a7198a commit 90831c8
Show file tree
Hide file tree
Showing 7 changed files with 169 additions and 62 deletions.
3 changes: 0 additions & 3 deletions pkg/cmd/infra/router/template.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
204 changes: 169 additions & 35 deletions pkg/router/controller/route_secret_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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{},
}
}

Expand All @@ -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 {
Expand All @@ -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)
}
Expand Down Expand Up @@ -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{}) {
Expand Down Expand Up @@ -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)
}
2 changes: 0 additions & 2 deletions pkg/router/router_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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 {
Expand Down
5 changes: 0 additions & 5 deletions pkg/router/template/fake.go
Original file line number Diff line number Diff line change
@@ -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 {
Expand All @@ -13,7 +9,6 @@ func NewFakeTemplateRouter() *templateRouter {
serviceUnits: make(map[ServiceUnitKey]ServiceUnit),
certManager: fakeCertManager,
rateLimitedCommitFunction: nil,
secretManager: &fakesm.SecretManager{},
}
}

Expand Down
3 changes: 0 additions & 3 deletions pkg/router/template/plugin.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
Loading

0 comments on commit 90831c8

Please sign in to comment.