feat: run a terraform plan before creating workspaces with the given template parameters by deansheather · Pull Request #1732 · coder/coder

deansheather · 2022-05-25T08:46:07Z

Adds a new provisioner job type template_version_plan which is for running a plan against a template version with custom parameters, but without having to make a workspace first. This will be used in coder create so we can verify that the workspace creation will start successfully and not fail quickly due to bad parameter values (leaving an inconsolable workspace state).

Subtasks

Future:

Detect parameter validation errors and reprompt users instead of making them run the command again
Integrate the plan endpoint into the frontend

Screenshots

Successful validation:

Unsuccessful validation/plan:

Fixes #831

johnstcn

I haven't tested this out myself, but it looks solid to me.

johnstcn · 2022-05-27T19:55:27Z

+	// We use the workspace RBAC check since we don't want to allow plans if the
+	// user can't create workspaces.


Technically correct - the best kind of correct.

johnstcn · 2022-05-27T20:40:10Z

+-- It's not possible to drop enum values from enum types, so the UP has "IF NOT
+-- EXISTS".
+
+-- Delete all jobs that use the new enum value.
+DELETE FROM
+    provisioner_jobs
+WHERE
+    provisioner_job_type = 'template_version_plan'
+;


⚠️ reminder: rename me ⚠️

johnstcn · 2022-05-27T20:40:15Z

+ALTER TYPE provisioner_job_type
+ADD VALUE IF NOT EXISTS 'template_version_plan';


⚠️ reminder: rename me ⚠️

johnstcn · 2022-05-27T20:42:37Z

+				// TODO (Dean): reprompt for parameter values if we deem it to
+				// be a validation error


TODO: open an issue

Opened two issues:

#1872 - for integrating with frontend
#1873 - for reprompting for values in CLI

johnstcn · 2022-05-27T20:43:37Z

 		transition, err := convertWorkspaceTransition(workspaceBuild.Transition)
 		if err != nil {
-			return nil, failJob(fmt.Sprint("convert workspace transition: %w", err))
+			return nil, failJob(fmt.Sprintf("convert workspace transition: %s", err))


No wrapping? I guess this gets protobuffed in the end, right?

Can't wrap with Sprintf, and these just get condensed to strings sadly :(

kylecarbs · 2022-05-31T13:40:47Z

+	var req codersdk.CreateTemplateVersionPlanRequest
+	if !httpapi.Read(rw, r, &req) {
+		return
+	}


As a user, I could spam this ~1000 times and take down a Coder deployment for the day. The same could be said for workspaces, but we have issues tracking quotas for those.

OK so I'll just add details to those issues that whatever fix we do needs to apply to this endpoint too

Could you link the issue? I can't seem to find it after searching "rate limit" and "quota"

Ahh my bad for being slow here.

Can we limit the parallelization here? This is a remarkably easy attack vector that we don't seem to have a solid plan to fix.

That seems out of scope for this PR because we have the same issue with regular workspace builds and template imports which also needs to be fixed. It'd make sense to come up with a good solution that covers all of these job types

In that case, what's the mechanism we use to block multiple workspace builds so we can implement here

Multiple builds cannot occur for the same workspace in parallel because they are stateful.

Do we have any protection across multiple builds across multiple workspaces on the same user?

Nope, but it's less of a problem because that'd be extremely easy to detect malice via our current API. An admin could list workspaces to easily detect who's the malicious actor.

That's fair, but I still think that a global solution should be applied to all jobs simultaneously. You said earlier there was a ticket about this, could you link it so I can add context to it? I will try to tackle it before community MVP

kylecarbs · 2022-05-31T13:43:29Z

+		var input templateVersionPlanJob
+		err = json.Unmarshal(job.Input, &input)
+		if err != nil {
+			return nil, failJob(fmt.Sprintf("unmarshal job input %q: %s", job.Input, err))
+		}


It scares me to add another job with such similar scope to the others, but I understand why.

It'd be helpful if you explained why we couldn't use the workspace_build job to do similarly. I believe it'd just require not being attached to a literal workspace build, which seems like a reasonable change anyways.

We need to have a separate job for this because the job endpoints have different schemas and different endpoints to access the job details, not a unified /jobs endpoint. You've said in the past that you'd like to split these different job types apart more in the future (so they don't share DB tables for metadata etc.), so merging these two jobs together would be counterintuitive to that goal

The inputs are slightly different, but the outputs are the same, right?

Effectively, yes

It's probably worth opening a ticket considering merging those job types, or at least stating what the impl would look like to merge them. I'm always nervous about adding more job types.

deansheather · 2022-05-31T17:36:45Z

-				return err
+				return xerrors.Errorf("begin workspace dry-run: %w", err)
+			}
+			_, _ = fmt.Fprintln(cmd.OutOrStdout(), "Planning workspace...")


I kept the language "Planning workspace" because I couldn't come up with something that made sense with the word "dry-run"....

"Dry-running workspace"

"Running workspace dry-run"

etc.

…template parameters (#1732)

feat: add new provisioner job type template_version_plan

3e9ecc3

deansheather self-assigned this May 25, 2022

deansheather added 2 commits May 27, 2022 09:52

feat: finish implementing workspace planning

cf033aa

merge main

bafe1c1

deansheather requested a review from kylecarbs May 27, 2022 09:58

big tests

cfe34af

deansheather marked this pull request as ready for review May 27, 2022 19:50

deansheather requested a review from a team May 27, 2022 19:50

johnstcn and others added 4 commits May 27, 2022 21:25

fix: workspaces: relax error assertion

0d03a39

fix

4078fd1

fixup! fix

8d04749

gen

4b06c72

deansheather requested a review from a team as a code owner May 27, 2022 20:45

johnstcn approved these changes May 27, 2022

View reviewed changes

deansheather changed the title ~~feat: add new provisioner job type template_version_plan~~ feat: run a terraform plan before creating workspaces with the given template parameters May 27, 2022

deansheather added 2 commits May 27, 2022 20:57

fixup! gen

2fee23a

remove heavy object nesting in tests

6b95e54

deansheather requested review from a team and removed request for a team May 28, 2022 10:21

kylecarbs reviewed May 31, 2022

View reviewed changes

Rename template version plan to template version dry run

0b6d083

deansheather requested a review from kylecarbs May 31, 2022 17:35

deansheather commented May 31, 2022

View reviewed changes

spikecurtis mentioned this pull request May 31, 2022

coder show incorrectly shows permanent resources as ephemeral. #1937

Closed

zounce approved these changes Jun 1, 2022

View reviewed changes

deansheather merged commit 6be8a37 into main Jun 1, 2022

deansheather deleted the template-version-plan branch June 1, 2022 14:44

kylecarbs pushed a commit that referenced this pull request Jun 10, 2022

feat: run a terraform plan before creating workspaces with the given …

b41750d

…template parameters (#1732)

		// We use the workspace RBAC check since we don't want to allow plans if the
		// user can't create workspaces.

		ALTER TYPE provisioner_job_type
		ADD VALUE IF NOT EXISTS 'template_version_plan';

		// TODO (Dean): reprompt for parameter values if we deem it to
		// be a validation error

Uh oh!

Conversation

deansheather commented May 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Subtasks

Future:

Screenshots

Uh oh!

johnstcn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

Choose a reason for hiding this comment

Uh oh!

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!

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!

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

deansheather commented May 25, 2022 •

edited

Loading