Skip to content

Commit 00ba027

Browse files
EmyrkEmyrk
authored
chore: modify parameter dynamic immutability behavior (#18583)
Immutability behavior is determined by the current build, not affected by the previous
1 parent 9c61ef8 commit 00ba027

File tree

4 files changed

+174
-26
lines changed

4 files changed

+174
-26
lines changed

‎coderd/dynamicparameters/resolver.go

Lines changed: 16 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -55,19 +55,21 @@ func ResolveParameters(
5555
values[preset.Name] = parameterValue{Source: sourcePreset, Value: preset.Value}
5656
}
5757

58-
// originalValues is going to be used to detect if a user tried to change
58+
// originalInputValues is going to be used to detect if a user tried to change
5959
// an immutable parameter after the first build.
60-
originalValues := make(map[string]parameterValue, len(values))
60+
// The actual input values are mutated based on attributes like mutability
61+
// and ephemerality.
62+
originalInputValues := make(map[string]parameterValue, len(values))
6163
for name, value := range values {
6264
// Store the original values for later use.
63-
originalValues[name] = value
65+
originalInputValues[name] = value
6466
}
6567

6668
// Render the parameters using the values that were supplied to the previous build.
6769
//
6870
// This is how the form should look to the user on their workspace settings page.
6971
// This is the original form truth that our validations should initially be based on.
70-
output, diags := renderer.Render(ctx, ownerID, values.ValuesMap())
72+
output, diags := renderer.Render(ctx, ownerID, previousValuesMap)
7173
if diags.HasErrors() {
7274
// Top level diagnostics should break the build. Previous values (and new) should
7375
// always be valid. If there is a case where this is not true, then this has to
@@ -91,22 +93,6 @@ func ResolveParameters(
9193
delete(values, parameter.Name)
9294
}
9395
}
94-
95-
// Immutable parameters should also not be allowed to be changed from
96-
// the previous build. Remove any values taken from the preset or
97-
// new build params. This forces the value to be the same as it was before.
98-
//
99-
// We do this so the next form render uses the original immutable value.
100-
if !firstBuild && !parameter.Mutable {
101-
delete(values, parameter.Name)
102-
prev, ok := previousValuesMap[parameter.Name]
103-
if ok {
104-
values[parameter.Name] = parameterValue{
105-
Value: prev,
106-
Source: sourcePrevious,
107-
}
108-
}
109-
}
11096
}
11197

11298
// This is the final set of values that will be used. Any errors at this stage
@@ -116,23 +102,28 @@ func ResolveParameters(
116102
return nil, parameterValidationError(diags)
117103
}
118104

