Refactor application wireup

Relates to #2845

With some of the changes I've made, I have started to run into issues
with the way we currently wire up the server, event handler and
reconcilers:

1. The way in which structs are shared across events, server and
   reconcilers is a bit hacky, and will get more hacky when I start
   sharing more structs.
2. The functional options pattern is starting to cause complexity now that we
   need to wire up dependencies which depend on some of the structs
   passed by the options.

This moves most of the application wireup outside of `server.go` and
into `service.go`. The optional parameters for the server are removed
entirely, and are now passed in as explicit arguments. Various tests
have been cleaned up to instantiate the Server directly with appropriate
stubs for different dependencies.

The use of optional parameters for the reconciler and event handler will
be cleaned up in a future PR.

cmd/dev/app/testserver/testserver.go

@@ -28,16 +28,17 @@ import (
 	"github.com/spf13/cobra"
 	"github.com/spf13/viper"
 
+	"github.com/stacklok/minder/internal/auth"
 	noopauth "github.com/stacklok/minder/internal/auth/noop"
 	mockauthz "github.com/stacklok/minder/internal/authz/mock"
 	"github.com/stacklok/minder/internal/config"
 	serverconfig "github.com/stacklok/minder/internal/config/server"
-	"github.com/stacklok/minder/internal/controlplane"
+	"github.com/stacklok/minder/internal/controlplane/metrics"
 	"github.com/stacklok/minder/internal/db/embedded"
 	"github.com/stacklok/minder/internal/engine"
 	"github.com/stacklok/minder/internal/logger"
 	"github.com/stacklok/minder/internal/providers/ratecache"
-	"github.com/stacklok/minder/internal/reconcilers"
+	provtelemetry "github.com/stacklok/minder/internal/providers/telemetry"
 	"github.com/stacklok/minder/internal/service"
 )
 
@@ -77,7 +78,7 @@ func runTestServer(cmd *cobra.Command, _ []string) error {
 	cfg.Events.RouterCloseTimeout = 10
 	cfg.Events.Aggregator.LockInterval = 30
 
-	vldtr := noopauth.NewJwtValidator("mindev")
+	validator := noopauth.NewJwtValidator("mindev")
 
 	testServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
 		w.WriteHeader(http.StatusOK)
@@ -94,12 +95,12 @@ func runTestServer(cmd *cobra.Command, _ []string) error {
 		ctx,
 		cfg,
 		store,
-		vldtr,
+		validator,
 		&ratecache.NoopRestClientCache{},
-		[]controlplane.ServerOption{
-			controlplane.WithAuthzClient(&mockauthz.SimpleClient{}),
-		},
+		&mockauthz.SimpleClient{},
+		&auth.IdentityClient{},
+		metrics.NewNoopMetrics(),
+		provtelemetry.NewNoopMetrics(),
 		[]engine.ExecutorOption{},
-		[]reconcilers.ReconcilerOption{},
 	)
 }

cmd/server/app/serve.go

@@ -31,14 +31,12 @@ import (
 	"github.com/stacklok/minder/internal/authz"
 	"github.com/stacklok/minder/internal/config"
 	serverconfig "github.com/stacklok/minder/internal/config/server"
-	"github.com/stacklok/minder/internal/controlplane"
 	cpmetrics "github.com/stacklok/minder/internal/controlplane/metrics"
 	"github.com/stacklok/minder/internal/db"
 	"github.com/stacklok/minder/internal/engine"
 	"github.com/stacklok/minder/internal/logger"
 	"github.com/stacklok/minder/internal/providers/ratecache"
 	provtelemetry "github.com/stacklok/minder/internal/providers/telemetry"
-	"github.com/stacklok/minder/internal/reconcilers"
 	"github.com/stacklok/minder/internal/service"
 )
 
