diff --git a/hack/Makefile.debug b/hack/Makefile.debug index d0d1c0e34..638f4d4f5 100644 --- a/hack/Makefile.debug +++ b/hack/Makefile.debug @@ -1,6 +1,7 @@ # -*- mode: makefile -*- export GOOS=linux +export GOARCH ?= amd64 REGISTRY ?= quay.io IMAGE ?= openshift/openshift-router @@ -11,8 +12,8 @@ REMOTE_IMAGE ?= $(REGISTRY)/$(IMAGE):$(TAG) OPENSHIFT_ENDPOINT ?= $(shell oc config view --minify --template '{{(index .clusters 0).cluster.server}}' | grep -o '//[^ :]*' | sed 's/^..//') new-openshift-router-image: - GO111MODULE=on CGO_ENABLED=0 GOFLAGS=-mod=vendor go build -o openshift-router -gcflags=all="-N -l" ./cmd/openshift-router - $(IMAGEBUILDER) build -t $(LOCAL_IMAGE) -f hack/Dockerfile.debug . + GO111MODULE=on CGO_ENABLED=0 GOOS=$(GOOS) GOARCH=$(GOARCH) GOFLAGS=-mod=vendor go build -o openshift-router -gcflags=all="-N -l" ./cmd/openshift-router + $(IMAGEBUILDER) build --arch $(GOARCH) -t $(LOCAL_IMAGE) -f hack/Dockerfile.debug . push: $(IMAGEBUILDER) tag $(LOCAL_IMAGE) $(REMOTE_IMAGE) diff --git a/pkg/cmd/infra/router/clientcmd.go b/pkg/cmd/infra/router/clientcmd.go index d3d59135c..ddad1f314 100644 --- a/pkg/cmd/infra/router/clientcmd.go +++ b/pkg/cmd/infra/router/clientcmd.go @@ -60,6 +60,13 @@ func (cfg *Config) KubeConfig() (*restclient.Config, string, error) { if err != nil { return nil, "", err } + + // Increase client-side rate limiting to support higher throughput during + // router startup, especially when many external certificate routes are + // present. + clientConfig.QPS = 50 + clientConfig.Burst = 100 + return clientConfig, namespace, nil } diff --git a/pkg/cmd/infra/router/template.go b/pkg/cmd/infra/router/template.go index 6733ae0a3..350164efc 100644 --- a/pkg/cmd/infra/router/template.go +++ b/pkg/cmd/infra/router/template.go @@ -37,13 +37,13 @@ import ( routelisters "github.com/openshift/client-go/route/listers/route/v1" "github.com/openshift/library-go/pkg/crypto" "github.com/openshift/library-go/pkg/proc" - "github.com/openshift/library-go/pkg/route/secretmanager" "github.com/openshift/router/pkg/router" "github.com/openshift/router/pkg/router/client" "github.com/openshift/router/pkg/router/controller" "github.com/openshift/router/pkg/router/metrics" "github.com/openshift/router/pkg/router/metrics/haproxy" + "github.com/openshift/router/pkg/router/shutdown" templateplugin "github.com/openshift/router/pkg/router/template" haproxyconfigmanager "github.com/openshift/router/pkg/router/template/configmanager/haproxy" @@ -770,7 +770,7 @@ func (o *TemplateRouterOptions) Run(stopCh <-chan struct{}) error { return err } - secretManager := secretmanager.NewManager(kc, nil) + secretManager := controller.NewSharedSecretManager(kc, nil) pluginCfg := templateplugin.TemplatePluginConfig{ AppCtx: ctx, @@ -816,7 +816,7 @@ func (o *TemplateRouterOptions) Run(stopCh <-chan struct{}) error { informer := factory.CreateRoutesSharedInformer() routeLister := routelisters.NewRouteLister(informer.GetIndexer()) if o.UpdateStatus { - lease := writerlease.New(time.Minute, 3*time.Second) + lease := writerlease.New(time.Minute, 3*time.Second, 1) go lease.Run(stopCh) tracker := controller.NewSimpleContentionTracker(informer, o.RouterName, o.ResyncInterval/10) tracker.SetConflictMessage(fmt.Sprintf("The router detected another process is writing conflicting updates to route status with name %q. Please ensure that the configuration of all routers is consistent. Route status will not be updated as long as conflicts are detected.", o.RouterName)) diff --git a/pkg/router/controller/contention.go b/pkg/router/controller/contention.go index 42aa32b9d..21ec86c51 100644 --- a/pkg/router/controller/contention.go +++ b/pkg/router/controller/contention.go @@ -46,6 +46,7 @@ var ( ExtCrtStatusReasonSecretRecreated, ExtCrtStatusReasonSecretUpdated, ExtCrtStatusReasonSecretDeleted, + ExtCrtStatusReasonSARCompleted, ) ) @@ -294,8 +295,10 @@ func ingressConditionsEqual(a, b []routev1.RouteIngressCondition) bool { // conditionsEqual compares two RouteIngressConditions, ignoring LastTransitionTime and any reason in ignoreIngressConditionReason. func conditionsEqual(a, b *routev1.RouteIngressCondition) bool { - if ignoreIngressConditionReason.Has(a.Reason) || ignoreIngressConditionReason.Has(b.Reason) { - return true + if a.Type == b.Type && a.Status == b.Status { + if ignoreIngressConditionReason.Has(a.Reason) || ignoreIngressConditionReason.Has(b.Reason) { + return true + } } return a.Type == b.Type && a.Status == b.Status && diff --git a/pkg/router/controller/factory/factory_endpointslices_test.go b/pkg/router/controller/factory/factory_endpointslices_test.go index 444e2c7e2..04487836d 100644 --- a/pkg/router/controller/factory/factory_endpointslices_test.go +++ b/pkg/router/controller/factory/factory_endpointslices_test.go @@ -2,7 +2,6 @@ package factory_test import ( "context" - "os" "testing" "time" @@ -77,7 +76,7 @@ func protocolPtr(p kapi.Protocol) *kapi.Protocol { return &p } -func newEndpointSliceTestSetup(plugin router.Plugin, initialObjects ...runtime.Object) (*fakekubeclient.Clientset, chan struct{}) { +func newEndpointSliceTestSetup(t *testing.T, plugin router.Plugin, initialObjects ...runtime.Object) (*fakekubeclient.Clientset, chan struct{}) { stopCh := make(chan struct{}) client := fakekubeclient.NewSimpleClientset(initialObjects...) fakeProject := &fakeproject.FakeProjectV1{} @@ -85,7 +84,7 @@ func newEndpointSliceTestSetup(plugin router.Plugin, initialObjects ...runtime.O // WatchListClient featuregate is enabled by default since v0.35. Fake client does not support // initializing its cache from Watch, so falling back to use List instead. The envvar below // configures the featuregate state. - os.Setenv("KUBE_FEATURE_"+string(features.WatchListClient), "False") + t.Setenv("KUBE_FEATURE_"+string(features.WatchListClient), "False") factory.NewDefaultRouterControllerFactory( fakerouterclient.NewSimpleClientset(), @@ -104,7 +103,7 @@ func TestEndpointSlicesAdd(t *testing.T) { handleEndpointsCh: make(chan handleEndpointsEvent), } - client, stopCh := newEndpointSliceTestSetup(plugin) + client, stopCh := newEndpointSliceTestSetup(t, plugin) defer close(stopCh) type testCase struct { @@ -339,7 +338,7 @@ func TestEndpointSlicesDelete(t *testing.T) { handleEndpointsCh: make(chan handleEndpointsEvent), } - client, stopCh := newEndpointSliceTestSetup(plugin) + client, stopCh := newEndpointSliceTestSetup(t, plugin) defer close(stopCh) for _, eps := range []discoveryv1.EndpointSlice{eps1, eps2} { diff --git a/pkg/router/controller/route_secret_manager.go b/pkg/router/controller/route_secret_manager.go index 6aa3d2639..3ae4caed1 100644 --- a/pkg/router/controller/route_secret_manager.go +++ b/pkg/router/controller/route_secret_manager.go @@ -4,6 +4,8 @@ import ( "context" "fmt" "sync" + "sync/atomic" + "time" routev1 "github.com/openshift/api/route/v1" routelisters "github.com/openshift/client-go/route/listers/route/v1" @@ -11,6 +13,7 @@ import ( "github.com/openshift/router/pkg/router" "github.com/openshift/router/pkg/router/routeapihelpers" kapi "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apimachinery/pkg/watch" @@ -25,8 +28,27 @@ const ( ExtCrtStatusReasonSecretUpdated = "ExternalCertificateSecretUpdated" ExtCrtStatusReasonSecretDeleted = "ExternalCertificateSecretDeleted" ExtCrtStatusReasonGetFailed = "ExternalCertificateGetFailed" + ExtCrtStatusReasonSARCompleted = "ExternalCertificateSARCompleted" + + // certResourceVersionAnnotation is an in-memory-only annotation set + // by populateRouteTLSFromSecret to carry the secret's + // ResourceVersion through the plugin chain into + // templateRouter.AddRoute, where it is used as a staleness guard. + certResourceVersionAnnotation = "router.openshift.io/cert-resource-version" ) +// secretUpdateRecheckDelay is how long the UpdateFunc secret handler waits +// before re-checking SAR permissions after a secret update, to catch RBAC +// revocations that haven't propagated to the API server's authorizer yet. +// Overridable in tests (via atomic Store/Load, since a prior test's spawned +// goroutine may still be sleeping on this value when a later test runs) to +// avoid real sleeps. +var secretUpdateRecheckDelay atomic.Int64 + +func init() { + secretUpdateRecheckDelay.Store(int64(3 * time.Second)) +} + // RouteSecretManager implements the router.Plugin interface to register // or unregister route with secretManger if externalCertificate is used. // It also reads the referenced secret to update in-memory tls.Certificate and tls.Key @@ -46,6 +68,40 @@ type RouteSecretManager struct { // Populated inside DeleteFunc, and consumed or cleaned inside AddFunc and unregister(). // It is thread safe and "namespace/routeName" is used as its key. deletedSecrets sync.Map + + // routeLocks serializes cert validation and refresh (validate + + // populateRouteTLSFromSecret + propagation to the plugin chain) per + // route, keyed by "namespace/routeName". This work can be triggered + // concurrently from two different goroutines for the same route: the + // route-watch-driven HandleRoute path (re-validation on any Modified + // event) and the secret-watch-driven UpdateFunc path (reacting to a + // secret change). GetSecret always reads the current, monotonically + // advancing informer cache rather than a captured-earlier snapshot, so + // serializing the full read-then-propagate sequence guarantees whichever + // side runs second observes state at least as fresh as the first -- + // closing the race where a slower call that started earlier finishes + // after a faster one and silently overwrites its fresh cert with stale + // data. + // + // Deliberately never cleaned up: safely removing a keyed mutex entry + // requires knowing no one else is about to look it up, which a simple + // sync.Map can't guarantee -- deleting while another goroutine is + // mid-LoadOrStore for the same key would hand out two different mutex + // objects for the same route, silently defeating the serialization this + // exists for. A live router process seeing enough distinct route names + // over its lifetime to make this map's size a real concern is not a + // realistic scenario, so unbounded (but tiny, one *sync.Mutex per name) + // growth is the safer tradeoff over a subtly-reintroduced race. + routeLocks sync.Map // map[types.NamespacedName]*sync.Mutex +} + +// lockRoute acquires the per-route lock for key, creating it on first use, +// and returns a function to release it. +func (p *RouteSecretManager) lockRoute(key types.NamespacedName) func() { + value, _ := p.routeLocks.LoadOrStore(key, &sync.Mutex{}) + mu := value.(*sync.Mutex) + mu.Lock() + return mu.Unlock } // NewRouteSecretManager creates a new instance of RouteSecretManager. @@ -107,14 +163,30 @@ func (p *RouteSecretManager) Commit() error { func (p *RouteSecretManager) HandleRoute(eventType watch.EventType, route *routev1.Route) error { log.V(10).Info("HandleRoute: RouteSecretManager", "eventType", eventType) + // DeepCopy the route before any mutation. The route pointer may come from + // the informer cache (via the lister), which is shared across goroutines. + // populateRouteTLSFromSecret writes Certificate and Key in-place, which + // would race with informer goroutines that read the same object (e.g., + // secret handler UpdateFunc calling route.DeepCopy()). + if hasExternalCertificate(route) { + route = route.DeepCopy() + } + + // registered tracks whether validateAndRegister was called, meaning this + // is a first-time or new-cert registration that should emit SARCompleted. + registered := false + switch eventType { case watch.Added: // register with secret monitor if hasExternalCertificate(route) { log.V(4).Info("Validating and registering external certificate", "namespace", route.Namespace, "secret", route.Spec.TLS.ExternalCertificate.Name, "route", route.Name) - if err := p.validateAndRegister(route); err != nil { + unlock, err := p.validateAndRegister(route) + if err != nil { return err } + defer unlock() + registered = true } case watch.Modified: @@ -132,9 +204,12 @@ func (p *RouteSecretManager) HandleRoute(eventType watch.EventType, route *route if err := p.unregister(route); err != nil { return err } - if err := p.validateAndRegister(route); err != nil { + unlock, err := p.validateAndRegister(route) + if err != nil { return err } + defer unlock() + registered = true } else { // ExternalCertificate is not updated // Re-validate and update the in-memory TLS certificate and key (even if ExternalCertificate remains unchanged) @@ -157,23 +232,47 @@ func (p *RouteSecretManager) HandleRoute(eventType watch.EventType, route *route // Therefore, it is essential to re-sync the secret to ensure the plugin chain correctly handles the route. log.V(4).Info("Re-validating existing external certificate", "namespace", route.Namespace, "secret", oldSecret, "route", route.Name) - // re-validate + // re-validate (synchronous, throttled by semaphore). Runs + // outside the per-route lock below: it only checks SAR/secret + // existence and doesn't write cert content, so there's + // nothing here for a concurrent UpdateFunc refresh to race + // against -- serializing it too would just add this call's + // SAR-check latency to the critical section for no benefit. if err := p.validate(route); err != nil { return err } + // Don't let the SAR result from this re-validation persist + // in the cache. If RBAC was just revoked, the API server + // may not have propagated the change yet, producing a stale + // "allowed" result. Invalidating ensures the next evaluation + // (triggered by the rejection→re-admission status cycle) + // does a fresh SAR check. + routeapihelpers.InvalidateAsyncSARCache(route.Namespace, route.Spec.TLS.ExternalCertificate.Name) + + // Serialize with any concurrent secret-triggered refresh (see + // UpdateFunc) from here through the propagation call below, + // so the two can never race to write different cert content + // to the plugin chain (see the routeLocks field comment). + unlock := p.lockRoute(routeKey(route.Namespace, route.Name)) + defer unlock() + // read referenced secret and update TLS certificate and key if err := p.populateRouteTLSFromSecret(route); err != nil { return err } + } case newHasExt && !oldHadExt: // New route has externalCertificate, old route did not log.V(4).Info("Validating and registering new external certificate", "namespace", route.Namespace, "secret", route.Spec.TLS.ExternalCertificate.Name, "route", route.Name) // register with secret monitor - if err := p.validateAndRegister(route); err != nil { + unlock, err := p.validateAndRegister(route) + if err != nil { return err } + defer unlock() + registered = true case !newHasExt && oldHadExt: // Old route had externalCertificate, new route does not @@ -198,27 +297,68 @@ func (p *RouteSecretManager) HandleRoute(eventType watch.EventType, route *route } // call next plugin - return p.plugin.HandleRoute(eventType, route) + err := p.plugin.HandleRoute(eventType, route) + + // Only emit SARCompleted when validateAndRegister was called in this + // pass — i.e., on first-time registration or cert change. Skip it on + // re-validation (Modified with same cert), which would create a + // re-enqueue feedback loop and can re-admit routes that were rejected + // by the secret handlers. + // + // registered alone is not enough: validateAndRegister runs concurrently + // with the secret informer's own DeleteFunc on a different goroutine, so + // a watch.Added registration that was already in flight when the secret + // got deleted can finish afterward and write this SARCompleted status, + // silently overwriting DeleteFunc's rejection. Checking deletedSecrets + // here closes that race regardless of which goroutine finishes last. + if err == nil && registered { + key := routeKey(route.Namespace, route.Name) + if _, secretDeleted := p.deletedSecrets.Load(key); !secretDeleted { + msg := fmt.Sprintf("SAR check and secret load completed for secret %q", route.Spec.TLS.ExternalCertificate.Name) + p.recorder.RecordRouteUpdate(route, ExtCrtStatusReasonSARCompleted, msg) + } + } + + return err } -// validateAndRegister validates the route's externalCertificate configuration and registers it with the secret manager. -// It also updates the in-memory TLS certificate and key after reading from secret informer's cache. -func (p *RouteSecretManager) validateAndRegister(route *routev1.Route) error { - // validate - if err := p.validate(route); err != nil { - return err - } - // register route with secretManager +// validateAndRegister registers the route with the secret manager, validates +// its externalCertificate configuration, and loads the TLS cert data. +// +// Registration happens BEFORE validation so the route receives informer +// events (Add/Update/Delete) even if the initial SAR check fails due to +// RBAC propagation delays. Without this ordering, a transient SAR failure +// permanently orphans the route from the secret informer — it never +// receives UpdateFunc events and can never pick up secret changes. +// +// If validation fails after registration, the route stays registered (so +// future informer events can trigger re-evaluation) but is not admitted. +// +// The per-route lock is acquired after registration and held until the +// caller releases it, so cert-population and propagation happen as one +// atomic unit with respect to any concurrent secret-triggered refresh. +func (p *RouteSecretManager) validateAndRegister(route *routev1.Route) (unlock func(), err error) { + // Register route with secretManager first, so it receives informer + // events regardless of whether the SAR check below passes. handler := p.generateSecretHandler(route.Namespace, route.Name) if err := p.secretManager.RegisterRoute(context.TODO(), route.Namespace, route.Name, route.Spec.TLS.ExternalCertificate.Name, handler); err != nil { - return fmt.Errorf("failed to register router: %w", err) + return nil, fmt.Errorf("failed to register router: %w", err) } + + // validate (synchronous, throttled by semaphore) + if err := p.validate(route); err != nil { + return nil, err + } + + unlock = p.lockRoute(routeKey(route.Namespace, route.Name)) + // read referenced secret and update TLS certificate and key if err := p.populateRouteTLSFromSecret(route); err != nil { - return err + unlock() + return nil, err } - return nil + return unlock, nil } // generateSecretHandler creates ResourceEventHandlerFuncs to handle Add, Update, and Delete events on secrets. @@ -260,22 +400,25 @@ func (p *RouteSecretManager) generateSecretHandler(namespace, routeName string) AddFunc: func(obj interface{}) { secret := obj.(*kapi.Secret) log.V(4).Info("Secret added for route", "namespace", namespace, "secret", secret.Name, "route", routeName) + routeapihelpers.InvalidateAsyncSARCache(namespace, secret.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 handling the route. // 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(namespace, routeName) + key := routeKey(namespace, routeName) if _, deleted := p.deletedSecrets.LoadAndDelete(key); deleted { log.V(4).Info("Secret recreated for route", "namespace", namespace, "secret", secret.Name, "route", routeName) - // Ensure fetching the updated route + // Ensure fetching the updated route and DeepCopy to avoid + // reading/writing the shared informer cache object. route, err := p.routelister.Routes(namespace).Get(routeName) if err != nil { log.Error(err, "failed to get route", "namespace", namespace, "route", routeName) return } + route = route.DeepCopy() // The route should *remain* rejected until it's re-evaluated // by all the plugins (including this plugin). Once passes, the route will become active again. @@ -287,45 +430,105 @@ func (p *RouteSecretManager) generateSecretHandler(namespace, routeName string) UpdateFunc: func(old interface{}, new interface{}) { secretOld := old.(*kapi.Secret) secretNew := new.(*kapi.Secret) - key := generateKey(namespace, routeName) + key := routeKey(namespace, routeName) log.V(4).Info("Secret updated for route", "namespace", namespace, "secret", secretNew.Name, "oldSecretVersion", secretOld.ResourceVersion, "newSecretVersion", secretNew.ResourceVersion, "route", routeName) + routeapihelpers.InvalidateAsyncSARCache(namespace, secretNew.Name) - // Ensure fetching the updated route + // Ensure fetching the updated route and DeepCopy to avoid + // reading/writing the shared informer cache object. route, err := p.routelister.Routes(namespace).Get(routeName) if err != nil { log.Error(err, "failed to get route", "namespace", namespace, "route", routeName) return } + route = route.DeepCopy() msg := fmt.Sprintf("secret %q updated for route %q (oldSecretVersion=%v, newSecretVersion=%v)", secretNew.Name, key, secretOld.ResourceVersion, secretNew.ResourceVersion) - // Update the route status to notify plugins, including this plugin, for re-evaluation. - // - If the route is admitted (Admitted=True), record an update event. - // - If the route is not admitted, record a rejection event (keep it rejected). - if isRouteAdmittedTrue(route.DeepCopy(), p.routerName) { - p.recorder.RecordRouteUpdate(route, ExtCrtStatusReasonSecretUpdated, msg) - } else { - p.recorder.RecordRouteRejection(route, ExtCrtStatusReasonSecretUpdated, msg) - } + // Keep the route admitted so it remains reachable while the + // new cert is picked up below. + p.recorder.RecordRouteUpdate(route, ExtCrtStatusReasonSecretUpdated, msg) + + // Read the new secret and push it through the plugin chain + // immediately. We skip the synchronous validate() (SAR + + // secret-existence check) here because: + // 1. The secret was just updated — it exists. + // 2. RBAC was verified when the route was first admitted. + // 3. A delayed re-check goroutine (below) catches any RBAC + // revocation that happened concurrently. + // Removing validate() from this synchronous path is critical + // for performance: SharedSecretManager.notify() dispatches to + // all route handlers for the secret sequentially on one + // goroutine. With N routes sharing a secret, each validate() + // makes 4 blocking API calls, creating N×4 sequential + // round-trips that can exceed the test's poll timeout under + // API-server load (e.g. HyperShift CI). + func() { + unlock := p.lockRoute(key) + defer unlock() + if err := p.populateRouteTLSFromSecret(route); err != nil { + return + } + if err := p.plugin.HandleRoute(watch.Modified, route); err != nil { + log.Error(err, "failed to propagate route after secret update", "namespace", namespace, "route", routeName) + } + }() + + // Trigger a rate-limited HAProxy reload directly instead of + // depending on the indirect round trip (status write → API + // server → route informer → RouterController.HandleRoute → + // Commit), which adds 10-30s under HyperShift conditions. + p.plugin.Commit() + + // Schedule a delayed re-check to catch RBAC revocations that + // may not have propagated yet. The SAR cache was already + // invalidated above (line 423), so the re-check does a fresh + // SAR. validate() rejects and deactivates the route only if + // the check now fails; a passing check is a no-op. + go func() { + time.Sleep(time.Duration(secretUpdateRecheckDelay.Load())) + routeapihelpers.InvalidateAsyncSARCache(namespace, secretNew.Name) + route, err := p.routelister.Routes(namespace).Get(routeName) + if err != nil { + return + } + route = route.DeepCopy() + _ = p.validate(route) + }() }, DeleteFunc: func(obj interface{}) { - secret := obj.(*kapi.Secret) - key := generateKey(namespace, routeName) - msg := fmt.Sprintf("secret %q deleted for route %q", secret.Name, key) + secret, ok := obj.(*kapi.Secret) + if !ok { + tombstone, ok := obj.(cache.DeletedFinalStateUnknown) + if !ok { + log.Error(nil, "Couldn't get object from tombstone", "type", fmt.Sprintf("%T", obj)) + return + } + secret, ok = tombstone.Obj.(*kapi.Secret) + if !ok { + log.Error(nil, "Tombstone contained object that is not a secret", "type", fmt.Sprintf("%T", tombstone.Obj)) + return + } + } + key := routeKey(namespace, routeName) + msg := fmt.Sprintf("external certificate validation failed: secret %q deleted for route %q", secret.Name, key) log.V(4).Info(msg) + routeapihelpers.InvalidateAsyncSARCache(namespace, secret.Name) // keep the secret monitor active and mark the secret as deleted for this route. p.deletedSecrets.Store(key, true) - // Ensure fetching the updated route + // Ensure fetching the updated route and DeepCopy to avoid + // reading/writing the shared informer cache object. route, err := p.routelister.Routes(namespace).Get(routeName) if err != nil { log.Error(err, "failed to get route", "namespace", namespace, "route", routeName) return } + route = route.DeepCopy() // Reject this route - p.recorder.RecordRouteRejection(route, ExtCrtStatusReasonSecretDeleted, msg) + p.recorder.RecordRouteRejection(route, ExtCrtStatusReasonValidationFailed, msg) }, } } @@ -334,10 +537,14 @@ func (p *RouteSecretManager) generateSecretHandler(namespace, routeName string) // 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. // +// This function is synchronous: it blocks until the SAR check completes. +// Concurrency is throttled by the semaphore in ValidateTLSExternalCertificate. +// // NOTE: TLS data validation and sanitization are handled by the next plugin `ExtendedValidator`, // by reading the "tls.crt" and "tls.key" added by populateRouteTLSFromSecret. func (p *RouteSecretManager) validate(route *routev1.Route) error { 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, ExtCrtStatusReasonValidationFailed, err.Error()) @@ -352,7 +559,9 @@ func (p *RouteSecretManager) validate(route *routev1.Route) error { // the deletion of the route by calling the HandleRoute method with a watch.Deleted event. // Note: This function performs an in-place update of the route. The caller should be aware that the route's TLS configuration will be modified directly. func (p *RouteSecretManager) populateRouteTLSFromSecret(route *routev1.Route) error { - // read referenced secret + // read referenced secret from the informer cache. + // GetSecret attempts to read from the cache and falls back to a direct API + // call if the cache is not synced or the secret is not found. secret, err := p.secretManager.GetSecret(context.TODO(), route.Namespace, route.Name) if err != nil { log.Error(err, "failed to get referenced secret") @@ -368,6 +577,14 @@ func (p *RouteSecretManager) populateRouteTLSFromSecret(route *routev1.Route) er route.Spec.TLS.Certificate = string(secret.Data["tls.crt"]) route.Spec.TLS.Key = string(secret.Data["tls.key"]) + // Stamp the secret's ResourceVersion onto the route as an in-memory + // annotation so that templateRouter.AddRoute can use it as a + // staleness guard (see CertResourceVersion on ServiceAliasConfig). + if route.Annotations == nil { + route.Annotations = make(map[string]string) + } + route.Annotations[certResourceVersionAnnotation] = secret.ResourceVersion + return nil } @@ -381,7 +598,7 @@ func (p *RouteSecretManager) unregister(route *routev1.Route) error { } // 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)) + p.deletedSecrets.Delete(routeKey(route.Namespace, route.Name)) return nil } @@ -391,24 +608,6 @@ func hasExternalCertificate(route *routev1.Route) bool { return tls != nil && tls.ExternalCertificate != nil && len(tls.ExternalCertificate.Name) > 0 } -// generateKey creates a unique identifier for a route -func generateKey(namespace, routeName string) string { - return fmt.Sprintf("%s/%s", namespace, routeName) -} - -// isRouteAdmittedTrue returns true if the given route has been admitted -// by the current router, otherwise false. -func isRouteAdmittedTrue(route *routev1.Route, routerName string) bool { - for _, ingress := range route.Status.Ingress { - if ingress.RouterName != routerName { - continue - } - - for _, condition := range ingress.Conditions { - if condition.Type == routev1.RouteAdmitted && condition.Status == kapi.ConditionTrue { - return true - } - } - } - return false +func routeKey(namespace, routeName string) types.NamespacedName { + return types.NamespacedName{Namespace: namespace, Name: routeName} } diff --git a/pkg/router/controller/route_secret_manager_test.go b/pkg/router/controller/route_secret_manager_test.go index e9e42e7f0..0c10401e5 100644 --- a/pkg/router/controller/route_secret_manager_test.go +++ b/pkg/router/controller/route_secret_manager_test.go @@ -3,25 +3,22 @@ package controller import ( "context" "fmt" - "os" "reflect" + "sync" + "sync/atomic" "testing" + "time" "github.com/openshift/library-go/pkg/route/secretmanager/fake" - "github.com/openshift/router/pkg/router" + "github.com/openshift/router/pkg/router/routeapihelpers" routev1 "github.com/openshift/api/route/v1" authorizationv1 "k8s.io/api/authorization/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/fields" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/watch" - "k8s.io/client-go/features" testclient "k8s.io/client-go/kubernetes/fake" corev1client "k8s.io/client-go/kubernetes/typed/core/v1" - "k8s.io/client-go/tools/cache" ) const testRouterName = "test-router" @@ -50,30 +47,6 @@ func (t *testSecretGetter) Secrets(_ string) corev1client.SecretInterface { return testclient.NewSimpleClientset(t.secret).CoreV1().Secrets(t.namespace) } -// fakeSecretInformer will list/watch only one secret inside a namespace -func fakeSecretInformer(fakeKubeClient *testclient.Clientset, namespace, name string) cache.SharedInformer { - // WatchListClient featuregate is enabled by default since v0.35. Fake client does not support - // initializing its cache from Watch, so falling back to use List instead. The envvar below - // configures the featuregate state. - os.Setenv("KUBE_FEATURE_"+string(features.WatchListClient), "False") - - fieldSelector := fields.OneTermEqualSelector("metadata.name", name).String() - return cache.NewSharedInformer( - &cache.ListWatch{ - ListFunc: func(options metav1.ListOptions) (runtime.Object, error) { - options.FieldSelector = fieldSelector - return fakeKubeClient.CoreV1().Secrets(namespace).List(context.TODO(), options) - }, - WatchFunc: func(options metav1.ListOptions) (watch.Interface, error) { - options.FieldSelector = fieldSelector - return fakeKubeClient.CoreV1().Secrets(namespace).Watch(context.TODO(), options) - }, - }, - &corev1.Secret{}, - 0, - ) -} - func fakeSecret(namespace, name string, secretType corev1.SecretType, data map[string][]byte) *corev1.Secret { return &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -85,60 +58,55 @@ func fakeSecret(namespace, name string, secretType corev1.SecretType, data map[s } } -type fakePluginDone struct { - eventType watch.EventType - route *routev1.Route - err error - doneCh chan struct{} -} - -func (p *fakePluginDone) HandleRoute(eventType watch.EventType, route *routev1.Route) error { - defer close(p.doneCh) - p.eventType, p.route = eventType, route - return p.err -} -func (p *fakePluginDone) HandleNode(t watch.EventType, node *corev1.Node) error { - return fmt.Errorf("not expected") -} -func (p *fakePluginDone) HandleEndpoints(watch.EventType, *corev1.Endpoints) error { - return fmt.Errorf("not expected") -} -func (p *fakePluginDone) HandleNamespaces(namespaces sets.String) error { - return fmt.Errorf("not expected") -} -func (p *fakePluginDone) Commit() error { - return p.err -} - -var _ router.Plugin = &fakePluginDone{} - type statusRecorder struct { + sync.Mutex rejections []string updates []string unservableInFutureVersions map[string]string - doneCh chan struct{} } func (r *statusRecorder) routeKey(route *routev1.Route) string { return route.Namespace + "-" + route.Name } func (r *statusRecorder) RecordRouteRejection(route *routev1.Route, reason, message string) { - defer close(r.doneCh) + r.Lock() + defer r.Unlock() r.rejections = append(r.rejections, fmt.Sprintf("%s:%s", r.routeKey(route), reason)) } func (r *statusRecorder) RecordRouteUpdate(route *routev1.Route, reason, message string) { - defer close(r.doneCh) + r.Lock() + defer r.Unlock() r.updates = append(r.updates, fmt.Sprintf("%s:%s", r.routeKey(route), reason)) } func (r *statusRecorder) RecordRouteUnservableInFutureVersionsClear(route *routev1.Route) { + r.Lock() + defer r.Unlock() delete(r.unservableInFutureVersions, r.routeKey(route)) } func (r *statusRecorder) RecordRouteUnservableInFutureVersions(route *routev1.Route, reason, message string) { + r.Lock() + defer r.Unlock() r.unservableInFutureVersions[r.routeKey(route)] = reason } +func (r *statusRecorder) GetRejections() []string { + r.Lock() + defer r.Unlock() + var res []string + res = append(res, r.rejections...) + return res +} + +func (r *statusRecorder) GetUpdates() []string { + r.Lock() + defer r.Unlock() + var res []string + res = append(res, r.updates...) + return res +} + var _ RouteStatusRecorder = &statusRecorder{} func TestRouteSecretManager(t *testing.T) { @@ -152,6 +120,7 @@ func TestRouteSecretManager(t *testing.T) { expectedRoute *routev1.Route expectedEventType watch.EventType expectedRejections []string + expectedUpdates []string expectedError bool }{ // scenarios when route is added @@ -325,6 +294,9 @@ func TestRouteSecretManager(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "route-test", Namespace: "sandbox", + Annotations: map[string]string{ + certResourceVersionAnnotation: "", + }, }, Spec: routev1.RouteSpec{ TLS: &routev1.TLSConfig{ @@ -337,6 +309,9 @@ func TestRouteSecretManager(t *testing.T) { }, }, expectedEventType: watch.Added, + expectedUpdates: []string{ + "sandbox-route-test:ExternalCertificateSARCompleted", + }, }, { name: "route added without externalCertificate", @@ -543,6 +518,9 @@ func TestRouteSecretManager(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "route-test", Namespace: "sandbox", + Annotations: map[string]string{ + certResourceVersionAnnotation: "", + }, }, Spec: routev1.RouteSpec{ TLS: &routev1.TLSConfig{ @@ -555,6 +533,9 @@ func TestRouteSecretManager(t *testing.T) { }, }, expectedEventType: watch.Modified, + expectedUpdates: []string{ + "sandbox-route-test:ExternalCertificateSARCompleted", + }, }, // scenarios when route is updated (old route with externalCertificate, new route with same externalCertificate) @@ -736,7 +717,7 @@ func TestRouteSecretManager(t *testing.T) { expectedError: true, }, { - name: "route updated: old route with externalCertificate, new route with same externalCertificate allowed and correct secret", + name: "route updated: old route with externalCertificate, new route with same externalCertificate allowed and correct secret (no SARCompleted on re-validation)", route: &routev1.Route{ ObjectMeta: metav1.ObjectMeta{ Name: "route-test", @@ -764,6 +745,9 @@ func TestRouteSecretManager(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "route-test", Namespace: "sandbox", + Annotations: map[string]string{ + certResourceVersionAnnotation: "", + }, }, Spec: routev1.RouteSpec{ TLS: &routev1.TLSConfig{ @@ -968,6 +952,9 @@ func TestRouteSecretManager(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "route-test", Namespace: "sandbox", + Annotations: map[string]string{ + certResourceVersionAnnotation: "", + }, }, Spec: routev1.RouteSpec{ TLS: &routev1.TLSConfig{ @@ -980,6 +967,9 @@ func TestRouteSecretManager(t *testing.T) { }, }, expectedEventType: watch.Modified, + expectedUpdates: []string{ + "sandbox-route-test:ExternalCertificateSARCompleted", + }, }, // scenarios when route is updated (old route with externalCertificate, new route without externalCertificate) @@ -1131,13 +1121,13 @@ func TestRouteSecretManager(t *testing.T) { for _, s := range scenarios { t.Run(s.name, func(t *testing.T) { + routeapihelpers.ClearAsyncSARCacheForTest() p := &fakePlugin{} - recorder := &statusRecorder{ - doneCh: make(chan struct{}), - } - rsm := NewRouteSecretManager(p, recorder, &s.secretManager, testRouterName, &testSecretGetter{namespace: s.route.Namespace, secret: s.secretManager.Secret}, &routeLister{}, &testSARCreator{allow: s.allow}) + recorder := &statusRecorder{} + rsm := NewRouteSecretManager(p, recorder, &s.secretManager, testRouterName, &testSecretGetter{namespace: s.route.Namespace, secret: s.secretManager.Secret}, &routeLister{items: []*routev1.Route{s.route}}, &testSARCreator{allow: s.allow}) gotErr := rsm.HandleRoute(s.eventType, s.route) + if (gotErr != nil) != s.expectedError { t.Fatalf("expected error to be %t, but got %t", s.expectedError, gotErr != nil) } @@ -1147,16 +1137,275 @@ func TestRouteSecretManager(t *testing.T) { if s.expectedEventType != p.t { t.Fatalf("expected %s event for next plugin, but got %s", s.expectedEventType, p.t) } - if !reflect.DeepEqual(s.expectedRejections, recorder.rejections) { - t.Fatalf("expected rejections %v, but got %v", s.expectedRejections, recorder.rejections) + if !reflect.DeepEqual(s.expectedRejections, recorder.GetRejections()) { + t.Fatalf("expected rejections %v, but got %v", s.expectedRejections, recorder.GetRejections()) + } + if !reflect.DeepEqual(s.expectedUpdates, recorder.GetUpdates()) { + t.Fatalf("expected updates %v, but got %v", s.expectedUpdates, recorder.GetUpdates()) } - if _, exists := rsm.deletedSecrets.Load(generateKey(s.route.Namespace, s.route.Name)); exists { - t.Fatalf("expected deletedSecrets to not have %q key", generateKey(s.route.Namespace, s.route.Name)) + if _, exists := rsm.deletedSecrets.Load(routeKey(s.route.Namespace, s.route.Name)); exists { + t.Fatalf("expected deletedSecrets to not have %q key", routeKey(s.route.Namespace, s.route.Name)) } }) } } +// TestPopulateRouteTLSRace verifies that HandleRoute's DeepCopy of the route +// prevents data races between the main controller goroutine (which populates +// TLS cert/key fields) and informer goroutines (which read the same route via +// DeepCopy, as the secret handler's UpdateFunc does in production). +// Run with -race to confirm no races are detected. +func TestPopulateRouteTLSRace(t *testing.T) { + routeapihelpers.ClearAsyncSARCacheForTest() + + secret := fakeSecret("sandbox", "tls-secret", corev1.SecretTypeTLS, map[string][]byte{ + "tls.crt": []byte("my-crt"), + "tls.key": []byte("my-key"), + }) + + route := &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "route-test", + Namespace: "sandbox", + }, + Spec: routev1.RouteSpec{ + TLS: &routev1.TLSConfig{ + ExternalCertificate: &routev1.LocalObjectReference{ + Name: "tls-secret", + }, + }, + }, + } + + // The routeLister returns a pointer to the SAME route object, + // faithfully reproducing the informer cache behavior in production. + lister := &routeLister{items: []*routev1.Route{route}} + + secretMgr := &fake.SecretManager{ + Secret: secret, + IsPresent: true, + SecretName: "tls-secret", + } + + rsm := NewRouteSecretManager( + &fakePlugin{}, + &statusRecorder{}, + secretMgr, + testRouterName, + &testSecretGetter{namespace: "sandbox", secret: secret}, + lister, + &testSARCreator{allow: true}, + ) + + // First call to register the route with the secret manager. + if err := rsm.HandleRoute(watch.Added, route); err != nil { + t.Fatalf("initial HandleRoute failed: %v", err) + } + + var wg sync.WaitGroup + const iterations = 100 + + // Goroutine A: simulates the main controller goroutine calling + // HandleRoute, which calls populateRouteTLSFromSecret and WRITES + // to route.Spec.TLS.Certificate and route.Spec.TLS.Key. + wg.Add(1) + go func() { + defer wg.Done() + for i := 0; i < iterations; i++ { + routeapihelpers.ClearAsyncSARCacheForTest() + if err := rsm.HandleRoute(watch.Modified, route); err != nil { + t.Errorf("HandleRoute iteration %d: %v", i, err) + return + } + } + }() + + // Goroutine B: simulates the informer goroutine reading the same + // shared route object. In production, the secret handler's UpdateFunc + // calls isRouteAdmittedTrue(route.DeepCopy(), ...) which READS all + // fields including the TLS fields being written by goroutine A. + wg.Add(1) + go func() { + defer wg.Done() + for i := 0; i < iterations; i++ { + _ = route.DeepCopy() + } + }() + + wg.Wait() +} + +// TestSARCompletedOnlyOnAdded verifies that HandleRoute emits +// RecordRouteUpdate(SARCompleted) only on watch.Added events, not on +// watch.Modified. Emitting on Modified creates a re-enqueue feedback +// loop and can re-admit routes that were rejected by secret handlers. +func TestSARCompletedOnlyOnAdded(t *testing.T) { + routeapihelpers.ClearAsyncSARCacheForTest() + + secret := fakeSecret("sandbox", "tls-secret", corev1.SecretTypeTLS, map[string][]byte{ + "tls.crt": []byte("my-crt"), + "tls.key": []byte("my-key"), + }) + + route := &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "route-test", + Namespace: "sandbox", + }, + Spec: routev1.RouteSpec{ + TLS: &routev1.TLSConfig{ + ExternalCertificate: &routev1.LocalObjectReference{ + Name: "tls-secret", + }, + }, + }, + } + + lister := &routeLister{items: []*routev1.Route{route}} + recorder := &statusRecorder{} + + rsm := NewRouteSecretManager( + &fakePlugin{}, + recorder, + &fake.SecretManager{ + Secret: secret, + IsPresent: true, + SecretName: "tls-secret", + }, + testRouterName, + &testSecretGetter{namespace: "sandbox", secret: secret}, + lister, + &testSARCreator{allow: true}, + ) + + // HandleRoute(Added): should emit SARCompleted. + if err := rsm.HandleRoute(watch.Added, route); err != nil { + t.Fatalf("HandleRoute(Added) failed: %v", err) + } + updates := recorder.GetUpdates() + if len(updates) != 1 || updates[0] != "sandbox-route-test:ExternalCertificateSARCompleted" { + t.Fatalf("expected one SARCompleted on Added, got: %v", updates) + } + + // HandleRoute(Modified): should NOT emit SARCompleted. + routeapihelpers.ClearAsyncSARCacheForTest() + if err := rsm.HandleRoute(watch.Modified, route); err != nil { + t.Fatalf("HandleRoute(Modified) failed: %v", err) + } + + updates = recorder.GetUpdates() + sarCount := 0 + for _, u := range updates { + if u == "sandbox-route-test:ExternalCertificateSARCompleted" { + sarCount++ + } + } + if sarCount != 1 { + t.Fatalf("expected exactly 1 SARCompleted (from Added only), got %d: %v", sarCount, updates) + } +} + +// TestDeletedSecretDoesNotGetReadmitted reproduces the failure mode from +// the Hypershift conformance test "the secret is deleted then routes are +// not reachable": after the DeleteFunc rejects the route (Admitted=False), +// the status update triggers a re-enqueue. If the subsequent HandleRoute +// call succeeds (because GetSecret still returns the secret from cache +// during the deletion propagation window), the SARCompleted write must +// NOT flip the route back to Admitted=True. Otherwise the route bounces +// between admitted and rejected, and the E2E test polls for Admitted=False +// until timeout. +// +// Guards against a regression where the SARCompleted write re-admits +// the route during the informer cache propagation window after deletion. +func TestDeletedSecretDoesNotGetReadmitted(t *testing.T) { + routeapihelpers.ClearAsyncSARCacheForTest() + + secret := fakeSecret("sandbox", "tls-secret", corev1.SecretTypeTLS, map[string][]byte{ + "tls.crt": []byte("my-crt"), + "tls.key": []byte("my-key"), + }) + + route := &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "route-test", + Namespace: "sandbox", + }, + Spec: routev1.RouteSpec{ + TLS: &routev1.TLSConfig{ + ExternalCertificate: &routev1.LocalObjectReference{ + Name: "tls-secret", + }, + }, + }, + } + + lister := &routeLister{items: []*routev1.Route{route}} + recorder := &statusRecorder{} + secretMgr := &fake.SecretManager{ + Secret: secret, + IsPresent: true, + SecretName: "tls-secret", + } + + rsm := NewRouteSecretManager( + &fakePlugin{}, + recorder, + secretMgr, + testRouterName, + &testSecretGetter{namespace: "sandbox", secret: secret}, + lister, + &testSARCreator{allow: true}, + ) + + // Step 1: Admit the route — should succeed and write SARCompleted. + if err := rsm.HandleRoute(watch.Added, route); err != nil { + t.Fatalf("initial HandleRoute failed: %v", err) + } + updates := recorder.GetUpdates() + if len(updates) != 1 || updates[0] != "sandbox-route-test:ExternalCertificateSARCompleted" { + t.Fatalf("expected SARCompleted after initial admission, got: %v", updates) + } + + // Step 2: Simulate secret deletion via the handler. + // This records a rejection (Admitted=False, ValidationFailed). + handler := rsm.generateSecretHandler(route.Namespace, route.Name) + handler.DeleteFunc(secret) + + rejections := recorder.GetRejections() + if len(rejections) != 1 || rejections[0] != "sandbox-route-test:ExternalCertificateValidationFailed" { + t.Fatalf("expected ValidationFailed rejection after delete, got: %v", rejections) + } + + // Step 3: Simulate the re-enqueue triggered by the rejection status + // update. The route still has externalCertificate in its spec. The + // SecretManager still returns the secret (simulating the informer + // cache race where the delete hasn't propagated yet). validate() and + // populateRouteTLSFromSecret() both succeed. + // + // On unfixed code, HandleRoute succeeds, then the SARCompleted guard + // sees the route has Admitted=False (no ext-cert admitted reason) and + // writes SARCompleted — flipping the route BACK to Admitted=True. + // This re-admission is the bug that causes the E2E test to timeout. + routeapihelpers.ClearAsyncSARCacheForTest() + if err := rsm.HandleRoute(watch.Modified, route); err != nil { + t.Fatalf("re-enqueued HandleRoute failed: %v", err) + } + + // Verify the route was NOT re-admitted. After the DeleteFunc rejection, + // no new SARCompleted update should have been recorded. + allUpdates := recorder.GetUpdates() + sarCount := 0 + for _, u := range allUpdates { + if u == "sandbox-route-test:ExternalCertificateSARCompleted" { + sarCount++ + } + } + if sarCount != 1 { + t.Fatalf("expected exactly 1 SARCompleted (from initial admission), got %d: %v", + sarCount, allUpdates) + } +} + func TestSecretUpdate(t *testing.T) { scenarios := []struct { @@ -1255,56 +1504,77 @@ func TestSecretUpdate(t *testing.T) { for _, s := range scenarios { t.Run(s.name, func(t *testing.T) { - recorder := &statusRecorder{ - doneCh: make(chan struct{}), - } + recorder := &statusRecorder{} lister := &routeLister{items: []*routev1.Route{s.route}} - rsm := NewRouteSecretManager(&fakePlugin{}, recorder, &fake.SecretManager{}, testRouterName, &testSecretGetter{}, lister, &testSARCreator{}) - // Create a fakeSecret and start an informer for it + // Create a fakeSecret secret := fakeSecret("sandbox", "tls-secret", corev1.SecretTypeTLS, map[string][]byte{}) - kubeClient := testclient.NewSimpleClientset(secret) - informer := fakeSecretInformer(kubeClient, "sandbox", "tls-secret") - go informer.Run(context.TODO().Done()) - - // wait for informer to start - if !cache.WaitForCacheSync(context.TODO().Done(), informer.HasSynced) { - t.Fatal("cache not synced yet") - } - - if _, err := informer.AddEventHandler(rsm.generateSecretHandler(s.route.Namespace, s.route.Name)); err != nil { - t.Fatalf("failed to add handler: %v", err) - } // update the secret updatedSecret := secret.DeepCopy() + updatedSecret.ResourceVersion = "200" updatedSecret.Data = map[string][]byte{ "tls.crt": []byte("my-crt"), "tls.key": []byte("my-key"), } - if _, err := kubeClient.CoreV1().Secrets(s.route.Namespace).Update(context.TODO(), updatedSecret, metav1.UpdateOptions{}); err != nil { - t.Fatalf("failed to update secret: %v", err) + + plugin := &fakePlugin{} + rsm := NewRouteSecretManager( + plugin, + recorder, + &fake.SecretManager{Secret: updatedSecret, IsPresent: true, SecretName: "tls-secret"}, + testRouterName, + &testSecretGetter{namespace: "sandbox", secret: updatedSecret}, + lister, + // SAR is set to deny — UpdateFunc must NOT call validate() + // synchronously, so SAR denial should not block the cert + // refresh. The delayed re-check goroutine will check SAR + // later, but we only assert immediate results here. + &testSARCreator{allow: false}, + ) + + // Get the handler + handler := rsm.generateSecretHandler(s.route.Namespace, s.route.Name) + + // Call the handler directly (synchronous — the delayed goroutine + // fires in the background but we only check immediate results). + handler.UpdateFunc(secret, updatedSecret) + + // UpdateFunc always calls RecordRouteUpdate (keeps Admitted=True) + // to ensure the route remains reachable while the new cert is + // picked up on re-enqueue. + expectedUpdates := []string{"sandbox-route-test:ExternalCertificateSecretUpdated"} + if !reflect.DeepEqual(expectedUpdates, recorder.GetUpdates()) { + t.Fatalf("expected updates %v, but got %v", expectedUpdates, recorder.GetUpdates()) } - // wait until route's status is updated - <-recorder.doneCh + // Verify HandleRoute was called with Modified event and the + // cert data was populated from the secret. + if plugin.t != watch.Modified { + t.Fatalf("expected HandleRoute called with Modified, got %v", plugin.t) + } + if plugin.route == nil { + t.Fatal("expected HandleRoute to receive a route") + } + if plugin.route.Spec.TLS.Certificate != "my-crt" { + t.Fatalf("expected cert 'my-crt', got %q", plugin.route.Spec.TLS.Certificate) + } + if plugin.route.Spec.TLS.Key != "my-key" { + t.Fatalf("expected key 'my-key', got %q", plugin.route.Spec.TLS.Key) + } - expectedStatus := []string{"sandbox-route-test:ExternalCertificateSecretUpdated"} + // Verify Commit() was called to trigger the HAProxy reload. + if plugin.commits != 1 { + t.Fatalf("expected Commit() called once, got %d", plugin.commits) + } - if s.isRouteAdmittedTrue { - // RecordRouteUpdate will be called if `Admitted=True` - if !reflect.DeepEqual(expectedStatus, recorder.updates) { - t.Fatalf("expected status %v, but got %v", expectedStatus, recorder.updates) - } - } else { - // RecordRouteRejection will be called if `Admitted=False` - if !reflect.DeepEqual(expectedStatus, recorder.rejections) { - t.Fatalf("expected status %v, but got %v", expectedStatus, recorder.rejections) - } + // Verify the cert-resource-version annotation was set. + if v := plugin.route.Annotations[certResourceVersionAnnotation]; v != "200" { + t.Fatalf("expected cert-resource-version annotation '200', got %q", v) } - if _, exists := rsm.deletedSecrets.Load(generateKey(s.route.Namespace, s.route.Name)); exists { - t.Fatalf("expected deletedSecrets to not have %q key", generateKey(s.route.Namespace, s.route.Name)) + if _, exists := rsm.deletedSecrets.Load(routeKey(s.route.Namespace, s.route.Name)); exists { + t.Fatalf("expected deletedSecrets to not have %q key", routeKey(s.route.Namespace, s.route.Name)) } }) @@ -1312,7 +1582,131 @@ func TestSecretUpdate(t *testing.T) { } -func TestSecretDelete(t *testing.T) { +// TestSecretUpdateDelayedRecheck verifies the delayed re-check spawned by +// UpdateFunc only writes a status update when it actually needs to: it must +// be a silent no-op when SAR still passes (the common case: secret rotated, +// RBAC unchanged), and it must reject the route when SAR now fails (RBAC was +// revoked and has since propagated). Making the common case a no-op avoids +// doubling the load on the router's status-write queue for every secret +// update, regardless of whether RBAC ever changes. +func TestSecretUpdateDelayedRecheck(t *testing.T) { + // Shrink the delay so the test doesn't sleep for real seconds. Stored + // atomically since a prior test's spawned goroutine (e.g. TestSecretUpdate, + // which uses the real default delay) may still be sleeping on this value. + originalDelay := secretUpdateRecheckDelay.Load() + secretUpdateRecheckDelay.Store(int64(10 * time.Millisecond)) + defer secretUpdateRecheckDelay.Store(originalDelay) + + scenarios := []struct { + name string + allow bool + expectedRejections []string + }{ + { + name: "SAR still allowed: delayed re-check is a no-op", + allow: true, + expectedRejections: nil, + }, + { + name: "SAR now denied: delayed re-check rejects the route", + allow: false, + expectedRejections: []string{ + "sandbox-route-test:ExternalCertificateValidationFailed", + }, + }, + } + + for _, s := range scenarios { + t.Run(s.name, func(t *testing.T) { + routeapihelpers.ClearAsyncSARCacheForTest() + + route := &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "route-test", + Namespace: "sandbox", + }, + Spec: routev1.RouteSpec{ + TLS: &routev1.TLSConfig{ + ExternalCertificate: &routev1.LocalObjectReference{ + Name: "tls-secret", + }, + }, + }, + } + + recorder := &statusRecorder{} + lister := &routeLister{items: []*routev1.Route{route}} + secret := fakeSecret("sandbox", "tls-secret", corev1.SecretTypeTLS, map[string][]byte{ + "tls.crt": []byte("my-crt"), + "tls.key": []byte("my-key"), + }) + updatedSecret := secret.DeepCopy() + updatedSecret.Data = map[string][]byte{ + "tls.crt": []byte("new-crt"), + "tls.key": []byte("new-key"), + } + // UpdateFunc reads the secret synchronously (no SAR check) + // and defers the SAR check to the delayed re-check goroutine. + rsm := NewRouteSecretManager( + &fakePlugin{}, + recorder, + &fake.SecretManager{Secret: updatedSecret, IsPresent: true, SecretName: "tls-secret"}, + testRouterName, + &testSecretGetter{namespace: "sandbox", secret: secret}, + lister, + &testSARCreator{allow: s.allow}, + ) + + handler := rsm.generateSecretHandler(route.Namespace, route.Name) + handler.UpdateFunc(secret, updatedSecret) + + // Wait for the delayed re-check goroutine to finish rather than + // sleeping a fixed amount, keeping the test fast and non-flaky. + deadline := time.Now().Add(2 * time.Second) + for time.Now().Before(deadline) { + if len(recorder.GetRejections()) == len(s.expectedRejections) { + break + } + time.Sleep(time.Millisecond) + } + + if !reflect.DeepEqual(s.expectedRejections, recorder.GetRejections()) { + t.Fatalf("expected rejections %v, but got %v", s.expectedRejections, recorder.GetRejections()) + } + + // The immediate write always happens regardless of the delayed + // re-check's outcome. + expectedUpdates := []string{"sandbox-route-test:ExternalCertificateSecretUpdated"} + if !reflect.DeepEqual(expectedUpdates, recorder.GetUpdates()) { + t.Fatalf("expected updates %v, but got %v", expectedUpdates, recorder.GetUpdates()) + } + }) + } +} + +// TestInFlightRegistrationDoesNotReAdmitDeletedSecretRoute reproduces the +// production failure of the same "the secret is deleted then routes are not +// reachable" E2E test, but via a different trigger than +// TestDeletedSecretDoesNotGetReadmitted: a watch.Added registration that was +// already in flight when the secret got deleted, rather than a re-validation +// after the rejection. +// +// validateAndRegister (driven by the route informer, on the router's main +// control loop) and DeleteFunc (driven by the secret informer, on its own +// goroutine) run concurrently. If DeleteFunc's rejection lands first but the +// already-in-flight Added registration finishes afterward, the `registered` +// flag alone does not protect against it -- registered is legitimately true +// for a first-time registration, so without also checking deletedSecrets the +// late-finishing SARCompleted write silently re-admits a route that was just +// correctly rejected. +func TestInFlightRegistrationDoesNotReAdmitDeletedSecretRoute(t *testing.T) { + routeapihelpers.ClearAsyncSARCacheForTest() + + secret := fakeSecret("sandbox", "tls-secret", corev1.SecretTypeTLS, map[string][]byte{ + "tls.crt": []byte("my-crt"), + "tls.key": []byte("my-key"), + }) + route := &routev1.Route{ ObjectMeta: metav1.ObjectMeta{ Name: "route-test", @@ -1326,42 +1720,92 @@ func TestSecretDelete(t *testing.T) { }, }, } - recorder := &statusRecorder{ - doneCh: make(chan struct{}), - } + lister := &routeLister{items: []*routev1.Route{route}} - rsm := NewRouteSecretManager(&fakePlugin{}, recorder, &fake.SecretManager{}, testRouterName, &testSecretGetter{}, lister, &testSARCreator{}) + recorder := &statusRecorder{} - // Create a fakeSecret and start an informer for it - secret := fakeSecret("sandbox", "tls-secret", corev1.SecretTypeTLS, map[string][]byte{}) - kubeClient := testclient.NewSimpleClientset(secret) - informer := fakeSecretInformer(kubeClient, "sandbox", "tls-secret") - go informer.Run(context.TODO().Done()) + rsm := NewRouteSecretManager( + &fakePlugin{}, + recorder, + &fake.SecretManager{ + Secret: secret, + IsPresent: true, + SecretName: "tls-secret", + }, + testRouterName, + &testSecretGetter{namespace: "sandbox", secret: secret}, + lister, + &testSARCreator{allow: true}, + ) + + // Simulate the secret being deleted BEFORE the route's own watch.Added + // registration (started earlier, e.g. right after route creation) gets a + // chance to finish. This is exactly DeleteFunc's own behavior. + handler := rsm.generateSecretHandler(route.Namespace, route.Name) + handler.DeleteFunc(secret) + + rejections := recorder.GetRejections() + if len(rejections) != 1 || rejections[0] != "sandbox-route-test:ExternalCertificateValidationFailed" { + t.Fatalf("expected ValidationFailed rejection after delete, got: %v", rejections) + } - // wait for informer to start - if !cache.WaitForCacheSync(context.TODO().Done(), informer.HasSynced) { - t.Fatal("cache not synced yet") + // Now the in-flight registration finishes. The fake SecretManager still + // returns the secret (simulating the informer cache race where the + // delete hasn't propagated to GetSecret's cache yet), so validate() and + // populateRouteTLSFromSecret() both succeed and registered=true. + // + // On unfixed code, the SARCompleted guard only checks `registered`, so + // it writes SARCompleted here -- flipping the route BACK to Admitted=True + // even though the secret is already gone. + if err := rsm.HandleRoute(watch.Added, route); err != nil { + t.Fatalf("in-flight HandleRoute(Added) failed: %v", err) } - if _, err := informer.AddEventHandler(rsm.generateSecretHandler(route.Namespace, route.Name)); err != nil { - t.Fatalf("failed to add handler: %v", err) + updates := recorder.GetUpdates() + for _, u := range updates { + if u == "sandbox-route-test:ExternalCertificateSARCompleted" { + t.Fatalf("expected no SARCompleted write for a route whose secret was already deleted, got updates: %v", updates) + } } +} - // delete the secret - if err := kubeClient.CoreV1().Secrets(route.Namespace).Delete(context.TODO(), secret.Name, metav1.DeleteOptions{}); err != nil { - t.Fatalf("failed to delete secret: %v", err) +func TestSecretDelete(t *testing.T) { + route := &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "route-test", + Namespace: "sandbox", + }, + Spec: routev1.RouteSpec{ + TLS: &routev1.TLSConfig{ + ExternalCertificate: &routev1.LocalObjectReference{ + Name: "tls-secret", + }, + }, + }, } + recorder := &statusRecorder{} + lister := &routeLister{items: []*routev1.Route{route}} + rsm := NewRouteSecretManager(&fakePlugin{}, recorder, &fake.SecretManager{}, testRouterName, &testSecretGetter{}, lister, &testSARCreator{}) + + // Create a fakeSecret + secret := fakeSecret("sandbox", "tls-secret", corev1.SecretTypeTLS, map[string][]byte{}) - <-recorder.doneCh // wait until the route's status is updated + // Get the handler + handler := rsm.generateSecretHandler(route.Namespace, route.Name) - expectedRejections := []string{"sandbox-route-test:ExternalCertificateSecretDeleted"} + // delete the secret by calling the handler directly + handler.DeleteFunc(secret) + + expectedRejections := []string{ + "sandbox-route-test:ExternalCertificateValidationFailed", + } expectedDeletedSecrets := true - if !reflect.DeepEqual(expectedRejections, recorder.rejections) { - t.Fatalf("expected rejections %v, but got %v", expectedRejections, recorder.rejections) + if !reflect.DeepEqual(expectedRejections, recorder.GetRejections()) { + t.Fatalf("expected rejections %v, but got %v", expectedRejections, recorder.GetRejections()) } - if val, _ := rsm.deletedSecrets.Load(generateKey(route.Namespace, route.Name)); !reflect.DeepEqual(val, expectedDeletedSecrets) { + if val, _ := rsm.deletedSecrets.Load(routeKey(route.Namespace, route.Name)); !reflect.DeepEqual(val, expectedDeletedSecrets) { t.Fatalf("expected deletedSecrets %v, but got %v", expectedDeletedSecrets, val) } } @@ -1380,50 +1824,110 @@ func TestSecretRecreation(t *testing.T) { }, }, } - recorder := &statusRecorder{ - doneCh: make(chan struct{}), - } + recorder := &statusRecorder{} lister := &routeLister{items: []*routev1.Route{route}} rsm := NewRouteSecretManager(&fakePlugin{}, recorder, &fake.SecretManager{}, testRouterName, &testSecretGetter{}, lister, &testSARCreator{}) - // Create a fakeSecret and start an informer for it + // Create a fakeSecret secret := fakeSecret("sandbox", "tls-secret", corev1.SecretTypeTLS, map[string][]byte{}) - kubeClient := testclient.NewSimpleClientset(secret) - informer := fakeSecretInformer(kubeClient, "sandbox", "tls-secret") - go informer.Run(context.TODO().Done()) - // wait for informer to start - if !cache.WaitForCacheSync(context.TODO().Done(), informer.HasSynced) { - t.Fatal("cache not synced yet") - } + // Get the handler + handler := rsm.generateSecretHandler(route.Namespace, route.Name) - if _, err := informer.AddEventHandler(rsm.generateSecretHandler(route.Namespace, route.Name)); err != nil { - t.Fatalf("failed to add handler: %v", err) - } + // 1. delete the secret + handler.DeleteFunc(secret) - // delete the secret - if err := kubeClient.CoreV1().Secrets(route.Namespace).Delete(context.TODO(), secret.Name, metav1.DeleteOptions{}); err != nil { - t.Fatalf("failed to delete secret: %v", err) + // 2. re-create the secret + handler.AddFunc(secret) + + expectedRejections := []string{ + "sandbox-route-test:ExternalCertificateValidationFailed", + "sandbox-route-test:ExternalCertificateSecretRecreated", + } + if !reflect.DeepEqual(expectedRejections, recorder.GetRejections()) { + t.Fatalf("expected rejections %v, but got %v", expectedRejections, recorder.GetRejections()) + } + if _, exists := rsm.deletedSecrets.Load(routeKey(route.Namespace, route.Name)); exists { + t.Fatalf("expected deletedSecrets to not have %q key", routeKey(route.Namespace, route.Name)) } +} - <-recorder.doneCh // wait until the route's status is updated (deletion) +// TestLockRouteSerializesSameKey verifies lockRoute provides genuine mutual +// exclusion per key: concurrent callers for the SAME route key never +// overlap their critical sections, while callers for DIFFERENT keys don't +// block each other at all. This is the core guarantee that closes the race +// between HandleRoute's periodic re-validation and UpdateFunc's +// secret-triggered refresh (see the routeLocks field comment) -- both call +// lockRoute with the same "namespace/routeName" key, so proving the +// primitive itself is correct here is what makes that higher-level claim +// trustworthy without needing to reproduce the full race end-to-end. +func TestLockRouteSerializesSameKey(t *testing.T) { + rsm := &RouteSecretManager{} - // re-create the secret - recorder.doneCh = make(chan struct{}) // need a new doneCh for re-creation - if _, err := kubeClient.CoreV1().Secrets(route.Namespace).Create(context.TODO(), secret, metav1.CreateOptions{}); err != nil { - t.Fatalf("failed to create secret: %v", err) - } + const goroutines = 50 + var active int32 + var maxObservedActive int32 + var wg sync.WaitGroup - <-recorder.doneCh // wait until the route's status is updated (re-creation) + for i := 0; i < goroutines; i++ { + wg.Add(1) + go func() { + defer wg.Done() + unlock := rsm.lockRoute(routeKey("sandbox", "route-test")) + defer unlock() - expectedRejections := []string{ - "sandbox-route-test:ExternalCertificateSecretDeleted", - "sandbox-route-test:ExternalCertificateSecretRecreated", + n := atomic.AddInt32(&active, 1) + for { + m := atomic.LoadInt32(&maxObservedActive) + if n <= m || atomic.CompareAndSwapInt32(&maxObservedActive, m, n) { + break + } + } + // Give another goroutine a chance to (incorrectly) enter the + // critical section concurrently, if the lock were not working. + time.Sleep(time.Millisecond) + atomic.AddInt32(&active, -1) + }() } - if !reflect.DeepEqual(expectedRejections, recorder.rejections) { - t.Fatalf("expected rejections %v, but got %v", expectedRejections, recorder.rejections) + wg.Wait() + + if maxObservedActive != 1 { + t.Fatalf("expected at most 1 goroutine in the critical section at a time for the same key, observed %d", maxObservedActive) } - if _, exists := rsm.deletedSecrets.Load(generateKey(route.Namespace, route.Name)); exists { - t.Fatalf("expected deletedSecrets to not have %q key", generateKey(route.Namespace, route.Name)) +} + +// TestLockRouteDoesNotSerializeDifferentKeys verifies lockRoute only +// serializes callers sharing the same key -- different routes must still be +// able to make progress concurrently. +func TestLockRouteDoesNotSerializeDifferentKeys(t *testing.T) { + rsm := &RouteSecretManager{} + + release := make(chan struct{}) + holding := make(chan struct{}) + + go func() { + unlock := rsm.lockRoute(routeKey("sandbox", "route-a")) + defer unlock() + close(holding) + <-release + }() + + <-holding + + done := make(chan struct{}) + go func() { + unlock := rsm.lockRoute(routeKey("sandbox", "route-b")) + unlock() + close(done) + }() + + select { + case <-done: + // Different key acquired the lock without waiting for route-a's + // holder to release -- correct. + case <-time.After(2 * time.Second): + t.Fatal("lockRoute for a different key blocked on an unrelated key's lock") } + + close(release) } diff --git a/pkg/router/controller/shared_secret_manager.go b/pkg/router/controller/shared_secret_manager.go new file mode 100644 index 000000000..26436a84e --- /dev/null +++ b/pkg/router/controller/shared_secret_manager.go @@ -0,0 +1,301 @@ +package controller + +import ( + "context" + "fmt" + "strings" + "sync" + "time" + + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/fields" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/kubernetes" + "k8s.io/client-go/tools/cache" + "k8s.io/client-go/util/workqueue" + "k8s.io/klog/v2" +) + +type informerState struct { + informer cache.SharedIndexInformer + cancel context.CancelFunc +} + +// SharedSecretManager implements secretmanager.SecretManager using per-namespace SharedIndexInformers. +// This prevents creating a new API watch for every individual route/secret combination. +// It uses a hybrid strategy: it attempts to watch all secrets in a namespace (Fast Path), +// but falls back to watching specific secrets by name if RBAC is restricted (Safe Path). +type SharedSecretManager struct { + kubeClient kubernetes.Interface + queue workqueue.RateLimitingInterface + + lock sync.RWMutex + informers map[types.NamespacedName]*informerState // Namespace is always set, Name is empty for Fast Path, or secretName for Safe Path + + // restrictedNamespaces tracks namespaces where we don't have permission to list all secrets. + // value is true if restricted, false if not restricted. + restrictedNamespaces map[string]bool + + // registeredRoutes maps "namespace/routeName" -> referencedSecret + registeredRoutes map[string]referencedSecret +} + +type referencedSecret struct { + secretName string + handler cache.ResourceEventHandlerFuncs + restricted bool // indicates if this route is using a restricted (per-secret) informer +} + +func NewSharedSecretManager(kubeClient kubernetes.Interface, queue workqueue.RateLimitingInterface) *SharedSecretManager { + return &SharedSecretManager{ + kubeClient: kubeClient, + queue: queue, + informers: make(map[types.NamespacedName]*informerState), + restrictedNamespaces: make(map[string]bool), + registeredRoutes: make(map[string]referencedSecret), + } +} + +func (m *SharedSecretManager) Queue() workqueue.RateLimitingInterface { + return m.queue +} + +func (m *SharedSecretManager) RegisterRoute(ctx context.Context, namespace string, routeName string, secretName string, handler cache.ResourceEventHandlerFuncs) error { + m.lock.Lock() + key := namespace + "/" + routeName + if ref, exists := m.registeredRoutes[key]; exists { + if ref.secretName == secretName { + // Already registered for the same secret, just update the handler and return success. + m.registeredRoutes[key] = referencedSecret{ + secretName: secretName, + handler: handler, + restricted: ref.restricted, + } + m.lock.Unlock() + return nil + } + m.lock.Unlock() + return fmt.Errorf("route already registered with key %s", key) + } + + // Determine if this namespace is restricted (e.g. resourceNames used in RBAC). + // Restricted namespaces require per-secret informers using FieldSelectors. + restricted, known := m.restrictedNamespaces[namespace] + if !known { + // Release lock during API probe to avoid blocking other registrations. + m.lock.Unlock() + // We use a metadata-only List check to see if we have namespace-wide permissions. + _, err := m.kubeClient.CoreV1().Secrets(namespace).List(ctx, metav1.ListOptions{Limit: 1}) + m.lock.Lock() + + // Re-check if someone else probed while we were unlocked. + if restricted, known = m.restrictedNamespaces[namespace]; !known { + restricted = (err != nil && apierrors.IsForbidden(err)) + m.restrictedNamespaces[namespace] = restricted + if restricted { + klog.V(2).Infof("namespace %s is restricted (RBAC), falling back to per-secret informers for routes", namespace) + } + } + } + + m.registeredRoutes[key] = referencedSecret{ + secretName: secretName, + handler: handler, + restricted: restricted, + } + + infKey := m.getInformerKey(namespace, secretName, restricted) + infState, exists := m.informers[infKey] + if !exists { + var selector fields.Selector + if restricted { + // Safe Path: Watch only this specific secret by name. + selector = fields.OneTermEqualSelector("metadata.name", secretName) + } else { + // Fast Path: Watch all secrets in the namespace. + selector = fields.Everything() + } + + inf := cache.NewSharedIndexInformer( + cache.NewListWatchFromClient( + m.kubeClient.CoreV1().RESTClient(), + "secrets", + namespace, + selector, + ), + &corev1.Secret{}, + 30*time.Second, + cache.Indexers{}, + ) + + if _, err := inf.AddEventHandler(cache.ResourceEventHandlerFuncs{ + AddFunc: func(obj interface{}) { + m.notify(namespace, obj, "Add", nil) + }, + UpdateFunc: func(oldObj, newObj interface{}) { + m.notify(namespace, newObj, "Update", oldObj) + }, + DeleteFunc: func(obj interface{}) { + m.notify(namespace, obj, "Delete", nil) + }, + }); err != nil { + delete(m.registeredRoutes, key) + m.lock.Unlock() + return fmt.Errorf("failed to add secret informer handler for key %s: %w", infKey, err) + } + + ctx, cancel := context.WithCancel(context.Background()) + infState = &informerState{ + informer: inf, + cancel: cancel, + } + m.informers[infKey] = infState + + // Start the informer + go inf.Run(ctx.Done()) + } + m.lock.Unlock() + + klog.V(4).Infof("secret manager registered route for key %s with secret %s (restricted=%v)", key, secretName, restricted) + return nil +} + +func (m *SharedSecretManager) getInformerKey(namespace, secretName string, restricted bool) types.NamespacedName { + if restricted { + return types.NamespacedName{Namespace: namespace, Name: secretName} + } + return types.NamespacedName{Namespace: namespace} +} + +func (m *SharedSecretManager) notify(namespace string, obj interface{}, eventType string, oldObj interface{}) { + var secret *corev1.Secret + switch t := obj.(type) { + case *corev1.Secret: + secret = t + case cache.DeletedFinalStateUnknown: + secret, _ = t.Obj.(*corev1.Secret) + } + + if secret == nil { + return + } + + // Find all routes in this namespace that reference this secret + var handlers []cache.ResourceEventHandlerFuncs + prefix := namespace + "/" + + m.lock.RLock() + for key, ref := range m.registeredRoutes { + if ref.secretName == secret.Name && strings.HasPrefix(key, prefix) { + handlers = append(handlers, ref.handler) + } + } + m.lock.RUnlock() + + for _, h := range handlers { + switch eventType { + case "Add": + if h.AddFunc != nil { + h.AddFunc(obj) + } + case "Update": + if h.UpdateFunc != nil { + h.UpdateFunc(oldObj, obj) + } + case "Delete": + if h.DeleteFunc != nil { + h.DeleteFunc(obj) + } + } + } +} + +func (m *SharedSecretManager) UnregisterRoute(namespace string, routeName string) error { + m.lock.Lock() + defer m.lock.Unlock() + + key := namespace + "/" + routeName + ref, exists := m.registeredRoutes[key] + if !exists { + return fmt.Errorf("no handler registered with key %s", key) + } + + delete(m.registeredRoutes, key) + + // Check if there are any remaining routes using the same informer (same namespace and same secret if restricted). + infKey := m.getInformerKey(namespace, ref.secretName, ref.restricted) + hasRoutesForInformer := false + for k, r := range m.registeredRoutes { + // Only check routes in the same namespace + if !strings.HasPrefix(k, namespace+"/") { + continue + } + if m.getInformerKey(namespace, r.secretName, r.restricted) == infKey { + hasRoutesForInformer = true + break + } + } + + // If no routes remain for this informer, stop it and clean up. + if !hasRoutesForInformer { + if infState, exists := m.informers[infKey]; exists { + infState.cancel() + delete(m.informers, infKey) + klog.V(4).Infof("secret manager shut down informer for key %s", infKey) + } + } + + klog.V(4).Infof("secret manager unregistered route for key %s", key) + return nil +} + +// GetSecret returns the secret from the informer cache or falls back to an API call. +// WARNING: To maintain high throughput and reduce GC pressure during route syncs, +// the returned Secret is a direct pointer to the shared informer cache object. +// Callers MUST NOT mutate the returned Secret. If mutation is required, the caller +// must explicitly call secret.DeepCopy() first. +func (m *SharedSecretManager) GetSecret(ctx context.Context, namespace string, routeName string) (*corev1.Secret, error) { + m.lock.RLock() + key := namespace + "/" + routeName + ref, exists := m.registeredRoutes[key] + if !exists { + m.lock.RUnlock() + return nil, fmt.Errorf("no handler registered with key %s", key) + } + + infKey := m.getInformerKey(namespace, ref.secretName, ref.restricted) + inf, infExists := m.informers[infKey] + m.lock.RUnlock() + + if !infExists { + return nil, fmt.Errorf("no informer for key %s", infKey) + } + + // Try to get from cache first + if inf.informer.HasSynced() { + obj, exists, err := inf.informer.GetStore().GetByKey(namespace + "/" + ref.secretName) + if err == nil && exists { + return obj.(*corev1.Secret), nil + } + } + + // Fallback to direct API call if cache is not synced or object is missing + secret, err := m.kubeClient.CoreV1().Secrets(namespace).Get(ctx, ref.secretName, metav1.GetOptions{}) + if err != nil { + return nil, err + } + return secret, nil +} + +func (m *SharedSecretManager) LookupRouteSecret(namespace string, routeName string) (string, bool) { + m.lock.RLock() + defer m.lock.RUnlock() + key := namespace + "/" + routeName + ref, exists := m.registeredRoutes[key] + if !exists { + return "", false + } + return ref.secretName, true +} diff --git a/pkg/router/controller/shared_secret_manager_test.go b/pkg/router/controller/shared_secret_manager_test.go new file mode 100644 index 000000000..d92d5e60c --- /dev/null +++ b/pkg/router/controller/shared_secret_manager_test.go @@ -0,0 +1,143 @@ +package controller + +import ( + "context" + "fmt" + "testing" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/kubernetes/fake" + testing2 "k8s.io/client-go/testing" + "k8s.io/client-go/tools/cache" +) + +func TestSharedSecretManagerHybrid(t *testing.T) { + scenarios := []struct { + name string + namespace string + allowList bool + expectedKey types.NamespacedName + expectedRestricted bool + }{ + { + name: "unrestricted namespace uses namespace key", + namespace: "unrestricted", + allowList: true, + expectedKey: types.NamespacedName{Namespace: "unrestricted"}, + expectedRestricted: false, + }, + { + name: "restricted namespace uses per-secret key", + namespace: "restricted", + allowList: false, + expectedKey: types.NamespacedName{Namespace: "restricted", Name: "my-secret"}, + expectedRestricted: true, + }, + } + + for _, s := range scenarios { + t.Run(s.name, func(t *testing.T) { + client := fake.NewSimpleClientset() + if !s.allowList { + client.PrependReactor("list", "secrets", func(action testing2.Action) (handled bool, ret runtime.Object, err error) { + return true, nil, apierrors.NewForbidden(schema.GroupResource{Resource: "secrets"}, "", fmt.Errorf("restricted")) + }) + } + + mgr := NewSharedSecretManager(client, nil) + ctx := context.Background() + routeName := "my-route" + secretName := "my-secret" + handler := cache.ResourceEventHandlerFuncs{} + + err := mgr.RegisterRoute(ctx, s.namespace, routeName, secretName, handler) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + mgr.lock.RLock() + infKey := mgr.getInformerKey(s.namespace, secretName, s.expectedRestricted) + if infKey != s.expectedKey { + t.Errorf("expected informer key %q, got %q", s.expectedKey, infKey) + } + + if _, exists := mgr.informers[infKey]; !exists { + t.Errorf("expected informer with key %q to exist", infKey) + } + + ref, exists := mgr.registeredRoutes[s.namespace+"/"+routeName] + if !exists { + t.Fatalf("route not registered") + } + if ref.restricted != s.expectedRestricted { + t.Errorf("expected restricted=%v, got %v", s.expectedRestricted, ref.restricted) + } + mgr.lock.RUnlock() + + // Test Unregister + err = mgr.UnregisterRoute(s.namespace, routeName) + if err != nil { + t.Fatalf("unexpected error during unregister: %v", err) + } + + mgr.lock.RLock() + if _, exists := mgr.informers[infKey]; exists { + t.Errorf("expected informer with key %q to be removed", infKey) + } + mgr.lock.RUnlock() + }) + } +} + +func TestSharedSecretManagerMultiRoute(t *testing.T) { + client := fake.NewSimpleClientset() + mgr := NewSharedSecretManager(client, nil) + ctx := context.Background() + ns := "test-ns" + secretName := "shared-secret" + + // Register first route + err := mgr.RegisterRoute(ctx, ns, "route1", secretName, cache.ResourceEventHandlerFuncs{}) + if err != nil { + t.Fatal(err) + } + + // Register second route + err = mgr.RegisterRoute(ctx, ns, "route2", secretName, cache.ResourceEventHandlerFuncs{}) + if err != nil { + t.Fatal(err) + } + + mgr.lock.RLock() + if len(mgr.informers) != 1 { + t.Errorf("expected 1 informer, got %d", len(mgr.informers)) + } + mgr.lock.RUnlock() + + // Unregister first route + err = mgr.UnregisterRoute(ns, "route1") + if err != nil { + t.Fatal(err) + } + + mgr.lock.RLock() + if len(mgr.informers) != 1 { + t.Errorf("expected informer to persist because route2 still exists") + } + mgr.lock.RUnlock() + + // Unregister second route + err = mgr.UnregisterRoute(ns, "route2") + if err != nil { + t.Fatal(err) + } + + mgr.lock.RLock() + if len(mgr.informers) != 0 { + t.Errorf("expected informer to be removed") + } + mgr.lock.RUnlock() +} diff --git a/pkg/router/controller/status.go b/pkg/router/controller/status.go index 985e3cd56..5ad869eb2 100644 --- a/pkg/router/controller/status.go +++ b/pkg/router/controller/status.go @@ -83,7 +83,6 @@ type StatusAdmitter struct { // with differing configurations are writing updates at the same time. func NewStatusAdmitter(plugin router.Plugin, client client.RoutesGetter, lister routelisters.RouteLister, name, hostName string, lease writerlease.Lease, tracker ContentionTracker) *StatusAdmitter { return &StatusAdmitter{ - lock: sync.Mutex{}, plugin: plugin, client: client, lister: lister, @@ -107,8 +106,6 @@ var nowFn = getRfc3339Timestamp // HandleRoute attempts to admit the provided route on watch add / modifications. func (a *StatusAdmitter) HandleRoute(eventType watch.EventType, route *routev1.Route) error { - a.lock.Lock() - defer a.lock.Unlock() log.V(10).Info("HandleRoute: StatusAdmitter") switch eventType { case watch.Added, watch.Modified: @@ -305,10 +302,11 @@ func handleRouteStatusUpdate(ctx context.Context, action string, oc client.Route log.V(4).Info("route was deleted before we could update status", "action", action, "namespace", route.Namespace, "name", route.Name) return writerlease.Release, false case errors.IsConflict(err): - // just follow the normal process, and retry when we receive the update notification due to - // the other entity updating the route. - log.V(4).Info("updating route status failed due to write conflict", "action", action, "namespace", route.Namespace, "name", route.Name) - return writerlease.Release, true + // A write conflict is expected under high concurrency. + // Return None instead of Release to prevent this router from dropping its leader lease, + // which would otherwise cause a 60-second delay for subsequent routes. + log.V(4).Info("updating route status failed due to write conflict, retrying", "action", action, "namespace", route.Namespace, "name", route.Name) + return writerlease.None, true default: utilruntime.HandleError(fmt.Errorf("Unable to write router status for %s/%s: %v", route.Namespace, route.Name, err)) return writerlease.Release, true @@ -339,6 +337,14 @@ func recordIngressCondition(route *routev1.Route, name, hostName string, conditi existingCondition := findCondition(existing, condition.Type) if existingCondition != nil { condition.LastTransitionTime = existingCondition.LastTransitionTime + + // Protect meaningful 'ignored' reasons from being overwritten by empty reasons (e.g., from HandleRoute's standard reconciliation). + // This prevents status flapping while allowing legitimate transitions like SecretUpdated -> SARCompleted to be persisted. + if condition.Reason == "" && ignoreIngressConditionReason.Has(existingCondition.Reason) && existingCondition.Status == condition.Status { + condition.Reason = existingCondition.Reason + condition.Message = existingCondition.Message + } + if *existingCondition != condition { changed = true } diff --git a/pkg/router/controller/status_test.go b/pkg/router/controller/status_test.go index 8c16e1d22..f5c9970c6 100644 --- a/pkg/router/controller/status_test.go +++ b/pkg/router/controller/status_test.go @@ -50,9 +50,10 @@ func (_ noopLease) Remove(key writerlease.WorkKey) { } type fakePlugin struct { - t watch.EventType - route *routev1.Route - err error + t watch.EventType + route *routev1.Route + err error + commits int } func (p *fakePlugin) HandleRoute(t watch.EventType, route *routev1.Route) error { @@ -71,7 +72,8 @@ func (p *fakePlugin) HandleNamespaces(namespaces sets.String) error { return fmt.Errorf("not expected") } func (p *fakePlugin) Commit() error { - return fmt.Errorf("not expected") + p.commits++ + return nil } type routeLister struct { @@ -1499,6 +1501,43 @@ func Test_recordIngressCondition(t *testing.T) { expectChanged: false, expectCreated: false, }, + { + name: "do not overwrite existing ignored reason with empty reason", + routerName: "foo", + routerCanonicalHostname: "router-foo.foo.local", + route: &routev1.Route{ + Spec: routev1.RouteSpec{Host: "foo.foo.local"}, + Status: routev1.RouteStatus{Ingress: []routev1.RouteIngress{{ + Host: "foo.foo.local", + RouterName: "foo", + RouterCanonicalHostname: "router-foo.foo.local", + Conditions: []routev1.RouteIngressCondition{{ + Type: routev1.RouteAdmitted, + Status: corev1.ConditionTrue, + Reason: ExtCrtStatusReasonSARCompleted, + }}}, + }}, + }, + condition: routev1.RouteIngressCondition{ + Type: routev1.RouteAdmitted, + Status: corev1.ConditionTrue, + }, + expectedRoute: &routev1.Route{ + Spec: routev1.RouteSpec{Host: "foo.foo.local"}, + Status: routev1.RouteStatus{Ingress: []routev1.RouteIngress{{ + Host: "foo.foo.local", + RouterName: "foo", + RouterCanonicalHostname: "router-foo.foo.local", + Conditions: []routev1.RouteIngressCondition{{ + Type: routev1.RouteAdmitted, + Status: corev1.ConditionTrue, + Reason: ExtCrtStatusReasonSARCompleted, + }}}, + }}, + }, + expectChanged: false, + expectCreated: false, + }, { name: "add new condition to existing ingress with existing condition", routerName: "foo", diff --git a/pkg/router/routeapihelpers/validation.go b/pkg/router/routeapihelpers/validation.go index 9cadee238..b3e1b8cdf 100644 --- a/pkg/router/routeapihelpers/validation.go +++ b/pkg/router/routeapihelpers/validation.go @@ -9,13 +9,12 @@ import ( "crypto/x509" "encoding/pem" "fmt" - "strings" + "sync" + "time" - "k8s.io/apiserver/pkg/authentication/user" "k8s.io/client-go/util/cert" routev1 "github.com/openshift/api/route/v1" - "github.com/openshift/library-go/pkg/authorization/authorizationutil" authorizationv1 "k8s.io/api/authorization/v1" kapi "k8s.io/api/core/v1" @@ -252,13 +251,6 @@ func ExtendedValidateRoute(route *routev1.Route) field.ErrorList { if len(keyBytes) == 0 { result = append(result, field.Invalid(tlsFieldPath.Child("key"), "", "no key specified")) } else { - // Validate that final key contains only private key, and cert contains only public keys - if err := validatePEMContent(keyBytes, "PRIVATE KEY"); err != nil { - result = append(result, field.Invalid(tlsFieldPath.Child("key"), "redacted key data", err.Error())) - } - if err := validatePEMContent(certBytes, "CERTIFICATE"); err != nil { - result = append(result, field.Invalid(tlsFieldPath.Child("certificate"), "redacted certificate data", err.Error())) - } // Validate if the keypair is valid (eg.: the leaf certificate should be the first on certBytes) if _, err := tls.X509KeyPair(certBytes, keyBytes); err != nil { result = append(result, field.Invalid(tlsFieldPath.Child("key"), "redacted key data", err.Error())) @@ -290,31 +282,6 @@ func ExtendedValidateRoute(route *routev1.Route) field.ErrorList { return result } -// validatePEMContent takes content and pemType, PEM decodes content and -// validates that the first block matches the expected pemType. This validation -// 1. Ensures that the required pemType is first in the order -// 2. Blocks attempts of passing certificates where keys should be used, and -// 3. Blocks attempts of passing keys where certificates should be used. -// Passing an out of order certificate, or key as certificate, or certificate as key -// breaks HAProxy. Note that the match of content and pemType is not exact, but -// content must CONTAIN the expected pemType. -func validatePEMContent(content []byte, pemType string) error { - if len(content) == 0 { - return fmt.Errorf("the PEM content cannot be null") - } - for len(content) > 0 { - var pemBlock *pem.Block - pemBlock, content = pem.Decode(content) - if pemBlock == nil { - break - } - if !strings.Contains(pemBlock.Type, pemType) { - return fmt.Errorf("field contains invalid types %s, expecting only %s", pemBlock.Type, pemType) - } - } - return nil -} - // validateTLS tests fields for different types of TLS combinations are set. Called // by ValidateRoute. func validateTLS(route *routev1.Route, fldPath *field.Path) field.ErrorList { @@ -515,56 +482,181 @@ func UpgradeRouteValidation(route *routev1.Route) field.ErrorList { return nil } -// ValidateTLSExternalCertificate tests different pre-conditions required for -// using externalCertificate. +// MaxConcurrentSARChecks limits the number of simultaneous SubjectAccessReview +// API calls to avoid overwhelming the API server during router startup with +// many externalCertificate routes. +const MaxConcurrentSARChecks = 50 + +var sarSemaphore = make(chan struct{}, MaxConcurrentSARChecks) + +// sarCacheEntry holds a cached successful SAR validation result. +type sarCacheEntry struct { + errs field.ErrorList + createdAt time.Time +} + +// sarCacheTTL determines how long successful SAR results are cached before +// revalidation. This ensures eventual consistency when RBAC permissions change. +const sarCacheTTL = 2 * time.Minute + +// sarCache stores successful SAR validation results keyed by "namespace/secretName". +// Only successful validations are cached; failures always trigger fresh checks. +var sarCache sync.Map + +// InvalidateAsyncSARCache removes the cached result for a specific secret, +// forcing revalidation on the next route event. Called when the secret is +// created, updated, or deleted. +func InvalidateAsyncSARCache(namespace, secretName string) { + sarCache.Delete(namespace + "/" + secretName) +} + +// ClearAsyncSARCacheForTest clears the global SAR cache for testing purposes. +func ClearAsyncSARCacheForTest() { + sarCache.Range(func(key, _ any) bool { + sarCache.Delete(key) + return true + }) +} + +// checkSARCache returns the cached SAR result if it exists and hasn't expired. +// Returns nil if there is no valid cache entry. +func checkSARCache(cacheKey string) *sarCacheEntry { + if raw, ok := sarCache.Load(cacheKey); ok { + entry := raw.(*sarCacheEntry) + if time.Since(entry.createdAt) > sarCacheTTL { + sarCache.Delete(cacheKey) + return nil + } + return entry + } + return nil +} + +// ValidateTLSExternalCertificate validates that the router service account has +// the required RBAC permissions to access the referenced secret and that the +// secret exists and is of type kubernetes.io/tls. +// +// This function is synchronous and throttled: it blocks on a semaphore to limit +// the number of concurrent SAR API calls, preventing API server overload during +// startup with many externalCertificate routes. Successful results are cached +// with a 2-minute TTL to avoid redundant API calls on subsequent route events. func ValidateTLSExternalCertificate(route *routev1.Route, fldPath *field.Path, sarc authorizationclient.SubjectAccessReviewInterface, secretsGetter corev1client.SecretsGetter) field.ErrorList { tls := route.Spec.TLS + if tls == nil || tls.ExternalCertificate == nil || tls.ExternalCertificate.Name == "" { + return nil + } + + secretName := tls.ExternalCertificate.Name + cacheKey := route.Namespace + "/" + secretName + + // Fast path: return cached successful result. + if entry := checkSARCache(cacheKey); entry != nil { + return entry.errs + } + + // For tests where dependencies might be mocked/nil, avoid panic. + if sarc == nil || secretsGetter == nil { + return field.ErrorList{ + field.InternalError(fldPath, fmt.Errorf("external certificate validation dependencies are not configured")), + } + } + // Acquire semaphore slot — blocks if all slots are in use. + // This throttles concurrent SAR API calls to MaxConcurrentSARChecks. + sarSemaphore <- struct{}{} + defer func() { <-sarSemaphore }() + + // Double-check cache after acquiring semaphore — another goroutine + // may have cached the result while we were waiting. + if entry := checkSARCache(cacheKey); entry != nil { + return entry.errs + } + + // Perform SAR checks synchronously. errs := field.ErrorList{} - // The router serviceaccount must have permission to get/list/watch the referenced secret. - // The role and rolebinding to provide this access must be provided by the user. - if err := authorizationutil.Authorize(sarc, &user.DefaultInfo{Name: routerServiceAccount}, - &authorizationv1.ResourceAttributes{ - Namespace: route.Namespace, - Verb: "get", - Resource: "secrets", - Name: tls.ExternalCertificate.Name, - }); err != nil { + + timeoutCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + + sarGet := &authorizationv1.SubjectAccessReview{ + Spec: authorizationv1.SubjectAccessReviewSpec{ + User: routerServiceAccount, + Groups: []string{ + "system:serviceaccounts", + "system:serviceaccounts:openshift-ingress", + "system:authenticated", + }, + ResourceAttributes: &authorizationv1.ResourceAttributes{ + Namespace: route.Namespace, Verb: "get", Resource: "secrets", Name: secretName, + }, + }, + } + resp, err := sarc.Create(timeoutCtx, sarGet, metav1.CreateOptions{}) + if err != nil { + errs = append(errs, field.InternalError(fldPath, fmt.Errorf("failed to check 'get' permission for secret %q: %v", secretName, err))) + } else if !resp.Status.Allowed { errs = append(errs, field.Forbidden(fldPath, "router serviceaccount does not have permission to get this secret")) } - if err := authorizationutil.Authorize(sarc, &user.DefaultInfo{Name: routerServiceAccount}, - &authorizationv1.ResourceAttributes{ - Namespace: route.Namespace, - Verb: "watch", - Resource: "secrets", - Name: tls.ExternalCertificate.Name, - }); err != nil { + sarWatch := &authorizationv1.SubjectAccessReview{ + Spec: authorizationv1.SubjectAccessReviewSpec{ + User: routerServiceAccount, + Groups: []string{ + "system:serviceaccounts", + "system:serviceaccounts:openshift-ingress", + "system:authenticated", + }, + ResourceAttributes: &authorizationv1.ResourceAttributes{ + Namespace: route.Namespace, Verb: "watch", Resource: "secrets", Name: secretName, + }, + }, + } + resp, err = sarc.Create(timeoutCtx, sarWatch, metav1.CreateOptions{}) + if err != nil { + errs = append(errs, field.InternalError(fldPath, fmt.Errorf("failed to check 'watch' permission for secret %q: %v", secretName, err))) + } else if !resp.Status.Allowed { errs = append(errs, field.Forbidden(fldPath, "router serviceaccount does not have permission to watch this secret")) } - if err := authorizationutil.Authorize(sarc, &user.DefaultInfo{Name: routerServiceAccount}, - &authorizationv1.ResourceAttributes{ - Namespace: route.Namespace, - Verb: "list", - Resource: "secrets", - Name: tls.ExternalCertificate.Name, - }); err != nil { + sarList := &authorizationv1.SubjectAccessReview{ + Spec: authorizationv1.SubjectAccessReviewSpec{ + User: routerServiceAccount, + Groups: []string{ + "system:serviceaccounts", + "system:serviceaccounts:openshift-ingress", + "system:authenticated", + }, + ResourceAttributes: &authorizationv1.ResourceAttributes{ + Namespace: route.Namespace, Verb: "list", Resource: "secrets", Name: secretName, + }, + }, + } + resp, err = sarc.Create(timeoutCtx, sarList, metav1.CreateOptions{}) + if err != nil { + errs = append(errs, field.InternalError(fldPath, fmt.Errorf("failed to check 'list' permission for secret %q: %v", secretName, err))) + } else if !resp.Status.Allowed { errs = append(errs, field.Forbidden(fldPath, "router serviceaccount does not have permission to list this secret")) } - // The secret should be in the same namespace as that of the route. - secret, err := secretsGetter.Secrets(route.Namespace).Get(context.TODO(), tls.ExternalCertificate.Name, metav1.GetOptions{}) + secret, err := secretsGetter.Secrets(route.Namespace).Get(timeoutCtx, secretName, metav1.GetOptions{}) if err != nil { if apierrors.IsNotFound(err) { - return append(errs, field.NotFound(fldPath, err.Error())) + errs = append(errs, field.NotFound(fldPath, err.Error())) + } else { + errs = append(errs, field.InternalError(fldPath, err)) } - return append(errs, field.InternalError(fldPath, err)) - } - - // The secret should be of type kubernetes.io/tls - if secret.Type != kapi.SecretTypeTLS { - errs = append(errs, field.Invalid(fldPath, tls.ExternalCertificate.Name, fmt.Sprintf("secret of type %q required", kapi.SecretTypeTLS))) + } else if secret.Type != kapi.SecretTypeTLS { + errs = append(errs, field.Invalid(fldPath, secretName, fmt.Sprintf("secret of type %q required", kapi.SecretTypeTLS))) + } + + // Cache only successful validations. Failures trigger a fresh check + // on the next route event, matching the original synchronous validation + // behavior where every event retried if the route wasn't yet admitted. + if len(errs) == 0 { + sarCache.Store(cacheKey, &sarCacheEntry{ + errs: errs, + createdAt: time.Now(), + }) } return errs diff --git a/pkg/router/routeapihelpers/validation_test.go b/pkg/router/routeapihelpers/validation_test.go index e26847474..ef8d7f74e 100644 --- a/pkg/router/routeapihelpers/validation_test.go +++ b/pkg/router/routeapihelpers/validation_test.go @@ -2509,8 +2509,8 @@ func TestExtendedValidateRoute(t *testing.T) { expectedErrors: 1, }, { - // Private key containing the CA chain should be rejected - name: "A key field containing Public Key/certificate attributes should be rejected", + // Private key containing the CA chain is allowed + name: "A key field containing Public Key/certificate attributes should be allowed", route: &routev1.Route{ Spec: routev1.RouteSpec{ TLS: &routev1.TLSConfig{ @@ -2520,7 +2520,7 @@ func TestExtendedValidateRoute(t *testing.T) { }, }, }, - expectedErrors: 1, + expectedErrors: 0, }, { // A certificate field that contains bundled private and public key should be allowed @@ -2578,7 +2578,7 @@ func TestExtendedValidateRoute(t *testing.T) { }, }, }, - expectedErrors: 3, + expectedErrors: 2, }, { // A cert with valid paylod but wrong PEM header should be denied @@ -2592,7 +2592,7 @@ func TestExtendedValidateRoute(t *testing.T) { }, }, }, - expectedErrors: 3, + expectedErrors: 2, }, { name: "When both Certificate and Key are empty, should not report an error", diff --git a/pkg/router/router_test.go b/pkg/router/router_test.go index 12a686476..fd2fcceee 100644 --- a/pkg/router/router_test.go +++ b/pkg/router/router_test.go @@ -67,7 +67,7 @@ func TestMain(m *testing.M) { logFlags := flag.FlagSet{} klog.InitFlags(&logFlags) if err := logFlags.Set("v", "6"); err != nil { - fmt.Println(err) + fmt.Fprintln(os.Stderr, err) os.Exit(1) } @@ -94,7 +94,7 @@ func TestMain(m *testing.M) { factory := routerSelection.NewFactory(routeClient, projectClient.ProjectV1().Projects(), client) informer := factory.CreateRoutesSharedInformer() routeLister := routelisters.NewRouteLister(informer.GetIndexer()) - lease := writerlease.New(time.Minute, 3*time.Second) + lease := writerlease.New(time.Minute, 3*time.Second, 1) go lease.Run(wait.NeverStop) tracker := controller.NewSimpleContentionTracker(informer, namespace, 60*time.Second) tracker.SetConflictMessage(fmt.Sprintf("The router detected another process is writing conflicting updates to route status with name %q. Please ensure that the configuration of all routers is consistent. Route status will not be updated as long as conflicts are detected.", namespace)) diff --git a/pkg/router/template/certmanager.go b/pkg/router/template/certmanager.go index 6dbf3b2bf..106f55f07 100644 --- a/pkg/router/template/certmanager.go +++ b/pkg/router/template/certmanager.go @@ -178,15 +178,41 @@ func newSimpleCertificateWriter() certificateWriter { } // WriteCertificate creates and writes the file identified by in . The file extension -// .pem will be added to id. +// .pem will be added to id. The write is atomic: data is written to a temporary file first, then +// renamed into place, so concurrent readers (HAProxy during reload) never observe a truncated or +// empty PEM file. func (cm *simpleCertificateWriter) WriteCertificate(directory string, id string, cert []byte) error { fileName := filepath.Join(directory, id+".pem") - err := os.WriteFile(fileName, cert, 0644) + tmpFile, err := os.CreateTemp(directory, id+"*.pem.tmp") if err != nil { - log.Error(err, "error writing certificate file", "file", fileName) + log.Error(err, "error creating temp certificate file", "directory", directory) return err } + tmpName := tmpFile.Name() + + if _, err := tmpFile.Write(cert); err != nil { + tmpFile.Close() + os.Remove(tmpName) + log.Error(err, "error writing temp certificate file", "file", tmpName) + return err + } + if err := tmpFile.Close(); err != nil { + os.Remove(tmpName) + log.Error(err, "error closing temp certificate file", "file", tmpName) + return err + } + if err := os.Chmod(tmpName, 0644); err != nil { + os.Remove(tmpName) + log.Error(err, "error setting permissions on temp certificate file", "file", tmpName) + return err + } + if err := os.Rename(tmpName, fileName); err != nil { + os.Remove(tmpName) + log.Error(err, "error renaming temp certificate file", "tmpFile", tmpName, "targetFile", fileName) + return err + } + return nil } diff --git a/pkg/router/template/certmanager_test.go b/pkg/router/template/certmanager_test.go index 5d24ae3f4..d884a713e 100644 --- a/pkg/router/template/certmanager_test.go +++ b/pkg/router/template/certmanager_test.go @@ -1,8 +1,12 @@ package templaterouter import ( + "os" + "path/filepath" "reflect" "sort" + "sync" + "sync/atomic" "testing" routev1 "github.com/openshift/api/route/v1" @@ -243,3 +247,73 @@ func TestCertManagerConfig(t *testing.T) { } } } + +// TestWriteCertificateAtomicity verifies that WriteCertificate's temp+rename +// approach prevents concurrent readers from observing truncated or empty PEM +// files. A writer goroutine repeatedly overwrites the cert while a reader +// goroutine checks that the file is never empty or truncated. +func TestWriteCertificateAtomicity(t *testing.T) { + dir := t.TempDir() + writer := &simpleCertificateWriter{} + + certA := []byte("-----BEGIN RSA PRIVATE KEY-----\nAAAAAAAAAAAAAAAA\n-----END RSA PRIVATE KEY-----\n-----BEGIN CERTIFICATE-----\nBBBBBBBBBBBBBBBB\n-----END CERTIFICATE-----\n") + certB := []byte("-----BEGIN RSA PRIVATE KEY-----\nCCCCCCCCCCCCCCCC\n-----END RSA PRIVATE KEY-----\n-----BEGIN CERTIFICATE-----\nDDDDDDDDDDDDDDDD\n-----END CERTIFICATE-----\n") + + // Seed the file so there is always something to read. + if err := writer.WriteCertificate(dir, "test", certA); err != nil { + t.Fatalf("initial write failed: %v", err) + } + + const iterations = 2000 + var truncatedReads atomic.Int64 + var emptyReads atomic.Int64 + var totalReads atomic.Int64 + + var wg sync.WaitGroup + + // Writer goroutine: alternates between certA and certB. + wg.Add(1) + go func() { + defer wg.Done() + for i := 0; i < iterations; i++ { + cert := certA + if i%2 == 1 { + cert = certB + } + if err := writer.WriteCertificate(dir, "test", cert); err != nil { + t.Errorf("WriteCertificate iteration %d: %v", i, err) + return + } + } + }() + + // Reader goroutine: reads the PEM file and checks for truncation. + wg.Add(1) + go func() { + defer wg.Done() + pemPath := filepath.Join(dir, "test.pem") + for i := 0; i < iterations*2; i++ { + data, err := os.ReadFile(pemPath) + if err != nil { + continue + } + totalReads.Add(1) + if len(data) == 0 { + emptyReads.Add(1) + } else if len(data) < len(certA) && len(data) < len(certB) { + truncatedReads.Add(1) + } + } + }() + + wg.Wait() + + t.Logf("total reads: %d, empty: %d, truncated: %d", + totalReads.Load(), emptyReads.Load(), truncatedReads.Load()) + + if emptyReads.Load() > 0 || truncatedReads.Load() > 0 { + t.Errorf("observed %d empty and %d truncated reads out of %d total — "+ + "WriteCertificate must use atomic temp+rename to prevent HAProxy from reading partial PEM files during reload", + emptyReads.Load(), truncatedReads.Load(), totalReads.Load()) + } +} diff --git a/pkg/router/template/configmanager/haproxy/testing/haproxy.go b/pkg/router/template/configmanager/haproxy/testing/haproxy.go index 27c5c648b..c2b6b017f 100644 --- a/pkg/router/template/configmanager/haproxy/testing/haproxy.go +++ b/pkg/router/template/configmanager/haproxy/testing/haproxy.go @@ -46,7 +46,7 @@ type fakeHAProxy struct { } func startFakeHAProxyServer(prefix string) (*fakeHAProxy, error) { - f, err := os.CreateTemp(os.TempDir(), prefix) + f, err := os.CreateTemp("", prefix) if err != nil { return nil, err } @@ -59,8 +59,7 @@ func startFakeHAProxyServer(prefix string) (*fakeHAProxy, error) { } func StartFakeServerForTest(t *testing.T) *fakeHAProxy { - // Shorten the prefix to avoid hitting the 104/108 byte UNIX domain socket path length limit. - server, err := startFakeHAProxyServer("fake-haproxy-") + server, err := startFakeHAProxyServer("fake-haproxy-*") if err != nil { t.Errorf("%s error: %v", t.Name(), err) } @@ -107,12 +106,10 @@ func (p *fakeHAProxy) Commands() []string { func (p *fakeHAProxy) Start() { started := make(chan bool) - listenErr := make(chan error, 1) - go func() error { + go func() { listener, err := net.Listen("unix", p.socketFile) if err != nil { - listenErr <- err - return err + panic(fmt.Sprintf("fakeHAProxy Start failed to listen on %s: %v", p.socketFile, err)) } started <- true @@ -121,22 +118,18 @@ func (p *fakeHAProxy) Start() { shutdown := p.shutdown p.lock.Unlock() if shutdown { - return nil + return } conn, err := listener.Accept() if err != nil { - return err + return } go p.process(conn) } }() - // wait for server to indicate it started up or failed. - select { - case <-started: - case err := <-listenErr: - panic(fmt.Sprintf("fakeHAProxy: failed to listen on %s: %v", p.socketFile, err)) - } + // wait for server to indicate it started up. + <-started } func (p *fakeHAProxy) Stop() { diff --git a/pkg/router/template/router.go b/pkg/router/template/router.go index 22eed080d..c4100aa1e 100644 --- a/pkg/router/template/router.go +++ b/pkg/router/template/router.go @@ -49,6 +49,8 @@ const ( // '_' is not used as this could be part of the name in the future // '/' is not safe to use in names of router config files routeKeySeparator = ":" + + certResourceVersionAnnotation = "router.openshift.io/cert-resource-version" ) // templateRouter is a backend-agnostic router implementation @@ -1105,6 +1107,7 @@ func (r *templateRouter) createServiceAliasConfig(route *routev1.Route, backendK ActiveServiceUnits: activeServiceUnits, HTTPResponseHeaders: httpResponseHeadersList, HTTPRequestHeaders: httpRequestHeadersList, + CertResourceVersion: route.Annotations[certResourceVersionAnnotation], } if route.Spec.Port != nil { @@ -1209,6 +1212,14 @@ func (r *templateRouter) AddRoute(route *routev1.Route) { return } + if isStaleExtCert(newConfig, &existingConfig) { + log.V(4).Info("dropping stale external certificate update", + "namespace", route.Namespace, "name", route.Name, + "existingVersion", existingConfig.CertResourceVersion, + "incomingVersion", newConfig.CertResourceVersion) + return + } + log.V(4).Info("updating route", "namespace", route.Namespace, "name", route.Name) // Delete the route first, because modify is to be treated as delete+add @@ -1583,6 +1594,25 @@ func configsAreEqual(config1, config2 *ServiceAliasConfig) bool { reflect.DeepEqual(config1.ServiceUnits, config2.ServiceUnits) } +// isStaleExtCert returns true when incoming carries an external-certificate +// ResourceVersion that is numerically <= the existing one, indicating the +// incoming config was built from a stale secret read. Both versions must +// be non-empty (only set for externalCertificate routes) for the guard to +// fire; routes without externalCertificate always return false. +// Non-cert route changes (spec edits, label changes, etc.) never carry a +// CertResourceVersion, so this guard cannot drop legitimate route updates. +func isStaleExtCert(incoming, existing *ServiceAliasConfig) bool { + if incoming.CertResourceVersion == "" || existing.CertResourceVersion == "" { + return false + } + inV, errIn := strconv.ParseInt(incoming.CertResourceVersion, 10, 64) + exV, errEx := strconv.ParseInt(existing.CertResourceVersion, 10, 64) + if errIn != nil || errEx != nil { + return false + } + return inV <= exV +} + // privateKeysFromPEM extracts all blocks recognized as private keys into an output PEM encoded byte array, // or returns an error. If there are no private keys it will return an empty byte buffer. func privateKeysFromPEM(pemCerts []byte) ([]byte, error) { diff --git a/pkg/router/template/router_test.go b/pkg/router/template/router_test.go index b3915e9de..d67791f1b 100644 --- a/pkg/router/template/router_test.go +++ b/pkg/router/template/router_test.go @@ -1516,6 +1516,88 @@ func Test_configsAreEqual(t *testing.T) { } } +func TestAddRouteStalenessGuard(t *testing.T) { + router := NewFakeTemplateRouter() + + route := &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "ns", + Name: "extcert-route", + Annotations: map[string]string{ + certResourceVersionAnnotation: "100", + }, + }, + Spec: routev1.RouteSpec{ + Host: "extcert.example.com", + To: routev1.RouteTargetReference{Name: "svc"}, + TLS: &routev1.TLSConfig{ + Termination: routev1.TLSTerminationEdge, + Certificate: "cert-v100", + Key: "key-v100", + }, + }, + } + router.AddRoute(route) + + backendKey := routeKey(route) + cfg, exists := router.state[backendKey] + if !exists { + t.Fatal("route not added to state") + } + if cfg.CertResourceVersion != "100" { + t.Fatalf("expected CertResourceVersion '100', got %q", cfg.CertResourceVersion) + } + + // Update with a newer version — should be accepted. + routeV200 := route.DeepCopy() + routeV200.Annotations[certResourceVersionAnnotation] = "200" + routeV200.Spec.TLS.Certificate = "cert-v200" + routeV200.Spec.TLS.Key = "key-v200" + router.AddRoute(routeV200) + + cfg = router.state[backendKey] + if cfg.CertResourceVersion != "200" { + t.Fatalf("expected CertResourceVersion '200' after newer update, got %q", cfg.CertResourceVersion) + } + + // Update with an older version — should be silently dropped. + routeV150 := route.DeepCopy() + routeV150.Annotations[certResourceVersionAnnotation] = "150" + routeV150.Spec.TLS.Certificate = "cert-v150" + routeV150.Spec.TLS.Key = "key-v150" + router.AddRoute(routeV150) + + cfg = router.state[backendKey] + if cfg.CertResourceVersion != "200" { + t.Fatalf("stale update should have been dropped, but CertResourceVersion is %q (expected '200')", cfg.CertResourceVersion) + } + + // Update with the same version — should be dropped (<=). + routeV200Again := route.DeepCopy() + routeV200Again.Annotations[certResourceVersionAnnotation] = "200" + routeV200Again.Spec.TLS.Certificate = "cert-v200-again" + routeV200Again.Spec.TLS.Key = "key-v200-again" + router.AddRoute(routeV200Again) + + cfg = router.state[backendKey] + if cfg.CertResourceVersion != "200" { + t.Fatalf("same-version update should have been dropped, but CertResourceVersion is %q", cfg.CertResourceVersion) + } + + // Update without cert annotation (non-extcert route update) — should proceed normally. + routeNoCert := route.DeepCopy() + routeNoCert.Annotations = map[string]string{} + routeNoCert.Spec.TLS.Certificate = "cert-nocert" + routeNoCert.Spec.TLS.Key = "key-nocert" + routeNoCert.Spec.Path = "/changed" + router.AddRoute(routeNoCert) + + cfg = router.state[backendKey] + if cfg.Path != "/changed" { + t.Fatalf("non-extcert update should have proceeded, but Path is %q", cfg.Path) + } +} + const ( testWildcardCertificate = `-----BEGIN CERTIFICATE----- MIIFJjCCAw4CCQCLGB4wxqgxHjANBgkqhkiG9w0BAQsFADBOMQswCQYDVQQGEwJV diff --git a/pkg/router/template/types.go b/pkg/router/template/types.go index 292d4d29a..d48f880a3 100644 --- a/pkg/router/template/types.go +++ b/pkg/router/template/types.go @@ -87,6 +87,16 @@ type ServiceAliasConfig struct { // PrimaryServiceUnitKey is the key of the primary service of the route. PrimaryServiceUnitKey ServiceUnitKey + + // CertResourceVersion is the Kubernetes ResourceVersion of the secret + // that supplied the TLS certificate for this config. It is set only + // for routes using externalCertificate and is used as a staleness + // guard in AddRoute: an incoming config whose CertResourceVersion is + // numerically <= the existing one is silently dropped, preventing + // stale cert data from overwriting fresher data. This field is + // deliberately excluded from configsAreEqual — it is a version + // marker, not user-visible configuration. + CertResourceVersion string } type ServiceAliasConfigStatus string diff --git a/pkg/router/writerlease/writerlease.go b/pkg/router/writerlease/writerlease.go index 67244c5c9..14591912a 100644 --- a/pkg/router/writerlease/writerlease.go +++ b/pkg/router/writerlease/writerlease.go @@ -98,11 +98,13 @@ type WriterLease struct { state State expires time.Time tick int + + workers int } // New creates a new Lease. Specify the duration to hold leases for and the retry // interval on requests that fail. -func New(leaseDuration, retryInterval time.Duration) *WriterLease { +func New(leaseDuration, retryInterval time.Duration, workers int) *WriterLease { backoff := wait.Backoff{ Duration: 20 * time.Millisecond, Factor: 4, @@ -110,6 +112,10 @@ func New(leaseDuration, retryInterval time.Duration) *WriterLease { Jitter: 0.5, } + if workers < 1 { + workers = 1 + } + return &WriterLease{ name: fmt.Sprintf("%08d", rand.Int31()), backoff: backoff, @@ -120,12 +126,18 @@ func New(leaseDuration, retryInterval time.Duration) *WriterLease { queued: make(map[WorkKey]*work), queue: workqueue.NewDelayingQueue(), once: make(chan struct{}), + + workers: workers, } } // NewWithBackoff creates a new Lease. Specify the duration to hold leases for and the retry // interval on requests that fail. -func NewWithBackoff(name string, leaseDuration, retryInterval time.Duration, backoff wait.Backoff) *WriterLease { +func NewWithBackoff(name string, leaseDuration, retryInterval time.Duration, backoff wait.Backoff, workers int) *WriterLease { + if workers < 1 { + workers = 1 + } + return &WriterLease{ name: name, backoff: backoff, @@ -136,21 +148,29 @@ func NewWithBackoff(name string, leaseDuration, retryInterval time.Duration, bac queued: make(map[WorkKey]*work), queue: workqueue.NewNamedDelayingQueue(name), once: make(chan struct{}), + + workers: workers, } } func (l *WriterLease) Run(stopCh <-chan struct{}) { defer utilruntime.HandleCrash() - defer l.queue.ShutDown() - go func() { - defer utilruntime.HandleCrash() - for l.work() { - } - log.V(4).Info("worker stopped", "worker", l.name) - }() + var wg sync.WaitGroup + for i := 0; i < l.workers; i++ { + wg.Add(1) + go func(workerID int) { + defer utilruntime.HandleCrash() + defer wg.Done() + for l.work() { + } + log.V(4).Info("worker stopped", "worker", l.name, "workerID", workerID) + }(i) + } <-stopCh + l.queue.ShutDown() + wg.Wait() } func (l *WriterLease) Expire() { @@ -250,9 +270,8 @@ func (l *WriterLease) work() bool { if leaseState == Follower { // if we are following, continue to defer work until the lease expires if remaining := leaseExpires.Sub(l.nowFn()); remaining > 0 { - log.V(4).Info("follower awaiting lease expiration", "worker", l.name, "key", key, "leaseTimeRemaining", remaining) - time.Sleep(remaining) - l.queue.Add(key) + log.V(4).Info("follower awaiting lease expiration, requeueing", "worker", l.name, "key", key, "leaseTimeRemaining", remaining) + l.queue.AddAfter(key, remaining) l.queue.Done(key) return true } diff --git a/pkg/router/writerlease/writerlease_test.go b/pkg/router/writerlease/writerlease_test.go index 14fd3baeb..0cb990a23 100644 --- a/pkg/router/writerlease/writerlease_test.go +++ b/pkg/router/writerlease/writerlease_test.go @@ -6,7 +6,7 @@ import ( ) func TestWaitForLeader(t *testing.T) { - l := New(0, 0) + l := New(0, 0, 1) defer func() { if len(l.queued) > 0 { t.Fatalf("queue was not empty on shutdown: %#v", l.queued) @@ -30,7 +30,7 @@ func TestWaitForLeader(t *testing.T) { } func TestBecomeLeaderAfterRetry(t *testing.T) { - l := New(0, 0) + l := New(0, 0, 1) ch := make(chan struct{}) defer close(ch) go l.Run(ch) @@ -50,7 +50,7 @@ func TestBecomeLeaderAfterRetry(t *testing.T) { } func TestBecomeFollowerAfterRetry(t *testing.T) { - l := New(0, 0) + l := New(0, 0, 1) l.backoff.Steps = 0 l.backoff.Duration = 0 ch := make(chan struct{}) @@ -72,7 +72,7 @@ func TestBecomeFollowerAfterRetry(t *testing.T) { } func TestRunOverlappingWork(t *testing.T) { - l := New(0, 0) + l := New(0, 0, 1) l.backoff.Steps = 0 l.backoff.Duration = 0 done := make(chan struct{}) @@ -112,7 +112,7 @@ func TestRunOverlappingWork(t *testing.T) { } func TestExtend(t *testing.T) { - l := New(10*time.Millisecond, 0) + l := New(10*time.Millisecond, 0, 1) l.nowFn = func() time.Time { return time.Unix(0, 0) } l.backoff.Steps = 0 l.backoff.Duration = 2 * time.Millisecond diff --git a/vendor/github.com/openshift/library-go/pkg/authorization/authorizationutil/subject.go b/vendor/github.com/openshift/library-go/pkg/authorization/authorizationutil/subject.go deleted file mode 100644 index 74c179e68..000000000 --- a/vendor/github.com/openshift/library-go/pkg/authorization/authorizationutil/subject.go +++ /dev/null @@ -1,56 +0,0 @@ -package authorizationutil - -import ( - rbacv1 "k8s.io/api/rbac/v1" - "k8s.io/apiserver/pkg/authentication/serviceaccount" -) - -func BuildRBACSubjects(users, groups []string) []rbacv1.Subject { - subjects := []rbacv1.Subject{} - - for _, user := range users { - saNamespace, saName, err := serviceaccount.SplitUsername(user) - if err == nil { - subjects = append(subjects, rbacv1.Subject{Kind: rbacv1.ServiceAccountKind, Namespace: saNamespace, Name: saName}) - } else { - subjects = append(subjects, rbacv1.Subject{Kind: rbacv1.UserKind, APIGroup: rbacv1.GroupName, Name: user}) - } - } - - for _, group := range groups { - subjects = append(subjects, rbacv1.Subject{Kind: rbacv1.GroupKind, APIGroup: rbacv1.GroupName, Name: group}) - } - - return subjects -} - -func RBACSubjectsToUsersAndGroups(subjects []rbacv1.Subject, defaultNamespace string) (users []string, groups []string) { - for _, subject := range subjects { - - switch { - case subject.APIGroup == rbacv1.GroupName && subject.Kind == rbacv1.GroupKind: - groups = append(groups, subject.Name) - case subject.APIGroup == rbacv1.GroupName && subject.Kind == rbacv1.UserKind: - users = append(users, subject.Name) - case subject.APIGroup == "" && subject.Kind == rbacv1.ServiceAccountKind: - // default the namespace to namespace we're working in if - // it's available. This allows rolebindings that reference - // SAs in the local namespace to avoid having to qualify - // them. - ns := defaultNamespace - if len(subject.Namespace) > 0 { - ns = subject.Namespace - } - if len(ns) > 0 { - name := serviceaccount.MakeUsername(ns, subject.Name) - users = append(users, name) - } else { - // maybe error? this fails safe at any rate - } - default: - // maybe error? This fails safe at any rate - } - } - - return users, groups -} diff --git a/vendor/github.com/openshift/library-go/pkg/authorization/authorizationutil/util.go b/vendor/github.com/openshift/library-go/pkg/authorization/authorizationutil/util.go deleted file mode 100644 index 040d0f643..000000000 --- a/vendor/github.com/openshift/library-go/pkg/authorization/authorizationutil/util.go +++ /dev/null @@ -1,50 +0,0 @@ -package authorizationutil - -import ( - "context" - "errors" - - authorizationv1 "k8s.io/api/authorization/v1" - kerrors "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime/schema" - "k8s.io/apiserver/pkg/authentication/user" - authorizationclient "k8s.io/client-go/kubernetes/typed/authorization/v1" -) - -// AddUserToSAR adds the requisite user information to a SubjectAccessReview. -// It returns the modified SubjectAccessReview. -func AddUserToSAR(user user.Info, sar *authorizationv1.SubjectAccessReview) *authorizationv1.SubjectAccessReview { - sar.Spec.User = user.GetName() - // reminiscent of the bad old days of C. Copies copy the min number of elements of both source and dest - sar.Spec.Groups = make([]string, len(user.GetGroups())) - copy(sar.Spec.Groups, user.GetGroups()) - sar.Spec.Extra = map[string]authorizationv1.ExtraValue{} - - for k, v := range user.GetExtra() { - sar.Spec.Extra[k] = authorizationv1.ExtraValue(v) - } - - return sar -} - -// Authorize verifies that a given user is permitted to carry out a given -// action. If this cannot be determined, or if the user is not permitted, an -// error is returned. -func Authorize(sarClient authorizationclient.SubjectAccessReviewInterface, user user.Info, resourceAttributes *authorizationv1.ResourceAttributes) error { - sar := AddUserToSAR(user, &authorizationv1.SubjectAccessReview{ - Spec: authorizationv1.SubjectAccessReviewSpec{ - ResourceAttributes: resourceAttributes, - }, - }) - - resp, err := sarClient.Create(context.TODO(), sar, metav1.CreateOptions{}) - if err == nil && resp != nil && resp.Status.Allowed { - return nil - } - - if err == nil { - err = errors.New(resp.Status.Reason) - } - return kerrors.NewForbidden(schema.GroupResource{Group: resourceAttributes.Group, Resource: resourceAttributes.Resource}, resourceAttributes.Name, err) -} diff --git a/vendor/modules.txt b/vendor/modules.txt index 08837174c..034d20645 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -251,7 +251,6 @@ github.com/openshift/client-go/route/clientset/versioned/typed/route/v1/fake github.com/openshift/client-go/route/listers/route/v1 # github.com/openshift/library-go v0.0.0-20260713143403-795ac1a480b5 ## explicit; go 1.25.0 -github.com/openshift/library-go/pkg/authorization/authorizationutil github.com/openshift/library-go/pkg/crypto github.com/openshift/library-go/pkg/proc github.com/openshift/library-go/pkg/route/secretmanager