fix: make worker.Stop() idempotent so a failed reconcile cannot crash Authorino

controllers/auth_config_controller.go

@@ -62,6 +62,7 @@ import (
 
 const (
 	failedToCleanConfig = "failed to clean up all asynchronous workers"
+	failedToStartConfig = "failed to start all asynchronous workers"
 
 	AuthConfigsReadyzSubpath = "authconfigs"
 )
@@ -187,12 +188,10 @@ func (r *AuthConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 		)
 		span.AddEvent("authconfig.found")
 
-		// clean all async workers of the config, i.e. shuts down channels and goroutines
-		if err := r.cleanConfigs(ctx, resourceId); err != nil {
-			logger.Error(err, failedToCleanConfig)
-			span.RecordError(err)
-		}
-
+		// translate first and only then swap: evaluators are built without starting any of their
+		// background workers, so a translation that fails leaves the config currently in the index
+		// untouched and still refreshing, instead of tearing it down before we know the new one is
+		// even valid
 		translatedAuthConfig, err := r.translateAuthConfig(log.IntoContext(ctx, logger), &authConfig)
 		if err != nil {
 			r.StatusReport.Set(resourceId, api.StatusReasonInvalidResource, err.Error(), []string{})
@@ -201,6 +200,13 @@ func (r *AuthConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 			return ctrl.Result{}, err
 		}
 
+		// the new config is good, so the one it replaces can be cleaned up, i.e. shuts down
+		// channels and goroutines
+		if err := r.cleanConfigs(ctx, resourceId); err != nil {
+			logger.Error(err, failedToCleanConfig)
+			span.RecordError(err)
+		}
+
 		// delete unused hosts from the index
 		unusedHosts := utils.SubtractSlice(r.Index.FindKeys(resourceId), authConfig.Spec.Hosts)
 		if len(unusedHosts) > 0 {
@@ -227,6 +233,17 @@ func (r *AuthConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 			return ctrl.Result{}, err
 		}
 
+		// only now, and only for a config that actually made it into the index. an authconfig whose
+		// hosts are all taken by another one translates just fine and is never indexed, so anything
+		// started before this point would run for a config that no request can ever reach and that
+		// cleanConfigs() would never find again
+		if len(linkedHosts) > 0 {
+			if err := r.startConfigs(ctx, translatedAuthConfig); err != nil {
+				logger.Error(err, failedToStartConfig)
+				span.RecordError(err)
+			}
+		}
+
 		span.SetAttributes(
 			attribute.Int("authconfig.linked_hosts_count", len(linkedHosts)),
 			attribute.Int("authconfig.loose_hosts_count", len(looseHosts)),
@@ -245,6 +262,22 @@ func (r *AuthConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 	return ctrl.Result{}, nil
 }
 
+// startConfigs kicks off the asynchronous workers of a config that has been successfully
+// translated and indexed, e.g. the openid connect configuration and external policy refreshers
+func (r *AuthConfigReconciler) startConfigs(ctx context.Context, authConfig *evaluators.AuthConfig) error {
+	ctx, span := trace.NewSpan(ctx, "authconfig", "authconfig.start_configs")
+	defer span.End()
+
+	if err := authConfig.Start(ctx); err != nil {
+		span.RecordError(err)
+		span.SetStatus(codes.Error, "failed to start authconfig")
+		return err
+	}
+
+	span.AddEvent("authconfig.started")
+	return nil
+}
+
 func (r *AuthConfigReconciler) cleanConfigs(ctx context.Context, resourceId string) error {
 	ctx, span := trace.NewSpan(ctx, "authconfig", "authconfig.clean_configs")
 	defer span.End()

controllers/auth_config_controller_test.go

@@ -4,7 +4,9 @@ import (
 	"context"
 	"fmt"
 	"os"
+	goruntime "runtime"
 	"testing"
+	"time"
 
 	api "github.com/kuadrant/authorino/api/v1beta3"
 	"github.com/kuadrant/authorino/pkg/evaluators"
@@ -446,3 +448,192 @@ func BenchmarkReconcileAuthConfig(b *testing.B) {
 	b.StopTimer()
 	assert.NilError(b, err)
 }
+
+func newTestAuthConfigWithRefresher() api.AuthConfig {
+	return api.AuthConfig{
+		TypeMeta:   metav1.TypeMeta{Kind: "AuthConfig", APIVersion: "authorino.kuadrant.io/v1beta3"},
+		ObjectMeta: metav1.ObjectMeta{Name: "auth-config-1", Namespace: "authorino"},
+		Spec: api.AuthConfigSpec{
+			Hosts: []string{"echo-api"},
+			Authentication: map[string]api.AuthenticationSpec{
+				"keycloak": {
+					AuthenticationMethodSpec: api.AuthenticationMethodSpec{
+						Jwt: &api.JwtAuthenticationSpec{
+							IssuerUrl: "http://127.0.0.1:9001/auth/realms/demo",
+							TTL:       60, // starts the background OIDC refresher worker
+						},
+					},
+				},
+			},
+		},
+	}
+}
+
+// breaks translateAuthConfig() the same way a rotated or deleted credentialsRef does in a cluster.
+// the metadata phase is translated after the identity phase, so the identity refresher is always
+// built before the failure - authentication is a map and its iteration order is not deterministic
+func breakTranslation(t *testing.T, k8sClient client.WithWatch, name types.NamespacedName) {
+	t.Helper()
+	authConfig := &api.AuthConfig{}
+	assert.NilError(t, k8sClient.Get(context.Background(), name, authConfig))
+	authConfig.Spec.Metadata = map[string]api.MetadataSpec{
+		"uma": {
+			MetadataMethodSpec: api.MetadataMethodSpec{
+				Uma: &api.UmaMetadataSpec{
+					Endpoint:    "http://127.0.0.1:9001/auth/realms/demo",
+					Credentials: &v1.LocalObjectReference{Name: "no-such-secret"},
+				},
+			},
+		},
+	}
+	assert.NilError(t, k8sClient.Update(context.Background(), authConfig))
+}
+
+// A reconcile that fails in translateAuthConfig() returns before addToIndex(), so the config it
+// just cleaned up is still the one in the index. The requeue then cleans that very same instance
+// again, which used to close an already-closed channel and take the whole process down.
+func TestReconcileCleansTheSameIndexedConfigTwiceWithoutPanicking(t *testing.T) {
+	authConfigIndex := index.NewIndex()
+	authConfig := newTestAuthConfigWithRefresher()
+	k8sClient := newTestK8sClient(&authConfig)
+	reconciler := newTestAuthConfigReconciler(k8sClient, authConfigIndex)
+	name := types.NamespacedName{Name: authConfig.Name, Namespace: authConfig.Namespace}
+	req := reconcile.Request{NamespacedName: name}
+
+	_, err := reconciler.Reconcile(context.Background(), req)
+	assert.NilError(t, err)
+	assert.Check(t, authConfigIndex.Get("echo-api") != nil)
+
+	breakTranslation(t, k8sClient, name)
+
+	// reconcile #1: cleans up the indexed config, fails to translate, requeues
+	_, err = reconciler.Reconcile(context.Background(), req)
+	assert.ErrorContains(t, err, "no-such-secret")
+	assert.Check(t, authConfigIndex.Get("echo-api") != nil, "the last known good config should stay in the index")
+
+	// reconcile #2: the requeue, cleaning up that same instance all over again
+	_, err = reconciler.Reconcile(context.Background(), req)
+	assert.ErrorContains(t, err, "no-such-secret")
+	assert.Check(t, authConfigIndex.Get("echo-api") != nil)
+}
+
+// dropIdentity removes the identity evaluator from the resource. Kept apart from breakTranslation
+// on purpose: the tests that check nothing gets orphaned need an identity in the failing config,
+// or there would be no refresher to orphan in the first place and they would pass for free.
+func dropIdentity(t *testing.T, k8sClient client.WithWatch, name types.NamespacedName) {
+	t.Helper()
+	authConfig := &api.AuthConfig{}
+	assert.NilError(t, k8sClient.Get(context.Background(), name, authConfig))
+	authConfig.Spec.Authentication = nil
+	assert.NilError(t, k8sClient.Update(context.Background(), authConfig))
+}
+
+// A translation that fails must not start anything: the evaluators it built never reach the index,
+// so nothing would ever clean them up and a persistently failing reconcile is requeued forever.
+func TestReconcileStartsNoWorkersWhenTranslationFails(t *testing.T) {
+	authConfig := newTestAuthConfigWithRefresher()
+	k8sClient := newTestK8sClient(&authConfig)
+	reconciler := newTestAuthConfigReconciler(k8sClient, index.NewIndex())
+	name := types.NamespacedName{Name: authConfig.Name, Namespace: authConfig.Namespace}
+	req := reconcile.Request{NamespacedName: name}
+
+	breakTranslation(t, k8sClient, name)
+
+	_, err := reconciler.Reconcile(context.Background(), req)
+	assert.ErrorContains(t, err, "no-such-secret")
+	time.Sleep(200 * time.Millisecond)
+	goruntime.GC()
+	before := goruntime.NumGoroutine()
+
+	const reconciles = 20
+	for i := 0; i < reconciles; i++ {
+		_, err := reconciler.Reconcile(context.Background(), req)
+		assert.ErrorContains(t, err, "no-such-secret")
+	}
+
+	time.Sleep(500 * time.Millisecond)
+	goruntime.GC()
+	leaked := goruntime.NumGoroutine() - before
+	assert.Check(t, leaked < reconciles/2, "leaked %d goroutines over %d failed reconciles", leaked, reconciles)
+}
+
+// An authconfig whose hosts are all taken by another one translates cleanly but is never indexed,
+// and addToIndex() reports that with an empty linkedHosts and no error at all. Nothing may be
+// started for it either, for exactly the same reason.
+func TestReconcileStartsNoWorkersWhenNoHostIsLinked(t *testing.T) {
+	authConfigIndex := index.NewIndex()
+
+	winner := newTestAuthConfigWithRefresher()
+	winner.Name = "auth-config-winner"
+	loser := newTestAuthConfigWithRefresher()
+	loser.Name = "auth-config-loser"
+
+	k8sClient := newTestK8sClient(&winner, &loser)
+	reconciler := newTestAuthConfigReconciler(k8sClient, authConfigIndex)
+	winnerReq := reconcile.Request{NamespacedName: types.NamespacedName{Name: winner.Name, Namespace: winner.Namespace}}
+	loserReq := reconcile.Request{NamespacedName: types.NamespacedName{Name: loser.Name, Namespace: loser.Namespace}}
+
+	_, err := reconciler.Reconcile(context.Background(), winnerReq)
+	assert.NilError(t, err)
+
+	_, err = reconciler.Reconcile(context.Background(), loserReq)
+	assert.NilError(t, err) // a host collision is reported on the status, it is not a reconcile error
+	time.Sleep(200 * time.Millisecond)
+	goruntime.GC()
+	before := goruntime.NumGoroutine()
+
+	const reconciles = 20
+	for i := 0; i < reconciles; i++ {
+		_, err := reconciler.Reconcile(context.Background(), loserReq)
+		assert.NilError(t, err)
+	}
+
+	time.Sleep(500 * time.Millisecond)
+	goruntime.GC()
+	leaked := goruntime.NumGoroutine() - before
+	assert.Check(t, leaked < reconciles/2, "leaked %d goroutines over %d reconciles of an unlinked authconfig", leaked, reconciles)
+}
+
+// The config in the index is only torn down once its replacement is known to be good, so a failed
+// translation leaves the last known good config both indexed AND still refreshing.
+func TestReconcileKeepsTheIndexedConfigRunningWhenTranslationFails(t *testing.T) {
+	authConfigIndex := index.NewIndex()
+	authConfig := newTestAuthConfigWithRefresher()
+	k8sClient := newTestK8sClient(&authConfig)
+	reconciler := newTestAuthConfigReconciler(k8sClient, authConfigIndex)
+	name := types.NamespacedName{Name: authConfig.Name, Namespace: authConfig.Namespace}
+	req := reconcile.Request{NamespacedName: name}
+
+	_, err := reconciler.Reconcile(context.Background(), req)
+	assert.NilError(t, err)
+
+	indexed := authConfigIndex.Get("echo-api")
+	assert.Check(t, indexed != nil)
+	assert.Check(t, refresherRunning(indexed), "the indexed config should be refreshing once it is reconciled")
+
+	breakTranslation(t, k8sClient, name)
+	// and take the identity away, so the version of the resource that fails to translate has no
+	// jwt evaluator of its own. a refresher still running below can then only have come from the
+	// config that was indexed before it, rather than from a config this reconcile put there
+	dropIdentity(t, k8sClient, name)
+
+	_, err = reconciler.Reconcile(context.Background(), req)
+	assert.ErrorContains(t, err, "no-such-secret")
+
+	stillIndexed := authConfigIndex.Get("echo-api")
+	assert.Check(t, stillIndexed != nil, "the last known good config should stay in the index")
+	assert.Check(t, refresherRunning(stillIndexed), "the refresher can only be the one from the previously indexed config, and it should still be running")
+}
+
+func refresherRunning(authConfig *evaluators.AuthConfig) bool {
+	for _, evaluator := range authConfig.IdentityConfigs {
+		idConfig, ok := evaluator.(*evaluators.IdentityConfig)
+		if !ok || idConfig.JWTAuthentication == nil {
+			continue
+		}
+		if idConfig.JWTAuthentication.RefresherRunning() {
+			return true
+		}
+	}
+	return false
+}

pkg/auth/auth.go

@@ -28,6 +28,15 @@ type AuthConfigEvaluator interface {
 	Call(AuthPipeline, context.Context) (interface{}, error)
 }
 
+type AuthConfigStarter interface {
+	// Start is used to give the different auth configs a chance to kick off anything that should
+	// only be running once the config is known to be valid and reachable, e.g. background workers
+	// that refresh remote state. Evaluators must not start those from their constructors: a config
+	// can fail to translate or lose every one of its hosts to another AuthConfig, and in neither
+	// case does it ever reach the index for anything to clean it up again.
+	Start(context.Context) error
+}
+
 type AuthConfigCleaner interface {
 	// Clean is used to give the different auth configs chance to clean up anything internal to that config
 	Clean(context.Context) error

pkg/evaluators/authorization.go

@@ -120,6 +120,24 @@ func (config *AuthorizationConfig) MetricsEnabled() bool {
 	return config.Metrics
 }
 
+// impl:AuthConfigStarter
+
+func (config *AuthorizationConfig) Start(ctx context.Context) error {
+	if starter := config.getStarter(); starter != nil {
+		return starter.Start(log.IntoContext(ctx, log.FromContext(ctx).WithName("authorization")))
+	}
+	return nil
+}
+
+func (config *AuthorizationConfig) getStarter() auth.AuthConfigStarter {
+	switch {
+	case config.OPA != nil:
+		return config.OPA
+	default:
+		return nil
+	}
+}
+
 // impl:AuthConfigCleaner
 
 func (config *AuthorizationConfig) Clean(ctx context.Context) error {