@@ -93,7 +91,7 @@ var serveCmd = &cobra.Command{
 		if err != nil {
 			return fmt.Errorf("failed to create JWKS URL: %w\n", err)
 		}
-		vldtr, err := auth.NewJwtValidator(ctx, jwksUrl.String())
+		validator, err := auth.NewJwtValidator(ctx, jwksUrl.String())
 		if err != nil {
 			return fmt.Errorf("failed to fetch and cache identity provider JWKS: %w\n", err)
 		}
@@ -120,25 +118,24 @@ var serveCmd = &cobra.Command{
 		restClientCache := ratecache.NewRestClientCache(ctx)
 		defer restClientCache.Close()
 
-		serverOpts := []controlplane.ServerOption{
-			controlplane.WithServerMetrics(cpmetrics.NewMetrics()),
-			controlplane.WithProviderMetrics(providerMetrics),
-			controlplane.WithAuthzClient(authzc),
-			controlplane.WithIdentityClient(idClient),
-			controlplane.WithRestClientCache(restClientCache),
-		}
-
 		tsmdw := logger.NewTelemetryStoreWMMiddleware(l)
 		executorOpts := []engine.ExecutorOption{
 			engine.WithProviderMetrics(providerMetrics),
 			engine.WithMiddleware(tsmdw.TelemetryStoreMiddleware),
 		}
 
-		reconcilerOpts := []reconcilers.ReconcilerOption{
-			reconcilers.WithProviderMetrics(providerMetrics),
-		}
-
-		return service.AllInOneServerService(ctx, cfg, store, vldtr, restClientCache, serverOpts, executorOpts, reconcilerOpts)
+		return service.AllInOneServerService(
+			ctx,
+			cfg,
+			store,
+			validator,
+			restClientCache,
+			authzc,
+			idClient,
+			cpmetrics.NewMetrics(),
+			providerMetrics,
+			executorOpts,
+		)
 	},
 }
 

internal/controlplane/handlers_githubwebhooks_test.go

