ipn/ipnext: remove some interface indirection to add hooks

Now that 25c4dc5 removed unregistering hooks and made them into
slices, just expose the slices and remove the setter funcs.

This removes boilerplate ceremony around adding new hooks.

This does export the hooks and make them mutable at runtime in theory,
but that'd be a data race. If we really wanted to lock it down in the
future we could make the feature.Hooks slice type be an opaque struct
with an All() iterator and a "frozen" bool and we could freeze all the
hooks after init. But that doesn't seem worth it.

This means that hook registration is also now all in one place, rather
than being mixed into ProfilesService vs ipnext.Host vs FooService vs
BarService. I view that as a feature. When we have a ton of hooks and
the list is long, then we can rearrange the fields in the Hooks struct
as needed, or make sub-structs, or big comments.

Updates tailscale#12614

Change-Id: I05ce5baa45a61e79c04591c2043c05f3288d8587
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>

Author: bradfitz

feature/feature.go

@@ -52,3 +52,21 @@ func (h *Hook[Func]) Get() Func {
 	}
 	return h.f
 }
+
+// Hooks is a slice of funcs.
+//
+// As opposed to a single Hook, this is meant to be used when
+// multiple parties are able to install the same hook.
+type Hooks[Func any] []Func
+
+// Add adds a hook to the list of hooks.
+//
+// Add should only be called during early program
+// startup before Tailscale has started.
+// It is not safe for concurrent use.
+func (h *Hooks[Func]) Add(f Func) {
+	if reflect.ValueOf(f).IsZero() {
+		panic("Add with zero value")
+	}
+	*h = append(*h, f)
+}

feature/relayserver/relayserver.go

