refactor to use interface instead of atomic.Pointers

coder · johnstcn · Nov 24, 2023 · Nov 21, 2023 · Nov 23, 2023 · Nov 23, 2023
commit 79dc190556ba3c2adba4e858e826c1bd486fefe1
diff --git a/coderd/coderd.go b/coderd/coderd.go
@@ -137,12 +137,10 @@ type Options struct {
 	// workspace applications. It consists of both a signing and encryption key.
 	AppSecurityKey workspaceapps.SecurityKey
 
-	FetchWorkspaceProxiesFunc *atomic.Pointer[func(context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error)]
-	UpdateProxyHealthFunc     *atomic.Pointer[func(context.Context) error]
-
-	HealthcheckFunc    func(ctx context.Context, apiKey string) *healthcheck.Report
-	HealthcheckTimeout time.Duration
-	HealthcheckRefresh time.Duration
+	HealthcheckFunc              func(ctx context.Context, apiKey string) *healthcheck.Report
+	HealthcheckTimeout           time.Duration
+	HealthcheckRefresh           time.Duration
+	WorkspaceProxiesFetchUpdater *atomic.Pointer[healthcheck.WorkspaceProxiesFetchUpdater]
 
 	// OAuthSigningKey is the crypto key used to sign and encrypt state strings
 	// related to OAuth. This is a symmetric secret key using hmac to sign payloads.
@@ -401,14 +399,11 @@ func New(options *Options) *API {
 		)
 	}
 
-	// The following two functions are dependencies of HealthcheckFunc but are only implemented
-	// in enterprise. Stubbing them out here.
-	if options.FetchWorkspaceProxiesFunc == nil {
-		options.FetchWorkspaceProxiesFunc = &atomic.Pointer[func(context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error)]{}
-	}
-
-	if options.UpdateProxyHealthFunc == nil {
-		options.UpdateProxyHealthFunc = &atomic.Pointer[func(context.Context) error]{}
+	if options.WorkspaceProxiesFetchUpdater == nil {
+		options.WorkspaceProxiesFetchUpdater = &atomic.Pointer[healthcheck.WorkspaceProxiesFetchUpdater]{}
+		var wpfu healthcheck.WorkspaceProxiesFetchUpdater //nolint:gosimple
+		wpfu = &healthcheck.AGPLWorkspaceProxiesFetchUpdater{}
+		options.WorkspaceProxiesFetchUpdater.Store(&wpfu)
 	}
 
 	if options.HealthcheckFunc == nil {
@@ -430,9 +425,8 @@ func New(options *Options) *API {
 					DERPMap: api.DERPMap(),
 				},
 				WorkspaceProxy: healthcheck.WorkspaceProxyReportOptions{
-					CurrentVersion:        buildinfo.Version(),
-					FetchWorkspaceProxies: options.FetchWorkspaceProxiesFunc.Load(),
-					UpdateProxyHealth:     options.UpdateProxyHealthFunc.Load(),
+					CurrentVersion:               buildinfo.Version(),
+					WorkspaceProxiesFetchUpdater: *(options.WorkspaceProxiesFetchUpdater).Load(),
 				},
 			})
 		}

