Address review comments

coder · ammario · Mar 31, 2023 · Mar 14, 2023 · Mar 15, 2023 · Mar 15, 2023
commit dc631f5fa1a85a391a50d430ae08383580b7e1f3
diff --git a/agent/agent.go b/agent/agent.go
@@ -214,9 +214,8 @@ func (a *agent) collectMetadata(ctx context.Context, md codersdk.WorkspaceAgentM
 	if timeout == 0 {
 		timeout = md.Interval
 	}
-	ctx, cancel := context.WithDeadline(ctx, time.Now().Add(
+	ctx, cancel := context.WithTimeout(ctx,
 		time.Duration(timeout)*time.Second,
-	),
 	)
 	defer cancel()
 
@@ -295,63 +294,65 @@ func (a *agent) reportMetadataLoop(ctx context.Context) {
 				a.logger.Error(ctx, "report metadata", slog.Error(err))
 			}
 		case <-baseTicker.C:
-			if len(metadataResults) > cap(metadataResults)/2 {
-				// If we're backpressured on sending back results, we risk
-				// runaway goroutine growth and/or overloading coderd. So,
-				// we just skip the collection. Since we never update
-				// the collections map, we'll retry the collection
-				// on the next tick.
-				a.logger.Debug(
-					ctx, "metadata collection backpressured",
-					slog.F("queue_len", len(metadataResults)),
-				)
-				continue
-			}
+			break
+		}
+
+		if len(metadataResults) > cap(metadataResults)/2 {
+			// If we're backpressured on sending back results, we risk
+			// runaway goroutine growth and/or overloading coderd. So,
+			// we just skip the collection. Since we never update
+			// the collections map, we'll retry the collection
+			// on the next tick.
+			a.logger.Debug(
+				ctx, "metadata collection backpressured",
+				slog.F("queue_len", len(metadataResults)),
+			)
+			continue
+		}
 
-			manifest := a.manifest.Load()
-			if manifest == nil {
-				continue
+		manifest := a.manifest.Load()
+		if manifest == nil {
+			continue
+		}
+		// If the manifest changes (e.g. on agent reconnect) we need to
+		// purge old cache values to prevent lastCollectedAt from growing
+		// boundlessly.
+		for key := range lastCollectedAts {
+			if slices.IndexFunc(manifest.Metadata, func(md codersdk.WorkspaceAgentMetadataDescription) bool {
+				return md.Key == key
+			}) < 0 {
+				delete(lastCollectedAts, key)
 			}
-			// If the manifest changes (e.g. on agent reconnect) we need to
-			// purge old cache values to prevent lastCollectedAt from growing
-			// boundlessly.
-			for key := range lastCollectedAts {
-				if slices.IndexFunc(manifest.Metadata, func(md codersdk.WorkspaceAgentMetadataDescription) bool {
-					return md.Key == key
-				}) < 0 {
-					delete(lastCollectedAts, key)
+		}
+
+		// Spawn a goroutine for each metadata collection, and use a
+		// channel to synchronize the results and avoid both messy
+		// mutex logic and overloading the API.
+		for _, md := range manifest.Metadata {
+			collectedAt, ok := lastCollectedAts[md.Key]
+			if ok {
+				// If the interval is zero, we assume the user just wants
+				// a single collection at startup, not a spinning loop.
+				if md.Interval == 0 {
+					continue
+				}
+				if collectedAt.Add(
+					convertInterval(md.Interval),
+				).After(time.Now()) {
+					continue
 				}
 			}
 
-			// Spawn a goroutine for each metadata collection, and use a
-			// channel to synchronize the results and avoid both messy
-			// mutex logic and overloading the API.
-			for _, md := range manifest.Metadata {
-				collectedAt, ok := lastCollectedAts[md.Key]
-				if ok {
-					// If the interval is zero, we assume the user just wants
-					// a single collection at startup, not a spinning loop.
-					if md.Interval == 0 {
-						continue
-					}
-					if collectedAt.Add(
-						convertInterval(md.Interval),
-					).After(time.Now()) {
-						continue
-					}
+			go func(md codersdk.WorkspaceAgentMetadataDescription) {
+				select {
+				case <-ctx.Done():
+					return
+				case metadataResults <- metadataResultAndKey{
+					key:    md.Key,
+					result: a.collectMetadata(ctx, md),
+				}:
 				}
-
-				go func(md codersdk.WorkspaceAgentMetadataDescription) {
-					select {
-					case <-ctx.Done():
-						return
-					case metadataResults <- metadataResultAndKey{
-						key:    md.Key,
-						result: a.collectMetadata(ctx, md),
-					}:
-					}
-				}(md)
-			}
+			}(md)
 		}
 	}
 }
@@ -1077,7 +1078,7 @@ func (a *agent) init(ctx context.Context) {
 }
 
 // createCommand processes raw command input with OpenSSH-like behavior.
-// If the rawScript provided is empty, it will default to the users shell.
+// If the createCommand provided is empty, it will default to the users shell.
 // This injects environment variables specified by the user at launch too.
 func (a *agent) createCommand(ctx context.Context, script string, env []string) (*exec.Cmd, error) {
 	currentUser, err := user.Current()

diff --git a/coderd/coderd.go b/coderd/coderd.go
@@ -603,6 +603,9 @@ func New(options *Options) *API {
 			r.Route("/me", func(r chi.Router) {
 				r.Use(httpmw.ExtractWorkspaceAgent(options.Database))
 				r.Get("/manifest", api.workspaceAgentManifest)
+				// This route is deprecated and will be removed in a future release.
+				// New agents will use /me/manifest instead.
+				r.Get("/metadata", api.workspaceAgentManifest)
 				r.Post("/startup", api.postWorkspaceAgentStartup)
 				r.Patch("/startup-logs", api.patchWorkspaceAgentStartupLogs)
 				r.Post("/app-health", api.postWorkspaceAppHealth)

diff --git a/coderd/database/dbauthz/querier.go b/coderd/database/dbauthz/querier.go
@@ -1556,13 +1556,9 @@ func (q *querier) InsertWorkspaceAgentStat(ctx context.Context, arg database.Ins
 }
 
 func (q *querier) InsertWorkspaceAgentMetadata(ctx context.Context, arg database.InsertWorkspaceAgentMetadataParams) error {
-	workspace, err := q.db.GetWorkspaceByAgentID(ctx, arg.WorkspaceAgentID)
-	if err != nil {
-		return xerrors.Errorf("find workspace by agent %v: %v", arg.WorkspaceAgentID, err)
-	}
-
-	err = q.authorizeContext(ctx, rbac.ActionUpdate, workspace)
-	if err != nil {
+	// We don't check for workspace ownership here since the agent metadata may
+	// be associated with an orphaned agent used by a dry run build.
+	if err := q.authorizeContext(ctx, rbac.ActionCreate, rbac.ResourceSystem); err != nil {
 		return err
 	}
 

diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql
diff --git a/coderd/database/migrations/000111_workspace_agent_metadata.up.sql b/coderd/database/migrations/000111_workspace_agent_metadata.up.sql
@@ -2,11 +2,11 @@
 -- key enough?
 CREATE TABLE workspace_agent_metadata (
     workspace_agent_id uuid NOT NULL,
-    display_name text NOT NULL,
-    key character varying(128) NOT NULL,
-    script text NOT NULL,
-    value text NOT NULL DEFAULT '',
-    error text NOT NULL DEFAULT '',
+    display_name varchar(127) NOT NULL,
+    key varchar(127) NOT NULL,
+    script varchar(65535) NOT NULL,
+    value varchar(65535) NOT NULL DEFAULT '',
+    error varchar(65535) NOT NULL DEFAULT '',
     timeout bigint NOT NULL,
     interval bigint NOT NULL,
     collected_at timestamp with time zone NOT NULL DEFAULT '0001-01-01 00:00:00+00',

diff --git a/coderd/provisionerdserver/provisionerdserver.go b/coderd/provisionerdserver/provisionerdserver.go
@@ -1276,27 +1276,21 @@ func InsertWorkspaceResource(ctx context.Context, db database.Store, jobID uuid.
 		}
 		snapshot.WorkspaceAgents = append(snapshot.WorkspaceAgents, telemetry.ConvertWorkspaceAgent(dbAgent))
 
-		// We don't need to insert metadata if the agent is not for a workspace.
-		// This is probably an agent made during the provisioning process.
-		_, err = db.GetWorkspaceByAgentID(ctx, agentID)
-		if err == nil {
-			for _, md := range prAgent.Metadata {
-				p := database.InsertWorkspaceAgentMetadataParams{
-					WorkspaceAgentID: agentID,
-					DisplayName:      md.DisplayName,
-					Script:           md.Script,
-					Key:              md.Key,
-					Timeout:          md.Timeout,
-					Interval:         md.Interval,
-				}
-				err := db.InsertWorkspaceAgentMetadata(ctx, p)
-				if err != nil {
-					return xerrors.Errorf("insert agent metadata: %w, params: %+v", err, p)
-				}
+		for _, md := range prAgent.Metadata {
+			p := database.InsertWorkspaceAgentMetadataParams{
+				WorkspaceAgentID: agentID,
+				DisplayName:      md.DisplayName,
+				Script:           md.Script,
+				Key:              md.Key,
+				Timeout:          md.Timeout,
+				Interval:         md.Interval,
+			}
+			err := db.InsertWorkspaceAgentMetadata(ctx, p)
+			if err != nil {
+				return xerrors.Errorf("insert agent metadata: %w, params: %+v", err, p)
 			}
-		} else if !errors.Is(err, sql.ErrNoRows) {
-			return xerrors.Errorf("get workspace by agent ID: %w", err)
 		}
+
 		for _, app := range prAgent.Apps {
 			slug := app.Slug
 			if slug == "" {

diff --git a/coderd/workspaceagents.go b/coderd/workspaceagents.go
@@ -1373,13 +1373,6 @@ func (api *API) workspaceAgentReportStats(rw http.ResponseWriter, r *http.Reques
 	})
 }
 
-func ellipse(s string, maxLength int) string {
-	if len(s) > maxLength {
-		return s[:maxLength] + "..."
-	}
-	return s
-}
-
 // @Summary Submit workspace agent metadata
 // @ID submit-workspace-agent-metadata
 // @Security CoderSessionToken
@@ -1411,12 +1404,30 @@ func (api *API) workspaceAgentPostMetadata(rw http.ResponseWriter, r *http.Reque
 
 	key := chi.URLParam(r, "key")
 
+	const (
+		maxValueLen = 64 << 10
+		maxErrorLen = maxValueLen
+	)
+
+	metadataError := req.Error
+
+	// We overwrite the error if the provided payload is too long.
+	if len(req.Value) > maxValueLen {
+		metadataError = fmt.Sprintf("value of %d bytes exceeded %d bytes", len(req.Value), maxValueLen)
+		req.Value = req.Value[:maxValueLen]
+	}
+
+	if len(req.Error) > maxErrorLen {
+		metadataError = fmt.Sprintf("error of %d bytes exceeded %d bytes", len(req.Error), maxErrorLen)
+		req.Error = req.Error[:maxErrorLen]
+	}
+
 	datum := database.UpdateWorkspaceAgentMetadataParams{
 		WorkspaceAgentID: workspaceAgent.ID,
 		// We don't want a misconfigured agent to fill the database.
-		Key:   ellipse(key, 128),
-		Value: ellipse(req.Value, 10<<10),
-		Error: ellipse(req.Error, 10<<10),
+		Key:   key,
+		Value: req.Value,
+		Error: metadataError,
 		// We ignore the CollectedAt from the agent to avoid bugs caused by
 		// clock skew.
 		CollectedAt: time.Now(),
@@ -1434,7 +1445,6 @@ func (api *API) workspaceAgentPostMetadata(rw http.ResponseWriter, r *http.Reque
 		slog.F("workspace", workspace.ID),
 		slog.F("collected_at", datum.CollectedAt),
 		slog.F("key", datum.Key),
-		slog.F("value", ellipse(datum.Value, 5)),
 	)
 
 	err = api.Pubsub.Publish(watchWorkspaceAgentMetadataChannel(workspaceAgent.ID), []byte(datum.Key))
@@ -1535,15 +1545,17 @@ func (api *API) watchWorkspaceAgentMetadata(rw http.ResponseWriter, r *http.Requ
 
 	for {
 		select {
-		case <-refreshTicker.C:
-			// Avoid spamming the DB with reads we know there are no updates. We want
-			// to continue sending updates to the frontend so that "Result.Age"
-			// is always accurate. This way, the frontend doesn't need to
-			// sync its own clock with the backend.
-			sendMetadata(false)
 		case <-senderClosed:
 			return
+		case <-refreshTicker.C:
+			break
 		}
+
+		// Avoid spamming the DB with reads we know there are no updates. We want
+		// to continue sending updates to the frontend so that "Result.Age"
+		// is always accurate. This way, the frontend doesn't need to
+		// sync its own clock with the backend.
+		sendMetadata(false)
 	}
 }
 

diff --git a/coderd/workspaceagents_test.go b/coderd/workspaceagents_test.go
@@ -1291,6 +1291,13 @@ func TestWorkspaceAgent_Metadata(t *testing.T) {
 									Interval:    10,
 									Timeout:     3,
 								},
+								{
+									DisplayName: "TooLong",
+									Key:         "foo3",
+									Script:      "echo howdy",
+									Interval:    10,
+									Timeout:     3,
+								},
 							},
 							Id: uuid.NewString(),
 							Auth: &proto.Agent_Token{
@@ -1370,25 +1377,35 @@ func TestWorkspaceAgent_Metadata(t *testing.T) {
 	}
 
 	update = recvUpdate()
-	require.Len(t, update, 2)
+	require.Len(t, update, 3)
 	check(wantMetadata1, update[0])
 	// The second metadata result is not yet posted.
 	require.Zero(t, update[1].Result.CollectedAt)
 
 	wantMetadata2 := wantMetadata1
 	post("foo2", wantMetadata2)
 	update = recvUpdate()
-	require.Len(t, update, 2)
+	require.Len(t, update, 3)
 	check(wantMetadata1, update[0])
 	check(wantMetadata2, update[1])
 
 	wantMetadata1.Error = "error"
 	post("foo1", wantMetadata1)
 	update = recvUpdate()
-	require.Len(t, update, 2)
+	require.Len(t, update, 3)
 	check(wantMetadata1, update[0])
 
-	badMetadata := wantMetadata1
-	err = agentClient.PostMetadata(ctx, "unknown", badMetadata)
+	const maxValueLen = 64 << 10
+	tooLongValueMetadata := wantMetadata1
+	tooLongValueMetadata.Value = strings.Repeat("a", maxValueLen*2)
+	tooLongValueMetadata.Error = ""
+	tooLongValueMetadata.CollectedAt = time.Now()
+	post("foo3", tooLongValueMetadata)
+	got := recvUpdate()[2]
+	require.Len(t, got.Result.Value, maxValueLen)
+	require.NotEmpty(t, got.Result.Error)
+
+	unknownKeyMetadata := wantMetadata1
+	err = agentClient.PostMetadata(ctx, "unknown", unknownKeyMetadata)
 	require.Error(t, err)
 }