@@ -71,7 +71,7 @@ func (e *extension) Name() string {
 func (e *extension) Init(host ipnext.Host) error {
 	profile, prefs := host.Profiles().CurrentProfileState()
 	e.profileStateChanged(profile, prefs, false)
-	host.Profiles().RegisterProfileStateChangeCallback(e.profileStateChanged)
+	host.Hooks().ProfileStateChange.Add(e.profileStateChanged)
 	// TODO(jwhited): callback for netmap/nodeattr changes (e.hasNodeAttrRelayServer)
 	return nil
 }

ipn/auditlog/extension.go

@@ -63,9 +63,9 @@ func (e *extension) Name() string {
 // Init implements [ipnext.Extension] by registering callbacks and providers
 // for the duration of the extension's lifetime.
 func (e *extension) Init(h ipnext.Host) error {
-	h.RegisterControlClientCallback(e.controlClientChanged)
-	h.Profiles().RegisterProfileStateChangeCallback(e.profileChanged)
-	h.RegisterAuditLogProvider(e.getCurrentLogger)
+	h.Hooks().NewControlClient.Add(e.controlClientChanged)
+	h.Hooks().ProfileStateChange.Add(e.profileChanged)
+	h.Hooks().AuditLoggers.Add(e.getCurrentLogger)
 	return nil
 }
 

ipn/desktop/extension.go

@@ -77,7 +77,7 @@ func (e *desktopSessionsExt) Init(host ipnext.Host) (err error) {
 	if err != nil {
 		return fmt.Errorf("session callback registration failed: %w", err)
 	}
-	host.Profiles().RegisterBackgroundProfileResolver(e.getBackgroundProfile)
+	host.Hooks().BackgroundProfileResolvers.Add(e.getBackgroundProfile)
 	e.cleanup = []func(){unregisterSessionCb}
 	return nil
 }

ipn/ipnext/ipnext.go

@@ -10,6 +10,7 @@ import (
 	"fmt"
 
 	"tailscale.com/control/controlclient"
+	"tailscale.com/feature"
 	"tailscale.com/ipn"
 	"tailscale.com/ipn/ipnauth"
 	"tailscale.com/tsd"
@@ -182,14 +183,6 @@ type Host interface {
 	// Profiles returns the host's [ProfileServices].
 	Profiles() ProfileServices
 
-	// RegisterAuditLogProvider registers an audit log provider,
-	// which returns a function to be called when an auditable action
-	// is about to be performed.
-	//
-	// It is a runtime error to register a nil provider or call after the host
-	// has been initialized.
-	RegisterAuditLogProvider(AuditLogProvider)
-
 	// AuditLogger returns a function that calls all currently registered audit loggers.
 	// The function fails if any logger returns an error, indicating that the action
 	// cannot be logged and must not be performed.
@@ -198,12 +191,9 @@ type Host interface {
 	// the time of the call and must not be persisted.
 	AuditLogger() ipnauth.AuditLogFunc
 
-	// RegisterControlClientCallback registers a function to be called every time a new
-	// control client is created.
-	//
-	// It is a runtime error to register a nil provider or call after the host
-	// has been initialized.
-	RegisterControlClientCallback(NewControlClientCallback)
+	// Hooks returns a non-nil pointer to a [Hooks] struct.
+	// Hooks must not be modified concurrently or after Tailscale has started.
+	Hooks() *Hooks
 
 	// SendNotifyAsync sends a notification to the IPN bus,
 	// typically to the GUI client.
@@ -269,28 +259,6 @@ type ProfileServices interface {
 	// to a client connecting or disconnecting or a change in the desktop
 	// session state. It is used for logging.
 	SwitchToBestProfileAsync(reason string)
-
-	// RegisterBackgroundProfileResolver registers a function to be used when
-	// resolving the background profile.
-	//
-	// It is a runtime error to register a nil provider or call after the host
-	// has been initialized.
-	//
-	// TODO(nickkhyl): allow specifying some kind of priority/altitude for the resolver.
-	// TODO(nickkhyl): make it a "profile resolver" instead of a "background profile resolver".
-	// The concepts of the "current user", "foreground profile" and "background profile"
-	// only exist on Windows, and we're moving away from them anyway.
-	RegisterBackgroundProfileResolver(ProfileResolver)
-
-	// RegisterProfileStateChangeCallback registers a function to be called when the current
-	// [ipn.LoginProfile] or its [ipn.Prefs] change.
-	//
-	// To get the initial profile or prefs, use [ProfileServices.CurrentProfileState]
-	// or [ProfileServices.CurrentPrefs] from the extension's [Extension.Init].
-	//
-	// It is a runtime error to register a nil provider or call after the host
-	// has been initialized.
-	RegisterProfileStateChangeCallback(ProfileStateChangeCallback)
 }
 
 // ProfileStore provides read-only access to available login profiles and their preferences.
@@ -354,3 +322,36 @@ type ProfileStateChangeCallback func(_ ipn.LoginProfileView, _ ipn.PrefsView, sa
 // It returns a function to be called when the cc is being shut down,
 // or nil if no cleanup is needed.
 type NewControlClientCallback func(controlclient.Client, ipn.LoginProfileView) (cleanup func())
+
+// Hooks is a collection of hooks that extensions can add to (non-concurrently)
+// during program initialization and can be called by LocalBackend and others at
+// runtime.
+//
+// Each hook has its own rules about when it's called and what environment it
+// has access to and what it's allowed to do.
+type Hooks struct {
+	// ProfileStateChange are callbacks that are invoked when the current login profile
+	// or its [ipn.Prefs] change, after those changes have been made. The current login profile
+	// may be changed either because of a profile switch, or because the profile information
+	// was updated by [LocalBackend.SetControlClientStatus], including when the profile
+	// is first populated and persisted.
+	ProfileStateChange feature.Hooks[ProfileStateChangeCallback]
+
+	// BackgroundProfileResolvers are registered background profile resolvers.
+	// They're used to determine the profile to use when no GUI/CLI client is connected.
+	//
+	// TODO(nickkhyl): allow specifying some kind of priority/altitude for the resolver.
+	// TODO(nickkhyl): make it a "profile resolver" instead of a "background profile resolver".
+	// The concepts of the "current user", "foreground profile" and "background profile"
+	// only exist on Windows, and we're moving away from them anyway.
+	BackgroundProfileResolvers feature.Hooks[ProfileResolver]
+
+	// AuditLoggers are registered [AuditLogProvider]s.
+	// Each provider is called to get an [ipnauth.AuditLogFunc] when an auditable action
+	// is about to be performed. If an audit logger returns an error, the action is denied.
+	AuditLoggers feature.Hooks[AuditLogProvider]
+
+	// NewControlClient are the functions to be called when a new control client
+	// is created. It is called with the LocalBackend locked.
+	NewControlClient feature.Hooks[NewControlClientCallback]
+}

ipn/ipnlocal/extension_host.go

@@ -64,8 +64,9 @@ import (
 // and to further reduce the risk of accessing unexported methods or fields of [LocalBackend], the host interacts
 // with it via the [Backend] interface.
 type ExtensionHost struct {
-	b    Backend
-	logf logger.Logf // prefixed with "ipnext:"
+	b     Backend
+	hooks ipnext.Hooks
+	logf  logger.Logf // prefixed with "ipnext:"
 
 	// allExtensions holds the extensions in the order they were registered,
 	// including those that have not yet attempted initialization or have failed to initialize.
@@ -84,22 +85,6 @@ type ExtensionHost struct {
 	// doEnqueueBackendOperation adds an asynchronous [LocalBackend] operation to the workQueue.
 	doEnqueueBackendOperation func(func(Backend))
 
-	// profileStateChangeCbs are callbacks that are invoked when the current login profile
-	// or its [ipn.Prefs] change, after those changes have been made. The current login profile
-	// may be changed either because of a profile switch, or because the profile information
-	// was updated by [LocalBackend.SetControlClientStatus], including when the profile
-	// is first populated and persisted.
-	profileStateChangeCbs []ipnext.ProfileStateChangeCallback
-	// backgroundProfileResolvers are registered background profile resolvers.
-	// They're used to determine the profile to use when no GUI/CLI client is connected.
-	backgroundProfileResolvers []ipnext.ProfileResolver
-	// auditLoggers are registered [AuditLogProvider]s.
-	// Each provider is called to get an [ipnauth.AuditLogFunc] when an auditable action
-	// is about to be performed. If an audit logger returns an error, the action is denied.
-	auditLoggers []ipnext.AuditLogProvider
-	// newControlClientCbs are the functions to be called when a new control client is created.
-	newControlClientCbs []ipnext.NewControlClientCallback
-
 	shuttingDown atomic.Bool
 
 	// mu protects the following fields.
@@ -208,6 +193,15 @@ func (h *ExtensionHost) Init() {
 	}
 }
 
+var zeroHooks ipnext.Hooks
+
+func (h *ExtensionHost) Hooks() *ipnext.Hooks {
+	if h == nil {
+		return &zeroHooks
+	}
+	return &h.hooks
+}
+
 func (h *ExtensionHost) init() {
 	defer h.initDone.Store(true)
 
@@ -360,24 +354,6 @@ func (h *ExtensionHost) SendNotifyAsync(n ipn.Notify) {
 	})
 }
 
-// addFuncHook appends non-nil fn to hooks.
-func addFuncHook[F any](h *ExtensionHost, hooks *[]F, fn F) {
-	if h.initDone.Load() {
-		panic("invalid callback register after init")
-	}
-	if reflect.ValueOf(fn).IsZero() {
-		panic("nil function hook")
-	}
-	*hooks = append(*hooks, fn)
-}
-
-// RegisterProfileStateChangeCallback implements [ipnext.ProfileServices].
-func (h *ExtensionHost) RegisterProfileStateChangeCallback(cb ipnext.ProfileStateChangeCallback) {
-	if h != nil {
-		addFuncHook(h, &h.profileStateChangeCbs, cb)
-	}
-}
-
 // NotifyProfileChange invokes registered profile state change callbacks
 // and updates the current profile and prefs in the host.
 // It strips private keys from the [ipn.Prefs] before preserving
@@ -397,7 +373,7 @@ func (h *ExtensionHost) NotifyProfileChange(profile ipn.LoginProfileView, prefs
 	h.currentProfile = profile
 	h.mu.Unlock()
 
-	for _, cb := range h.profileStateChangeCbs {
+	for _, cb := range h.hooks.ProfileStateChange {
 		cb(profile, prefs, sameNode)
 	}
 }
@@ -421,18 +397,11 @@ func (h *ExtensionHost) NotifyProfilePrefsChanged(profile ipn.LoginProfileView,
 	// Get the callbacks to be invoked.
 	h.mu.Unlock()
 
-	for _, cb := range h.profileStateChangeCbs {
+	for _, cb := range h.hooks.ProfileStateChange {
 		cb(profile, newPrefs, true)
 	}
 }
 
-// RegisterBackgroundProfileResolver implements [ipnext.ProfileServices].
-func (h *ExtensionHost) RegisterBackgroundProfileResolver(resolver ipnext.ProfileResolver) {
-	if h != nil {
-		addFuncHook(h, &h.backgroundProfileResolvers, resolver)
-	}
-}
-
 func (h *ExtensionHost) active() bool {
 	return h != nil && !h.shuttingDown.Load()
 }
@@ -455,7 +424,7 @@ func (h *ExtensionHost) DetermineBackgroundProfile(profiles ipnext.ProfileStore)
 
 	// Attempt to resolve the background profile using the registered
 	// background profile resolvers (e.g., [ipn/desktop.desktopSessionsExt] on Windows).
-	for _, resolver := range h.backgroundProfileResolvers {
+	for _, resolver := range h.hooks.BackgroundProfileResolvers {
 		if profile := resolver(profiles); profile.Valid() {
 			return profile
 		}
@@ -466,37 +435,20 @@ func (h *ExtensionHost) DetermineBackgroundProfile(profiles ipnext.ProfileStore)
 	return ipn.LoginProfileView{}
 }
 
-// RegisterControlClientCallback implements [ipnext.Host].
-func (h *ExtensionHost) RegisterControlClientCallback(cb ipnext.NewControlClientCallback) {
-	if h != nil {
-		addFuncHook(h, &h.newControlClientCbs, cb)
-	}
-}
-
 // NotifyNewControlClient invokes all registered control client callbacks.
 // It returns callbacks to be executed when the control client shuts down.
 func (h *ExtensionHost) NotifyNewControlClient(cc controlclient.Client, profile ipn.LoginProfileView) (ccShutdownCbs []func()) {
 	if !h.active() {
 		return nil
 	}
-	if len(h.newControlClientCbs) > 0 {
-		ccShutdownCbs = make([]func(), 0, len(h.newControlClientCbs))
-		for _, cb := range h.newControlClientCbs {
-			if shutdown := cb(cc, profile); shutdown != nil {
-				ccShutdownCbs = append(ccShutdownCbs, shutdown)
-			}
+	for _, cb := range h.hooks.NewControlClient {
+		if shutdown := cb(cc, profile); shutdown != nil {
+			ccShutdownCbs = append(ccShutdownCbs, shutdown)
 		}
 	}
 	return ccShutdownCbs
 }
 
-// RegisterAuditLogProvider implements [ipnext.Host].
-func (h *ExtensionHost) RegisterAuditLogProvider(provider ipnext.AuditLogProvider) {
-	if h != nil {
-		addFuncHook(h, &h.auditLoggers, provider)
-	}
-}
-
 // AuditLogger returns a function that reports an auditable action
 // to all registered audit loggers. It fails if any of them returns an error,
 // indicating that the action cannot be logged and must not be performed.
@@ -510,8 +462,8 @@ func (h *ExtensionHost) AuditLogger() ipnauth.AuditLogFunc {
 	if !h.active() {
 		return func(tailcfg.ClientAuditAction, string) error { return nil }
 	}
-	loggers := make([]ipnauth.AuditLogFunc, 0, len(h.auditLoggers))
-	for _, provider := range h.auditLoggers {
+	loggers := make([]ipnauth.AuditLogFunc, 0, len(h.hooks.AuditLoggers))
+	for _, provider := range h.hooks.AuditLoggers {
 		loggers = append(loggers, provider())
 	}
 	return func(action tailcfg.ClientAuditAction, details string) error {

ipn/ipnlocal/extension_host_test.go

@@ -748,7 +748,7 @@ func TestExtensionHostProfileStateChangeCallback(t *testing.T) {
 				tt.ext.InitHook = func(e *testExtension) error {
 					// Create and register the callback on init.
 					handler := makeStateChangeAppender(e)
-					e.host.Profiles().RegisterProfileStateChangeCallback(handler)
+					e.host.Hooks().ProfileStateChange.Add(handler)
 					return nil
 				}
 			}
@@ -875,7 +875,7 @@ func TestBackgroundProfileResolver(t *testing.T) {
 			// This is typically done by the extensions themselves,
 			// but we do it here for testing purposes.
 			for _, r := range tt.resolvers {
-				h.Profiles().RegisterBackgroundProfileResolver(r)
+				h.Hooks().BackgroundProfileResolvers.Add(r)
 			}
 			h.Init()
 
@@ -968,7 +968,7 @@ func TestAuditLogProviders(t *testing.T) {
 					}
 				}
 				ext.InitHook = func(e *testExtension) error {
-					e.host.RegisterAuditLogProvider(provider)
+					e.host.Hooks().AuditLoggers.Add(provider)
 					return nil
 				}
 				exts = append(exts, ext)