NotificationsYou must be signed in to change notification settings
Fork928
Star10.1k

feat: run a terraform plan before creating workspaces with the given template parameters#1732

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

deansheather merged 11 commits intomainfromtemplate-version-plan

Jun 1, 2022

Merged

feat: run a terraform plan before creating workspaces with the given template parameters#1732

deansheather merged 11 commits intomainfromtemplate-version-plan

Jun 1, 2022

Conversation

Copy link

Member

deansheather commentedMay 25, 2022•
edited
Loading

Adds a new provisioner job typetemplate_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 incoder 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

Addtemplate_version_plan provisioner job type to database
AddPOST /templateversions/:template_version_id/plan endpoint and codersdk methods
Add endpoints to monitor template version plan logs and cancel early (talking to Kyle in Slack about what to do here to avoid duplication)
Add tests for template plan endpoint
Add provisionerd protobuf types forTemplateVersionPlan
Dispatchtemplate_version_plan provisioner jobs =>TemplateVersionPlan protobuf jobs
Add provisionerd job handling forTemplateVersionPlan jobs
Add tests for provisionerd job handling
Add template version plan tocoder create before creating the workspace
- Add the plan
- Wait for the plan
- Handle logs and cancellation
- Check for parameter validation errors and reprompt for parameters
Add tests forcoder create plan
Add workspace name to the plan for name validation checks

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

feat: add new provisioner job type template_version_plan

3e9ecc3

deansheather self-assigned this

May 25, 2022

deansheather added2 commits

May 27, 2022 09:52

feat: finish implementing workspace planning

cf033aa

merge main

bafe1c1

deansheather requested a review fromkylecarbs

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 froma team

May 27, 2022 19:50

johnstcnand others added4 commits

May 27, 2022 21:25

fix: workspaces: relax error assertion

0d03a39

fix

4078fd1

fixup! fix

8d04749

gen

4b06c72

deansheather requested a review froma team as acode owner

May 27, 2022 20:45

johnstcn approved these changes

May 27, 2022

View reviewed changes

Copy link

Member

johnstcn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

coderd/templateversions.go Outdated

Comment on lines 173 to 174

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

Copy link

Member

johnstcnMay 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Technically correct - the best kind of correct.

cli/cliui/provisionerjob.goShow resolvedHide resolved

coderd/database/migrations/000099_provisioner_job_type_plan.down.sql Outdated

Comment on lines 1 to 9

		-- 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'
		;

Copy link

Member

johnstcnMay 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

⚠️ reminder: rename me⚠️

coderd/database/migrations/000099_provisioner_job_type_plan.up.sql Outdated

Comment on lines 1 to 2

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

Copy link

Member

johnstcnMay 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

⚠️ reminder: rename me⚠️

provisionerd/provisionerd_test.go OutdatedShow resolvedHide resolved

cli/create.go

Comment on lines +197 to +198

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

Copy link

Member

johnstcnMay 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

TODO: open an issue

Copy link

MemberAuthor

deansheatherMay 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Opened two issues:

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

coderd/provisionerdaemons.go

		}
		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))

Copy link

Member

johnstcnMay 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

MemberAuthor

deansheatherMay 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

coderd/templateversions_test.goShow resolvedHide resolved

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 added2 commits

May 27, 2022 20:57

fixup! gen

2fee23a

remove heavy object nesting in tests

6b95e54

deansheather requested review froma team and removed request fora team

May 28, 2022 10:21

kylecarbs reviewed

May 31, 2022

View reviewed changes

coderd/templateversions.go Outdated

Comment on lines 180 to 183

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

Copy link

Member

kylecarbsMay 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link

MemberAuthor

deansheatherMay 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

MemberAuthor

deansheatherMay 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Member

kylecarbsJun 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link

MemberAuthor

deansheatherJun 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

MemberAuthor

deansheatherJun 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Member

kylecarbsJun 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

MemberAuthor

deansheatherJun 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Member

kylecarbsJun 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

MemberAuthor

deansheatherJun 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

coderd/templateversions.go OutdatedShow resolvedHide resolved

coderd/provisionerdaemons.go Outdated

Comment on lines 257 to 261

		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))
		}

Copy link

Member

kylecarbsMay 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 theworkspace_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.

Copy link

MemberAuthor

deansheatherMay 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Member

kylecarbsJun 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

MemberAuthor

deansheatherJun 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Effectively, yes

Copy link

Member

kylecarbsJun 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.