diff --git a/CODEOWNERS b/CODEOWNERS index a24dfad099030..327c43dd3bb81 100644 --- a/CODEOWNERS +++ b/CODEOWNERS @@ -4,3 +4,5 @@ agent/proto/ @spikecurtis @johnstcn tailnet/proto/ @spikecurtis @johnstcn vpn/vpn.proto @spikecurtis @johnstcn vpn/version.go @spikecurtis @johnstcn +provisionerd/proto/ @spikecurtis @johnstcn +provisionersdk/proto/ @spikecurtis @johnstcn diff --git a/agent/agentscripts/agentscripts.go b/agent/agentscripts/agentscripts.go index 4e4921b87ee5b..79606a80233b9 100644 --- a/agent/agentscripts/agentscripts.go +++ b/agent/agentscripts/agentscripts.go @@ -10,7 +10,6 @@ import ( "os/user" "path/filepath" "sync" - "sync/atomic" "time" "github.com/google/uuid" @@ -104,7 +103,6 @@ type Runner struct { closed chan struct{} closeMutex sync.Mutex cron *cron.Cron - initialized atomic.Bool scripts []runnerScript dataDir string scriptCompleted ScriptCompletedFunc @@ -113,6 +111,9 @@ type Runner struct { // execute startup scripts, and scripts on a cron schedule. Both will increment // this counter. scriptsExecuted *prometheus.CounterVec + + initMutex sync.Mutex + initialized bool } // DataDir returns the directory where scripts data is stored. @@ -154,10 +155,12 @@ func WithPostStartScripts(scripts ...codersdk.WorkspaceAgentScript) InitOption { // It also schedules any scripts that have a schedule. // This function must be called before Execute. func (r *Runner) Init(scripts []codersdk.WorkspaceAgentScript, scriptCompleted ScriptCompletedFunc, opts ...InitOption) error { - if r.initialized.Load() { + r.initMutex.Lock() + defer r.initMutex.Unlock() + if r.initialized { return xerrors.New("init: already initialized") } - r.initialized.Store(true) + r.initialized = true r.scripts = toRunnerScript(scripts...) r.scriptCompleted = scriptCompleted for _, opt := range opts { @@ -227,6 +230,18 @@ const ( // Execute runs a set of scripts according to a filter. func (r *Runner) Execute(ctx context.Context, option ExecuteOption) error { + initErr := func() error { + r.initMutex.Lock() + defer r.initMutex.Unlock() + if !r.initialized { + return xerrors.New("execute: not initialized") + } + return nil + }() + if initErr != nil { + return initErr + } + var eg errgroup.Group for _, script := range r.scripts { runScript := (option == ExecuteStartScripts && script.RunOnStart) || diff --git a/codersdk/richparameters.go b/codersdk/richparameters.go index 2ddd5d00f6c41..6fc6b8e0c343f 100644 --- a/codersdk/richparameters.go +++ b/codersdk/richparameters.go @@ -164,7 +164,7 @@ type ParameterResolver struct { // resolves the correct value. It returns the value of the parameter, if valid, and an error if invalid. func (r *ParameterResolver) ValidateResolve(p TemplateVersionParameter, v *WorkspaceBuildParameter) (value string, err error) { prevV := r.findLastValue(p) - if !p.Mutable && v != nil && prevV != nil { + if !p.Mutable && v != nil && prevV != nil && v.Value != prevV.Value { return "", xerrors.Errorf("Parameter %q is not mutable, so it can't be updated after creating a workspace.", p.Name) } if p.Required && v == nil && prevV == nil { diff --git a/codersdk/richparameters_test.go b/codersdk/richparameters_test.go index 16365f7c2f416..5635a82beb6c6 100644 --- a/codersdk/richparameters_test.go +++ b/codersdk/richparameters_test.go @@ -1,6 +1,7 @@ package codersdk_test import ( + "fmt" "testing" "github.com/stretchr/testify/require" @@ -121,20 +122,60 @@ func TestParameterResolver_ValidateResolve_NewOverridesOld(t *testing.T) { func TestParameterResolver_ValidateResolve_Immutable(t *testing.T) { t.Parallel() uut := codersdk.ParameterResolver{ - Rich: []codersdk.WorkspaceBuildParameter{{Name: "n", Value: "5"}}, + Rich: []codersdk.WorkspaceBuildParameter{{Name: "n", Value: "old"}}, } p := codersdk.TemplateVersionParameter{ Name: "n", - Type: "number", + Type: "string", Required: true, Mutable: false, } - v, err := uut.ValidateResolve(p, &codersdk.WorkspaceBuildParameter{ - Name: "n", - Value: "6", - }) - require.Error(t, err) - require.Equal(t, "", v) + + cases := []struct { + name string + newValue string + expectedErr string + }{ + { + name: "mutation", + newValue: "new", // "new" != "old" + expectedErr: fmt.Sprintf("Parameter %q is not mutable", p.Name), + }, + { + // Values are case-sensitive. + name: "case change", + newValue: "Old", // "Old" != "old" + expectedErr: fmt.Sprintf("Parameter %q is not mutable", p.Name), + }, + { + name: "default", + newValue: "", // "" != "old" + expectedErr: fmt.Sprintf("Parameter %q is not mutable", p.Name), + }, + { + name: "no change", + newValue: "old", // "old" == "old" + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + v, err := uut.ValidateResolve(p, &codersdk.WorkspaceBuildParameter{ + Name: "n", + Value: tc.newValue, + }) + + if tc.expectedErr == "" { + require.NoError(t, err) + require.Equal(t, tc.newValue, v) + } else { + require.ErrorContains(t, err, tc.expectedErr) + require.Equal(t, "", v) + } + }) + } } func TestRichParameterValidation(t *testing.T) { diff --git a/codersdk/toolsdk/toolsdk.go b/codersdk/toolsdk/toolsdk.go index 024e3bad6efdc..166bde730efc5 100644 --- a/codersdk/toolsdk/toolsdk.go +++ b/codersdk/toolsdk/toolsdk.go @@ -327,6 +327,7 @@ var ListWorkspaces = Tool[ListWorkspacesArgs, []MinimalWorkspace]{ "description": "The owner of the workspaces to list. Use \"me\" to list workspaces for the authenticated user. If you do not specify an owner, \"me\" will be assumed by default.", }, }, + Required: []string{}, }, }, Handler: func(ctx context.Context, deps Deps, args ListWorkspacesArgs) ([]MinimalWorkspace, error) { @@ -1256,6 +1257,7 @@ var DeleteTemplate = Tool[DeleteTemplateArgs, codersdk.Response]{ "type": "string", }, }, + Required: []string{"template_id"}, }, }, Handler: func(ctx context.Context, deps Deps, args DeleteTemplateArgs) (codersdk.Response, error) { diff --git a/codersdk/toolsdk/toolsdk_test.go b/codersdk/toolsdk/toolsdk_test.go index fae4e85e52a66..f9c35dba5951d 100644 --- a/codersdk/toolsdk/toolsdk_test.go +++ b/codersdk/toolsdk/toolsdk_test.go @@ -539,6 +539,33 @@ func TestWithCleanContext(t *testing.T) { }) } +func TestToolSchemaFields(t *testing.T) { + t.Parallel() + + // Test that all tools have the required Schema fields (Properties and Required) + for _, tool := range toolsdk.All { + t.Run(tool.Tool.Name, func(t *testing.T) { + t.Parallel() + + // Check that Properties is not nil + require.NotNil(t, tool.Tool.Schema.Properties, + "Tool %q missing Schema.Properties", tool.Tool.Name) + + // Check that Required is not nil + require.NotNil(t, tool.Tool.Schema.Required, + "Tool %q missing Schema.Required", tool.Tool.Name) + + // Ensure Properties has entries for all required fields + for _, requiredField := range tool.Tool.Schema.Required { + _, exists := tool.Tool.Schema.Properties[requiredField] + require.True(t, exists, + "Tool %q requires field %q but it is not defined in Properties", + tool.Tool.Name, requiredField) + } + }) + } +} + // TestMain runs after all tests to ensure that all tools in this package have // been tested once. func TestMain(m *testing.M) { diff --git a/enterprise/coderd/prebuilds/claim_test.go b/enterprise/coderd/prebuilds/claim_test.go index 145095e6533e7..ad31d2b4eff1b 100644 --- a/enterprise/coderd/prebuilds/claim_test.go +++ b/enterprise/coderd/prebuilds/claim_test.go @@ -329,9 +329,8 @@ func TestClaimPrebuild(t *testing.T) { require.NoError(t, err) stopBuild, err := userClient.CreateWorkspaceBuild(ctx, workspace.ID, codersdk.CreateWorkspaceBuildRequest{ - TemplateVersionID: version.ID, - Transition: codersdk.WorkspaceTransitionStop, - RichParameterValues: wp, + TemplateVersionID: version.ID, + Transition: codersdk.WorkspaceTransitionStop, }) require.NoError(t, err) coderdtest.AwaitWorkspaceBuildJobCompleted(t, userClient, stopBuild.ID) @@ -369,6 +368,17 @@ func templateWithAgentAndPresetsWithPrebuilds(desiredInstances int32) *echo.Resp }, }, }, + // Make sure immutable params don't break claiming logic + Parameters: []*proto.RichParameter{ + { + Name: "k1", + Description: "immutable param", + Type: "string", + DefaultValue: "", + Required: false, + Mutable: false, + }, + }, Presets: []*proto.Preset{ { Name: "preset-a", diff --git a/enterprise/coderd/prebuilds/reconcile_test.go b/enterprise/coderd/prebuilds/reconcile_test.go index 9783b215f185b..bc886fc0a8231 100644 --- a/enterprise/coderd/prebuilds/reconcile_test.go +++ b/enterprise/coderd/prebuilds/reconcile_test.go @@ -901,6 +901,16 @@ func setupTestDBTemplateVersion( ID: templateID, ActiveVersionID: templateVersion.ID, })) + // Make sure immutable params don't break prebuilt workspace deletion logic + dbgen.TemplateVersionParameter(t, db, database.TemplateVersionParameter{ + TemplateVersionID: templateVersion.ID, + Name: "test", + Description: "required & immutable param", + Type: "string", + DefaultValue: "", + Required: true, + Mutable: false, + }) return templateVersion.ID } @@ -999,7 +1009,7 @@ func setupTestDBWorkspace( OrganizationID: orgID, Error: buildError, }) - dbgen.WorkspaceBuild(t, db, database.WorkspaceBuild{ + workspaceBuild := dbgen.WorkspaceBuild(t, db, database.WorkspaceBuild{ WorkspaceID: workspace.ID, InitiatorID: initiatorID, TemplateVersionID: templateVersionID, @@ -1008,6 +1018,13 @@ func setupTestDBWorkspace( Transition: transition, CreatedAt: clock.Now(), }) + dbgen.WorkspaceBuildParameters(t, db, []database.WorkspaceBuildParameter{ + { + WorkspaceBuildID: workspaceBuild.ID, + Name: "test", + Value: "test", + }, + }) return workspace }