server: make DI practical for controllers

internal/controllers/builder.go

@@ -0,0 +1,53 @@
+package controllers
+
+import (
+	"context"
+	"errors"
+	"fmt"
+
+	ctrl "sigs.k8s.io/controller-runtime"
+	ctrlclient "sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/reconcile"
+
+	"github.com/tilt-dev/tilt/internal/store"
+)
+
+type Controller interface {
+	reconcile.Reconciler
+	SetClient(client ctrlclient.Client)
+	SetupWithManager(mgr ctrl.Manager) error
+}
+
+type ControllerBuilder struct {
+	tscm        *TiltServerControllerManager
+	controllers []Controller
+}
+
+func NewControllerBuilder(tscm *TiltServerControllerManager, controllers []Controller) *ControllerBuilder {
+	return &ControllerBuilder{
+		tscm:        tscm,
+		controllers: controllers,
+	}
+}
+
+var _ store.Subscriber = &ControllerBuilder{}
+var _ store.SetUpper = &ControllerBuilder{}
+
+func (c *ControllerBuilder) OnChange(_ context.Context, _ store.RStore) {}
+
+func (c *ControllerBuilder) SetUp(_ context.Context, _ store.RStore) error {
+	mgr := c.tscm.GetManager()
+	client := c.tscm.GetClient()
+
+	if mgr == nil || client == nil {
+		return errors.New("controller manager not initialized")
+	}
+
+	for _, controller := range c.controllers {
+		controller.SetClient(client)
+		if err := controller.SetupWithManager(mgr); err != nil {
+			return fmt.Errorf("error initializing %T controller: %v", controller, err)
+		}
+	}
+	return nil
+}

internal/controllers/core/filewatch/controller.go

