feat!: extract provisioner tags from coder_workspace_tags data source by johnstcn · Pull Request #15578 · coder/coder

johnstcn · 2024-11-18T22:26:20Z

Extracts provisioner job tags from coder_workspace_tags on template version creation using provisioner/terraform/tfparse added in feat(provisioner/terraform/tfparse): implement WorkspaceTagDefaultsFromFile #15236
Drops a WARN log in coderd if no matching provisioners found.
Also drops a warning message in the CLI if no provisioners are found.
To support both CLI and UI warnings, added a codersdk.MatchedProvisioners struct to the TemplateVersion response containing details of how many provisioners were around at the time of the insert.

NOTE: As this effectively blocks template creation if coder_workspace_tags is malformed or references anything other than a Terraform variable or coder_parameter, I am marking this as a breaking change.

Testing notes:

I elected to forego standing up a Terraform provisioner in coderdtest in favour of just creating the template version and asserting that a provisioner job was created with the expected tags.

Checklist:

API updates: extract tags from uploaded file_id, apply tags to provisioner job
CLI updates: show warning if no provisioners available upon template version creation

Manual testing performed:

./develop.sh -- --provisioner-daemons=0 with a separate ./scripts/coder-dev.sh --key=foobar where foobar.keys = {"foo": "bar"}
Edited sample docker template with following:

data coder_parameter "foo" {
  name = "foo"
  type = "string"
  default = "bar"
}
data coder_workspace_tags "tags" {
  tags = {
    "foo": data.coder_parameter.foo.value
  }
}

Ran coder template push and observed the job get picked up
Checked in local database and observed the correct row in template_version_workspace_tags:

coder=# select * from template_version_workspace_tags where template_version_id = 'e2406123-a492-4bc3-a41a-811aba7536c2';
         template_version_id          | key |             value              
--------------------------------------+-----+--------------------------------
 e2406123-a492-4bc3-a41a-811aba7536c2 | foo | data.coder_parameter.foo.value

Ran coder create and observed the job getting picked up correctly.
Failure case: coder templates push <template> with no workspace tags available:

> Upload "examples/templates/docker"? (yes/no) yes
WARN: No provisioners are available to handle the job!
Please contact your deployment administrator for assistance.
Details:
        Provisioner Job ID : e0808861-62e0-40e9-b45b-817d8fede28e
        Requested tags     : {"foo":"baz","owner":"","scope":"organization"}


==> ⧗ Queued (next)

Failure case: create template but provisioners have not been around for a while:

WARN: All available provisioner daemons have been silent for a while.
Your build will proceed once they become available.
If this persists, please contact your deployment administrator for assistance.
Details:
        Provisioner Job ID : e8b5abee-b325-40b5-8571-3f6c486445f7
        Requested tags     : {"foo":"bar","owner":"","scope":"organization"}
        Most Recently Seen : 2024-11-22 17:29:00.862597 +0000 UTC

Remaining work:

