From 9212109bd4301d39f9bff7e7b9553fcc1664c917 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 25 Jun 2025 10:53:39 -0500 Subject: [PATCH 01/11] feat: test parameter immutability validation --- enterprise/coderd/dynamicparameters_test.go | 43 ++++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-) diff --git a/enterprise/coderd/dynamicparameters_test.go b/enterprise/coderd/dynamicparameters_test.go index e13d370a059ad..6e0e7edbbcf77 100644 --- a/enterprise/coderd/dynamicparameters_test.go +++ b/enterprise/coderd/dynamicparameters_test.go @@ -338,7 +338,6 @@ func TestDynamicParameterBuild(t *testing.T) { bld, err := templateAdmin.CreateWorkspaceBuild(ctx, wrk.ID, codersdk.CreateWorkspaceBuildRequest{ TemplateVersionID: immutable.ID, // Use the new template version with the immutable parameter Transition: codersdk.WorkspaceTransitionDelete, - DryRun: false, }) require.NoError(t, err) coderdtest.AwaitWorkspaceBuildJobCompleted(t, templateAdmin, bld.ID) @@ -354,6 +353,48 @@ func TestDynamicParameterBuild(t *testing.T) { require.NoError(t, err) require.Equal(t, wrk.ID, deleted.ID, "workspace should be deleted") }) + + t.Run("ImmutableChangeValue", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitShort) + // Start with a new template that has 1 parameter that is immutable + immutable, _ := coderdtest.DynamicParameterTemplate(t, templateAdmin, orgID, coderdtest.DynamicParameterTemplateParams{ + MainTF: string(must(os.ReadFile("testdata/parameters/immutable/main.tf"))), + }) + + // Create the workspace with the immutable parameter + wrk, err := templateAdmin.CreateUserWorkspace(ctx, codersdk.Me, codersdk.CreateWorkspaceRequest{ + TemplateID: immutable.ID, + Name: coderdtest.RandomUsername(t), + RichParameterValues: []codersdk.WorkspaceBuildParameter{ + {Name: "immutable", Value: "coder"}, + }, + }) + require.NoError(t, err) + coderdtest.AwaitWorkspaceBuildJobCompleted(t, templateAdmin, wrk.LatestBuild.ID) + + // No new value is acceptable + bld, err := templateAdmin.CreateWorkspaceBuild(ctx, wrk.ID, codersdk.CreateWorkspaceBuildRequest{ + Transition: codersdk.WorkspaceTransitionStart, + }) + require.NoError(t, err) + coderdtest.AwaitWorkspaceBuildJobCompleted(t, templateAdmin, bld.ID) + + params, err := templateAdmin.WorkspaceBuildParameters(ctx, bld.ID) + require.NoError(t, err) + require.Len(t, params, 1) + require.Equal(t, "coder", params[0].Value) + + // Update the value to something else, which should fail + _, err = templateAdmin.CreateWorkspaceBuild(ctx, wrk.ID, codersdk.CreateWorkspaceBuildRequest{ + Transition: codersdk.WorkspaceTransitionStart, + RichParameterValues: []codersdk.WorkspaceBuildParameter{ + {Name: "immutable", Value: "foo"}, + }, + }) + require.ErrorContains(t, err, `Parameter "immutable" is not mutable`) + }) }) } From ef2a8fdc7dfa49cd87d5272d550dc09c0a50f6da Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 25 Jun 2025 11:25:26 -0500 Subject: [PATCH 02/11] fixup tf --- enterprise/coderd/dynamicparameters_test.go | 25 +++++++------------ .../parameters/dynamicimmutable/main.tf | 23 +++++++++++++++++ 2 files changed, 32 insertions(+), 16 deletions(-) create mode 100644 enterprise/coderd/testdata/parameters/dynamicimmutable/main.tf diff --git a/enterprise/coderd/dynamicparameters_test.go b/enterprise/coderd/dynamicparameters_test.go index 6e0e7edbbcf77..6e363ce43d853 100644 --- a/enterprise/coderd/dynamicparameters_test.go +++ b/enterprise/coderd/dynamicparameters_test.go @@ -355,12 +355,14 @@ func TestDynamicParameterBuild(t *testing.T) { }) t.Run("ImmutableChangeValue", func(t *testing.T) { + // Ok this is a weird test to document how things are working. + // What if a parameter flips it's immutability based on a value? t.Parallel() ctx := testutil.Context(t, testutil.WaitShort) // Start with a new template that has 1 parameter that is immutable immutable, _ := coderdtest.DynamicParameterTemplate(t, templateAdmin, orgID, coderdtest.DynamicParameterTemplateParams{ - MainTF: string(must(os.ReadFile("testdata/parameters/immutable/main.tf"))), + MainTF: string(must(os.ReadFile("testdata/parameters/dynamicimmutable/main.tf"))), }) // Create the workspace with the immutable parameter @@ -368,32 +370,23 @@ func TestDynamicParameterBuild(t *testing.T) { TemplateID: immutable.ID, Name: coderdtest.RandomUsername(t), RichParameterValues: []codersdk.WorkspaceBuildParameter{ + {Name: "isimmutable", Value: "true"}, {Name: "immutable", Value: "coder"}, }, }) require.NoError(t, err) coderdtest.AwaitWorkspaceBuildJobCompleted(t, templateAdmin, wrk.LatestBuild.ID) - // No new value is acceptable + // Try new values bld, err := templateAdmin.CreateWorkspaceBuild(ctx, wrk.ID, codersdk.CreateWorkspaceBuildRequest{ Transition: codersdk.WorkspaceTransitionStart, - }) - require.NoError(t, err) - coderdtest.AwaitWorkspaceBuildJobCompleted(t, templateAdmin, bld.ID) - - params, err := templateAdmin.WorkspaceBuildParameters(ctx, bld.ID) - require.NoError(t, err) - require.Len(t, params, 1) - require.Equal(t, "coder", params[0].Value) - - // Update the value to something else, which should fail - _, err = templateAdmin.CreateWorkspaceBuild(ctx, wrk.ID, codersdk.CreateWorkspaceBuildRequest{ - Transition: codersdk.WorkspaceTransitionStart, RichParameterValues: []codersdk.WorkspaceBuildParameter{ - {Name: "immutable", Value: "foo"}, + {Name: "isimmutable", Value: "false"}, + {Name: "immutable", Value: "not-coder"}, }, }) - require.ErrorContains(t, err, `Parameter "immutable" is not mutable`) + require.NoError(t, err) + coderdtest.AwaitWorkspaceBuildJobCompleted(t, templateAdmin, bld.ID) }) }) } diff --git a/enterprise/coderd/testdata/parameters/dynamicimmutable/main.tf b/enterprise/coderd/testdata/parameters/dynamicimmutable/main.tf new file mode 100644 index 0000000000000..e2c90d4d20642 --- /dev/null +++ b/enterprise/coderd/testdata/parameters/dynamicimmutable/main.tf @@ -0,0 +1,23 @@ +terraform { + required_providers { + coder = { + source = "coder/coder" + } + } +} + +data "coder_workspace_owner" "me" {} + +data "coder_parameter" "isimmutable" { + name = "isimmutable" + type = "bool" + mutable = true + default = "true" +} + +data "coder_parameter" "immutable" { + name = "immutable" + type = "string" + mutable = data.coder_parameter.isimmutable.value == "false" + default = "Hello World" +} From e03d15f47415a72da14a2d0bf3766ec8ec08e95a Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 25 Jun 2025 11:35:39 -0500 Subject: [PATCH 03/11] fixup test to take an opinion --- coderd/dynamicparameters/resolver.go | 19 ++++++-- coderd/dynamicparameters/resolver_test.go | 52 +++++++++++++++++++++ enterprise/coderd/dynamicparameters_test.go | 6 +-- 3 files changed, 71 insertions(+), 6 deletions(-) diff --git a/coderd/dynamicparameters/resolver.go b/coderd/dynamicparameters/resolver.go index bd8e2294cf136..f32945e7c88d3 100644 --- a/coderd/dynamicparameters/resolver.go +++ b/coderd/dynamicparameters/resolver.go @@ -67,7 +67,7 @@ func ResolveParameters( // // This is how the form should look to the user on their workspace settings page. // This is the original form truth that our validations should initially be based on. - output, diags := renderer.Render(ctx, ownerID, values.ValuesMap()) + output, diags := renderer.Render(ctx, ownerID, previousValuesMap) if diags.HasErrors() { // Top level diagnostics should break the build. Previous values (and new) should // always be valid. If there is a case where this is not true, then this has to @@ -81,6 +81,7 @@ func ResolveParameters( // // To enforce these, the user's input values are trimmed based on the // mutability and ephemeral parameters defined in the template version. + wasImmutable := make(map[string]struct{}) for _, parameter := range output.Parameters { // Ephemeral parameters should not be taken from the previous build. // They must always be explicitly set in every build. @@ -98,6 +99,7 @@ func ResolveParameters( // // We do this so the next form render uses the original immutable value. if !firstBuild && !parameter.Mutable { + wasImmutable[parameter.Name] = struct{}{} delete(values, parameter.Name) prev, ok := previousValuesMap[parameter.Name] if ok { @@ -123,7 +125,12 @@ func ResolveParameters( for _, parameter := range output.Parameters { parameterNames[parameter.Name] = struct{}{} - if !firstBuild && !parameter.Mutable { + // Immutability is sourced from the current `mutable` argument, and also the + // previous parameter's `mutable` argument. This means you cannot flip an + // `immutable` parameter to `mutable` in a single build. This is to preserve the + // original mutability of the parameter. + _, wi := wasImmutable[parameter.Name] + if !firstBuild && (!parameter.Mutable || wi) { originalValue, ok := originalValues[parameter.Name] // Immutable parameters should not be changed after the first build. // If the value matches the original value, that is fine. @@ -137,13 +144,19 @@ func ResolveParameters( if parameter.Source != nil { src = ¶meter.Source.HCLBlock().TypeRange } + errTitle := "Immutable" + // In the strange case someone flips mutability from `true` to `false`. + // Change the error title to indicate that this was previously immutable. + if wi && parameter.Mutable { + errTitle = "Previously immutable" + } // An immutable parameter was changed, which is not allowed. // Add a failed diagnostic to the output. parameterError.Extend(parameter.Name, hcl.Diagnostics{ &hcl.Diagnostic{ Severity: hcl.DiagError, - Summary: "Immutable parameter changed", + Summary: fmt.Sprintf("%s parameter changed", errTitle), Detail: fmt.Sprintf("Parameter %q is not mutable, so it can't be updated after creating a workspace.", parameter.Name), Subject: src, }, diff --git a/coderd/dynamicparameters/resolver_test.go b/coderd/dynamicparameters/resolver_test.go index ec5218613ff03..b5d27db0df6a7 100644 --- a/coderd/dynamicparameters/resolver_test.go +++ b/coderd/dynamicparameters/resolver_test.go @@ -20,6 +20,58 @@ import ( func TestResolveParameters(t *testing.T) { t.Parallel() + t.Run("ChangeImmutable", func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + render := rendermock.NewMockRenderer(ctrl) + + // The first render takes in the existing values + render.EXPECT(). + Render(gomock.Any(), gomock.Any(), gomock.Any()). + Return(&preview.Output{ + Parameters: []previewtypes.Parameter{ + { + ParameterData: previewtypes.ParameterData{ + Name: "immutable", + Type: previewtypes.ParameterTypeString, + FormType: provider.ParameterFormTypeInput, + Mutable: false, + DefaultValue: previewtypes.StringLiteral("foo"), + Required: true, + }, + Value: previewtypes.StringLiteral("foo"), + Diagnostics: nil, + }, + }, + }, nil). + Return(&preview.Output{ + Parameters: []previewtypes.Parameter{ + { + ParameterData: previewtypes.ParameterData{ + Name: "immutable", + Type: previewtypes.ParameterTypeString, + FormType: provider.ParameterFormTypeInput, + Mutable: false, + DefaultValue: previewtypes.StringLiteral("foo"), + Required: true, + }, + Value: previewtypes.StringLiteral("foo"), + Diagnostics: nil, + }, + }, + }, nil) + + ctx := testutil.Context(t, testutil.WaitShort) + values, err := dynamicparameters.ResolveParameters(ctx, uuid.New(), render, false, + []database.WorkspaceBuildParameter{}, // No previous values + []codersdk.WorkspaceBuildParameter{}, // No new build values + []database.TemplateVersionPresetParameter{}, // No preset values + ) + require.NoError(t, err) + require.Equal(t, map[string]string{"immutable": "foo"}, values) + }) + t.Run("NewImmutable", func(t *testing.T) { t.Parallel() diff --git a/enterprise/coderd/dynamicparameters_test.go b/enterprise/coderd/dynamicparameters_test.go index 6e363ce43d853..71d814ff6b43c 100644 --- a/enterprise/coderd/dynamicparameters_test.go +++ b/enterprise/coderd/dynamicparameters_test.go @@ -357,6 +357,7 @@ func TestDynamicParameterBuild(t *testing.T) { t.Run("ImmutableChangeValue", func(t *testing.T) { // Ok this is a weird test to document how things are working. // What if a parameter flips it's immutability based on a value? + // The current behavior is to source immutability from the previous build. t.Parallel() ctx := testutil.Context(t, testutil.WaitShort) @@ -378,15 +379,14 @@ func TestDynamicParameterBuild(t *testing.T) { coderdtest.AwaitWorkspaceBuildJobCompleted(t, templateAdmin, wrk.LatestBuild.ID) // Try new values - bld, err := templateAdmin.CreateWorkspaceBuild(ctx, wrk.ID, codersdk.CreateWorkspaceBuildRequest{ + _, err = templateAdmin.CreateWorkspaceBuild(ctx, wrk.ID, codersdk.CreateWorkspaceBuildRequest{ Transition: codersdk.WorkspaceTransitionStart, RichParameterValues: []codersdk.WorkspaceBuildParameter{ {Name: "isimmutable", Value: "false"}, {Name: "immutable", Value: "not-coder"}, }, }) - require.NoError(t, err) - coderdtest.AwaitWorkspaceBuildJobCompleted(t, templateAdmin, bld.ID) + require.ErrorContains(t, err, `Previously immutable parameter changed`) }) }) } From 7e4ef44bbcc8bc0ae6830c2a9f1aed46b9d2498d Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 25 Jun 2025 11:40:47 -0500 Subject: [PATCH 04/11] fmt --- coderd/dynamicparameters/resolver_test.go | 52 ------------------- .../parameters/dynamicimmutable/main.tf | 2 +- 2 files changed, 1 insertion(+), 53 deletions(-) diff --git a/coderd/dynamicparameters/resolver_test.go b/coderd/dynamicparameters/resolver_test.go index b5d27db0df6a7..ec5218613ff03 100644 --- a/coderd/dynamicparameters/resolver_test.go +++ b/coderd/dynamicparameters/resolver_test.go @@ -20,58 +20,6 @@ import ( func TestResolveParameters(t *testing.T) { t.Parallel() - t.Run("ChangeImmutable", func(t *testing.T) { - t.Parallel() - - ctrl := gomock.NewController(t) - render := rendermock.NewMockRenderer(ctrl) - - // The first render takes in the existing values - render.EXPECT(). - Render(gomock.Any(), gomock.Any(), gomock.Any()). - Return(&preview.Output{ - Parameters: []previewtypes.Parameter{ - { - ParameterData: previewtypes.ParameterData{ - Name: "immutable", - Type: previewtypes.ParameterTypeString, - FormType: provider.ParameterFormTypeInput, - Mutable: false, - DefaultValue: previewtypes.StringLiteral("foo"), - Required: true, - }, - Value: previewtypes.StringLiteral("foo"), - Diagnostics: nil, - }, - }, - }, nil). - Return(&preview.Output{ - Parameters: []previewtypes.Parameter{ - { - ParameterData: previewtypes.ParameterData{ - Name: "immutable", - Type: previewtypes.ParameterTypeString, - FormType: provider.ParameterFormTypeInput, - Mutable: false, - DefaultValue: previewtypes.StringLiteral("foo"), - Required: true, - }, - Value: previewtypes.StringLiteral("foo"), - Diagnostics: nil, - }, - }, - }, nil) - - ctx := testutil.Context(t, testutil.WaitShort) - values, err := dynamicparameters.ResolveParameters(ctx, uuid.New(), render, false, - []database.WorkspaceBuildParameter{}, // No previous values - []codersdk.WorkspaceBuildParameter{}, // No new build values - []database.TemplateVersionPresetParameter{}, // No preset values - ) - require.NoError(t, err) - require.Equal(t, map[string]string{"immutable": "foo"}, values) - }) - t.Run("NewImmutable", func(t *testing.T) { t.Parallel() diff --git a/enterprise/coderd/testdata/parameters/dynamicimmutable/main.tf b/enterprise/coderd/testdata/parameters/dynamicimmutable/main.tf index e2c90d4d20642..08bdd3336faa9 100644 --- a/enterprise/coderd/testdata/parameters/dynamicimmutable/main.tf +++ b/enterprise/coderd/testdata/parameters/dynamicimmutable/main.tf @@ -9,7 +9,7 @@ terraform { data "coder_workspace_owner" "me" {} data "coder_parameter" "isimmutable" { - name = "isimmutable" + name = "isimmutable" type = "bool" mutable = true default = "true" From 9ef8f429a067283efd59df021eb00f828cd6b8b3 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 7 Jul 2025 09:41:53 -0600 Subject: [PATCH 05/11] revert resolver changes --- coderd/dynamicparameters/resolver.go | 19 +++---------------- 1 file changed, 3 insertions(+), 16 deletions(-) diff --git a/coderd/dynamicparameters/resolver.go b/coderd/dynamicparameters/resolver.go index f32945e7c88d3..bd8e2294cf136 100644 --- a/coderd/dynamicparameters/resolver.go +++ b/coderd/dynamicparameters/resolver.go @@ -67,7 +67,7 @@ func ResolveParameters( // // This is how the form should look to the user on their workspace settings page. // This is the original form truth that our validations should initially be based on. - output, diags := renderer.Render(ctx, ownerID, previousValuesMap) + output, diags := renderer.Render(ctx, ownerID, values.ValuesMap()) if diags.HasErrors() { // Top level diagnostics should break the build. Previous values (and new) should // always be valid. If there is a case where this is not true, then this has to @@ -81,7 +81,6 @@ func ResolveParameters( // // To enforce these, the user's input values are trimmed based on the // mutability and ephemeral parameters defined in the template version. - wasImmutable := make(map[string]struct{}) for _, parameter := range output.Parameters { // Ephemeral parameters should not be taken from the previous build. // They must always be explicitly set in every build. @@ -99,7 +98,6 @@ func ResolveParameters( // // We do this so the next form render uses the original immutable value. if !firstBuild && !parameter.Mutable { - wasImmutable[parameter.Name] = struct{}{} delete(values, parameter.Name) prev, ok := previousValuesMap[parameter.Name] if ok { @@ -125,12 +123,7 @@ func ResolveParameters( for _, parameter := range output.Parameters { parameterNames[parameter.Name] = struct{}{} - // Immutability is sourced from the current `mutable` argument, and also the - // previous parameter's `mutable` argument. This means you cannot flip an - // `immutable` parameter to `mutable` in a single build. This is to preserve the - // original mutability of the parameter. - _, wi := wasImmutable[parameter.Name] - if !firstBuild && (!parameter.Mutable || wi) { + if !firstBuild && !parameter.Mutable { originalValue, ok := originalValues[parameter.Name] // Immutable parameters should not be changed after the first build. // If the value matches the original value, that is fine. @@ -144,19 +137,13 @@ func ResolveParameters( if parameter.Source != nil { src = ¶meter.Source.HCLBlock().TypeRange } - errTitle := "Immutable" - // In the strange case someone flips mutability from `true` to `false`. - // Change the error title to indicate that this was previously immutable. - if wi && parameter.Mutable { - errTitle = "Previously immutable" - } // An immutable parameter was changed, which is not allowed. // Add a failed diagnostic to the output. parameterError.Extend(parameter.Name, hcl.Diagnostics{ &hcl.Diagnostic{ Severity: hcl.DiagError, - Summary: fmt.Sprintf("%s parameter changed", errTitle), + Summary: "Immutable parameter changed", Detail: fmt.Sprintf("Parameter %q is not mutable, so it can't be updated after creating a workspace.", parameter.Name), Subject: src, }, From 560115d601015fd88c7ac144551b4bb25c1236c3 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 7 Jul 2025 11:02:09 -0600 Subject: [PATCH 06/11] update unit tests --- coderd/dynamicparameters/resolver.go | 14 +---- coderd/dynamicparameters/resolver_test.go | 66 +++++++++++++++++++++ enterprise/coderd/dynamicparameters_test.go | 40 ++++++++++++- 3 files changed, 106 insertions(+), 14 deletions(-) diff --git a/coderd/dynamicparameters/resolver.go b/coderd/dynamicparameters/resolver.go index bd8e2294cf136..c2111ccd67422 100644 --- a/coderd/dynamicparameters/resolver.go +++ b/coderd/dynamicparameters/resolver.go @@ -55,19 +55,11 @@ func ResolveParameters( values[preset.Name] = parameterValue{Source: sourcePreset, Value: preset.Value} } - // originalValues is going to be used to detect if a user tried to change - // an immutable parameter after the first build. - originalValues := make(map[string]parameterValue, len(values)) - for name, value := range values { - // Store the original values for later use. - originalValues[name] = value - } - // Render the parameters using the values that were supplied to the previous build. // // This is how the form should look to the user on their workspace settings page. // This is the original form truth that our validations should initially be based on. - output, diags := renderer.Render(ctx, ownerID, values.ValuesMap()) + output, diags := renderer.Render(ctx, ownerID, previousValuesMap) if diags.HasErrors() { // Top level diagnostics should break the build. Previous values (and new) should // always be valid. If there is a case where this is not true, then this has to @@ -124,7 +116,7 @@ func ResolveParameters( parameterNames[parameter.Name] = struct{}{} if !firstBuild && !parameter.Mutable { - originalValue, ok := originalValues[parameter.Name] + originalValue, ok := previousValuesMap[parameter.Name] // Immutable parameters should not be changed after the first build. // If the value matches the original value, that is fine. // @@ -132,7 +124,7 @@ func ResolveParameters( // immutable parameters are allowed. This is an opinionated choice to prevent // workspaces failing to update or delete. Ideally we would block this, as // immutable parameters should only be able to be set at creation time. - if ok && parameter.Value.AsString() != originalValue.Value { + if ok && parameter.Value.AsString() != originalValue { var src *hcl.Range if parameter.Source != nil { src = ¶meter.Source.HCLBlock().TypeRange diff --git a/coderd/dynamicparameters/resolver_test.go b/coderd/dynamicparameters/resolver_test.go index ec5218613ff03..3f936af350da2 100644 --- a/coderd/dynamicparameters/resolver_test.go +++ b/coderd/dynamicparameters/resolver_test.go @@ -10,6 +10,7 @@ import ( "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/dynamicparameters" "github.com/coder/coder/v2/coderd/dynamicparameters/rendermock" + "github.com/coder/coder/v2/coderd/httpapi/httperror" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/testutil" "github.com/coder/preview" @@ -56,4 +57,69 @@ func TestResolveParameters(t *testing.T) { require.NoError(t, err) require.Equal(t, map[string]string{"immutable": "foo"}, values) }) + + // Tests a parameter going from mutable -> immutable + t.Run("BecameImmutable", func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + render := rendermock.NewMockRenderer(ctrl) + + mutable := previewtypes.ParameterData{ + Name: "immutable", + Type: previewtypes.ParameterTypeString, + FormType: provider.ParameterFormTypeInput, + Mutable: false, + DefaultValue: previewtypes.StringLiteral("foo"), + Required: true, + } + immutable := mutable + immutable.Mutable = false + + // A single immutable parameter with no previous value. + render.EXPECT(). + Render(gomock.Any(), gomock.Any(), gomock.Any()). + // Return the mutable param first + Return(&preview.Output{ + Parameters: []previewtypes.Parameter{ + { + ParameterData: mutable, + Value: previewtypes.StringLiteral("foo"), + Diagnostics: nil, + }, + }, + }, nil) + + render.EXPECT(). + Render(gomock.Any(), gomock.Any(), gomock.Any()). + // Then the immutable param + Return(&preview.Output{ + Parameters: []previewtypes.Parameter{ + { + ParameterData: immutable, + // User changes the value to 'bar' + Value: previewtypes.StringLiteral("bar"), + Diagnostics: nil, + }, + }, + }, nil) + + ctx := testutil.Context(t, testutil.WaitShort) + _, err := dynamicparameters.ResolveParameters(ctx, uuid.New(), render, false, + []database.WorkspaceBuildParameter{ + {Name: "immutable", Value: "foo"}, // Previous value foo + }, + []codersdk.WorkspaceBuildParameter{ + {Name: "immutable", Value: "bar"}, // New value + }, + []database.TemplateVersionPresetParameter{}, // No preset values + ) + require.Error(t, err) + resp, ok := httperror.IsResponder(err) + require.True(t, ok) + + _, respErr := resp.Response() + require.Len(t, respErr.Validations, 1) + require.Contains(t, respErr.Validations[0].Error(), "is not mutable") + }) } diff --git a/enterprise/coderd/dynamicparameters_test.go b/enterprise/coderd/dynamicparameters_test.go index 71d814ff6b43c..3b9b215b44e08 100644 --- a/enterprise/coderd/dynamicparameters_test.go +++ b/enterprise/coderd/dynamicparameters_test.go @@ -354,10 +354,11 @@ func TestDynamicParameterBuild(t *testing.T) { require.Equal(t, wrk.ID, deleted.ID, "workspace should be deleted") }) - t.Run("ImmutableChangeValue", func(t *testing.T) { + t.Run("PreviouslyImmutable", func(t *testing.T) { // Ok this is a weird test to document how things are working. // What if a parameter flips it's immutability based on a value? - // The current behavior is to source immutability from the previous build. + // The current behavior is to source immutability from the new state. + // So the value is allowed to be changed. t.Parallel() ctx := testutil.Context(t, testutil.WaitShort) @@ -386,7 +387,40 @@ func TestDynamicParameterBuild(t *testing.T) { {Name: "immutable", Value: "not-coder"}, }, }) - require.ErrorContains(t, err, `Previously immutable parameter changed`) + require.NoError(t, err) + }) + + t.Run("PreviouslyMutable", func(t *testing.T) { + // The value cannot be changed because it becomes immutable. + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitShort) + immutable, _ := coderdtest.DynamicParameterTemplate(t, templateAdmin, orgID, coderdtest.DynamicParameterTemplateParams{ + MainTF: string(must(os.ReadFile("testdata/parameters/dynamicimmutable/main.tf"))), + }) + + // Create the workspace with the mutable parameter + wrk, err := templateAdmin.CreateUserWorkspace(ctx, codersdk.Me, codersdk.CreateWorkspaceRequest{ + TemplateID: immutable.ID, + Name: coderdtest.RandomUsername(t), + RichParameterValues: []codersdk.WorkspaceBuildParameter{ + {Name: "isimmutable", Value: "false"}, + {Name: "immutable", Value: "coder"}, + }, + }) + require.NoError(t, err) + coderdtest.AwaitWorkspaceBuildJobCompleted(t, templateAdmin, wrk.LatestBuild.ID) + + // Switch it to immutable, which breaks the validation + _, err = templateAdmin.CreateWorkspaceBuild(ctx, wrk.ID, codersdk.CreateWorkspaceBuildRequest{ + Transition: codersdk.WorkspaceTransitionStart, + RichParameterValues: []codersdk.WorkspaceBuildParameter{ + {Name: "isimmutable", Value: "true"}, + {Name: "immutable", Value: "not-coder"}, + }, + }) + require.Error(t, err) + require.ErrorContains(t, err, "is not mutable") }) }) } From 4ce5004ce642f3cd7f56c8a28a1bd3d771d0e86f Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 7 Jul 2025 11:10:34 -0600 Subject: [PATCH 07/11] fixup --- coderd/dynamicparameters/resolver_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/dynamicparameters/resolver_test.go b/coderd/dynamicparameters/resolver_test.go index 3f936af350da2..2854b790986fc 100644 --- a/coderd/dynamicparameters/resolver_test.go +++ b/coderd/dynamicparameters/resolver_test.go @@ -69,7 +69,7 @@ func TestResolveParameters(t *testing.T) { Name: "immutable", Type: previewtypes.ParameterTypeString, FormType: provider.ParameterFormTypeInput, - Mutable: false, + Mutable: true, DefaultValue: previewtypes.StringLiteral("foo"), Required: true, } From 22489900a2bf5e51e8125b58cb84e34a2df365e4 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 7 Jul 2025 11:23:48 -0600 Subject: [PATCH 08/11] fix immutable check against --- coderd/dynamicparameters/resolver.go | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/coderd/dynamicparameters/resolver.go b/coderd/dynamicparameters/resolver.go index c2111ccd67422..08f40bc65b619 100644 --- a/coderd/dynamicparameters/resolver.go +++ b/coderd/dynamicparameters/resolver.go @@ -55,6 +55,16 @@ func ResolveParameters( values[preset.Name] = parameterValue{Source: sourcePreset, Value: preset.Value} } + // originalInputValues is going to be used to detect if a user tried to change + // an immutable parameter after the first build. + // The actual input values are mutated based on attributes like mutability + // and ephemerality. + originalInputValues := make(map[string]parameterValue, len(values)) + for name, value := range values { + // Store the original values for later use. + originalInputValues[name] = value + } + // Render the parameters using the values that were supplied to the previous build. // // This is how the form should look to the user on their workspace settings page. @@ -116,15 +126,15 @@ func ResolveParameters( parameterNames[parameter.Name] = struct{}{} if !firstBuild && !parameter.Mutable { - originalValue, ok := previousValuesMap[parameter.Name] + originalValue, ok := originalInputValues[parameter.Name] // Immutable parameters should not be changed after the first build. - // If the value matches the original value, that is fine. + // If the value matches the original input value, that is fine. // // If the original value is not set, that means this is a new parameter. New // immutable parameters are allowed. This is an opinionated choice to prevent // workspaces failing to update or delete. Ideally we would block this, as // immutable parameters should only be able to be set at creation time. - if ok && parameter.Value.AsString() != originalValue { + if ok && parameter.Value.AsString() != originalValue.Value { var src *hcl.Range if parameter.Source != nil { src = ¶meter.Source.HCLBlock().TypeRange From 4a5819676f3c0a1a885764436170f479b6dba326 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 7 Jul 2025 11:33:05 -0600 Subject: [PATCH 09/11] fix mock response --- coderd/dynamicparameters/resolver_test.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/coderd/dynamicparameters/resolver_test.go b/coderd/dynamicparameters/resolver_test.go index 2854b790986fc..994b2c7eb05c6 100644 --- a/coderd/dynamicparameters/resolver_test.go +++ b/coderd/dynamicparameters/resolver_test.go @@ -97,9 +97,8 @@ func TestResolveParameters(t *testing.T) { Parameters: []previewtypes.Parameter{ { ParameterData: immutable, - // User changes the value to 'bar' - Value: previewtypes.StringLiteral("bar"), - Diagnostics: nil, + Value: previewtypes.StringLiteral("foo"), + Diagnostics: nil, }, }, }, nil) From 35a2d8887d867a03570a6f72204e22ebb3482381 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 7 Jul 2025 11:52:17 -0600 Subject: [PATCH 10/11] file hash changing --- coderd/dynamicparameters/resolver.go | 4 ++-- enterprise/coderd/dynamicparameters_test.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/coderd/dynamicparameters/resolver.go b/coderd/dynamicparameters/resolver.go index 08f40bc65b619..0d3d7d7642e0a 100644 --- a/coderd/dynamicparameters/resolver.go +++ b/coderd/dynamicparameters/resolver.go @@ -126,7 +126,7 @@ func ResolveParameters( parameterNames[parameter.Name] = struct{}{} if !firstBuild && !parameter.Mutable { - originalValue, ok := originalInputValues[parameter.Name] + originalValue, ok := previousValuesMap[parameter.Name] // Immutable parameters should not be changed after the first build. // If the value matches the original input value, that is fine. // @@ -134,7 +134,7 @@ func ResolveParameters( // immutable parameters are allowed. This is an opinionated choice to prevent // workspaces failing to update or delete. Ideally we would block this, as // immutable parameters should only be able to be set at creation time. - if ok && parameter.Value.AsString() != originalValue.Value { + if ok && parameter.Value.AsString() != originalValue { var src *hcl.Range if parameter.Source != nil { src = ¶meter.Source.HCLBlock().TypeRange diff --git a/enterprise/coderd/dynamicparameters_test.go b/enterprise/coderd/dynamicparameters_test.go index 3b9b215b44e08..94a4158dc8354 100644 --- a/enterprise/coderd/dynamicparameters_test.go +++ b/enterprise/coderd/dynamicparameters_test.go @@ -364,7 +364,7 @@ func TestDynamicParameterBuild(t *testing.T) { ctx := testutil.Context(t, testutil.WaitShort) // Start with a new template that has 1 parameter that is immutable immutable, _ := coderdtest.DynamicParameterTemplate(t, templateAdmin, orgID, coderdtest.DynamicParameterTemplateParams{ - MainTF: string(must(os.ReadFile("testdata/parameters/dynamicimmutable/main.tf"))), + MainTF: "# PreviouslyImmutable\n" + string(must(os.ReadFile("testdata/parameters/dynamicimmutable/main.tf"))), }) // Create the workspace with the immutable parameter @@ -396,7 +396,7 @@ func TestDynamicParameterBuild(t *testing.T) { ctx := testutil.Context(t, testutil.WaitShort) immutable, _ := coderdtest.DynamicParameterTemplate(t, templateAdmin, orgID, coderdtest.DynamicParameterTemplateParams{ - MainTF: string(must(os.ReadFile("testdata/parameters/dynamicimmutable/main.tf"))), + MainTF: "# PreviouslyMutable\n" + string(must(os.ReadFile("testdata/parameters/dynamicimmutable/main.tf"))), }) // Create the workspace with the mutable parameter From 02d07976f50e6e9ffb05ef1e919d6b3609928671 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 7 Jul 2025 13:51:23 -0600 Subject: [PATCH 11/11] parameter values can change mutability --- coderd/dynamicparameters/resolver.go | 27 +++++++---------------- coderd/dynamicparameters/resolver_test.go | 5 +++-- 2 files changed, 11 insertions(+), 21 deletions(-) diff --git a/coderd/dynamicparameters/resolver.go b/coderd/dynamicparameters/resolver.go index 0d3d7d7642e0a..7fc67d29a0d55 100644 --- a/coderd/dynamicparameters/resolver.go +++ b/coderd/dynamicparameters/resolver.go @@ -93,22 +93,6 @@ func ResolveParameters( delete(values, parameter.Name) } } - - // Immutable parameters should also not be allowed to be changed from - // the previous build. Remove any values taken from the preset or - // new build params. This forces the value to be the same as it was before. - // - // We do this so the next form render uses the original immutable value. - if !firstBuild && !parameter.Mutable { - delete(values, parameter.Name) - prev, ok := previousValuesMap[parameter.Name] - if ok { - values[parameter.Name] = parameterValue{ - Value: prev, - Source: sourcePrevious, - } - } - } } // This is the final set of values that will be used. Any errors at this stage @@ -118,7 +102,7 @@ func ResolveParameters( return nil, parameterValidationError(diags) } - // parameterNames is going to be used to remove any excess values that were left + // parameterNames is going to be used to remove any excess values left // around without a parameter. parameterNames := make(map[string]struct{}, len(output.Parameters)) parameterError := parameterValidationError(nil) @@ -126,11 +110,16 @@ func ResolveParameters( parameterNames[parameter.Name] = struct{}{} if !firstBuild && !parameter.Mutable { + // previousValuesMap should be used over the first render output + // for the previous state of parameters. The previous build + // should emit all values, so the previousValuesMap should be + // complete with all parameter values (user specified and defaults) originalValue, ok := previousValuesMap[parameter.Name] + // Immutable parameters should not be changed after the first build. - // If the value matches the original input value, that is fine. + // If the value matches the previous input value, that is fine. // - // If the original value is not set, that means this is a new parameter. New + // If the previous value is not set, that means this is a new parameter. New // immutable parameters are allowed. This is an opinionated choice to prevent // workspaces failing to update or delete. Ideally we would block this, as // immutable parameters should only be able to be set at creation time. diff --git a/coderd/dynamicparameters/resolver_test.go b/coderd/dynamicparameters/resolver_test.go index 994b2c7eb05c6..e6675e6f4c7dc 100644 --- a/coderd/dynamicparameters/resolver_test.go +++ b/coderd/dynamicparameters/resolver_test.go @@ -97,8 +97,9 @@ func TestResolveParameters(t *testing.T) { Parameters: []previewtypes.Parameter{ { ParameterData: immutable, - Value: previewtypes.StringLiteral("foo"), - Diagnostics: nil, + // The user set the value to bar + Value: previewtypes.StringLiteral("bar"), + Diagnostics: nil, }, }, }, nil)