From dedc32fb1ad53a4370899fc92c2ba5ac6fadd29a Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Thu, 20 Feb 2025 09:58:04 +0200 Subject: [PATCH] fix(coderd): avoid fetching extra parameters for a preset (#16642) This pull request fixes a bug in presets and adds tests to ensure it doesn't happen again. Due to an oversight in refactoring, we returned extra and incorrect parameters from other presets in the same template version when calling `/templateversions/{templateversion}/presets`. --- coderd/presets.go | 4 ++ coderd/presets_test.go | 152 ++++++++++++++++++++++++++++++----------- 2 files changed, 115 insertions(+), 41 deletions(-) diff --git a/coderd/presets.go b/coderd/presets.go index a2787b63e7..1b5f646438 100644 --- a/coderd/presets.go +++ b/coderd/presets.go @@ -45,6 +45,10 @@ func (api *API) templateVersionPresets(rw http.ResponseWriter, r *http.Request) Name: preset.Name, } for _, presetParam := range presetParams { + if presetParam.TemplateVersionPresetID != preset.ID { + continue + } + sdkPreset.Parameters = append(sdkPreset.Parameters, codersdk.PresetParameter{ Name: presetParam.Name, Value: presetParam.Value, diff --git a/coderd/presets_test.go b/coderd/presets_test.go index 96d1a03e94..08ff7c76f2 100644 --- a/coderd/presets_test.go +++ b/coderd/presets_test.go @@ -17,58 +17,128 @@ import ( func TestTemplateVersionPresets(t *testing.T) { t.Parallel() - givenPreset := codersdk.Preset{ - Name: "My Preset", - Parameters: []codersdk.PresetParameter{ - { - Name: "preset_param1", - Value: "A1B2C3", + testCases := []struct { + name string + presets []codersdk.Preset + }{ + { + name: "no presets", + presets: []codersdk.Preset{}, + }, + { + name: "single preset with parameters", + presets: []codersdk.Preset{ + { + Name: "My Preset", + Parameters: []codersdk.PresetParameter{ + { + Name: "preset_param1", + Value: "A1B2C3", + }, + { + Name: "preset_param2", + Value: "D4E5F6", + }, + }, + }, }, - { - Name: "preset_param2", - Value: "D4E5F6", + }, + { + name: "multiple presets with overlapping parameters", + presets: []codersdk.Preset{ + { + Name: "Preset 1", + Parameters: []codersdk.PresetParameter{ + { + Name: "shared_param", + Value: "value1", + }, + { + Name: "unique_param1", + Value: "unique1", + }, + }, + }, + { + Name: "Preset 2", + Parameters: []codersdk.PresetParameter{ + { + Name: "shared_param", + Value: "value2", + }, + { + Name: "unique_param2", + Value: "unique2", + }, + }, + }, }, }, } - ctx := testutil.Context(t, testutil.WaitShort) - client, db := coderdtest.NewWithDatabase(t, &coderdtest.Options{IncludeProvisionerDaemon: true}) - user := coderdtest.CreateFirstUser(t, client) - version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) + for _, tc := range testCases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitShort) - // nolint:gocritic // This is a test - provisionerCtx := dbauthz.AsProvisionerd(ctx) + client, db := coderdtest.NewWithDatabase(t, &coderdtest.Options{IncludeProvisionerDaemon: true}) + user := coderdtest.CreateFirstUser(t, client) + version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) - dbPreset, err := db.InsertPreset(provisionerCtx, database.InsertPresetParams{ - Name: givenPreset.Name, - TemplateVersionID: version.ID, - }) - require.NoError(t, err) + // nolint:gocritic // This is a test + provisionerCtx := dbauthz.AsProvisionerd(ctx) - var presetParameterNames []string - var presetParameterValues []string - for _, presetParameter := range givenPreset.Parameters { - presetParameterNames = append(presetParameterNames, presetParameter.Name) - presetParameterValues = append(presetParameterValues, presetParameter.Value) - } - _, err = db.InsertPresetParameters(provisionerCtx, database.InsertPresetParametersParams{ - TemplateVersionPresetID: dbPreset.ID, - Names: presetParameterNames, - Values: presetParameterValues, - }) - require.NoError(t, err) + // Insert all presets for this test case + for _, givenPreset := range tc.presets { + dbPreset, err := db.InsertPreset(provisionerCtx, database.InsertPresetParams{ + Name: givenPreset.Name, + TemplateVersionID: version.ID, + }) + require.NoError(t, err) - userSubject, _, err := httpmw.UserRBACSubject(ctx, db, user.UserID, rbac.ScopeAll) - require.NoError(t, err) - userCtx := dbauthz.As(ctx, userSubject) + if len(givenPreset.Parameters) > 0 { + var presetParameterNames []string + var presetParameterValues []string + for _, presetParameter := range givenPreset.Parameters { + presetParameterNames = append(presetParameterNames, presetParameter.Name) + presetParameterValues = append(presetParameterValues, presetParameter.Value) + } + _, err = db.InsertPresetParameters(provisionerCtx, database.InsertPresetParametersParams{ + TemplateVersionPresetID: dbPreset.ID, + Names: presetParameterNames, + Values: presetParameterValues, + }) + require.NoError(t, err) + } + } - gotPresets, err := client.TemplateVersionPresets(userCtx, version.ID) - require.NoError(t, err) + userSubject, _, err := httpmw.UserRBACSubject(ctx, db, user.UserID, rbac.ScopeAll) + require.NoError(t, err) + userCtx := dbauthz.As(ctx, userSubject) - require.Equal(t, 1, len(gotPresets)) - require.Equal(t, givenPreset.Name, gotPresets[0].Name) + gotPresets, err := client.TemplateVersionPresets(userCtx, version.ID) + require.NoError(t, err) - for _, presetParameter := range givenPreset.Parameters { - require.Contains(t, gotPresets[0].Parameters, presetParameter) + require.Equal(t, len(tc.presets), len(gotPresets)) + + for _, expectedPreset := range tc.presets { + found := false + for _, gotPreset := range gotPresets { + if gotPreset.Name == expectedPreset.Name { + found = true + + // verify not only that we get the right number of parameters, but that we get the right parameters + // This ensures that we don't get extra parameters from other presets + require.Equal(t, len(expectedPreset.Parameters), len(gotPreset.Parameters)) + for _, expectedParam := range expectedPreset.Parameters { + require.Contains(t, gotPreset.Parameters, expectedParam) + } + break + } + } + require.True(t, found, "Expected preset %s not found in results", expectedPreset.Name) + } + }) } }