From 8d7eb1728cc9988d8f6d078209661e6f1e3f437a Mon Sep 17 00:00:00 2001 From: Spike Curtis Date: Fri, 8 Sep 2023 14:37:36 +0400 Subject: [PATCH] fix: stop inserting provisioner daemons into the database (#9108) Signed-off-by: Spike Curtis --- coderd/coderd.go | 37 +++++++------------- codersdk/organizations.go | 3 ++ enterprise/coderd/provisionerdaemons.go | 33 +++++------------ enterprise/coderd/provisionerdaemons_test.go | 26 -------------- 4 files changed, 24 insertions(+), 75 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index f71e681955..52c7740de4 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -46,7 +46,6 @@ import ( "github.com/coder/coder/v2/coderd/batchstats" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/dbauthz" - "github.com/coder/coder/v2/coderd/database/dbtime" "github.com/coder/coder/v2/coderd/database/pubsub" "github.com/coder/coder/v2/coderd/gitauth" "github.com/coder/coder/v2/coderd/gitsshkey" @@ -1078,32 +1077,24 @@ func (api *API) CreateInMemoryProvisionerDaemon(ctx context.Context, debounce ti } }() - name := namesgenerator.GetRandomName(1) - // nolint:gocritic // Inserting a provisioner daemon is a system function. - daemon, err := api.Database.InsertProvisionerDaemon(dbauthz.AsSystemRestricted(ctx), database.InsertProvisionerDaemonParams{ - ID: uuid.New(), - CreatedAt: dbtime.Now(), - Name: name, - Provisioners: []database.ProvisionerType{database.ProvisionerTypeEcho, database.ProvisionerTypeTerraform}, - Tags: database.StringMap{ - provisionerdserver.TagScope: provisionerdserver.ScopeOrganization, - }, + tags, err := json.Marshal(database.StringMap{ + provisionerdserver.TagScope: provisionerdserver.ScopeOrganization, }) - if err != nil { - return nil, xerrors.Errorf("insert provisioner daemon %q: %w", name, err) - } - - tags, err := json.Marshal(daemon.Tags) if err != nil { return nil, xerrors.Errorf("marshal tags: %w", err) } mux := drpcmux.New() + name := namesgenerator.GetRandomName(1) + logger := api.Logger.Named(fmt.Sprintf("inmem-provisionerd-%s", name)) + logger.Info(ctx, "starting in-memory provisioner daemon") srv, err := provisionerdserver.NewServer( api.AccessURL, - daemon.ID, - api.Logger.Named(fmt.Sprintf("provisionerd-%s", daemon.Name)), - daemon.Provisioners, + uuid.New(), + logger, + []database.ProvisionerType{ + database.ProvisionerTypeEcho, database.ProvisionerTypeTerraform, + }, tags, api.Database, api.Pubsub, @@ -1133,16 +1124,14 @@ func (api *API) CreateInMemoryProvisionerDaemon(ctx context.Context, debounce ti if xerrors.Is(err, io.EOF) { return } - api.Logger.Debug(ctx, "drpc server error", slog.Error(err)) + logger.Debug(ctx, "drpc server error", slog.Error(err)) }, }, ) go func() { err := server.Serve(ctx, serverSession) - if err != nil && !xerrors.Is(err, io.EOF) { - api.Logger.Debug(ctx, "provisioner daemon disconnected", slog.Error(err)) - } - // close the sessions so we don't leak goroutines serving them. + logger.Info(ctx, "provisioner daemon disconnected", slog.Error(err)) + // close the sessions, so we don't leak goroutines serving them. _ = clientSession.Close() _ = serverSession.Close() }() diff --git a/codersdk/organizations.go b/codersdk/organizations.go index aaa052a39d..f88d526c51 100644 --- a/codersdk/organizations.go +++ b/codersdk/organizations.go @@ -154,6 +154,9 @@ func (c *Client) Organization(ctx context.Context, id uuid.UUID) (Organization, } // ProvisionerDaemons returns provisioner daemons available. +// +// Deprecated: We no longer track provisioner daemons as they connect. This function may return historical data +// but new provisioner daemons will not appear. func (c *Client) ProvisionerDaemons(ctx context.Context) ([]ProvisionerDaemon, error) { res, err := c.Request(ctx, http.MethodGet, // TODO: the organization path parameter is currently ignored. diff --git a/enterprise/coderd/provisionerdaemons.go b/enterprise/coderd/provisionerdaemons.go index a0b0a1773d..e3f2bda732 100644 --- a/enterprise/coderd/provisionerdaemons.go +++ b/enterprise/coderd/provisionerdaemons.go @@ -25,7 +25,6 @@ import ( "cdr.dev/slog" "github.com/coder/coder/v2/coderd" "github.com/coder/coder/v2/coderd/database" - "github.com/coder/coder/v2/coderd/database/dbtime" "github.com/coder/coder/v2/coderd/httpapi" "github.com/coder/coder/v2/coderd/httpmw" "github.com/coder/coder/v2/coderd/provisionerdserver" @@ -198,25 +197,7 @@ func (api *API) provisionerDaemonServe(rw http.ResponseWriter, r *http.Request) slog.F("provisioners", provisioners), slog.F("tags", tags), ) - daemon, err := api.Database.InsertProvisionerDaemon(ctx, database.InsertProvisionerDaemonParams{ - ID: uuid.New(), - CreatedAt: dbtime.Now(), - Name: name, - Provisioners: provisioners, - Tags: tags, - }) - if err != nil { - if !xerrors.Is(err, context.Canceled) { - log.Error(ctx, "write provisioner daemon", slog.Error(err)) - } - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error writing provisioner daemon.", - Detail: err.Error(), - }) - return - } - - rawTags, err := json.Marshal(daemon.Tags) + rawTags, err := json.Marshal(tags) if err != nil { if !xerrors.Is(err, context.Canceled) { log.Error(ctx, "marshal provisioner tags", slog.Error(err)) @@ -263,11 +244,13 @@ func (api *API) provisionerDaemonServe(rw http.ResponseWriter, r *http.Request) return } mux := drpcmux.New() + logger := api.Logger.Named(fmt.Sprintf("ext-provisionerd-%s", name)) + logger.Info(ctx, "starting external provisioner daemon") srv, err := provisionerdserver.NewServer( api.AccessURL, - daemon.ID, - api.Logger.Named(fmt.Sprintf("provisionerd-%s", daemon.Name)), - daemon.Provisioners, + uuid.New(), + logger, + provisioners, rawTags, api.Database, api.Pubsub, @@ -302,12 +285,12 @@ func (api *API) provisionerDaemonServe(rw http.ResponseWriter, r *http.Request) if xerrors.Is(err, io.EOF) { return } - api.Logger.Debug(ctx, "drpc server error", slog.Error(err)) + logger.Debug(ctx, "drpc server error", slog.Error(err)) }, }) err = server.Serve(ctx, session) + logger.Info(ctx, "provisioner daemon disconnected", slog.Error(err)) if err != nil && !xerrors.Is(err, io.EOF) { - api.Logger.Debug(ctx, "provisioner daemon disconnected", slog.Error(err)) _ = conn.Close(websocket.StatusInternalError, httpapi.WebsocketCloseSprintf("serve: %s", err)) return } diff --git a/enterprise/coderd/provisionerdaemons_test.go b/enterprise/coderd/provisionerdaemons_test.go index aa4b1295dd..9c7daccd22 100644 --- a/enterprise/coderd/provisionerdaemons_test.go +++ b/enterprise/coderd/provisionerdaemons_test.go @@ -47,9 +47,6 @@ func TestProvisionerDaemonServe(t *testing.T) { }) require.NoError(t, err) srv.DRPCConn().Close() - daemons, err := client.ProvisionerDaemons(ctx) - require.NoError(t, err) - require.Len(t, daemons, 1) }) t.Run("NoLicense", func(t *testing.T) { @@ -68,11 +65,6 @@ func TestProvisionerDaemonServe(t *testing.T) { var apiError *codersdk.Error require.ErrorAs(t, err, &apiError) require.Equal(t, http.StatusForbidden, apiError.StatusCode()) - - // querying provisioner daemons is forbidden without license - _, err = client.ProvisionerDaemons(ctx) - require.ErrorAs(t, err, &apiError) - require.Equal(t, http.StatusForbidden, apiError.StatusCode()) }) t.Run("Organization", func(t *testing.T) { @@ -98,9 +90,6 @@ func TestProvisionerDaemonServe(t *testing.T) { var apiError *codersdk.Error require.ErrorAs(t, err, &apiError) require.Equal(t, http.StatusForbidden, apiError.StatusCode()) - daemons, err := client.ProvisionerDaemons(ctx) - require.NoError(t, err) - require.Len(t, daemons, 0) }) t.Run("OrganizationNoPerms", func(t *testing.T) { @@ -126,9 +115,6 @@ func TestProvisionerDaemonServe(t *testing.T) { var apiError *codersdk.Error require.ErrorAs(t, err, &apiError) require.Equal(t, http.StatusForbidden, apiError.StatusCode()) - daemons, err := client.ProvisionerDaemons(ctx) - require.NoError(t, err) - require.Len(t, daemons, 0) }) t.Run("UserLocal", func(t *testing.T) { @@ -214,9 +200,6 @@ func TestProvisionerDaemonServe(t *testing.T) { require.NoError(t, err) err = srv.DRPCConn().Close() require.NoError(t, err) - daemons, err := client.ProvisionerDaemons(ctx) - require.NoError(t, err) - require.Len(t, daemons, 1) }) t.Run("PSK_daily_cost", func(t *testing.T) { @@ -347,9 +330,6 @@ func TestProvisionerDaemonServe(t *testing.T) { var apiError *codersdk.Error require.ErrorAs(t, err, &apiError) require.Equal(t, http.StatusForbidden, apiError.StatusCode()) - daemons, err := client.ProvisionerDaemons(ctx) - require.NoError(t, err) - require.Len(t, daemons, 0) }) t.Run("NoAuth", func(t *testing.T) { @@ -378,9 +358,6 @@ func TestProvisionerDaemonServe(t *testing.T) { var apiError *codersdk.Error require.ErrorAs(t, err, &apiError) require.Equal(t, http.StatusForbidden, apiError.StatusCode()) - daemons, err := client.ProvisionerDaemons(ctx) - require.NoError(t, err) - require.Len(t, daemons, 0) }) t.Run("NoPSK", func(t *testing.T) { @@ -409,8 +386,5 @@ func TestProvisionerDaemonServe(t *testing.T) { var apiError *codersdk.Error require.ErrorAs(t, err, &apiError) require.Equal(t, http.StatusForbidden, apiError.StatusCode()) - daemons, err := client.ProvisionerDaemons(ctx) - require.NoError(t, err) - require.Len(t, daemons, 0) }) }