119-
// parameterNames is going to be used to remove any excess values that were left
105+
// parameterNames is going to be used to remove any excess values left
120106
// around without a parameter.
121107
parameterNames := make(map[string]struct{}, len(output.Parameters))
122108
parameterError := parameterValidationError(nil)
123109
for _, parameter := range output.Parameters {
124110
parameterNames[parameter.Name] = struct{}{}
125111

126112
if !firstBuild && !parameter.Mutable {
127-
originalValue, ok := originalValues[parameter.Name]
113+
// previousValuesMap should be used over the first render output
114+
// for the previous state of parameters. The previous build
115+
// should emit all values, so the previousValuesMap should be
116+
// complete with all parameter values (user specified and defaults)
117+
originalValue, ok := previousValuesMap[parameter.Name]
118+
128119
// Immutable parameters should not be changed after the first build.
129-
// If the value matches the original value, that is fine.
120+
// If the value matches the previous input value, that is fine.
130121
//
131-
// If the original value is not set, that means this is a new parameter. New
122+
// If the previous value is not set, that means this is a new parameter. New
132123
// immutable parameters are allowed. This is an opinionated choice to prevent
133124
// workspaces failing to update or delete. Ideally we would block this, as
134125
// immutable parameters should only be able to be set at creation time.
135-
if ok && parameter.Value.AsString() != originalValue.Value {
126+
if ok && parameter.Value.AsString() != originalValue {
136127
var src *hcl.Range
137128
if parameter.Source != nil {
138129
src = &parameter.Source.HCLBlock().TypeRange

‎coderd/dynamicparameters/resolver_test.go

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"github.com/coder/coder/v2/coderd/database"
1111
"github.com/coder/coder/v2/coderd/dynamicparameters"
1212
"github.com/coder/coder/v2/coderd/dynamicparameters/rendermock"
13+
"github.com/coder/coder/v2/coderd/httpapi/httperror"
1314
"github.com/coder/coder/v2/codersdk"
1415
"github.com/coder/coder/v2/testutil"
1516
"github.com/coder/preview"
@@ -56,4 +57,69 @@ func TestResolveParameters(t *testing.T) {
5657
require.NoError(t, err)
5758
require.Equal(t, map[string]string{"immutable": "foo"}, values)
5859
})
60+
61+
// Tests a parameter going from mutable -> immutable
62+
t.Run("BecameImmutable", func(t *testing.T) {
63+
t.Parallel()
64+
65+
ctrl := gomock.NewController(t)
66+
render := rendermock.NewMockRenderer(ctrl)
67+
68+
mutable := previewtypes.ParameterData{
69+
Name: "immutable",
70+
Type: previewtypes.ParameterTypeString,
71+
FormType: provider.ParameterFormTypeInput,
72+
Mutable: true,
73+
DefaultValue: previewtypes.StringLiteral("foo"),
74+
Required: true,
75+
}
76+
immutable := mutable
77+
immutable.Mutable = false
78+
79+
// A single immutable parameter with no previous value.
80+
render.EXPECT().
81+
Render(gomock.Any(), gomock.Any(), gomock.Any()).
82+
// Return the mutable param first
83+
Return(&preview.Output{
84+
Parameters: []previewtypes.Parameter{
85+
{
86+
ParameterData: mutable,
87+
Value: previewtypes.StringLiteral("foo"),
88+
Diagnostics: nil,
89+
},
90+
},
91+
}, nil)
92+
93+
render.EXPECT().
94+
Render(gomock.Any(), gomock.Any(), gomock.Any()).
95+
// Then the immutable param
96+
Return(&preview.Output{
97+
Parameters: []previewtypes.Parameter{
98+
{
99+
ParameterData: immutable,
100+
// The user set the value to bar
101+
Value: previewtypes.StringLiteral("bar"),
102+
Diagnostics: nil,
103+
},
104+
},
105+
}, nil)
106+
107+
ctx := testutil.Context(t, testutil.WaitShort)
108+
_, err := dynamicparameters.ResolveParameters(ctx, uuid.New(), render, false,
109+
[]database.WorkspaceBuildParameter{
110+
{Name: "immutable", Value: "foo"}, // Previous value foo
111+
},
112+
[]codersdk.WorkspaceBuildParameter{
113+
{Name: "immutable", Value: "bar"}, // New value
114+
},
115+
[]database.TemplateVersionPresetParameter{}, // No preset values
116+
)
117+
require.Error(t, err)
118+
resp, ok := httperror.IsResponder(err)
119+
require.True(t, ok)
120+
121+
_, respErr := resp.Response()
122+
require.Len(t, respErr.Validations, 1)
123+
require.Contains(t, respErr.Validations[0].Error(), "is not mutable")
124+
})
59125
}

‎enterprise/coderd/dynamicparameters_test.go

Lines changed: 69 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -338,7 +338,6 @@ func TestDynamicParameterBuild(t *testing.T) {
338338
bld, err := templateAdmin.CreateWorkspaceBuild(ctx, wrk.ID, codersdk.CreateWorkspaceBuildRequest{
339339
TemplateVersionID: immutable.ID, // Use the new template version with the immutable parameter
340340
Transition: codersdk.WorkspaceTransitionDelete,
341-
DryRun: false,
342341
})
343342
require.NoError(t, err)
344343
coderdtest.AwaitWorkspaceBuildJobCompleted(t, templateAdmin, bld.ID)
@@ -354,6 +353,75 @@ func TestDynamicParameterBuild(t *testing.T) {
354353
require.NoError(t, err)
355354
require.Equal(t, wrk.ID, deleted.ID, "workspace should be deleted")
356355
})
356+
357+
t.Run("PreviouslyImmutable", func(t *testing.T) {
358+
// Ok this is a weird test to document how things are working.
359+
// What if a parameter flips it's immutability based on a value?
360+
// The current behavior is to source immutability from the new state.
361+
// So the value is allowed to be changed.
362+
t.Parallel()
363+
364+
ctx := testutil.Context(t, testutil.WaitShort)
365+
// Start with a new template that has 1 parameter that is immutable
366+
immutable, _ := coderdtest.DynamicParameterTemplate(t, templateAdmin, orgID, coderdtest.DynamicParameterTemplateParams{
367+
MainTF: "# PreviouslyImmutable\n" + string(must(os.ReadFile("testdata/parameters/dynamicimmutable/main.tf"))),
368+
})
369+
370+
// Create the workspace with the immutable parameter
371+
wrk, err := templateAdmin.CreateUserWorkspace(ctx, codersdk.Me, codersdk.CreateWorkspaceRequest{
372+
TemplateID: immutable.ID,
373+
Name: coderdtest.RandomUsername(t),
374+
RichParameterValues: []codersdk.WorkspaceBuildParameter{
375+
{Name: "isimmutable", Value: "true"},
376+
{Name: "immutable", Value: "coder"},
377+
},
378+
})
379+
require.NoError(t, err)
380+
coderdtest.AwaitWorkspaceBuildJobCompleted(t, templateAdmin, wrk.LatestBuild.ID)
381+
382+
// Try new values
383+
_, err = templateAdmin.CreateWorkspaceBuild(ctx, wrk.ID, codersdk.CreateWorkspaceBuildRequest{
384+
Transition: codersdk.WorkspaceTransitionStart,
385+
RichParameterValues: []codersdk.WorkspaceBuildParameter{
386+
{Name: "isimmutable", Value: "false"},
387+
{Name: "immutable", Value: "not-coder"},
388+
},
389+
})
390+
require.NoError(t, err)
391+
})
392+
393+
t.Run("PreviouslyMutable", func(t *testing.T) {
394+
// The value cannot be changed because it becomes immutable.
395+
t.Parallel()
396+
397+
ctx := testutil.Context(t, testutil.WaitShort)
398+
immutable, _ := coderdtest.DynamicParameterTemplate(t, templateAdmin, orgID, coderdtest.DynamicParameterTemplateParams{
399+
MainTF: "# PreviouslyMutable\n" + string(must(os.ReadFile("testdata/parameters/dynamicimmutable/main.tf"))),
400+
})
401+
402+
// Create the workspace with the mutable parameter
403+
wrk, err := templateAdmin.CreateUserWorkspace(ctx, codersdk.Me, codersdk.CreateWorkspaceRequest{
404+
TemplateID: immutable.ID,
405+
Name: coderdtest.RandomUsername(t),
406+
RichParameterValues: []codersdk.WorkspaceBuildParameter{
407+
{Name: "isimmutable", Value: "false"},
408+
{Name: "immutable", Value: "coder"},
409+
},
410+
})
411+
require.NoError(t, err)
412+
coderdtest.AwaitWorkspaceBuildJobCompleted(t, templateAdmin, wrk.LatestBuild.ID)
413+
414+
// Switch it to immutable, which breaks the validation
415+
_, err = templateAdmin.CreateWorkspaceBuild(ctx, wrk.ID, codersdk.CreateWorkspaceBuildRequest{
416+
Transition: codersdk.WorkspaceTransitionStart,
417+
RichParameterValues: []codersdk.WorkspaceBuildParameter{
418+
{Name: "isimmutable", Value: "true"},
419+
{Name: "immutable", Value: "not-coder"},
420+
},
421+
})
422+
require.Error(t, err)
423+
require.ErrorContains(t, err, "is not mutable")
424+
})
357425
})
358426
}
359427

‎enterprise/coderd/testdata/parameters/dynamicimmutable/main.tf

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
terraform {
2+
required_providers {
3+
coder = {
4+
source = "coder/coder"
5+
}
6+
}
7+
}
8+
9+
data "coder_workspace_owner" "me" {}
10+
11+
data "coder_parameter" "isimmutable" {
12+
name = "isimmutable"
13+
type = "bool"
14+
mutable = true
15+
default = "true"
16+
}
17+
18+
data "coder_parameter" "immutable" {
19+
name = "immutable"
20+
type = "string"
21+
mutable = data.coder_parameter.isimmutable.value == "false"
22+
default = "Hello World"
23+
}

0 commit comments

Comments
 (0)