8000 chore: pass previous values into terraform apply by Emyrk · Pull Request #17696 · coder/coder · GitHub
[go: up one dir, main page]

Skip to content

chore: pass previous values into terraform apply #17696

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 23 commits into from
May 12, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10000
37 changes: 31 additions & 6 deletions coderd/provisionerdserver/provisionerdserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -549,6 +549,30 @@ func (s *server) acquireProtoJob(ctx context.Context, job database.ProvisionerJo
return nil, failJob(fmt.Sprintf("convert workspace transition: %s", err))
}

// A previous workspace build exists
var lastWorkspaceBuildParameters []database.WorkspaceBuildParameter
if workspaceBuild.BuildNumber > 1 {
// TODO: Should we fetch the last build that succeeded? This fetches the
// previous build regardless of the status of the build.
Comment on lines +555 to +556
Copy link
Member

Choose a reason for hiding this comment

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

If we check for the last successful build, we could end up with no builds. What do we do then? Do we just settle for the last build? IMO just checking the previous build is simpler conceptually, and is more likely to be what users expect.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, the wsbuilder just takes the last build regardless of status. Just feels a bit off since the tfstate is different if the previous failed. 🤷‍♂️

buildNum := workspaceBuild.BuildNumber - 1
previous, err := s.Database.GetWorkspaceBuildByWorkspaceIDAndBuildNumber(ctx, database.GetWorkspaceBuildByWorkspaceIDAndBuildNumberParams{
WorkspaceID: workspaceBuild.WorkspaceID,
BuildNumber: buildNum,
})
Comment on lines +555 to +561
Copy link
Member

Choose a reason for hiding this comment

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

What problem are we actually trying to solve here?

wsbuilder already fetches the last build parameters, if they exist:

func (b *Builder) getLastBuildParameters() ([]database.WorkspaceBuildParameter, error) {
if b.lastBuildParameters != nil {
return *b.lastBuildParameters, nil
}
bld, err := b.getLastBuild()
if xerrors.Is(err, sql.ErrNoRows) {
// if the build doesn't exist, then clearly there can be no parameters.
b.lastBuildParameters = &[]database.WorkspaceBuildParameter{}
return *b.lastBuildParameters, nil
}
if err != nil {
return nil, xerrors.Errorf("get last build to get parameters: %w", err)
}
values, err := b.store.GetWorkspaceBuildParameters(b.ctx, bld.ID)
if err != nil && !xerrors.Is(err, sql.ErrNoRows) {
return nil, xerrors.Errorf("get last build %s parameters: %w", bld.ID, err)
}
b.lastBuildParameters = &values
return values, nil
}

Given that this is the case, why do we need to do this extra work for all jobs? Isn't this just for template version import jobs?

Copy link
Member Author

Choose a reason for hiding this comment

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

That does it in coder/coder at workspace create yes, but this passes the previous values to the terraform via env vars.

The terraform provider now enforces monotonicity: coder/terraform-provider-coder#392

So this is duplicating that check in wsbuilder at terraform apply/plan.
For dynamic parameters, we skip validating params in wsbuilder, so we need to make sure validation is applied in terraform

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe I could pass the values from wsbuilder to here via the job? Rather than refetch

Copy link
Member

Choose a reason for hiding this comment

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

I like that approach -- wsbuilder is then still responsible for fetching all of the various baggage related to a workspace build, but just defers the validation part to Terraform.

Copy link
Member Author

Choose a reason for hiding this comment

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

@johnstcn actually, as much as I'd like to have wsbuilder be the source of truth. We fetch everything again at this step in the workspace build.

Current params, workspace data, external auth, etc.

We store very little in the job payload:

type WorkspaceProvisionJob struct {
WorkspaceBuildID uuid.UUID `json:"workspace_build_id"`
DryRun bool `json:"dry_run"`
LogLevel string `json:"log_level,omitempty"`
PrebuiltWorkspaceBuildStage sdkproto.PrebuiltWorkspaceBuildStage `json:"prebuilt_workspace_stage,omitempty"`
}

So I'm going to keep this as a refetching. Ideally I would use the same function to fetch the previous params in both cases, however at wsbuild the latestbuild is the "previous". And at the point I added code, the previous is build -1.

Copy link
Member

Choose a reason for hiding this comment

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

Ahh... gotcha. That's unfortunate :(

Copy link
Member Author

Choose a reason for hiding this comment

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

It is, I think it would be a large refactor to move all the fields into wsbuilder


// If the error is ErrNoRows, then assume previous values are empty.
if err != nil && !xerrors.Is(err, sql.ErrNoRows) {
return nil, xerrors.Errorf("get last build with number=%d: %w", buildNum, err)
}

if err == nil {
lastWorkspaceBuildParameters, err = s.Database.GetWorkspaceBuildParameters(ctx, previous.ID)
if err != nil {
return nil, xerrors.Errorf("get last build parameters %q: %w", previous.ID, err)
}
}
}

