fix(swarm): sanitize healthcheck test serialization and persist swarm settings (#5171, #5223) - #5448
Open
fliptrigga13 wants to merge 1 commit into
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+653
to
+667
| if (test.length > 0) { | ||
| const first = test[0]; | ||
| if (first !== "NONE" && first !== "CMD" && first !== "CMD-SHELL") { | ||
| if (first.startsWith("CMD-SHELL ")) { | ||
| test = ["CMD-SHELL", first.slice(10).trim()]; | ||
| } else if (first.startsWith("CMD ")) { | ||
| test = ["CMD", ...first.slice(4).trim().split(/\s+/)]; | ||
| } else if (test.length === 1) { | ||
| test = ["CMD-SHELL", first]; | ||
| } else { | ||
| test = ["CMD", ...test]; | ||
| } | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
The sanitizer accepts any nonempty array beginning with NONE, CMD, or CMD-SHELL without checking the remaining entries. The form can produce values such as ["CMD-SHELL"] after removing an empty command, or ["NONE", "curl"] when an extra row is present. These malformed tests are then attached to the service specification, so Docker can reject the service update. Validate that NONE is the only element and that command directives contain a valid command.
Comment on lines
+11
to
+129
| export const cleanHealthCheckSwarm = ( | ||
| healthCheck?: HealthCheckSwarm | null, | ||
| ): HealthCheckSwarm | undefined => { | ||
| if (!healthCheck) return undefined; | ||
|
|
||
| let test = healthCheck.Test; | ||
| if (test) { | ||
| test = test | ||
| .map((t) => (typeof t === "string" ? t.trim() : "")) | ||
| .filter(Boolean); | ||
|
|
||
| if (test.length === 1 && test[0].startsWith("[") && test[0].endsWith("]")) { | ||
| try { | ||
| const parsed = JSON.parse(test[0]); | ||
| if (Array.isArray(parsed)) { | ||
| test = parsed | ||
| .map((t) => (typeof t === "string" ? t.trim() : String(t).trim())) | ||
| .filter(Boolean); | ||
| } | ||
| } catch { | ||
| // Not valid JSON array, keep original | ||
| } | ||
| } | ||
|
|
||
| if (test.length > 0) { | ||
| const first = test[0]; | ||
| if (first !== "NONE" && first !== "CMD" && first !== "CMD-SHELL") { | ||
| if (first.startsWith("CMD-SHELL ")) { | ||
| test = ["CMD-SHELL", first.slice(10).trim()]; | ||
| } else if (first.startsWith("CMD ")) { | ||
| test = ["CMD", ...first.slice(4).trim().split(/\s+/)]; | ||
| } else if (test.length === 1) { | ||
| test = ["CMD-SHELL", first]; | ||
| } else { | ||
| test = ["CMD", ...test]; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| const hasValidTest = Boolean(test && test.length > 0); | ||
| const hasOptions = | ||
| healthCheck.Interval !== undefined || | ||
| healthCheck.Timeout !== undefined || | ||
| healthCheck.StartPeriod !== undefined || | ||
| healthCheck.Retries !== undefined; | ||
|
|
||
| if (!hasValidTest && !hasOptions) { | ||
| return undefined; | ||
| } | ||
|
|
||
| return { | ||
| ...(hasValidTest && { Test: test }), | ||
| ...(healthCheck.Interval !== undefined && { | ||
| Interval: Number(healthCheck.Interval), | ||
| }), | ||
| ...(healthCheck.Timeout !== undefined && { | ||
| Timeout: Number(healthCheck.Timeout), | ||
| }), | ||
| ...(healthCheck.StartPeriod !== undefined && { | ||
| StartPeriod: Number(healthCheck.StartPeriod), | ||
| }), | ||
| ...(healthCheck.Retries !== undefined && { | ||
| Retries: Number(healthCheck.Retries), | ||
| }), | ||
| }; | ||
| }; | ||
|
|
||
| export const generateConfigContainerMock = (application: { | ||
| healthCheckSwarm?: HealthCheckSwarm | null; | ||
| updateConfigSwarm?: any; | ||
| stopGracePeriodSwarm?: number | null; | ||
| }) => { | ||
| const cleanedHealthCheck = cleanHealthCheckSwarm( | ||
| application.healthCheckSwarm, | ||
| ); | ||
| return { | ||
| ...(cleanedHealthCheck && { | ||
| HealthCheck: cleanedHealthCheck, | ||
| }), | ||
| ...(application.updateConfigSwarm | ||
| ? { UpdateConfig: application.updateConfigSwarm } | ||
| : { | ||
| UpdateConfig: { | ||
| Parallelism: 1, | ||
| Order: "start-first", | ||
| FailureAction: "rollback", | ||
| }, | ||
| }), | ||
| ...(application.stopGracePeriodSwarm !== null && | ||
| application.stopGracePeriodSwarm !== undefined && { | ||
| StopGracePeriod: application.stopGracePeriodSwarm, | ||
| }), | ||
| }; | ||
| }; | ||
|
|
||
| export const normalizeStopGracePeriod = (val: unknown): number | null => { | ||
| if (val === null || val === undefined || val === "") return null; | ||
| const num = Number(val); | ||
| return Number.isNaN(num) ? null : num; | ||
| }; | ||
|
|
||
| export const validateUpdateConfigPartial = (config: Record<string, any>) => { | ||
| const result: Record<string, any> = {}; | ||
| if (config.Parallelism !== undefined) | ||
| result.Parallelism = Number(config.Parallelism); | ||
| if (config.Delay !== undefined) result.Delay = Number(config.Delay); | ||
| if (config.FailureAction !== undefined) | ||
| result.FailureAction = String(config.FailureAction); | ||
| if (config.Monitor !== undefined) result.Monitor = Number(config.Monitor); | ||
| if (config.MaxFailureRatio !== undefined) | ||
| result.MaxFailureRatio = Number(config.MaxFailureRatio); | ||
| if (config.Order !== undefined) result.Order = String(config.Order); | ||
| return result; | ||
| }; | ||
|
|
||
| export const computeForceUpdate = (existingForceUpdate?: number): number => { | ||
| return (existingForceUpdate ?? 0) + 1; | ||
| }; |
Contributor
There was a problem hiding this comment.
This suite reimplements the health-check sanitizer, container-config generator, schema parsing, and ForceUpdate calculation inside the test file instead of importing the production implementations. As a result, these tests can remain green if the real outage fixes regress or diverge. Import and exercise the production utilities and schemas so the suite provides meaningful regression coverage.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Fixes #5171 and #5223.
Resolves critical production outage and configuration bugs in Docker Swarm settings for Applications and Databases:
["CMD", "curl", "-f", "..."]) or single command strings (e.g.curl -f http://localhost:3000/health) resulted in malformed serialization (Test: ["[\"CMD\", ...]"]or single commands withoutCMD-SHELL). Docker Engine rejected this withUnknown healthcheck type '' (expected 'CMD'), leaving tasks stuck inStartingindefinitely and causing VIP/DNS resolution failures.Test: [""]or{ Test: [] }by introducingcleanHealthCheckSwarm(healthCheckSwarm):["CMD-SHELL", command].CMD,CMD-SHELL, andNONEdirectives.UpdateConfigSwarmSchema,ParallelismandOrderwere strictly required (z.number()andz.string()), causing partial updates (such as updating onlyMonitor,Delay, orOrder) fromUpdateConfigFormandRollbackConfigFormto fail Zod validation with 400 Bad Request, reverting settings on refresh.stopGracePeriodSwarm, input values from HTML number inputs were strings, failingz.number().nullable()validation in backend router schemas. Converted toz.coerce.number().nullable()acrossapplicationand database schemas (postgres,mysql,redis,mongo,mariadb,libsql) and normalized inStopGracePeriodForm.ShowClusterSettings,useEffectpreviously checkedif (data?.command)to reset the form forreplicasandregistryId. Services without custom run commands failed to populatereplicasandregistryIdproperly. Changed toif (data)and updated toast messages.mechanizeDockerContainerand database service updates (postgres,redis,mysql,mongo,mariadb,traefik-setup,forward-auth-setup,rollbacks), changedForceUpdate: inspect.Spec.TaskTemplate.ForceUpdate + 1to(inspect.Spec.TaskTemplate.ForceUpdate ?? 0) + 1to preventNaNwhenForceUpdatewas initially unset by Docker.Verification & Testing
apps/dokploy/__test__/cluster/swarm-settings-healthcheck.test.tswith 15 test cases covering:Test: [""]discarding.CMD-SHELLwrapping.CMD-SHELL,CMD, andNONE.generateConfigContainerbehavior for valid and invalid healthchecks.UpdateConfigSwarmSchemaparsing.StopGracePeriodSchemanumber coercion and null/undefined handling.ForceUpdatecalculation preventingNaN.npx @biomejs/biome check).The PR is not yet safe to merge because malformed directive-only or overpopulated health-check arrays can still reach Docker and defeat the outage fix.
Summary
Reviews (1) · Last reviewed commit: "fix(swarm): sanitize healthcheck test se..."