From a17ec4c89666b9bc3c820f52e6a3ac6075efd39c Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 21 Nov 2023 17:33:15 +0000 Subject: [PATCH 01/20] feat(coderd/healthcheck): add health check for proxy --- coderd/healthcheck/healthcheck.go | 48 ++++++-- coderd/healthcheck/healthcheck_test.go | 55 ++++++++- coderd/healthcheck/workspaceproxy.go | 87 +++++++++++++ coderd/healthcheck/workspaceproxy_test.go | 142 ++++++++++++++++++++++ 4 files changed, 316 insertions(+), 16 deletions(-) create mode 100644 coderd/healthcheck/workspaceproxy.go create mode 100644 coderd/healthcheck/workspaceproxy_test.go diff --git a/coderd/healthcheck/healthcheck.go b/coderd/healthcheck/healthcheck.go index d5498482f977e..cc772d7718d84 100644 --- a/coderd/healthcheck/healthcheck.go +++ b/coderd/healthcheck/healthcheck.go @@ -13,10 +13,11 @@ import ( ) const ( - SectionDERP string = "DERP" - SectionAccessURL string = "AccessURL" - SectionWebsocket string = "Websocket" - SectionDatabase string = "Database" + SectionDERP string = "DERP" + SectionAccessURL string = "AccessURL" + SectionWebsocket string = "Websocket" + SectionDatabase string = "Database" + SectionWorkspaceProxy string = "WorkspaceProxy" ) type Checker interface { @@ -24,6 +25,7 @@ type Checker interface { AccessURL(ctx context.Context, opts *AccessURLReportOptions) AccessURLReport Websocket(ctx context.Context, opts *WebsocketReportOptions) WebsocketReport Database(ctx context.Context, opts *DatabaseReportOptions) DatabaseReport + WorkspaceProxy(ctx context.Context, opts *WorkspaceProxyReportOptions) WorkspaceProxyReport } // @typescript-generate Report @@ -38,20 +40,22 @@ type Report struct { // FailingSections is a list of sections that have failed their healthcheck. FailingSections []string `json:"failing_sections"` - DERP derphealth.Report `json:"derp"` - AccessURL AccessURLReport `json:"access_url"` - Websocket WebsocketReport `json:"websocket"` - Database DatabaseReport `json:"database"` + DERP derphealth.Report `json:"derp"` + AccessURL AccessURLReport `json:"access_url"` + Websocket WebsocketReport `json:"websocket"` + Database DatabaseReport `json:"database"` + WorkspaceProxy WorkspaceProxyReport `json:"workspace_proxy"` // The Coder version of the server that the report was generated on. CoderVersion string `json:"coder_version"` } type ReportOptions struct { - AccessURL AccessURLReportOptions - Database DatabaseReportOptions - DerpHealth derphealth.ReportOptions - Websocket WebsocketReportOptions + AccessURL AccessURLReportOptions + Database DatabaseReportOptions + DerpHealth derphealth.ReportOptions + Websocket WebsocketReportOptions + WorkspaceProxy WorkspaceProxyReportOptions Checker Checker } @@ -78,6 +82,11 @@ func (defaultChecker) Database(ctx context.Context, opts *DatabaseReportOptions) return report } +func (defaultChecker) WorkspaceProxy(ctx context.Context, opts *WorkspaceProxyReportOptions) (report WorkspaceProxyReport) { + report.Run(ctx, opts) + return report +} + func Run(ctx context.Context, opts *ReportOptions) *Report { var ( wg sync.WaitGroup @@ -136,6 +145,18 @@ func Run(ctx context.Context, opts *ReportOptions) *Report { report.Database = opts.Checker.Database(ctx, &opts.Database) }() + wg.Add(1) + go func() { + defer wg.Done() + defer func() { + if err := recover(); err != nil { + report.WorkspaceProxy.Error = ptr.Ref(fmt.Sprint(err)) + } + }() + + report.WorkspaceProxy = opts.Checker.WorkspaceProxy(ctx, &opts.WorkspaceProxy) + }() + report.CoderVersion = buildinfo.Version() wg.Wait() @@ -153,6 +174,9 @@ func Run(ctx context.Context, opts *ReportOptions) *Report { if !report.Database.Healthy { report.FailingSections = append(report.FailingSections, SectionDatabase) } + if !report.WorkspaceProxy.Healthy { + report.FailingSections = append(report.FailingSections, SectionWorkspaceProxy) + } report.Healthy = len(report.FailingSections) == 0 diff --git a/coderd/healthcheck/healthcheck_test.go b/coderd/healthcheck/healthcheck_test.go index 7d55366170f61..220bb772142c4 100644 --- a/coderd/healthcheck/healthcheck_test.go +++ b/coderd/healthcheck/healthcheck_test.go @@ -11,10 +11,11 @@ import ( ) type testChecker struct { - DERPReport derphealth.Report - AccessURLReport healthcheck.AccessURLReport - WebsocketReport healthcheck.WebsocketReport - DatabaseReport healthcheck.DatabaseReport + DERPReport derphealth.Report + AccessURLReport healthcheck.AccessURLReport + WebsocketReport healthcheck.WebsocketReport + DatabaseReport healthcheck.DatabaseReport + WorkspaceProxyReport healthcheck.WorkspaceProxyReport } func (c *testChecker) DERP(context.Context, *derphealth.ReportOptions) derphealth.Report { @@ -33,6 +34,10 @@ func (c *testChecker) Database(context.Context, *healthcheck.DatabaseReportOptio return c.DatabaseReport } +func (c *testChecker) WorkspaceProxy(context.Context, *healthcheck.WorkspaceProxyReportOptions) healthcheck.WorkspaceProxyReport { + return c.WorkspaceProxyReport +} + func TestHealthcheck(t *testing.T) { t.Parallel() @@ -56,6 +61,9 @@ func TestHealthcheck(t *testing.T) { DatabaseReport: healthcheck.DatabaseReport{ Healthy: true, }, + WorkspaceProxyReport: healthcheck.WorkspaceProxyReport{ + Healthy: true, + }, }, healthy: true, failingSections: []string{}, @@ -74,6 +82,9 @@ func TestHealthcheck(t *testing.T) { DatabaseReport: healthcheck.DatabaseReport{ Healthy: true, }, + WorkspaceProxyReport: healthcheck.WorkspaceProxyReport{ + Healthy: true, + }, }, healthy: false, failingSections: []string{healthcheck.SectionDERP}, @@ -93,6 +104,9 @@ func TestHealthcheck(t *testing.T) { DatabaseReport: healthcheck.DatabaseReport{ Healthy: true, }, + WorkspaceProxyReport: healthcheck.WorkspaceProxyReport{ + Healthy: true, + }, }, healthy: true, failingSections: []string{}, @@ -111,6 +125,9 @@ func TestHealthcheck(t *testing.T) { DatabaseReport: healthcheck.DatabaseReport{ Healthy: true, }, + WorkspaceProxyReport: healthcheck.WorkspaceProxyReport{ + Healthy: true, + }, }, healthy: false, failingSections: []string{healthcheck.SectionAccessURL}, @@ -129,6 +146,9 @@ func TestHealthcheck(t *testing.T) { DatabaseReport: healthcheck.DatabaseReport{ Healthy: true, }, + WorkspaceProxyReport: healthcheck.WorkspaceProxyReport{ + Healthy: true, + }, }, healthy: false, failingSections: []string{healthcheck.SectionWebsocket}, @@ -147,9 +167,33 @@ func TestHealthcheck(t *testing.T) { DatabaseReport: healthcheck.DatabaseReport{ Healthy: false, }, + WorkspaceProxyReport: healthcheck.WorkspaceProxyReport{ + Healthy: true, + }, }, healthy: false, failingSections: []string{healthcheck.SectionDatabase}, + }, { + name: "ProxyFail", + checker: &testChecker{ + DERPReport: derphealth.Report{ + Healthy: true, + }, + AccessURLReport: healthcheck.AccessURLReport{ + Healthy: true, + }, + WebsocketReport: healthcheck.WebsocketReport{ + Healthy: true, + }, + DatabaseReport: healthcheck.DatabaseReport{ + Healthy: true, + }, + WorkspaceProxyReport: healthcheck.WorkspaceProxyReport{ + Healthy: false, + }, + }, + healthy: false, + failingSections: []string{healthcheck.SectionWorkspaceProxy}, }, { name: "AllFail", checker: &testChecker{}, @@ -159,6 +203,7 @@ func TestHealthcheck(t *testing.T) { healthcheck.SectionAccessURL, healthcheck.SectionWebsocket, healthcheck.SectionDatabase, + healthcheck.SectionWorkspaceProxy, }, }} { c := c @@ -175,6 +220,8 @@ func TestHealthcheck(t *testing.T) { assert.Equal(t, c.checker.DERPReport.Warnings, report.DERP.Warnings) assert.Equal(t, c.checker.AccessURLReport.Healthy, report.AccessURL.Healthy) assert.Equal(t, c.checker.WebsocketReport.Healthy, report.Websocket.Healthy) + assert.Equal(t, c.checker.WorkspaceProxyReport.Healthy, report.WorkspaceProxy.Healthy) + assert.Equal(t, c.checker.WorkspaceProxyReport.Warnings, report.WorkspaceProxy.Warnings) assert.NotZero(t, report.Time) assert.NotZero(t, report.CoderVersion) }) diff --git a/coderd/healthcheck/workspaceproxy.go b/coderd/healthcheck/workspaceproxy.go new file mode 100644 index 0000000000000..78ed0a98f6c58 --- /dev/null +++ b/coderd/healthcheck/workspaceproxy.go @@ -0,0 +1,87 @@ +package healthcheck + +import ( + "context" + "fmt" + + "cdr.dev/slog" + "github.com/coder/coder/v2/buildinfo" + "github.com/coder/coder/v2/coderd/util/ptr" + "github.com/coder/coder/v2/codersdk" +) + +type WorkspaceProxyReportOptions struct { + // 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 + // FetchWorkspaceProxies is a function that returns the available workspace proxies. + FetchWorkspaceProxies func(context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) + // CurrentVersion is the current server version. + // We pass this in to make it easier to test. + CurrentVersion string + Logger slog.Logger +} + +// @typescript-generate Report +type WorkspaceProxyReport struct { + Healthy bool `json:"healthy"` + Warnings []string `json:"warnings"` + Error *string `json:"error"` + + WorkspaceProxies codersdk.RegionsResponse[codersdk.WorkspaceProxy] +} + +func (r *WorkspaceProxyReport) Run(ctx context.Context, opts *WorkspaceProxyReportOptions) { + r.Healthy = true + r.Warnings = []string{} + + if opts.FetchWorkspaceProxies == nil { + opts.Logger.Debug(ctx, "no workspace proxies configured") + return + } + + if opts.UpdateProxyHealth == nil { + err := "opts.UpdateProxyHealth must not be nil if opts.FetchWorkspaceProxies is not nil" + opts.Logger.Error(ctx, "developer error: "+err) + r.Error = ptr.Ref(err) + return + } + + if err := opts.UpdateProxyHealth(ctx); err != nil { + opts.Logger.Error(ctx, "failed to update proxy health: %w", err) + r.Error = ptr.Ref(err.Error()) + return + } + + proxies, err := opts.FetchWorkspaceProxies(ctx) + if err != nil { + opts.Logger.Error(ctx, "failed to fetch workspace proxies", slog.Error(err)) + r.Healthy = false + r.Error = ptr.Ref(err.Error()) + return + } + + r.WorkspaceProxies = proxies + + var numProxies int + var healthyProxies int + for _, proxy := range r.WorkspaceProxies.Regions { + numProxies++ + if proxy.Healthy { + healthyProxies++ + } + + // check versions + if !buildinfo.VersionsMatch(proxy.Version, opts.CurrentVersion) { + opts.Logger.Warn(ctx, "proxy version mismatch", + slog.F("version", opts.CurrentVersion), + slog.F("proxy_version", proxy.Version), + slog.F("proxy_name", proxy.Name), + ) + r.Healthy = false + r.Warnings = append(r.Warnings, fmt.Sprintf("Proxy %q version %q does not match primary server version %q", proxy.Name, proxy.Version, opts.CurrentVersion)) + } + } +} diff --git a/coderd/healthcheck/workspaceproxy_test.go b/coderd/healthcheck/workspaceproxy_test.go new file mode 100644 index 0000000000000..2a89aacc026e0 --- /dev/null +++ b/coderd/healthcheck/workspaceproxy_test.go @@ -0,0 +1,142 @@ +package healthcheck_test + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" + + "cdr.dev/slog/sloggers/slogtest" + + "github.com/coder/coder/v2/coderd/healthcheck" + "github.com/coder/coder/v2/codersdk" + "github.com/coder/coder/v2/testutil" +) + +func TestWorkspaceProxies(t *testing.T) { + t.Parallel() + + t.Run("NotEnabled", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitShort) + log := slogtest.Make(t, nil) + rpt := healthcheck.WorkspaceProxyReport{} + rpt.Run(ctx, &healthcheck.WorkspaceProxyReportOptions{ + Logger: log, + }) + + require.True(t, rpt.Healthy, "expected report to be healthy") + require.Empty(t, rpt.Warnings, "expected no warnings") + require.Empty(t, rpt.WorkspaceProxies, "expected no proxies") + }) + + t.Run("Enabled/None", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitShort) + log := slogtest.Make(t, nil) + rpt := healthcheck.WorkspaceProxyReport{} + rpt.Run(ctx, &healthcheck.WorkspaceProxyReportOptions{ + CurrentVersion: "v2.34.5", + FetchWorkspaceProxies: func(_ context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) { + return codersdk.RegionsResponse[codersdk.WorkspaceProxy]{ + Regions: []codersdk.WorkspaceProxy{}, + }, nil + }, + UpdateProxyHealth: func(context.Context) error { return nil }, + Logger: log, + }) + + require.True(t, rpt.Healthy, "expected report to be healthy") + require.Empty(t, rpt.Warnings, "expected no warnings") + require.NotEmpty(t, rpt.WorkspaceProxies, "expected at least one proxy") + }) + + t.Run("Enabled/Match", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitShort) + log := slogtest.Make(t, nil) + rpt := healthcheck.WorkspaceProxyReport{} + rpt.Run(ctx, &healthcheck.WorkspaceProxyReportOptions{ + CurrentVersion: "v2.34.5", + FetchWorkspaceProxies: func(_ context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) { + return codersdk.RegionsResponse[codersdk.WorkspaceProxy]{ + Regions: []codersdk.WorkspaceProxy{ + fakeWorkspaceProxy(true, "v2.34.5"), + fakeWorkspaceProxy(true, "v2.34.5"), + }, + }, nil + }, + UpdateProxyHealth: func(context.Context) error { return nil }, + Logger: log, + }) + + require.True(t, rpt.Healthy, "expected report to be healthy") + require.Empty(t, rpt.Warnings, "expected no warnings") + require.NotEmpty(t, rpt.WorkspaceProxies, "expected at least one proxy") + }) + + t.Run("Enabled/Mismatch/One", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitShort) + log := slogtest.Make(t, nil) + rpt := healthcheck.WorkspaceProxyReport{} + rpt.Run(ctx, &healthcheck.WorkspaceProxyReportOptions{ + CurrentVersion: "v2.35.0", + FetchWorkspaceProxies: func(_ context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) { + return codersdk.RegionsResponse[codersdk.WorkspaceProxy]{ + Regions: []codersdk.WorkspaceProxy{ + fakeWorkspaceProxy(true, "v2.35.0"), + fakeWorkspaceProxy(true, "v2.34.5"), + }, + }, nil + }, + Logger: log, + UpdateProxyHealth: func(context.Context) error { return nil }, + }) + + require.False(t, rpt.Healthy, "expected report not to be healthy") + require.Len(t, rpt.Warnings, 1) + require.Contains(t, rpt.Warnings[0], "does not match primary server version") + require.NotEmpty(t, rpt.WorkspaceProxies) + }) + + t.Run("Enabled/Mismatch/Multiple", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitShort) + log := slogtest.Make(t, nil) + rpt := healthcheck.WorkspaceProxyReport{} + rpt.Run(ctx, &healthcheck.WorkspaceProxyReportOptions{ + CurrentVersion: "v2.35.0", + FetchWorkspaceProxies: func(_ context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) { + return codersdk.RegionsResponse[codersdk.WorkspaceProxy]{ + Regions: []codersdk.WorkspaceProxy{ + fakeWorkspaceProxy(true, "v2.34.5"), + fakeWorkspaceProxy(true, "v2.34.5"), + }, + }, nil + }, + Logger: log, + UpdateProxyHealth: func(context.Context) error { return nil }, + }) + + require.False(t, rpt.Healthy, "expected report not to be healthy") + require.Len(t, rpt.Warnings, 2) + require.Contains(t, rpt.Warnings[0], "does not match primary server version") + require.Contains(t, rpt.Warnings[1], "does not match primary server version") + require.NotEmpty(t, rpt.WorkspaceProxies) + }) +} + +func fakeWorkspaceProxy(healthy bool, version string) codersdk.WorkspaceProxy { + return codersdk.WorkspaceProxy{ + Region: codersdk.Region{ + Healthy: healthy, + }, + Version: version, + } +} From 49b6e191dda677e230bcc37a85ea57f519903d90 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 23 Nov 2023 14:34:55 +0000 Subject: [PATCH 02/20] rm logger --- coderd/healthcheck/workspaceproxy.go | 11 ----------- coderd/healthcheck/workspaceproxy_test.go | 15 +-------------- 2 files changed, 1 insertion(+), 25 deletions(-) diff --git a/coderd/healthcheck/workspaceproxy.go b/coderd/healthcheck/workspaceproxy.go index 78ed0a98f6c58..1a7c10eb30063 100644 --- a/coderd/healthcheck/workspaceproxy.go +++ b/coderd/healthcheck/workspaceproxy.go @@ -4,7 +4,6 @@ import ( "context" "fmt" - "cdr.dev/slog" "github.com/coder/coder/v2/buildinfo" "github.com/coder/coder/v2/coderd/util/ptr" "github.com/coder/coder/v2/codersdk" @@ -21,7 +20,6 @@ type WorkspaceProxyReportOptions struct { // CurrentVersion is the current server version. // We pass this in to make it easier to test. CurrentVersion string - Logger slog.Logger } // @typescript-generate Report @@ -38,26 +36,22 @@ func (r *WorkspaceProxyReport) Run(ctx context.Context, opts *WorkspaceProxyRepo r.Warnings = []string{} if opts.FetchWorkspaceProxies == nil { - opts.Logger.Debug(ctx, "no workspace proxies configured") return } if opts.UpdateProxyHealth == nil { err := "opts.UpdateProxyHealth must not be nil if opts.FetchWorkspaceProxies is not nil" - opts.Logger.Error(ctx, "developer error: "+err) r.Error = ptr.Ref(err) return } if err := opts.UpdateProxyHealth(ctx); err != nil { - opts.Logger.Error(ctx, "failed to update proxy health: %w", err) r.Error = ptr.Ref(err.Error()) return } proxies, err := opts.FetchWorkspaceProxies(ctx) if err != nil { - opts.Logger.Error(ctx, "failed to fetch workspace proxies", slog.Error(err)) r.Healthy = false r.Error = ptr.Ref(err.Error()) return @@ -75,11 +69,6 @@ func (r *WorkspaceProxyReport) Run(ctx context.Context, opts *WorkspaceProxyRepo // check versions if !buildinfo.VersionsMatch(proxy.Version, opts.CurrentVersion) { - opts.Logger.Warn(ctx, "proxy version mismatch", - slog.F("version", opts.CurrentVersion), - slog.F("proxy_version", proxy.Version), - slog.F("proxy_name", proxy.Name), - ) r.Healthy = false r.Warnings = append(r.Warnings, fmt.Sprintf("Proxy %q version %q does not match primary server version %q", proxy.Name, proxy.Version, opts.CurrentVersion)) } diff --git a/coderd/healthcheck/workspaceproxy_test.go b/coderd/healthcheck/workspaceproxy_test.go index 2a89aacc026e0..fadbab32723db 100644 --- a/coderd/healthcheck/workspaceproxy_test.go +++ b/coderd/healthcheck/workspaceproxy_test.go @@ -6,8 +6,6 @@ import ( "github.com/stretchr/testify/require" - "cdr.dev/slog/sloggers/slogtest" - "github.com/coder/coder/v2/coderd/healthcheck" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/testutil" @@ -20,11 +18,8 @@ func TestWorkspaceProxies(t *testing.T) { t.Parallel() ctx := testutil.Context(t, testutil.WaitShort) - log := slogtest.Make(t, nil) rpt := healthcheck.WorkspaceProxyReport{} - rpt.Run(ctx, &healthcheck.WorkspaceProxyReportOptions{ - Logger: log, - }) + rpt.Run(ctx, &healthcheck.WorkspaceProxyReportOptions{}) require.True(t, rpt.Healthy, "expected report to be healthy") require.Empty(t, rpt.Warnings, "expected no warnings") @@ -35,7 +30,6 @@ func TestWorkspaceProxies(t *testing.T) { t.Parallel() ctx := testutil.Context(t, testutil.WaitShort) - log := slogtest.Make(t, nil) rpt := healthcheck.WorkspaceProxyReport{} rpt.Run(ctx, &healthcheck.WorkspaceProxyReportOptions{ CurrentVersion: "v2.34.5", @@ -45,7 +39,6 @@ func TestWorkspaceProxies(t *testing.T) { }, nil }, UpdateProxyHealth: func(context.Context) error { return nil }, - Logger: log, }) require.True(t, rpt.Healthy, "expected report to be healthy") @@ -57,7 +50,6 @@ func TestWorkspaceProxies(t *testing.T) { t.Parallel() ctx := testutil.Context(t, testutil.WaitShort) - log := slogtest.Make(t, nil) rpt := healthcheck.WorkspaceProxyReport{} rpt.Run(ctx, &healthcheck.WorkspaceProxyReportOptions{ CurrentVersion: "v2.34.5", @@ -70,7 +62,6 @@ func TestWorkspaceProxies(t *testing.T) { }, nil }, UpdateProxyHealth: func(context.Context) error { return nil }, - Logger: log, }) require.True(t, rpt.Healthy, "expected report to be healthy") @@ -82,7 +73,6 @@ func TestWorkspaceProxies(t *testing.T) { t.Parallel() ctx := testutil.Context(t, testutil.WaitShort) - log := slogtest.Make(t, nil) rpt := healthcheck.WorkspaceProxyReport{} rpt.Run(ctx, &healthcheck.WorkspaceProxyReportOptions{ CurrentVersion: "v2.35.0", @@ -94,7 +84,6 @@ func TestWorkspaceProxies(t *testing.T) { }, }, nil }, - Logger: log, UpdateProxyHealth: func(context.Context) error { return nil }, }) @@ -108,7 +97,6 @@ func TestWorkspaceProxies(t *testing.T) { t.Parallel() ctx := testutil.Context(t, testutil.WaitShort) - log := slogtest.Make(t, nil) rpt := healthcheck.WorkspaceProxyReport{} rpt.Run(ctx, &healthcheck.WorkspaceProxyReportOptions{ CurrentVersion: "v2.35.0", @@ -120,7 +108,6 @@ func TestWorkspaceProxies(t *testing.T) { }, }, nil }, - Logger: log, UpdateProxyHealth: func(context.Context) error { return nil }, }) From c7e202cd111a9a53728c478a1209fd5e349a7bdd Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 23 Nov 2023 15:11:43 +0000 Subject: [PATCH 03/20] wire it up --- coderd/coderd.go | 23 ++++++++++++++++++++++- coderd/healthcheck/workspaceproxy.go | 10 +++++----- enterprise/coderd/coderd.go | 5 +++++ 3 files changed, 32 insertions(+), 6 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index 8b54b708cc85d..00c3a0941cd52 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -135,7 +135,13 @@ type Options struct { AccessControlStore *atomic.Pointer[dbauthz.AccessControlStore] // AppSecurityKey is the crypto key used to sign and encrypt tokens related to // workspace applications. It consists of both a signing and encryption key. - AppSecurityKey workspaceapps.SecurityKey + AppSecurityKey workspaceapps.SecurityKey + + // The following two functions are dependencies of HealthcheckFunc but are only implemented + // in enterprise. Stubbing them out here. + 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 @@ -396,6 +402,15 @@ func New(options *Options) *API { *options.UpdateCheckOptions, ) } + + 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.HealthcheckFunc == nil { options.HealthcheckFunc = func(ctx context.Context, apiKey string) *healthcheck.Report { return healthcheck.Run(ctx, &healthcheck.ReportOptions{ @@ -413,9 +428,15 @@ func New(options *Options) *API { DerpHealth: derphealth.ReportOptions{ DERPMap: api.DERPMap(), }, + WorkspaceProxy: healthcheck.WorkspaceProxyReportOptions{ + CurrentVersion: buildinfo.Version(), + FetchWorkspaceProxies: *options.FetchWorkspaceProxiesFunc.Load(), + UpdateProxyHealth: *options.UpdateProxyHealthFunc.Load(), + }, }) } } + if options.HealthcheckTimeout == 0 { options.HealthcheckTimeout = 30 * time.Second } diff --git a/coderd/healthcheck/workspaceproxy.go b/coderd/healthcheck/workspaceproxy.go index 1a7c10eb30063..a5823b3187176 100644 --- a/coderd/healthcheck/workspaceproxy.go +++ b/coderd/healthcheck/workspaceproxy.go @@ -10,16 +10,16 @@ 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 - // FetchWorkspaceProxies is a function that returns the available workspace proxies. - FetchWorkspaceProxies func(context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) - // CurrentVersion is the current server version. - // We pass this in to make it easier to test. - CurrentVersion string } // @typescript-generate Report diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go index 1a34d594ce599..67e5324c9552e 100644 --- a/enterprise/coderd/coderd.go +++ b/enterprise/coderd/coderd.go @@ -29,6 +29,7 @@ import ( "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" @@ -374,6 +375,10 @@ func New(ctx context.Context, options *Options) (_ *API, err error) { // Use proxy health to return the healthy workspace proxy hostnames. f := api.ProxyHealth.ProxyHosts 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)) } err = api.PrometheusRegistry.Register(&api.licenseMetricsCollector) From 89bae7effe51ceeb40f36249cf2d606bb7518ac0 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 23 Nov 2023 18:04:10 +0000 Subject: [PATCH 04/20] add severity, convert to table-driven tests --- coderd/coderd.go | 4 +- coderd/healthcheck/healthcheck.go | 3 + coderd/healthcheck/workspaceproxy.go | 93 +++++-- coderd/healthcheck/workspaceproxy_test.go | 312 ++++++++++++++++------ 4 files changed, 305 insertions(+), 107 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index 00c3a0941cd52..2eb0619f233f8 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -430,8 +430,8 @@ func New(options *Options) *API { }, WorkspaceProxy: healthcheck.WorkspaceProxyReportOptions{ CurrentVersion: buildinfo.Version(), - FetchWorkspaceProxies: *options.FetchWorkspaceProxiesFunc.Load(), - UpdateProxyHealth: *options.UpdateProxyHealthFunc.Load(), + FetchWorkspaceProxies: options.FetchWorkspaceProxiesFunc.Load(), + UpdateProxyHealth: options.UpdateProxyHealthFunc.Load(), }, }) } diff --git a/coderd/healthcheck/healthcheck.go b/coderd/healthcheck/healthcheck.go index cc772d7718d84..9233626419634 100644 --- a/coderd/healthcheck/healthcheck.go +++ b/coderd/healthcheck/healthcheck.go @@ -195,6 +195,9 @@ func Run(ctx context.Context, opts *ReportOptions) *Report { if report.Database.Severity.Value() > report.Severity.Value() { report.Severity = report.Database.Severity } + if report.WorkspaceProxy.Severity.Value() > report.Severity.Value() { + report.Severity = report.WorkspaceProxy.Severity + } return &report } diff --git a/coderd/healthcheck/workspaceproxy.go b/coderd/healthcheck/workspaceproxy.go index a5823b3187176..5dd362f1b4faa 100644 --- a/coderd/healthcheck/workspaceproxy.go +++ b/coderd/healthcheck/workspaceproxy.go @@ -2,11 +2,15 @@ package healthcheck import ( "context" - "fmt" + + "golang.org/x/xerrors" "github.com/coder/coder/v2/buildinfo" + "github.com/coder/coder/v2/coderd/healthcheck/health" "github.com/coder/coder/v2/coderd/util/ptr" "github.com/coder/coder/v2/codersdk" + + "github.com/hashicorp/go-multierror" ) type WorkspaceProxyReportOptions struct { @@ -14,63 +18,120 @@ type WorkspaceProxyReportOptions struct { // 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) + 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 + UpdateProxyHealth *func(context.Context) error } // @typescript-generate Report type WorkspaceProxyReport struct { - Healthy bool `json:"healthy"` - Warnings []string `json:"warnings"` - Error *string `json:"error"` + Healthy bool `json:"healthy"` + Severity health.Severity `json:"severity"` + Warnings []string `json:"warnings"` + Error *string `json:"error"` WorkspaceProxies codersdk.RegionsResponse[codersdk.WorkspaceProxy] } 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 } + updateProxyHealthFunc := *opts.UpdateProxyHealth - if err := opts.UpdateProxyHealth(ctx); err != nil { - r.Error = ptr.Ref(err.Error()) + // If this fails, just mark it as a warning. It is still updated in the background. + if err := updateProxyHealthFunc(ctx); err != nil { + r.Severity = health.SeverityWarning + r.Warnings = append(r.Warnings, xerrors.Errorf("update proxy health: %w", err).Error()) return } - proxies, err := opts.FetchWorkspaceProxies(ctx) + proxies, err := fetchWorkspaceProxiesFunc(ctx) if err != nil { r.Healthy = false + r.Severity = health.SeverityError r.Error = ptr.Ref(err.Error()) return } r.WorkspaceProxies = proxies - var numProxies int - var healthyProxies int + var total, healthy int for _, proxy := range r.WorkspaceProxies.Regions { - numProxies++ + total++ if proxy.Healthy { - healthyProxies++ + healthy++ + } + + if len(proxy.Status.Report.Errors) > 0 { + for _, err := range proxy.Status.Report.Errors { + r.appendError(xerrors.New(err)) + } } + } + + r.Severity = calculateSeverity(total, healthy) + r.Healthy = r.Severity != health.SeverityError - // check versions - if !buildinfo.VersionsMatch(proxy.Version, opts.CurrentVersion) { + // Versions _must_ match. Perform this check last. This will clobber any other severity. + for _, proxy := range r.WorkspaceProxies.Regions { + if vErr := checkVersion(proxy, opts.CurrentVersion); vErr != nil { r.Healthy = false - r.Warnings = append(r.Warnings, fmt.Sprintf("Proxy %q version %q does not match primary server version %q", proxy.Name, proxy.Version, opts.CurrentVersion)) + r.Severity = health.SeverityError + r.appendError(vErr) } } } + +// appendError multierror-appends err onto r.Error. +// We only have one error, so multiple errors need to be squashed in there. +func (r *WorkspaceProxyReport) appendError(errs ...error) { + var prevErr error + if r.Error != nil { + prevErr = xerrors.New(*r.Error) + } + r.Error = ptr.Ref(multierror.Append(prevErr, errs...).Error()) +} + +func checkVersion(proxy codersdk.WorkspaceProxy, currentVersion string) error { + if proxy.Version == "" { + return nil // may have not connected yet, this is OK + } + if buildinfo.VersionsMatch(proxy.Version, currentVersion) { + return nil + } + + return xerrors.Errorf("proxy %q version %q does not match primary server version %q", + proxy.Name, + proxy.Version, + currentVersion, + ) +} + +// calculateSeverity returns: +// health.SeverityError if all proxies are unhealthy, +// health.SeverityOK if all proxies are healthy, +// health.SeverityWarning otherwise. +func calculateSeverity(total, healthy int) health.Severity { + if total == 0 || total == healthy { + return health.SeverityOK + } + if total-healthy == total { + return health.SeverityError + } + return health.SeverityWarning +} diff --git a/coderd/healthcheck/workspaceproxy_test.go b/coderd/healthcheck/workspaceproxy_test.go index fadbab32723db..cc07073ff1272 100644 --- a/coderd/healthcheck/workspaceproxy_test.go +++ b/coderd/healthcheck/workspaceproxy_test.go @@ -4,124 +4,258 @@ import ( "context" "testing" - "github.com/stretchr/testify/require" + "github.com/stretchr/testify/assert" "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" - "github.com/coder/coder/v2/testutil" ) func TestWorkspaceProxies(t *testing.T) { t.Parallel() - t.Run("NotEnabled", func(t *testing.T) { - t.Parallel() + var ( + newerPatchVersion = "v2.34.6" + currentVersion = "v2.34.5" + olderVersion = "v2.33.0" + ) - ctx := testutil.Context(t, testutil.WaitShort) - rpt := healthcheck.WorkspaceProxyReport{} - rpt.Run(ctx, &healthcheck.WorkspaceProxyReportOptions{}) - - require.True(t, rpt.Healthy, "expected report to be healthy") - require.Empty(t, rpt.Warnings, "expected no warnings") - require.Empty(t, rpt.WorkspaceProxies, "expected no proxies") - }) - - t.Run("Enabled/None", func(t *testing.T) { - t.Parallel() - - ctx := testutil.Context(t, testutil.WaitShort) - rpt := healthcheck.WorkspaceProxyReport{} - rpt.Run(ctx, &healthcheck.WorkspaceProxyReportOptions{ - CurrentVersion: "v2.34.5", - FetchWorkspaceProxies: func(_ context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) { + for _, tt := range []struct { + name string + fetchWorkspaceProxies *func(context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) + updateProxyHealth *func(context.Context) error + expectedHealthy bool + expectedError string + expectedSeverity health.Severity + }{ + { + name: "NotEnabled", + expectedHealthy: true, + expectedSeverity: health.SeverityOK, + }, + { + name: "Enabled/NoProxies", + fetchWorkspaceProxies: ptr.Ref(func(ctx context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) { return codersdk.RegionsResponse[codersdk.WorkspaceProxy]{ Regions: []codersdk.WorkspaceProxy{}, }, nil - }, - UpdateProxyHealth: func(context.Context) error { return nil }, - }) - - require.True(t, rpt.Healthy, "expected report to be healthy") - require.Empty(t, rpt.Warnings, "expected no warnings") - require.NotEmpty(t, rpt.WorkspaceProxies, "expected at least one proxy") - }) - - t.Run("Enabled/Match", func(t *testing.T) { - t.Parallel() - - ctx := testutil.Context(t, testutil.WaitShort) - rpt := healthcheck.WorkspaceProxyReport{} - rpt.Run(ctx, &healthcheck.WorkspaceProxyReportOptions{ - CurrentVersion: "v2.34.5", - FetchWorkspaceProxies: func(_ context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) { + }), + updateProxyHealth: ptr.Ref(func(ctx context.Context) error { + return nil + }), + expectedHealthy: true, + expectedSeverity: health.SeverityOK, + }, + { + name: "Enabled/OneHealthy", + fetchWorkspaceProxies: ptr.Ref(func(ctx context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) { return codersdk.RegionsResponse[codersdk.WorkspaceProxy]{ Regions: []codersdk.WorkspaceProxy{ - fakeWorkspaceProxy(true, "v2.34.5"), - fakeWorkspaceProxy(true, "v2.34.5"), + fakeWorkspaceProxy("alpha", true, currentVersion), }, }, nil - }, - UpdateProxyHealth: func(context.Context) error { return nil }, - }) - - require.True(t, rpt.Healthy, "expected report to be healthy") - require.Empty(t, rpt.Warnings, "expected no warnings") - require.NotEmpty(t, rpt.WorkspaceProxies, "expected at least one proxy") - }) - - t.Run("Enabled/Mismatch/One", func(t *testing.T) { - t.Parallel() - - ctx := testutil.Context(t, testutil.WaitShort) - rpt := healthcheck.WorkspaceProxyReport{} - rpt.Run(ctx, &healthcheck.WorkspaceProxyReportOptions{ - CurrentVersion: "v2.35.0", - FetchWorkspaceProxies: func(_ context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) { + }), + updateProxyHealth: ptr.Ref(func(ctx context.Context) error { + return nil + }), + expectedHealthy: true, + expectedSeverity: health.SeverityOK, + }, + { + name: "Enabled/OneUnhealthy", + fetchWorkspaceProxies: ptr.Ref(func(ctx context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) { return codersdk.RegionsResponse[codersdk.WorkspaceProxy]{ Regions: []codersdk.WorkspaceProxy{ - fakeWorkspaceProxy(true, "v2.35.0"), - fakeWorkspaceProxy(true, "v2.34.5"), + fakeWorkspaceProxy("alpha", false, currentVersion), }, }, nil - }, - UpdateProxyHealth: func(context.Context) error { return nil }, - }) - - require.False(t, rpt.Healthy, "expected report not to be healthy") - require.Len(t, rpt.Warnings, 1) - require.Contains(t, rpt.Warnings[0], "does not match primary server version") - require.NotEmpty(t, rpt.WorkspaceProxies) - }) - - t.Run("Enabled/Mismatch/Multiple", func(t *testing.T) { - t.Parallel() - - ctx := testutil.Context(t, testutil.WaitShort) - rpt := healthcheck.WorkspaceProxyReport{} - rpt.Run(ctx, &healthcheck.WorkspaceProxyReportOptions{ - CurrentVersion: "v2.35.0", - FetchWorkspaceProxies: func(_ context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) { + }), + updateProxyHealth: ptr.Ref(func(ctx context.Context) error { + return nil + }), + expectedHealthy: false, + expectedSeverity: health.SeverityError, + }, + { + name: "Enabled/OneUnreachable", + fetchWorkspaceProxies: ptr.Ref(func(ctx context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) { return codersdk.RegionsResponse[codersdk.WorkspaceProxy]{ Regions: []codersdk.WorkspaceProxy{ - fakeWorkspaceProxy(true, "v2.34.5"), - fakeWorkspaceProxy(true, "v2.34.5"), + { + Region: codersdk.Region{ + Name: "gone", + Healthy: false, + }, + Version: currentVersion, + Status: codersdk.WorkspaceProxyStatus{ + Status: codersdk.ProxyUnreachable, + Report: codersdk.ProxyHealthReport{ + Errors: []string{ + "request to proxy failed: Get \"http://127.0.0.1:3001/healthz-report\": dial tcp 127.0.0.1:3001: connect: connection refused", + }, + }, + }, + }, }, }, nil - }, - UpdateProxyHealth: func(context.Context) error { return nil }, - }) + }), + updateProxyHealth: ptr.Ref(func(ctx context.Context) error { + return nil + }), + expectedHealthy: false, + expectedSeverity: health.SeverityError, + expectedError: "connect: connection refused", + }, + { + name: "Enabled/AllHealthy", + fetchWorkspaceProxies: ptr.Ref(func(ctx context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) { + return codersdk.RegionsResponse[codersdk.WorkspaceProxy]{ + Regions: []codersdk.WorkspaceProxy{ + fakeWorkspaceProxy("alpha", true, currentVersion), + fakeWorkspaceProxy("beta", true, currentVersion), + }, + }, nil + }), + updateProxyHealth: ptr.Ref(func(ctx context.Context) error { + return nil + }), + expectedHealthy: true, + expectedSeverity: health.SeverityOK, + }, + { + name: "Enabled/OneHealthyOneUnhealthy", + fetchWorkspaceProxies: ptr.Ref(func(ctx context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) { + return codersdk.RegionsResponse[codersdk.WorkspaceProxy]{ + Regions: []codersdk.WorkspaceProxy{ + fakeWorkspaceProxy("alpha", true, currentVersion), + fakeWorkspaceProxy("beta", false, currentVersion), + }, + }, nil + }), + updateProxyHealth: ptr.Ref(func(ctx context.Context) error { + return nil + }), + expectedHealthy: true, + expectedSeverity: health.SeverityWarning, + }, + { + name: "Enabled/AllUnhealthy", + fetchWorkspaceProxies: ptr.Ref(func(ctx context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) { + return codersdk.RegionsResponse[codersdk.WorkspaceProxy]{ + Regions: []codersdk.WorkspaceProxy{ + fakeWorkspaceProxy("alpha", false, currentVersion), + fakeWorkspaceProxy("beta", false, currentVersion), + }, + }, nil + }), + updateProxyHealth: ptr.Ref(func(ctx context.Context) error { + return nil + }), + expectedHealthy: false, + expectedSeverity: health.SeverityError, + }, + { + name: "Enabled/OneOutOfDate", + fetchWorkspaceProxies: ptr.Ref(func(ctx context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) { + return codersdk.RegionsResponse[codersdk.WorkspaceProxy]{ + Regions: []codersdk.WorkspaceProxy{ + fakeWorkspaceProxy("alpha", true, currentVersion), + fakeWorkspaceProxy("beta", true, olderVersion), + }, + }, nil + }), + updateProxyHealth: ptr.Ref(func(ctx context.Context) error { + return nil + }), + expectedHealthy: false, + expectedSeverity: health.SeverityError, + expectedError: `proxy "beta" version "v2.33.0" does not match primary server version "v2.34.5"`, + }, + { + name: "Enabled/OneSlightlyNewerButStillOK", + fetchWorkspaceProxies: ptr.Ref(func(ctx context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) { + return codersdk.RegionsResponse[codersdk.WorkspaceProxy]{ + Regions: []codersdk.WorkspaceProxy{ + fakeWorkspaceProxy("alpha", true, currentVersion), + fakeWorkspaceProxy("beta", true, newerPatchVersion), + }, + }, nil + }), + updateProxyHealth: ptr.Ref(func(ctx context.Context) error { + return nil + }), + expectedHealthy: true, + expectedSeverity: health.SeverityOK, + }, + { + name: "Enabled/NotConnectedYet", + fetchWorkspaceProxies: ptr.Ref(func(ctx context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) { + return codersdk.RegionsResponse[codersdk.WorkspaceProxy]{ + Regions: []codersdk.WorkspaceProxy{ + fakeWorkspaceProxy("slowpoke", true, ""), + }, + }, nil + }), + updateProxyHealth: ptr.Ref(func(ctx context.Context) error { + return nil + }), + expectedHealthy: true, + expectedSeverity: health.SeverityOK, + }, + { + name: "Enabled/ErrFetchWorkspaceProxy", + fetchWorkspaceProxies: ptr.Ref(func(ctx context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) { + return codersdk.RegionsResponse[codersdk.WorkspaceProxy]{}, assert.AnError + }), + updateProxyHealth: ptr.Ref(func(ctx context.Context) error { + return nil + }), + expectedHealthy: false, + expectedSeverity: health.SeverityError, + expectedError: assert.AnError.Error(), + }, + { + name: "Enabled/ErrUpdateProxyHealth", + fetchWorkspaceProxies: ptr.Ref(func(ctx context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) { + return codersdk.RegionsResponse[codersdk.WorkspaceProxy]{}, nil + }), + updateProxyHealth: ptr.Ref(func(ctx context.Context) error { + return assert.AnError + }), + expectedHealthy: true, + expectedSeverity: health.SeverityWarning, + }, + } { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + var rpt healthcheck.WorkspaceProxyReport + var opts healthcheck.WorkspaceProxyReportOptions + opts.CurrentVersion = currentVersion + opts.FetchWorkspaceProxies = tt.fetchWorkspaceProxies + opts.UpdateProxyHealth = tt.updateProxyHealth - require.False(t, rpt.Healthy, "expected report not to be healthy") - require.Len(t, rpt.Warnings, 2) - require.Contains(t, rpt.Warnings[0], "does not match primary server version") - require.Contains(t, rpt.Warnings[1], "does not match primary server version") - require.NotEmpty(t, rpt.WorkspaceProxies) - }) + rpt.Run(context.Background(), &opts) + + assert.Equal(t, tt.expectedHealthy, rpt.Healthy) + assert.Equal(t, tt.expectedSeverity, rpt.Severity) + if tt.expectedError != "" { + assert.NotNil(t, rpt.Error) + assert.Contains(t, *rpt.Error, tt.expectedError) + } else { + if !assert.Nil(t, rpt.Error) { + assert.Empty(t, *rpt.Error) + } + } + }) + } } -func fakeWorkspaceProxy(healthy bool, version string) codersdk.WorkspaceProxy { +func fakeWorkspaceProxy(name string, healthy bool, version string) codersdk.WorkspaceProxy { return codersdk.WorkspaceProxy{ Region: codersdk.Region{ + Name: name, Healthy: healthy, }, Version: version, From bcdc08c209118760ee359c0658e2bf1939a71b0a Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 23 Nov 2023 18:16:07 +0000 Subject: [PATCH 05/20] dbauthz --- coderd/coderd.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index 2eb0619f233f8..834480ade0257 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -413,7 +413,8 @@ func New(options *Options) *API { if options.HealthcheckFunc == nil { options.HealthcheckFunc = func(ctx context.Context, apiKey string) *healthcheck.Report { - return healthcheck.Run(ctx, &healthcheck.ReportOptions{ + authCtx := dbauthz.AsSystemRestricted(ctx) //nolint:gocritic // internal function + return healthcheck.Run(authCtx, &healthcheck.ReportOptions{ Database: healthcheck.DatabaseReportOptions{ DB: options.Database, Threshold: options.DeploymentValues.Healthcheck.ThresholdDatabase.Value(), From 340a2766a7809b61780c565dce37f727d7d94a17 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 23 Nov 2023 18:18:14 +0000 Subject: [PATCH 06/20] make gen; make fmt --- coderd/apidoc/docs.go | 26 ++++++++++ coderd/apidoc/swagger.json | 26 ++++++++++ docs/api/debug.md | 33 ++++++++++++ docs/api/schemas.md | 104 +++++++++++++++++++++++++++++++++---- 4 files changed, 178 insertions(+), 11 deletions(-) diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index ed7968577b455..29767cb94bcef 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -12388,6 +12388,9 @@ const docTemplate = `{ }, "websocket": { "$ref": "#/definitions/healthcheck.WebsocketReport" + }, + "workspace_proxy": { + "$ref": "#/definitions/healthcheck.WorkspaceProxyReport" } } }, @@ -12427,6 +12430,29 @@ const docTemplate = `{ } } }, + "healthcheck.WorkspaceProxyReport": { + "type": "object", + "properties": { + "error": { + "type": "string" + }, + "healthy": { + "type": "boolean" + }, + "severity": { + "$ref": "#/definitions/health.Severity" + }, + "warnings": { + "type": "array", + "items": { + "type": "string" + } + }, + "workspaceProxies": { + "$ref": "#/definitions/codersdk.RegionsResponse-codersdk_WorkspaceProxy" + } + } + }, "netcheck.Report": { "type": "object", "properties": { diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index f92e11b92609f..b4025ec494ae3 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -11277,6 +11277,9 @@ }, "websocket": { "$ref": "#/definitions/healthcheck.WebsocketReport" + }, + "workspace_proxy": { + "$ref": "#/definitions/healthcheck.WorkspaceProxyReport" } } }, @@ -11312,6 +11315,29 @@ } } }, + "healthcheck.WorkspaceProxyReport": { + "type": "object", + "properties": { + "error": { + "type": "string" + }, + "healthy": { + "type": "boolean" + }, + "severity": { + "$ref": "#/definitions/health.Severity" + }, + "warnings": { + "type": "array", + "items": { + "type": "string" + } + }, + "workspaceProxies": { + "$ref": "#/definitions/codersdk.RegionsResponse-codersdk_WorkspaceProxy" + } + } + }, "netcheck.Report": { "type": "object", "properties": { diff --git a/docs/api/debug.md b/docs/api/debug.md index c2a26321716d3..37e0a821fd41d 100644 --- a/docs/api/debug.md +++ b/docs/api/debug.md @@ -253,6 +253,39 @@ curl -X GET http://coder-server:8080/api/v2/debug/health \ "healthy": true, "severity": "ok", "warnings": ["string"] + }, + "workspace_proxy": { + "error": "string", + "healthy": true, + "severity": "ok", + "warnings": ["string"], + "workspaceProxies": { + "regions": [ + { + "created_at": "2019-08-24T14:15:22Z", + "deleted": true, + "derp_enabled": true, + "derp_only": true, + "display_name": "string", + "healthy": true, + "icon_url": "string", + "id": "497f6eca-6276-4993-bfeb-53cbbbba6f08", + "name": "string", + "path_app_url": "string", + "status": { + "checked_at": "2019-08-24T14:15:22Z", + "report": { + "errors": ["string"], + "warnings": ["string"] + }, + "status": "ok" + }, + "updated_at": "2019-08-24T14:15:22Z", + "version": "string", + "wildcard_hostname": "string" + } + ] + } } } ``` diff --git a/docs/api/schemas.md b/docs/api/schemas.md index c4dc42883987d..5100a4dc20e06 100644 --- a/docs/api/schemas.md +++ b/docs/api/schemas.md @@ -7789,23 +7789,57 @@ If the schedule is empty, the user will be updated to use the default schedule.| "healthy": true, "severity": "ok", "warnings": ["string"] + }, + "workspace_proxy": { + "error": "string", + "healthy": true, + "severity": "ok", + "warnings": ["string"], + "workspaceProxies": { + "regions": [ + { + "created_at": "2019-08-24T14:15:22Z", + "deleted": true, + "derp_enabled": true, + "derp_only": true, + "display_name": "string", + "healthy": true, + "icon_url": "string", + "id": "497f6eca-6276-4993-bfeb-53cbbbba6f08", + "name": "string", + "path_app_url": "string", + "status": { + "checked_at": "2019-08-24T14:15:22Z", + "report": { + "errors": ["string"], + "warnings": ["string"] + }, + "status": "ok" + }, + "updated_at": "2019-08-24T14:15:22Z", + "version": "string", + "wildcard_hostname": "string" + } + ] + } } } ``` ### Properties -| Name | Type | Required | Restrictions | Description | -| ------------------ | ---------------------------------------------------------- | -------- | ------------ | ----------------------------------------------------------------------------------- | -| `access_url` | [healthcheck.AccessURLReport](#healthcheckaccessurlreport) | false | | | -| `coder_version` | string | false | | The Coder version of the server that the report was generated on. | -| `database` | [healthcheck.DatabaseReport](#healthcheckdatabasereport) | false | | | -| `derp` | [derphealth.Report](#derphealthreport) | false | | | -| `failing_sections` | array of string | false | | Failing sections is a list of sections that have failed their healthcheck. | -| `healthy` | boolean | false | | Healthy is true if the report returns no errors. Deprecated: use `Severity` instead | -| `severity` | [health.Severity](#healthseverity) | false | | Severity indicates the status of Coder health. | -| `time` | string | false | | Time is the time the report was generated at. | -| `websocket` | [healthcheck.WebsocketReport](#healthcheckwebsocketreport) | false | | | +| Name | Type | Required | Restrictions | Description | +| ------------------ | -------------------------------------------------------------------- | -------- | ------------ | ----------------------------------------------------------------------------------- | +| `access_url` | [healthcheck.AccessURLReport](#healthcheckaccessurlreport) | false | | | +| `coder_version` | string | false | | The Coder version of the server that the report was generated on. | +| `database` | [healthcheck.DatabaseReport](#healthcheckdatabasereport) | false | | | +| `derp` | [derphealth.Report](#derphealthreport) | false | | | +| `failing_sections` | array of string | false | | Failing sections is a list of sections that have failed their healthcheck. | +| `healthy` | boolean | false | | Healthy is true if the report returns no errors. Deprecated: use `Severity` instead | +| `severity` | [health.Severity](#healthseverity) | false | | Severity indicates the status of Coder health. | +| `time` | string | false | | Time is the time the report was generated at. | +| `websocket` | [healthcheck.WebsocketReport](#healthcheckwebsocketreport) | false | | | +| `workspace_proxy` | [healthcheck.WorkspaceProxyReport](#healthcheckworkspaceproxyreport) | false | | | #### Enumerated Values @@ -7847,6 +7881,54 @@ If the schedule is empty, the user will be updated to use the default schedule.| | `severity` | `warning` | | `severity` | `error` | +## healthcheck.WorkspaceProxyReport + +```json +{ + "error": "string", + "healthy": true, + "severity": "ok", + "warnings": ["string"], + "workspaceProxies": { + "regions": [ + { + "created_at": "2019-08-24T14:15:22Z", + "deleted": true, + "derp_enabled": true, + "derp_only": true, + "display_name": "string", + "healthy": true, + "icon_url": "string", + "id": "497f6eca-6276-4993-bfeb-53cbbbba6f08", + "name": "string", + "path_app_url": "string", + "status": { + "checked_at": "2019-08-24T14:15:22Z", + "report": { + "errors": ["string"], + "warnings": ["string"] + }, + "status": "ok" + }, + "updated_at": "2019-08-24T14:15:22Z", + "version": "string", + "wildcard_hostname": "string" + } + ] + } +} +``` + +### Properties + +| Name | Type | Required | Restrictions | Description | +| ------------------ | ---------------------------------------------------------------------------------------------------- | -------- | ------------ | ----------- | +| `error` | string | false | | | +| `healthy` | boolean | false | | | +| `severity` | [health.Severity](#healthseverity) | false | | | +| `warnings` | array of string | false | | | +| `workspaceProxies` | [codersdk.RegionsResponse-codersdk_WorkspaceProxy](#codersdkregionsresponse-codersdk_workspaceproxy) | false | | | + ## netcheck.Report ```json From c74602169df6621d932b5df31542883c7ff99fe3 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 24 Nov 2023 09:51:02 +0000 Subject: [PATCH 07/20] fix linter import shadowing complaint --- enterprise/coderd/coderd.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go index 67e5324c9552e..ca890ae2f179c 100644 --- a/enterprise/coderd/coderd.go +++ b/enterprise/coderd/coderd.go @@ -557,8 +557,8 @@ func (api *API) updateEntitlements(ctx context.Context) error { Log: api.Logger.Named("quota_committer"), Database: api.Database, } - ptr := proto.QuotaCommitter(&committer) - api.AGPL.QuotaCommitter.Store(&ptr) + qcPtr := proto.QuotaCommitter(&committer) + api.AGPL.QuotaCommitter.Store(&qcPtr) } else { api.AGPL.QuotaCommitter.Store(nil) } From 05d4b09dc08f1fe78354649374a628ed92f31cfc Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 24 Nov 2023 10:13:54 +0000 Subject: [PATCH 08/20] fix generated report name --- coderd/healthcheck/workspaceproxy.go | 2 +- site/src/api/typesGenerated.ts | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/coderd/healthcheck/workspaceproxy.go b/coderd/healthcheck/workspaceproxy.go index 5dd362f1b4faa..0d9f1941be52a 100644 --- a/coderd/healthcheck/workspaceproxy.go +++ b/coderd/healthcheck/workspaceproxy.go @@ -26,7 +26,7 @@ type WorkspaceProxyReportOptions struct { UpdateProxyHealth *func(context.Context) error } -// @typescript-generate Report +// @typescript-generate WorkspaceProxyReport type WorkspaceProxyReport struct { Healthy bool `json:"healthy"` Severity health.Severity `json:"severity"` diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index ccf06da3f8b47..cb1e2211be1a8 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -2115,6 +2115,7 @@ export interface HealthcheckReport { readonly access_url: HealthcheckAccessURLReport; readonly websocket: HealthcheckWebsocketReport; readonly database: HealthcheckDatabaseReport; + readonly workspace_proxy: HealthcheckWorkspaceProxyReport; readonly coder_version: string; } From 56be002a6f81696c6f244fdd649ada9fa15511cb Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 24 Nov 2023 10:47:18 +0000 Subject: [PATCH 09/20] gen harder --- coderd/healthcheck/workspaceproxy.go | 5 +++++ site/src/api/typesGenerated.ts | 11 +++++++++++ 2 files changed, 16 insertions(+) diff --git a/coderd/healthcheck/workspaceproxy.go b/coderd/healthcheck/workspaceproxy.go index 0d9f1941be52a..d7f53a2805613 100644 --- a/coderd/healthcheck/workspaceproxy.go +++ b/coderd/healthcheck/workspaceproxy.go @@ -2,6 +2,7 @@ package healthcheck import ( "context" + "sort" "golang.org/x/xerrors" @@ -69,6 +70,10 @@ func (r *WorkspaceProxyReport) Run(ctx context.Context, opts *WorkspaceProxyRepo } r.WorkspaceProxies = proxies + // Stable sort based on create timestamp. + sort.Slice(r.WorkspaceProxies.Regions, func(i int, j int) bool { + return r.WorkspaceProxies.Regions[i].CreatedAt.Before(r.WorkspaceProxies.Regions[j].CreatedAt) + }) var total, healthy int for _, proxy := range r.WorkspaceProxies.Regions { diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index cb1e2211be1a8..317a7f77e65c6 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -2129,6 +2129,17 @@ export interface HealthcheckWebsocketReport { readonly error?: string; } +// From healthcheck/workspaceproxy.go +export interface HealthcheckWorkspaceProxyReport { + readonly healthy: boolean; + readonly severity: HealthSeverity; + readonly warnings: string[]; + readonly error?: string; + // Named type "github.com/coder/coder/v2/codersdk.RegionsResponse[github.com/coder/coder/v2/codersdk.WorkspaceProxy]" unknown, using "any" + // eslint-disable-next-line @typescript-eslint/no-explicit-any -- External type + readonly WorkspaceProxies: any; +} + // The code below is generated from cli/clibase. // From clibase/clibase.go From 1112b1c275b1346614a673d872d03bebe24305bc Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 24 Nov 2023 11:23:44 +0000 Subject: [PATCH 10/20] address comments --- coderd/coderd.go | 4 +- coderd/healthcheck/workspaceproxy_test.go | 229 +++++++++------------- 2 files changed, 96 insertions(+), 137 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index 834480ade0257..e941c11785a70 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -137,8 +137,6 @@ type Options struct { // workspace applications. It consists of both a signing and encryption key. AppSecurityKey workspaceapps.SecurityKey - // The following two functions are dependencies of HealthcheckFunc but are only implemented - // in enterprise. Stubbing them out here. FetchWorkspaceProxiesFunc *atomic.Pointer[func(context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error)] UpdateProxyHealthFunc *atomic.Pointer[func(context.Context) error] @@ -403,6 +401,8 @@ 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)]{} } diff --git a/coderd/healthcheck/workspaceproxy_test.go b/coderd/healthcheck/workspaceproxy_test.go index cc07073ff1272..c0ff0fd64743c 100644 --- a/coderd/healthcheck/workspaceproxy_test.go +++ b/coderd/healthcheck/workspaceproxy_test.go @@ -35,47 +35,25 @@ func TestWorkspaceProxies(t *testing.T) { expectedSeverity: health.SeverityOK, }, { - name: "Enabled/NoProxies", - fetchWorkspaceProxies: ptr.Ref(func(ctx context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) { - return codersdk.RegionsResponse[codersdk.WorkspaceProxy]{ - Regions: []codersdk.WorkspaceProxy{}, - }, nil - }), - updateProxyHealth: ptr.Ref(func(ctx context.Context) error { - return nil - }), - expectedHealthy: true, - expectedSeverity: health.SeverityOK, + name: "Enabled/NoProxies", + fetchWorkspaceProxies: fakeFetchWorkspaceProxies(), + updateProxyHealth: fakeUpdateProxyHealth(nil), + expectedHealthy: true, + expectedSeverity: health.SeverityOK, }, { - name: "Enabled/OneHealthy", - fetchWorkspaceProxies: ptr.Ref(func(ctx context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) { - return codersdk.RegionsResponse[codersdk.WorkspaceProxy]{ - Regions: []codersdk.WorkspaceProxy{ - fakeWorkspaceProxy("alpha", true, currentVersion), - }, - }, nil - }), - updateProxyHealth: ptr.Ref(func(ctx context.Context) error { - return nil - }), - expectedHealthy: true, - expectedSeverity: health.SeverityOK, + name: "Enabled/OneHealthy", + fetchWorkspaceProxies: fakeFetchWorkspaceProxies(fakeWorkspaceProxy("alpha", true, currentVersion)), + updateProxyHealth: fakeUpdateProxyHealth(nil), + expectedHealthy: true, + expectedSeverity: health.SeverityOK, }, { - name: "Enabled/OneUnhealthy", - fetchWorkspaceProxies: ptr.Ref(func(ctx context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) { - return codersdk.RegionsResponse[codersdk.WorkspaceProxy]{ - Regions: []codersdk.WorkspaceProxy{ - fakeWorkspaceProxy("alpha", false, currentVersion), - }, - }, nil - }), - updateProxyHealth: ptr.Ref(func(ctx context.Context) error { - return nil - }), - expectedHealthy: false, - expectedSeverity: health.SeverityError, + name: "Enabled/OneUnhealthy", + fetchWorkspaceProxies: fakeFetchWorkspaceProxies(fakeWorkspaceProxy("alpha", false, currentVersion)), + updateProxyHealth: fakeUpdateProxyHealth(nil), + expectedHealthy: false, + expectedSeverity: health.SeverityError, }, { name: "Enabled/OneUnreachable", @@ -100,23 +78,17 @@ func TestWorkspaceProxies(t *testing.T) { }, }, nil }), - updateProxyHealth: ptr.Ref(func(ctx context.Context) error { - return nil - }), - expectedHealthy: false, - expectedSeverity: health.SeverityError, - expectedError: "connect: connection refused", + updateProxyHealth: fakeUpdateProxyHealth(nil), + expectedHealthy: false, + expectedSeverity: health.SeverityError, + expectedError: "connect: connection refused", }, { name: "Enabled/AllHealthy", - fetchWorkspaceProxies: ptr.Ref(func(ctx context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) { - return codersdk.RegionsResponse[codersdk.WorkspaceProxy]{ - Regions: []codersdk.WorkspaceProxy{ - fakeWorkspaceProxy("alpha", true, currentVersion), - fakeWorkspaceProxy("beta", true, currentVersion), - }, - }, nil - }), + fetchWorkspaceProxies: fakeFetchWorkspaceProxies( + fakeWorkspaceProxy("alpha", true, currentVersion), + fakeWorkspaceProxy("beta", true, currentVersion), + ), updateProxyHealth: ptr.Ref(func(ctx context.Context) error { return nil }), @@ -125,106 +97,68 @@ func TestWorkspaceProxies(t *testing.T) { }, { name: "Enabled/OneHealthyOneUnhealthy", - fetchWorkspaceProxies: ptr.Ref(func(ctx context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) { - return codersdk.RegionsResponse[codersdk.WorkspaceProxy]{ - Regions: []codersdk.WorkspaceProxy{ - fakeWorkspaceProxy("alpha", true, currentVersion), - fakeWorkspaceProxy("beta", false, currentVersion), - }, - }, nil - }), - updateProxyHealth: ptr.Ref(func(ctx context.Context) error { - return nil - }), - expectedHealthy: true, - expectedSeverity: health.SeverityWarning, + fetchWorkspaceProxies: fakeFetchWorkspaceProxies( + fakeWorkspaceProxy("alpha", false, currentVersion), + fakeWorkspaceProxy("beta", true, currentVersion), + ), + updateProxyHealth: fakeUpdateProxyHealth(nil), + expectedHealthy: true, + expectedSeverity: health.SeverityWarning, }, { name: "Enabled/AllUnhealthy", - fetchWorkspaceProxies: ptr.Ref(func(ctx context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) { - return codersdk.RegionsResponse[codersdk.WorkspaceProxy]{ - Regions: []codersdk.WorkspaceProxy{ - fakeWorkspaceProxy("alpha", false, currentVersion), - fakeWorkspaceProxy("beta", false, currentVersion), - }, - }, nil - }), - updateProxyHealth: ptr.Ref(func(ctx context.Context) error { - return nil - }), - expectedHealthy: false, - expectedSeverity: health.SeverityError, + fetchWorkspaceProxies: fakeFetchWorkspaceProxies( + fakeWorkspaceProxy("alpha", false, currentVersion), + fakeWorkspaceProxy("beta", false, currentVersion), + ), + updateProxyHealth: fakeUpdateProxyHealth(nil), + expectedHealthy: false, + expectedSeverity: health.SeverityError, }, { name: "Enabled/OneOutOfDate", - fetchWorkspaceProxies: ptr.Ref(func(ctx context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) { - return codersdk.RegionsResponse[codersdk.WorkspaceProxy]{ - Regions: []codersdk.WorkspaceProxy{ - fakeWorkspaceProxy("alpha", true, currentVersion), - fakeWorkspaceProxy("beta", true, olderVersion), - }, - }, nil - }), - updateProxyHealth: ptr.Ref(func(ctx context.Context) error { - return nil - }), - expectedHealthy: false, - expectedSeverity: health.SeverityError, - expectedError: `proxy "beta" version "v2.33.0" does not match primary server version "v2.34.5"`, + fetchWorkspaceProxies: fakeFetchWorkspaceProxies( + fakeWorkspaceProxy("alpha", true, currentVersion), + fakeWorkspaceProxy("beta", true, olderVersion), + ), + updateProxyHealth: fakeUpdateProxyHealth(nil), + expectedHealthy: false, + expectedSeverity: health.SeverityError, + expectedError: `proxy "beta" version "v2.33.0" does not match primary server version "v2.34.5"`, }, { name: "Enabled/OneSlightlyNewerButStillOK", - fetchWorkspaceProxies: ptr.Ref(func(ctx context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) { - return codersdk.RegionsResponse[codersdk.WorkspaceProxy]{ - Regions: []codersdk.WorkspaceProxy{ - fakeWorkspaceProxy("alpha", true, currentVersion), - fakeWorkspaceProxy("beta", true, newerPatchVersion), - }, - }, nil - }), - updateProxyHealth: ptr.Ref(func(ctx context.Context) error { - return nil - }), - expectedHealthy: true, - expectedSeverity: health.SeverityOK, + fetchWorkspaceProxies: fakeFetchWorkspaceProxies( + fakeWorkspaceProxy("alpha", true, currentVersion), + fakeWorkspaceProxy("beta", true, newerPatchVersion), + ), + updateProxyHealth: fakeUpdateProxyHealth(nil), + expectedHealthy: true, + expectedSeverity: health.SeverityOK, }, { name: "Enabled/NotConnectedYet", - fetchWorkspaceProxies: ptr.Ref(func(ctx context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) { - return codersdk.RegionsResponse[codersdk.WorkspaceProxy]{ - Regions: []codersdk.WorkspaceProxy{ - fakeWorkspaceProxy("slowpoke", true, ""), - }, - }, nil - }), - updateProxyHealth: ptr.Ref(func(ctx context.Context) error { - return nil - }), - expectedHealthy: true, - expectedSeverity: health.SeverityOK, + fetchWorkspaceProxies: fakeFetchWorkspaceProxies( + fakeWorkspaceProxy("slowpoke", true, ""), + ), + updateProxyHealth: fakeUpdateProxyHealth(nil), + expectedHealthy: true, + expectedSeverity: health.SeverityOK, }, { - name: "Enabled/ErrFetchWorkspaceProxy", - fetchWorkspaceProxies: ptr.Ref(func(ctx context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) { - return codersdk.RegionsResponse[codersdk.WorkspaceProxy]{}, assert.AnError - }), - updateProxyHealth: ptr.Ref(func(ctx context.Context) error { - return nil - }), - expectedHealthy: false, - expectedSeverity: health.SeverityError, - expectedError: assert.AnError.Error(), + name: "Enabled/ErrFetchWorkspaceProxy", + fetchWorkspaceProxies: fakeFetchWorkspaceProxiesErr(assert.AnError), + updateProxyHealth: fakeUpdateProxyHealth(nil), + expectedHealthy: false, + expectedSeverity: health.SeverityError, + expectedError: assert.AnError.Error(), }, { - name: "Enabled/ErrUpdateProxyHealth", - fetchWorkspaceProxies: ptr.Ref(func(ctx context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) { - return codersdk.RegionsResponse[codersdk.WorkspaceProxy]{}, nil - }), - updateProxyHealth: ptr.Ref(func(ctx context.Context) error { - return assert.AnError - }), - expectedHealthy: true, - expectedSeverity: health.SeverityWarning, + name: "Enabled/ErrUpdateProxyHealth", + fetchWorkspaceProxies: fakeFetchWorkspaceProxies(fakeWorkspaceProxy("alpha", true, currentVersion)), + updateProxyHealth: fakeUpdateProxyHealth(assert.AnError), + expectedHealthy: true, + expectedSeverity: health.SeverityWarning, }, } { tt := tt @@ -261,3 +195,28 @@ func fakeWorkspaceProxy(name string, healthy bool, version string) codersdk.Work Version: version, } } + +func fakeFetchWorkspaceProxies(ps ...codersdk.WorkspaceProxy) *func(context.Context) (codersdk.RegionsResponse[codersdk.WorkspaceProxy], error) { + fn := 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) { + 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 { + return err + } + return ptr.Ref(fn) +} From 7f8760a06a1d0dbf159f304f88503593ccb32736 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 24 Nov 2023 12:29:47 +0000 Subject: [PATCH 11/20] rename WorkspaceProxies -> workspace_proxies --- coderd/apidoc/docs.go | 2 +- coderd/apidoc/swagger.json | 2 +- coderd/healthcheck/workspaceproxy.go | 2 +- docs/api/debug.md | 2 +- docs/api/schemas.md | 18 +++++++-------- site/src/api/typesGenerated.ts | 2 +- site/src/testHelpers/entities.ts | 33 ++++++++++++++++++++++++++++ 7 files changed, 47 insertions(+), 14 deletions(-) diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index 29767cb94bcef..734dac12f99c8 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -12448,7 +12448,7 @@ const docTemplate = `{ "type": "string" } }, - "workspaceProxies": { + "workspace_proxies": { "$ref": "#/definitions/codersdk.RegionsResponse-codersdk_WorkspaceProxy" } } diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index b4025ec494ae3..a9dd58f9c7a3e 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -11333,7 +11333,7 @@ "type": "string" } }, - "workspaceProxies": { + "workspace_proxies": { "$ref": "#/definitions/codersdk.RegionsResponse-codersdk_WorkspaceProxy" } } diff --git a/coderd/healthcheck/workspaceproxy.go b/coderd/healthcheck/workspaceproxy.go index d7f53a2805613..ffaa9fb48e72d 100644 --- a/coderd/healthcheck/workspaceproxy.go +++ b/coderd/healthcheck/workspaceproxy.go @@ -34,7 +34,7 @@ type WorkspaceProxyReport struct { Warnings []string `json:"warnings"` Error *string `json:"error"` - WorkspaceProxies codersdk.RegionsResponse[codersdk.WorkspaceProxy] + WorkspaceProxies codersdk.RegionsResponse[codersdk.WorkspaceProxy] `json:"workspace_proxies"` } func (r *WorkspaceProxyReport) Run(ctx context.Context, opts *WorkspaceProxyReportOptions) { diff --git a/docs/api/debug.md b/docs/api/debug.md index 37e0a821fd41d..60576c1c0ac62 100644 --- a/docs/api/debug.md +++ b/docs/api/debug.md @@ -259,7 +259,7 @@ curl -X GET http://coder-server:8080/api/v2/debug/health \ "healthy": true, "severity": "ok", "warnings": ["string"], - "workspaceProxies": { + "workspace_proxies": { "regions": [ { "created_at": "2019-08-24T14:15:22Z", diff --git a/docs/api/schemas.md b/docs/api/schemas.md index 5100a4dc20e06..78992ff1a6c2e 100644 --- a/docs/api/schemas.md +++ b/docs/api/schemas.md @@ -7795,7 +7795,7 @@ If the schedule is empty, the user will be updated to use the default schedule.| "healthy": true, "severity": "ok", "warnings": ["string"], - "workspaceProxies": { + "workspace_proxies": { "regions": [ { "created_at": "2019-08-24T14:15:22Z", @@ -7889,7 +7889,7 @@ If the schedule is empty, the user will be updated to use the default schedule.| "healthy": true, "severity": "ok", "warnings": ["string"], - "workspaceProxies": { + "workspace_proxies": { "regions": [ { "created_at": "2019-08-24T14:15:22Z", @@ -7921,13 +7921,13 @@ If the schedule is empty, the user will be updated to use the default schedule.| ### Properties -| Name | Type | Required | Restrictions | Description | -| ------------------ | ---------------------------------------------------------------------------------------------------- | -------- | ------------ | ----------- | -| `error` | string | false | | | -| `healthy` | boolean | false | | | -| `severity` | [health.Severity](#healthseverity) | false | | | -| `warnings` | array of string | false | | | -| `workspaceProxies` | [codersdk.RegionsResponse-codersdk_WorkspaceProxy](#codersdkregionsresponse-codersdk_workspaceproxy) | false | | | +| Name | Type | Required | Restrictions | Description | +| ------------------- | ---------------------------------------------------------------------------------------------------- | -------- | ------------ | ----------- | +| `error` | string | false | | | +| `healthy` | boolean | false | | | +| `severity` | [health.Severity](#healthseverity) | false | | | +| `warnings` | array of string | false | | | +| `workspace_proxies` | [codersdk.RegionsResponse-codersdk_WorkspaceProxy](#codersdkregionsresponse-codersdk_workspaceproxy) | false | | | ## netcheck.Report diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index 317a7f77e65c6..06636d0ee41cb 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -2137,7 +2137,7 @@ export interface HealthcheckWorkspaceProxyReport { readonly error?: string; // Named type "github.com/coder/coder/v2/codersdk.RegionsResponse[github.com/coder/coder/v2/codersdk.WorkspaceProxy]" unknown, using "any" // eslint-disable-next-line @typescript-eslint/no-explicit-any -- External type - readonly WorkspaceProxies: any; + readonly workspace_proxies: any; } // The code below is generated from cli/clibase. diff --git a/site/src/testHelpers/entities.ts b/site/src/testHelpers/entities.ts index b448e08ff1cda..ee797cba0bf42 100644 --- a/site/src/testHelpers/entities.ts +++ b/site/src/testHelpers/entities.ts @@ -2877,4 +2877,37 @@ export const DeploymentHealthUnhealthy: TypesGen.HealthcheckReport = { body: "", code: 0, }, + workspace_proxy: { + healthy: false, + error: "some error", + severity: "error", + warnings: [], + workspace_proxies: { + regions: [ + { + id: "df7e4b2b-2d40-47e5-a021-e5d08b219c77", + name: "unhealthy", + display_name: "unhealthy", + icon_url: "/emojis/1f5fa.png", + healthy: false, + path_app_url: "http://127.0.0.1:3001", + wildcard_hostname: "", + derp_enabled: true, + derp_only: false, + status: { + status: "unreachable", + report: { + errors: ["some error"], + warnings: null, + }, + checked_at: "2023-11-24T12:14:05.743303497Z", + }, + created_at: "2023-11-23T15:37:25.513213Z", + updated_at: "2023-11-23T18:09:19.734747Z", + deleted: false, + version: "v2.4.0-devel+89bae7eff", + }, + ], + }, + }, }; From 5f5c48658ba367c620dd3c35aa7f0096ac04d6d0 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 24 Nov 2023 12:30:55 +0000 Subject: [PATCH 12/20] fixup! rename WorkspaceProxies -> workspace_proxies --- site/src/testHelpers/entities.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/site/src/testHelpers/entities.ts b/site/src/testHelpers/entities.ts index ee797cba0bf42..69ddc877ccc67 100644 --- a/site/src/testHelpers/entities.ts +++ b/site/src/testHelpers/entities.ts @@ -2827,6 +2827,14 @@ export const MockHealth: TypesGen.HealthcheckReport = { latency_ms: 92570, threshold_ms: 92570, }, + workspace_proxy: { + healthy: true, + severity: "ok", + warnings: [], + workspace_proxies: { + regions: [], + }, + }, coder_version: "v0.27.1-devel+c575292", }; From b9cbdd8f9f721f6957e9fc89b66956ed37de335b Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 24 Nov 2023 12:32:55 +0000 Subject: [PATCH 13/20] use severity value instead of direct comparison --- coderd/healthcheck/workspaceproxy.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/healthcheck/workspaceproxy.go b/coderd/healthcheck/workspaceproxy.go index ffaa9fb48e72d..c5cff0bfc5172 100644 --- a/coderd/healthcheck/workspaceproxy.go +++ b/coderd/healthcheck/workspaceproxy.go @@ -90,7 +90,7 @@ func (r *WorkspaceProxyReport) Run(ctx context.Context, opts *WorkspaceProxyRepo } r.Severity = calculateSeverity(total, healthy) - r.Healthy = r.Severity != health.SeverityError + r.Healthy = r.Severity.Value() < health.SeverityError.Value() // Versions _must_ match. Perform this check last. This will clobber any other severity. for _, proxy := range r.WorkspaceProxies.Regions { From ae30089cb70715ea0ed7e21740da7c20b1c5f06a Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 24 Nov 2023 12:49:02 +0000 Subject: [PATCH 14/20] more tests --- .../workspaceproxy_internal_test.go | 95 +++++++++++++++++++ 1 file changed, 95 insertions(+) create mode 100644 coderd/healthcheck/workspaceproxy_internal_test.go diff --git a/coderd/healthcheck/workspaceproxy_internal_test.go b/coderd/healthcheck/workspaceproxy_internal_test.go new file mode 100644 index 0000000000000..6b87b8de79eed --- /dev/null +++ b/coderd/healthcheck/workspaceproxy_internal_test.go @@ -0,0 +1,95 @@ +package healthcheck + +import ( + "fmt" + "testing" + + "github.com/coder/coder/v2/coderd/healthcheck/health" + "github.com/coder/coder/v2/coderd/util/ptr" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "golang.org/x/xerrors" +) + +func Test_WorkspaceProxyReport_appendErrors(t *testing.T) { + t.Parallel() + + for _, tt := range []struct { + name string + expected string + prevErr string + errs []error + }{ + { + name: "nil", + errs: nil, + }, + { + name: "one error", + expected: assert.AnError.Error(), + errs: []error{assert.AnError}, + }, + { + name: "one error, one prev", + prevErr: "previous error", + expected: "previous error\n" + assert.AnError.Error(), + errs: []error{assert.AnError}, + }, + { + name: "two errors", + expected: assert.AnError.Error() + "\nanother error", + errs: []error{assert.AnError, xerrors.Errorf("another error")}, + }, + { + name: "two errors, one prev", + prevErr: "previous error", + expected: "previous error\n" + assert.AnError.Error() + "\nanother error", + errs: []error{assert.AnError, xerrors.Errorf("another error")}, + }, + } { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + var rpt WorkspaceProxyReport + if tt.prevErr != "" { + rpt.Error = ptr.Ref(tt.prevErr) + } + rpt.appendError(tt.errs...) + if tt.expected == "" { + require.Nil(t, rpt.Error) + } else { + require.NotNil(t, rpt.Error) + require.Equal(t, tt.expected, *rpt.Error) + } + }) + } +} + +func Test_calculateSeverity(t *testing.T) { + t.Parallel() + + for _, tt := range []struct { + total int + healthy int + expected health.Severity + }{ + {0, 0, health.SeverityOK}, + {1, 1, health.SeverityOK}, + {1, 0, health.SeverityError}, + {2, 2, health.SeverityOK}, + {2, 1, health.SeverityWarning}, + {2, 0, health.SeverityError}, + } { + tt := tt + name := fmt.Sprintf("%d total, %d healthy -> %s", tt.total, tt.healthy, tt.expected) + t.Run(name, func(t *testing.T) { + t.Parallel() + + actual := calculateSeverity(tt.total, tt.healthy) + assert.Equal(t, tt.expected, actual) + }) + } +} From 21f52bb95f69931d7d344091f57b60473bae6d87 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 24 Nov 2023 12:49:16 +0000 Subject: [PATCH 15/20] use errors.Join instead of multierror.Append --- coderd/healthcheck/workspaceproxy.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/coderd/healthcheck/workspaceproxy.go b/coderd/healthcheck/workspaceproxy.go index c5cff0bfc5172..7968d56764171 100644 --- a/coderd/healthcheck/workspaceproxy.go +++ b/coderd/healthcheck/workspaceproxy.go @@ -2,6 +2,7 @@ package healthcheck import ( "context" + "errors" "sort" "golang.org/x/xerrors" @@ -10,8 +11,6 @@ import ( "github.com/coder/coder/v2/coderd/healthcheck/health" "github.com/coder/coder/v2/coderd/util/ptr" "github.com/coder/coder/v2/codersdk" - - "github.com/hashicorp/go-multierror" ) type WorkspaceProxyReportOptions struct { @@ -102,14 +101,16 @@ func (r *WorkspaceProxyReport) Run(ctx context.Context, opts *WorkspaceProxyRepo } } -// appendError multierror-appends err onto r.Error. +// appendError appends errs onto r.Error. // We only have one error, so multiple errors need to be squashed in there. func (r *WorkspaceProxyReport) appendError(errs ...error) { - var prevErr error + if len(errs) == 0 { + return + } if r.Error != nil { - prevErr = xerrors.New(*r.Error) + errs = append([]error{xerrors.New(*r.Error)}, errs...) } - r.Error = ptr.Ref(multierror.Append(prevErr, errs...).Error()) + r.Error = ptr.Ref(errors.Join(errs...).Error()) } func checkVersion(proxy codersdk.WorkspaceProxy, currentVersion string) error { From 79dc190556ba3c2adba4e858e826c1bd486fefe1 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 24 Nov 2023 13:35:44 +0000 Subject: [PATCH 16/20] refactor to use interface instead of atomic.Pointers --- coderd/coderd.go | 28 +++++------- coderd/healthcheck/workspaceproxy.go | 43 ++++++++++--------- coderd/healthcheck/workspaceproxy_test.go | 52 +++++++++++++++-------- enterprise/coderd/coderd.go | 10 +++-- enterprise/coderd/workspaceproxy.go | 17 ++++++++ 5 files changed, 92 insertions(+), 58 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index e941c11785a70..d66ad617f7294 100644 --- 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 index 7968d56764171..3cfaf6e8c0158 100644 --- 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 index c0ff0fd64743c..a96c13384fb5b 100644 --- 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 index ca890ae2f179c..869defb1470d0 100644 --- 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 index eaef17542c6bd..fb01d4ab9a690 100644 --- 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) +} From 2530320e3d809b28267ba48b4776e4e3f4d17757 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 24 Nov 2023 13:37:55 +0000 Subject: [PATCH 17/20] rm unnecessary authz --- coderd/coderd.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index d66ad617f7294..4dfb9a86cea7d 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -408,8 +408,7 @@ func New(options *Options) *API { if options.HealthcheckFunc == nil { options.HealthcheckFunc = func(ctx context.Context, apiKey string) *healthcheck.Report { - authCtx := dbauthz.AsSystemRestricted(ctx) //nolint:gocritic // internal function - return healthcheck.Run(authCtx, &healthcheck.ReportOptions{ + return healthcheck.Run(ctx, &healthcheck.ReportOptions{ Database: healthcheck.DatabaseReportOptions{ DB: options.Database, Threshold: options.DeploymentValues.Healthcheck.ThresholdDatabase.Value(), From f1924881a2d09e3935c95c4a33906bb6f6139d98 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 24 Nov 2023 14:09:44 +0000 Subject: [PATCH 18/20] fix typegen --- scripts/apitypings/main.go | 4 ++++ site/src/api/typesGenerated.ts | 4 +--- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/scripts/apitypings/main.go b/scripts/apitypings/main.go index 7db5565bf2bba..6a182d5c0d53b 100644 --- a/scripts/apitypings/main.go +++ b/scripts/apitypings/main.go @@ -867,6 +867,10 @@ func (g *Generator) typescriptType(ty types.Type) (TypescriptType, error) { return TypescriptType{ValueType: "Record"}, nil case "github.com/coder/coder/v2/cli/clibase.URL": return TypescriptType{ValueType: "string"}, nil + // XXX: For some reason, the type generator generates this as `any` + // so explicitly specifying the correct generic TS type. + case "github.com/coder/coder/v2/codersdk.RegionsResponse[github.com/coder/coder/v2/codersdk.WorkspaceProxy]": + return TypescriptType{ValueType: "RegionsResponse"}, nil } // Some hard codes are a bit trickier. diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index 06636d0ee41cb..fe2ad416f7315 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -2135,9 +2135,7 @@ export interface HealthcheckWorkspaceProxyReport { readonly severity: HealthSeverity; readonly warnings: string[]; readonly error?: string; - // Named type "github.com/coder/coder/v2/codersdk.RegionsResponse[github.com/coder/coder/v2/codersdk.WorkspaceProxy]" unknown, using "any" - // eslint-disable-next-line @typescript-eslint/no-explicit-any -- External type - readonly workspace_proxies: any; + readonly workspace_proxies: RegionsResponse; } // The code below is generated from cli/clibase. From 63fbafbe21afd8a7fd92147ac918e6f34844294c Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 24 Nov 2023 14:22:32 +0000 Subject: [PATCH 19/20] make typescript stop whingeing --- site/src/testHelpers/entities.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/src/testHelpers/entities.ts b/site/src/testHelpers/entities.ts index 69ddc877ccc67..076bdf2167e68 100644 --- a/site/src/testHelpers/entities.ts +++ b/site/src/testHelpers/entities.ts @@ -2906,7 +2906,7 @@ export const DeploymentHealthUnhealthy: TypesGen.HealthcheckReport = { status: "unreachable", report: { errors: ["some error"], - warnings: null, + warnings: [], }, checked_at: "2023-11-24T12:14:05.743303497Z", }, From b57cdeba9cde7f34f86f9f4796dc22b2c593be66 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 24 Nov 2023 14:52:24 +0000 Subject: [PATCH 20/20] remove need for gosimple nolint --- coderd/coderd.go | 3 +-- enterprise/coderd/coderd.go | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index 4dfb9a86cea7d..50cad7e0f8a2f 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -401,8 +401,7 @@ func New(options *Options) *API { if options.WorkspaceProxiesFetchUpdater == nil { options.WorkspaceProxiesFetchUpdater = &atomic.Pointer[healthcheck.WorkspaceProxiesFetchUpdater]{} - var wpfu healthcheck.WorkspaceProxiesFetchUpdater //nolint:gosimple - wpfu = &healthcheck.AGPLWorkspaceProxiesFetchUpdater{} + var wpfu healthcheck.WorkspaceProxiesFetchUpdater = &healthcheck.AGPLWorkspaceProxiesFetchUpdater{} options.WorkspaceProxiesFetchUpdater.Store(&wpfu) } diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go index 869defb1470d0..028ae5a6768c1 100644 --- a/enterprise/coderd/coderd.go +++ b/enterprise/coderd/coderd.go @@ -377,8 +377,7 @@ func New(ctx context.Context, options *Options) (_ *API, err error) { api.AGPL.WorkspaceProxyHostsFn.Store(&f) // Wire this up to healthcheck. - var fetchUpdater healthcheck.WorkspaceProxiesFetchUpdater //nolint:gosimple - fetchUpdater = &workspaceProxiesFetchUpdater{ + var fetchUpdater healthcheck.WorkspaceProxiesFetchUpdater = &workspaceProxiesFetchUpdater{ fetchFunc: api.fetchWorkspaceProxies, updateFunc: api.ProxyHealth.ForceUpdate, }