From a4ed1a20ffc87ba33a726a753bd2296cdabeed85 Mon Sep 17 00:00:00 2001 From: Colin Adler Date: Thu, 6 Apr 2023 15:51:50 -0500 Subject: [PATCH] fix: don't query workspace in `UpdateWorkspaceAgentConnectionByID` --- coderd/database/dbauthz/querier.go | 8 -------- coderd/database/dbauthz/querier_test.go | 9 --------- coderd/database/dbauthz/system.go | 7 +++++++ coderd/database/dbauthz/system_test.go | 9 +++++++++ coderd/workspaceagents.go | 14 ++------------ coderd/workspaces.go | 5 ----- 6 files changed, 18 insertions(+), 34 deletions(-) diff --git a/coderd/database/dbauthz/querier.go b/coderd/database/dbauthz/querier.go index 24df1b31bf681..8f9f0033c8585 100644 --- a/coderd/database/dbauthz/querier.go +++ b/coderd/database/dbauthz/querier.go @@ -1552,14 +1552,6 @@ func (q *querier) UpdateWorkspace(ctx context.Context, arg database.UpdateWorksp return updateWithReturn(q.log, q.auth, fetch, q.db.UpdateWorkspace)(ctx, arg) } -func (q *querier) UpdateWorkspaceAgentConnectionByID(ctx context.Context, arg database.UpdateWorkspaceAgentConnectionByIDParams) error { - // TODO: This is a workspace agent operation. Should users be able to query this? - fetch := func(ctx context.Context, arg database.UpdateWorkspaceAgentConnectionByIDParams) (database.Workspace, error) { - return q.db.GetWorkspaceByAgentID(ctx, arg.ID) - } - return update(q.log, q.auth, fetch, q.db.UpdateWorkspaceAgentConnectionByID)(ctx, arg) -} - func (q *querier) InsertWorkspaceAgentStat(ctx context.Context, arg database.InsertWorkspaceAgentStatParams) (database.WorkspaceAgentStat, error) { // TODO: This is a workspace agent operation. Should users be able to query this? // Not really sure what this is for. diff --git a/coderd/database/dbauthz/querier_test.go b/coderd/database/dbauthz/querier_test.go index 2ebd0c5f445da..e54e40e06efe1 100644 --- a/coderd/database/dbauthz/querier_test.go +++ b/coderd/database/dbauthz/querier_test.go @@ -1170,15 +1170,6 @@ func (s *MethodTestSuite) TestWorkspace() { ID: w.ID, }).Asserts(w, rbac.ActionUpdate).Returns(expected) })) - s.Run("UpdateWorkspaceAgentConnectionByID", s.Subtest(func(db database.Store, check *expects) { - ws := dbgen.Workspace(s.T(), db, database.Workspace{}) - build := dbgen.WorkspaceBuild(s.T(), db, database.WorkspaceBuild{WorkspaceID: ws.ID, JobID: uuid.New()}) - res := dbgen.WorkspaceResource(s.T(), db, database.WorkspaceResource{JobID: build.JobID}) - agt := dbgen.WorkspaceAgent(s.T(), db, database.WorkspaceAgent{ResourceID: res.ID}) - check.Args(database.UpdateWorkspaceAgentConnectionByIDParams{ - ID: agt.ID, - }).Asserts(ws, rbac.ActionUpdate).Returns() - })) s.Run("InsertWorkspaceAgentStat", s.Subtest(func(db database.Store, check *expects) { ws := dbgen.Workspace(s.T(), db, database.Workspace{}) check.Args(database.InsertWorkspaceAgentStatParams{ diff --git a/coderd/database/dbauthz/system.go b/coderd/database/dbauthz/system.go index 248159ff5c53b..dd47cb635b080 100644 --- a/coderd/database/dbauthz/system.go +++ b/coderd/database/dbauthz/system.go @@ -348,6 +348,13 @@ func (q *querier) InsertWorkspaceResourceMetadata(ctx context.Context, arg datab return q.db.InsertWorkspaceResourceMetadata(ctx, arg) } +func (q *querier) UpdateWorkspaceAgentConnectionByID(ctx context.Context, arg database.UpdateWorkspaceAgentConnectionByIDParams) error { + if err := q.authorizeContext(ctx, rbac.ActionUpdate, rbac.ResourceSystem); err != nil { + return err + } + return q.db.UpdateWorkspaceAgentConnectionByID(ctx, arg) +} + // TODO: We need to create a ProvisionerJob resource type func (q *querier) AcquireProvisionerJob(ctx context.Context, arg database.AcquireProvisionerJobParams) (database.ProvisionerJob, error) { // if err := q.authorizeContext(ctx, rbac.ActionUpdate, rbac.ResourceSystem); err != nil { diff --git a/coderd/database/dbauthz/system_test.go b/coderd/database/dbauthz/system_test.go index 65cf9d2743605..8400b27407c05 100644 --- a/coderd/database/dbauthz/system_test.go +++ b/coderd/database/dbauthz/system_test.go @@ -230,6 +230,15 @@ func (s *MethodTestSuite) TestSystemFunctions() { WorkspaceResourceID: uuid.New(), }).Asserts(rbac.ResourceSystem, rbac.ActionCreate) })) + s.Run("UpdateWorkspaceAgentConnectionByID", s.Subtest(func(db database.Store, check *expects) { + ws := dbgen.Workspace(s.T(), db, database.Workspace{}) + build := dbgen.WorkspaceBuild(s.T(), db, database.WorkspaceBuild{WorkspaceID: ws.ID, JobID: uuid.New()}) + res := dbgen.WorkspaceResource(s.T(), db, database.WorkspaceResource{JobID: build.JobID}) + agt := dbgen.WorkspaceAgent(s.T(), db, database.WorkspaceAgent{ResourceID: res.ID}) + check.Args(database.UpdateWorkspaceAgentConnectionByIDParams{ + ID: agt.ID, + }).Asserts(rbac.ResourceSystem, rbac.ActionUpdate).Returns() + })) s.Run("AcquireProvisionerJob", s.Subtest(func(db database.Store, check *expects) { // TODO: we need to create a ProvisionerJob resource j := dbgen.ProvisionerJob(s.T(), db, database.ProvisionerJob{ diff --git a/coderd/workspaceagents.go b/coderd/workspaceagents.go index 555020b15239d..c63437486ef94 100644 --- a/coderd/workspaceagents.go +++ b/coderd/workspaceagents.go @@ -22,7 +22,6 @@ import ( "github.com/bep/debounce" "github.com/go-chi/chi/v5" "github.com/google/uuid" - "go.opentelemetry.io/otel/trace" "golang.org/x/exp/slices" "golang.org/x/mod/semver" "golang.org/x/xerrors" @@ -36,7 +35,6 @@ import ( "github.com/coder/coder/coderd/httpapi" "github.com/coder/coder/coderd/httpmw" "github.com/coder/coder/coderd/rbac" - "github.com/coder/coder/coderd/tracing" "github.com/coder/coder/coderd/util/ptr" "github.com/coder/coder/codersdk" "github.com/coder/coder/codersdk/agentsdk" @@ -859,7 +857,8 @@ func (api *API) workspaceAgentCoordinate(rw http.ResponseWriter, r *http.Request } disconnectedAt := workspaceAgent.DisconnectedAt updateConnectionTimes := func(ctx context.Context) error { - err = api.Database.UpdateWorkspaceAgentConnectionByID(ctx, database.UpdateWorkspaceAgentConnectionByIDParams{ + //nolint:gocritic // We only update ourself. + err = api.Database.UpdateWorkspaceAgentConnectionByID(dbauthz.AsSystemRestricted(ctx), database.UpdateWorkspaceAgentConnectionByIDParams{ ID: workspaceAgent.ID, FirstConnectedAt: firstConnectedAt, LastConnectedAt: lastConnectedAt, @@ -920,11 +919,6 @@ func (api *API) workspaceAgentCoordinate(rw http.ResponseWriter, r *http.Request } api.publishWorkspaceUpdate(ctx, build.WorkspaceID) - // End span so we don't get long lived trace data. - tracing.EndHTTPSpan(r, http.StatusOK, trace.SpanFromContext(ctx)) - // Ignore all trace spans after this. - ctx = trace.ContextWithSpan(ctx, tracing.NoopSpan) - api.Logger.Info(ctx, "accepting agent", slog.F("agent", workspaceAgent)) defer conn.Close(websocket.StatusNormalClosure, "") @@ -1356,10 +1350,6 @@ func (api *API) watchWorkspaceAgentMetadata(rw http.ResponseWriter, r *http.Requ <-senderClosed }() - // We don't want this intentionally long request to skew our tracing - // reports. - ctx = trace.ContextWithSpan(ctx, tracing.NoopSpan) - const refreshInterval = time.Second * 5 refreshTicker := time.NewTicker(refreshInterval) defer refreshTicker.Stop() diff --git a/coderd/workspaces.go b/coderd/workspaces.go index 5c48d45d598b8..240095723f836 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -13,7 +13,6 @@ import ( "github.com/go-chi/chi/v5" "github.com/google/uuid" - "go.opentelemetry.io/otel/trace" "golang.org/x/xerrors" "cdr.dev/slog" @@ -26,7 +25,6 @@ import ( "github.com/coder/coder/coderd/schedule" "github.com/coder/coder/coderd/searchquery" "github.com/coder/coder/coderd/telemetry" - "github.com/coder/coder/coderd/tracing" "github.com/coder/coder/coderd/util/ptr" "github.com/coder/coder/codersdk" ) @@ -970,9 +968,6 @@ func (api *API) watchWorkspace(rw http.ResponseWriter, r *http.Request) { <-senderClosed }() - // Ignore all trace spans after this, they're not too useful. - ctx = trace.ContextWithSpan(ctx, tracing.NoopSpan) - sendUpdate := func(_ context.Context, _ []byte) { workspace, err := api.Database.GetWorkspaceByID(ctx, workspace.ID) if err != nil {