diff --git a/cli/server.go b/cli/server.go index 03dcc698c1..41a957815f 100644 --- a/cli/server.go +++ b/cli/server.go @@ -781,40 +781,42 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd. // This should be output before the logs start streaming. cliui.Infof(inv.Stdout, "\n==> Logs will stream in below (press ctrl+c to gracefully exit):") - if vals.Telemetry.Enable { - vals, err := vals.WithoutSecrets() - if err != nil { - return xerrors.Errorf("remove secrets from deployment values: %w", err) - } - options.Telemetry, err = telemetry.New(telemetry.Options{ - BuiltinPostgres: builtinPostgres, - DeploymentID: deploymentID, - Database: options.Database, - Logger: logger.Named("telemetry"), - URL: vals.Telemetry.URL.Value(), - Tunnel: tunnel != nil, - DeploymentConfig: vals, - ParseLicenseJWT: func(lic *telemetry.License) error { - // This will be nil when running in AGPL-only mode. - if options.ParseLicenseClaims == nil { - return nil - } - - email, trial, err := options.ParseLicenseClaims(lic.JWT) - if err != nil { - return err - } - if email != "" { - lic.Email = &email - } - lic.Trial = &trial + deploymentConfigWithoutSecrets, err := vals.WithoutSecrets() + if err != nil { + return xerrors.Errorf("remove secrets from deployment values: %w", err) + } + telemetryReporter, err := telemetry.New(telemetry.Options{ + Disabled: !vals.Telemetry.Enable.Value(), + BuiltinPostgres: builtinPostgres, + DeploymentID: deploymentID, + Database: options.Database, + Logger: logger.Named("telemetry"), + URL: vals.Telemetry.URL.Value(), + Tunnel: tunnel != nil, + DeploymentConfig: deploymentConfigWithoutSecrets, + ParseLicenseJWT: func(lic *telemetry.License) error { + // This will be nil when running in AGPL-only mode. + if options.ParseLicenseClaims == nil { return nil - }, - }) - if err != nil { - return xerrors.Errorf("create telemetry reporter: %w", err) - } - defer options.Telemetry.Close() + } + + email, trial, err := options.ParseLicenseClaims(lic.JWT) + if err != nil { + return err + } + if email != "" { + lic.Email = &email + } + lic.Trial = &trial + return nil + }, + }) + if err != nil { + return xerrors.Errorf("create telemetry reporter: %w", err) + } + defer telemetryReporter.Close() + if vals.Telemetry.Enable.Value() { + options.Telemetry = telemetryReporter } else { logger.Warn(ctx, fmt.Sprintf(`telemetry disabled, unable to notify of security issues. Read more: %s/admin/setup/telemetry`, vals.DocsURL.String())) } diff --git a/cli/server_test.go b/cli/server_test.go index 8ed4d89ceb..988fde808d 100644 --- a/cli/server_test.go +++ b/cli/server_test.go @@ -39,6 +39,7 @@ import ( "tailscale.com/types/key" "cdr.dev/slog/sloggers/slogtest" + "github.com/coder/coder/v2/buildinfo" "github.com/coder/coder/v2/cli" "github.com/coder/coder/v2/cli/clitest" "github.com/coder/coder/v2/cli/config" @@ -947,22 +948,7 @@ func TestServer(t *testing.T) { t.Run("Telemetry", func(t *testing.T) { t.Parallel() - deployment := make(chan struct{}, 64) - snapshot := make(chan *telemetry.Snapshot, 64) - r := chi.NewRouter() - r.Post("/deployment", func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusAccepted) - deployment <- struct{}{} - }) - r.Post("/snapshot", func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusAccepted) - ss := &telemetry.Snapshot{} - err := json.NewDecoder(r.Body).Decode(ss) - require.NoError(t, err) - snapshot <- ss - }) - server := httptest.NewServer(r) - defer server.Close() + telemetryServerURL, deployment, snapshot := mockTelemetryServer(t) inv, cfg := clitest.New(t, "server", @@ -970,7 +956,7 @@ func TestServer(t *testing.T) { "--http-address", ":0", "--access-url", "http://example.com", "--telemetry", - "--telemetry-url", server.URL, + "--telemetry-url", telemetryServerURL.String(), "--cache-dir", t.TempDir(), ) clitest.Start(t, inv) @@ -2009,3 +1995,148 @@ func TestServer_DisabledDERP(t *testing.T) { err = c.Connect(ctx) require.Error(t, err) } + +type runServerOpts struct { + waitForSnapshot bool + telemetryDisabled bool + waitForTelemetryDisabledCheck bool +} + +func TestServer_TelemetryDisabled_FinalReport(t *testing.T) { + t.Parallel() + + if !dbtestutil.WillUsePostgres() { + t.Skip("this test requires postgres") + } + + telemetryServerURL, deployment, snapshot := mockTelemetryServer(t) + dbConnURL, err := dbtestutil.Open(t) + require.NoError(t, err) + + cacheDir := t.TempDir() + runServer := func(t *testing.T, opts runServerOpts) (chan error, context.CancelFunc) { + ctx, cancelFunc := context.WithCancel(context.Background()) + inv, _ := clitest.New(t, + "server", + "--postgres-url", dbConnURL, + "--http-address", ":0", + "--access-url", "http://example.com", + "--telemetry="+strconv.FormatBool(!opts.telemetryDisabled), + "--telemetry-url", telemetryServerURL.String(), + "--cache-dir", cacheDir, + "--log-filter", ".*", + ) + finished := make(chan bool, 2) + errChan := make(chan error, 1) + pty := ptytest.New(t).Attach(inv) + go func() { + errChan <- inv.WithContext(ctx).Run() + finished <- true + }() + go func() { + defer func() { + finished <- true + }() + if opts.waitForSnapshot { + pty.ExpectMatchContext(testutil.Context(t, testutil.WaitLong), "submitted snapshot") + } + if opts.waitForTelemetryDisabledCheck { + pty.ExpectMatchContext(testutil.Context(t, testutil.WaitLong), "finished telemetry status check") + } + }() + <-finished + return errChan, cancelFunc + } + waitForShutdown := func(t *testing.T, errChan chan error) error { + t.Helper() + select { + case err := <-errChan: + return err + case <-time.After(testutil.WaitMedium): + t.Fatalf("timed out waiting for server to shutdown") + } + return nil + } + + errChan, cancelFunc := runServer(t, runServerOpts{telemetryDisabled: true, waitForTelemetryDisabledCheck: true}) + cancelFunc() + require.NoError(t, waitForShutdown(t, errChan)) + + // Since telemetry was disabled, we expect no deployments or snapshots. + require.Empty(t, deployment) + require.Empty(t, snapshot) + + errChan, cancelFunc = runServer(t, runServerOpts{waitForSnapshot: true}) + cancelFunc() + require.NoError(t, waitForShutdown(t, errChan)) + // we expect to see a deployment and a snapshot twice: + // 1. the first pair is sent when the server starts + // 2. the second pair is sent when the server shuts down + for i := 0; i < 2; i++ { + select { + case <-snapshot: + case <-time.After(testutil.WaitShort / 2): + t.Fatalf("timed out waiting for snapshot") + } + select { + case <-deployment: + case <-time.After(testutil.WaitShort / 2): + t.Fatalf("timed out waiting for deployment") + } + } + + errChan, cancelFunc = runServer(t, runServerOpts{telemetryDisabled: true, waitForTelemetryDisabledCheck: true}) + cancelFunc() + require.NoError(t, waitForShutdown(t, errChan)) + + // Since telemetry is disabled, we expect no deployment. We expect a snapshot + // with the telemetry disabled item. + require.Empty(t, deployment) + select { + case ss := <-snapshot: + require.Len(t, ss.TelemetryItems, 1) + require.Equal(t, string(telemetry.TelemetryItemKeyTelemetryEnabled), ss.TelemetryItems[0].Key) + require.Equal(t, "false", ss.TelemetryItems[0].Value) + case <-time.After(testutil.WaitShort / 2): + t.Fatalf("timed out waiting for snapshot") + } + + errChan, cancelFunc = runServer(t, runServerOpts{telemetryDisabled: true, waitForTelemetryDisabledCheck: true}) + cancelFunc() + require.NoError(t, waitForShutdown(t, errChan)) + // Since telemetry is disabled and we've already sent a snapshot, we expect no + // new deployments or snapshots. + require.Empty(t, deployment) + require.Empty(t, snapshot) +} + +func mockTelemetryServer(t *testing.T) (*url.URL, chan *telemetry.Deployment, chan *telemetry.Snapshot) { + t.Helper() + deployment := make(chan *telemetry.Deployment, 64) + snapshot := make(chan *telemetry.Snapshot, 64) + r := chi.NewRouter() + r.Post("/deployment", func(w http.ResponseWriter, r *http.Request) { + require.Equal(t, buildinfo.Version(), r.Header.Get(telemetry.VersionHeader)) + dd := &telemetry.Deployment{} + err := json.NewDecoder(r.Body).Decode(dd) + require.NoError(t, err) + deployment <- dd + // Ensure the header is sent only after deployment is sent + w.WriteHeader(http.StatusAccepted) + }) + r.Post("/snapshot", func(w http.ResponseWriter, r *http.Request) { + require.Equal(t, buildinfo.Version(), r.Header.Get(telemetry.VersionHeader)) + ss := &telemetry.Snapshot{} + err := json.NewDecoder(r.Body).Decode(ss) + require.NoError(t, err) + snapshot <- ss + // Ensure the header is sent only after snapshot is sent + w.WriteHeader(http.StatusAccepted) + }) + server := httptest.NewServer(r) + t.Cleanup(server.Close) + serverURL, err := url.Parse(server.URL) + require.NoError(t, err) + + return serverURL, deployment, snapshot +} diff --git a/coderd/telemetry/telemetry.go b/coderd/telemetry/telemetry.go index 3b4bcb7d15..78819b0c65 100644 --- a/coderd/telemetry/telemetry.go +++ b/coderd/telemetry/telemetry.go @@ -15,6 +15,7 @@ import ( "regexp" "runtime" "slices" + "strconv" "strings" "sync" "time" @@ -42,6 +43,7 @@ const ( ) type Options struct { + Disabled bool Database database.Store Logger slog.Logger // URL is an endpoint to direct telemetry towards! @@ -116,8 +118,8 @@ type remoteReporter struct { shutdownAt *time.Time } -func (*remoteReporter) Enabled() bool { - return true +func (r *remoteReporter) Enabled() bool { + return !r.options.Disabled } func (r *remoteReporter) Report(snapshot *Snapshot) { @@ -161,10 +163,12 @@ func (r *remoteReporter) Close() { close(r.closed) now := dbtime.Now() r.shutdownAt = &now - // Report a final collection of telemetry prior to close! - // This could indicate final actions a user has taken, and - // the time the deployment was shutdown. - r.reportWithDeployment() + if r.Enabled() { + // Report a final collection of telemetry prior to close! + // This could indicate final actions a user has taken, and + // the time the deployment was shutdown. + r.reportWithDeployment() + } r.closeFunc() } @@ -177,7 +181,74 @@ func (r *remoteReporter) isClosed() bool { } } +// See the corresponding test in telemetry_test.go for a truth table. +func ShouldReportTelemetryDisabled(recordedTelemetryEnabled *bool, telemetryEnabled bool) bool { + return recordedTelemetryEnabled != nil && *recordedTelemetryEnabled && !telemetryEnabled +} + +// RecordTelemetryStatus records the telemetry status in the database. +// If the status changed from enabled to disabled, returns a snapshot to +// be sent to the telemetry server. +func RecordTelemetryStatus( //nolint:revive + ctx context.Context, + logger slog.Logger, + db database.Store, + telemetryEnabled bool, +) (*Snapshot, error) { + item, err := db.GetTelemetryItem(ctx, string(TelemetryItemKeyTelemetryEnabled)) + if err != nil && !errors.Is(err, sql.ErrNoRows) { + return nil, xerrors.Errorf("get telemetry enabled: %w", err) + } + var recordedTelemetryEnabled *bool + if !errors.Is(err, sql.ErrNoRows) { + value, err := strconv.ParseBool(item.Value) + if err != nil { + logger.Debug(ctx, "parse telemetry enabled", slog.Error(err)) + } + // If ParseBool fails, value will default to false. + // This may happen if an admin manually edits the telemetry item + // in the database. + recordedTelemetryEnabled = &value + } + + if err := db.UpsertTelemetryItem(ctx, database.UpsertTelemetryItemParams{ + Key: string(TelemetryItemKeyTelemetryEnabled), + Value: strconv.FormatBool(telemetryEnabled), + }); err != nil { + return nil, xerrors.Errorf("upsert telemetry enabled: %w", err) + } + + shouldReport := ShouldReportTelemetryDisabled(recordedTelemetryEnabled, telemetryEnabled) + if !shouldReport { + return nil, nil //nolint:nilnil + } + // If any of the following calls fail, we will never report that telemetry changed + // from enabled to disabled. This is okay. We only want to ping the telemetry server + // once, and never again. If that attempt fails, so be it. + item, err = db.GetTelemetryItem(ctx, string(TelemetryItemKeyTelemetryEnabled)) + if err != nil { + return nil, xerrors.Errorf("get telemetry enabled after upsert: %w", err) + } + return &Snapshot{ + TelemetryItems: []TelemetryItem{ + ConvertTelemetryItem(item), + }, + }, nil +} + func (r *remoteReporter) runSnapshotter() { + telemetryDisabledSnapshot, err := RecordTelemetryStatus(r.ctx, r.options.Logger, r.options.Database, r.Enabled()) + if err != nil { + r.options.Logger.Debug(r.ctx, "record and maybe report telemetry status", slog.Error(err)) + } + if telemetryDisabledSnapshot != nil { + r.reportSync(telemetryDisabledSnapshot) + } + r.options.Logger.Debug(r.ctx, "finished telemetry status check") + if !r.Enabled() { + return + } + first := true ticker := time.NewTicker(r.options.SnapshotFrequency) defer ticker.Stop() @@ -1567,6 +1638,7 @@ type telemetryItemKey string //revive:disable:exported const ( TelemetryItemKeyHTMLFirstServedAt telemetryItemKey = "html_first_served_at" + TelemetryItemKeyTelemetryEnabled telemetryItemKey = "telemetry_enabled" ) type TelemetryItem struct { @@ -1578,6 +1650,8 @@ type TelemetryItem struct { type noopReporter struct{} -func (*noopReporter) Report(_ *Snapshot) {} -func (*noopReporter) Enabled() bool { return false } -func (*noopReporter) Close() {} +func (*noopReporter) Report(_ *Snapshot) {} +func (*noopReporter) Enabled() bool { return false } +func (*noopReporter) Close() {} +func (*noopReporter) RunSnapshotter() {} +func (*noopReporter) ReportDisabledIfNeeded() error { return nil } diff --git a/coderd/telemetry/telemetry_test.go b/coderd/telemetry/telemetry_test.go index 647dcd834c..29fcb644fc 100644 --- a/coderd/telemetry/telemetry_test.go +++ b/coderd/telemetry/telemetry_test.go @@ -131,7 +131,8 @@ func TestTelemetry(t *testing.T) { require.Len(t, snapshot.WorkspaceProxies, 1) require.Len(t, snapshot.WorkspaceModules, 1) require.Len(t, snapshot.Organizations, 1) - require.Len(t, snapshot.TelemetryItems, 1) + // We create one item manually above. The other is TelemetryEnabled, created by the snapshotter. + require.Len(t, snapshot.TelemetryItems, 2) wsa := snapshot.WorkspaceAgents[0] require.Len(t, wsa.Subsystems, 2) require.Equal(t, string(database.WorkspaceAgentSubsystemEnvbox), wsa.Subsystems[0]) @@ -361,31 +362,112 @@ func TestTelemetryItem(t *testing.T) { require.Equal(t, item.Value, "new_value") } -func collectSnapshot(t *testing.T, db database.Store, addOptionsFn func(opts telemetry.Options) telemetry.Options) (*telemetry.Deployment, *telemetry.Snapshot) { +func TestShouldReportTelemetryDisabled(t *testing.T) { + t.Parallel() + // Description | telemetryEnabled (db) | telemetryEnabled (is) | Report Telemetry Disabled | + //----------------------------------------|-----------------------|-----------------------|---------------------------| + // New deployment | | true | No | + // New deployment with telemetry disabled | | false | No | + // Telemetry was enabled, and still is | true | true | No | + // Telemetry was enabled but now disabled | true | false | Yes | + // Telemetry was disabled, now is enabled | false | true | No | + // Telemetry was disabled, still disabled | false | false | No | + boolTrue := true + boolFalse := false + require.False(t, telemetry.ShouldReportTelemetryDisabled(nil, true)) + require.False(t, telemetry.ShouldReportTelemetryDisabled(nil, false)) + require.False(t, telemetry.ShouldReportTelemetryDisabled(&boolTrue, true)) + require.True(t, telemetry.ShouldReportTelemetryDisabled(&boolTrue, false)) + require.False(t, telemetry.ShouldReportTelemetryDisabled(&boolFalse, true)) + require.False(t, telemetry.ShouldReportTelemetryDisabled(&boolFalse, false)) +} + +func TestRecordTelemetryStatus(t *testing.T) { + t.Parallel() + for _, testCase := range []struct { + name string + recordedTelemetryEnabled string + telemetryEnabled bool + shouldReport bool + }{ + {name: "New deployment", recordedTelemetryEnabled: "nil", telemetryEnabled: true, shouldReport: false}, + {name: "Telemetry disabled", recordedTelemetryEnabled: "nil", telemetryEnabled: false, shouldReport: false}, + {name: "Telemetry was enabled and still is", recordedTelemetryEnabled: "true", telemetryEnabled: true, shouldReport: false}, + {name: "Telemetry was enabled but now disabled", recordedTelemetryEnabled: "true", telemetryEnabled: false, shouldReport: true}, + {name: "Telemetry was disabled now is enabled", recordedTelemetryEnabled: "false", telemetryEnabled: true, shouldReport: false}, + {name: "Telemetry was disabled still disabled", recordedTelemetryEnabled: "false", telemetryEnabled: false, shouldReport: false}, + {name: "Telemetry was disabled still disabled, invalid value", recordedTelemetryEnabled: "invalid", telemetryEnabled: false, shouldReport: false}, + } { + testCase := testCase + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + + db, _ := dbtestutil.NewDB(t) + ctx := testutil.Context(t, testutil.WaitMedium) + logger := testutil.Logger(t) + if testCase.recordedTelemetryEnabled != "nil" { + db.UpsertTelemetryItem(ctx, database.UpsertTelemetryItemParams{ + Key: string(telemetry.TelemetryItemKeyTelemetryEnabled), + Value: testCase.recordedTelemetryEnabled, + }) + } + snapshot1, err := telemetry.RecordTelemetryStatus(ctx, logger, db, testCase.telemetryEnabled) + require.NoError(t, err) + + if testCase.shouldReport { + require.NotNil(t, snapshot1) + require.Equal(t, snapshot1.TelemetryItems[0].Key, string(telemetry.TelemetryItemKeyTelemetryEnabled)) + require.Equal(t, snapshot1.TelemetryItems[0].Value, "false") + } else { + require.Nil(t, snapshot1) + } + + for i := 0; i < 3; i++ { + // Whatever happens, subsequent calls should not report if telemetryEnabled didn't change + snapshot2, err := telemetry.RecordTelemetryStatus(ctx, logger, db, testCase.telemetryEnabled) + require.NoError(t, err) + require.Nil(t, snapshot2) + } + }) + } +} + +func mockTelemetryServer(t *testing.T) (*url.URL, chan *telemetry.Deployment, chan *telemetry.Snapshot) { t.Helper() deployment := make(chan *telemetry.Deployment, 64) snapshot := make(chan *telemetry.Snapshot, 64) r := chi.NewRouter() r.Post("/deployment", func(w http.ResponseWriter, r *http.Request) { require.Equal(t, buildinfo.Version(), r.Header.Get(telemetry.VersionHeader)) - w.WriteHeader(http.StatusAccepted) dd := &telemetry.Deployment{} err := json.NewDecoder(r.Body).Decode(dd) require.NoError(t, err) deployment <- dd + // Ensure the header is sent only after deployment is sent + w.WriteHeader(http.StatusAccepted) }) r.Post("/snapshot", func(w http.ResponseWriter, r *http.Request) { require.Equal(t, buildinfo.Version(), r.Header.Get(telemetry.VersionHeader)) - w.WriteHeader(http.StatusAccepted) ss := &telemetry.Snapshot{} err := json.NewDecoder(r.Body).Decode(ss) require.NoError(t, err) snapshot <- ss + // Ensure the header is sent only after snapshot is sent + w.WriteHeader(http.StatusAccepted) }) server := httptest.NewServer(r) t.Cleanup(server.Close) serverURL, err := url.Parse(server.URL) require.NoError(t, err) + + return serverURL, deployment, snapshot +} + +func collectSnapshot(t *testing.T, db database.Store, addOptionsFn func(opts telemetry.Options) telemetry.Options) (*telemetry.Deployment, *telemetry.Snapshot) { + t.Helper() + + serverURL, deployment, snapshot := mockTelemetryServer(t) + options := telemetry.Options{ Database: db, Logger: testutil.Logger(t),