From 2f05efa02a13272d9712d6eef994d770735a8122 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A9lix=20Sanz?= Date: Wed, 4 Nov 2026 16:01:47 +0100 Subject: [PATCH 01/13] feat(serverless): deploy a packaged source from a URL with --from --- docs/runware_serverless_deploy.md | 21 ++ internal/cmd/serverless/deploy.go | 121 +++++++-- internal/cmd/serverless/deploy_test.go | 39 ++- internal/cmd/serverless/from.go | 197 +++++++++++++++ internal/cmd/serverless/from_test.go | 275 +++++++++++++++++++++ internal/cmd/serverless/init_test.go | 2 +- internal/cmd/serverless/manifest.go | 253 +++++++++++++++++++ internal/cmd/serverless/manifest_test.go | 300 +++++++++++++++++++++++ internal/cmd/serverless/pack_test.go | 14 +- 9 files changed, 1194 insertions(+), 28 deletions(-) create mode 100644 internal/cmd/serverless/from.go create mode 100644 internal/cmd/serverless/from_test.go create mode 100644 internal/cmd/serverless/manifest.go create mode 100644 internal/cmd/serverless/manifest_test.go diff --git a/docs/runware_serverless_deploy.md b/docs/runware_serverless_deploy.md index 4344063..0d04244 100644 --- a/docs/runware_serverless_deploy.md +++ b/docs/runware_serverless_deploy.md @@ -42,6 +42,19 @@ minWorkers=0 stays scaled to zero until the first invoke. --container cannot be combined with an entry file, --src-dir, --base-image, or --requirement. +--from takes the URL of a packaged source: a zip whose root holds the app's +files and a runware.yaml recording the deploy options. The CLI fetches it, +reads that file, and deploys what it describes, so a published example deploys +in one command instead of a clone and a command. Flags still win over the +file, so the same archive goes to another GPU type or another --id without +being edited first. The archive supplies the source directory, so --from +cannot be combined with an entry file, --container, or --src-dir. + +A runware.yaml carries no secret values. Its secrets name what the app needs, +and each one has to already exist in the environment. A manifest key this CLI +does not know is an error rather than a silent omission, which is what +configVersion is there to tell apart. + Exclude what the app does not need with a .runwareignore file at the root of the source directory; it takes gitignore syntax. A .gitignore is NOT consulted -- what a project keeps out of version control is a different question from what it @@ -124,6 +137,13 @@ runware serverless deploy [file] [flags] # wait until the first rollout is active or failed runware serverless deploy ./app.py --id my-app --gpu-type h100 --wait --timeout 10m + + # deploy a packaged source, reading its id and options from its runware.yaml + runware serverless deploy --from https://runware.ai/docs/serverless/examples/echo.zip + + # the same archive under another id and on another GPU type + runware serverless deploy --from https://runware.ai/docs/serverless/examples/echo.zip \ + --id my-echo --gpu-type l40s ``` ### Options @@ -136,6 +156,7 @@ runware serverless deploy [file] [flags] --env-file stringArray File of KEY=VALUE lines to read environment variables from (repeatable) --env-var stringArray Environment variable as KEY=VALUE (repeatable) --fallback-gpu-type string Secondary GPU type if the preferred type is unavailable + --from string URL of a packaged source to fetch and deploy, reading its runware.yaml --gpu-type string GPU type ID (see 'serverless gpus'; required when creating) --gpus-per-worker int32 GPUs allocated per worker (1, 2, 4, or 8) (default 1) -h, --help help for deploy diff --git a/internal/cmd/serverless/deploy.go b/internal/cmd/serverless/deploy.go index 10ed923..fe9af08 100644 --- a/internal/cmd/serverless/deploy.go +++ b/internal/cmd/serverless/deploy.go @@ -15,7 +15,7 @@ import ( "github.com/spf13/cobra" ) -const deploySourceChoice = "pass an entry file or --container" +const deploySourceChoice = "pass an entry file, --container, or --from" var codeOnlyDeployFlags = []string{ "src-dir", @@ -23,6 +23,12 @@ var codeOnlyDeployFlags = []string{ "requirement", } +// fromOnlyConflictFlags name a local source, which --from already supplies. +var fromOnlyConflictFlags = []string{ + "src-dir", + "container", +} + // createOnlyDeployFlags apply to CreateApp only. On an existing app they are // rejected so a leftover create invocation cannot wipe environment variables // or silently no-op. @@ -72,14 +78,31 @@ func (s deploySource) appSource(sourceID uuid.UUID) (serverlessapi.AppSourceUpse } } -func validateDeployArgs(cmd *cobra.Command, args []string, containerDir string) error { +func validateDeployArgs(cmd *cobra.Command, args []string, containerDir, from string) error { hasFile := len(args) == 1 hasContainer := containerDir != "" - if hasFile == hasContainer { - if hasFile { - return fmt.Errorf("%s, not both", deploySourceChoice) + hasFrom := from != "" + + chosen := 0 + for _, picked := range []bool{hasFile, hasContainer, hasFrom} { + if picked { + chosen++ + } + } + if chosen != 1 { + if chosen == 0 { + return fmt.Errorf("%s", deploySourceChoice) + } + return fmt.Errorf("%s, not more than one", deploySourceChoice) + } + + if hasFrom { + for _, name := range fromOnlyConflictFlags { + if cmd.Flags().Changed(name) { + return fmt.Errorf("--%s names a local source; --from takes the source from the archive", name) + } } - return fmt.Errorf("%s", deploySourceChoice) + return nil } if !hasContainer { return nil @@ -132,6 +155,7 @@ func newDeployCmd(logger *log.Logger) *cobra.Command { availableWorkersPct int32 srcDir string containerDir string + from string volumes []string secrets []string envVars []string @@ -182,6 +206,19 @@ minWorkers=0 stays scaled to zero until the first invoke. --container cannot be combined with an entry file, --src-dir, --base-image, or --requirement. +--from takes the URL of a packaged source: a zip whose root holds the app's +files and a runware.yaml recording the deploy options. The CLI fetches it, +reads that file, and deploys what it describes, so a published example deploys +in one command instead of a clone and a command. Flags still win over the +file, so the same archive goes to another GPU type or another --id without +being edited first. The archive supplies the source directory, so --from +cannot be combined with an entry file, --container, or --src-dir. + +A runware.yaml carries no secret values. Its secrets name what the app needs, +and each one has to already exist in the environment. A manifest key this CLI +does not know is an error rather than a silent omission, which is what +configVersion is there to tell apart. + Exclude what the app does not need with a .runwareignore file at the root of the source directory; it takes gitignore syntax. A .gitignore is NOT consulted -- what a project keeps out of version control is a different question from what it @@ -255,15 +292,68 @@ paths and an invoke example once the application is active.`, runware serverless deploy --id my-app --gpu-type h100 --container ./wrapper # wait until the first rollout is active or failed - runware serverless deploy ./app.py --id my-app --gpu-type h100 --wait --timeout 10m`, + runware serverless deploy ./app.py --id my-app --gpu-type h100 --wait --timeout 10m + + # deploy a packaged source, reading its id and options from its runware.yaml + runware serverless deploy --from https://runware.ai/docs/serverless/examples/echo.zip + + # the same archive under another id and on another GPU type + runware serverless deploy --from https://runware.ai/docs/serverless/examples/echo.zip \ + --id my-echo --gpu-type l40s`, Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { if err := validateWaitFlags(cmd, wait, timeout); err != nil { return err } - if err := validateDeployArgs(cmd, args, containerDir); err != nil { + if err := validateDeployArgs(cmd, args, containerDir, from); err != nil { return err } + + var manifest deployManifest + if from != "" { + dir, cleanup, err := fetchSourceArchive(cmd.Context(), from) + if err != nil { + return err + } + defer cleanup() + + if manifest, err = readDeployManifest(dir); err != nil { + return err + } + + // The manifest's own values, for every flag the command line left + // alone. A passed flag wins, which is what lets one packaged source + // deploy on another GPU type without being edited first. + // + // Nothing here marks a flag as changed. The create-only flags are + // refused on an update by exactly that test, and a manifest that + // carries a GPU type is describing the app, not asking to change a + // running one, so a redeploy from the same archive has to go through. + args, srcDir, containerDir = manifest.localSource(dir) + id = manifestString(cmd, "id", id, manifest.ID) + name = manifestString(cmd, "name", name, manifest.Name) + baseImage = manifestString(cmd, "base-image", baseImage, manifest.Build.BaseImage) + gpuType = manifestString(cmd, "gpu-type", gpuType, manifest.Compute.GpuType) + maxWorkers = manifestInt32(cmd, "max-workers", maxWorkers, manifest.Compute.MaxWorkers) + idleTTL = manifestInt32(cmd, "idle-ttl", idleTTL, manifest.Compute.IdleTTL) + scalingDelay = manifestInt32(cmd, "scaling-delay", scalingDelay, manifest.Compute.ScalingDelay) + if !cmd.Flags().Changed("requirement") { + requirements = manifest.Build.Requirements + } + if !cmd.Flags().Changed("volume") { + volumes = manifest.Volumes + } + if !cmd.Flags().Changed(envVarFlag) { + envVars = manifest.envVarFlags() + } + if !cmd.Flags().Changed("secret") { + secrets = manifest.secretFlags() + } + } + + if id == "" { + return fmt.Errorf("--id is required") + } if err := validateGPUsPerWorkerFlag(cmd, gpusPerWorker); err != nil { return err } @@ -361,11 +451,11 @@ paths and an invoke example once the application is active.`, IdleTtlSecs: idleTTL, ScalingDelaySecs: scalingDelay, GpuType: gpuType, - MinWorkers: optionalInt32Ptr(cmd, "min-workers", minWorkers), - GpusPerWorker: optionalInt32Ptr(cmd, "gpus-per-worker", gpusPerWorker), - FallbackGpuType: optionalFlagStringPtr(cmd, "fallback-gpu-type", fallbackGPUType), - MinAvailableWorkers: optionalInt32Ptr(cmd, "min-available-workers", minAvailableWorkers), - AvailableWorkersPct: optionalInt32Ptr(cmd, "available-workers-pct", availableWorkersPct), + MinWorkers: manifestInt32Ptr(cmd, "min-workers", minWorkers, manifest.Compute.MinWorkers), + GpusPerWorker: manifestInt32Ptr(cmd, "gpus-per-worker", gpusPerWorker, manifest.Compute.GpusPerWorker), + FallbackGpuType: manifestStringPtr(cmd, "fallback-gpu-type", fallbackGPUType, manifest.Compute.FallbackGpuType), + MinAvailableWorkers: manifestInt32Ptr(cmd, "min-available-workers", minAvailableWorkers, manifest.Compute.MinAvailableWorkers), + AvailableWorkersPct: manifestInt32Ptr(cmd, "available-workers-pct", availableWorkersPct, manifest.Compute.AvailableWorkersPct), }, }) } @@ -427,6 +517,7 @@ paths and an invoke example once the application is active.`, cmd.Flags().StringVar(&srcDir, "src-dir", "", "Directory to package as the application source (default: the working directory; code deploys only)") cmd.Flags().StringVar(&containerDir, "container", "", "Directory whose root contains Dockerfile and container.yaml") + cmd.Flags().StringVar(&from, "from", "", "URL of a packaged source to fetch and deploy, reading its "+manifestFile) cmd.Flags().StringArrayVar(&volumes, "volume", nil, "Absolute path inside the app backed by persistent node-local storage; immutable after create (repeatable)") cmd.Flags().StringArrayVar(&secrets, "secret", nil, "Secret of the same environment to attach at create, as NAME or NAME=ENV_VAR (repeatable)") cmd.Flags().StringArrayVar(&envVars, envVarFlag, nil, "Environment variable as KEY=VALUE (repeatable)") @@ -449,10 +540,6 @@ paths and an invoke example once the application is active.`, cmd.Flags().DurationVar(&pollInterval, "poll-interval", 2*time.Second, "Polling interval when waiting for the application") addEnvironmentFlag(cmd, cmd.Flags(), logger, environmentFlagHelp) - if err := cmd.MarkFlagRequired("id"); err != nil { - panic(err) - } - return cmd } diff --git a/internal/cmd/serverless/deploy_test.go b/internal/cmd/serverless/deploy_test.go index abaf78a..413dd71 100644 --- a/internal/cmd/serverless/deploy_test.go +++ b/internal/cmd/serverless/deploy_test.go @@ -24,6 +24,9 @@ const ( testPipPackage = "torch" testSourceID = "019c7654-8b21-7abc-9123-abcdef123456" testSrcDirFlag = "--src-dir" + testFromFlag = "--from" + testFallbackGPU = "l40s" + testArchiveURL = "https://runware.ai/docs/serverless/examples/echo.zip" testIDFlag = "--id" testWaitFlag = "--wait" testTimeoutFlag = "--timeout" @@ -49,10 +52,34 @@ func TestValidateDeployArgs(t *testing.T) { wantErr: deploySourceChoice, }, { - name: "both", + name: "a file and a container", args: []string{testModelFile}, flags: []string{testContainerFlag, testWrapperDir}, - wantErr: "not both", + wantErr: "not more than one", + }, + { + name: "from only", + flags: []string{testFromFlag, testArchiveURL}, + }, + { + name: "from and a file", + args: []string{testModelFile}, + flags: []string{testFromFlag, testArchiveURL}, + wantErr: "not more than one", + }, + { + name: "from and a container", + flags: []string{testFromFlag, testArchiveURL, testContainerFlag, testWrapperDir}, + wantErr: "not more than one", + }, + { + name: "from with src-dir", + flags: []string{testFromFlag, testArchiveURL, testSrcDirFlag, "."}, + wantErr: testSrcDirFlag, + }, + { + name: "from with a flag that overrides the manifest", + flags: []string{testFromFlag, testArchiveURL, "--gpu-type", testFallbackGPU}, }, { name: "container with src-dir", @@ -90,7 +117,11 @@ func TestValidateDeployArgs(t *testing.T) { if err != nil { t.Fatalf("container: %v", err) } - err = validateDeployArgs(cmd, tc.args, containerDir) + from, err := cmd.Flags().GetString("from") + if err != nil { + t.Fatalf("from: %v", err) + } + err = validateDeployArgs(cmd, tc.args, containerDir, from) if tc.wantErr == "" { if err != nil { t.Fatalf("unexpected error: %v", err) @@ -447,7 +478,7 @@ func TestValidateUpdateDeployFlags(t *testing.T) { {flags: []string{"--env-file", envDotfile}, wantErr: "apps env-vars"}, {flags: []string{"--volume", "/data"}, wantErr: "immutable"}, {flags: []string{"--secret", "ORG_TOKEN"}, wantErr: "secrets attach"}, - {flags: []string{"--fallback-gpu-type", "l40s"}, wantErr: scaleHint}, + {flags: []string{"--fallback-gpu-type", testFallbackGPU}, wantErr: scaleHint}, {flags: []string{"--min-available-workers", "1"}, wantErr: scaleHint}, {flags: []string{"--available-workers-pct", "10"}, wantErr: scaleHint}, {flags: []string{"--name", "My App"}, wantErr: "apps rename"}, diff --git a/internal/cmd/serverless/from.go b/internal/cmd/serverless/from.go new file mode 100644 index 0000000..d35aed6 --- /dev/null +++ b/internal/cmd/serverless/from.go @@ -0,0 +1,197 @@ +package serverless + +import ( + "archive/zip" + "bytes" + "context" + "fmt" + "io" + "net/http" + "net/url" + "os" + "path/filepath" + "strings" + "time" +) + +// fromFetchTimeout bounds the download of a packaged source. +const fromFetchTimeout = 60 * time.Second + +// maxArchiveEntries bounds the file count in a fetched archive. The byte caps +// alone do not: a few hundred thousand empty entries cost almost nothing to +// send and a directory each to create. +const maxArchiveEntries = 2000 + +// fetchSourceArchive downloads a packaged source and unpacks it into a new +// directory, returning that directory and a function that removes it. +// +// What comes back is read as code and uploaded to a builder, so the archive is +// treated as hostile: the caps bound what it can spend, and every entry is +// checked to land inside the directory as a regular file. +func fetchSourceArchive(ctx context.Context, rawURL string) (dir string, cleanup func(), err error) { + archive, err := downloadArchive(ctx, rawURL) + if err != nil { + return "", nil, err + } + + dir, err = os.MkdirTemp("", "runware-from-*") + if err != nil { + return "", nil, fmt.Errorf("create a directory for the fetched source: %w", err) + } + cleanup = func() { + _ = os.RemoveAll(dir) + } + + if err := unpackArchive(archive, dir); err != nil { + cleanup() + return "", nil, err + } + return dir, cleanup, nil +} + +// downloadArchive reads the URL into memory, refusing anything larger than the +// platform would accept as a source upload anyway. +func downloadArchive(ctx context.Context, rawURL string) ([]byte, error) { + parsed, err := url.Parse(rawURL) + if err != nil { + return nil, fmt.Errorf("--from %q is not a URL: %w", rawURL, err) + } + if parsed.Scheme != "http" && parsed.Scheme != "https" { + return nil, fmt.Errorf("--from takes an http or https URL, not %q", rawURL) + } + + ctx, cancel := context.WithTimeout(ctx, fromFetchTimeout) + defer cancel() + + req, err := http.NewRequestWithContext(ctx, http.MethodGet, rawURL, nil) + if err != nil { + return nil, fmt.Errorf("fetch %s: %w", rawURL, err) + } + resp, err := http.DefaultClient.Do(req) + if err != nil { + return nil, fmt.Errorf("fetch %s: %w", rawURL, err) + } + defer func() { + _, _ = io.Copy(io.Discard, resp.Body) + _ = resp.Body.Close() + }() + + if resp.StatusCode != http.StatusOK { + return nil, fmt.Errorf("fetch %s: the source answered %d", rawURL, resp.StatusCode) + } + + archive, err := io.ReadAll(io.LimitReader(resp.Body, maxArchiveBytes+1)) + if err != nil { + return nil, fmt.Errorf("fetch %s: %w", rawURL, err) + } + if int64(len(archive)) > maxArchiveBytes { + return nil, fmt.Errorf( + "fetch %s: the archive is larger than %s, which is the most a source upload accepts", + rawURL, humanBytes(maxArchiveBytes), + ) + } + return archive, nil +} + +// unpackArchive writes the archive's files under dir. +func unpackArchive(archive []byte, dir string) error { + reader, err := zip.NewReader(bytes.NewReader(archive), int64(len(archive))) + if err != nil { + return fmt.Errorf("read the fetched archive: %w", err) + } + if len(reader.File) > maxArchiveEntries { + return fmt.Errorf("the fetched archive holds %d files, more than the %d a source may have", len(reader.File), maxArchiveEntries) + } + + var total int64 + for _, entry := range reader.File { + written, err := unpackEntry(entry, dir) + if err != nil { + return err + } + total += written + if total > maxPackTotalBytes { + return fmt.Errorf("the fetched archive unpacks to more than %s", humanBytes(maxPackTotalBytes)) + } + } + return nil +} + +// unpackEntry writes one archive entry and reports the bytes it wrote. +func unpackEntry(entry *zip.File, dir string) (int64, error) { + rel, err := safeArchivePath(entry.Name) + if err != nil { + return 0, err + } + path := filepath.Join(dir, rel) + + if entry.FileInfo().IsDir() { + if err := os.MkdirAll(path, 0o750); err != nil { + return 0, fmt.Errorf("create %s: %w", entry.Name, err) + } + return 0, nil + } + + // A symlink in the archive would be followed by the packer that reads this + // directory back, and could name any file on the machine running the deploy. + if mode := entry.Mode(); !mode.IsRegular() { + return 0, fmt.Errorf("%s in the fetched archive is not a regular file", entry.Name) + } + if entry.UncompressedSize64 > uint64(maxPackEntryBytes) { + return 0, fmt.Errorf("%s in the fetched archive is larger than %s", entry.Name, humanBytes(maxPackEntryBytes)) + } + + if err := os.MkdirAll(filepath.Dir(path), 0o750); err != nil { + return 0, fmt.Errorf("create a directory for %s: %w", entry.Name, err) + } + + source, err := entry.Open() + if err != nil { + return 0, fmt.Errorf("read %s from the fetched archive: %w", entry.Name, err) + } + defer func() { + _ = source.Close() + }() + + dest, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o600) //nolint:gosec // safeArchivePath keeps path under dir + if err != nil { + return 0, fmt.Errorf("write %s: %w", entry.Name, err) + } + + // One byte past the cap, so a lying UncompressedSize64 is caught by the copy + // rather than trusted. + written, err := io.Copy(dest, io.LimitReader(source, maxPackEntryBytes+1)) + if closeErr := dest.Close(); err == nil { + err = closeErr + } + if err != nil { + return 0, fmt.Errorf("write %s: %w", entry.Name, err) + } + if written > maxPackEntryBytes { + return 0, fmt.Errorf("%s in the fetched archive is larger than %s", entry.Name, humanBytes(maxPackEntryBytes)) + } + return written, nil +} + +// safeArchivePath turns an entry name into a relative path under the +// destination, refusing one that would write outside it. +func safeArchivePath(name string) (string, error) { + if name == "" { + return "", fmt.Errorf("the fetched archive holds an entry with no name") + } + // Zip names are slash-separated by specification, but a Windows-made archive + // may still carry backslashes, which are an ordinary character in a POSIX + // filename and would hide a traversal from the check below. + if strings.ContainsRune(name, '\\') { + return "", fmt.Errorf("%q in the fetched archive is not a valid path", name) + } + if strings.HasPrefix(name, "/") || filepath.IsAbs(filepath.FromSlash(name)) { + return "", fmt.Errorf("%q in the fetched archive is an absolute path", name) + } + + clean := filepath.Clean(filepath.FromSlash(name)) + if clean == ".." || strings.HasPrefix(clean, ".."+string(filepath.Separator)) { + return "", fmt.Errorf("%q in the fetched archive points outside it", name) + } + return clean, nil +} diff --git a/internal/cmd/serverless/from_test.go b/internal/cmd/serverless/from_test.go new file mode 100644 index 0000000..d391070 --- /dev/null +++ b/internal/cmd/serverless/from_test.go @@ -0,0 +1,275 @@ +package serverless + +import ( + "archive/zip" + "bytes" + "context" + "fmt" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "testing" +) + +// archiveEntry is one file to put in a test archive. mode is optional, and a +// zero mode packs an ordinary file. +type archiveEntry struct { + name string + body string + mode os.FileMode +} + +// buildTestArchive zips the entries exactly as named, including names a packer +// would never produce, so the unpacker can be tested against them. +func buildTestArchive(t *testing.T, entries []archiveEntry) []byte { + t.Helper() + + var buf bytes.Buffer + writer := zip.NewWriter(&buf) + for _, entry := range entries { + header := &zip.FileHeader{ + Name: entry.name, + Method: zip.Deflate, + } + if entry.mode != 0 { + header.SetMode(entry.mode) + } + w, err := writer.CreateHeader(header) + if err != nil { + t.Fatal(err) + } + if _, err := w.Write([]byte(entry.body)); err != nil { + t.Fatal(err) + } + } + if err := writer.Close(); err != nil { + t.Fatal(err) + } + return buf.Bytes() +} + +// serveArchive hands the bytes back over HTTP and returns the URL. +func serveArchive(t *testing.T, archive []byte) string { + t.Helper() + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write(archive) + })) + t.Cleanup(server.Close) + return server.URL + "/echo.zip" +} + +func TestFetchSourceArchive(t *testing.T) { + archive := buildTestArchive(t, []archiveEntry{ + {name: manifestFile, body: codeManifest}, + {name: testEntryFile, body: testPySource}, + {name: testNestedFile, body: "y = 2\n"}, + }) + + dir, cleanup, err := fetchSourceArchive(context.Background(), serveArchive(t, archive)) + if err != nil { + t.Fatalf("fetchSourceArchive: %v", err) + } + defer cleanup() + + manifest, err := readDeployManifest(dir) + if err != nil { + t.Fatalf("readDeployManifest: %v", err) + } + if manifest.ID != "resumable-job" { + t.Errorf("id = %q", manifest.ID) + } + + // A nested file has to survive, because an entry file is allowed to import + // its own modules. + nested, err := os.ReadFile(filepath.Join(dir, "lib", "helpers.py")) + if err != nil { + t.Fatalf("read the nested file: %v", err) + } + if string(nested) != "y = 2\n" { + t.Errorf("nested file = %q", nested) + } + + cleanup() + if _, err := os.Stat(dir); !os.IsNotExist(err) { + t.Errorf("cleanup left %s behind", dir) + } +} + +// The archive comes off the network, so an entry that writes outside the +// directory we unpack into has to be refused rather than followed. +func TestFetchSourceArchiveRefusesEscapingEntries(t *testing.T) { + for _, tc := range []struct { + name string + entry string + want string + }{ + { + name: "parent traversal", + entry: "../escaped.py", + want: "points outside it", + }, + { + name: "deep traversal", + entry: "nested/../../escaped.py", + want: "points outside it", + }, + { + name: "absolute path", + entry: "/etc/passwd", + want: "absolute path", + }, + { + // A backslash is an ordinary filename character on POSIX, so a + // traversal written this way would pass a slash-only check. + name: "backslash traversal", + entry: `..\..\escaped.py`, + want: "not a valid path", + }, + } { + t.Run(tc.name, func(t *testing.T) { + archive := buildTestArchive(t, []archiveEntry{ + {name: manifestFile, body: codeManifest}, + {name: tc.entry, body: "import os\n"}, + }) + + _, cleanup, err := fetchSourceArchive(context.Background(), serveArchive(t, archive)) + if cleanup != nil { + cleanup() + } + if err == nil { + t.Fatalf("%q was accepted", tc.entry) + } + if !strings.Contains(err.Error(), tc.want) { + t.Errorf("error %q does not mention %q", err, tc.want) + } + }) + } +} + +// A symlink would be read by the packer that walks this directory back, and +// could name any file on the machine running the deploy. +func TestFetchSourceArchiveRefusesSymlink(t *testing.T) { + archive := buildTestArchive(t, []archiveEntry{ + {name: manifestFile, body: codeManifest}, + {name: testEntryFile, body: "/etc/passwd", mode: os.ModeSymlink | 0o777}, + }) + + _, cleanup, err := fetchSourceArchive(context.Background(), serveArchive(t, archive)) + if cleanup != nil { + cleanup() + } + if err == nil { + t.Fatal("a symlink was accepted") + } + if !strings.Contains(err.Error(), "not a regular file") { + t.Errorf("error = %v", err) + } +} + +// The byte caps do not bound the file count, and a few hundred thousand empty +// entries cost almost nothing to send. +func TestFetchSourceArchiveRefusesTooManyEntries(t *testing.T) { + entries := make([]archiveEntry, 0, maxArchiveEntries+2) + entries = append(entries, archiveEntry{ + name: manifestFile, + body: codeManifest, + }) + for i := range maxArchiveEntries + 1 { + entries = append(entries, archiveEntry{ + name: fmt.Sprintf("pad/%d.py", i), + body: "", + }) + } + + _, cleanup, err := fetchSourceArchive(context.Background(), serveArchive(t, buildTestArchive(t, entries))) + if cleanup != nil { + cleanup() + } + if err == nil { + t.Fatal("an archive over the entry cap was accepted") + } + if !strings.Contains(err.Error(), "more than the") { + t.Errorf("error = %v", err) + } +} + +func TestFetchSourceArchiveRejectsNonHTTP(t *testing.T) { + for _, url := range []string{ + "file:///etc/passwd", + "ftp://example.com/echo.zip", + "/local/echo.zip", + } { + _, _, err := fetchSourceArchive(context.Background(), url) + if err == nil { + t.Fatalf("%q was accepted", url) + } + if !strings.Contains(err.Error(), "http or https") { + t.Errorf("%q: error = %v", url, err) + } + } +} + +func TestFetchSourceArchiveReportsStatus(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + })) + defer server.Close() + + _, _, err := fetchSourceArchive(context.Background(), server.URL+"/missing.zip") + if err == nil { + t.Fatal("expected an error") + } + if !strings.Contains(err.Error(), "answered 404") { + t.Errorf("error = %v", err) + } +} + +func TestFetchSourceArchiveRejectsNonZip(t *testing.T) { + _, _, err := fetchSourceArchive(context.Background(), serveArchive(t, []byte("\n"))) + if err == nil { + t.Fatal("expected an error") + } + if !strings.Contains(err.Error(), "read the fetched archive") { + t.Errorf("error = %v", err) + } +} + +// An archive past the source-upload cap cannot be deployed, so it is refused +// before it is held in memory rather than after the platform rejects it. +func TestFetchSourceArchiveRejectsOversizedDownload(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + chunk := make([]byte, 1<<20) + for range (maxArchiveBytes >> 20) + 1 { + if _, err := w.Write(chunk); err != nil { + return + } + } + })) + defer server.Close() + + _, _, err := fetchSourceArchive(context.Background(), server.URL+"/big.zip") + if err == nil { + t.Fatal("expected an error") + } + if !strings.Contains(err.Error(), "larger than") { + t.Errorf("error = %v", err) + } +} + +func TestSafeArchivePathKeepsOrdinaryNames(t *testing.T) { + for _, name := range []string{testEntryFile, testNestedFile, "./model.py", "a/b/c/d.txt"} { + got, err := safeArchivePath(name) + if err != nil { + t.Errorf("%q: %v", name, err) + continue + } + if filepath.IsAbs(got) || strings.HasPrefix(got, "..") { + t.Errorf("%q cleaned to %q", name, got) + } + } + if _, err := safeArchivePath(""); err == nil { + t.Error("an empty name was accepted") + } +} diff --git a/internal/cmd/serverless/init_test.go b/internal/cmd/serverless/init_test.go index ea651a4..aaa7144 100644 --- a/internal/cmd/serverless/init_test.go +++ b/internal/cmd/serverless/init_test.go @@ -499,7 +499,7 @@ func TestInit_NoStarterAppWhenPythonFilesExist(t *testing.T) { dir := t.TempDir() writeTree(t, dir, map[string]string{ pyprojectFile: minimalPyproject, - "model.py": testPySource, + testEntryFile: testPySource, }) if err := h.run(dir); err != nil { diff --git a/internal/cmd/serverless/manifest.go b/internal/cmd/serverless/manifest.go new file mode 100644 index 0000000..8a910f0 --- /dev/null +++ b/internal/cmd/serverless/manifest.go @@ -0,0 +1,253 @@ +package serverless + +import ( + "bytes" + "fmt" + "maps" + "os" + "path/filepath" + "regexp" + "slices" + "strings" + + serverlessapi "github.com/runware/runware-cli/internal/api/serverless" + "github.com/spf13/cobra" + "gopkg.in/yaml.v3" +) + +// manifestFile is the project file a packaged source carries at its root. It +// records the deploy options a person would otherwise retype as flags, so a +// client can offer the deploy without someone first reading how. +const manifestFile = "runware.yaml" + +// manifestVersion is the only configVersion this CLI understands. The field +// exists so a later format can be recognized rather than misread, and it is +// counted separately from container.yaml's own version. +const manifestVersion = 1 + +// manifestAppIDPattern is the server's AppId rule, which also fixes the length +// at 6 to 30 characters. It is checked here because with --from the 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 it broke. +var manifestAppIDPattern = regexp.MustCompile(`^[a-z][a-z0-9-]{4,28}[a-z0-9]$`) + +// deployManifest is runware.yaml. Every field maps to a deploy flag, so the +// file adds no behavior of its own: reading it and passing the flags by hand +// produce the same application. +// +// It holds no secret values. `secrets` names what the app needs, and the +// secret has to already exist in the environment. +type deployManifest struct { + ConfigVersion int `yaml:"configVersion"` + ID string `yaml:"id"` + Name string `yaml:"name"` + Source manifestSource `yaml:"source"` + Compute manifestCompute `yaml:"compute"` + Build manifestBuild `yaml:"build"` + Volumes []string `yaml:"volumes"` + EnvVars map[string]string `yaml:"envVars"` + Secrets []manifestSecret `yaml:"secrets"` +} + +type manifestSource struct { + Type string `yaml:"type"` + Entry string `yaml:"entry"` +} + +type manifestCompute struct { + GpuType string `yaml:"gpuType"` + FallbackGpuType string `yaml:"fallbackGpuType"` + GpusPerWorker *int32 `yaml:"gpusPerWorker"` + MinWorkers *int32 `yaml:"minWorkers"` + MaxWorkers *int32 `yaml:"maxWorkers"` + MinAvailableWorkers *int32 `yaml:"minAvailableWorkers"` + AvailableWorkersPct *int32 `yaml:"availableWorkersPct"` + IdleTTL *int32 `yaml:"idleTtl"` + ScalingDelay *int32 `yaml:"scalingDelay"` +} + +type manifestBuild struct { + BaseImage string `yaml:"baseImage"` + Requirements []string `yaml:"requirements"` +} + +type manifestSecret struct { + Name string `yaml:"name"` + EnvVarName string `yaml:"envVarName"` +} + +// flag renders the secret the way --secret takes it. +func (s manifestSecret) flag() string { + if s.EnvVarName == "" { + return s.Name + } + return s.Name + "=" + s.EnvVarName +} + +// readDeployManifest reads and validates the manifest at the root of dir. +// +// Unknown keys are rejected rather than ignored. A manifest is hand-written, +// and a misspelled key that silently does nothing surfaces later as a missing +// required flag, which says nothing about the typo that caused it. +func readDeployManifest(dir string) (deployManifest, error) { + path := filepath.Join(dir, manifestFile) + + raw, err := os.ReadFile(path) //nolint:gosec // the path is manifestFile under a directory we chose + if err != nil { + if os.IsNotExist(err) { + return deployManifest{}, fmt.Errorf("no %s at the root of the source; there is nothing to deploy it with", manifestFile) + } + return deployManifest{}, fmt.Errorf("read %s: %w", manifestFile, err) + } + + decoder := yaml.NewDecoder(bytes.NewReader(raw)) + decoder.KnownFields(true) + + var manifest deployManifest + if err := decoder.Decode(&manifest); err != nil { + return deployManifest{}, fmt.Errorf("read %s: %w", manifestFile, err) + } + + if err := manifest.validate(); err != nil { + return deployManifest{}, fmt.Errorf("%s: %w", manifestFile, err) + } + return manifest, nil +} + +func (m deployManifest) validate() error { + if m.ConfigVersion != manifestVersion { + if m.ConfigVersion == 0 { + return fmt.Errorf("configVersion is required and must be %d", manifestVersion) + } + return fmt.Errorf("configVersion %d is not supported; this CLI understands %d", m.ConfigVersion, manifestVersion) + } + if m.ID == "" { + return fmt.Errorf("id is required") + } + if !manifestAppIDPattern.MatchString(m.ID) { + return fmt.Errorf( + "id %q is not a valid application id (6 to 30 characters, lowercase letters, digits and hyphens, starting with a letter and ending with a letter or digit)", + m.ID, + ) + } + + switch m.Source.Type { + case string(serverlessapi.AppSourceTypeCode): + if m.Source.Entry == "" { + return fmt.Errorf("source.entry is required for a %s app", serverlessapi.AppSourceTypeCode) + } + if err := validateManifestEntry(m.Source.Entry); err != nil { + return err + } + case string(serverlessapi.AppSourceTypeContainer): + if m.Source.Entry != "" { + return fmt.Errorf("source.entry applies to a %s app only", serverlessapi.AppSourceTypeCode) + } + if m.Build.BaseImage != "" || len(m.Build.Requirements) > 0 { + return fmt.Errorf("build applies to a %s app only", serverlessapi.AppSourceTypeCode) + } + case "": + return fmt.Errorf("source.type is required (%s or %s)", serverlessapi.AppSourceTypeCode, serverlessapi.AppSourceTypeContainer) + default: + return fmt.Errorf("source.type %q is not %s or %s", m.Source.Type, serverlessapi.AppSourceTypeCode, serverlessapi.AppSourceTypeContainer) + } + + for _, secret := range m.Secrets { + if _, _, err := splitSecretAttach(secret.flag()); err != nil { + return fmt.Errorf("secret %q is not a name, or a name and an environment variable", secret.flag()) + } + } + if m.Compute.GpusPerWorker != nil { + if err := validateGPUsPerWorker(*m.Compute.GpusPerWorker); err != nil { + return fmt.Errorf("compute.gpusPerWorker: %w", err) + } + } + if m.Compute.AvailableWorkersPct != nil { + if err := validateAvailableWorkersPct(*m.Compute.AvailableWorkersPct); err != nil { + return fmt.Errorf("compute.availableWorkersPct: %w", err) + } + } + if m.Name != "" { + if err := validateAppName(m.Name); err != nil { + return fmt.Errorf("name: %w", err) + } + } + return nil +} + +// validateManifestEntry keeps the entry file inside the unpacked source. The +// deploy resolves it against a directory this command created, and a path that +// climbs out of it would read the machine running the deploy. +func validateManifestEntry(entry string) error { + if filepath.IsAbs(entry) || strings.HasPrefix(entry, "/") { + return fmt.Errorf("source.entry %q must be relative to the source root", entry) + } + clean := filepath.ToSlash(filepath.Clean(filepath.FromSlash(entry))) + if clean == ".." || strings.HasPrefix(clean, "../") { + return fmt.Errorf("source.entry %q must stay inside the source root", entry) + } + return nil +} + +// localSource says how the deploy reads the unpacked archive: the arguments +// the command would have been given, and the directory flags that go with +// them. A code app names its entry file, a container app names its directory. +func (m deployManifest) localSource(dir string) (args []string, srcDir, containerDir string) { + if m.Source.Type == string(serverlessapi.AppSourceTypeContainer) { + return nil, "", dir + } + return []string{m.Source.Entry}, dir, "" +} + +// envVarFlags renders the environment variables the way --env-var takes them, +// in a fixed order so two deploys of the same manifest behave the same. +func (m deployManifest) envVarFlags() []string { + out := make([]string, 0, len(m.EnvVars)) + for _, key := range slices.Sorted(maps.Keys(m.EnvVars)) { + out = append(out, key+"="+m.EnvVars[key]) + } + return out +} + +// secretFlags renders the secrets the way --secret takes them. +func (m deployManifest) secretFlags() []string { + out := make([]string, 0, len(m.Secrets)) + for _, secret := range m.Secrets { + out = append(out, secret.flag()) + } + return out +} + +// manifestString is the manifest's value for a flag the command line left +// alone. The four helpers below are the whole merge rule: a flag the person +// passed always wins, and an absent manifest field changes nothing. +func manifestString(cmd *cobra.Command, name, passed, fromManifest string) string { + if cmd.Flags().Changed(name) || fromManifest == "" { + return passed + } + return fromManifest +} + +func manifestInt32(cmd *cobra.Command, name string, passed int32, fromManifest *int32) int32 { + if cmd.Flags().Changed(name) || fromManifest == nil { + return passed + } + return *fromManifest +} + +// manifestInt32Ptr keeps optionalInt32Ptr's contract: nil when neither the +// command line nor the manifest says anything, so the field stays out of the +// request and the server's own default applies. +func manifestInt32Ptr(cmd *cobra.Command, name string, passed int32, fromManifest *int32) *int32 { + if cmd.Flags().Changed(name) { + return &passed + } + return fromManifest +} + +func manifestStringPtr(cmd *cobra.Command, name, passed, fromManifest string) *string { + if cmd.Flags().Changed(name) { + return &passed + } + return optionalStringPtr(fromManifest) +} diff --git a/internal/cmd/serverless/manifest_test.go b/internal/cmd/serverless/manifest_test.go new file mode 100644 index 0000000..653f07a --- /dev/null +++ b/internal/cmd/serverless/manifest_test.go @@ -0,0 +1,300 @@ +package serverless + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/spf13/cobra" +) + +const testManifestID = "resumable-job" + +const codeManifest = `configVersion: 1 +id: resumable-job +source: + type: code + entry: model.py +compute: + gpuType: rtx-pro-6000 +secrets: + - name: S3_BUCKET + - name: HF_TOKEN + envVarName: HUGGING_FACE_HUB_TOKEN +` + +// writeManifest puts body at the root of a new directory and returns it. +func writeManifest(t *testing.T, body string) string { + t.Helper() + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, manifestFile), []byte(body), 0o600); err != nil { + t.Fatal(err) + } + return dir +} + +func readManifestBody(t *testing.T, body string) deployManifest { + t.Helper() + manifest, err := readDeployManifest(writeManifest(t, body)) + if err != nil { + t.Fatalf("readDeployManifest: %v", err) + } + return manifest +} + +func manifestError(t *testing.T, body string) string { + t.Helper() + _, err := readDeployManifest(writeManifest(t, body)) + if err == nil { + t.Fatal("expected an error") + } + return err.Error() +} + +func TestReadDeployManifestCode(t *testing.T) { + manifest := readManifestBody(t, codeManifest) + + if manifest.ID != testManifestID { + t.Errorf("id = %q", manifest.ID) + } + if manifest.Compute.GpuType != "rtx-pro-6000" { + t.Errorf("compute.gpuType = %q", manifest.Compute.GpuType) + } + + args, srcDir, containerDir := manifest.localSource("/unpacked") + if len(args) != 1 || args[0] != testEntryFile { + t.Errorf("args = %v", args) + } + if srcDir != "/unpacked" || containerDir != "" { + t.Errorf("srcDir = %q, containerDir = %q", srcDir, containerDir) + } + + secrets := manifest.secretFlags() + want := []string{"S3_BUCKET", "HF_TOKEN=HUGGING_FACE_HUB_TOKEN"} + if strings.Join(secrets, ",") != strings.Join(want, ",") { + t.Errorf("secretFlags = %v, want %v", secrets, want) + } +} + +func TestReadDeployManifestContainer(t *testing.T) { + manifest := readManifestBody(t, `configVersion: 1 +id: render-farm +source: + type: container +compute: + gpuType: rtx-pro-6000 + maxWorkers: 4 +`) + + args, srcDir, containerDir := manifest.localSource("/unpacked") + if args != nil || srcDir != "" { + t.Errorf("a container app named args %v and srcDir %q", args, srcDir) + } + if containerDir != "/unpacked" { + t.Errorf("containerDir = %q", containerDir) + } + if manifest.Compute.MaxWorkers == nil || *manifest.Compute.MaxWorkers != 4 { + t.Errorf("compute.maxWorkers = %v", manifest.Compute.MaxWorkers) + } +} + +// envVarFlags has to be ordered: an unordered map would make two deploys of +// one manifest differ in the order they report and send variables. +func TestManifestEnvVarFlagsAreSorted(t *testing.T) { + manifest := readManifestBody(t, `configVersion: 1 +id: config-and-secrets +source: + type: code + entry: model.py +envVars: + LOG_LEVEL: info + CACHE_DIR: /weights + BATCH: "8" +`) + + got := strings.Join(manifest.envVarFlags(), " ") + want := "BATCH=8 CACHE_DIR=/weights LOG_LEVEL=info" + if got != want { + t.Errorf("envVarFlags = %q, want %q", got, want) + } +} + +// A misspelled key is the failure this file exists to catch: ignored, it turns +// into a missing required flag that says nothing about the typo behind it. +func TestReadDeployManifestRejectsUnknownKey(t *testing.T) { + msg := manifestError(t, `configVersion: 1 +id: echo-app +source: + type: code + entry: model.py +compute: + gpu_type: rtx-pro-6000 +`) + if !strings.Contains(msg, "gpu_type") { + t.Errorf("error does not name the unknown key: %s", msg) + } +} + +func TestReadDeployManifestVersion(t *testing.T) { + missing := manifestError(t, `id: echo-app +source: + type: code + entry: model.py +`) + if !strings.Contains(missing, "configVersion is required") { + t.Errorf("missing configVersion: %s", missing) + } + + future := manifestError(t, `configVersion: 2 +id: echo-app +source: + type: code + entry: model.py +`) + if !strings.Contains(future, "not supported") { + t.Errorf("future configVersion: %s", future) + } +} + +func TestReadDeployManifestRequiresSource(t *testing.T) { + for body, want := range map[string]string{ + "configVersion: 1\nid: echo-app\n": "source.type is required", + "configVersion: 1\nid: echo-app\nsource:\n type: wasm\n": "is not code or container", + "configVersion: 1\nid: echo-app\nsource:\n type: code\n": "source.entry is required", + "configVersion: 1\nsource:\n type: code\n entry: model.py\n": "id is required", + "configVersion: 1\nid: echo-app\nsource:\n type: container\n entry: m.py\n": "source.entry applies to a code app only", + } { + if msg := manifestError(t, body); !strings.Contains(msg, want) { + t.Errorf("%q: got %q, want it to mention %q", body, msg, want) + } + } +} + +// The entry file is resolved inside a directory this command created, so a +// path that climbs out of it would read the machine running the deploy. +func TestReadDeployManifestRejectsEscapingEntry(t *testing.T) { + for _, entry := range []string{"../../etc/passwd", "/etc/passwd"} { + msg := manifestError(t, "configVersion: 1\nid: echo-app\nsource:\n type: code\n entry: "+entry+"\n") + if !strings.Contains(msg, "source.entry") { + t.Errorf("entry %q: %s", entry, msg) + } + } +} + +func TestReadDeployManifestMissingFile(t *testing.T) { + _, err := readDeployManifest(t.TempDir()) + if err == nil || !strings.Contains(err.Error(), "nothing to deploy it with") { + t.Fatalf("err = %v", err) + } +} + +func TestReadDeployManifestRejectsBadSecret(t *testing.T) { + msg := manifestError(t, `configVersion: 1 +id: echo-app +source: + type: code + entry: model.py +secrets: + - name: not a name +`) + if !strings.Contains(msg, "secret") { + t.Errorf("error = %s", msg) + } +} + +func TestReadDeployManifestRejectsBadCompute(t *testing.T) { + msg := manifestError(t, `configVersion: 1 +id: echo-app +source: + type: code + entry: model.py +compute: + gpusPerWorker: 3 +`) + if !strings.Contains(msg, "gpusPerWorker") { + t.Errorf("error = %s", msg) + } +} + +// mergeCmd is a command carrying the deploy flags the merge helpers consult. +func mergeCmd(t *testing.T, passed ...string) *cobra.Command { + t.Helper() + cmd := &cobra.Command{ + Use: "deploy", + RunE: func(*cobra.Command, []string) error { + return nil + }, + } + cmd.Flags().String("id", "", "") + cmd.Flags().String("gpu-type", "", "") + cmd.Flags().Int32("max-workers", 1, "") + cmd.Flags().Int32("min-workers", 0, "") + cmd.Flags().String("fallback-gpu-type", "", "") + + cmd.SetArgs(passed) + cmd.SetOut(nil) + if err := cmd.Execute(); err != nil { + t.Fatal(err) + } + return cmd +} + +// A flag the person passed wins over the manifest, which is what lets one +// archive deploy on another GPU type without being edited. +func TestManifestMergePrefersPassedFlags(t *testing.T) { + cmd := mergeCmd(t, "--gpu-type", testFallbackGPU, "--min-workers", "2") + + if got := manifestString(cmd, "gpu-type", testFallbackGPU, "rtx-pro-6000"); got != testFallbackGPU { + t.Errorf("gpu-type = %q, want the passed l40s", got) + } + if got := manifestString(cmd, "id", "", testManifestID); got != testManifestID { + t.Errorf("id = %q, want the manifest's", got) + } + + fromManifest := int32(4) + if got := manifestInt32(cmd, "max-workers", 1, &fromManifest); got != 4 { + t.Errorf("max-workers = %d, want the manifest's 4", got) + } + + manifestMin := int32(1) + if got := manifestInt32Ptr(cmd, "min-workers", 2, &manifestMin); got == nil || *got != 2 { + t.Errorf("min-workers = %v, want the passed 2", got) + } +} + +// Nothing in the merge may invent a value. An absent field has to leave the +// API request without the key, so the server's own default applies. +func TestManifestMergeKeepsAbsentAbsent(t *testing.T) { + cmd := mergeCmd(t) + + if got := manifestInt32Ptr(cmd, "min-workers", 0, nil); got != nil { + t.Errorf("min-workers = %v, want nil", got) + } + if got := manifestStringPtr(cmd, "fallback-gpu-type", "", ""); got != nil { + t.Errorf("fallback-gpu-type = %v, want nil", got) + } + if got := manifestStringPtr(cmd, "fallback-gpu-type", "", testFallbackGPU); got == nil || *got != testFallbackGPU { + t.Errorf("fallback-gpu-type = %v, want the manifest's l40s", got) + } + if got := manifestInt32(cmd, "max-workers", 1, nil); got != 1 { + t.Errorf("max-workers = %d, want the flag default 1", got) + } +} + +// With --from the id comes out of a file the person deploying did not write, +// so an id the platform cannot accept has to be named here rather than come +// back as a path-parameter complaint. +func TestReadDeployManifestRejectsBadAppID(t *testing.T) { + for _, id := range []string{"echo", "Echo-App", "-echo-app", "echo-app-", "echo_app", strings.Repeat("e", 31)} { + msg := manifestError(t, "configVersion: 1\nid: "+id+"\nsource:\n type: code\n entry: model.py\n") + if !strings.Contains(msg, "valid application id") { + t.Errorf("id %q: %s", id, msg) + } + } + for _, id := range []string{"echo-app", testManifestID, "a1b2c3", strings.Repeat("e", 30)} { + if _, err := readDeployManifest(writeManifest(t, "configVersion: 1\nid: "+id+"\nsource:\n type: code\n entry: model.py\n")); err != nil { + t.Errorf("id %q was refused: %v", id, err) + } + } +} diff --git a/internal/cmd/serverless/pack_test.go b/internal/cmd/serverless/pack_test.go index 6cf4ead..dad0e2b 100644 --- a/internal/cmd/serverless/pack_test.go +++ b/internal/cmd/serverless/pack_test.go @@ -57,8 +57,10 @@ func unpack(t *testing.T, raw []byte) map[string]string { // Repeated across the table-free tests below; goconst wants them named. const ( - testModelFile = "app.py" - testPySource = "x = 1\n" + testModelFile = "app.py" + testPySource = "x = 1\n" + testEntryFile = "model.py" + testNestedFile = "lib/helpers.py" ) func names(packed map[string]string) []string { @@ -78,7 +80,7 @@ func TestPackDirectory(t *testing.T) { writeTree(t, dir, map[string]string{ testModelFile: "import lib\n", "lib/__init__.py": "", - "lib/helpers.py": "def go(): pass\n", + testNestedFile: "def go(): pass\n", "data/prompt.txt": "hello", "nested/deep/x.py": testPySource, }) @@ -92,13 +94,13 @@ func TestPackDirectory(t *testing.T) { } packed := unpack(t, encoded) - for _, want := range []string{testModelFile, "lib/helpers.py", "data/prompt.txt", "nested/deep/x.py"} { + for _, want := range []string{testModelFile, testNestedFile, "data/prompt.txt", "nested/deep/x.py"} { if _, ok := packed[want]; !ok { t.Errorf("%q missing from the archive; got %v", want, names(packed)) } } - if packed["lib/helpers.py"] != "def go(): pass\n" { - t.Errorf("lib/helpers.py content = %q", packed["lib/helpers.py"]) + if packed[testNestedFile] != "def go(): pass\n" { + t.Errorf("lib/helpers.py content = %q", packed[testNestedFile]) } } From 36772526095690f6647fd4a21ba464ac951058db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A9lix=20Sanz?= Date: Wed, 4 Nov 2026 16:54:42 +0100 Subject: [PATCH 02/13] fix(serverless): refuse code-only flags on a container archive, keep its executable bits --- internal/cmd/serverless/deploy.go | 17 ++++++- internal/cmd/serverless/deploy_test.go | 70 +++++++++++++++++++++++--- internal/cmd/serverless/from.go | 14 +++++- internal/cmd/serverless/from_test.go | 33 ++++++++++++ 4 files changed, 125 insertions(+), 9 deletions(-) diff --git a/internal/cmd/serverless/deploy.go b/internal/cmd/serverless/deploy.go index fe9af08..0125621 100644 --- a/internal/cmd/serverless/deploy.go +++ b/internal/cmd/serverless/deploy.go @@ -102,14 +102,23 @@ func validateDeployArgs(cmd *cobra.Command, args []string, containerDir, from st return fmt.Errorf("--%s names a local source; --from takes the source from the archive", name) } } + // The source type is inside the archive, so the code-only flags cannot be + // judged yet. refuseCodeOnlyFlags runs again once the manifest is read. return nil } if !hasContainer { return nil } + return refuseCodeOnlyFlags(cmd, "omit it when using --container") +} + +// refuseCodeOnlyFlags rejects the flags a container deploy has no use for. A +// deploy that accepted and then discarded them would differ from the command +// the person wrote without saying so. +func refuseCodeOnlyFlags(cmd *cobra.Command, remedy string) error { for _, name := range codeOnlyDeployFlags { if cmd.Flags().Changed(name) { - return fmt.Errorf("--%s applies to code deploys only; omit it when using --container", name) + return fmt.Errorf("--%s applies to code deploys only; %s", name, remedy) } } return nil @@ -329,6 +338,12 @@ paths and an invoke example once the application is active.`, // refused on an update by exactly that test, and a manifest that // carries a GPU type is describing the app, not asking to change a // running one, so a redeploy from the same archive has to go through. + if manifest.Source.Type == string(serverlessapi.AppSourceTypeContainer) { + if err := refuseCodeOnlyFlags(cmd, "this archive holds a container source"); err != nil { + return err + } + } + args, srcDir, containerDir = manifest.localSource(dir) id = manifestString(cmd, "id", id, manifest.ID) name = manifestString(cmd, "name", name, manifest.Name) diff --git a/internal/cmd/serverless/deploy_test.go b/internal/cmd/serverless/deploy_test.go index 413dd71..2a2f876 100644 --- a/internal/cmd/serverless/deploy_test.go +++ b/internal/cmd/serverless/deploy_test.go @@ -26,6 +26,8 @@ const ( testSrcDirFlag = "--src-dir" testFromFlag = "--from" testFallbackGPU = "l40s" + testBaseImageFlag = "--base-image" + testRequirement = "--requirement" testArchiveURL = "https://runware.ai/docs/serverless/examples/echo.zip" testIDFlag = "--id" testWaitFlag = "--wait" @@ -88,13 +90,13 @@ func TestValidateDeployArgs(t *testing.T) { }, { name: "container with base-image", - flags: []string{testContainerFlag, testWrapperDir, "--base-image", "python:3.12-slim"}, - wantErr: "--base-image", + flags: []string{testContainerFlag, testWrapperDir, testBaseImageFlag, "python:3.12-slim"}, + wantErr: testBaseImageFlag, }, { name: "container with requirement", - flags: []string{testContainerFlag, testWrapperDir, "--requirement", testPipPackage}, - wantErr: "--requirement", + flags: []string{testContainerFlag, testWrapperDir, testRequirement, testPipPackage}, + wantErr: testRequirement, }, { name: "code with src-dir", @@ -482,8 +484,8 @@ func TestValidateUpdateDeployFlags(t *testing.T) { {flags: []string{"--min-available-workers", "1"}, wantErr: scaleHint}, {flags: []string{"--available-workers-pct", "10"}, wantErr: scaleHint}, {flags: []string{"--name", "My App"}, wantErr: "apps rename"}, - {flags: []string{"--requirement", testPipPackage}}, - {flags: []string{"--base-image", "python:3.12-slim"}}, + {flags: []string{testRequirement, testPipPackage}}, + {flags: []string{testBaseImageFlag, "python:3.12-slim"}}, {flags: []string{testSrcDirFlag, "."}}, {flags: []string{testWaitFlag}}, } @@ -725,3 +727,59 @@ func TestAppFailedErr_BuildsHintAddressesTheSameCopy(t *testing.T) { }) } } + +// --from cannot judge the code-only flags up front, because the source type is +// inside the archive. Once the manifest says container, they have to be refused +// rather than accepted and dropped by the packer. +func TestRefuseCodeOnlyFlags(t *testing.T) { + for _, tc := range []struct { + name string + flags []string + wantErr string + }{ + { + name: "nothing code-only passed", + flags: []string{testFromFlag, testArchiveURL}, + }, + { + name: "base-image", + flags: []string{testFromFlag, testArchiveURL, testBaseImageFlag, "python:3.12-slim"}, + wantErr: testBaseImageFlag, + }, + { + name: "requirement", + flags: []string{testFromFlag, testArchiveURL, testRequirement, testPipPackage}, + wantErr: testRequirement, + }, + } { + t.Run(tc.name, func(t *testing.T) { + cmd := newDeployCmd(nil) + if err := cmd.ParseFlags(tc.flags); err != nil { + t.Fatalf("ParseFlags: %v", err) + } + + // What validateDeployArgs cannot do before the fetch, and what the + // deploy does as soon as the manifest names a container source. + if err := validateDeployArgs(cmd, nil, "", testArchiveURL); err != nil { + t.Fatalf("the source choice itself was refused: %v", err) + } + + err := refuseCodeOnlyFlags(cmd, "this archive holds a container source") + if tc.wantErr == "" { + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + return + } + if err == nil { + t.Fatal("expected an error") + } + if !strings.Contains(err.Error(), tc.wantErr) { + t.Errorf("error %q does not name %q", err, tc.wantErr) + } + if !strings.Contains(err.Error(), "container source") { + t.Errorf("error %q does not say why", err) + } + }) + } +} diff --git a/internal/cmd/serverless/from.go b/internal/cmd/serverless/from.go index d35aed6..cb5b81e 100644 --- a/internal/cmd/serverless/from.go +++ b/internal/cmd/serverless/from.go @@ -71,8 +71,10 @@ func downloadArchive(ctx context.Context, rawURL string) ([]byte, error) { if err != nil { return nil, fmt.Errorf("fetch %s: %w", rawURL, err) } + // Closed without being drained. The happy path reads the body to the end + // anyway, and on every other path the body is one we have just refused: + // reading the rest of it only gives a hostile source longer to send. defer func() { - _, _ = io.Copy(io.Discard, resp.Body) _ = resp.Body.Close() }() @@ -153,7 +155,15 @@ func unpackEntry(entry *zip.File, dir string) (int64, error) { _ = source.Close() }() - dest, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o600) //nolint:gosec // safeArchivePath keeps path under dir + // Owner-only, since this directory is ours, but the executable bit is kept: + // the packer that reads the directory back archives an executable entry as + // 0755, and a container whose entrypoint script lost it would not start. + mode := os.FileMode(0o600) + if entry.Mode().Perm()&0o111 != 0 { + mode = 0o700 + } + + dest, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, mode) //nolint:gosec // safeArchivePath keeps path under dir if err != nil { return 0, fmt.Errorf("write %s: %w", entry.Name, err) } diff --git a/internal/cmd/serverless/from_test.go b/internal/cmd/serverless/from_test.go index d391070..03f5627 100644 --- a/internal/cmd/serverless/from_test.go +++ b/internal/cmd/serverless/from_test.go @@ -273,3 +273,36 @@ func TestSafeArchivePathKeepsOrdinaryNames(t *testing.T) { t.Error("an empty name was accepted") } } + +// The packer archives an executable entry as 0755, so an entrypoint script that +// arrives executable has to still be executable when the packer walks it back. +func TestFetchSourceArchiveKeepsTheExecutableBit(t *testing.T) { + archive := buildTestArchive(t, []archiveEntry{ + {name: manifestFile, body: codeManifest}, + {name: testEntryFile, body: testPySource}, + {name: "entrypoint.sh", body: "#!/bin/sh\nexec python model.py\n", mode: 0o755}, + {name: "notes.txt", body: "plain\n", mode: 0o644}, + }) + + dir, cleanup, err := fetchSourceArchive(context.Background(), serveArchive(t, archive)) + if err != nil { + t.Fatalf("fetchSourceArchive: %v", err) + } + defer cleanup() + + executable, err := os.Stat(filepath.Join(dir, "entrypoint.sh")) + if err != nil { + t.Fatal(err) + } + if executable.Mode().Perm()&0o100 == 0 { + t.Errorf("entrypoint.sh came out %v, with no owner execute bit", executable.Mode().Perm()) + } + + plain, err := os.Stat(filepath.Join(dir, "notes.txt")) + if err != nil { + t.Fatal(err) + } + if plain.Mode().Perm()&0o111 != 0 { + t.Errorf("notes.txt came out %v, executable when the archive said otherwise", plain.Mode().Perm()) + } +} From 7f93e82f4486898d93cc63b6f30a8e13995343a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A9lix=20Sanz?= Date: Wed, 4 Nov 2026 17:58:08 +0100 Subject: [PATCH 03/13] test(serverless): skip the executable-bit check on Windows --- internal/cmd/serverless/from_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/internal/cmd/serverless/from_test.go b/internal/cmd/serverless/from_test.go index 03f5627..4a82899 100644 --- a/internal/cmd/serverless/from_test.go +++ b/internal/cmd/serverless/from_test.go @@ -9,6 +9,7 @@ import ( "net/http/httptest" "os" "path/filepath" + "runtime" "strings" "testing" ) @@ -277,6 +278,9 @@ func TestSafeArchivePathKeepsOrdinaryNames(t *testing.T) { // The packer archives an executable entry as 0755, so an entrypoint script that // arrives executable has to still be executable when the packer walks it back. func TestFetchSourceArchiveKeepsTheExecutableBit(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("no executable bit to preserve on Windows") + } archive := buildTestArchive(t, []archiveEntry{ {name: manifestFile, body: codeManifest}, {name: testEntryFile, body: testPySource}, From b95f479843167f002f50706f95f9d0072ed58f38 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A9lix=20Sanz?= Date: Wed, 4 Nov 2026 17:58:48 +0100 Subject: [PATCH 04/13] test(serverless): use the existing goosWindows constant --- internal/cmd/serverless/from_test.go | 2 +- internal/cmd/serverless/pack_test.go | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/cmd/serverless/from_test.go b/internal/cmd/serverless/from_test.go index 4a82899..4292b97 100644 --- a/internal/cmd/serverless/from_test.go +++ b/internal/cmd/serverless/from_test.go @@ -278,7 +278,7 @@ func TestSafeArchivePathKeepsOrdinaryNames(t *testing.T) { // The packer archives an executable entry as 0755, so an entrypoint script that // arrives executable has to still be executable when the packer walks it back. func TestFetchSourceArchiveKeepsTheExecutableBit(t *testing.T) { - if runtime.GOOS == "windows" { + if runtime.GOOS == goosWindows { t.Skip("no executable bit to preserve on Windows") } archive := buildTestArchive(t, []archiveEntry{ diff --git a/internal/cmd/serverless/pack_test.go b/internal/cmd/serverless/pack_test.go index dad0e2b..43dcee2 100644 --- a/internal/cmd/serverless/pack_test.go +++ b/internal/cmd/serverless/pack_test.go @@ -360,7 +360,7 @@ func TestPackDirectory_ModelFileAlwaysPacked(t *testing.T) { // TestPackDirectory_SkipsSymlinks: a symlink may point outside the tree, and the // archive is a copy rather than a checkout. func TestPackDirectory_SkipsSymlinks(t *testing.T) { - if runtime.GOOS == "windows" { + if runtime.GOOS == goosWindows { t.Skip("symlink creation needs privileges on Windows") } dir := t.TempDir() @@ -733,7 +733,7 @@ func TestPackDirectory_ExcludedDirectoryKeepsExcludingSiblings(t *testing.T) { // entrypoint script, and one that arrives non-executable fails at run time with // nothing about the archive to explain it. func TestPackDirectory_PreservesFileMode(t *testing.T) { - if runtime.GOOS == "windows" { + if runtime.GOOS == goosWindows { t.Skip("no executable bit to preserve on Windows") } dir := t.TempDir() @@ -767,7 +767,7 @@ func TestPackDirectory_PreservesFileMode(t *testing.T) { // A symlink as the model file used to pass the local stat and then be skipped by // the walk, uploading an archive whose declared entry point is absent. func TestPackDirectory_SymlinkModelFileRejected(t *testing.T) { - if runtime.GOOS == "windows" { + if runtime.GOOS == goosWindows { t.Skip("symlink creation needs privileges on Windows") } dir := t.TempDir() From 75373e3f14516f108249ce5964691f6090de53ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A9lix=20Sanz?= Date: Wed, 4 Nov 2026 18:11:45 +0100 Subject: [PATCH 05/13] fix(serverless): bound what an archive can unpack, and send a manifest's values unparsed --- internal/cmd/serverless/deploy.go | 12 +++- internal/cmd/serverless/env_vars.go | 40 +++++++++++--- internal/cmd/serverless/env_vars_test.go | 70 +++++++++++++++++++----- internal/cmd/serverless/from.go | 16 +++++- internal/cmd/serverless/from_test.go | 29 ++++++++++ internal/cmd/serverless/manifest.go | 12 ---- internal/cmd/serverless/manifest_test.go | 21 ++++--- 7 files changed, 152 insertions(+), 48 deletions(-) diff --git a/internal/cmd/serverless/deploy.go b/internal/cmd/serverless/deploy.go index 0125621..6c53866 100644 --- a/internal/cmd/serverless/deploy.go +++ b/internal/cmd/serverless/deploy.go @@ -318,7 +318,10 @@ paths and an invoke example once the application is active.`, return err } - var manifest deployManifest + var ( + manifest deployManifest + manifestEnv map[string]string + ) if from != "" { dir, cleanup, err := fetchSourceArchive(cmd.Context(), from) if err != nil { @@ -359,7 +362,10 @@ paths and an invoke example once the application is active.`, volumes = manifest.Volumes } if !cmd.Flags().Changed(envVarFlag) { - envVars = manifest.envVarFlags() + // The map itself, not rendered as KEY=VALUE: a value that + // legitimately carries quotes would be unquoted a second + // time on the way back in. + manifestEnv = manifest.EnvVars } if !cmd.Flags().Changed("secret") { secrets = manifest.secretFlags() @@ -421,7 +427,7 @@ paths and an invoke example once the application is active.`, if err != nil { return err } - appEnv, err = buildEnvironmentVariables(envFiles, envVars) + appEnv, err = buildEnvironmentVariables(envFiles, envVars, manifestEnv) if err != nil { return err } diff --git a/internal/cmd/serverless/env_vars.go b/internal/cmd/serverless/env_vars.go index ca75b6e..81f12b1 100644 --- a/internal/cmd/serverless/env_vars.go +++ b/internal/cmd/serverless/env_vars.go @@ -158,7 +158,7 @@ allowed.`, if hasSingleValue { return fmt.Errorf("%s", envSetUsage) } - updates, err := buildEnvironmentVariables(envFiles, envPairs) + updates, err := buildEnvironmentVariables(envFiles, envPairs, nil) if err != nil { return err } @@ -332,12 +332,22 @@ var envNamePattern = regexp.MustCompile(`^[A-Za-z_][A-Za-z0-9_]{0,127}$`) // buildEnvironmentVariables turns --env-var KEY=VALUE pairs and --env-file paths // into a name-to-value map. Files are read first, so an explicit --env-var wins // over a file entry with the same name. -func buildEnvironmentVariables(files, pairs []string) (*map[string]string, error) { - if len(files) == 0 && len(pairs) == 0 { +// buildEnvironmentVariables collects the variables a create sends. `decoded` +// holds values that never were text, read from a manifest: they are taken as +// they are, because running them back through the KEY=VALUE parser would +// unquote a value that legitimately carries quotes. +func buildEnvironmentVariables(files, pairs []string, decoded map[string]string) (*map[string]string, error) { + if len(files) == 0 && len(pairs) == 0 && len(decoded) == 0 { return nil, nil } env := make(map[string]string) + for name, value := range decoded { + if err := validateEnvEntry(name, value); err != nil { + return nil, err + } + env[name] = value + } for _, path := range files { if err := readEnvFile(path, env); err != nil { return nil, err @@ -420,20 +430,32 @@ func splitEnvAssignment(assignment string) (name, value string, err error) { name = strings.TrimSpace(name) value = unquote(value) - switch { - case name == "": + if name == "" { return "", "", fmt.Errorf("%q has an empty name", assignment) + } + if err := validateEnvEntry(name, value); err != nil { + return "", "", err + } + return name, value, nil +} + +// validateEnvEntry checks one name and value against the limits the server +// enforces. Separate from the parsing above because a value that was never +// text -- one decoded from a manifest -- must be checked without being +// unquoted a second time. +func validateEnvEntry(name, value string) error { + switch { // Counted in runes, not bytes: the API's maxLength is characters, so // len() would reject a valid non-ASCII value at half the documented limit. case utf8.RuneCountInString(name) > maxEnvNameLen: - return "", "", fmt.Errorf("environment variable name %q exceeds %d characters", name, maxEnvNameLen) + return fmt.Errorf("environment variable name %q exceeds %d characters", name, maxEnvNameLen) case !envNamePattern.MatchString(name): - return "", "", fmt.Errorf( + return fmt.Errorf( "environment variable name %q must be POSIX-style: letters, digits and underscore, not starting with a digit", name, ) case utf8.RuneCountInString(value) > maxEnvValueLen: - return "", "", fmt.Errorf("value for %q exceeds %d characters", name, maxEnvValueLen) + return fmt.Errorf("value for %q exceeds %d characters", name, maxEnvValueLen) } - return name, value, nil + return nil } diff --git a/internal/cmd/serverless/env_vars_test.go b/internal/cmd/serverless/env_vars_test.go index dd3e86b..c0ffdbf 100644 --- a/internal/cmd/serverless/env_vars_test.go +++ b/internal/cmd/serverless/env_vars_test.go @@ -18,7 +18,7 @@ import ( ) func TestBuildEnvironmentVariables(t *testing.T) { - got, err := buildEnvironmentVariables(nil, []string{"HF_TOKEN=abc", "LOG_LEVEL=debug"}) + got, err := buildEnvironmentVariables(nil, []string{"HF_TOKEN=abc", "LOG_LEVEL=debug"}, nil) if err != nil { t.Fatalf("buildEnvironmentVariables: %v", err) } @@ -30,7 +30,7 @@ func TestBuildEnvironmentVariables(t *testing.T) { // Nothing supplied must send nil, not an empty map: the field is optional and an // explicit {} is a different statement from saying nothing. func TestBuildEnvironmentVariables_NoneIsNil(t *testing.T) { - got, err := buildEnvironmentVariables(nil, nil) + got, err := buildEnvironmentVariables(nil, nil, nil) if err != nil { t.Fatalf("buildEnvironmentVariables: %v", err) } @@ -42,7 +42,7 @@ func TestBuildEnvironmentVariables_NoneIsNil(t *testing.T) { // A value may legitimately contain '=' -- base64 padding, a connection string -- // so only the first separator splits. func TestBuildEnvironmentVariables_ValueMayContainEquals(t *testing.T) { - got, err := buildEnvironmentVariables(nil, []string{"TOKEN=abc==def="}) + got, err := buildEnvironmentVariables(nil, []string{"TOKEN=abc==def="}, nil) if err != nil { t.Fatalf("buildEnvironmentVariables: %v", err) } @@ -54,7 +54,7 @@ func TestBuildEnvironmentVariables_ValueMayContainEquals(t *testing.T) { // An empty value is a legitimate assignment: unsetting by setting empty is how // callers override a default the image bakes in. func TestBuildEnvironmentVariables_EmptyValueAllowed(t *testing.T) { - got, err := buildEnvironmentVariables(nil, []string{"QUIET="}) + got, err := buildEnvironmentVariables(nil, []string{"QUIET="}, nil) if err != nil { t.Fatalf("buildEnvironmentVariables: %v", err) } @@ -77,7 +77,7 @@ func TestBuildEnvironmentVariables_Rejects(t *testing.T) { } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - _, err := buildEnvironmentVariables(nil, []string{tc.pair}) + _, err := buildEnvironmentVariables(nil, []string{tc.pair}, nil) if err == nil { t.Fatalf("expected an error for %q", tc.pair) } @@ -89,7 +89,7 @@ func TestBuildEnvironmentVariables_Rejects(t *testing.T) { } func TestBuildEnvironmentVariables_TooLongValue(t *testing.T) { - _, err := buildEnvironmentVariables(nil, []string{"BIG=" + strings.Repeat("x", maxEnvValueLen+1)}) + _, err := buildEnvironmentVariables(nil, []string{"BIG=" + strings.Repeat("x", maxEnvValueLen+1)}, nil) if err == nil || !strings.Contains(err.Error(), "exceeds") { t.Fatalf("expected a length error, got %v", err) } @@ -105,7 +105,7 @@ func TestBuildEnvironmentVariables_FromFile(t *testing.T) { t.Fatal(err) } - got, err := buildEnvironmentVariables([]string{path}, nil) + got, err := buildEnvironmentVariables([]string{path}, nil, nil) if err != nil { t.Fatalf("buildEnvironmentVariables: %v", err) } @@ -129,7 +129,7 @@ func TestBuildEnvironmentVariables_InlineOverridesFile(t *testing.T) { t.Fatal(err) } - got, err := buildEnvironmentVariables([]string{path}, []string{"LOG_LEVEL=debug"}) + got, err := buildEnvironmentVariables([]string{path}, []string{"LOG_LEVEL=debug"}, nil) if err != nil { t.Fatalf("buildEnvironmentVariables: %v", err) } @@ -146,7 +146,7 @@ func TestBuildEnvironmentVariables_FileErrorNamesTheLine(t *testing.T) { t.Fatal(err) } - _, err := buildEnvironmentVariables([]string{path}, nil) + _, err := buildEnvironmentVariables([]string{path}, nil, nil) if err == nil { t.Fatal("expected an error for a malformed line") } @@ -156,7 +156,7 @@ func TestBuildEnvironmentVariables_FileErrorNamesTheLine(t *testing.T) { } func TestBuildEnvironmentVariables_MissingFile(t *testing.T) { - _, err := buildEnvironmentVariables([]string{filepath.Join(t.TempDir(), "nope")}, nil) + _, err := buildEnvironmentVariables([]string{filepath.Join(t.TempDir(), "nope")}, nil, nil) if err == nil { t.Fatal("expected an error for a missing env file") } @@ -167,7 +167,7 @@ func TestBuildEnvironmentVariables_TooMany(t *testing.T) { for i := range pairs { pairs[i] = "VAR_" + strings.Repeat("a", i%20) + string(rune('A'+i%26)) + string(rune('A'+i/26)) + "=x" } - if _, err := buildEnvironmentVariables(nil, pairs); err == nil { + if _, err := buildEnvironmentVariables(nil, pairs, nil); err == nil { t.Fatal("expected an error past the variable limit") } } @@ -197,7 +197,7 @@ func TestBuildEnvironmentVariables_StripsSurroundingQuotes(t *testing.T) { if err := os.WriteFile(path, []byte(tc.line+"\n"), 0o600); err != nil { t.Fatal(err) } - got, err := buildEnvironmentVariables([]string{path}, nil) + got, err := buildEnvironmentVariables([]string{path}, nil, nil) if err != nil { t.Fatalf("buildEnvironmentVariables: %v", err) } @@ -220,7 +220,7 @@ func TestBuildEnvironmentVariables_PreservesValueWhitespace(t *testing.T) { t.Fatal(err) } - got, err := buildEnvironmentVariables([]string{path}, nil) + got, err := buildEnvironmentVariables([]string{path}, nil, nil) if err != nil { t.Fatalf("buildEnvironmentVariables: %v", err) } @@ -237,7 +237,7 @@ func TestBuildEnvironmentVariables_PreservesValueWhitespace(t *testing.T) { func TestBuildEnvironmentVariables_LimitIsCountedInRunes(t *testing.T) { // 4096 two-byte runes: 4096 characters, 8192 bytes. value := strings.Repeat("é", maxEnvValueLen) - got, err := buildEnvironmentVariables(nil, []string{"MSG=" + value}) + got, err := buildEnvironmentVariables(nil, []string{"MSG=" + value}, nil) if err != nil { t.Fatalf("a value of exactly the character limit was rejected: %v", err) } @@ -245,7 +245,7 @@ func TestBuildEnvironmentVariables_LimitIsCountedInRunes(t *testing.T) { t.Error("value did not survive") } // One rune past is still rejected. - if _, err := buildEnvironmentVariables(nil, []string{"MSG=" + value + "é"}); err == nil { + if _, err := buildEnvironmentVariables(nil, []string{"MSG=" + value + "é"}, nil); err == nil { t.Error("expected a rejection one character past the limit") } } @@ -363,3 +363,43 @@ func derefEnv(in map[string]*string) map[string]string { } return out } + +// A manifest's values were decoded by the YAML parser, never typed as +// KEY=VALUE. Running them back through that parser unquotes a value whose +// quotes are part of it, and the app receives something it never asked for. +func TestBuildEnvironmentVariablesKeepsDecodedValuesIntact(t *testing.T) { + quoted := `"hello"` + + got, err := buildEnvironmentVariables(nil, nil, map[string]string{ + "PAYLOAD": quoted, + "EMPTY": `''`, + }) + if err != nil { + t.Fatalf("buildEnvironmentVariables: %v", err) + } + if (*got)["PAYLOAD"] != quoted { + t.Errorf("PAYLOAD = %q, want %q", (*got)["PAYLOAD"], quoted) + } + if (*got)["EMPTY"] != `''` { + t.Errorf("EMPTY = %q, want two quote characters", (*got)["EMPTY"]) + } + + // The same value typed on the command line is unquoted, which is what the + // flag has always promised and what the map path exists to avoid. + viaFlag, err := buildEnvironmentVariables(nil, []string{"PAYLOAD=" + quoted}, nil) + if err != nil { + t.Fatalf("buildEnvironmentVariables: %v", err) + } + if (*viaFlag)["PAYLOAD"] != "hello" { + t.Errorf("the flag path changed: %q", (*viaFlag)["PAYLOAD"]) + } +} + +func TestBuildEnvironmentVariablesValidatesDecodedNames(t *testing.T) { + if _, err := buildEnvironmentVariables(nil, nil, map[string]string{"2BAD": "x"}); err == nil { + t.Error("a name that is not POSIX-style was accepted") + } + if _, err := buildEnvironmentVariables(nil, nil, map[string]string{"OK": strings.Repeat("x", maxEnvValueLen+1)}); err == nil { + t.Error("an oversized value was accepted") + } +} diff --git a/internal/cmd/serverless/from.go b/internal/cmd/serverless/from.go index cb5b81e..f800861 100644 --- a/internal/cmd/serverless/from.go +++ b/internal/cmd/serverless/from.go @@ -22,6 +22,12 @@ const fromFetchTimeout = 60 * time.Second // send and a directory each to create. const maxArchiveEntries = 2000 +// maxArchiveDepth bounds how deep an entry may sit. The entry cap alone does +// not bound what unpacking creates: 2000 entries under distinct chains of a +// thousand parents each are a handful of kilobytes to send and two million +// directories to make. +const maxArchiveDepth = 32 + // fetchSourceArchive downloads a packaged source and unpacks it into a new // directory, returning that directory and a function that removes it. // @@ -200,8 +206,16 @@ func safeArchivePath(name string) (string, error) { } clean := filepath.Clean(filepath.FromSlash(name)) - if clean == ".." || strings.HasPrefix(clean, ".."+string(filepath.Separator)) { + + // IsLocal rather than a prefix test: on Windows a drive-relative entry such + // as `C:../../../escaped.py` is not absolute and does not begin with "..", + // so it passes a hand-written check and then climbs out of the directory + // once joined. It also refuses the reserved device names. + if !filepath.IsLocal(clean) { return "", fmt.Errorf("%q in the fetched archive points outside it", name) } + if depth := len(strings.Split(filepath.ToSlash(clean), "/")); depth > maxArchiveDepth { + return "", fmt.Errorf("%q in the fetched archive is nested deeper than %d directories", name, maxArchiveDepth) + } return clean, nil } diff --git a/internal/cmd/serverless/from_test.go b/internal/cmd/serverless/from_test.go index 4292b97..96cb6d4 100644 --- a/internal/cmd/serverless/from_test.go +++ b/internal/cmd/serverless/from_test.go @@ -310,3 +310,32 @@ func TestFetchSourceArchiveKeepsTheExecutableBit(t *testing.T) { t.Errorf("notes.txt came out %v, executable when the archive said otherwise", plain.Mode().Perm()) } } + +// The entry cap bounds files, not the directories unpacking creates. A few +// thousand entries under long chains of parents are kilobytes to send and +// millions of directories to make. +func TestFetchSourceArchiveRefusesDeepNesting(t *testing.T) { + deep := strings.Repeat("a/", maxArchiveDepth) + "model.py" + archive := buildTestArchive(t, []archiveEntry{ + {name: manifestFile, body: codeManifest}, + {name: deep, body: testPySource}, + }) + + _, cleanup, err := fetchSourceArchive(context.Background(), serveArchive(t, archive)) + if cleanup != nil { + cleanup() + } + if err == nil { + t.Fatal("a deeply nested entry was accepted") + } + if !strings.Contains(err.Error(), "nested deeper than") { + t.Errorf("error = %v", err) + } +} + +func TestSafeArchivePathAllowsOrdinaryNesting(t *testing.T) { + ok := strings.Repeat("a/", maxArchiveDepth-1) + "model.py" + if _, err := safeArchivePath(ok); err != nil { + t.Errorf("a path at the limit was refused: %v", err) + } +} diff --git a/internal/cmd/serverless/manifest.go b/internal/cmd/serverless/manifest.go index 8a910f0..060e8ff 100644 --- a/internal/cmd/serverless/manifest.go +++ b/internal/cmd/serverless/manifest.go @@ -3,11 +3,9 @@ package serverless import ( "bytes" "fmt" - "maps" "os" "path/filepath" "regexp" - "slices" "strings" serverlessapi "github.com/runware/runware-cli/internal/api/serverless" @@ -199,16 +197,6 @@ func (m deployManifest) localSource(dir string) (args []string, srcDir, containe return []string{m.Source.Entry}, dir, "" } -// envVarFlags renders the environment variables the way --env-var takes them, -// in a fixed order so two deploys of the same manifest behave the same. -func (m deployManifest) envVarFlags() []string { - out := make([]string, 0, len(m.EnvVars)) - for _, key := range slices.Sorted(maps.Keys(m.EnvVars)) { - out = append(out, key+"="+m.EnvVars[key]) - } - return out -} - // secretFlags renders the secrets the way --secret takes them. func (m deployManifest) secretFlags() []string { out := make([]string, 0, len(m.Secrets)) diff --git a/internal/cmd/serverless/manifest_test.go b/internal/cmd/serverless/manifest_test.go index 653f07a..e99aa25 100644 --- a/internal/cmd/serverless/manifest_test.go +++ b/internal/cmd/serverless/manifest_test.go @@ -99,9 +99,10 @@ compute: } } -// envVarFlags has to be ordered: an unordered map would make two deploys of -// one manifest differ in the order they report and send variables. -func TestManifestEnvVarFlagsAreSorted(t *testing.T) { +// The decoded values go to the deploy as they are. Rendering them as +// KEY=VALUE and parsing them back would unquote a value whose quotes are part +// of it, so the map is what travels. +func TestManifestEnvVarsAreDecodedValues(t *testing.T) { manifest := readManifestBody(t, `configVersion: 1 id: config-and-secrets source: @@ -110,13 +111,17 @@ source: envVars: LOG_LEVEL: info CACHE_DIR: /weights - BATCH: "8" + PAYLOAD: '"hello"' `) - got := strings.Join(manifest.envVarFlags(), " ") - want := "BATCH=8 CACHE_DIR=/weights LOG_LEVEL=info" - if got != want { - t.Errorf("envVarFlags = %q, want %q", got, want) + for key, want := range map[string]string{ + "LOG_LEVEL": "info", + "CACHE_DIR": "/weights", + "PAYLOAD": `"hello"`, + } { + if manifest.EnvVars[key] != want { + t.Errorf("envVars[%s] = %q, want %q", key, manifest.EnvVars[key], want) + } } } From 2e3702f629f4612b24c7782f12f33ea136638eca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A9lix=20Sanz?= Date: Wed, 4 Nov 2026 18:32:49 +0100 Subject: [PATCH 06/13] fix(serverless): validate a manifest's secrets and entry path as fields, not as text --- internal/cmd/serverless/manifest.go | 37 +++++++++++-- internal/cmd/serverless/manifest_test.go | 69 ++++++++++++++++++++++++ 2 files changed, 102 insertions(+), 4 deletions(-) diff --git a/internal/cmd/serverless/manifest.go b/internal/cmd/serverless/manifest.go index 060e8ff..3864494 100644 --- a/internal/cmd/serverless/manifest.go +++ b/internal/cmd/serverless/manifest.go @@ -2,7 +2,9 @@ package serverless import ( "bytes" + "errors" "fmt" + "io" "os" "path/filepath" "regexp" @@ -74,6 +76,20 @@ type manifestSecret struct { EnvVarName string `yaml:"envVarName"` } +// validate checks the two fields as the two fields they are. Rendering them +// into `--secret` and parsing that back would read a name holding an `=` as a +// name and an alias, so a manifest naming one secret would attach a different +// one under a variable nobody wrote. +func (s manifestSecret) validate() error { + if s.Name == "" || !envNamePattern.MatchString(s.Name) { + return fmt.Errorf("secret name %q must be POSIX-style: letters, digits and underscore, not starting with a digit", s.Name) + } + if s.EnvVarName != "" && !envNamePattern.MatchString(s.EnvVarName) { + return fmt.Errorf("secret %s names the environment variable %q, which must be POSIX-style", s.Name, s.EnvVarName) + } + return nil +} + // flag renders the secret the way --secret takes it. func (s manifestSecret) flag() string { if s.EnvVarName == "" { @@ -103,9 +119,19 @@ func readDeployManifest(dir string) (deployManifest, error) { var manifest deployManifest if err := decoder.Decode(&manifest); err != nil { + if errors.Is(err, io.EOF) { + return deployManifest{}, fmt.Errorf("%s is empty", manifestFile) + } return deployManifest{}, fmt.Errorf("read %s: %w", manifestFile, err) } + // A file holding more than one document would otherwise be read down to its + // first `---` and the rest dropped without a word, which is the silent + // omission KnownFields exists to prevent one key at a time. + if err := decoder.Decode(new(deployManifest)); !errors.Is(err, io.EOF) { + return deployManifest{}, fmt.Errorf("%s holds more than one document, and only the first would be deployed", manifestFile) + } + if err := manifest.validate(); err != nil { return deployManifest{}, fmt.Errorf("%s: %w", manifestFile, err) } @@ -151,8 +177,8 @@ func (m deployManifest) validate() error { } for _, secret := range m.Secrets { - if _, _, err := splitSecretAttach(secret.flag()); err != nil { - return fmt.Errorf("secret %q is not a name, or a name and an environment variable", secret.flag()) + if err := secret.validate(); err != nil { + return err } } if m.Compute.GpusPerWorker != nil { @@ -180,8 +206,11 @@ func validateManifestEntry(entry string) error { if filepath.IsAbs(entry) || strings.HasPrefix(entry, "/") { return fmt.Errorf("source.entry %q must be relative to the source root", entry) } - clean := filepath.ToSlash(filepath.Clean(filepath.FromSlash(entry))) - if clean == ".." || strings.HasPrefix(clean, "../") { + // The same test safeArchivePath uses, because this is the same class of + // path: one an archive we did not write chose. A hand-written prefix check + // misses a Windows drive-relative name, which is neither absolute nor + // prefixed with "..", and the two guards must not disagree about that. + if !filepath.IsLocal(filepath.Clean(filepath.FromSlash(entry))) { return fmt.Errorf("source.entry %q must stay inside the source root", entry) } return nil diff --git a/internal/cmd/serverless/manifest_test.go b/internal/cmd/serverless/manifest_test.go index e99aa25..d56010b 100644 --- a/internal/cmd/serverless/manifest_test.go +++ b/internal/cmd/serverless/manifest_test.go @@ -303,3 +303,72 @@ func TestReadDeployManifestRejectsBadAppID(t *testing.T) { } } } + +// A secret is two fields. Validating it by rendering `--secret` and parsing +// that back reads a name holding an `=` as a name and an alias, so a manifest +// naming one secret would attach a different one under a variable nobody wrote. +func TestReadDeployManifestRejectsASecretNameHoldingAnEquals(t *testing.T) { + msg := manifestError(t, `configVersion: 1 +id: echo-app +source: + type: code + entry: model.py +secrets: + - name: A_NAME=OTHER_VAR +`) + if !strings.Contains(msg, "secret name") { + t.Errorf("error = %s", msg) + } +} + +func TestReadDeployManifestChecksTheAliasToo(t *testing.T) { + msg := manifestError(t, `configVersion: 1 +id: echo-app +source: + type: code + entry: model.py +secrets: + - name: HF_TOKEN + envVarName: not a name +`) + if !strings.Contains(msg, "POSIX-style") { + t.Errorf("error = %s", msg) + } + + manifest := readManifestBody(t, `configVersion: 1 +id: echo-app +source: + type: code + entry: model.py +secrets: + - name: HF_TOKEN + envVarName: HUGGING_FACE_HUB_TOKEN +`) + if got := manifest.secretFlags(); len(got) != 1 || got[0] != "HF_TOKEN=HUGGING_FACE_HUB_TOKEN" { + t.Errorf("secretFlags = %v", got) + } +} + +// Read down to the first `---` and the rest would be dropped without a word. +func TestReadDeployManifestRefusesASecondDocument(t *testing.T) { + msg := manifestError(t, `configVersion: 1 +id: echo-app +source: + type: code + entry: model.py +--- +id: something-else +`) + if !strings.Contains(msg, "more than one document") { + t.Errorf("error = %s", msg) + } +} + +func TestReadDeployManifestNamesAnEmptyFile(t *testing.T) { + if msg := manifestError(t, ""); !strings.Contains(msg, "is empty") { + t.Errorf("error = %s", msg) + } + if msg := manifestError(t, "\n# nothing but a comment\n"); !strings.Contains(msg, "is empty") { + t.Errorf("a comment-only file: %s", msg) + } +} From 5319a04e8b84033b28f90bda70875fdac0b72930 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A9lix=20Sanz?= Date: Thu, 5 Nov 2026 15:48:17 +0100 Subject: [PATCH 07/13] test(serverless): run deploy --from through the command, create and redeploy --- internal/cmd/serverless/apps_health_test.go | 2 +- internal/cmd/serverless/deploy_test.go | 137 +++++++++++++++++++- 2 files changed, 136 insertions(+), 3 deletions(-) diff --git a/internal/cmd/serverless/apps_health_test.go b/internal/cmd/serverless/apps_health_test.go index 087d4b8..1fcb01e 100644 --- a/internal/cmd/serverless/apps_health_test.go +++ b/internal/cmd/serverless/apps_health_test.go @@ -59,7 +59,7 @@ func TestAppsHealthFromAPI(t *testing.T) { body = `{"data":[` + body + `]}` } srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - path := "/v1/apps" + path := testAppsPath if command == appHealthShowCommand { path += "/" + testAppID } diff --git a/internal/cmd/serverless/deploy_test.go b/internal/cmd/serverless/deploy_test.go index 2a2f876..36c7695 100644 --- a/internal/cmd/serverless/deploy_test.go +++ b/internal/cmd/serverless/deploy_test.go @@ -379,7 +379,7 @@ func TestDeployCreate_SendsSecretsAndMinAvailableWorkers(t *testing.T) { w.Header().Set("Content-Type", "application/problem+json") w.WriteHeader(http.StatusNotFound) _, _ = w.Write([]byte(`{"type":"about:blank","title":"Not Found","status":404}`)) - case r.Method == http.MethodPost && r.URL.Path == "/v1/apps": + case r.Method == http.MethodPost && r.URL.Path == testAppsPath: creates++ if err := json.NewDecoder(r.Body).Decode(&created); err != nil { t.Errorf("decode create: %v", err) @@ -531,7 +531,7 @@ func TestDeployWait_TimeoutDoesNotPanic(t *testing.T) { } w.Header().Set("Content-Type", "application/json") _, _ = w.Write([]byte(strings.Replace(activeAppBody(""), `"status":"active"`, `"status":"initializing"`, 1))) - case r.Method == http.MethodPost && r.URL.Path == "/v1/apps": + case r.Method == http.MethodPost && r.URL.Path == testAppsPath: w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusCreated) _, _ = w.Write([]byte(strings.Replace(activeAppBody(""), `"status":"active"`, `"status":"initializing"`, 1))) @@ -783,3 +783,136 @@ func TestRefuseCodeOnlyFlags(t *testing.T) { }) } } + +// deployFromArchiveManifest is a packaged code source whose manifest carries a +// GPU type and a volume. Both are create-only flags, which is what makes it the +// fixture for the redeploy rule below. +const deployFromArchiveManifest = `configVersion: 1 +id: ` + testAppID + ` +source: + type: code + entry: ` + testModelFile + ` +compute: + gpuType: ` + testGPUType + ` +volumes: + - /weights +` + +// serveDeployFromArchive packages the manifest with its entry file and returns +// the URL to deploy from. +func serveDeployFromArchive(t *testing.T) string { + t.Helper() + return serveArchive(t, buildTestArchive(t, []archiveEntry{ + {name: manifestFile, body: deployFromArchiveManifest}, + {name: testModelFile, body: testPySource}, + })) +} + +func TestDeployFrom_CreatesFromTheManifest(t *testing.T) { + from := serveDeployFromArchive(t) + + var created serverlessapi.AppCreate + var creates int + serveSourceUpload := newSourceUploadStub(t) + + api := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if serveSourceUpload(w, r) { + return + } + switch { + case r.Method == http.MethodGet && strings.HasPrefix(r.URL.Path, "/v1/apps/"): + w.Header().Set("Content-Type", "application/problem+json") + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"type":"about:blank","title":"Not Found","status":404}`)) + case r.Method == http.MethodPost && r.URL.Path == testAppsPath: + creates++ + if err := json.NewDecoder(r.Body).Decode(&created); err != nil { + t.Errorf("decode create: %v", err) + return + } + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(activeAppBody(""))) + default: + t.Errorf("unexpected %s %s", r.Method, r.URL.Path) + w.WriteHeader(http.StatusNotFound) + } + })) + defer api.Close() + + t.Setenv("RUNWARE_API_KEY", "test-key") + t.Setenv("RUNWARE_SERVERLESS_BASE_URL", api.URL) + + cmd := newDeployCmd(log.New(io.Discard)) + cmd.SetOut(&bytes.Buffer{}) + cmd.SetErr(&bytes.Buffer{}) + cmd.SetArgs([]string{testFromFlag, from}) + if err := cmd.Execute(); err != nil { + t.Fatalf("deploy %s: %v", testFromFlag, err) + } + + if creates != 1 { + t.Fatalf("creates = %d, want 1", creates) + } + if created.AppId != testAppID { + t.Fatalf("appId = %q, want %q", created.AppId, testAppID) + } + if created.Configuration.GpuType != testGPUType { + t.Fatalf("gpuType = %q, want %q", created.Configuration.GpuType, testGPUType) + } + if created.Volumes == nil || len(*created.Volumes) != 1 || (*created.Volumes)[0].MountPath != "/weights" { + t.Fatalf("volumes = %#v", created.Volumes) + } +} + +// TestDeployFrom_RedeploysAnExistingApp fixes the redeploy rule: the manifest +// describes the app rather than asking to change a running one, so its +// create-only GPU type and volume must not be refused on an update. +func TestDeployFrom_RedeploysAnExistingApp(t *testing.T) { + from := serveDeployFromArchive(t) + + var creates, updates int + serveSourceUpload := newSourceUploadStub(t) + + api := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if serveSourceUpload(w, r) { + return + } + switch { + case r.Method == http.MethodGet && strings.HasPrefix(r.URL.Path, "/v1/apps/"): + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(activeAppBody(""))) + case r.Method == http.MethodPatch && r.URL.Path == testAppsPath+"/"+testAppID: + updates++ + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(activeAppBody(""))) + case r.Method == http.MethodPost && r.URL.Path == testAppsPath: + creates++ + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(activeAppBody(""))) + default: + t.Errorf("unexpected %s %s", r.Method, r.URL.Path) + w.WriteHeader(http.StatusNotFound) + } + })) + defer api.Close() + + t.Setenv("RUNWARE_API_KEY", "test-key") + t.Setenv("RUNWARE_SERVERLESS_BASE_URL", api.URL) + + cmd := newDeployCmd(log.New(io.Discard)) + cmd.SetOut(&bytes.Buffer{}) + cmd.SetErr(&bytes.Buffer{}) + cmd.SetArgs([]string{testFromFlag, from}) + if err := cmd.Execute(); err != nil { + t.Fatalf("redeploy %s: %v", testFromFlag, err) + } + + if updates != 1 { + t.Fatalf("updates = %d, want 1", updates) + } + if creates != 0 { + t.Fatalf("creates = %d, want 0", creates) + } +} From 098f345796e1df62570749c8a9be2f677d052a62 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A9lix=20Sanz?= Date: Thu, 5 Nov 2026 15:57:12 +0100 Subject: [PATCH 08/13] fix(serverless): refuse an https downgrade and a duplicate path in a fetched archive --- internal/cmd/serverless/from.go | 37 ++++++++++++++++- internal/cmd/serverless/from_test.go | 60 ++++++++++++++++++++++++++++ 2 files changed, 96 insertions(+), 1 deletion(-) diff --git a/internal/cmd/serverless/from.go b/internal/cmd/serverless/from.go index f800861..f5b480a 100644 --- a/internal/cmd/serverless/from.go +++ b/internal/cmd/serverless/from.go @@ -55,6 +55,26 @@ func fetchSourceArchive(ctx context.Context, rawURL string) (dir string, cleanup return dir, cleanup, nil } +// maxArchiveRedirects is what the default client follows, kept so refusing a +// downgrade does not also change how far a redirect chain may go. +const maxArchiveRedirects = 10 + +// archiveClient refuses a redirect that leaves https for anything else. The +// scheme check in downloadArchive sees only the URL the person typed, and a +// client that follows redirects blindly would fetch the archive in the clear +// from a host the person never named. +var archiveClient = &http.Client{ + CheckRedirect: func(req *http.Request, via []*http.Request) error { + if len(via) >= maxArchiveRedirects { + return fmt.Errorf("stopped after %d redirects", maxArchiveRedirects) + } + if from := via[len(via)-1].URL.Scheme; from == "https" && req.URL.Scheme != "https" { + return fmt.Errorf("the source redirected from https to %s, which would fetch the archive unencrypted", req.URL.Scheme) + } + return nil + }, +} + // downloadArchive reads the URL into memory, refusing anything larger than the // platform would accept as a source upload anyway. func downloadArchive(ctx context.Context, rawURL string) ([]byte, error) { @@ -73,7 +93,7 @@ func downloadArchive(ctx context.Context, rawURL string) ([]byte, error) { if err != nil { return nil, fmt.Errorf("fetch %s: %w", rawURL, err) } - resp, err := http.DefaultClient.Do(req) + resp, err := archiveClient.Do(req) if err != nil { return nil, fmt.Errorf("fetch %s: %w", rawURL, err) } @@ -112,7 +132,22 @@ func unpackArchive(archive []byte, dir string) error { } var total int64 + // Paths already written. unpackEntry opens each file with O_TRUNC and keeps + // no record, so without this the second of two entries naming one path + // silently replaces the first. + seen := make(map[string]struct{}, len(reader.File)) for _, entry := range reader.File { + rel, err := safeArchivePath(entry.Name) + if err != nil { + return err + } + if !entry.FileInfo().IsDir() { + if _, dup := seen[rel]; dup { + return fmt.Errorf("the fetched archive writes %s twice", rel) + } + seen[rel] = struct{}{} + } + written, err := unpackEntry(entry, dir) if err != nil { return err diff --git a/internal/cmd/serverless/from_test.go b/internal/cmd/serverless/from_test.go index 96cb6d4..ebaedfe 100644 --- a/internal/cmd/serverless/from_test.go +++ b/internal/cmd/serverless/from_test.go @@ -10,6 +10,7 @@ import ( "os" "path/filepath" "runtime" + "slices" "strings" "testing" ) @@ -339,3 +340,62 @@ func TestSafeArchivePathAllowsOrdinaryNesting(t *testing.T) { t.Errorf("a path at the limit was refused: %v", err) } } + +// A TLS httptest server signs with a CA the client does not trust, so the rule +// is tested where it lives rather than through a redirect the fetch cannot make. +func TestArchiveClientRefusesAnHTTPSDowngrade(t *testing.T) { + chain := func(schemes ...string) []*http.Request { + var via []*http.Request + for _, scheme := range schemes { + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, scheme+"://example.invalid/echo.zip", nil) + if err != nil { + t.Fatal(err) + } + via = append(via, req) + } + return via + } + + cases := []struct { + name string + to string + via []*http.Request + wantErr string + }{ + {name: "https to http", to: "http", via: chain("https"), wantErr: "unencrypted"}, + {name: "https to https", to: "https", via: chain("https")}, + {name: "http to http", to: "http", via: chain("http")}, + {name: "too many", to: "https", via: chain(slices.Repeat([]string{"https"}, maxArchiveRedirects)...), wantErr: "redirects"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, tc.to+"://elsewhere.invalid/echo.zip", nil) + if err != nil { + t.Fatal(err) + } + err = archiveClient.CheckRedirect(req, tc.via) + if tc.wantErr == "" { + if err != nil { + t.Fatalf("allowed redirect refused: %v", err) + } + return + } + if err == nil || !strings.Contains(err.Error(), tc.wantErr) { + t.Fatalf("err = %v, want one containing %q", err, tc.wantErr) + } + }) + } +} + +func TestFetchSourceArchiveRefusesTwoEntriesOnOnePath(t *testing.T) { + archive := buildTestArchive(t, []archiveEntry{ + {name: manifestFile, body: codeManifest}, + {name: testEntryFile, body: testPySource}, + {name: "./" + testEntryFile, body: "x = 2\n"}, + }) + + _, _, err := fetchSourceArchive(context.Background(), serveArchive(t, archive)) + if err == nil || !strings.Contains(err.Error(), "twice") { + t.Fatalf("expected a duplicate-path refusal, got %v", err) + } +} From ad63230adfb78cd7560a5a0effc7f22664a12aba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A9lix=20Sanz?= Date: Thu, 5 Nov 2026 15:57:12 +0100 Subject: [PATCH 09/13] feat(serverless): name the manifest sections a redeploy does not apply --- internal/cmd/serverless/deploy.go | 30 ++++++++++++++++++++++++ internal/cmd/serverless/deploy_test.go | 32 +++++++++++++++++++++++++- 2 files changed, 61 insertions(+), 1 deletion(-) diff --git a/internal/cmd/serverless/deploy.go b/internal/cmd/serverless/deploy.go index 6c53866..776b5bd 100644 --- a/internal/cmd/serverless/deploy.go +++ b/internal/cmd/serverless/deploy.go @@ -452,6 +452,11 @@ paths and an invoke example once the application is active.`, var app *serverlessapi.App if update { + if sections := unappliedManifestSections(from, manifest); len(sections) > 0 { + _, _ = fmt.Fprintf(cmd.ErrOrStderr(), + "Updating the source only. An existing app keeps its configuration, so the manifest sections %s do not apply here. Change configuration with 'runware serverless apps scale' or 'apps env-vars'.\n", + strings.Join(sections, ", ")) + } spin = cmdutil.NewSpinner(fmt.Sprintf("Updating application %s...", id)) spin.Start() app, err = client.UpdateApp(cmd.Context(), id, serverlessapi.AppUpdate{ @@ -663,6 +668,31 @@ func validateUpdateDeployFlags(cmd *cobra.Command) error { return nil } +// unappliedManifestSections names the manifest sections an update does not +// send. The update carries only the source, so a packaged version that adds a +// volume or a secret would otherwise deploy its new code without them and say +// nothing. Empty for a deploy that read no manifest. +func unappliedManifestSections(from string, manifest deployManifest) []string { + if from == "" { + return nil + } + + var sections []string + if len(manifest.Volumes) > 0 { + sections = append(sections, "volumes") + } + if len(manifest.EnvVars) > 0 { + sections = append(sections, "envVars") + } + if len(manifest.Secrets) > 0 { + sections = append(sections, "secrets") + } + if manifest.Compute != (manifestCompute{}) { + sections = append(sections, "compute") + } + return sections +} + // createOnlyDeployHint says how to change a create-only setting on the copy // in environment, under defaultEnvironment. func createOnlyDeployHint(name, environment, defaultEnvironment string) string { diff --git a/internal/cmd/serverless/deploy_test.go b/internal/cmd/serverless/deploy_test.go index 36c7695..667bc75 100644 --- a/internal/cmd/serverless/deploy_test.go +++ b/internal/cmd/serverless/deploy_test.go @@ -8,6 +8,7 @@ import ( "log/slog" "net/http" "net/http/httptest" + "slices" "strings" "testing" "time" @@ -901,14 +902,23 @@ func TestDeployFrom_RedeploysAnExistingApp(t *testing.T) { t.Setenv("RUNWARE_API_KEY", "test-key") t.Setenv("RUNWARE_SERVERLESS_BASE_URL", api.URL) + var stderr bytes.Buffer cmd := newDeployCmd(log.New(io.Discard)) cmd.SetOut(&bytes.Buffer{}) - cmd.SetErr(&bytes.Buffer{}) + cmd.SetErr(&stderr) cmd.SetArgs([]string{testFromFlag, from}) if err := cmd.Execute(); err != nil { t.Fatalf("redeploy %s: %v", testFromFlag, err) } + // The manifest carries a volume and a compute block, and an update sends + // neither, so the command has to say which sections it dropped. + for _, want := range []string{"volumes", "compute", "the source only"} { + if !strings.Contains(stderr.String(), want) { + t.Fatalf("stderr = %q, want it to mention %q", stderr.String(), want) + } + } + if updates != 1 { t.Fatalf("updates = %d, want 1", updates) } @@ -916,3 +926,23 @@ func TestDeployFrom_RedeploysAnExistingApp(t *testing.T) { t.Fatalf("creates = %d, want 0", creates) } } + +func TestUnappliedManifestSections(t *testing.T) { + full := deployManifest{ + Volumes: []string{"/weights"}, + EnvVars: map[string]string{"A": "1"}, + Secrets: []manifestSecret{{Name: "HF_TOKEN"}}, + Compute: manifestCompute{GpuType: testGPUType}, + } + + if got := unappliedManifestSections("", full); got != nil { + t.Fatalf("without --from: %v, want nil", got) + } + want := []string{"volumes", "envVars", "secrets", "compute"} + if got := unappliedManifestSections(testArchiveURL, full); !slices.Equal(got, want) { + t.Fatalf("full manifest: %v, want %v", got, want) + } + if got := unappliedManifestSections(testArchiveURL, deployManifest{}); got != nil { + t.Fatalf("empty manifest: %v, want nil", got) + } +} From 9830895f000911f67cf2068412208679d5d1e653 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A9lix=20Sanz?= Date: Thu, 5 Nov 2026 16:01:52 +0100 Subject: [PATCH 10/13] fix(serverless): let one --env-var override one manifest key instead of all of them --- docs/runware_serverless_deploy.md | 6 +- internal/cmd/serverless/deploy.go | 21 +++--- internal/cmd/serverless/deploy_test.go | 81 +++++++++++++++++++++++- internal/cmd/serverless/manifest_test.go | 2 +- 4 files changed, 97 insertions(+), 13 deletions(-) diff --git a/docs/runware_serverless_deploy.md b/docs/runware_serverless_deploy.md index 0d04244..b796012 100644 --- a/docs/runware_serverless_deploy.md +++ b/docs/runware_serverless_deploy.md @@ -47,8 +47,10 @@ files and a runware.yaml recording the deploy options. The CLI fetches it, reads that file, and deploys what it describes, so a published example deploys in one command instead of a clone and a command. Flags still win over the file, so the same archive goes to another GPU type or another --id without -being edited first. The archive supplies the source directory, so --from -cannot be combined with an entry file, --container, or --src-dir. +being edited first. An --env-var or --env-file sets one key over the +manifest's and leaves the rest, while --volume, --requirement and --secret +replace the manifest's whole list. The archive supplies the source directory, +so --from cannot be combined with an entry file, --container, or --src-dir. A runware.yaml carries no secret values. Its secrets name what the app needs, and each one has to already exist in the environment. A manifest key this CLI diff --git a/internal/cmd/serverless/deploy.go b/internal/cmd/serverless/deploy.go index 776b5bd..673aeb7 100644 --- a/internal/cmd/serverless/deploy.go +++ b/internal/cmd/serverless/deploy.go @@ -220,8 +220,10 @@ files and a runware.yaml recording the deploy options. The CLI fetches it, reads that file, and deploys what it describes, so a published example deploys in one command instead of a clone and a command. Flags still win over the file, so the same archive goes to another GPU type or another --id without -being edited first. The archive supplies the source directory, so --from -cannot be combined with an entry file, --container, or --src-dir. +being edited first. An --env-var or --env-file sets one key over the +manifest's and leaves the rest, while --volume, --requirement and --secret +replace the manifest's whole list. The archive supplies the source directory, +so --from cannot be combined with an entry file, --container, or --src-dir. A runware.yaml carries no secret values. Its secrets name what the app needs, and each one has to already exist in the environment. A manifest key this CLI @@ -361,12 +363,15 @@ paths and an invoke example once the application is active.`, if !cmd.Flags().Changed("volume") { volumes = manifest.Volumes } - if !cmd.Flags().Changed(envVarFlag) { - // The map itself, not rendered as KEY=VALUE: a value that - // legitimately carries quotes would be unquoted a second - // time on the way back in. - manifestEnv = manifest.EnvVars - } + // The map itself, not rendered as KEY=VALUE: a value that + // legitimately carries quotes would be unquoted a second time + // on the way back in. + // + // Passed whether or not --env-var was, because + // buildEnvironmentVariables reads it before --env-file and + // --env-var. One flag then overrides one key rather than + // dropping the whole set a template needs to run. + manifestEnv = manifest.EnvVars if !cmd.Flags().Changed("secret") { secrets = manifest.secretFlags() } diff --git a/internal/cmd/serverless/deploy_test.go b/internal/cmd/serverless/deploy_test.go index 667bc75..d0cafd5 100644 --- a/internal/cmd/serverless/deploy_test.go +++ b/internal/cmd/serverless/deploy_test.go @@ -6,6 +6,7 @@ import ( "encoding/json" "io" "log/slog" + "maps" "net/http" "net/http/httptest" "slices" @@ -31,6 +32,7 @@ const ( testRequirement = "--requirement" testArchiveURL = "https://runware.ai/docs/serverless/examples/echo.zip" testIDFlag = "--id" + testVolumePath = "/weights" testWaitFlag = "--wait" testTimeoutFlag = "--timeout" ) @@ -861,7 +863,7 @@ func TestDeployFrom_CreatesFromTheManifest(t *testing.T) { if created.Configuration.GpuType != testGPUType { t.Fatalf("gpuType = %q, want %q", created.Configuration.GpuType, testGPUType) } - if created.Volumes == nil || len(*created.Volumes) != 1 || (*created.Volumes)[0].MountPath != "/weights" { + if created.Volumes == nil || len(*created.Volumes) != 1 || (*created.Volumes)[0].MountPath != testVolumePath { t.Fatalf("volumes = %#v", created.Volumes) } } @@ -929,7 +931,7 @@ func TestDeployFrom_RedeploysAnExistingApp(t *testing.T) { func TestUnappliedManifestSections(t *testing.T) { full := deployManifest{ - Volumes: []string{"/weights"}, + Volumes: []string{testVolumePath}, EnvVars: map[string]string{"A": "1"}, Secrets: []manifestSecret{{Name: "HF_TOKEN"}}, Compute: manifestCompute{GpuType: testGPUType}, @@ -946,3 +948,78 @@ func TestUnappliedManifestSections(t *testing.T) { t.Fatalf("empty manifest: %v, want nil", got) } } + +// deployFromEnvManifest carries two environment variables, so a single +// --env-var can be seen to override one of them and keep the other. +const deployFromEnvManifest = `configVersion: 1 +id: ` + testAppID + ` +source: + type: code + entry: ` + testModelFile + ` +compute: + gpuType: ` + testGPUType + ` +envVars: + MODEL_REVISION: main + CACHE_DIR: /weights +` + +func TestDeployFrom_EnvVarOverridesOneManifestKey(t *testing.T) { + from := serveArchive(t, buildTestArchive(t, []archiveEntry{ + {name: manifestFile, body: deployFromEnvManifest}, + {name: testModelFile, body: testPySource}, + })) + + var created serverlessapi.AppCreate + serveSourceUpload := newSourceUploadStub(t) + + api := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if serveSourceUpload(w, r) { + return + } + switch { + case r.Method == http.MethodGet && strings.HasPrefix(r.URL.Path, "/v1/apps/"): + w.Header().Set("Content-Type", "application/problem+json") + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"type":"about:blank","title":"Not Found","status":404}`)) + case r.Method == http.MethodPost && r.URL.Path == testAppsPath: + if err := json.NewDecoder(r.Body).Decode(&created); err != nil { + t.Errorf("decode create: %v", err) + return + } + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(activeAppBody(""))) + default: + t.Errorf("unexpected %s %s", r.Method, r.URL.Path) + w.WriteHeader(http.StatusNotFound) + } + })) + defer api.Close() + + t.Setenv("RUNWARE_API_KEY", "test-key") + t.Setenv("RUNWARE_SERVERLESS_BASE_URL", api.URL) + + cmd := newDeployCmd(log.New(io.Discard)) + cmd.SetOut(&bytes.Buffer{}) + cmd.SetErr(&bytes.Buffer{}) + cmd.SetArgs([]string{ + testFromFlag, from, + "--" + envVarFlag, "MODEL_REVISION=abc123", + "--" + envVarFlag, "EXTRA=1", + }) + if err := cmd.Execute(); err != nil { + t.Fatalf("deploy %s: %v", testFromFlag, err) + } + + if created.EnvironmentVariables == nil { + t.Fatal("environmentVariables = nil") + } + want := map[string]string{ + "MODEL_REVISION": "abc123", + "CACHE_DIR": testVolumePath, + "EXTRA": "1", + } + if got := *created.EnvironmentVariables; !maps.Equal(got, want) { + t.Fatalf("environmentVariables = %v, want %v", got, want) + } +} diff --git a/internal/cmd/serverless/manifest_test.go b/internal/cmd/serverless/manifest_test.go index d56010b..1d08d83 100644 --- a/internal/cmd/serverless/manifest_test.go +++ b/internal/cmd/serverless/manifest_test.go @@ -116,7 +116,7 @@ envVars: for key, want := range map[string]string{ "LOG_LEVEL": "info", - "CACHE_DIR": "/weights", + "CACHE_DIR": testVolumePath, "PAYLOAD": `"hello"`, } { if manifest.EnvVars[key] != want { From 05428d3f2e697eb7cc79d278f53db1ef6a493bf6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A9lix=20Sanz?= Date: Fri, 6 Nov 2026 12:33:19 +0100 Subject: [PATCH 11/13] fix: deploy --from only creates, so a template cannot take over an app --- docs/runware_serverless_deploy.md | 9 +- internal/cmd/serverless/deploy.go | 55 ++++------- internal/cmd/serverless/deploy_test.go | 121 +++++++++++++++---------- 3 files changed, 101 insertions(+), 84 deletions(-) diff --git a/docs/runware_serverless_deploy.md b/docs/runware_serverless_deploy.md index b796012..ee5bea5 100644 --- a/docs/runware_serverless_deploy.md +++ b/docs/runware_serverless_deploy.md @@ -52,6 +52,13 @@ manifest's and leaves the rest, while --volume, --requirement and --secret replace the manifest's whole list. The archive supplies the source directory, so --from cannot be combined with an entry file, --container, or --src-dir. +--from only creates. A manifest names an id, and deploying it over an +application that already has that id would roll the archive out as that +application's next version, running this code with the secrets, environment +variables and volumes it already has. So an existing id is refused: pass --id +to create a separate application, or unpack the archive and deploy the +directory to update the one that is there. + A runware.yaml carries no secret values. Its secrets name what the app needs, and each one has to already exist in the environment. A manifest key this CLI does not know is an error rather than a silent omission, which is what @@ -158,7 +165,7 @@ runware serverless deploy [file] [flags] --env-file stringArray File of KEY=VALUE lines to read environment variables from (repeatable) --env-var stringArray Environment variable as KEY=VALUE (repeatable) --fallback-gpu-type string Secondary GPU type if the preferred type is unavailable - --from string URL of a packaged source to fetch and deploy, reading its runware.yaml + --from string URL of a packaged source to fetch and deploy as a new application, reading its runware.yaml --gpu-type string GPU type ID (see 'serverless gpus'; required when creating) --gpus-per-worker int32 GPUs allocated per worker (1, 2, 4, or 8) (default 1) -h, --help help for deploy diff --git a/internal/cmd/serverless/deploy.go b/internal/cmd/serverless/deploy.go index c7bceda..e45a87d 100644 --- a/internal/cmd/serverless/deploy.go +++ b/internal/cmd/serverless/deploy.go @@ -225,6 +225,13 @@ manifest's and leaves the rest, while --volume, --requirement and --secret replace the manifest's whole list. The archive supplies the source directory, so --from cannot be combined with an entry file, --container, or --src-dir. +--from only creates. A manifest names an id, and deploying it over an +application that already has that id would roll the archive out as that +application's next version, running this code with the secrets, environment +variables and volumes it already has. So an existing id is refused: pass --id +to create a separate application, or unpack the archive and deploy the +directory to update the one that is there. + A runware.yaml carries no secret values. Its secrets name what the app needs, and each one has to already exist in the environment. A manifest key this CLI does not know is an error rather than a silent omission, which is what @@ -340,9 +347,9 @@ paths and an invoke example once the application is active.`, // deploy on another GPU type without being edited first. // // Nothing here marks a flag as changed. The create-only flags are - // refused on an update by exactly that test, and a manifest that - // carries a GPU type is describing the app, not asking to change a - // running one, so a redeploy from the same archive has to go through. + // refused on an update by exactly that test, and a deploy from a + // packaged source only ever creates, so the manifest's own values + // always reach a create. if manifest.Source.Type == string(serverlessapi.AppSourceTypeContainer) { if err := refuseCodeOnlyFlags(cmd, "this archive holds a container source"); err != nil { return err @@ -404,7 +411,7 @@ paths and an invoke example once the application is active.`, client := newServerlessClient(cmd, logger) - update, err := resolveDeployTarget(cmd, client, id, gpuType) + update, err := resolveDeployTarget(cmd, client, id, gpuType, from) if err != nil { return err } @@ -457,11 +464,6 @@ paths and an invoke example once the application is active.`, var app *serverlessapi.App if update { - if sections := unappliedManifestSections(from, manifest); len(sections) > 0 { - _, _ = fmt.Fprintf(cmd.ErrOrStderr(), - "Updating the source only. An existing app keeps its configuration, so the manifest sections %s do not apply here. Change configuration with 'runware serverless apps scale' or 'apps env-vars'.\n", - strings.Join(sections, ", ")) - } spin = cmdutil.NewSpinner(fmt.Sprintf("Updating application %s...", id)) spin.Start() app, err = client.UpdateApp(cmd.Context(), id, serverlessapi.AppUpdate{ @@ -548,7 +550,7 @@ paths and an invoke example once the application is active.`, cmd.Flags().StringVar(&srcDir, "src-dir", "", "Directory to package as the application source (default: the working directory; code deploys only)") cmd.Flags().StringVar(&containerDir, "container", "", "Directory whose root contains Dockerfile and container.yaml") - cmd.Flags().StringVar(&from, "from", "", "URL of a packaged source to fetch and deploy, reading its "+manifestFile) + cmd.Flags().StringVar(&from, "from", "", "URL of a packaged source to fetch and deploy as a new application, reading its "+manifestFile) cmd.Flags().StringArrayVar(&volumes, "volume", nil, "Absolute path inside the app backed by persistent node-local storage; immutable after create (repeatable)") cmd.Flags().StringArrayVar(&secrets, "secret", nil, "Secret of the same environment to attach at create, as NAME or NAME=ENV_VAR (repeatable)") cmd.Flags().StringArrayVar(&envVars, envVarFlag, nil, "Environment variable as KEY=VALUE (repeatable)") @@ -578,7 +580,7 @@ paths and an invoke example once the application is active.`, // creates one. Before the archive is uploaded it refuses a copy that is still // deleting, create-only flags on an update, and an unknown --env or a missing // --gpu-type on a create. -func resolveDeployTarget(cmd *cobra.Command, client *serverlessapi.Client, id, gpuType string) (update bool, err error) { +func resolveDeployTarget(cmd *cobra.Command, client *serverlessapi.Client, id, gpuType, from string) (update bool, err error) { app, err := existingApp(cmd.Context(), client, id) if err != nil { return false, err @@ -587,6 +589,12 @@ func resolveDeployTarget(cmd *cobra.Command, client *serverlessapi.Client, id, g if app.Status == serverlessapi.AppStatusDeleting { return false, fmt.Errorf("application %q is deleting; wait until it is deleted, then deploy", id) } + if from != "" { + return false, fmt.Errorf( + "application %q already exists, and --from only creates: deploying would roll out this archive as the next version of that application, which keeps the secrets, environment variables and volumes it has now. Pass --id to create a separate application, or unpack the archive and deploy the directory to update that one", + id, + ) + } return true, validateUpdateDeployFlags(cmd) } if err := requireEnvironment(cmd.Context(), client, commandEnvironment(cmd)); err != nil { @@ -679,31 +687,6 @@ func validateUpdateDeployFlags(cmd *cobra.Command) error { return nil } -// unappliedManifestSections names the manifest sections an update does not -// send. The update carries only the source, so a packaged version that adds a -// volume or a secret would otherwise deploy its new code without them and say -// nothing. Empty for a deploy that read no manifest. -func unappliedManifestSections(from string, manifest deployManifest) []string { - if from == "" { - return nil - } - - var sections []string - if len(manifest.Volumes) > 0 { - sections = append(sections, "volumes") - } - if len(manifest.EnvVars) > 0 { - sections = append(sections, "envVars") - } - if len(manifest.Secrets) > 0 { - sections = append(sections, "secrets") - } - if manifest.Compute != (manifestCompute{}) { - sections = append(sections, "compute") - } - return sections -} - // createOnlyDeployHint says how to change a create-only setting on the copy // in environment, under defaultEnvironment. func createOnlyDeployHint(name, environment, defaultEnvironment string) string { diff --git a/internal/cmd/serverless/deploy_test.go b/internal/cmd/serverless/deploy_test.go index af75a75..37efac5 100644 --- a/internal/cmd/serverless/deploy_test.go +++ b/internal/cmd/serverless/deploy_test.go @@ -9,7 +9,6 @@ import ( "maps" "net/http" "net/http/httptest" - "slices" "strings" "testing" "time" @@ -29,6 +28,7 @@ const ( testFromFlag = "--from" testFallbackGPU = "l40s" testBaseImageFlag = "--base-image" + testBaseImage = "python:3.12-slim" testRequirement = "--requirement" testArchiveURL = "https://runware.ai/docs/serverless/examples/echo.zip" testIDFlag = "--id" @@ -93,7 +93,7 @@ func TestValidateDeployArgs(t *testing.T) { }, { name: "container with base-image", - flags: []string{testContainerFlag, testWrapperDir, testBaseImageFlag, "python:3.12-slim"}, + flags: []string{testContainerFlag, testWrapperDir, testBaseImageFlag, testBaseImage}, wantErr: testBaseImageFlag, }, { @@ -491,7 +491,7 @@ func TestValidateUpdateDeployFlags(t *testing.T) { {flags: []string{"--available-workers-pct", "10"}, wantErr: scaleHint}, {flags: []string{"--name", "My App"}, wantErr: "apps rename"}, {flags: []string{testRequirement, testPipPackage}}, - {flags: []string{testBaseImageFlag, "python:3.12-slim"}}, + {flags: []string{testBaseImageFlag, testBaseImage}}, {flags: []string{testSrcDirFlag, "."}}, {flags: []string{testWaitFlag}}, } @@ -749,7 +749,7 @@ func TestRefuseCodeOnlyFlags(t *testing.T) { }, { name: "base-image", - flags: []string{testFromFlag, testArchiveURL, testBaseImageFlag, "python:3.12-slim"}, + flags: []string{testFromFlag, testArchiveURL, testBaseImageFlag, testBaseImage}, wantErr: testBaseImageFlag, }, { @@ -791,8 +791,7 @@ func TestRefuseCodeOnlyFlags(t *testing.T) { } // deployFromArchiveManifest is a packaged code source whose manifest carries a -// GPU type and a volume. Both are create-only flags, which is what makes it the -// fixture for the redeploy rule below. +// GPU type, a volume and a secret, so a create has to carry all three. const deployFromArchiveManifest = `configVersion: 1 id: ` + testAppID + ` source: @@ -802,8 +801,13 @@ compute: gpuType: ` + testGPUType + ` volumes: - /weights +secrets: + - name: ` + testManifestSecret + ` ` +// testManifestSecret is the secret deployFromArchiveManifest names. +const testManifestSecret = "HF_TOKEN" + // serveDeployFromArchive packages the manifest with its entry file and returns // the URL to deploy from. func serveDeployFromArchive(t *testing.T) string { @@ -869,15 +873,19 @@ func TestDeployFrom_CreatesFromTheManifest(t *testing.T) { if created.Volumes == nil || len(*created.Volumes) != 1 || (*created.Volumes)[0].MountPath != testVolumePath { t.Fatalf("volumes = %#v", created.Volumes) } + if created.Secrets == nil || len(*created.Secrets) != 1 || (*created.Secrets)[0].SecretName != testManifestSecret { + t.Fatalf("secrets = %#v", created.Secrets) + } } -// TestDeployFrom_RedeploysAnExistingApp fixes the redeploy rule: the manifest -// describes the app rather than asking to change a running one, so its -// create-only GPU type and volume must not be refused on an update. -func TestDeployFrom_RedeploysAnExistingApp(t *testing.T) { +// TestDeployFrom_RefusesAnExistingApp fixes the create-only rule: a manifest +// names an id, so deploying one over an application that already has that id +// would roll this archive out as that application's next version, running it +// with the secrets, environment variables and volumes already attached. +func TestDeployFrom_RefusesAnExistingApp(t *testing.T) { from := serveDeployFromArchive(t) - var creates, updates int + var writes int serveSourceUpload := newSourceUploadStub(t) api := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -888,12 +896,8 @@ func TestDeployFrom_RedeploysAnExistingApp(t *testing.T) { case r.Method == http.MethodGet && strings.HasPrefix(r.URL.Path, "/v1/apps/"): w.Header().Set("Content-Type", "application/json") _, _ = w.Write([]byte(activeAppBody(""))) - case r.Method == http.MethodPatch && r.URL.Path == testAppsPath+"/"+testAppID: - updates++ - w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte(activeAppBody(""))) - case r.Method == http.MethodPost && r.URL.Path == testAppsPath: - creates++ + case r.Method == http.MethodPatch || r.Method == http.MethodPost: + writes++ w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusCreated) _, _ = w.Write([]byte(activeAppBody(""))) @@ -907,48 +911,71 @@ func TestDeployFrom_RedeploysAnExistingApp(t *testing.T) { t.Setenv("RUNWARE_API_KEY", "test-key") t.Setenv("RUNWARE_SERVERLESS_BASE_URL", api.URL) - var stderr bytes.Buffer cmd := newDeployCmd(log.New(io.Discard)) cmd.SetOut(&bytes.Buffer{}) - cmd.SetErr(&stderr) + cmd.SetErr(&bytes.Buffer{}) cmd.SetArgs([]string{testFromFlag, from}) - if err := cmd.Execute(); err != nil { - t.Fatalf("redeploy %s: %v", testFromFlag, err) + err := cmd.Execute() + if err == nil { + t.Fatal("expected the deploy to be refused") } - - // The manifest carries a volume and a compute block, and an update sends - // neither, so the command has to say which sections it dropped. - for _, want := range []string{"volumes", "compute", "the source only"} { - if !strings.Contains(stderr.String(), want) { - t.Fatalf("stderr = %q, want it to mention %q", stderr.String(), want) + for _, want := range []string{testAppID, "only creates", "--id"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q does not mention %q", err, want) } } - - if updates != 1 { - t.Fatalf("updates = %d, want 1", updates) - } - if creates != 0 { - t.Fatalf("creates = %d, want 0", creates) + if writes != 0 { + t.Fatalf("writes = %d, want 0", writes) } } -func TestUnappliedManifestSections(t *testing.T) { - full := deployManifest{ - Volumes: []string{testVolumePath}, - EnvVars: map[string]string{"A": "1"}, - Secrets: []manifestSecret{{Name: "HF_TOKEN"}}, - Compute: manifestCompute{GpuType: testGPUType}, - } +// deployFromContainerManifest is a packaged container source. Its archive +// carries the container directory a container deploy expects. +const deployFromContainerManifest = `configVersion: 1 +id: ` + testAppID + ` +source: + type: container +compute: + gpuType: ` + testGPUType + ` +` + +// TestDeployFrom_RefusesCodeOnlyFlagsOnAContainerArchive drives the check +// through the command. The source type is inside the archive, so a code-only +// flag can only be refused once the manifest has been read, and the packer +// would otherwise drop it without a word. +func TestDeployFrom_RefusesCodeOnlyFlagsOnAContainerArchive(t *testing.T) { + from := serveArchive(t, buildTestArchive(t, []archiveEntry{ + {name: manifestFile, body: deployFromContainerManifest}, + {name: containerDockerfile, body: testDockerfile}, + {name: containerConfig, body: testContainer}, + })) + + var calls int + api := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + calls++ + t.Errorf("unexpected %s %s", r.Method, r.URL.Path) + w.WriteHeader(http.StatusNotFound) + })) + defer api.Close() - if got := unappliedManifestSections("", full); got != nil { - t.Fatalf("without --from: %v, want nil", got) + t.Setenv("RUNWARE_API_KEY", "test-key") + t.Setenv("RUNWARE_SERVERLESS_BASE_URL", api.URL) + + cmd := newDeployCmd(log.New(io.Discard)) + cmd.SetOut(&bytes.Buffer{}) + cmd.SetErr(&bytes.Buffer{}) + cmd.SetArgs([]string{testFromFlag, from, testBaseImageFlag, testBaseImage}) + err := cmd.Execute() + if err == nil { + t.Fatal("expected the deploy to be refused") } - want := []string{"volumes", "envVars", "secrets", "compute"} - if got := unappliedManifestSections(testArchiveURL, full); !slices.Equal(got, want) { - t.Fatalf("full manifest: %v, want %v", got, want) + for _, want := range []string{testBaseImageFlag, "container source"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q does not mention %q", err, want) + } } - if got := unappliedManifestSections(testArchiveURL, deployManifest{}); got != nil { - t.Fatalf("empty manifest: %v, want nil", got) + if calls != 0 { + t.Fatalf("the API was called %d times, want 0", calls) } } From c4d139c65b90d8d9b2ecff3446abbf3ea4ca99e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A9lix=20Sanz?= Date: Fri, 6 Nov 2026 12:33:19 +0100 Subject: [PATCH 12/13] fix: refuse two archive entries that differ only in case --- internal/cmd/serverless/from.go | 24 +++++++++++----- internal/cmd/serverless/from_test.go | 42 ++++++++++++++++++++++------ 2 files changed, 51 insertions(+), 15 deletions(-) diff --git a/internal/cmd/serverless/from.go b/internal/cmd/serverless/from.go index f5b480a..6ea22a7 100644 --- a/internal/cmd/serverless/from.go +++ b/internal/cmd/serverless/from.go @@ -132,20 +132,30 @@ func unpackArchive(archive []byte, dir string) error { } var total int64 - // Paths already written. unpackEntry opens each file with O_TRUNC and keeps - // no record, so without this the second of two entries naming one path - // silently replaces the first. - seen := make(map[string]struct{}, len(reader.File)) + // Paths already written, keyed without case and holding the name they were + // written under. unpackEntry opens each file with O_TRUNC and keeps no + // record, so without this the second of two entries naming one path silently + // replaces the first. + // + // Case is folded because `app.py` and `App.py` are one file on macOS and + // Windows and two on Linux, so an archive carrying both deploys different + // code depending on the machine that unpacks it. That refuses a pair Linux + // would have handled, which is the price of one archive meaning one app. + seen := make(map[string]string, len(reader.File)) for _, entry := range reader.File { rel, err := safeArchivePath(entry.Name) if err != nil { return err } if !entry.FileInfo().IsDir() { - if _, dup := seen[rel]; dup { - return fmt.Errorf("the fetched archive writes %s twice", rel) + key := strings.ToLower(rel) + if first, dup := seen[key]; dup { + if first == rel { + return fmt.Errorf("the fetched archive writes %s twice", rel) + } + return fmt.Errorf("the fetched archive writes both %s and %s, which are one file on a case-insensitive filesystem", first, rel) } - seen[rel] = struct{}{} + seen[key] = rel } written, err := unpackEntry(entry, dir) diff --git a/internal/cmd/serverless/from_test.go b/internal/cmd/serverless/from_test.go index ebaedfe..c91f58b 100644 --- a/internal/cmd/serverless/from_test.go +++ b/internal/cmd/serverless/from_test.go @@ -388,14 +388,40 @@ func TestArchiveClientRefusesAnHTTPSDowngrade(t *testing.T) { } func TestFetchSourceArchiveRefusesTwoEntriesOnOnePath(t *testing.T) { - archive := buildTestArchive(t, []archiveEntry{ - {name: manifestFile, body: codeManifest}, - {name: testEntryFile, body: testPySource}, - {name: "./" + testEntryFile, body: "x = 2\n"}, - }) + for _, tc := range []struct { + name string + entries []archiveEntry + wantErr string + }{ + { + name: "the same path twice", + entries: []archiveEntry{ + {name: testEntryFile, body: testPySource}, + {name: "./" + testEntryFile, body: "x = 2\n"}, + }, + wantErr: "twice", + }, + { + // One file on macOS and Windows, two on Linux, so the archive would + // deploy different code depending on where it was unpacked. + name: "two paths that differ only in case", + entries: []archiveEntry{ + {name: testEntryFile, body: testPySource}, + {name: strings.ToUpper(testEntryFile[:1]) + testEntryFile[1:], body: "x = 2\n"}, + }, + wantErr: "case-insensitive", + }, + } { + t.Run(tc.name, func(t *testing.T) { + archive := buildTestArchive(t, append( + []archiveEntry{{name: manifestFile, body: codeManifest}}, + tc.entries..., + )) - _, _, err := fetchSourceArchive(context.Background(), serveArchive(t, archive)) - if err == nil || !strings.Contains(err.Error(), "twice") { - t.Fatalf("expected a duplicate-path refusal, got %v", err) + _, _, err := fetchSourceArchive(context.Background(), serveArchive(t, archive)) + if err == nil || !strings.Contains(err.Error(), tc.wantErr) { + t.Fatalf("expected a duplicate-path refusal naming %q, got %v", tc.wantErr, err) + } + }) } } From 64bd744e60aaab1a5676ff67e9b80e80c1021d5c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A9lix=20Sanz?= Date: Fri, 6 Nov 2026 12:58:03 +0100 Subject: [PATCH 13/13] test: let an upload before the create-only refusal fail the test --- internal/cmd/serverless/deploy_test.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/internal/cmd/serverless/deploy_test.go b/internal/cmd/serverless/deploy_test.go index 37efac5..a6cd66d 100644 --- a/internal/cmd/serverless/deploy_test.go +++ b/internal/cmd/serverless/deploy_test.go @@ -885,13 +885,11 @@ func TestDeployFrom_CreatesFromTheManifest(t *testing.T) { func TestDeployFrom_RefusesAnExistingApp(t *testing.T) { from := serveDeployFromArchive(t) + // No upload stub: the refusal has to come before the source is uploaded, so + // an upload POST counts as a write here rather than being served. var writes int - serveSourceUpload := newSourceUploadStub(t) api := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if serveSourceUpload(w, r) { - return - } switch { case r.Method == http.MethodGet && strings.HasPrefix(r.URL.Path, "/v1/apps/"): w.Header().Set("Content-Type", "application/json")