chore: track terraform modules in telemetry #15450

hugodutka · 2024-11-08T11:40:42Z

Addresses https://github.com/coder/nexus/issues/35.

This PR:

Adds a workspace_modules table to track modules used by the Terraform provisioner in provisioner jobs.
Adds a module_path column to the workspace_resources table, allowing to identify which module a resource originates from.
Starts pushing this new information into telemetry.

For the person reviewing this PR, do not fret about the 1,500 new lines - ~1,000 of them are auto-generated.

bpmct · 2024-11-08T13:58:53Z

wooo!!!

provisioner/terraform/resources.go

coderd/database/migrations/000274_workspace_modules.up.sql

provisioner/terraform/resources.go

johnstcn · 2024-11-08T17:56:51Z

provisioner/terraform/provision.go

+	if err != nil {
+		return provisionersdk.PlanErrorf("get modules: %s", err)
+	}


So, if we fail to get the list of modules for the build that we only need for telemetry purposes, we fail to build?

Good catch, it'd probably be better to only log an error.

On second thought, after the discussion regarding using modules in places other than telemetry, it may make sense to return an error rather than just log. Otherwise any code using module info would have to take into account that it may be missing.

Just so we're clear, this means that any failure to fetch the modules in the working directory will result in a failure of the whole plan stage.

Right now, as far as I can tell, the only scenario that will trigger this is a malformed modules file. Are there any other possible scenarios I'm missing?

Pair review (blocking): as this is currently only for telemetry, we should not fail to plan if anything goes wrong with extracting modules.

Are there any other possible scenarios I'm missing?

I don't think there are any other scenarios - barring disk read failures and such. In normal operation this should only happen if terraform changes how it stores modules on disk and we upgrade to that version.

dannykopping · 2024-11-11T10:50:01Z

coderd/database/migrations/000274_workspace_modules.up.sql

+CREATE TABLE workspace_modules (
+    id uuid NOT NULL,
+    job_id uuid NOT NULL REFERENCES provisioner_jobs (id) ON DELETE CASCADE,
+    transition workspace_transition NOT NULL,


Do we need to capture the module for all transitions? What's the value of this?

I mirrored workspace_resources. I think for template import jobs you get resources that reference the same provisioner job but have different workspace transitions. My thinking was if you'd like to associate a specific resource from a template import job with its module, you'd need to know which transition it came from.

coderd/database/migrations/000274_workspace_modules.up.sql

dannykopping · 2024-11-11T10:56:59Z

provisioner/terraform/modules.go

@@ -0,0 +1,64 @@
+package terraform


I'm curious about this.

We extract all other resources from the template by executing terraform graph (see provisioner/terraform/executor.go -> graph()) and then pass that to provisioner/terraform/resources.go -> ConvertState(). From there we retrieve all the template's resources, and they're then persisted.

Did this approach not work for modules?

I couldn't find either module versions or their sources inside the state. This was the first approach I tried, and I switched to reading files only when I couldn't make it work.

This is the output from a terraform graph run locally with a module specified:

$ terraform graph digraph G { ... "module.jetbrains_gateway.coder_app.gateway" -> "coder_agent.main"; "module.jetbrains_gateway.coder_app.gateway" -> "module.jetbrains_gateway.data.coder_parameter.jetbrains_ide"; "module.jetbrains_gateway.coder_app.gateway" -> "module.jetbrains_gateway.data.coder_workspace.me"; "module.jetbrains_gateway.coder_app.gateway" -> "module.jetbrains_gateway.data.coder_workspace_owner.me"; "module.jetbrains_gateway.coder_app.gateway" -> "module.jetbrains_gateway.data.http.jetbrains_ide_versions"; }

Yeah, the graph only displays the module names specified by the user in the template. It doesn’t show the source, like registry.coder.com/modules/jetbrains-gateway/coder, or the version, like 1.0.23. That information is available in the files on disk.

coderd/database/migrations/000274_workspace_modules.up.sql

johnstcn · 2024-11-11T11:11:12Z

Thought: I can imagine a valid use-case for storing module data separate to telemetry -- showing in the UI which module a resource came from is probably useful from a template editor / admin perspective when trying to answer the question "Which module did this resource come from?"

coderd/provisionerdserver/provisionerdserver_test.go

hugodutka · 2024-11-13T12:56:03Z

@johnstcn I addressed your comments:

changed the invalid module path string to a static sentinel string
obfuscated module source and version for modules outside of registry.coder.com
changed the error message when we fail to parse a terraform address

johnstcn

I have a few more comments and issues I'd like to see clarified before merging this.

johnstcn · 2024-11-14T15:47:01Z

provisionerd/runner/runner.go

@@ -1033,6 +1038,7 @@ func (r *Runner) runWorkspaceBuild(ctx context.Context) (*proto.CompletedJob, *p
 				State:     applyComplete.State,
 				Resources: applyComplete.Resources,
 				Timings:   applyComplete.Timings,
+				Modules:   planComplete.Modules,


Why are we using modules from the plan response here? Will these be different from the apply response?

The module files on disk are created by terraform init. init is called only by plan here, so modules at the time of apply should never be different from those at the time of plan. I admit this is highly non-obvious. I'll add a comment about it.

Thanks, a comment would definitely help. From someone else's perspective, this simply looks like a typo.

johnstcn · 2024-11-14T15:48:13Z

provisioner/terraform/resources.go

+				// the database, a null value in WorkspaceResource's ModulePath
+				// indicates "this resource was created before module paths
+				// were tracked."
+				modulePath = "FAILED_TO_PARSE_TERRAFORM_ADDRESS"


Could we extract this to a constant we can refer to elsewhere?

EDIT: it might actually be even better as a sentinel error ErrInvalidTerraformAddr

johnstcn · 2024-11-14T15:57:33Z

provisioner/terraform/provision_test.go

+			Files: map[string]string{
+				"main.tf": `module "hello" {
+                    source = "./module"
+                  }`,
+				"module/module.tf": `
+				  resource "null_resource" "example" {}
+
+				  module "there" {
+					source = "./inner_module"
+				  }
+				`,
+				"module/inner_module/inner_module.tf": `
+				  resource "null_resource" "inner_example" {}
+				`,


Can we add a test with a malformed or otherwise invalid module?

provisioner/terraform/provision_test.go

johnstcn · 2024-11-14T20:44:17Z

provisioner/terraform/provision.go

+	if err != nil {
+		return provisionersdk.PlanErrorf("get modules: %s", err)
+	}


Just so we're clear, this means that any failure to fetch the modules in the working directory will result in a failure of the whole plan stage.

Right now, as far as I can tell, the only scenario that will trigger this is a malformed modules file. Are there any other possible scenarios I'm missing?

provisioner/terraform/executor.go

johnstcn · 2024-11-14T20:48:32Z

coderd/provisionerdserver/provisionerdserver.go

+				err = InsertWorkspaceModule(ctx, s.Database, jobID, transition, module, telemetrySnapshot)
+				if err != nil {
+					return nil, xerrors.Errorf("insert module: %w", err)
+				}


nit: you can combine assignment and conditional here:

Suggested change

err = InsertWorkspaceModule(ctx, s.Database, jobID, transition, module, telemetrySnapshot)

if err != nil {

return nil, xerrors.Errorf("insert module: %w", err)

}

if err := InsertWorkspaceModule(ctx, s.Database, jobID, transition, module, telemetrySnapshot); err != nil {

return nil, xerrors.Errorf("insert module: %w", err)

}

coderd/provisionerdserver/provisionerdserver.go

johnstcn · 2024-11-14T20:50:51Z

coderd/database/dbauthz/dbauthz.go

@@ -2666,6 +2666,20 @@ func (q *querier) GetWorkspaceByWorkspaceAppID(ctx context.Context, workspaceApp
 	return fetch(q.log, q.auth, q.db.GetWorkspaceByWorkspaceAppID)(ctx, workspaceAppID)
 }

+func (q *querier) GetWorkspaceModulesByJobID(ctx context.Context, jobID uuid.UUID) ([]database.WorkspaceModule, error) {
+	if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceSystem); err != nil {


note: rbac.ResourceSystem is fine just for telemetry, but we'll probably want to create a separate RBAC resource in future for some of the other use-cases here. Can you create a follow-up issue for this and add a comment referencing it?

johnstcn

Requesting changes based on pair review with @dannykopping.

The major worry we have is that there is the possibility that a failure to read the modules on-disk will result in a failure to Plan(). While a malformed or otherwise problematic modules.json will probably cause Terraform to bail, we shouldn't.

johnstcn · 2024-11-15T09:43:23Z

provisioner/terraform/provision.go

+	if err != nil {
+		return provisionersdk.PlanErrorf("get modules: %s", err)
+	}


Pair review (blocking): as this is currently only for telemetry, we should not fail to plan if anything goes wrong with extracting modules.

johnstcn · 2024-11-15T09:46:45Z

coderd/telemetry/telemetry_test.go

@@ -119,6 +123,28 @@ func TestTelemetry(t *testing.T) {
 		require.Len(t, snapshot.Users, 1)
 		require.Equal(t, snapshot.Users[0].EmailHashed, "bb44bf07cf9a2db0554bba63a03d822c927deae77df101874496df5a6a3e896d@coder.com")
 	})
+	t.Run("HashedModule", func(t *testing.T) {
+		t.Parallel()
+		db := dbmem.New()


Can we used dbtestutil.NewDB here instead? Also, less work for you later when dbmem goes away ;-)

johnstcn · 2024-11-15T09:56:50Z

coderd/telemetry/telemetry.go

@@ -661,6 +676,26 @@ func ConvertWorkspaceResourceMetadata(metadata database.WorkspaceResourceMetadat
 	}
 }

+func ConvertWorkspaceModule(module database.WorkspaceModule) WorkspaceModule {
+	isCoderModule := strings.Contains(module.Source, "registry.coder.com")


We should extract this to a top-level function. If we have multiple registries in the future we may need to add to this ~~exclusion~~ inclusion list.

coderd/telemetry/telemetry_test.go

coderd/database/migrations/000274_workspace_modules.up.sql

coderd/provisionerdserver/provisionerdserver.go

johnstcn · 2024-11-15T10:17:13Z

coderd/database/queries/workspacemodules.sql

+	job_id = $1;
+
+-- name: GetWorkspaceModulesCreatedAfter :many
+SELECT * FROM workspace_modules WHERE created_at > $1;


non-blocking: If we're going to be querying on created_at, do we need an index?

Won't hurt, I'll add it. 😄

johnstcn · 2024-11-15T10:21:07Z

coderd/database/migrations/testdata/fixtures/000274_workspace_modules.up.sql

⚠️ You'll need to rename the migration number. I think you may want 276, but check just before you merge.

…esources

…sources_test.go

hugodutka · 2024-11-16T12:48:44Z

@johnstcn @dannykopping I made changes based on your feedback.

List of changes

Each point references the source comment.

rebase and update migration number
add an index on workspace_modules created_at
add a comment why we use planComplete.Modules instead of applyComplete.Modules. Comment.
convert sentinel string to error
- added a test for it
don’t fail if we can’t get modules from disk
add a test with a malformed or otherwise invalid module
combine assignment and conditional
dbtestutil.NewDB instead of dbmem
extract IsCoderModule to a top-level function
add a follow up issue about RBAC for workspace_modules
- chore: create a separate RBAC resource for WorkspaceModule #15543

johnstcn · 2024-11-16T13:04:17Z

provisioner/terraform/provision_test.go

 	writer := tar.NewWriter(&buffer)
+
+	addedDirs := make(map[string]bool)
 	for name, content := range files {
+		// Add parent directories if they don't exist
+		dir := filepath.Dir(name)
+		if dir != "." && !addedDirs[dir] {
+			err := writer.WriteHeader(&tar.Header{
+				Name:     dir + "/", // Directory names must end with /
+				Mode:     0o755,
+				Typeflag: tar.TypeDir,
+			})
+			require.NoError(t, err)
+			addedDirs[dir] = true
+		}


I found many instances of this function sprinkled around our codebase so I added a helper function for this in #15540

I'll wait for this PR to be merged and then update these tests to use testutil.CreateTar 👍

johnstcn

Thanks for working on this!

github-actions bot assigned hugodutka Nov 8, 2024

hugodutka force-pushed the hugodutka/track-workspace-modules branch from d406ef6 to 67795fc Compare November 8, 2024 11:44

hugodutka marked this pull request as ready for review November 8, 2024 14:04

hugodutka requested review from dannykopping and johnstcn November 8, 2024 14:24

matifali changed the title ~~feat: track terraform modules in telemetry~~ chore: track terraform modules in telemetry Nov 8, 2024

johnstcn reviewed Nov 8, 2024

View reviewed changes

dannykopping reviewed Nov 11, 2024

View reviewed changes

johnstcn reviewed Nov 11, 2024

View reviewed changes

coderd/provisionerdserver/provisionerdserver_test.go Show resolved Hide resolved

hugodutka force-pushed the hugodutka/track-workspace-modules branch from 175c10e to b72b8ef Compare November 14, 2024 10:00

johnstcn reviewed Nov 14, 2024

View reviewed changes

johnstcn requested changes Nov 15, 2024

View reviewed changes

hugodutka added 15 commits November 15, 2024 16:57

add the workspace_modules table, add the module column to workspace_r…

0f713ed

…esources

insert workspace modules on provisioner job completion

82c8a3c

add foreign key to job on workspace_modules

307a57e

don't return the root module

2ae16fc

add and populate the module_path column to workspace_resources

7f2f155

TestCompleteJob - Modules WIP

863235d

TemplateImport CompleteJob Modules test

993f0ab

TestCompleteJob Modules WorkspaceBuild

e8fbd40

add a test for returning modules in provision_test.go, fix test in re…

5c542e5

…sources_test.go

make gen

9ec5ee1

fix dbauthz tests

119a686

add workspace modules and workspace resources's ModulePath to telemetry

3a5a025

add a workspace_modules fixture

3377d22

add a workspace resource's modulePath default to the e2e helper

422b112

add modules default on PlanComplete for e2e tests

46b9b36

hugodutka added 7 commits November 15, 2024 16:57

lint

f3f4d5c

set module path to sentinel value when parsing failed and log an error

93ed136

change error string

301a153

obfuscate workspace module source and version

212e7d5

fixes after rebase

e9b7c46

change migration number

25cba6d

change fixture number

780730f

hugodutka force-pushed the hugodutka/track-workspace-modules branch from b72b8ef to 780730f Compare November 15, 2024 16:59

hugodutka added 10 commits November 15, 2024 17:00

add index on created_at

b47a018

add a comment about using modules from plan instead of apply

03be52e

convert sentinel string to an error

00b8131

remove unnecessary comment

4187fbf

don't fail if we can't get modules from disk

7e82c3d

add a test for a malformed module

42e57ca

add a comment

368fc25

combine assignment and conditional

73b22cf

make isCoderModule into a top level function

7b9d70a

use dbtestutil.NewDB instead of dbmem

ea82313

hugodutka mentioned this pull request Nov 16, 2024

chore: create a separate RBAC resource for WorkspaceModule #15543

Open

make gen

8564e9a

combine 2 more assignments and conditionals

d11fc82

johnstcn reviewed Nov 16, 2024

View reviewed changes

johnstcn approved these changes Nov 16, 2024

View reviewed changes

hugodutka merged commit aa0dc2d into main Nov 16, 2024
29 checks passed

hugodutka deleted the hugodutka/track-workspace-modules branch November 16, 2024 20:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: track terraform modules in telemetry #15450

chore: track terraform modules in telemetry #15450

hugodutka commented Nov 8, 2024 •

edited

Loading

bpmct commented Nov 8, 2024

johnstcn Nov 8, 2024

hugodutka Nov 11, 2024

hugodutka Nov 11, 2024

johnstcn Nov 14, 2024 •

edited

Loading

johnstcn Nov 15, 2024

hugodutka Nov 15, 2024

dannykopping Nov 11, 2024

hugodutka Nov 11, 2024

dannykopping Nov 11, 2024

hugodutka Nov 11, 2024

dannykopping Nov 11, 2024

hugodutka Nov 11, 2024

johnstcn commented Nov 11, 2024

hugodutka commented Nov 13, 2024

johnstcn left a comment

johnstcn Nov 14, 2024

hugodutka Nov 15, 2024

johnstcn Nov 15, 2024

johnstcn Nov 14, 2024 •

edited

Loading

johnstcn Nov 14, 2024

johnstcn Nov 14, 2024 •

edited

Loading

johnstcn Nov 14, 2024

johnstcn Nov 14, 2024 •

edited

Loading

johnstcn left a comment

johnstcn Nov 15, 2024

johnstcn Nov 15, 2024

johnstcn Nov 15, 2024 •

edited

Loading

johnstcn Nov 15, 2024

hugodutka Nov 15, 2024

johnstcn Nov 15, 2024

hugodutka commented Nov 16, 2024

johnstcn Nov 16, 2024 •

edited

Loading

johnstcn left a comment

chore: track terraform modules in telemetry #15450

chore: track terraform modules in telemetry #15450

Conversation

hugodutka commented Nov 8, 2024 • edited Loading

bpmct commented Nov 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnstcn Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnstcn commented Nov 11, 2024

hugodutka commented Nov 13, 2024

johnstcn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnstcn Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnstcn Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnstcn Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

johnstcn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnstcn Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hugodutka commented Nov 16, 2024

List of changes

johnstcn Nov 16, 2024 • edited Loading

Choose a reason for hiding this comment

johnstcn left a comment

Choose a reason for hiding this comment

hugodutka commented Nov 8, 2024 •

edited

Loading

johnstcn Nov 14, 2024 •

edited

Loading

johnstcn Nov 14, 2024 •

edited

Loading

johnstcn Nov 14, 2024 •

edited

Loading

johnstcn Nov 14, 2024 •

edited

Loading

johnstcn Nov 15, 2024 •

edited

Loading

johnstcn Nov 16, 2024 •

edited

Loading