diff --git a/coderd/healthcheck/workspaceproxy.go b/coderd/healthcheck/workspaceproxy.go
@@ -16,14 +16,8 @@ import (
 type WorkspaceProxyReportOptions struct {
 	// CurrentVersion is the current server version.
 	// We pass this in to make it easier to test.
-	CurrentVersion string
-	// FetchWorkspaceProxies is a function that returns the available workspace proxies.
-	FetchWorkspaceProxies *func(context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error)
-	// UpdateProxyHealth is a function called when healthcheck is run.
-	// This would normally be ProxyHealth.ForceUpdate().
-	// We do this because if someone mashes the healthcheck refresh button
-	// they would expect up-to-date data.
-	UpdateProxyHealth *func(context.Context) error
+	CurrentVersion               string
+	WorkspaceProxiesFetchUpdater WorkspaceProxiesFetchUpdater
 }
 
 // @typescript-generate WorkspaceProxyReport
@@ -36,31 +30,40 @@ type WorkspaceProxyReport struct {
 	WorkspaceProxies codersdk.RegionsResponse[codersdk.WorkspaceProxy] `json:"workspace_proxies"`
 }
 
+type WorkspaceProxiesFetchUpdater interface {
+	Fetch(context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error)
+	Update(context.Context) error
+}
+
+// AGPLWorkspaceProxiesFetchUpdater implements WorkspaceProxiesFetchUpdater
+// to the extent required by AGPL code. Which isn't that much.
+type AGPLWorkspaceProxiesFetchUpdater struct{}
+
+func (*AGPLWorkspaceProxiesFetchUpdater) Fetch(context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) {
+	return codersdk.RegionsResponse[codersdk.WorkspaceProxy]{}, nil
+}
+
+func (*AGPLWorkspaceProxiesFetchUpdater) Update(context.Context) error {
+	return nil
+}
+
 func (r *WorkspaceProxyReport) Run(ctx context.Context, opts *WorkspaceProxyReportOptions) {
 	r.Healthy = true
 	r.Severity = health.SeverityOK
 	r.Warnings = []string{}
 
-	if opts.FetchWorkspaceProxies == nil {
-		return
-	}
-	fetchWorkspaceProxiesFunc := *opts.FetchWorkspaceProxies
-
-	if opts.UpdateProxyHealth == nil {
-		err := "opts.UpdateProxyHealth must not be nil if opts.FetchWorkspaceProxies is not nil"
-		r.Error = ptr.Ref(err)
-		return
+	if opts.WorkspaceProxiesFetchUpdater == nil {
+		opts.WorkspaceProxiesFetchUpdater = &AGPLWorkspaceProxiesFetchUpdater{}
 	}
-	updateProxyHealthFunc := *opts.UpdateProxyHealth
 
 	// If this fails, just mark it as a warning. It is still updated in the background.
-	if err := updateProxyHealthFunc(ctx); err != nil {
+	if err := opts.WorkspaceProxiesFetchUpdater.Update(ctx); err != nil {
 		r.Severity = health.SeverityWarning
 		r.Warnings = append(r.Warnings, xerrors.Errorf("update proxy health: %w", err).Error())
 		return
 	}
 
-	proxies, err := fetchWorkspaceProxiesFunc(ctx)
+	proxies, err := opts.WorkspaceProxiesFetchUpdater.Fetch(ctx)
 	if err != nil {
 		r.Healthy = false
 		r.Severity = health.SeverityError

diff --git a/coderd/healthcheck/workspaceproxy_test.go b/coderd/healthcheck/workspaceproxy_test.go
@@ -8,7 +8,6 @@ import (
 
 	"github.com/coder/coder/v2/coderd/healthcheck"
 	"github.com/coder/coder/v2/coderd/healthcheck/health"
-	"github.com/coder/coder/v2/coderd/util/ptr"
 	"github.com/coder/coder/v2/codersdk"
 )
 
@@ -23,8 +22,8 @@ func TestWorkspaceProxies(t *testing.T) {
 
 	for _, tt := range []struct {
 		name                  string
-		fetchWorkspaceProxies *func(context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error)
-		updateProxyHealth     *func(context.Context) error
+		fetchWorkspaceProxies func(context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error)
+		updateProxyHealth     func(context.Context) error
 		expectedHealthy       bool
 		expectedError         string
 		expectedSeverity      health.Severity
@@ -57,7 +56,7 @@ func TestWorkspaceProxies(t *testing.T) {
 		},
 		{
 			name: "Enabled/OneUnreachable",
-			fetchWorkspaceProxies: ptr.Ref(func(ctx context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) {
+			fetchWorkspaceProxies: func(ctx context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) {
 				return codersdk.RegionsResponse[codersdk.WorkspaceProxy]{
 					Regions: []codersdk.WorkspaceProxy{
 						{
@@ -77,7 +76,7 @@ func TestWorkspaceProxies(t *testing.T) {
 						},
 					},
 				}, nil
-			}),
+			},
 			updateProxyHealth: fakeUpdateProxyHealth(nil),
 			expectedHealthy:   false,
 			expectedSeverity:  health.SeverityError,
@@ -89,9 +88,9 @@ func TestWorkspaceProxies(t *testing.T) {
 				fakeWorkspaceProxy("alpha", true, currentVersion),
 				fakeWorkspaceProxy("beta", true, currentVersion),
 			),
-			updateProxyHealth: ptr.Ref(func(ctx context.Context) error {
+			updateProxyHealth: func(ctx context.Context) error {
 				return nil
-			}),
+			},
 			expectedHealthy:  true,
 			expectedSeverity: health.SeverityOK,
 		},
@@ -167,8 +166,14 @@ func TestWorkspaceProxies(t *testing.T) {
 			var rpt healthcheck.WorkspaceProxyReport
 			var opts healthcheck.WorkspaceProxyReportOptions
 			opts.CurrentVersion = currentVersion
-			opts.FetchWorkspaceProxies = tt.fetchWorkspaceProxies
-			opts.UpdateProxyHealth = tt.updateProxyHealth
+			if tt.fetchWorkspaceProxies != nil && tt.updateProxyHealth != nil {
+				opts.WorkspaceProxiesFetchUpdater = &fakeWorkspaceProxyFetchUpdater{
+					fetchFunc:  tt.fetchWorkspaceProxies,
+					updateFunc: tt.updateProxyHealth,
+				}
+			} else {
+				opts.WorkspaceProxiesFetchUpdater = &healthcheck.AGPLWorkspaceProxiesFetchUpdater{}
+			}
 
 			rpt.Run(context.Background(), &opts)
 
@@ -186,6 +191,20 @@ func TestWorkspaceProxies(t *testing.T) {
 	}
 }
 
+// yet another implementation of the thing
+type fakeWorkspaceProxyFetchUpdater struct {
+	fetchFunc  func(context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error)
+	updateFunc func(context.Context) error
+}
+
+func (u *fakeWorkspaceProxyFetchUpdater) Fetch(ctx context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) {
+	return u.fetchFunc(ctx)
+}
+
+func (u *fakeWorkspaceProxyFetchUpdater) Update(ctx context.Context) error {
+	return u.updateFunc(ctx)
+}
+
 func fakeWorkspaceProxy(name string, healthy bool, version string) codersdk.WorkspaceProxy {
 	return codersdk.WorkspaceProxy{
 		Region: codersdk.Region{
@@ -196,27 +215,24 @@ func fakeWorkspaceProxy(name string, healthy bool, version string) codersdk.Work
 	}
 }
 
-func fakeFetchWorkspaceProxies(ps ...codersdk.WorkspaceProxy) *func(context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) {
-	fn := func(context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) {
+func fakeFetchWorkspaceProxies(ps ...codersdk.WorkspaceProxy) func(context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) {
+	return func(context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) {
 		return codersdk.RegionsResponse[codersdk.WorkspaceProxy]{
 			Regions: ps,
 		}, nil
 	}
-	return ptr.Ref(fn)
 }
 
-func fakeFetchWorkspaceProxiesErr(err error) *func(context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) {
-	fn := func(context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) {
+func fakeFetchWorkspaceProxiesErr(err error) func(context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) {
+	return func(context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) {
 		return codersdk.RegionsResponse[codersdk.WorkspaceProxy]{
 			Regions: []codersdk.WorkspaceProxy{},
 		}, err
 	}
-	return ptr.Ref(fn)
 }
 
-func fakeUpdateProxyHealth(err error) *func(context.Context) error {
-	fn := func(context.Context) error {
+func fakeUpdateProxyHealth(err error) func(context.Context) error {
+	return func(context.Context) error {
 		return err
 	}
-	return ptr.Ref(fn)
 }
diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go
@@ -25,11 +25,11 @@ import (
 	"github.com/coder/coder/v2/coderd"
 	agplaudit "github.com/coder/coder/v2/coderd/audit"
 	agpldbauthz "github.com/coder/coder/v2/coderd/database/dbauthz"
+	"github.com/coder/coder/v2/coderd/healthcheck"
 	"github.com/coder/coder/v2/coderd/httpapi"
 	"github.com/coder/coder/v2/coderd/httpmw"
 	"github.com/coder/coder/v2/coderd/rbac"
 	agplschedule "github.com/coder/coder/v2/coderd/schedule"
-	"github.com/coder/coder/v2/coderd/util/ptr"
 	"github.com/coder/coder/v2/codersdk"
 	"github.com/coder/coder/v2/enterprise/coderd/dbauthz"
 	"github.com/coder/coder/v2/enterprise/coderd/license"
@@ -377,8 +377,12 @@ func New(ctx context.Context, options *Options) (_ *API, err error) {
 		api.AGPL.WorkspaceProxyHostsFn.Store(&f)
 
 		// Wire this up to healthcheck.
-		api.AGPL.FetchWorkspaceProxiesFunc.Store(ptr.Ref(api.fetchWorkspaceProxies))
-		api.AGPL.UpdateProxyHealthFunc.Store(ptr.Ref(api.ProxyHealth.ForceUpdate))
+		var fetchUpdater healthcheck.WorkspaceProxiesFetchUpdater //nolint:gosimple
+		fetchUpdater = &workspaceProxiesFetchUpdater{
+			fetchFunc:  api.fetchWorkspaceProxies,
+			updateFunc: api.ProxyHealth.ForceUpdate,
+		}
+		api.AGPL.WorkspaceProxiesFetchUpdater.Store(&fetchUpdater)
 	}
 
 	err = api.PrometheusRegistry.Register(&api.licenseMetricsCollector)

diff --git a/enterprise/coderd/workspaceproxy.go b/enterprise/coderd/workspaceproxy.go
@@ -960,3 +960,20 @@ func convertProxy(p database.WorkspaceProxy, status proxyhealth.ProxyStatus) cod
 		},
 	}
 }
+
+// workspaceProxiesFetchUpdater implements healthcheck.WorkspaceProxyFetchUpdater
+// in an actually useful and meaningful way.
+type workspaceProxiesFetchUpdater struct {
+	fetchFunc  func(context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error)
+	updateFunc func(context.Context) error
+}
+
+func (w *workspaceProxiesFetchUpdater) Fetch(ctx context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) {
+	//nolint:gocritic // Need perms to read all workspace proxies.
+	authCtx := dbauthz.AsSystemRestricted(ctx)
+	return w.fetchFunc(authCtx)
+}
+
+func (w *workspaceProxiesFetchUpdater) Update(ctx context.Context) error {
+	return w.updateFunc(ctx)
+}