improve error handling

On-behalf-of: @SAP [email protected]

Author: xrstf

pkg/authentication/authentication_controller.go

@@ -73,7 +73,7 @@ func NewController(
 	shardInformer corev1alpha1informers.ShardInformer,
 	clientGetter ClusterClientGetter,
 	baseAudiences authenticator.Audiences,
-) *Controller {
+) (*Controller, error) {
 	queue := workqueue.NewTypedRateLimitingQueueWithConfig(
 		workqueue.DefaultTypedControllerRateLimiter[string](),
 		workqueue.TypedRateLimitingQueueConfig[string]{
@@ -93,7 +93,7 @@ func NewController(
 		shardWatchers: map[string]*shardWatcher{},
 	}
 
-	_, _ = shardInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
+	_, err := shardInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
 		AddFunc: func(obj interface{}) {
 			shard := obj.(*corev1alpha1.Shard)
 			c.enqueueShard(ctx, shard)
@@ -116,8 +116,11 @@ func NewController(
 			c.stopShard(shard.Name)
 		},
 	})
+	if err != nil {
+		return nil, fmt.Errorf("failed to start Shard informer: %w", err)
+	}
 
-	return c
+	return c, nil
 }
 
 // Start the controller. It does not really do anything, but to keep the shape of a normal
@@ -215,7 +218,11 @@ func (c *Controller) process(ctx context.Context, key string) error {
 			return err
 		}
 
-		watcher := NewShardWatcher(ctx, shard.Name, client, &c.authIndex)
+		watcher, err := NewShardWatcher(ctx, shard.Name, client, &c.authIndex)
+		if err != nil {
+			return fmt.Errorf("failed to start shard watcher: %w", err)
+		}
+
 		c.shardWatchers[shard.Name] = watcher
 	}
 
@@ -250,9 +257,9 @@ func NewShardWatcher(
 	shardName string,
 	shardClient kcpclientset.ClusterInterface,
 	state *state,
-) *shardWatcher {
+) (*shardWatcher, error) {
 	wacInformer := tenancyv1alpha1informers.NewWorkspaceAuthenticationConfigurationClusterInformer(shardClient, resyncPeriod, nil)
-	_, _ = wacInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{
+	_, err := wacInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{
 		AddFunc: func(obj interface{}) {
 			wac := obj.(*tenancyv1alpha1.WorkspaceAuthenticationConfiguration)
 			state.UpsertWorkspaceAuthenticationConfiguration(shardName, wac)
@@ -269,9 +276,12 @@ func NewShardWatcher(
 			state.DeleteWorkspaceAuthenticationConfiguration(shardName, wac)
 		},
 	})
+	if err != nil {
+		return nil, fmt.Errorf("failed to start WorkspaceAuthenticationConfiguration informer: %w", err)
+	}
 
 	wtInformer := tenancyv1alpha1informers.NewWorkspaceTypeClusterInformer(shardClient, resyncPeriod, nil)
-	_, _ = wtInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{
+	_, err = wtInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{
 		AddFunc: func(obj interface{}) {
 			wt := obj.(*tenancyv1alpha1.WorkspaceType)
 			state.UpsertWorkspaceType(shardName, wt)
@@ -288,6 +298,9 @@ func NewShardWatcher(
 			state.DeleteWorkspaceType(shardName, wt)
 		},
 	})
+	if err != nil {
+		return nil, fmt.Errorf("failed to start WorkspaceType informer: %w", err)
+	}
 
 	ctx, cancel := context.WithCancel(ctx)
 
@@ -303,7 +316,7 @@ func NewShardWatcher(
 
 	// no need to wait. We only care about events and they arrive when they arrive.
 
-	return watcher
+	return watcher, nil
 }
 
 func (w *shardWatcher) Stop() {

pkg/proxy/server.go

@@ -115,7 +115,12 @@ func NewServer(ctx context.Context, c CompletedConfig) (*Server, error) {
 
 	if hasWorkspaceAuth {
 		// This controller is similar to the index controller, but keeps track of the per-workspace authenticators.
-		s.AuthController = authentication.NewController(ctx, s.KcpSharedInformerFactory.Core().V1alpha1().Shards(), getClientFunc, nil)
+		ctrl, err := authentication.NewController(ctx, s.KcpSharedInformerFactory.Core().V1alpha1().Shards(), getClientFunc, nil)
+		if err != nil {
+			return nil, fmt.Errorf("failed to start authentication controller: %w", err)
+		}
+
+		s.AuthController = ctrl
 
 		// When workspace auth is enabled, it depends on the target cluster whether
 		// a custom authenticator exists or not. This needs to be determined before

pkg/server/config.go

@@ -409,6 +409,22 @@ func NewConfig(ctx context.Context, opts kcpserveroptions.CompletedOptions) (*Co
 	// Make sure to set our RequestInfoResolver that is capable of populating a RequestInfo even for /services/... URLs.
 	c.GenericConfig.RequestInfoResolver = requestinfo.NewKCPRequestInfoResolver()
 
+	// Prepare an authentication index to be used later by a middleware. We start it early
+	// because it can potentially fail and the BuildHandlerChainFunc() has no way to return
+	// an error.
+	var authIndex authentication.AuthenticatorIndex
+	if kcpfeatures.DefaultFeatureGate.Enabled(kcpfeatures.WorkspaceAuthentication) {
+		// Start an index and a shard watcher to fill this index;
+		// the shard watcher's lifetime is tied to the given context.
+		authIndexState := authentication.NewIndex(ctx, c.GenericConfig.Authentication.APIAudiences)
+		_, err := authentication.NewShardWatcher(ctx, c.Options.Extra.ShardName, c.KcpClusterClient, authIndexState)
+		if err != nil {
+			return nil, fmt.Errorf("failed to start shard watcher: %w", err)
+		}
+
+		authIndex = authIndexState
+	}
+
 	// preHandlerChainMux is called before the actual handler chain. Note that BuildHandlerChainFunc below
 	// is called multiple times, but only one of the handler chain will actually be used. Hence, we wrap it
 	// to give handlers below one mux.Handle func to call.
@@ -463,17 +479,11 @@ func NewConfig(ctx context.Context, opts kcpserveroptions.CompletedOptions) (*Co
 		// Wrap authenticator with a per-workspace authenticator if desired. This authenticator
 		// requires the WorkspaceAuth middleware to have looked up and injected the relevant
 		// authenticator into the request context already.
-		var authIndex authentication.AuthenticatorIndex
 		if kcpfeatures.DefaultFeatureGate.Enabled(kcpfeatures.WorkspaceAuthentication) {
-			authIndexState := authentication.NewIndex(ctx, genericConfig.Authentication.APIAudiences)
-			authentication.NewShardWatcher(ctx, c.Options.Extra.ShardName, c.KcpClusterClient, authIndexState)
-
 			genericConfig.Authentication.Authenticator = authenticatorunion.New(
 				genericConfig.Authentication.Authenticator,
 				authentication.NewWorkspaceAuthenticator(),
 			)
-
-			authIndex = authIndexState
 		}
 
 		// There is ordering here in play: