diff --git a/docs/appendices/file-formats/app-json.md b/docs/appendices/file-formats/app-json.md index 4146aeb82..dd55093cc 100644 --- a/docs/appendices/file-formats/app-json.md +++ b/docs/appendices/file-formats/app-json.md @@ -158,7 +158,7 @@ On subsequent deploys: - `autoscaling` (map of string to object, optional) autoscaling rules. See the autoscaling section for more details - `max_parallel`: (int, optional) number of instances to deploy in parallel at a given time -- `quantity`: (int, optional) number of processes to maintain. Default 1 for web processes, 0 for all others. +- `quantity`: (int, optional) number of processes to maintain. Must be zero or greater. Default 1 for web processes, 0 for all others. - `service`: (map of string to oject, optional) governs how non-web processes are exposed as services on the network ### Autoscaling diff --git a/docs/development/plugin-triggers.md b/docs/development/plugin-triggers.md index 247239ed8..1a15fdee1 100644 --- a/docs/development/plugin-triggers.md +++ b/docs/development/plugin-triggers.md @@ -2396,7 +2396,7 @@ set -eo pipefail; [[ $DOKKU_TRACE ]] && set -x ### `ps-set-scale` -- Description: Sets the scale for an app based on a specified formation (process-type=quantity). Any unspecified process types will be left as is, or have their process count set to zero when `$CLEAR_EXISTING` is `true`. +- Description: Sets the scale for an app based on a specified formation (process-type=quantity). Any unspecified process types will be left as is, or have their process count set to zero when `$CLEAR_EXISTING` is `true`. Each process tuple must name a process type, and process counts must be zero or greater. - Invoked by: - Arguments: `$APP $SKIP_DEPLOY $CLEAR_EXISTING [$PROCESS_TUPLE...]` - Example: diff --git a/docs/processes/process-management.md b/docs/processes/process-management.md index 1a69b49f9..87526890b 100644 --- a/docs/processes/process-management.md +++ b/docs/processes/process-management.md @@ -248,6 +248,9 @@ Multiple process types can be scaled at once: dokku ps:scale node-js-app web=1 worker=1 ``` +> [!NOTE] +> Each argument must name a process type, and process counts must be zero or greater. Scaling a process type to `0` stops all of its containers, while a negative count is rejected. + If desired, the corresponding deploy will be skipped by using the `--skip-deploy` flag: ```shell diff --git a/plugins/app-json/functions.go b/plugins/app-json/functions.go index 5beeb3bda..1422c37f5 100644 --- a/plugins/app-json/functions.go +++ b/plugins/app-json/functions.go @@ -431,6 +431,10 @@ func setScale(appName string) error { args := []string{appName, strconv.FormatBool(skipDeploy), strconv.FormatBool(clearExisting)} for processType, formation := range appJSON.Formation { if formation.Quantity != nil { + if *formation.Quantity < 0 { + return fmt.Errorf("Invalid quantity for process type %s in app.json: value must be zero or greater", processType) + } + args = append(args, fmt.Sprintf("%s=%d", processType, *formation.Quantity)) } } diff --git a/plugins/ps/functions.go b/plugins/ps/functions.go index 6b48c1a49..03a4567df 100644 --- a/plugins/ps/functions.go +++ b/plugins/ps/functions.go @@ -196,7 +196,7 @@ func parseProcessTuples(processTuples []string) (FormationSlice, error) { processType := strings.TrimSpace(s[0]) quantity, err := strconv.Atoi(strings.TrimSpace(s[1])) if err != nil { - return formations, fmt.Errorf("Invalid count for process type %s", s[0]) + return formations, fmt.Errorf("Invalid count for process type %s", processType) } if foundFormations[processType] { @@ -351,6 +351,10 @@ func scaleSet(input scaleSetInput) error { return err } + if err := validateFormations(formations); err != nil { + return err + } + if input.clearExisting { formations = appendClearedFormations(formations, existingFormations) } @@ -484,3 +488,20 @@ func updateScale(appName string, clearExisting bool, formationUpdates FormationS return common.PropertyListWrite("ps", appName, "scale", values) } + +// validateFormations returns an error when a user specified formation cannot be scaled to. +// Formations read back from the scale property are deliberately not validated so that a value +// persisted by an older dokku version remains visible and repairable via ps:scale. +func validateFormations(formations FormationSlice) error { + for _, formation := range formations { + if formation.ProcessType == "" { + return fmt.Errorf("Missing process type for count %d", formation.Quantity) + } + + if formation.Quantity < 0 { + return fmt.Errorf("Invalid count for process type %s: value must be zero or greater", formation.ProcessType) + } + } + + return nil +} diff --git a/plugins/ps/functions_test.go b/plugins/ps/functions_test.go index 793660743..fc2634353 100644 --- a/plugins/ps/functions_test.go +++ b/plugins/ps/functions_test.go @@ -90,3 +90,160 @@ func TestDefaultProcessTuplesWithoutProcfile(t *testing.T) { t.Fatalf("expected [web=1], got %v", processTuples) } } + +func TestParseProcessTuples(t *testing.T) { + testCases := []struct { + name string + processTuples []string + expected []Formation + expectedError string + }{ + { + name: "no process tuples", + processTuples: []string{}, + expected: []Formation{}, + }, + { + name: "single process type", + processTuples: []string{"web=1"}, + expected: []Formation{{ProcessType: "web", Quantity: 1}}, + }, + { + name: "multiple process types", + processTuples: []string{"web=1", "worker=2"}, + expected: []Formation{{ProcessType: "web", Quantity: 1}, {ProcessType: "worker", Quantity: 2}}, + }, + { + name: "trims surrounding whitespace", + processTuples: []string{" web = 2 "}, + expected: []Formation{{ProcessType: "web", Quantity: 2}}, + }, + { + name: "first specified quantity wins", + processTuples: []string{"web=1", "web=3"}, + expected: []Formation{{ProcessType: "web", Quantity: 1}}, + }, + { + // persisted formations are parsed without validation so that a negative + // quantity written by an older dokku version remains readable + name: "negative quantities are parsed", + processTuples: []string{"web=-1"}, + expected: []Formation{{ProcessType: "web", Quantity: -1}}, + }, + { + name: "missing count", + processTuples: []string{"web"}, + expectedError: "Missing count for process type web", + }, + { + name: "non-numeric count", + processTuples: []string{"web=one"}, + expectedError: "Invalid count for process type web", + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + formations, err := parseProcessTuples(testCase.processTuples) + if testCase.expectedError != "" { + if err == nil { + t.Fatalf("expected error %q, got none", testCase.expectedError) + } + + if err.Error() != testCase.expectedError { + t.Fatalf("expected error %q, got %q", testCase.expectedError, err.Error()) + } + + return + } + + if err != nil { + t.Fatalf("unexpected error parsing process tuples: %v", err) + } + + assertFormations(t, formations, testCase.expected) + }) + } +} + +func TestValidateFormations(t *testing.T) { + testCases := []struct { + name string + processTuples []string + expectedError string + }{ + { + name: "zero is a valid quantity", + processTuples: []string{"web=0"}, + }, + { + name: "positive quantities are valid", + processTuples: []string{"web=1", "worker=10"}, + }, + { + name: "negative quantity", + processTuples: []string{"web=-1"}, + expectedError: "Invalid count for process type web: value must be zero or greater", + }, + { + name: "names the offending process type", + processTuples: []string{"web=1", "worker=-2"}, + expectedError: "Invalid count for process type worker: value must be zero or greater", + }, + { + name: "missing process type", + processTuples: []string{"=1"}, + expectedError: "Missing process type for count 1", + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + formations, err := parseProcessTuples(testCase.processTuples) + if err != nil { + t.Fatalf("unexpected error parsing process tuples: %v", err) + } + + err = validateFormations(formations) + if testCase.expectedError == "" { + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + return + } + + if err == nil { + t.Fatalf("expected error %q, got none", testCase.expectedError) + } + + if err.Error() != testCase.expectedError { + t.Fatalf("expected error %q, got %q", testCase.expectedError, err.Error()) + } + }) + } +} + +func TestScaleSetRejectsNegativeQuantity(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "ps-test") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(tmpDir) + + t.Setenv("DOKKU_LIB_ROOT", tmpDir) + t.Setenv("DOKKU_PID", "") + + err = scaleSet(scaleSetInput{ + appName: "test-app", + skipDeploy: true, + processTuples: []string{"web=-1"}, + }) + if err == nil { + t.Fatal("expected an error scaling to a negative quantity, got none") + } + + if err.Error() != "Invalid count for process type web: value must be zero or greater" { + t.Fatalf("unexpected error: %v", err) + } +} diff --git a/tests/unit/app-json-3.bats b/tests/unit/app-json-3.bats index efc8de7f8..0f7b79ef9 100644 --- a/tests/unit/app-json-3.bats +++ b/tests/unit/app-json-3.bats @@ -58,6 +58,23 @@ teardown() { assert_output_contains "cron: 1" 1 } +@test "(app-json) negative formation quantity" { + mkdir -p /var/lib/dokku/data/app-json/$TEST_APP + negative_formation_callback "$TEST_APP" /var/lib/dokku/data/app-json/$TEST_APP + + run_plugn_trigger post-release-builder herokuish $TEST_APP dokku/$TEST_APP:latest + echo "output: $output" + echo "status: $status" + assert_failure + assert_output_contains "Invalid quantity for process type web in app.json: value must be zero or greater" + + run /bin/bash -c "dokku --quiet ps:scale $TEST_APP" + output=$(echo "$output" | tr -s " ") + echo "output: ($output)" + echo "status: $status" + assert_output "web: 1" +} + persist_scale_callback_a() { local APP="$1" local APP_REPO_DIR="$2" @@ -81,3 +98,19 @@ persist_scale_callback_b() { echo "cron: python3 worker.py" >>"$APP_REPO_DIR/Procfile" mv "$APP_REPO_DIR/app-5205b.json" "$APP_REPO_DIR/app.json" } + +negative_formation_callback() { + local APP="$1" + local APP_REPO_DIR="$2" + [[ -z "$APP" ]] && local APP="$TEST_APP" + + cat >"$APP_REPO_DIR/app.json" </var/lib/dokku/config/ps/$TEST_APP/scale assert_output_contains "The --clear and --replace flags cannot be specified together" } +@test "(ps:scale) invalid process counts" { + run /bin/bash -c "dokku ps:scale $TEST_APP web=-1" + echo "output: $output" + echo "status: $status" + assert_failure + assert_output_contains "Invalid count for process type web: value must be zero or greater" + + run /bin/bash -c "dokku ps:scale --replace $TEST_APP web=1 worker=-2" + echo "output: $output" + echo "status: $status" + assert_failure + assert_output_contains "Invalid count for process type worker: value must be zero or greater" + + run /bin/bash -c "dokku ps:scale $TEST_APP =1" + echo "output: $output" + echo "status: $status" + assert_failure + assert_output_contains "Missing process type for count 1" + + run_plugn_trigger ps-set-scale $TEST_APP true false web=-1 + echo "output: $output" + echo "status: $status" + assert_failure + assert_output_contains "Invalid count for process type web: value must be zero or greater" + + run /bin/bash -c "dokku --quiet ps:scale $TEST_APP" + output=$(echo "$output" | tr -s " ") + echo "output: ($output)" + echo "status: $status" + assert_output "web: 1" + + run /bin/bash -c "dokku ps:scale --skip-deploy $TEST_APP web=0" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku --quiet ps:scale $TEST_APP" + output=$(echo "$output" | tr -s " ") + echo "output: ($output)" + echo "status: $status" + assert_output "web: 0" +} + +@test "(ps:scale) repairing a negative process count" { + echo "web=-1 +worker=2" >/var/lib/dokku/config/ps/$TEST_APP/scale + + run /bin/bash -c "dokku --quiet ps:scale $TEST_APP" + output=$(echo "$output" | tr -s " ") + echo "output: ($output)" + echo "status: $status" + assert_success + assert_output $'web: -1\nworker: 2' + + run_plugn_trigger ps-current-scale $TEST_APP + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku ps:scale --skip-deploy $TEST_APP web=1" + echo "output: $output" + echo "status: $status" + assert_success + + run /bin/bash -c "dokku --quiet ps:scale $TEST_APP" + output=$(echo "$output" | tr -s " ") + echo "output: ($output)" + echo "status: $status" + assert_output $'web: 1\nworker: 2' +} + @test "(ps) handle windows newlines in procfile" { run deploy_app python dokku@$DOKKU_DOMAIN:$TEST_APP procfile_line_endings_to_windows echo "output: $output"