From 57047656ec48bce34a59d5edee84dc5bb66ea80b Mon Sep 17 00:00:00 2001 From: evgeniy-scherbina Date: Wed, 30 Apr 2025 17:58:43 +0000 Subject: [PATCH 1/7] fix: fix for prebuilds claiming and deletion --- coderd/wsbuilder/wsbuilder.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/coderd/wsbuilder/wsbuilder.go b/coderd/wsbuilder/wsbuilder.go index 942829004309c..de76147ed7d57 100644 --- a/coderd/wsbuilder/wsbuilder.go +++ b/coderd/wsbuilder/wsbuilder.go @@ -647,6 +647,11 @@ func (b *Builder) findNewBuildParameterValue(name string) *codersdk.WorkspaceBui } func (b *Builder) getLastBuildParameters() ([]database.WorkspaceBuildParameter, error) { + // TODO: exclude preset params from this list instead of returning nothing? + if b.prebuildClaimedBy != uuid.Nil || b.prebuild { + return nil, nil + } + if b.lastBuildParameters != nil { return *b.lastBuildParameters, nil } From 1f3902474383e650a1e776edf2c490095c6e3432 Mon Sep 17 00:00:00 2001 From: evgeniy-scherbina Date: Wed, 30 Apr 2025 20:42:24 +0000 Subject: [PATCH 2/7] test: add unit-test for claiming & immutable params scenario --- enterprise/coderd/prebuilds/claim_test.go | 24 ++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/enterprise/coderd/prebuilds/claim_test.go b/enterprise/coderd/prebuilds/claim_test.go index 145095e6533e7..4ebdc24b6e895 100644 --- a/enterprise/coderd/prebuilds/claim_test.go +++ b/enterprise/coderd/prebuilds/claim_test.go @@ -325,21 +325,16 @@ func TestClaimPrebuild(t *testing.T) { spy.claims.Store(0) // Reset counter because we need to check if any new claim requests happen. - wp, err := userClient.WorkspaceBuildParameters(ctx, userWorkspace.LatestBuild.ID) - require.NoError(t, err) - stopBuild, err := userClient.CreateWorkspaceBuild(ctx, workspace.ID, codersdk.CreateWorkspaceBuildRequest{ - TemplateVersionID: version.ID, - Transition: codersdk.WorkspaceTransitionStop, - RichParameterValues: wp, + TemplateVersionID: version.ID, + Transition: codersdk.WorkspaceTransitionStop, }) require.NoError(t, err) coderdtest.AwaitWorkspaceBuildJobCompleted(t, userClient, stopBuild.ID) startBuild, err := userClient.CreateWorkspaceBuild(ctx, workspace.ID, codersdk.CreateWorkspaceBuildRequest{ - TemplateVersionID: version.ID, - Transition: codersdk.WorkspaceTransitionStart, - RichParameterValues: wp, + TemplateVersionID: version.ID, + Transition: codersdk.WorkspaceTransitionStart, }) require.NoError(t, err) coderdtest.AwaitWorkspaceBuildJobCompleted(t, userClient, startBuild.ID) @@ -369,6 +364,17 @@ func templateWithAgentAndPresetsWithPrebuilds(desiredInstances int32) *echo.Resp }, }, }, + // Make sure immutable params don't break claiming logic + Parameters: []*proto.RichParameter{ + { + Name: "k1", + Description: "immutable param", + Type: "string", + DefaultValue: "", + Required: false, + Mutable: false, + }, + }, Presets: []*proto.Preset{ { Name: "preset-a", From dd1ab455fb082f1ab304e8c90690ec621dbb26a2 Mon Sep 17 00:00:00 2001 From: evgeniy-scherbina Date: Wed, 30 Apr 2025 21:22:02 +0000 Subject: [PATCH 3/7] test: add unit-test for deletion & immutable params scenario --- enterprise/coderd/prebuilds/reconcile_test.go | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/enterprise/coderd/prebuilds/reconcile_test.go b/enterprise/coderd/prebuilds/reconcile_test.go index 9783b215f185b..bf65eda941598 100644 --- a/enterprise/coderd/prebuilds/reconcile_test.go +++ b/enterprise/coderd/prebuilds/reconcile_test.go @@ -901,6 +901,15 @@ func setupTestDBTemplateVersion( ID: templateID, ActiveVersionID: templateVersion.ID, })) + dbgen.TemplateVersionParameter(t, db, database.TemplateVersionParameter{ + TemplateVersionID: templateVersion.ID, + Name: "test", + Description: "required & immutable param", + Type: "string", + DefaultValue: "", + Required: true, + Mutable: false, + }) return templateVersion.ID } @@ -999,7 +1008,7 @@ func setupTestDBWorkspace( OrganizationID: orgID, Error: buildError, }) - dbgen.WorkspaceBuild(t, db, database.WorkspaceBuild{ + workspaceBuild := dbgen.WorkspaceBuild(t, db, database.WorkspaceBuild{ WorkspaceID: workspace.ID, InitiatorID: initiatorID, TemplateVersionID: templateVersionID, @@ -1008,6 +1017,13 @@ func setupTestDBWorkspace( Transition: transition, CreatedAt: clock.Now(), }) + dbgen.WorkspaceBuildParameters(t, db, []database.WorkspaceBuildParameter{ + { + WorkspaceBuildID: workspaceBuild.ID, + Name: "test", + Value: "test", + }, + }) return workspace } From 77708ed78305d085b26559345682d942e01020e9 Mon Sep 17 00:00:00 2001 From: evgeniy-scherbina Date: Wed, 30 Apr 2025 21:26:32 +0000 Subject: [PATCH 4/7] test: add small comment --- enterprise/coderd/prebuilds/reconcile_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/enterprise/coderd/prebuilds/reconcile_test.go b/enterprise/coderd/prebuilds/reconcile_test.go index bf65eda941598..bc886fc0a8231 100644 --- a/enterprise/coderd/prebuilds/reconcile_test.go +++ b/enterprise/coderd/prebuilds/reconcile_test.go @@ -901,6 +901,7 @@ func setupTestDBTemplateVersion( ID: templateID, ActiveVersionID: templateVersion.ID, })) + // Make sure immutable params don't break prebuilt workspace deletion logic dbgen.TemplateVersionParameter(t, db, database.TemplateVersionParameter{ TemplateVersionID: templateVersion.ID, Name: "test", From e04ae4411d0047acc6c5f7ca27184fac1dc0bde0 Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Thu, 1 May 2025 10:04:12 +0200 Subject: [PATCH 5/7] fix: ensure resolver evaluates previous & new values to determine mutation Signed-off-by: Danny Kopping --- coderd/wsbuilder/wsbuilder.go | 5 --- codersdk/richparameters.go | 2 +- codersdk/richparameters_test.go | 57 ++++++++++++++++++++++++++++----- 3 files changed, 50 insertions(+), 14 deletions(-) diff --git a/coderd/wsbuilder/wsbuilder.go b/coderd/wsbuilder/wsbuilder.go index de76147ed7d57..942829004309c 100644 --- a/coderd/wsbuilder/wsbuilder.go +++ b/coderd/wsbuilder/wsbuilder.go @@ -647,11 +647,6 @@ func (b *Builder) findNewBuildParameterValue(name string) *codersdk.WorkspaceBui } func (b *Builder) getLastBuildParameters() ([]database.WorkspaceBuildParameter, error) { - // TODO: exclude preset params from this list instead of returning nothing? - if b.prebuildClaimedBy != uuid.Nil || b.prebuild { - return nil, nil - } - if b.lastBuildParameters != nil { return *b.lastBuildParameters, nil } diff --git a/codersdk/richparameters.go b/codersdk/richparameters.go index 2ddd5d00f6c41..6fc6b8e0c343f 100644 --- a/codersdk/richparameters.go +++ b/codersdk/richparameters.go @@ -164,7 +164,7 @@ type ParameterResolver struct { // resolves the correct value. It returns the value of the parameter, if valid, and an error if invalid. func (r *ParameterResolver) ValidateResolve(p TemplateVersionParameter, v *WorkspaceBuildParameter) (value string, err error) { prevV := r.findLastValue(p) - if !p.Mutable && v != nil && prevV != nil { + if !p.Mutable && v != nil && prevV != nil && v.Value != prevV.Value { return "", xerrors.Errorf("Parameter %q is not mutable, so it can't be updated after creating a workspace.", p.Name) } if p.Required && v == nil && prevV == nil { diff --git a/codersdk/richparameters_test.go b/codersdk/richparameters_test.go index 16365f7c2f416..5635a82beb6c6 100644 --- a/codersdk/richparameters_test.go +++ b/codersdk/richparameters_test.go @@ -1,6 +1,7 @@ package codersdk_test import ( + "fmt" "testing" "github.com/stretchr/testify/require" @@ -121,20 +122,60 @@ func TestParameterResolver_ValidateResolve_NewOverridesOld(t *testing.T) { func TestParameterResolver_ValidateResolve_Immutable(t *testing.T) { t.Parallel() uut := codersdk.ParameterResolver{ - Rich: []codersdk.WorkspaceBuildParameter{{Name: "n", Value: "5"}}, + Rich: []codersdk.WorkspaceBuildParameter{{Name: "n", Value: "old"}}, } p := codersdk.TemplateVersionParameter{ Name: "n", - Type: "number", + Type: "string", Required: true, Mutable: false, } - v, err := uut.ValidateResolve(p, &codersdk.WorkspaceBuildParameter{ - Name: "n", - Value: "6", - }) - require.Error(t, err) - require.Equal(t, "", v) + + cases := []struct { + name string + newValue string + expectedErr string + }{ + { + name: "mutation", + newValue: "new", // "new" != "old" + expectedErr: fmt.Sprintf("Parameter %q is not mutable", p.Name), + }, + { + // Values are case-sensitive. + name: "case change", + newValue: "Old", // "Old" != "old" + expectedErr: fmt.Sprintf("Parameter %q is not mutable", p.Name), + }, + { + name: "default", + newValue: "", // "" != "old" + expectedErr: fmt.Sprintf("Parameter %q is not mutable", p.Name), + }, + { + name: "no change", + newValue: "old", // "old" == "old" + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + v, err := uut.ValidateResolve(p, &codersdk.WorkspaceBuildParameter{ + Name: "n", + Value: tc.newValue, + }) + + if tc.expectedErr == "" { + require.NoError(t, err) + require.Equal(t, tc.newValue, v) + } else { + require.ErrorContains(t, err, tc.expectedErr) + require.Equal(t, "", v) + } + }) + } } func TestRichParameterValidation(t *testing.T) { From d62589e3d25d1e2318fd84df9c30412a133a7a4e Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Thu, 1 May 2025 10:28:06 +0200 Subject: [PATCH 6/7] chore: restore unnecessarily removed code Signed-off-by: Danny Kopping --- enterprise/coderd/prebuilds/claim_test.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/enterprise/coderd/prebuilds/claim_test.go b/enterprise/coderd/prebuilds/claim_test.go index 4ebdc24b6e895..f87fb239d2e55 100644 --- a/enterprise/coderd/prebuilds/claim_test.go +++ b/enterprise/coderd/prebuilds/claim_test.go @@ -325,16 +325,20 @@ func TestClaimPrebuild(t *testing.T) { spy.claims.Store(0) // Reset counter because we need to check if any new claim requests happen. + wp, err := userClient.WorkspaceBuildParameters(ctx, userWorkspace.LatestBuild.ID) + require.NoError(t, err) + stopBuild, err := userClient.CreateWorkspaceBuild(ctx, workspace.ID, codersdk.CreateWorkspaceBuildRequest{ - TemplateVersionID: version.ID, - Transition: codersdk.WorkspaceTransitionStop, + TemplateVersionID: version.ID, + Transition: codersdk.WorkspaceTransitionStop, }) require.NoError(t, err) coderdtest.AwaitWorkspaceBuildJobCompleted(t, userClient, stopBuild.ID) startBuild, err := userClient.CreateWorkspaceBuild(ctx, workspace.ID, codersdk.CreateWorkspaceBuildRequest{ - TemplateVersionID: version.ID, - Transition: codersdk.WorkspaceTransitionStart, + TemplateVersionID: version.ID, + Transition: codersdk.WorkspaceTransitionStart, + RichParameterValues: wp, }) require.NoError(t, err) coderdtest.AwaitWorkspaceBuildJobCompleted(t, userClient, startBuild.ID) From 21bb4816dd1efc5e5e9d329e1991d829ae004724 Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Thu, 1 May 2025 10:39:02 +0200 Subject: [PATCH 7/7] make fmt Signed-off-by: Danny Kopping --- enterprise/coderd/prebuilds/claim_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/enterprise/coderd/prebuilds/claim_test.go b/enterprise/coderd/prebuilds/claim_test.go index f87fb239d2e55..ad31d2b4eff1b 100644 --- a/enterprise/coderd/prebuilds/claim_test.go +++ b/enterprise/coderd/prebuilds/claim_test.go @@ -329,8 +329,8 @@ func TestClaimPrebuild(t *testing.T) { require.NoError(t, err) stopBuild, err := userClient.CreateWorkspaceBuild(ctx, workspace.ID, codersdk.CreateWorkspaceBuildRequest{ - TemplateVersionID: version.ID, - Transition: codersdk.WorkspaceTransitionStop, + TemplateVersionID: version.ID, + Transition: codersdk.WorkspaceTransitionStop, }) require.NoError(t, err) coderdtest.AwaitWorkspaceBuildJobCompleted(t, userClient, stopBuild.ID)