From 1b33dd8c51b8f4ccd8d07d63b6c5328a1ec0985a Mon Sep 17 00:00:00 2001 From: chiragkyal Date: Mon, 21 Oct 2024 12:45:46 +0530 Subject: [PATCH] Use RouteLister and update route's status during secret events Signed-off-by: chiragkyal --- pkg/cmd/infra/router/template.go | 6 +- pkg/router/controller/host_admitter_test.go | 4 + pkg/router/controller/route_secret_manager.go | 175 ++++-- .../controller/route_secret_manager_test.go | 547 ++++++------------ pkg/router/controller/status.go | 17 + pkg/router/template/plugin_test.go | 3 + 6 files changed, 341 insertions(+), 411 deletions(-) diff --git a/pkg/cmd/infra/router/template.go b/pkg/cmd/infra/router/template.go index 898062b21..1431d87d5 100644 --- a/pkg/cmd/infra/router/template.go +++ b/pkg/cmd/infra/router/template.go @@ -793,14 +793,14 @@ func (o *TemplateRouterOptions) Run(stopCh <-chan struct{}) error { var plugin router.Plugin = templatePlugin var recorder controller.RouteStatusRecorder = controller.LogRejections + informer := factory.CreateRoutesSharedInformer() + routeLister := routelisters.NewRouteLister(informer.GetIndexer()) if o.UpdateStatus { lease := writerlease.New(time.Minute, 3*time.Second) go lease.Run(stopCh) - informer := factory.CreateRoutesSharedInformer() 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)) go tracker.Run(stopCh) - routeLister := routelisters.NewRouteLister(informer.GetIndexer()) status := controller.NewStatusAdmitter(plugin, routeclient.RouteV1(), routeLister, o.RouterName, o.RouterCanonicalHostname, lease, tracker) recorder = status plugin = status @@ -812,7 +812,7 @@ func (o *TemplateRouterOptions) Run(stopCh <-chan struct{}) error { plugin = controller.NewExtendedValidator(plugin, recorder) } if o.AllowExternalCertificates { - plugin = controller.NewRouteSecretManager(plugin, recorder, secretManager, kc.CoreV1(), authorizationClient.SubjectAccessReviews()) + plugin = controller.NewRouteSecretManager(plugin, recorder, secretManager, kc.CoreV1(), routeLister, authorizationClient.SubjectAccessReviews()) } plugin = controller.NewUniqueHost(plugin, o.RouterSelection.DisableNamespaceOwnershipCheck, recorder) plugin = controller.NewHostAdmitter(plugin, o.RouteAdmissionFunc(), o.AllowWildcardRoutes, o.RouterSelection.DisableNamespaceOwnershipCheck, recorder) diff --git a/pkg/router/controller/host_admitter_test.go b/pkg/router/controller/host_admitter_test.go index d2f7069fa..41ef89d80 100644 --- a/pkg/router/controller/host_admitter_test.go +++ b/pkg/router/controller/host_admitter_test.go @@ -35,6 +35,10 @@ func (r routeStatusRecorder) RecordRouteRejection(route *routev1.Route, reason, r.rejections[r.rejectionKey(route)] = reason } +func (r routeStatusRecorder) RecordRouteUpdate(route *routev1.Route, reason, message string) { + panic("not implemented") +} + func (r routeStatusRecorder) RecordRouteUnservableInFutureVersionsClear(route *routev1.Route) { delete(r.unservableInFutureVersions, r.rejectionKey(route)) } diff --git a/pkg/router/controller/route_secret_manager.go b/pkg/router/controller/route_secret_manager.go index b00241fe9..ae51a0538 100644 --- a/pkg/router/controller/route_secret_manager.go +++ b/pkg/router/controller/route_secret_manager.go @@ -6,6 +6,7 @@ import ( "sync" routev1 "github.com/openshift/api/route/v1" + routelisters "github.com/openshift/client-go/route/listers/route/v1" "github.com/openshift/library-go/pkg/route/secretmanager" "github.com/openshift/router/pkg/router" "github.com/openshift/router/pkg/router/routeapihelpers" @@ -29,6 +30,7 @@ type RouteSecretManager struct { secretManager secretmanager.SecretManager secretsGetter corev1client.SecretsGetter + routelister routelisters.RouteLister sarClient authorizationclient.SubjectAccessReviewInterface // deletedSecrets tracks routes for which the associated secret was deleted after intial creation of the secret monitor. // This helps to differentiate between a new secret creation and a recreation of a previously deleted secret. @@ -39,12 +41,20 @@ type RouteSecretManager struct { // NewRouteSecretManager creates a new instance of RouteSecretManager. // It wraps the provided plugin and adds secret management capabilities. -func NewRouteSecretManager(plugin router.Plugin, recorder RouteStatusRecorder, secretManager secretmanager.SecretManager, secretsGetter corev1client.SecretsGetter, sarClient authorizationclient.SubjectAccessReviewInterface) *RouteSecretManager { +func NewRouteSecretManager( + plugin router.Plugin, + recorder RouteStatusRecorder, + secretManager secretmanager.SecretManager, + secretsGetter corev1client.SecretsGetter, + routelister routelisters.RouteLister, + sarClient authorizationclient.SubjectAccessReviewInterface, +) *RouteSecretManager { return &RouteSecretManager{ plugin: plugin, recorder: recorder, secretManager: secretManager, secretsGetter: secretsGetter, + routelister: routelister, sarClient: sarClient, deletedSecrets: sync.Map{}, } @@ -95,10 +105,6 @@ func (p *RouteSecretManager) HandleRoute(eventType watch.EventType, route *route } } - // TODO always creating a new `secretHandler` ensures that there are no stale route specs - // in the next plugin chain, especially when the referenced secret is updated or deleted. - // This prevents sending outdated routes to subsequent plugins, preserving expected functionality. - // TODO : we might need to add RouteLister() case watch.Modified: // Determine if the route's external certificate configuration has changed newHasExt := hasExternalCertificate(route) @@ -120,13 +126,23 @@ func (p *RouteSecretManager) HandleRoute(eventType watch.EventType, route *route } } else { // ExternalCertificate is not updated - // Revalidate and update the in-memory TLS certificate and key (even if ExternalCertificate remains unchanged) - // It is the responsibility of this plugin to ensure everything is synced properly. - - // One Scenario: The user deletes the secret, causing the route's status to be updated to reject. - // This triggers the entire plugin chain again. Without re-validating the external certificate - // and re-syncing the secret here, the route could become active again and start serving - // the default certificate, even though its spec has an external certificate. + // Re-validate and update the in-memory TLS certificate and key (even if ExternalCertificate remains unchanged) + // It is the responsibility of this plugin to ensure everything is synced properly, because there might + // have been updates to the secret data or other events that require re-evaluation. + // + // 1. Secret update and re-create: Even if the externalCertificate name remains + // the same, the router needs to be aware of the events when the content of + // the secret is updated or the secret is recreated, to re-validate and + // fetch the new TLS data. Note: These events won't trigger the apiserver + // route admission, hence we need to rely on the router controller for this validation. + // + // 2. Consider a case where a user deletes the secret, causing the route's + // status to be updated to "reject". This status update triggers the + // entire plugin chain again. Without re-validating the external certificate + // and re-syncing the secret here, the route could incorrectly transition + // back to an "active" state and start serving the default certificate + // even though its spec still references an external certificate. + // // Therefore, it is essential to re-sync the secret to ensure the plugin chain correctly handles the route. log.V(4).Info("ExternalCertificate is not updated", "namespace", route.Namespace, "route", route.Name) @@ -181,7 +197,7 @@ func (p *RouteSecretManager) validateAndRegister(route *routev1.Route) error { return err } // register route with secretManager - handler := p.generateSecretHandler(route) + 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 { log.Error(err, "failed to register route") return err @@ -196,78 +212,107 @@ func (p *RouteSecretManager) validateAndRegister(route *routev1.Route) error { // generateSecretHandler creates ResourceEventHandlerFuncs to handle Add, Update, and Delete events on secrets. // -// AddFunc: Invoked when a new secret is added. It logs the addition of the secret. This function also handles the -// re-creation scenario where a previously deleted secret is added again. In such cases, it revalidates the route's -// external certificate configuration, updates the route's TLS certificate and key, and calls the next plugin's HandleRoute method with a watch.Modified event, and then commits the changes by calling the next plugin's Commit() method. +// To ensure that the handlers always operate on the most up-to-date route object, +// it fetches the latest route from the informer and then updates the route's status +// with specific reasons related to the secret event. This status update is crucial +// because it serves as a signal to trigger the entire route plugin chain to re-evaluate +// the route from the beginning. +// +// Triggering a re-evaluation ensures that both: +// - Changes made by `RouteModifierFn` registered with the `RouterControllerFactory` +// are propagated correctly to all plugins. +// - All plugins get a chance to react to the changes and make necessary +// in-memory modifications to the route object, ensuring consistent behavior. // -// UpdateFunc: Invoked when an existing secret is updated. It performs validation of the route's external certificate configuration. -// If the validation succeeds, it updates the route's TLS certificate and key with the new secret data and calls the next plugin's HandleRoute method with a watch.Modified event, and then commits the changes by calling the next plugin's Commit() method. +// - AddFunc: +// - Invoked when a new secret is added. +// - Handles secret recreation: If a secret is recreated (created after being +// deleted), it fetches the latest route object associated with it using the +// RouteLister and updates the route's status to trigger a full +// re-evaluation of the route by the entire plugin chain. // -// DeleteFunc: Invoked when the secret is deleted. It logs the deletion, marks the secret as deleted for this route in the deletedSecrets map, -// records the route rejection, and triggers the deletion of the route by calling the HandleRoute method with a watch.Deleted event. NOTE: It doesn't unregister the route. -func (p *RouteSecretManager) generateSecretHandler(route *routev1.Route) cache.ResourceEventHandlerFuncs { +// - UpdateFunc: +// - Invoked when an existing secret is updated. +// - Fetches the latest associated route object and +// updates the route's status to trigger a re-evaluation by the entire plugin chain. +// +// - DeleteFunc: +// - Invoked when the secret is deleted. +// - Marks the secret as deleted for the associated route in the `deletedSecrets` map. +// - Fetches the latest associated route object and records a route rejection event. +// - Triggers the deletion of the route by calling the HandleRoute method with a watch.Deleted event. +// - NOTE: It doesn't unregister the route. +func (p *RouteSecretManager) generateSecretHandler(namespace, routeName string) cache.ResourceEventHandlerFuncs { // secret handler return cache.ResourceEventHandlerFuncs{ AddFunc: func(obj interface{}) { secret := obj.(*kapi.Secret) - log.V(4).Info("secret added for route", "namespace", route.Namespace, "secret", secret.Name, "route", route.Name) + log.V(4).Info("secret added for route", "namespace", namespace, "secret", secret.Name, "route", routeName) // Secret re-creation scenario // Check if the route key exists in the deletedSecrets map, indicating that the secret was previously deleted for this route. - // If it exists, it means the secret is being recreated. Remove the key from the map and proceed with re-registration. + // 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(route) + key := generateKey(namespace, routeName) if _, deleted := p.deletedSecrets.LoadAndDelete(key); deleted { - log.V(4).Info("secret recreated for route", "namespace", route.Namespace, "secret", secret.Name, "route", route.Name) + msg := fmt.Sprintf("secret %q recreated for route %q", secret.Name, key) + log.V(4).Info(msg) - // re-validate - // since secret re-creation will not trigger the apiserver route admission, - // we need to rely on the router controller for this validation. - if err := p.validate(route); err != nil { + // Ensure fetching the updated route + route, err := p.getUpdatedRoute(namespace, routeName) + if err != nil { return } - // read the re-created secret and update TLS certificate and key - if err := p.populateRouteTLSFromSecret(route); err != nil { - return - } - // call the next plugin with watch.Modified - p.plugin.HandleRoute(watch.Modified, route) - // commit the changes - p.plugin.Commit() + + // 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. + p.recorder.RecordRouteRejection(route, "ExternalCertificateSecretRecreated", msg) } }, UpdateFunc: func(old interface{}, new interface{}) { secretOld := old.(*kapi.Secret) secretNew := new.(*kapi.Secret) - log.V(4).Info("secret updated for route", "namespace", route.Namespace, "secret", secretNew.Name, "old-version", secretOld.ResourceVersion, "new-version", secretNew.ResourceVersion, "route", route.Name) + key := generateKey(namespace, routeName) + msg := fmt.Sprintf("secret %q updated for route %q (old resourceVersion=%v, new resourceVersion=%v)", secretNew.Name, key, secretOld.ResourceVersion, secretNew.ResourceVersion) + log.V(4).Info(msg) - // re-validate - if err := p.validate(route); err != nil { + // Ensure fetching the updated route + route, err := p.getUpdatedRoute(namespace, routeName) + if err != nil { return } - // read referenced secret (updated data) and update TLS certificate and key - if err := p.populateRouteTLSFromSecret(route); err != nil { - return + + // 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.recorder.RecordRouteUpdate(route, "ExternalCertificateSecretUpdated", msg) + } else { + p.recorder.RecordRouteRejection(route, "ExternalCertificateSecretUpdated", msg) } - // call the next plugin with watch.Modified - p.plugin.HandleRoute(watch.Modified, route) - // commit the changes - p.plugin.Commit() }, DeleteFunc: func(obj interface{}) { secret := obj.(*kapi.Secret) - key := generateKey(route) - msg := fmt.Sprintf("secret %s deleted for route %s", secret.Name, key) + key := generateKey(namespace, routeName) + msg := fmt.Sprintf("secret %q deleted for route %q", secret.Name, key) log.V(4).Info(msg) // keep the secret monitor active and mark the secret as deleted for this route. p.deletedSecrets.Store(key, true) + // Ensure fetching the updated route + route, err := p.getUpdatedRoute(namespace, routeName) + if err != nil { + return + } + + // Reject this route p.recorder.RecordRouteRejection(route, "ExternalCertificateSecretDeleted", msg) + // Stop serving this route p.plugin.HandleRoute(watch.Deleted, route) }, } @@ -276,6 +321,9 @@ func (p *RouteSecretManager) generateSecretHandler(route *routev1.Route) cache.R // validate checks the route's external certificate configuration. // If the validation fails, it records the route rejection and triggers // the deletion of the route by calling the HandleRoute method with a watch.Deleted event. +// +// 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 { @@ -321,7 +369,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)) + p.deletedSecrets.Delete(generateKey(route.Namespace, route.Name)) return nil } @@ -332,6 +380,29 @@ func hasExternalCertificate(route *routev1.Route) bool { } // generateKey creates a unique identifier for a route -func generateKey(route *routev1.Route) string { - return fmt.Sprintf("%s/%s", route.Namespace, route.Name) +func generateKey(namespace, routeName string) string { + return fmt.Sprintf("%s/%s", namespace, routeName) +} + +// getUpdatedRoute fetches the latest version of the route using the RouteLister. +func (p *RouteSecretManager) getUpdatedRoute(namespace, routeName string) (*routev1.Route, error) { + updatedRoute, err := p.routelister.Routes(namespace).Get(routeName) + if err != nil { + log.Error(err, "failed to get route", "namespace", namespace, "route", routeName) + return nil, err + } + return updatedRoute, nil +} + +// isRouteAdmittedTrue returns true if the given route has been admitted +// by any of its ingress controllers, otherwise false. +func isRouteAdmittedTrue(route *routev1.Route) bool { + for _, ingress := range route.Status.Ingress { + for _, condition := range ingress.Conditions { + if condition.Type == routev1.RouteAdmitted && condition.Status == kapi.ConditionTrue { + return true + } + } + } + return false } diff --git a/pkg/router/controller/route_secret_manager_test.go b/pkg/router/controller/route_secret_manager_test.go index 5cfd1aa8d..6bacfbb4b 100644 --- a/pkg/router/controller/route_secret_manager_test.go +++ b/pkg/router/controller/route_secret_manager_test.go @@ -105,20 +105,29 @@ var _ router.Plugin = &fakePluginDone{} type statusRecorder struct { rejections []string + updates []string unservableInFutureVersions map[string]string + doneCh chan struct{} } -func (r *statusRecorder) rejectionKey(route *routev1.Route) string { +func (r *statusRecorder) routeKey(route *routev1.Route) string { return route.Namespace + "-" + route.Name } func (r *statusRecorder) RecordRouteRejection(route *routev1.Route, reason, message string) { - r.rejections = append(r.rejections, fmt.Sprintf("%s:%s", r.rejectionKey(route), reason)) + defer close(r.doneCh) + 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.updates = append(r.updates, fmt.Sprintf("%s:%s", r.routeKey(route), reason)) +} + func (r *statusRecorder) RecordRouteUnservableInFutureVersionsClear(route *routev1.Route) { - delete(r.unservableInFutureVersions, r.rejectionKey(route)) + delete(r.unservableInFutureVersions, r.routeKey(route)) } func (r *statusRecorder) RecordRouteUnservableInFutureVersions(route *routev1.Route, reason, message string) { - r.unservableInFutureVersions[r.rejectionKey(route)] = reason + r.unservableInFutureVersions[r.routeKey(route)] = reason } var _ RouteStatusRecorder = &statusRecorder{} @@ -1114,8 +1123,10 @@ func TestRouteSecretManager(t *testing.T) { for _, s := range scenarios { t.Run(s.name, func(t *testing.T) { p := &fakePlugin{} - recorder := &statusRecorder{} - rsm := NewRouteSecretManager(p, recorder, &s.secretManager, &testSecretGetter{namespace: s.route.Namespace, secret: s.secretManager.Secret}, &testSARCreator{allow: s.allow}) + recorder := &statusRecorder{ + doneCh: make(chan struct{}), + } + rsm := NewRouteSecretManager(p, recorder, &s.secretManager, &testSecretGetter{namespace: s.route.Namespace, secret: s.secretManager.Secret}, &routeLister{}, &testSARCreator{allow: s.allow}) gotErr := rsm.HandleRoute(s.eventType, s.route) if (gotErr != nil) != s.expectedError { @@ -1130,113 +1141,22 @@ func TestRouteSecretManager(t *testing.T) { if !reflect.DeepEqual(s.expectedRejections, recorder.rejections) { t.Fatalf("expected rejections %v, but got %v", s.expectedRejections, recorder.rejections) } - if _, exists := rsm.deletedSecrets.Load(generateKey(s.route)); exists { - t.Fatalf("expected deletedSecrets to not have %q key", generateKey(s.route)) + 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)) } }) } } -func TestSecretUpdateAndDelete(t *testing.T) { +func TestSecretUpdate(t *testing.T) { scenarios := []struct { - name string - route *routev1.Route - secretManager fake.SecretManager - allow bool - deleteSecret bool - expectedRoute *routev1.Route - expectedEventType watch.EventType - expectedRejections []string - expectedDeletedSecrets any + name string + route *routev1.Route + isRouteAdmittedTrue bool }{ { - name: "secret updated but permission revoked", - route: &routev1.Route{ - ObjectMeta: metav1.ObjectMeta{ - Name: "route-test", - Namespace: "sandbox", - }, - Spec: routev1.RouteSpec{ - TLS: &routev1.TLSConfig{ - ExternalCertificate: &routev1.LocalObjectReference{ - Name: "tls-secret", - }, - }, - }, - }, - secretManager: fake.SecretManager{ - Secret: fakeSecret("sandbox", "tls-secret", corev1.SecretTypeTLS, map[string][]byte{ - "tls.crt": []byte("my-crt"), - "tls.key": []byte("my-key"), - }), - IsPresent: true, - SecretName: "tls-secret", - }, - allow: false, - expectedRoute: &routev1.Route{ - ObjectMeta: metav1.ObjectMeta{ - Name: "route-test", - Namespace: "sandbox", - }, - Spec: routev1.RouteSpec{ - TLS: &routev1.TLSConfig{ - ExternalCertificate: &routev1.LocalObjectReference{ - Name: "tls-secret", - }, - }, - }, - }, - expectedEventType: watch.Deleted, - expectedRejections: []string{ - "sandbox-route-test:ExternalCertificateValidationFailed", - }, - }, - { - name: "secret updated with permission but got error from secretManager", - route: &routev1.Route{ - ObjectMeta: metav1.ObjectMeta{ - Name: "route-test", - Namespace: "sandbox", - }, - Spec: routev1.RouteSpec{ - TLS: &routev1.TLSConfig{ - ExternalCertificate: &routev1.LocalObjectReference{ - Name: "tls-secret", - }, - }, - }, - }, - secretManager: fake.SecretManager{ - Secret: fakeSecret("sandbox", "tls-secret", corev1.SecretTypeTLS, map[string][]byte{ - "tls.crt": []byte("my-crt"), - "tls.key": []byte("my-key"), - }), - IsPresent: true, - SecretName: "tls-secret", - Err: fmt.Errorf("something"), - }, - allow: true, - expectedRoute: &routev1.Route{ - ObjectMeta: metav1.ObjectMeta{ - Name: "route-test", - Namespace: "sandbox", - }, - Spec: routev1.RouteSpec{ - TLS: &routev1.TLSConfig{ - ExternalCertificate: &routev1.LocalObjectReference{ - Name: "tls-secret", - }, - }, - }, - }, - expectedEventType: watch.Deleted, - expectedRejections: []string{ - "sandbox-route-test:ExternalCertificateGetFailed", - }, - }, - { - name: "secret updated with permission correctly", + name: "Secret updated when route status was Admitted=False", route: &routev1.Route{ ObjectMeta: metav1.ObjectMeta{ Name: "route-test", @@ -1249,35 +1169,22 @@ func TestSecretUpdateAndDelete(t *testing.T) { }, }, }, - }, - secretManager: fake.SecretManager{ - Secret: fakeSecret("sandbox", "tls-secret", corev1.SecretTypeTLS, map[string][]byte{ - "tls.crt": []byte("my-crt"), - "tls.key": []byte("my-key"), - }), - IsPresent: true, - SecretName: "tls-secret", - }, - allow: true, - expectedRoute: &routev1.Route{ - ObjectMeta: metav1.ObjectMeta{ - Name: "route-test", - Namespace: "sandbox", - }, - Spec: routev1.RouteSpec{ - TLS: &routev1.TLSConfig{ - ExternalCertificate: &routev1.LocalObjectReference{ - Name: "tls-secret", + Status: routev1.RouteStatus{ + Ingress: []routev1.RouteIngress{ + { + Conditions: []routev1.RouteIngressCondition{ + { + Type: routev1.RouteAdmitted, + Status: corev1.ConditionFalse, + }, + }, }, - Certificate: "my-crt", - Key: "my-key", }, }, }, - expectedEventType: watch.Modified, }, { - name: "secret deleted and route successfully stored into deletedSecrets", + name: "Secret updated when route status was Admitted=True", route: &routev1.Route{ ObjectMeta: metav1.ObjectMeta{ Name: "route-test", @@ -1290,41 +1197,34 @@ func TestSecretUpdateAndDelete(t *testing.T) { }, }, }, - }, - secretManager: fake.SecretManager{ - Secret: fakeSecret("sandbox", "tls-secret", corev1.SecretTypeTLS, map[string][]byte{ - "tls.crt": []byte("my-crt"), - "tls.key": []byte("my-key"), - }), - IsPresent: true, - SecretName: "tls-secret", - }, - deleteSecret: true, - expectedRoute: &routev1.Route{ - ObjectMeta: metav1.ObjectMeta{ - Name: "route-test", - Namespace: "sandbox", - }, - Spec: routev1.RouteSpec{ - TLS: &routev1.TLSConfig{ - ExternalCertificate: &routev1.LocalObjectReference{ - Name: "tls-secret", + Status: routev1.RouteStatus{ + Ingress: []routev1.RouteIngress{ + { + Conditions: []routev1.RouteIngressCondition{ + { + Type: routev1.RouteAdmitted, + Status: corev1.ConditionTrue, + }, + }, }, }, }, }, - expectedEventType: watch.Deleted, - expectedRejections: []string{ - "sandbox-route-test:ExternalCertificateSecretDeleted", - }, - expectedDeletedSecrets: true, + isRouteAdmittedTrue: true, }, } for _, s := range scenarios { t.Run(s.name, func(t *testing.T) { - oldSecret := fakeSecret("sandbox", "tls-secret", corev1.SecretTypeTLS, map[string][]byte{}) - kubeClient := testclient.NewSimpleClientset(oldSecret) + recorder := &statusRecorder{ + doneCh: make(chan struct{}), + } + lister := &routeLister{items: []*routev1.Route{s.route}} + rsm := NewRouteSecretManager(&fakePlugin{}, recorder, &fake.SecretManager{}, &testSecretGetter{}, lister, &testSARCreator{}) + + // 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()) @@ -1333,239 +1233,174 @@ func TestSecretUpdateAndDelete(t *testing.T) { t.Fatal("cache not synced yet") } - p := &fakePluginDone{ - doneCh: make(chan struct{}), + if _, err := informer.AddEventHandler(rsm.generateSecretHandler(s.route.Namespace, s.route.Name)); err != nil { + t.Fatalf("failed to add handler: %v", err) } - recorder := &statusRecorder{} - rsm := NewRouteSecretManager(p, recorder, &s.secretManager, &testSecretGetter{namespace: s.route.Namespace, secret: oldSecret}, &testSARCreator{allow: s.allow}) - if _, err := informer.AddEventHandler(rsm.generateSecretHandler(s.route)); err != nil { - t.Fatalf("failed to add handler: %v", err) + // update the secret + updatedSecret := secret.DeepCopy() + 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) } - if s.deleteSecret { - // delete the secret - if err := kubeClient.CoreV1().Secrets(s.route.Namespace).Delete(context.TODO(), s.secretManager.Secret.Name, metav1.DeleteOptions{}); err != nil { - t.Fatalf("failed to delete secret: %v", err) - } + // wait until route's status is updated + <-recorder.doneCh + + expectedStatus := []string{"sandbox-route-test:ExternalCertificateSecretUpdated"} + 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 { - // update the secret - if _, err := kubeClient.CoreV1().Secrets(s.route.Namespace).Update(context.TODO(), s.secretManager.Secret, metav1.UpdateOptions{}); err != nil { - t.Fatalf("failed to update secret: %v", err) + // RecordRouteRejection will be called if `Admitted=False` + if !reflect.DeepEqual(expectedStatus, recorder.rejections) { + t.Fatalf("expected status %v, but got %v", expectedStatus, recorder.rejections) } } - // wait until p.plugin.HandleRoute() completes (required to handle race condition) - <-p.doneCh - if !reflect.DeepEqual(s.expectedRoute, p.route) { - t.Fatalf("expected route for next plugin %v, but got %v", s.expectedRoute, p.route) - } - if s.expectedEventType != p.eventType { - t.Fatalf("expected %s event for next plugin, but got %s", s.expectedEventType, p.eventType) - } - if !reflect.DeepEqual(s.expectedRejections, recorder.rejections) { - t.Fatalf("expected rejections %v, but got %v", s.expectedRejections, recorder.rejections) + 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 val, _ := rsm.deletedSecrets.Load(generateKey(s.route)); !reflect.DeepEqual(val, s.expectedDeletedSecrets) { - t.Fatalf("expected deletedSecrets %v, but got %v", s.expectedDeletedSecrets, val) - } }) } + } -func TestSecretRecreation(t *testing.T) { - scenarios := []struct { - name string - route *routev1.Route - secretManager fake.SecretManager - allow bool - expectedRoute *routev1.Route - expectedEventType watch.EventType - expectedRejections []string - }{ - { - name: "secret deleted and recreated with permission revoked", - route: &routev1.Route{ - ObjectMeta: metav1.ObjectMeta{ - Name: "route-test", - Namespace: "sandbox", - }, - Spec: routev1.RouteSpec{ - TLS: &routev1.TLSConfig{ - ExternalCertificate: &routev1.LocalObjectReference{ - Name: "tls-secret", - }, - }, - }, - }, - secretManager: fake.SecretManager{ - Secret: fakeSecret("sandbox", "tls-secret", corev1.SecretTypeTLS, map[string][]byte{ - "tls.crt": []byte("my-crt"), - "tls.key": []byte("my-key"), - }), - IsPresent: true, - SecretName: "tls-secret", - }, - allow: false, - expectedRoute: &routev1.Route{ - ObjectMeta: metav1.ObjectMeta{ - Name: "route-test", - Namespace: "sandbox", - }, - Spec: routev1.RouteSpec{ - TLS: &routev1.TLSConfig{ - ExternalCertificate: &routev1.LocalObjectReference{ - Name: "tls-secret", - }, - }, - }, - }, - expectedEventType: watch.Deleted, - expectedRejections: []string{ - "sandbox-route-test:ExternalCertificateSecretDeleted", - "sandbox-route-test:ExternalCertificateValidationFailed", - }, +func TestSecretDelete(t *testing.T) { + route := &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "route-test", + Namespace: "sandbox", }, - { - name: "secret deleted and recreated with permission but got error from secretManager", - route: &routev1.Route{ - ObjectMeta: metav1.ObjectMeta{ - Name: "route-test", - Namespace: "sandbox", + Spec: routev1.RouteSpec{ + TLS: &routev1.TLSConfig{ + ExternalCertificate: &routev1.LocalObjectReference{ + Name: "tls-secret", }, - Spec: routev1.RouteSpec{ - TLS: &routev1.TLSConfig{ - ExternalCertificate: &routev1.LocalObjectReference{ - Name: "tls-secret", - }, - }, - }, - }, - secretManager: fake.SecretManager{ - Secret: fakeSecret("sandbox", "tls-secret", corev1.SecretTypeTLS, map[string][]byte{ - "tls.crt": []byte("my-crt"), - "tls.key": []byte("my-key"), - }), - IsPresent: true, - SecretName: "tls-secret", - Err: fmt.Errorf("something"), - }, - allow: true, - expectedRoute: &routev1.Route{ - ObjectMeta: metav1.ObjectMeta{ - Name: "route-test", - Namespace: "sandbox", - }, - Spec: routev1.RouteSpec{ - TLS: &routev1.TLSConfig{ - ExternalCertificate: &routev1.LocalObjectReference{ - Name: "tls-secret", - }, - }, - }, - }, - expectedEventType: watch.Deleted, - expectedRejections: []string{ - "sandbox-route-test:ExternalCertificateSecretDeleted", - "sandbox-route-test:ExternalCertificateGetFailed", }, }, - { - name: "secret deleted and recreated with permission correctly", - route: &routev1.Route{ - ObjectMeta: metav1.ObjectMeta{ - Name: "route-test", - Namespace: "sandbox", - }, - Spec: routev1.RouteSpec{ - TLS: &routev1.TLSConfig{ - ExternalCertificate: &routev1.LocalObjectReference{ - Name: "tls-secret", - }, - }, - }, - }, - secretManager: fake.SecretManager{ - Secret: fakeSecret("sandbox", "tls-secret", corev1.SecretTypeTLS, map[string][]byte{ - "tls.crt": []byte("my-crt"), - "tls.key": []byte("my-key"), - }), - IsPresent: true, - SecretName: "tls-secret", - }, - allow: true, - expectedRoute: &routev1.Route{ - ObjectMeta: metav1.ObjectMeta{ - Name: "route-test", - Namespace: "sandbox", - }, - Spec: routev1.RouteSpec{ - TLS: &routev1.TLSConfig{ - ExternalCertificate: &routev1.LocalObjectReference{ - Name: "tls-secret", - }, - Certificate: "my-crt", - Key: "my-key", - }, + } + recorder := &statusRecorder{ + doneCh: make(chan struct{}), + } + p := &fakePluginDone{ + doneCh: make(chan struct{}), + } + lister := &routeLister{items: []*routev1.Route{route}} + rsm := NewRouteSecretManager(p, recorder, &fake.SecretManager{}, &testSecretGetter{}, lister, &testSARCreator{}) + + // 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()) + + // 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(route.Namespace, route.Name)); err != nil { + t.Fatalf("failed to add handler: %v", err) + } + + // 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) + } + + <-recorder.doneCh // wait until the route's status is updated + <-p.doneCh // wait until p.plugin.HandleRoute() is completed + + expectedRoute := route + expectedEventType := watch.Deleted + expectedRejections := []string{"sandbox-route-test:ExternalCertificateSecretDeleted"} + expectedDeletedSecrets := true + + if !reflect.DeepEqual(expectedRoute, p.route) { + t.Fatalf("expected route for next plugin %v, but got %v", expectedRoute, p.route) + } + if expectedEventType != p.eventType { + t.Fatalf("expected %s event for next plugin, but got %s", expectedEventType, p.eventType) + } + if !reflect.DeepEqual(expectedRejections, recorder.rejections) { + t.Fatalf("expected rejections %v, but got %v", expectedRejections, recorder.rejections) + } + + if val, _ := rsm.deletedSecrets.Load(generateKey(route.Namespace, route.Name)); !reflect.DeepEqual(val, expectedDeletedSecrets) { + t.Fatalf("expected deletedSecrets %v, but got %v", expectedDeletedSecrets, val) + } +} + +func TestSecretRecreation(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", }, }, - expectedEventType: watch.Modified, - expectedRejections: []string{ - "sandbox-route-test:ExternalCertificateSecretDeleted", - }, }, } + recorder := &statusRecorder{ + doneCh: make(chan struct{}), + } + p := &fakePluginDone{ + doneCh: make(chan struct{}), + } + lister := &routeLister{items: []*routev1.Route{route}} + rsm := NewRouteSecretManager(p, recorder, &fake.SecretManager{}, &testSecretGetter{}, lister, &testSARCreator{}) - for _, s := range scenarios { - t.Run(s.name, func(t *testing.T) { - oldSecret := fakeSecret("sandbox", "tls-secret", corev1.SecretTypeTLS, map[string][]byte{}) - kubeClient := testclient.NewSimpleClientset(oldSecret) - informer := fakeSecretInformer(kubeClient, "sandbox", "tls-secret") - go informer.Run(context.TODO().Done()) + // 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()) - // wait for informer to start - if !cache.WaitForCacheSync(context.TODO().Done(), informer.HasSynced) { - t.Fatal("cache not synced yet") - } + // wait for informer to start + if !cache.WaitForCacheSync(context.TODO().Done(), informer.HasSynced) { + t.Fatal("cache not synced yet") + } - nextPlugin := &fakePluginDone{ - doneCh: make(chan struct{}), - } - recorder := &statusRecorder{} - rsm := NewRouteSecretManager(nextPlugin, recorder, &s.secretManager, &testSecretGetter{namespace: s.route.Namespace, secret: oldSecret}, &testSARCreator{allow: s.allow}) + if _, err := informer.AddEventHandler(rsm.generateSecretHandler(route.Namespace, route.Name)); err != nil { + t.Fatalf("failed to add handler: %v", err) + } - if _, err := informer.AddEventHandler(rsm.generateSecretHandler(s.route)); err != nil { - t.Fatalf("failed to add handler: %v", err) - } + // 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) + } - // secret deletion - if err := kubeClient.CoreV1().Secrets(s.route.Namespace).Delete(context.TODO(), s.secretManager.Secret.Name, metav1.DeleteOptions{}); err != nil { - t.Fatalf("failed to delete secret: %v", err) - } - <-nextPlugin.doneCh // wait for HandleRoute (deletion) to complete + <-recorder.doneCh // wait until the route's status is updated (deletion) + <-p.doneCh // wait until p.plugin.HandleRoute() is completed (deletion) - // recreate the secret - nextPlugin.doneCh = make(chan struct{}) // need a new doneCh for recreation - if _, err := kubeClient.CoreV1().Secrets(s.route.Namespace).Create(context.TODO(), s.secretManager.Secret, metav1.CreateOptions{}); err != nil { - t.Fatalf("failed to create secret: %v", err) - } + // 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) + } - <-nextPlugin.doneCh // wait for HandleRoute (recreation) to complete + <-recorder.doneCh // wait until the route's status is updated (re-creation) - if !reflect.DeepEqual(s.expectedRoute, nextPlugin.route) { - t.Fatalf("expected route for next plugin %v, but got %v", s.expectedRoute, nextPlugin.route) - } - if s.expectedEventType != nextPlugin.eventType { - t.Fatalf("expected %s event for next plugin, but got %s", s.expectedEventType, nextPlugin.eventType) - } - if !reflect.DeepEqual(s.expectedRejections, recorder.rejections) { - t.Fatalf("expected rejections %v, but got %v", s.expectedRejections, recorder.rejections) - } - if _, exists := rsm.deletedSecrets.Load(generateKey(s.route)); exists { - t.Fatalf("deletedSecrets must be cleaned") - } - }) + expectedRejections := []string{ + "sandbox-route-test:ExternalCertificateSecretDeleted", + "sandbox-route-test:ExternalCertificateSecretRecreated", + } + if !reflect.DeepEqual(expectedRejections, recorder.rejections) { + t.Fatalf("expected rejections %v, but got %v", expectedRejections, recorder.rejections) + } + 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)) } } diff --git a/pkg/router/controller/status.go b/pkg/router/controller/status.go index 9703c63ea..5f20afd11 100644 --- a/pkg/router/controller/status.go +++ b/pkg/router/controller/status.go @@ -36,6 +36,7 @@ const ( // RouteStatusRecorder is an object capable of recording why a route status condition changed. type RouteStatusRecorder interface { RecordRouteRejection(route *routev1.Route, reason, message string) + RecordRouteUpdate(route *routev1.Route, reason, message string) RecordRouteUnservableInFutureVersions(route *routev1.Route, reason, message string) RecordRouteUnservableInFutureVersionsClear(route *routev1.Route) } @@ -49,6 +50,10 @@ func (logRecorder) RecordRouteRejection(route *routev1.Route, reason, message st log.V(3).Info("rejected route", "name", route.Name, "namespace", route.Namespace, "reason", reason, "message", message) } +func (logRecorder) RecordRouteUpdate(route *routev1.Route, reason, message string) { + log.V(3).Info("updated route", "name", route.Name, "namespace", route.Namespace, "reason", reason, "message", message) +} + func (logRecorder) RecordRouteUnservableInFutureVersions(route *routev1.Route, reason, message string) { log.V(3).Info("route unservable in future versions", "name", route.Name, "namespace", route.Namespace, "reason", reason, "message", message) } @@ -126,6 +131,18 @@ func (a *StatusAdmitter) Commit() error { return a.plugin.Commit() } +// RecordRouteUpdate attempts to update the route status with a reason for a route being updated. +// This function is intended to be used to signal route updates, keeping `Admitted=True` to +// provide information about the change. +func (a *StatusAdmitter) RecordRouteUpdate(route *routev1.Route, reason, message string) { + performIngressConditionUpdate("admit", a.lease, a.tracker, a.client, a.lister, route, a.routerName, a.routerCanonicalHostname, routev1.RouteIngressCondition{ + Type: routev1.RouteAdmitted, + Status: corev1.ConditionTrue, + Reason: reason, + Message: message, + }) +} + // RecordRouteRejection attempts to update the route status with a reason for a route being rejected. func (a *StatusAdmitter) RecordRouteRejection(route *routev1.Route, reason, message string) { performIngressConditionUpdate("reject", a.lease, a.tracker, a.client, a.lister, route, a.routerName, a.routerCanonicalHostname, routev1.RouteIngressCondition{ diff --git a/pkg/router/template/plugin_test.go b/pkg/router/template/plugin_test.go index 1c19a6817..2e104024d 100644 --- a/pkg/router/template/plugin_test.go +++ b/pkg/router/template/plugin_test.go @@ -531,6 +531,9 @@ func (r *fakeStatusRecorder) RecordRouteRejection(route *routev1.Route, reason, func (r *fakeStatusRecorder) RecordRouteUnservableInFutureVersions(route *routev1.Route, reason, message string) { r.unservableInFutureVersions = append(r.unservableInFutureVersions, status{route: route, reason: reason, message: message}) } +func (r *fakeStatusRecorder) RecordRouteUpdate(route *routev1.Route, reason, message string) { + panic("not implemented") +} func (r *fakeStatusRecorder) RecordRouteUnservableInFutureVersionsClear(route *routev1.Route) { var unservableInFutureVersions []status for _, entry := range r.unservableInFutureVersions {