diff --git a/coderd/dynamicparameters/resolver.go b/coderd/dynamicparameters/resolver.go index bd8e2294cf..7fc67d29a0 100644 --- a/coderd/dynamicparameters/resolver.go +++ b/coderd/dynamicparameters/resolver.go @@ -55,19 +55,21 @@ 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 + // originalInputValues 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)) + // 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. - originalValues[name] = value + 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. // 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 @@ -91,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 @@ -116,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) @@ -124,15 +110,20 @@ func ResolveParameters( parameterNames[parameter.Name] = struct{}{} if !firstBuild && !parameter.Mutable { - originalValue, ok := originalValues[parameter.Name] + // 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 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. - 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 ec5218613f..e6675e6f4c 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: true, + 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, + // The user set 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 e13d370a05..94a4158dc8 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,75 @@ func TestDynamicParameterBuild(t *testing.T) { require.NoError(t, err) require.Equal(t, wrk.ID, deleted.ID, "workspace should be deleted") }) + + 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 new state. + // So the value is allowed to be changed. + 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: "# PreviouslyImmutable\n" + string(must(os.ReadFile("testdata/parameters/dynamicimmutable/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: "isimmutable", Value: "true"}, + {Name: "immutable", Value: "coder"}, + }, + }) + require.NoError(t, err) + coderdtest.AwaitWorkspaceBuildJobCompleted(t, templateAdmin, wrk.LatestBuild.ID) + + // Try new values + _, 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) + }) + + 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: "# PreviouslyMutable\n" + 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") + }) }) } diff --git a/enterprise/coderd/testdata/parameters/dynamicimmutable/main.tf b/enterprise/coderd/testdata/parameters/dynamicimmutable/main.tf new file mode 100644 index 0000000000..08bdd3336f --- /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" +}