workspaceBuildParameters, err := s.Database.GetWorkspaceBuildParameters(ctx, workspaceBuild.ID)
if err != nil {
return nil, failJob(fmt.Sprintf("get workspace build parameters: %s", err))
Expand Down Expand Up @@ -625,12 +649,13 @@ func (s *server) acquireProtoJob(ctx context.Context, job database.ProvisionerJo

protoJob.Type = &proto.AcquiredJob_WorkspaceBuild_{
WorkspaceBuild: &proto.AcquiredJob_WorkspaceBuild{
WorkspaceBuildId: workspaceBuild.ID.String(),
WorkspaceName: workspace.Name,
State: workspaceBuild.ProvisionerState,
RichParameterValues: convertRichParameterValues(workspaceBuildParameters),
VariableValues: asVariableValues(templateVariables),
ExternalAuthProviders: externalAuthProviders,
WorkspaceBuildId: workspaceBuild.ID.String(),
WorkspaceName: workspace.Name,
State: workspaceBuild.ProvisionerState,
RichParameterValues: convertRichParameterValues(workspaceBuildParameters),
PreviousParameterValues: convertRichParameterValues(lastWorkspaceBuildParameters),
VariableValues: asVariableValues(templateVariables),
ExternalAuthProviders: externalAuthProviders,< 67ED /td>
Metadata: &sdkproto.Metadata{
CoderUrl: s.AccessURL.String(),
WorkspaceTransition: transition,
Expand Down
9 changes: 6 additions & 3 deletions provisioner/terraform/provision.go
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ func (s *server) Plan(

s.logger.Debug(ctx, "ran initialization")

env, err := provisionEnv(sess.Config, request.Metadata, request.RichParameterValues, request.ExternalAuthProviders)
env, err := provisionEnv(sess.Config, request.Metadata, request.PreviousParameterValues, request.RichParameterValues, request.ExternalAuthProviders)
if err != nil {
return provisionersdk.PlanErrorf("setup env: %s", err)
}
Expand Down Expand Up @@ -205,7 +205,7 @@ func (s *server) Apply(

// Earlier in the session, Plan() will have written the state file and the plan file.
statefilePath := getStateFilePath(sess.WorkDirectory)
env, err := provisionEnv(sess.Config, request.Metadata, nil, nil)
env, err := provisionEnv(sess.Config, request.Metadata, nil, nil, nil)
if err != nil {
return provisionersdk.ApplyErrorf("provision env: %s", err)
}
Expand Down Expand Up @@ -236,7 +236,7 @@ func planVars(plan *proto.PlanRequest) ([]string, error) {

func provisionEnv(
config *proto.Config, metadata *proto.Metadata,
richParams []*proto.RichParameterValue, externalAuth []*proto.ExternalAuthProvider,
previousParams, richParams []*proto.RichParameterValue, externalAuth []*proto.ExternalAuthProvider,
) ([]string, error) {
env := safeEnviron()
ownerGroups, err := json.Marshal(metadata.GetWorkspaceOwnerGroups())
Expand Down Expand Up @@ -280,6 +280,9 @@ func provisionEnv(
for key, value := range provisionersdk.AgentScriptEnv() {
env = append(env, key+"="+value)
}
for _, param := range previousParams {
env = append(env, provider.ParameterEnvironmentVariablePrevious(param.Name)+"="+param.Value)
}
for _, param := range richParams {
env = append(env, provider.ParameterEnvironmentVariable(param.Name)+"="+param.Value)
}
Expand Down
538 changes: 278 additions & 260 deletions provisionerd/proto/provisionerd.pb.go

Large diffs are not rendered by default.

4 changes: 4 additions & 0 deletions provisionerd/proto/provisionerd.proto
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ message AcquiredJob {
provisioner.Metadata metadata = 7;
bytes state = 8;
string log_level = 9;
// previous_parameter_values is used to pass the values of the previous
// workspace build. Omit these values if the workspace is being created
// for the first time.
repeated provisioner.RichParameterValue previous_parameter_values = 10;
}
message TemplateImport {
provisioner.Metadata metadata = 1;
Expand Down
3 changes: 3 additions & 0 deletions provisionerd/proto/version.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ import "github.com/coder/coder/v2/apiversion"
// API v1.5:
// - Add new field named `prebuilt_workspace_build_stage` enum in the Metadata message.
// - Add `plan` and `module_files` fields to `CompletedJob.TemplateImport`.
// - Add previous parameter values to 'WorkspaceBuild' jobs. Provisioner passes
// the previous values for the `terraform apply` to enforce monotonicity
// in the terraform provider.
Comment on lines +19 to +21
Copy link
Member

Choose a reason for hiding this comment

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

👍

const (
CurrentMajor = 1
CurrentMinor = 5
Expand Down
13 changes: 8 additions & 5 deletions
Original file line number Diff line number Diff line change
Expand Up @@ -691,7 +691,9 @@ func (r *Runner) runTemplateImportProvisionWithRichParameters(
err := r.session.Send(&sdkproto.Request{Type: &sdkproto.Request_Plan{Plan: &sdkproto.PlanRequest{
Metadata: metadata,
RichParameterValues: richParameterValues,
VariableValues: variableValues,
// Template import has no previous values
PreviousParameterValues: make([]*sdkproto.RichParameterValue, 0),
VariableValues: variableValues,
}}})
if err != nil {
return nil, xerrors.Errorf("start provision: %w", err)
Expand Down Expand Up @@ -960,10 +962,11 @@ func (r *Runner) runWorkspaceBuild(ctx context.Context) (*proto.CompletedJob, *p
resp, failed := r.buildWorkspace(ctx, "Planning infrastructure", &sdkproto.Request{
Type: &sdkproto.Request_Plan{
Plan: &sdkproto.PlanRequest{
Metadata: r.job.GetWorkspaceBuild().Metadata,
RichParameterValues: r.job.GetWorkspaceBuild().RichParameterValues,
VariableValues: r.job.GetWorkspaceBuild().VariableValues,
ExternalAuthProviders: r.job.GetWorkspaceBuild().ExternalAuthProviders,
Metadata: r.job.GetWorkspaceBuild().Metadata,
RichParameterValues: r.job.GetWorkspaceBuild().RichParameterValues,
PreviousParameterValues: r.job.GetWorkspaceBuild().PreviousParameterValues,
VariableValues: r.job.GetWorkspaceBuild().VariableValues,
ExternalAuthProviders: r.job.GetWorkspaceBuild().ExternalAuthProviders,
},
},
})
Expand Down
Loading
Loading
0