Currently the interactive template editor UI does not handle any case where POST /api/v2/organizations/:org/templateversions returns an error. This will need to be addressed in a follow-up PR.
Update documentation -- will be addressed in a separate PR.
UI updates: show warning if no provisioners available upon template version creation (should be handled by Improve UX when there is no corresponding provisioner for a workspace/template #15048)

…when creating a template version

…space-tags-plumbing

johnstcn · 2024-11-21T09:49:47Z

+	// Tag order precedence:
+	// 1) User-specified tags in the request
+	// 2) Tags parsed from coder_workspace_tags data source in template file
+	// 2 may clobber 1.
+	tags := provisionersdk.MutateTags(apiKey.UserID, req.ProvisionerTags, parsedTags)


TBD: Which is the desired order here? 1 -> 2, 2-> 1, or 1 XOR 2?

Wouldn't it be a potential security issue if a user could override the template tags? The user could then swap to a provisioner they shouldn't be able to access. Just a thought.

As long as a user can't modify owner or scope, I think being able to override tags is a feature. It's how I would get a workspace to be provisioned using a provisioner in my specific region if the template defaults to a US region provisioner for example. And MutateTags protects the owner and scope tags afaik.

Although the template may well have a tag sourced from a variable for this.

SasSwart · 2024-11-22T06:55:50Z

Bit of an essay 🙈:

There's some duplication (checkProvisioners) in this PR that feels likely to drift over time. We do the same provisioner check in the CLI and the API via copied, unexported functions. On the API side we just log and have opted not to send warnings. On the CLI side we warn the user. Then, in my PR, we do the same check but in typescript land and warn with alerts.

Yesterday, I was in favour of the CLI and frontend checks for provisioners instead of warnings, because:

I hadn't considered logging as a third place where this check is done
at that point the checks in the endpoint weren't as robust. You've made them much nicer though.

Currently, it looks to me like it would actually be better to use your more robust endpoint-side check and send a warning, instead of doing the same check in three places. That would eliminate duplication not only in your CLI command but also in my frontend alerts. Another key aspect of this is the configurable health check interval that I can't currently access from the frontend. This might be more accessible in the API than in the other places.

mafredri

Nice work!

mafredri · 2024-11-22T12:22:58Z

+	// Tag order precedence:
+	// 1) User-specified tags in the request
+	// 2) Tags parsed from coder_workspace_tags data source in template file
+	// 2 may clobber 1.
+	tags := provisionersdk.MutateTags(apiKey.UserID, req.ProvisionerTags, parsedTags)


Wouldn't it be a potential security issue if a user could override the template tags? The user could then swap to a provisioner they shouldn't be able to access. Just a thought.

mafredri · 2024-11-22T12:33:07Z

+	}
+}`,
+				},
+				wantTags: map[string]string{"owner": "", "scope": "organization"},


Should this be an error? Silently replacing seems like a recipe for confusion.

I was debating that, but you've sold me. 👍

I might make this a follow-up, as the existing behaviour of provisionersdk.MutateTags is to overwrite provisionersdk.TagOwner

https://github.com/coder/coder/pull/15518/files#diff-01b662bbd7ba1a97489f36a381633c882cc2126dab8fb9ede81d3cb9239dd035R36-R40

johnstcn · 2024-11-22T14:47:36Z

Wouldn't it be a potential security issue if a user could override the template tags? The user could then swap to a provisioner they shouldn't be able to access. Just a thought.

At this point, the user still needs to be a template admin.
I want them to still have the option to specify the provisioner job tags for importing the template just in case they either

a) don't want to use coder_workspace_tags, or
b) need to override the default values they are specifying in coder_workspace_tags.

Right now there's no field in codersdk.CreateWorkspaceBuildRequest to override the provisioner tags when creating a workspace. So I think this should be fine.

johnstcn · 2024-11-22T17:40:09Z


-	Warnings []TemplateVersionWarning `json:"warnings,omitempty" enums:"DEPRECATED_PARAMETERS"`
+	Warnings            []TemplateVersionWarning `json:"warnings,omitempty" enums:"DEPRECATED_PARAMETERS"`
+	MatchedProvisioners MatchedProvisioners      `json:"matched_provisioners,omitempty"`


review: I considered adding as a field on the ProvisionerJob but it did not seem 100% correct.

However, as it does not make sense to return this from e.g. getting a previous template version, it may make sense instead to make this nullable (e.g. *MatchedProvisioners). Thoughts?

From what I could see last week, the only way to see tags for a workspace build is to get that workspace's template version. Therefore, I think we need this even for previous template versions.

Looked at where else we use this payload. You're right, it isn't necessary elsewhere. That means we could make it nullable. Whether that would be better for API clients, I'm not sure. Would be happy if you made it nullable. Would be happy if you left it as is.

I'll keep it as-is for now so you can build on top of it. It should be a fairly simple refactor to make it a nullable field later.

johnstcn · 2024-11-22T17:43:00Z

+		return codersdk.MatchedProvisioners{}, xerrors.Errorf("provisioner daemons by organization: %w", err)
+	}
+
+	threePollsAgo := time.Now().Add(-3 * pollInterval)


review: this is duplicated from / in line with coderd/healthcheck:

if opts.StaleInterval == 0 { opts.StaleInterval = provisionerdserver.DefaultHeartbeatInterval * 3 }

Do we need the above as a code comment or perhaps a test that checks that they remain in sync?

I'm not sure of the value of adding a test for this, but we can possibly refactor where it's defined in a follow-up so that it's consistent. For now, I'll add a comment 👍

On the surface it kinda makes sense to use the same logic (3*poll), but at the same time I feel multiplying by 3 is pretty extreme, I'm more in favor of a fixed time like poll+30s or something like that. In this instance poll+30s would surface the issue in half the time. If poll is ever raised, to say 2min, the multiplication would go from 1.5min to 3.5min longer to surface the issue, etc.

SasSwart

Looks good. There are some comments left, but none of them block merge IMO. I'm going to be actively consuming this shortly, so I'll get to nits in a subsequent PR if we're happy to do that.

mafredri

Minor nits, and not sold on the three-polls duration (I feel that's longer than what was discussed previously), but otherwise LGTM. 👍🏻

mafredri · 2024-11-25T10:42:42Z

+		return codersdk.MatchedProvisioners{}, xerrors.Errorf("provisioner daemons by organization: %w", err)
+	}
+
+	threePollsAgo := time.Now().Add(-3 * pollInterval)


On the surface it kinda makes sense to use the same logic (3*poll), but at the same time I feel multiplying by 3 is pretty extreme, I'm more in favor of a fixed time like poll+30s or something like that. In this instance poll+30s would surface the issue in half the time. If poll is ever raised, to say 2min, the multiplication would go from 1.5min to 3.5min longer to surface the issue, etc.

mafredri · 2024-11-25T10:43:46Z

+	// MostRecentlySeen is the most recently seen time of the set of matched
+	// provisioners. If no provisioners matched, this field will be null.
+	MostRecentlySeen NullTime `json:"most_recently_seen,omitempty" format:"date-time"`
+}


johnstcn · 2024-11-25T10:53:10Z

On the surface it kinda makes sense to use the same logic (3*poll), but at the same time I feel multiplying by 3 is pretty extreme, I'm more in favor of a fixed time like poll+30s or something like that. In this instance poll+30s would surface the issue in half the time. If poll is ever raised, to say 2min, the multiplication would go from 1.5min to 3.5min longer to surface the issue, etc.

That's fair; I'd rather have it be consistent though. I'll open a follow-up PR to adjust this.

Co-authored-by: Mathias Fredriksson <mafredri@gmail.com>

Adds documentation relating to #15087 and #15427 See also: #15578

…15643) Follow-up from #15578 Extracts `provisionerdserver.StaleInterval` and sets it to 90 seconds by default

…15643) Follow-up from #15578 Extracts `provisionerdserver.StaleInterval` and sets it to 90 seconds by default (cherry picked from commit ef09b51)

feat: extract provisioner tags from coder_workspace_tags data source …

944eb3a

…when creating a template version

johnstcn self-assigned this Nov 18, 2024

johnstcn marked this pull request as draft November 18, 2024 22:26

johnstcn changed the title ~~feat: extract provisioner tags from coder_workspace_tags data source when creating a template version~~ feat: extract provisioner tags from coder_workspace_tags data source Nov 18, 2024

johnstcn added 6 commits November 20, 2024 10:12

Merge remote-tracking branch 'origin/main' into cj/15087-extract-work…

7bc2fc8

…space-tags-plumbing

add initial framework for API tests

4653638

Merge remote-tracking branch 'origin/main' into cj/15087-extract-work…

fc8ff56

…space-tags-plumbing

update tests

85c2c8f

drop warn log

2e9526b

var naming

01c8e99

johnstcn changed the title ~~feat: extract provisioner tags from coder_workspace_tags data source~~ feat!: extract provisioner tags from coder_workspace_tags data source Nov 20, 2024

johnstcn added 3 commits November 20, 2024 18:06

adjust control flow in error case

5a53d86

show warning in templates push

c1e321d

bump test timeout

ed534bd

johnstcn commented Nov 21, 2024

View reviewed changes

Comment thread coderd/templateversions.go Outdated

johnstcn commented Nov 21, 2024

View reviewed changes

johnstcn added 2 commits November 21, 2024 11:57

improve test coverage

42f39a7

fix tests

c6f33f7

johnstcn marked this pull request as ready for review November 21, 2024 16:11

johnstcn added 2 commits November 21, 2024 17:24

move to client-side checking for warnings

b22e890

make gen

a8a620f

johnstcn requested review from DanielleMaywood, SasSwart and mafredri November 21, 2024 17:26

johnstcn mentioned this pull request Nov 21, 2024

chore(docs): update documentation for coder_workspace_tags #15620

Merged

johnstcn commented Nov 22, 2024

View reviewed changes

Comment thread coderd/templateversions.go Outdated

mafredri reviewed Nov 22, 2024

View reviewed changes

johnstcn added 2 commits November 22, 2024 14:27

address indentation

8ec672c

use CacheDir for tfparse instead

7b7aa40

johnstcn added 3 commits November 22, 2024 17:10

feat: add codersdk.MatchedProvisioners

80dc6c2

update cli template push

5631577

cleanup and error handling

58fae97

johnstcn commented Nov 22, 2024

View reviewed changes

SasSwart approved these changes Nov 25, 2024

View reviewed changes

mafredri approved these changes Nov 25, 2024

View reviewed changes

Apply suggestions from code review

90991bd

Co-authored-by: Mathias Fredriksson <mafredri@gmail.com>

johnstcn merged commit 1cdc3e8 into main Nov 25, 2024

johnstcn deleted the cj/15087-extract-workspace-tags-plumbing branch November 25, 2024 11:19

johnstcn mentioned this pull request Nov 25, 2024

fix(coderd): extract provisionerdserver.StaleInterval to 90 seconds #15643

Merged

johnstcn added a commit that referenced this pull request Nov 25, 2024

chore(docs): update documentation for coder_workspace_tags (#15620)

e550365

Adds documentation relating to #15087 and #15427 See also: #15578

This was referenced Nov 25, 2024

Evaluate coder_workspace_tags during template create/push #15427

Closed

feature: extract coder_workspace_tags from template, detect dangerous data source usage #15087

Closed

johnstcn added a commit that referenced this pull request Nov 28, 2024

fix(coderd): extract provisionerdserver.StaleInterval to 90 seconds (#…

ef09b51

…15643) Follow-up from #15578 Extracts `provisionerdserver.StaleInterval` and sets it to 90 seconds by default

Uh oh!

Conversation

johnstcn commented Nov 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SasSwart Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SasSwart commented Nov 22, 2024

Uh oh!

Uh oh!

mafredri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

johnstcn commented Nov 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

johnstcn Nov 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mafredri Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SasSwart left a comment

Choose a reason for hiding this comment

Uh oh!

mafredri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mafredri Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnstcn commented Nov 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

johnstcn commented Nov 18, 2024 •

edited

Loading

SasSwart Nov 25, 2024 •

edited

Loading

johnstcn commented Nov 22, 2024 •

edited

Loading

johnstcn Nov 22, 2024 •

edited

Loading

mafredri Nov 25, 2024 •

edited

Loading

mafredri Nov 25, 2024 •

edited

Loading