diff --git a/docs/runware_serverless_deploy.md b/docs/runware_serverless_deploy.md index 4344063..ee5bea5 100644 --- a/docs/runware_serverless_deploy.md +++ b/docs/runware_serverless_deploy.md @@ -42,6 +42,28 @@ 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. 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. + +--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 +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 +146,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 +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 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/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.go b/internal/cmd/serverless/deploy.go index e8f7915..e45a87d 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,21 +78,47 @@ 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) + // 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 @@ -132,6 +164,7 @@ func newDeployCmd(logger *log.Logger) *cobra.Command { availableWorkersPct int32 srcDir string containerDir string + from string volumes []string secrets []string envVars []string @@ -182,6 +215,28 @@ 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. 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. + +--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 +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 +310,83 @@ 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 + manifestEnv map[string]string + ) + 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 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 + } + } + + 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 + } + // 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() + } + } + + if id == "" { + return fmt.Errorf("--id is required") + } if err := validateGPUsPerWorkerFlag(cmd, gpusPerWorker); err != nil { return err } @@ -288,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 } @@ -316,7 +439,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 } @@ -361,11 +484,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 +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 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)") @@ -449,10 +573,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 } @@ -460,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 @@ -469,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 { diff --git a/internal/cmd/serverless/deploy_test.go b/internal/cmd/serverless/deploy_test.go index 3b378ae..a6cd66d 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" "strings" @@ -24,7 +25,14 @@ const ( testPipPackage = "torch" testSourceID = "019c7654-8b21-7abc-9123-abcdef123456" testSrcDirFlag = "--src-dir" + 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" + testVolumePath = "/weights" testWaitFlag = "--wait" testTimeoutFlag = "--timeout" ) @@ -49,10 +57,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", @@ -61,13 +93,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, testBaseImage}, + 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", @@ -90,7 +122,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) @@ -349,7 +385,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) @@ -450,12 +486,12 @@ 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"}, - {flags: []string{"--requirement", testPipPackage}}, - {flags: []string{"--base-image", "python:3.12-slim"}}, + {flags: []string{testRequirement, testPipPackage}}, + {flags: []string{testBaseImageFlag, testBaseImage}}, {flags: []string{testSrcDirFlag, "."}}, {flags: []string{testWaitFlag}}, } @@ -501,7 +537,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))) @@ -698,6 +734,324 @@ 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, testBaseImage}, + 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) + } + }) + } +} + +// deployFromArchiveManifest is a packaged code source whose manifest carries a +// GPU type, a volume and a secret, so a create has to carry all three. +const deployFromArchiveManifest = `configVersion: 1 +id: ` + testAppID + ` +source: + type: code + entry: ` + testModelFile + ` +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 { + 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 != 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_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) + + // 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 + + api := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + 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.Method == http.MethodPost: + writes++ + 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}) + err := cmd.Execute() + if err == nil { + t.Fatal("expected the deploy to be refused") + } + 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 writes != 0 { + t.Fatalf("writes = %d, want 0", writes) + } +} + +// 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() + + 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") + } + for _, want := range []string{testBaseImageFlag, "container source"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q does not mention %q", err, want) + } + } + if calls != 0 { + t.Fatalf("the API was called %d times, want 0", calls) + } +} + +// 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) + } +} + // appIDValidationDetail is the getApp 422 for an appId outside the AppId // schema: one errors entry naming the path parameter, and no pointer, because // a parameter is not a field of the body. 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 new file mode 100644 index 0000000..6ea22a7 --- /dev/null +++ b/internal/cmd/serverless/from.go @@ -0,0 +1,266 @@ +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 + +// 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. +// +// 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 +} + +// 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) { + 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 := archiveClient.Do(req) + 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() { + _ = 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 + // 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() { + 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[key] = rel + } + + 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() + }() + + // 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) + } + + // 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)) + + // 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 new file mode 100644 index 0000000..c91f58b --- /dev/null +++ b/internal/cmd/serverless/from_test.go @@ -0,0 +1,427 @@ +package serverless + +import ( + "archive/zip" + "bytes" + "context" + "fmt" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "runtime" + "slices" + "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") + } +} + +// 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 == goosWindows { + t.Skip("no executable bit to preserve on Windows") + } + 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()) + } +} + +// 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) + } +} + +// 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) { + 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(), tc.wantErr) { + t.Fatalf("expected a duplicate-path refusal naming %q, got %v", tc.wantErr, err) + } + }) + } +} 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..3864494 --- /dev/null +++ b/internal/cmd/serverless/manifest.go @@ -0,0 +1,270 @@ +package serverless + +import ( + "bytes" + "errors" + "fmt" + "io" + "os" + "path/filepath" + "regexp" + "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"` +} + +// 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 == "" { + 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 { + 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) + } + 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 := secret.validate(); err != nil { + return err + } + } + 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) + } + // 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 +} + +// 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, "" +} + +// 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..1d08d83 --- /dev/null +++ b/internal/cmd/serverless/manifest_test.go @@ -0,0 +1,374 @@ +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) + } +} + +// 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: + type: code + entry: model.py +envVars: + LOG_LEVEL: info + CACHE_DIR: /weights + PAYLOAD: '"hello"' +`) + + for key, want := range map[string]string{ + "LOG_LEVEL": "info", + "CACHE_DIR": testVolumePath, + "PAYLOAD": `"hello"`, + } { + if manifest.EnvVars[key] != want { + t.Errorf("envVars[%s] = %q, want %q", key, manifest.EnvVars[key], 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) + } + } +} + +// 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) + } +} diff --git a/internal/cmd/serverless/pack_test.go b/internal/cmd/serverless/pack_test.go index 6cf4ead..43dcee2 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]) } } @@ -358,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() @@ -731,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() @@ -765,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()