@@ -0,0 +1,54 @@
+/*
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+    http://www.apache.org/licenses/LICENSE-2.0
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package filewatch
+
+import (
+	"context"
+
+	ctrl "sigs.k8s.io/controller-runtime"
+	ctrlclient "sigs.k8s.io/controller-runtime/pkg/client"
+
+	"github.com/tilt-dev/tilt/internal/store"
+	filewatches "github.com/tilt-dev/tilt/pkg/apis/core/v1alpha1"
+)
+
+// FileWatchController reconciles a FileWatch object
+type Controller struct {
+	ctrlclient.Client
+	Store store.RStore
+}
+
+func NewController(store store.RStore) *Controller {
+	return &Controller{
+		Store: store,
+	}
+}
+
+// +kubebuilder:rbac:groups=,resources=filewatches,verbs=get;list;watch;create;update;patch;delete
+// +kubebuilder:rbac:groups=,resources=filewatches/status,verbs=get;update;patch
+// +kubebuilder:rbac:groups=,resources=filewatches/finalizers,verbs=update
+
+func (r *Controller) Reconcile(_ context.Context, _ ctrl.Request) (ctrl.Result, error) {
+	// this is currently a no-op stub
+	return ctrl.Result{}, nil
+}
+
+func (r *Controller) SetClient(client ctrlclient.Client) {
+	r.Client = client
+}
+
+func (r *Controller) SetupWithManager(mgr ctrl.Manager) error {
+	return ctrl.NewControllerManagedBy(mgr).
+		For(&filewatches.FileWatch{}).
+		Complete(r)
+}

internal/controllers/manager.go

@@ -6,45 +6,54 @@ import (
 	"fmt"
 
 	"k8s.io/apimachinery/pkg/runtime"
-	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	"k8s.io/client-go/rest"
 	ctrl "sigs.k8s.io/controller-runtime"
+	ctrlclient "sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/log/zap"
 
 	"github.com/tilt-dev/tilt/internal/hud/server"
 	"github.com/tilt-dev/tilt/internal/store"
-	corev1alpha1 "github.com/tilt-dev/tilt/pkg/apis/core/v1alpha1"
 	"github.com/tilt-dev/tilt/pkg/logger"
 )
 
 type TiltServerControllerManager struct {
-	config *rest.Config
-	cancel context.CancelFunc
+	config  *rest.Config
+	scheme  *runtime.Scheme
+	manager ctrl.Manager
+	cancel  context.CancelFunc
 }
 
 var _ store.SetUpper = &TiltServerControllerManager{}
 var _ store.Subscriber = &TiltServerControllerManager{}
 var _ store.TearDowner = &TiltServerControllerManager{}
 
-func NewTiltServerControllerManager(config *server.APIServerConfig) *TiltServerControllerManager {
+func NewTiltServerControllerManager(config *server.APIServerConfig, scheme *runtime.Scheme) (*TiltServerControllerManager, error) {
 	return &TiltServerControllerManager{
 		config: config.GenericConfig.LoopbackClientConfig,
+		scheme: scheme,
+	}, nil
+}
+
+func (m *TiltServerControllerManager) GetManager() ctrl.Manager {
+	return m.manager
+}
+
+func (m *TiltServerControllerManager) GetClient() ctrlclient.Client {
+	if m.manager == nil {
+		return nil
 	}
+	return m.manager.GetClient()
 }
 
 func (m *TiltServerControllerManager) SetUp(ctx context.Context, st store.RStore) error {
-	scheme := runtime.NewScheme()
-
 	ctx, m.cancel = context.WithCancel(ctx)
 
 	// TODO(milas): we should provide a logr.Logger facade for our logger rather than using zap
 	w := logger.Get(ctx).Writer(logger.DebugLvl)
 	ctrl.SetLogger(zap.New(zap.UseDevMode(true), zap.WriteTo(w)))
 
-	utilruntime.Must(corev1alpha1.AddToScheme(scheme))
-
 	mgr, err := ctrl.NewManager(m.config, ctrl.Options{
-		Scheme: scheme,
+		Scheme: m.scheme,
 		// controller manager lazily listens on a port if a webhook is registered; this functionality
 		// is currently NOT used; to prevent it from listening on a default port (9443) and potentially
 		// causing conflicts running multiple instances of tilt, this is set to an invalid value
@@ -58,15 +67,18 @@ func (m *TiltServerControllerManager) SetUp(ctx context.Context, st store.RStore
 		LeaderElectionID: "tilt-apiserver-ctrl",
 	})
 	if err != nil {
-		return fmt.Errorf("unable to start manager: %v", err)
+		return fmt.Errorf("unable to create controller manager: %v", err)
 	}
 
 	go func() {
 		if err := mgr.Start(ctx); err != nil && !errors.Is(err, context.Canceled) {
+			err = fmt.Errorf("controller manager stopped unexpectedly: %v", err)
 			st.Dispatch(store.NewErrorAction(err))
 		}
 	}()
 
+	m.manager = mgr
+
 	return nil
 }
 

internal/controllers/scheme.go

@@ -0,0 +1,17 @@
+package controllers
+
+import (
+	"k8s.io/apimachinery/pkg/runtime"
+	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
+
+	corev1alpha1 "github.com/tilt-dev/tilt/pkg/apis/core/v1alpha1"
+)
+
+func NewScheme() *runtime.Scheme {
+	scheme := runtime.NewScheme()
+
+	// modify pkg/apis/core/v1alpha1/register.go to get new types within core group registered
+	utilruntime.Must(corev1alpha1.AddToScheme(scheme))
+
+	return scheme
+}

internal/controllers/wire.go

@@ -1,7 +1,28 @@
 package controllers
 
-import "github.com/google/wire"
+import (
+	"github.com/google/wire"
+
+	"github.com/tilt-dev/tilt/internal/controllers/core/filewatch"
+)
+
+var controllerSet = wire.NewSet(
+	filewatch.NewController,
+
+	ProvideControllers,
+)
+
+func ProvideControllers(fileWatch *filewatch.Controller) []Controller {
+	return []Controller{
+		fileWatch,
+	}
+}
 
 var WireSet = wire.NewSet(
 	NewTiltServerControllerManager,
+
+	NewScheme,
+	NewControllerBuilder,
+
+	controllerSet,
 )

internal/engine/subscribers.go

@@ -25,6 +25,7 @@ import (
 func ProvideSubscribers(
 	hudsc *server.HeadsUpServerController,
 	tscm *controllers.TiltServerControllerManager,
+	cb *controllers.ControllerBuilder,
 	hud hud.HeadsUpDisplay,
 	ts *hud.TerminalStream,
 	tp *prompt.TerminalPrompt,
@@ -58,6 +59,7 @@ func ProvideSubscribers(
 		// The controller manager must go after the API server,
 		// so that it can connect to it and make resources available.
 		tscm,
+		cb,
 
 		hud,
 		ts,

internal/engine/upper_test.go

@@ -3776,7 +3776,9 @@ func newTestFixture(t *testing.T) *testFixture {
 	tp := prompt.NewTerminalPrompt(ta, prompt.TTYOpen, prompt.BrowserOpen,
 		log, "localhost", model.WebURL{})
 	h := hud.NewFakeHud()
-	tscm := controllers.NewTiltServerControllerManager(serverOptions)
+	tscm, err := controllers.NewTiltServerControllerManager(serverOptions, controllers.NewScheme())
+	require.NoError(t, err, "Failed to create Tilt API server controller manager")
+	cb := controllers.NewControllerBuilder(tscm, nil)
 
 	dp := dockerprune.NewDockerPruner(dockerClient)
 	dp.DisabledForTesting(true)
@@ -3819,7 +3821,7 @@ func newTestFixture(t *testing.T) *testFixture {
 	mc := metrics.NewController(de, model.TiltBuild{}, "")
 	mcc := metrics.NewModeController("localhost", user.NewFakePrefs())
 
-	subs := ProvideSubscribers(hudsc, tscm, h, ts, tp, pw, sw, plm, pfc, fwm, gm, bc, cc, dcw, dclm, pm, ar, au, ewm, tcum, dp, tc, lc, podm, ec, mc, mcc)
+	subs := ProvideSubscribers(hudsc, tscm, cb, h, ts, tp, pw, sw, plm, pfc, fwm, gm, bc, cc, dcw, dclm, pm, ar, au, ewm, tcum, dp, tc, lc, podm, ec, mc, mcc)
 	ret.upper, err = NewUpper(ctx, st, subs)
 	require.NoError(t, err)
 

internal/store/subscriber.go

@@ -21,7 +21,9 @@ type Subscriber interface {
 //
 // Both hold the subscriber lock, so should return quickly.
 //
-// SetUp and TearDown are called in serial, in the order they were added.
+// SetUp and TearDown are called in serial.
+// SetUp is called in FIFO order while TearDown is LIFO so that
+// inter-subscriber dependencies are respected.
 type SetUpper interface {
 	// Initialize the subscriber.
 	//
@@ -91,14 +93,15 @@ func (l *subscriberList) SetUp(ctx context.Context, st RStore) error {
 	return nil
 }
 
+// TeardownAll removes subscribes in the reverse order as they were subscribed.
 func (l *subscriberList) TeardownAll(ctx context.Context) {
 	l.mu.Lock()
 	subscribers := append([]*subscriberEntry{}, l.subscribers...)
 	l.setup = false
 	l.mu.Unlock()
 
-	for _, s := range subscribers {
-		s.maybeTeardown(ctx)
+	for i := len(subscribers) - 1; i >= 0; i-- {
+		subscribers[i].maybeTeardown(ctx)
 	}
 }