feat(backup): s3 storage class support#3842
feat(backup): s3 storage class support#3842Tylerjames1504 wants to merge 18 commits intoDokploy:canaryfrom
Conversation
dosubot
bot
added
the
size:XXL
|
Related Documentation Checked 7 published document(s) in 1 knowledge base(s). No updates required. How did I do? Any feedback? |
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
|
@greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } catch (error) { | ||
| throw new TRPCError({ | ||
| code: "BAD_REQUEST", | ||
| message: | ||
| error instanceof Error | ||
| ? error.message | ||
| : "Failed to request archive restore.", | ||
| cause: error, | ||
| }); | ||
| } |
There was a problem hiding this comment.
TRPCErrors are caught and re-wrapped, losing original error codes
The outer catch block catches all errors, including the intentionally-thrown TRPCError instances inside the same try block (the guards for directory paths, empty paths, and glob patterns). It also catches errors from findDestinationById, which may throw a NOT_FOUND TRPCError. All of these are uniformly re-wrapped as BAD_REQUEST, discarding the original TRPC code.
The BAD_REQUEST validation guards at the top of the try block are already correctly-typed TRPCErrors and do not need to be caught — they should propagate as-is. A simpler fix is to add a TRPCError re-throw before the generic catch:
} catch (error) {
if (error instanceof TRPCError) {
throw error;
}
throw new TRPCError({
code: "INTERNAL_SERVER_ERROR",
message: "Failed to request archive restore.",
cause: error,
});
}
This ensures validation errors surface with the right code and that findDestinationById's NOT_FOUND is not silently converted to BAD_REQUEST.
| const validateStorageClassForDestination = async ({ | ||
| destinationId, | ||
| storageClass, | ||
| }: { | ||
| destinationId: string; | ||
| storageClass?: string | null; | ||
| }) => { | ||
| const normalizedStorageClass = normalizeS3StorageClass(storageClass); | ||
| if (!normalizedStorageClass) { | ||
| return; | ||
| } | ||
|
|
||
| const destination = await findDestinationById(destinationId); | ||
| const provider = destination.provider; | ||
| const supportedStorageClasses = getS3StorageClassesForProvider(provider); | ||
|
|
||
| if (supportedStorageClasses.length === 0) { | ||
| throw new TRPCError({ | ||
| code: "BAD_REQUEST", | ||
| message: `Storage class is not supported for provider "${provider || "Unknown"}".`, | ||
| }); | ||
| } | ||
|
|
||
| if (!supportedStorageClasses.includes(normalizedStorageClass)) { | ||
| throw new TRPCError({ | ||
| code: "BAD_REQUEST", | ||
| message: `Invalid storage class for provider "${provider}". Allowed values: ${supportedStorageClasses.join(", ")}.`, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Duplicated validateStorageClassForDestination function
This function is defined identically in both apps/dokploy/server/api/routers/backup.ts (lines 103–131) and apps/dokploy/server/api/routers/volume-backups.ts (lines 34–65). Any future change (e.g., adding support for a new provider) would need to be applied in two places.
Consider extracting it to packages/server/src/utils/backups/s3-storage-class.ts, which already exports normalizeS3StorageClass and getS3StorageClassesForProvider, and importing it in both router files.
This pattern also appears at apps/dokploy/server/api/routers/volume-backups.ts:34.
| @@ -1,11 +1,14 @@ | |||
| import { standardSchemaResolver as zodResolver } from "@hookform/resolvers/standard-schema"; | |||
| import { zodResolver } from "@hookform/resolvers/zod"; | |||
There was a problem hiding this comment.
Inconsistent resolver import vs the rest of the codebase
This file switches from standardSchemaResolver as zodResolver (the pattern used everywhere else in the project, including restore-volume-backups.tsx) to zodResolver imported directly from @hookform/resolvers/zod. At the same time, package.json adds "@hookform/resolvers>zod": "^4.3.6" as a pnpm override, which forces the zod peer-dependency of @hookform/resolvers to v4.
If the rest of the project still uses Zod v3, this mismatch means @hookform/resolvers/zod will reference Zod v4 types while the actual schema objects passed at runtime are Zod v3 instances. This can cause TypeScript type errors and subtle runtime failures (e.g. if the resolver does version-specific duck-typing).
Please clarify:
- Has the entire project migrated to Zod v4, or only this one file?
- If only this file, why was
standardSchemaResolvernot sufficient here as it is inrestore-volume-backups.tsx?
Keeping both resolver strategies in the same codebase without explanation is a maintenance hazard.
|
@greptile |
package.json
Outdated
| @@ -58,13 +60,12 @@ | |||
| "esbuild", | |||
| "msgpackr-extract", | |||
| "node-pty", | |||
| "prisma", | |||
There was a problem hiding this comment.
Unrelated package.json changes
This diff adds @prisma/client, @prisma/engines, and prisma to onlyBuiltDependencies and reorganises the array. These changes appear unrelated to S3 storage class support. If they originate from a rebase or merge from another branch, it would be worth confirming they are intentional before merging to avoid accidentally introducing unexpected dependency-build configuration changes.
Additional Comments (1)
// constants.ts
if (!provider) return []; // new [] every call
return S3_PROVIDER_STORAGE_CLASS_OPTIONS[provider] ?? []; // also new [] for unknown providersBecause Memoising the derived value removes the instability: const storageClassOptions = useMemo(
() => getS3StorageClassOptionsByProvider(selectedProvider),
[selectedProvider],
);The same issue is present in |
…1504/dokploy into feat/s3-storage-class-support
|
@greptile |
| const rcloneFlags = getS3Credentials( | ||
| destination, | ||
| volumeBackup.storageClass, | ||
| ); | ||
| const normalizedPrefix = normalizeS3Path(prefix); | ||
| const backupFilesPath = `:s3:${destination.bucket}/${normalizedPrefix}`; | ||
| const listCommand = `rclone lsf ${rcloneFlags.join(" ")} --include \"${volumeName}-*.tar\" :s3:${destination.bucket}/${normalizedPrefix}`; |
There was a problem hiding this comment.
--s3-storage-class write flag incorrectly passed to read commands
getS3Credentials(destination, volumeBackup.storageClass) appends --s3-storage-class="…" to rcloneFlags, which are then used for both the rclone lsf list command (line 89) and the rclone delete command (line 91). The --s3-storage-class flag is a write-only config: it tells rclone which storage class to assign when creating new objects. Passing it to a listing (lsf) command is a no-op at best; at worst, stricter S3-compatible providers may reject the request.
The list/delete commands should use credentials without the storage-class override:
const rcloneFlags = getS3Credentials(destination); // no storageClass for reads
const rcloneWriteFlags = getS3Credentials(destination, volumeBackup.storageClass); // only used for uploads| export const validateS3StorageClassForDestination = async ({ | ||
| destinationId, | ||
| storageClass, | ||
| }: { | ||
| destinationId: string; | ||
| storageClass?: string | null; | ||
| }) => { | ||
| const normalizedStorageClass = normalizeS3StorageClass(storageClass); | ||
| if (!normalizedStorageClass) { | ||
| return; | ||
| } | ||
|
|
||
| const destination = await findDestinationById(destinationId); | ||
| const provider = destination.provider; | ||
| const supportedStorageClasses = getS3StorageClassesForProvider(provider); | ||
|
|
||
| if (supportedStorageClasses.length === 0) { | ||
| throw new TRPCError({ | ||
| code: "BAD_REQUEST", | ||
| message: `Storage class is not supported for provider "${provider || "Unknown"}".`, | ||
| }); | ||
| } | ||
|
|
||
| if (!supportedStorageClasses.includes(normalizedStorageClass)) { | ||
| throw new TRPCError({ | ||
| code: "BAD_REQUEST", | ||
| message: `Invalid storage class for provider "${provider}". Allowed values: ${supportedStorageClasses.join(", ")}.`, | ||
| }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Validated (normalised) storage class not returned to callers
validateS3StorageClassForDestination internally normalises the input with normalizeS3StorageClass (trim + toUpperCase) before running the allow-list check, but returns void. The callers in both routers then pass the original, un-normalised value directly to the database:
// backup.ts (create)
await validateS3StorageClassForDestination({ storageClass: input.storageClass });
const newBackup = await createBackup(input); // stores raw input.storageClass, e.g. "glacier"If a consumer calls the API directly (e.g. via curl) with a lowercase value like "glacier", it passes validation (normalised → "GLACIER" is valid) but "glacier" is persisted. When the UI later loads the backup, field.value === "glacier" won't match any <SelectItem value="GLACIER">, so the dropdown will appear blank even though a class is actually configured.
Consider returning the normalised value so the router can use it for storage:
export const validateS3StorageClassForDestination = async (...): Promise<string | undefined> => {
const normalizedStorageClass = normalizeS3StorageClass(storageClass);
if (!normalizedStorageClass) return undefined;
// ... validation ...
return normalizedStorageClass;
};1 participant
What is this PR about?
This PR adds S3 storage class support for backups and volume backups across the stack. It introduces a new storageClass field in the backup schemas and UI, validates provider-specific storage class values on create/update, and passes the selected class to rclone so objects are written with the expected tier. It also improves restore UX by surfacing storage class/restore availability and adding an archive restore request flow for cold-storage objects. Additionally, it includes migration updates for the new columns and backup-related tests.
Checklist
Before submitting this PR, please make sure that:`
canarybranch.Related Issue:
closes #3840
Screenshots (if applicable)
Configure backup storage class
Choosing S3 storage class when setting up backup:
Restore file selection and status
Selecting a backup to restore, including readiness/archive/readable-until status:
After selecting a backup that is ready to restore:
Archive restore flow
After selecting a backup that must be restored from AWS archive:
Selecting retrieval tier:
Selecting readable duration:
Modal after clicking Request restore from AWS:
Object restored from AWS and now ready to use for DB restore:
Greptile Summary
This PR adds S3 storage class support to both database and volume backups by introducing a
storageClassfield in the schemas and DB, a news3-storage-class.tsutility module for normalisation and validation, storage-class-aware rclone flag generation, and enriched restore UX with archive status badges and a Glacier restore-request flow.Key observations:
validateStorageClassForDestinationfunction flagged in a previous review has been correctly consolidated intopackages/server/src/utils/backups/s3-storage-class.tsand shared across both routers.TRPCErrorre-throw guard inrequestBackupFileRestorecorrectly preserves original error codes, addressing the prior review thread.--s3-storage-class(a write-only rclone flag) is passed viagetS3CredentialsintocleanupOldVolumeBackups, where it is appended to both therclone lsf(list) andrclone deletecommands. While rclone silently ignores write flags on reads in most cases, this is semantically incorrect and could cause issues with stricter S3-compatible providers.validateS3StorageClassForDestinationnormalises (trim + toUpperCase) the input for validation but returnsvoid, so the original un-normalised value is persisted to the DB. If a consumer calls the API directly with lowercase (e.g."glacier"), the stored value won't match the uppercase<SelectItem>keys and the dropdown will render blank when the form is loaded for editing.Confidence Score: 3/5
--s3-storage-classis incorrectly forwarded torclone lsfandrclone deleteduring volume backup cleanup — while rclone likely ignores it today, it's semantically wrong and could break on some S3-compatible endpoints; (2) storage class values may be persisted in non-normalised (lowercase) form, which would silently break the editing dropdown in the UI for API-created backups.packages/server/src/utils/volume-backups/utils.ts(write flag on read commands) andpackages/server/src/utils/backups/s3-storage-class.ts(normalised value not returned to callers for DB persistence).Last reviewed commit: ada2704