chore: modify parameter dynamic immutability behavior (#18583)

Immutability behavior is determined by the current build, not affected by the previous

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 = &parameter.Source.HCLBlock().TypeRange

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

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

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"
+}