Refactor uiserver to separate package, cleaner Reloading

GNUmakefile

@@ -306,7 +306,7 @@ lint:
 # also run as part of the release build script when it verifies that there are no
 # changes to the UI assets that aren't checked in.
 static-assets:
-	@go-bindata-assetfs -pkg agent -prefix pkg -o $(ASSETFS_PATH) ./pkg/web_ui/...
+	@go-bindata-assetfs -pkg uiserver -prefix pkg -o $(ASSETFS_PATH) ./pkg/web_ui/...
 	@go fmt $(ASSETFS_PATH)
 
 

agent/agent.go

@@ -15,7 +15,6 @@ import (
 	"strconv"
 	"strings"
 	"sync"
-	"sync/atomic"
 	"time"
 
 	"github.com/hashicorp/consul/agent/dns"
@@ -261,6 +260,23 @@ type Agent struct {
 	// fail, the agent will be shutdown.
 	apiServers *apiServers
 
+	// httpHandlers provides direct access to (one of) the HTTPHandlers started by
+	// this agent. This is used in tests to test HTTP endpoints without overhead
+	// of TCP connections etc.
+	//
+	// TODO: this is a temporary re-introduction after we removed a list of
+	// HTTPServers in favour of apiServers abstraction. Now that HTTPHandlers is
+	// stateful and has config reloading though it's not OK to just use a
+	// different instance of handlers in tests to the ones that the agent is wired
+	// up to since then config reloads won't actually affect the handlers under
+	// test while plumbing the external handlers in the TestAgent through bypasses
+	// testing that the agent itself is actually reloading the state correctly.
+	// Once we move `apiServers` to be a passed-in dependency for NewAgent, we
+	// should be able to remove this and have the Test Agent create the
+	// HTTPHandlers and pass them in removing the need to pull them back out
+	// again.
+	httpHandlers *HTTPHandlers
+
 	// wgServers is the wait group for all HTTP and DNS servers
 	// TODO: remove once dnsServers are handled by apiServers
 	wgServers sync.WaitGroup
@@ -302,16 +318,9 @@ type Agent struct {
 	// Shared RPC Router
 	router *router.Router
 
-	// uiConfig is an atomically held copy of the current UI config. It's
-	// populated from config.UIConfig when the agent is first constructed but then
-	// becomes the canonical source where all reads during runtime should be made.
-	// It needs to be separate because it may be updated by a config reload at
-	// runtime and so must be able to be read and reloaded safely from different
-	// goroutines.
-	//
-	// Internal components that need the latest UI Config should use the
-	// a.getUIConfig() method.
-	uiConfig atomic.Value
+	// configReloaders are subcomponents that need to be notified on a reload so
+	// they can update their internal state.
+	configReloaders []ConfigReloader
 
 	// enterpriseAgent embeds fields that we only access in consul-enterprise builds
 	enterpriseAgent
@@ -361,9 +370,6 @@ func New(bd BaseDeps) (*Agent, error) {
 		router:          bd.Router,
 	}
 
-	// Initialize the UI Config
-	a.uiConfig.Store(a.config.UIConfig)
-
 	a.serviceManager = NewServiceManager(&a)
 
 	// TODO: do this somewhere else, maybe move to newBaseDeps
@@ -773,6 +779,8 @@ func (a *Agent) listenHTTP() ([]apiServer, error) {
 				agent:    a,
 				denylist: NewDenylist(a.config.HTTPBlockEndpoints),
 			}
+			a.configReloaders = append(a.configReloaders, srv.ReloadConfig)
+			a.httpHandlers = srv
 			httpServer := &http.Server{
 				Addr:      l.Addr().String(),
 				TLSConfig: tlscfg,
@@ -3602,8 +3610,11 @@ func (a *Agent) reloadConfigInternal(newCfg *config.RuntimeConfig) error {
 
 	a.State.SetDiscardCheckOutput(newCfg.DiscardCheckOutput)
 
-	// Reload metrics config
-	a.uiConfig.Store(newCfg.UIConfig)
+	for _, r := range a.configReloaders {
+		if err := r(newCfg); err != nil {
+			return err
+		}
+	}
 
 	return nil
 }
@@ -3857,14 +3868,3 @@ func defaultIfEmpty(val, defaultVal string) string {
 	}
 	return defaultVal
 }
-
-// getUIConfig is the canonical way to read the value of the UIConfig at
-// runtime. It is thread safe and returns the most recent configuration which
-// may have changed since the agent started due to config reload.
-func (a *Agent) getUIConfig() config.UIConfig {
-	if cfg, ok := a.uiConfig.Load().(config.UIConfig); ok {
-		return cfg
-	}
-	// Shouldn't happen but be defensive
-	return config.UIConfig{}
-}

agent/agent_test.go

@@ -3492,36 +3492,6 @@ func TestAgent_ReloadConfigTLSConfigFailure(t *testing.T) {
 	require.Len(t, tlsConf.RootCAs.Subjects(), 1)
 }
 
-func TestAgent_ReloadConfigUIConfig(t *testing.T) {
-	t.Parallel()
-	dataDir := testutil.TempDir(t, "agent") // we manage the data dir
-	hcl := `
-		data_dir = "` + dataDir + `"
-		ui_config {
-			enabled = true // note that this is _not_ reloadable
-			metrics_provider = "foo"
-		}
-	`
-	a := NewTestAgent(t, hcl)
-	defer a.Shutdown()
-
-	uiCfg := a.getUIConfig()
-	require.Equal(t, "foo", uiCfg.MetricsProvider)
-
-	hcl = `
-		data_dir = "` + dataDir + `"
-		ui_config {
-			enabled = true
-			metrics_provider = "bar"
-		}
-	`
-	c := TestConfig(testutil.Logger(t), config.FileSource{Name: t.Name(), Format: "hcl", Data: hcl})
-	require.NoError(t, a.reloadConfigInternal(c))
-
-	uiCfg = a.getUIConfig()
-	require.Equal(t, "bar", uiCfg.MetricsProvider)
-}
-
 func TestAgent_consulConfig_AutoEncryptAllowTLS(t *testing.T) {
 	t.Parallel()
 	dataDir := testutil.TempDir(t, "agent") // we manage the data dir