You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
feat(serverless): deploy a packaged source from a URL with --from - #174
deploy only takes a local path today, so deploying a published example means a clone, a cd and a command, and anything that wants to offer the deploy has no way to know what the app needs.
--from <url> fetches a zip whose root holds the app's files and a runware.yaml recording the deploy options, then deploys what that file describes:
no new api surface. the manifest's keys map one to one onto flags deploy already accepts, so reading the file and passing the flags by hand produce the same application. a flag passed on the command line wins over the file, so one archive goes to another gpu type or another --id without being edited.
two things worth a look in review:
--from only creates. a manifest names an id, and deploying it over an app that already has that id would roll the archive out as that app's next version, running this code with the secrets, environment variables and volumes already attached. an existing id is refused and points at --id. there is a test that runs the command and asserts no write reaches the api
the archive comes off the network, so from.go treats it as hostile: entries that climb out of the directory, absolute paths, backslash traversal, symlinks, a file count past the cap and a download past the source-upload limit are all refused, each with a test
the manifest also checks the app id against the server's AppId pattern. with --from that id comes out of a file the person deploying did not write, and the api's own answer names the field without naming the rule
verified end to end against production: created an app from the public url with nothing but --from, with the gpu type and entry file coming from the manifest, invoked it and deleted it
make lint, make test and make check-go-fix are clean, and make docs is regenerated
notes for review
copilot reviewed this twice and everything it raised is fixed, with tests, except one thing i would rather you decided on since it is your packer:
on windows the executable bit cannot survive the unpack.--from writes the archive to a temp directory and the existing packer reads it back, deriving each entry's mode from Stat. windows has no execute bit, so a container archive whose dockerfile runs a bundled script directly would deploy with that script at 0644 and fail to start.
fixing it means carrying the zip's own modes through to the repack, which is a new field on packedFile and a new parameter on collectFiles, writeArchive, packDirectory and packContainerDirectory — shared code every deploy goes through, not just --from.
i left it alone because the exposure is narrow: it needs windows, a third-party archive, a container source, and a script the image executes directly. none of the published runware templates can hit it, and i checked rather than assumed — every entry in all fifteen is 0644.
happy to do it in this pr or as a follow-up, whichever you prefer.
Auto reviews are disabled on this repository. To trigger a review, include coderabbit-review in the PR description. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 809157c8-cde7-4176-9392-17043768d326
You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.
Use the checkbox below for a quick retry:
Trigger review
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Adds serverless deploy --from <url> to deploy packaged applications through the existing deployment flow, using a root-level runware.yaml for configuration.
Changes:
Downloads and extracts ZIP sources with path validation and resource limits.
Validates manifest options and merges them with explicit CLI flags taking precedence.
Adds manifest and archive tests, updates argument-validation tests, and documents URL-based deployment.
File
Description
internal/cmd/serverless/pack_test.go
Shares source-file constants across tests.
internal/cmd/serverless/manifest.go
Defines manifest parsing, validation, and flag-merge helpers.
internal/cmd/serverless/manifest_test.go
Tests manifest validation and override precedence.
internal/cmd/serverless/init_test.go
Reuses the shared entry-file constant.
internal/cmd/serverless/from.go
Implements bounded downloads and validated ZIP extraction.
internal/cmd/serverless/from_test.go
Tests fetching, extraction, and hostile archive rejection.
internal/cmd/serverless/deploy.go
Integrates --from and manifest-derived deployment options.
internal/cmd/serverless/deploy_test.go
Tests URL-source argument combinations and conflicts.
docs/runware_serverless_deploy.md
Documents --from, manifest behavior, and examples.
The entry and byte caps do not bound implicit directories: 2,000 empty entries with distinct, deeply nested paths can make MkdirAll create far more than 2,000 directories. A hostile --from archive can therefore exhaust local disk space or inodes and make extraction and cleanup very slow. Count implicit directories toward a cumulative budget, or enforce a path-depth limit, before either MkdirAll call in unpackEntry.
When a manifest defines envVars.LOG_LEVEL: info, deploying with --env-file production.env containing LOG_LEVEL=debug still sends info. This branch injects the manifest values as inline pairs, and buildEnvironmentVariables applies those after the explicitly supplied env files. Apply manifest variables before command-line env files, or suppress them when --env-file is supplied, so an explicit flag can override the manifest as documented.
YAML environment values are unquoted twice
internal/cmd/serverless/manifest.go:207
YAML decoding has already interpreted quoting, but these assignments subsequently pass through buildEnvironmentVariables → splitEnvAssignment → unquote. A YAML value such as PAYLOAD: '"ready"' therefore reaches the application as ready rather than the literal JSON string "ready". Preserve the decoded YAML values when validating and merging environment variables instead of applying shell/dotenv unquoting to them again.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
Archive extraction permits Windows path traversal that can overwrite local files and fails to budget implicit parent directories, risking disk or inode exhaustion.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
The ZIP record cap does not bound implicit parent directories created during extraction, allowing small hostile archives to exhaust disk space or inodes and cause prolonged extraction or cleanup.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Human assessment is needed to evaluate environment-variable loss, duplicate ZIP-entry permission risks, and untested manifest-based updates to existing apps.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Duplicate ZIP destinations can leave executable scripts without execute permission, creating a container startup failure risk that requires human assessment.
The blocking test.TestDeployFrom_CreatesFromTheManifest and TestDeployFrom_RedeploysAnExistingApp both run deploy --from through cmd.Execute() against an httptest API, one creating and one updating. The redeploy case is the rule you named: marking gpu-type as changed in the --from branch makes it fail with --gpu-type applies to create only, so the test holds the rule rather than describing it.
The https downgrade.archiveClient carries a CheckRedirect that refuses a redirect leaving https, and keeps the default limit of 10 so only the downgrade changed. Tested at the rule, because an httptest TLS server signs with a CA the client does not trust.
Two entries on one path.unpackArchive records each regular entry's resolved path and refuses the second one that names it. Directory entries are not counted, since a directory and the files under it are not a collision.
What a redeploy does not apply. An update sends only AppSource, so the command now names the manifest sections it drops (volumes, envVars, secrets, compute, whichever the manifest carried) and points at apps scale and apps env-vars. Silent before.
Still open: the --env-var and --env-file asymmetry, which is a behavior choice rather than a fix, and the follow-up ticket for the Windows executable bit.
9830895 takes the last one too. --env-var is now additive over the manifest rather than instead of it.
buildEnvironmentVariables already read the manifest map before --env-file and before the --env-var pairs, so the precedence was right and the --from branch was dropping the map whenever the flag was passed. It now passes it either way, and one flag overrides one key.
TestDeployFrom_EnvVarOverridesOneManifestKey fixes it: a manifest carrying MODEL_REVISION and CACHE_DIR, deployed with --env-var MODEL_REVISION=abc123 --env-var EXTRA=1, has to create with all three. Restoring the old guard makes it fail with CACHE_DIR missing.
The --from help now states the asymmetry that remains on purpose: --env-var and --env-file set one key over the manifest's, while --volume, --requirement and --secret replace its whole list.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
Critical issues allow case-colliding archive entries to silently overwrite files and lose executable metadata during Windows repacking, potentially preventing deployed applications from starting.
also one behavior change from your RFC comment, since it lands here rather than there: --from only creates now. an id that already exists is refused and points at --id, because deploying over it would roll the archive out as that app's next version and run this code with the secrets, env vars and volumes already attached. so TestDeployFrom_RedeploysAnExistingApp became TestDeployFrom_RefusesAnExistingApp, the unapplied-sections warning is gone with the path that needed it, and the pr description is updated. an update from a url can get its own flag when something asks for one
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
Executable ZIP permissions are lost on Windows, potentially preventing deployed containers from starting, and the refusal test fails to detect uploads performed before refusal.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Human assessment is needed because parent-directory casing conflicts can produce platform-dependent package paths and break imports in Linux containers.
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
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.
danmrichards
left a comment
deployonly takes a local path today, so deploying a published example means a clone, a cd and a command, and anything that wants to offer the deploy has no way to know what the app needs.--from <url>fetches a zip whose root holds the app's files and arunware.yamlrecording the deploy options, then deploys what that file describes:no new api surface. the manifest's keys map one to one onto flags
deployalready accepts, so reading the file and passing the flags by hand produce the same application. a flag passed on the command line wins over the file, so one archive goes to another gpu type or another--idwithout being edited.two things worth a look in review:
--fromonly creates. a manifest names an id, and deploying it over an app that already has that id would roll the archive out as that app's next version, running this code with the secrets, environment variables and volumes already attached. an existing id is refused and points at--id. there is a test that runs the command and asserts no write reaches the apifrom.gotreats it as hostile: entries that climb out of the directory, absolute paths, backslash traversal, symlinks, a file count past the cap and a download past the source-upload limit are all refused, each with a testthe manifest also checks the app id against the server's
AppIdpattern. with--fromthat id comes out of a file the person deploying did not write, and the api's own answer names the field without naming the ruleverified end to end against production: created an app from the public url with nothing but
--from, with the gpu type and entry file coming from the manifest, invoked it and deleted itmake lint,make testandmake check-go-fixare clean, andmake docsis regeneratednotes for review
copilot reviewed this twice and everything it raised is fixed, with tests, except one thing i would rather you decided on since it is your packer:
on windows the executable bit cannot survive the unpack.
--fromwrites the archive to a temp directory and the existing packer reads it back, deriving each entry's mode fromStat. windows has no execute bit, so a container archive whose dockerfile runs a bundled script directly would deploy with that script at 0644 and fail to start.fixing it means carrying the zip's own modes through to the repack, which is a new field on
packedFileand a new parameter oncollectFiles,writeArchive,packDirectoryandpackContainerDirectory— shared code every deploy goes through, not just--from.i left it alone because the exposure is narrow: it needs windows, a third-party archive, a container source, and a script the image executes directly. none of the published runware templates can hit it, and i checked rather than assumed — every entry in all fifteen is 0644.
happy to do it in this pr or as a follow-up, whichever you prefer.