@@ -93,7 +93,7 @@ func (s *UnitTestSuite) TestHandleWebHookPing() {
 	defer ctrl.Finish()
 
 	mockStore := mockdb.NewMockStore(ctrl)
-	srv, evt := newDefaultServer(t, mockStore)
+	srv, evt := newDefaultServer(t, mockStore, nil)
 	srv.cfg.WebhookConfig.WebhookSecretFile = whSecretFile.Name()
 	defer evt.Close()
 
@@ -149,7 +149,7 @@ func (s *UnitTestSuite) TestHandleWebHookUnexistentRepository() {
 	defer ctrl.Finish()
 
 	mockStore := mockdb.NewMockStore(ctrl)
-	srv, evt := newDefaultServer(t, mockStore)
+	srv, evt := newDefaultServer(t, mockStore, nil)
 	defer evt.Close()
 
 	pq := testqueue.NewPassthroughQueue(t)
@@ -221,7 +221,7 @@ func (s *UnitTestSuite) TestHandleWebHookRepository() {
 	defer os.Remove(prevCredsFile.Name())
 
 	mockStore := mockdb.NewMockStore(ctrl)
-	srv, evt := newDefaultServer(t, mockStore)
+	srv, evt := newDefaultServer(t, mockStore, nil)
 	srv.cfg.WebhookConfig.WebhookSecret = "not-our-secret"
 	srv.cfg.WebhookConfig.PreviousWebhookSecretFile = prevCredsFile.Name()
 	defer evt.Close()
@@ -329,7 +329,7 @@ func (s *UnitTestSuite) TestHandleWebHookUnexistentRepoPackage() {
 	defer ctrl.Finish()
 
 	mockStore := mockdb.NewMockStore(ctrl)
-	srv, evt := newDefaultServer(t, mockStore)
+	srv, evt := newDefaultServer(t, mockStore, nil)
 	defer evt.Close()
 
 	pq := testqueue.NewPassthroughQueue(t)
@@ -397,7 +397,7 @@ func (s *UnitTestSuite) TestNoopWebhookHandler() {
 	defer ctrl.Finish()
 
 	mockStore := mockdb.NewMockStore(ctrl)
-	srv, evt := newDefaultServer(t, mockStore)
+	srv, evt := newDefaultServer(t, mockStore, nil)
 	defer evt.Close()
 
 	go func() {

internal/controlplane/handlers_oauth_test.go

@@ -43,6 +43,7 @@ import (
 	"github.com/stacklok/minder/internal/auth"
 	mockjwt "github.com/stacklok/minder/internal/auth/mock"
 	serverconfig "github.com/stacklok/minder/internal/config/server"
+	"github.com/stacklok/minder/internal/controlplane/metrics"
 	"github.com/stacklok/minder/internal/db"
 	"github.com/stacklok/minder/internal/engine"
 	"github.com/stacklok/minder/internal/events"
@@ -313,15 +314,15 @@ func TestGetAuthorizationURL(t *testing.T) {
 			}
 			mockJwt := mockjwt.NewMockJwtValidator(ctrl)
 
-			server, err := NewServer(
-				store,
-				evt,
-				c,
-				mockJwt,
-				&ratecache.NoopRestClientCache{},
-				providers.NewProviderStore(store),
-			)
-			require.NoError(t, err, "failed to create server")
+			server := &Server{
+				store:               store,
+				validator:           mockJwt,
+				evt:                 evt,
+				cfg:                 c,
+				providerStore:       providers.NewProviderStore(store),
+				mt:                  metrics.NewNoopMetrics(),
+				providerAuthFactory: auth.NewOAuthConfig,
+			}
 
 			res, err := server.GetAuthorizationURL(ctx, tc.req)
 			tc.checkResponse(t, res, err)
@@ -405,19 +406,16 @@ func TestProviderCallback(t *testing.T) {
 			gh := mockgh.NewMockGitHub(ctrl)
 			gh.EXPECT().GetUserId(gomock.Any()).Return(int64(31337), nil).AnyTimes()
 
-			var opts []ServerOption
+			var restClientCache ratecache.RestClientCache
 			if tc.remoteUser.String != "" {
 				// TODO: verfifyProviderTokenIdentity
 				cancelable, cancel := context.WithCancel(context.Background())
 				defer cancel()
-				clientCache := ratecache.NewRestClientCache(cancelable)
-				clientCache.Set("", "anAccessToken", db.ProviderTypeGithub, gh)
-				opts = []ServerOption{
-					WithRestClientCache(clientCache),
-				}
+				restClientCache = ratecache.NewRestClientCache(cancelable)
+				restClientCache.Set("", "anAccessToken", db.ProviderTypeGithub, gh)
 			}
 
-			s, _ := newDefaultServer(t, store, opts...)
+			s, _ := newDefaultServer(t, store, restClientCache)
 
 			var err error
 			encryptedUrlString, err := s.cryptoEngine.EncryptString(tc.redirectUrl)

internal/controlplane/handlers_providers_test.go

@@ -136,7 +136,6 @@ func TestDeleteProvider(t *testing.T) {
 		authzClient:     authzClient,
 		providerStore:   providerStore,
 		providerManager: providerManager,
-		restClientCache: clientCache,
 		cfg:             &serverconfig.Config{},
 	}
 
@@ -250,7 +249,6 @@ func TestDeleteProviderByID(t *testing.T) {
 		authzClient:     authzClient,
 		providerStore:   providerStore,
 		providerManager: providerManager,
-		restClientCache: clientCache,
 		cfg:             &serverconfig.Config{},
 	}
 

internal/controlplane/handlers_token.go

@@ -65,7 +65,7 @@ func TokenValidationInterceptor(ctx context.Context, req interface{}, info *grpc
 
 	server := info.Server.(*Server)
 
-	parsedToken, err := server.vldtr.ParseAndValidate(token)
+	parsedToken, err := server.validator.ParseAndValidate(token)
 	if err != nil {
 		zerolog.Ctx(ctx).Info().Msgf("Error validating token %s", token)
 		return nil, status.Errorf(codes.Unauthenticated, "invalid auth token: %v", err)

internal/controlplane/handlers_user.go

@@ -45,7 +45,7 @@ func (s *Server) CreateUser(ctx context.Context,
 		return nil, status.Errorf(codes.InvalidArgument, "no auth token: %v", err)
 	}
 
-	token, err := s.vldtr.ParseAndValidate(tokenString)
+	token, err := s.validator.ParseAndValidate(tokenString)
 	if err != nil {
 		return nil, status.Errorf(codes.Unauthenticated, "failed to parse bearer token: %v", err)
 	}
@@ -156,7 +156,7 @@ func (s *Server) DeleteUser(ctx context.Context,
 		return nil, status.Errorf(codes.InvalidArgument, "no auth token: %v", err)
 	}
 
-	token, err := s.vldtr.ParseAndValidate(tokenString)
+	token, err := s.validator.ParseAndValidate(tokenString)
 	if err != nil {
 		return nil, status.Errorf(codes.Unauthenticated, "failed to parse bearer token: %v", err)
 	}
@@ -218,7 +218,7 @@ func (s *Server) GetUser(ctx context.Context, _ *pb.GetUserRequest) (*pb.GetUser
 		return nil, status.Errorf(codes.InvalidArgument, "no auth token: %v", err)
 	}
 
-	openIdToken, err := s.vldtr.ParseAndValidate(tokenString)
+	openIdToken, err := s.validator.ParseAndValidate(tokenString)
 	if err != nil {
 		return nil, status.Errorf(codes.Unauthenticated, "failed to parse bearer token: %v", err)
 	}

internal/controlplane/handlers_user_test.go

@@ -44,7 +44,6 @@ import (
 	"github.com/stacklok/minder/internal/projects"
 	"github.com/stacklok/minder/internal/providers"
 	mockprov "github.com/stacklok/minder/internal/providers/github/service/mock"
-	"github.com/stacklok/minder/internal/providers/ratecache"
 	pb "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1"
 )
 
@@ -265,7 +264,7 @@ func TestCreateUser_gRPC(t *testing.T) {
 				store:        mockStore,
 				cfg:          &serverconfig.Config{},
 				cryptoEngine: crypeng,
-				vldtr:        mockJwtValidator,
+				validator:    mockJwtValidator,
 				ghProviders:  mockProviders,
 				authzClient:  authz,
 				projectCreator: projects.NewProjectCreator(
@@ -356,7 +355,7 @@ func TestDeleteUserDBMock(t *testing.T) {
 				},
 			},
 		},
-		vldtr:        mockJwtValidator,
+		validator:    mockJwtValidator,
 		cryptoEngine: crypeng,
 		authzClient:  &mock.NoopClient{Authorized: true},
 	}
@@ -454,10 +453,13 @@ func TestDeleteUser_gRPC(t *testing.T) {
 				GoChannel: serverconfig.GoChannelEventConfig{},
 			})
 			require.NoError(t, err, "failed to setup eventer")
-			server, err := NewServer(
-				mockStore,
-				evt,
-				&serverconfig.Config{
+			server := &Server{
+				evt:           evt,
+				store:         mockStore,
+				validator:     mockJwtValidator,
+				providerStore: providers.NewProviderStore(mockStore),
+				authzClient:   &mock.SimpleClient{},
+				cfg: &serverconfig.Config{
 					Auth: serverconfig.AuthConfig{
 						TokenKey: generateTokenKey(t),
 					},
@@ -469,11 +471,7 @@ func TestDeleteUser_gRPC(t *testing.T) {
 						},
 					},
 				},
-				mockJwtValidator,
-				&ratecache.NoopRestClientCache{},
-				providers.NewProviderStore(mockStore),
-			)
-			require.NoError(t, err, "failed to create test server")
+			}
 
 			resp, err := server.DeleteUser(ctx, tc.req)
 			tc.checkResponse(t, resp, err)