From 55675dea5f8069d171c971c62d70e781eda4a2ee Mon Sep 17 00:00:00 2001 From: D1-3105 Date: Thu, 20 Aug 2026 23:19:16 +0000 Subject: [PATCH 1/6] feat(serverless): pack the source directory, not just the entry file `deploy ./app.py` shipped exactly one file: packPythonFile stat'd a single path, rejected a directory and wrote one zip entry. An app that imported a helper module or read a data file of its own could not be deployed at all, even though the builder unpacks the archive, puts every top-level entry on PYTHONPATH and hands them to MLflow code_paths -- it has always been built for a project tree. packDirectory walks the whole source directory instead. --src-dir sets the root and defaults to the working directory; the entry file must live inside it, and a relative path is resolved there rather than against the working directory, because modelFile is a path *inside* the codebase -- that is what the field means on the wire and what the builder looks up in the zip. Exclusions are gitignore semantics via go-git's matcher, which the .gitignore fallback needs: a bare `*.pyc` has to match at any depth, and dockerignore matchers do not do that. .runwareignore wins outright when present rather than unioning with .gitignore, so the result stays possible to reason about. An excluded directory is pruned rather than walked, which both keeps a 300MB .venv from costing a stat per file and reproduces git's own rule that a negation cannot re-include a file whose parent directory is excluded. .env files and .git are excluded absolutely, ahead of the matcher, so no rule -- not even an explicit `!.env` -- can re-include them. The build pod unpacks whatever it is sent, and the builder keeping top-level dotfiles out of the *image* is a later step, not a promise about the build. Caps are per-file and total, and the total error names the largest files and points at .runwareignore: "your upload is too big" without saying what filled it leaves the caller hunting through a deep tree. --- docs/runware_serverless_deploy.md | 26 +- go.mod | 12 +- go.sum | 27 +- internal/cmd/serverless/deploy.go | 29 +- internal/cmd/serverless/pack.go | 418 +++++++++++++++++++-- internal/cmd/serverless/pack_test.go | 532 +++++++++++++++++++++++++-- 6 files changed, 952 insertions(+), 92 deletions(-) diff --git a/docs/runware_serverless_deploy.md b/docs/runware_serverless_deploy.md index 42c7efe..2a8cbb1 100644 --- a/docs/runware_serverless_deploy.md +++ b/docs/runware_serverless_deploy.md @@ -6,9 +6,20 @@ Deploy a new serverless application Create a new serverless application from a Python entry file. -The file is zipped and submitted as the application source. Worker settings -are supplied via flags (a local project config via 'runware serverless init' -is planned). Endpoints are derived server-side from the SDK. +The whole source directory is zipped and submitted as the application source, so +the entry file can import its own modules and read its own data files. That +directory is the working directory unless --src-dir says otherwise. + +The entry file must live inside the source directory. A relative path is resolved +inside it; an absolute path is taken as given. + +Exclude what the app does not need with a .runwareignore file at the root of the +source directory; it takes gitignore syntax. Without one, a .gitignore is used +instead. Either way .env files are never uploaded, and neither are .git, +__pycache__, .venv or node_modules. + +Worker settings are supplied via flags (a local project config via 'runware +serverless init' is planned). Endpoints are derived server-side from the SDK. ``` runware serverless deploy [flags] @@ -17,9 +28,15 @@ runware serverless deploy [flags] ### Examples ``` - # deploy a Python entry file + # deploy the current directory, with app.py as the entry point runware serverless deploy ./app.py --id my-app --gpu-type h100 + # deploy a project that lives elsewhere; app.py is resolved inside --src-dir + runware serverless deploy app.py --src-dir ~/projects/my-app --id my-app --gpu-type h100 + + # an entry file in a subdirectory of the project + runware serverless deploy src/app.py --src-dir ~/projects/my-app --id my-app --gpu-type h100 + # override worker settings and base image runware serverless deploy ./app.py --id my-app --name "My App" \ --max-workers 2 --idle-ttl 120 --gpu-type h100 \ @@ -40,6 +57,7 @@ runware serverless deploy [flags] --name string Display name (defaults to --id) --requirement stringArray Additional pip package to install (repeatable) --scaling-delay int32 Scaling delay in seconds (default 10) + --src-dir string Directory to package as the application source (default: the working directory) ``` ### Options inherited from parent commands diff --git a/go.mod b/go.mod index 24bd7dd..ddfc50c 100644 --- a/go.mod +++ b/go.mod @@ -5,13 +5,15 @@ go 1.26.4 require ( github.com/briandowns/spinner v1.23.2 github.com/charmbracelet/log v1.0.0 + github.com/go-git/go-git/v5 v5.16.3 github.com/google/uuid v1.6.0 github.com/gorilla/websocket v1.5.3 github.com/jedib0t/go-pretty/v6 v6.7.10 github.com/oapi-codegen/runtime v1.6.0 github.com/pkg/browser v0.0.0-20240102092130-5ac0b6a4141c github.com/spf13/cobra v1.10.2 - golang.org/x/term v0.41.0 + github.com/spf13/pflag v1.0.10 + golang.org/x/term v0.45.0 gopkg.in/yaml.v3 v3.0.1 ) @@ -28,10 +30,13 @@ require ( github.com/dprotaso/go-yit v0.0.0-20220510233725-9ba8df137936 // indirect github.com/fatih/color v1.7.0 // indirect github.com/getkin/kin-openapi v0.142.0 // indirect + github.com/go-git/gcfg v1.5.1-0.20230307220236-3a3c6141e376 // indirect + github.com/go-git/go-billy/v5 v5.6.2 // indirect github.com/go-logfmt/logfmt v0.6.1 // indirect github.com/go-openapi/jsonpointer v0.23.1 // indirect github.com/go-openapi/swag/jsonname v0.26.0 // indirect github.com/inconshreveable/mousetrap v1.1.0 // indirect + github.com/jbenet/go-context v0.0.0-20150711004518-d14ea06fba99 // indirect github.com/lucasb-eyer/go-colorful v1.2.0 // indirect github.com/mattn/go-colorable v0.1.14 // indirect github.com/mattn/go-isatty v0.0.20 // indirect @@ -45,16 +50,17 @@ require ( github.com/santhosh-tekuri/jsonschema/v6 v6.0.2 // indirect github.com/speakeasy-api/jsonpath v0.6.3 // indirect github.com/speakeasy-api/openapi v1.24.0 // indirect - github.com/spf13/pflag v1.0.10 // indirect github.com/vmware-labs/yaml-jsonpath v0.3.2 // indirect github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e // indirect go.yaml.in/yaml/v3 v3.0.4 // indirect - golang.org/x/exp v0.0.0-20240404231335-c0f41cb1a7a0 // indirect + golang.org/x/exp v0.0.0-20240719175910-8a7402abbf56 // indirect golang.org/x/mod v0.38.0 // indirect + golang.org/x/net v0.57.0 // indirect golang.org/x/sync v0.22.0 // indirect golang.org/x/sys v0.47.0 // indirect golang.org/x/text v0.40.0 // indirect golang.org/x/tools v0.48.0 // indirect + gopkg.in/warnings.v0 v0.1.2 // indirect ) tool github.com/oapi-codegen/oapi-codegen/v2/cmd/oapi-codegen diff --git a/go.sum b/go.sum index ac91484..4e0d51b 100644 --- a/go.sum +++ b/go.sum @@ -41,6 +41,12 @@ github.com/fsnotify/fsnotify v1.4.9 h1:hsms1Qyu0jgnwNXIxa+/V/PDsU6CfLf6CNO8H7IWo github.com/fsnotify/fsnotify v1.4.9/go.mod h1:znqG4EE+3YCdAaPaxE2ZRY/06pZUdp0tY4IgpuI1SZQ= github.com/getkin/kin-openapi v0.142.0 h1:izj0vBdFprMhitfzaX8sTqztsEQyvwhssBoB6n8NO7w= github.com/getkin/kin-openapi v0.142.0/go.mod h1:3BH9M9XDe/y9M5DSvEocVYAYq1w0qrhJHjC/vZi0AaY= +github.com/go-git/gcfg v1.5.1-0.20230307220236-3a3c6141e376 h1:+zs/tPmkDkHx3U66DAb0lQFJrpS6731Oaa12ikc+DiI= +github.com/go-git/gcfg v1.5.1-0.20230307220236-3a3c6141e376/go.mod h1:an3vInlBmSxCcxctByoQdvwPiA7DTK7jaaFDBTtu0ic= +github.com/go-git/go-billy/v5 v5.6.2 h1:6Q86EsPXMa7c3YZ3aLAQsMA0VlWmy43r6FHqa/UNbRM= +github.com/go-git/go-billy/v5 v5.6.2/go.mod h1:rcFC2rAsp/erv7CMz9GczHcuD0D32fWzH+MJAU+jaUU= +github.com/go-git/go-git/v5 v5.16.3 h1:Z8BtvxZ09bYm/yYNgPKCzgWtaRqDTgIKRgIRHBfU6Z8= +github.com/go-git/go-git/v5 v5.16.3/go.mod h1:4Ge4alE/5gPs30F2H1esi2gPd69R0C39lolkucHBOp8= github.com/go-logfmt/logfmt v0.6.1 h1:4hvbpePJKnIzH1B+8OR/JPbTx37NktoI9LE2QZBBkvE= github.com/go-logfmt/logfmt v0.6.1/go.mod h1:EV2pOAQoZaT1ZXZbqDl5hrymndi4SY9ED9/z6CO0XAk= github.com/go-openapi/jsonpointer v0.23.1 h1:1HBACs7XIwR2RcmItfdSFlALhGbe6S92p0ry4d1GWg4= @@ -74,6 +80,8 @@ github.com/hpcloud/tail v1.0.0/go.mod h1:ab1qPbhIpdTxEkNHXyeSf5vhxWSCs/tWer42PpO github.com/ianlancetaylor/demangle v0.0.0-20200824232613-28f6c0f3b639/go.mod h1:aSSvb/t6k1mPoxDqO4vJh6VOCGPwU4O0C2/Eqndh1Sc= github.com/inconshreveable/mousetrap v1.1.0 h1:wN+x4NVGpMsO7ErUn/mUI3vEoE6Jt13X2s0bqwp9tc8= github.com/inconshreveable/mousetrap v1.1.0/go.mod h1:vpF70FUmC8bwa3OWnCshd2FqLfsEA9PFc4w1p2J65bw= +github.com/jbenet/go-context v0.0.0-20150711004518-d14ea06fba99 h1:BQSFePA1RWJOlocH6Fxy8MmwDt+yVQYULKfN0RoTN8A= +github.com/jbenet/go-context v0.0.0-20150711004518-d14ea06fba99/go.mod h1:1lJo3i6rXxKeerYnT8Nvf0QmHCRC1n8sfWVwXF2Frvo= github.com/jedib0t/go-pretty/v6 v6.7.10 h1:B/2qW2Bkv2L6n14PP8o1kx75kWzHOQ3YTluWzg9icac= github.com/jedib0t/go-pretty/v6 v6.7.10/go.mod h1:YwC5CE4fJ1HFUDeivSV1r//AmANFHyqczZk+U6BDALU= github.com/juju/gnuflag v0.0.0-20171113085948-2ce1bb71843d/go.mod h1:2PavIy+JPciBPrBUjwbNvtwB6RQlve+hkpll6QSNmOE= @@ -117,10 +125,13 @@ github.com/onsi/gomega v1.7.0/go.mod h1:ex+gbHU/CVuBBDIJjb2X0qEXbFg53c61hWP/1Cpa github.com/onsi/gomega v1.7.1/go.mod h1:XdKZgCCFLUoM/7CFJVPcG8C1xQ1AJ0vpAezJrB7JYyY= github.com/onsi/gomega v1.10.1/go.mod h1:iN09h71vgCQne3DLsj+A5owkum+a2tYe+TOCB1ybHNo= github.com/onsi/gomega v1.17.0/go.mod h1:HnhC7FXeEQY45zxNK3PPoIUhzk/80Xly9PcubAlGdZY= -github.com/onsi/gomega v1.19.0 h1:4ieX6qQjPP/BfC3mpsAtIGGlxTWPeA3Inl/7DtXw1tw= github.com/onsi/gomega v1.19.0/go.mod h1:LY+I3pBVzYsTBU1AnDwOSxaYi9WoWiqgwooUqq9yPro= +github.com/onsi/gomega v1.34.1 h1:EUMJIKUjM8sKjYbtxQI9A4z2o+rruxnzNvpknOXie6k= +github.com/onsi/gomega v1.34.1/go.mod h1:kU1QgUvBDLXBJq618Xvm2LUX6rSAfRaFRTcdOeDLwwY= github.com/pkg/browser v0.0.0-20240102092130-5ac0b6a4141c h1:+mdjkGKdHQG3305AYmdv1U2eRNDiU2ErMBj1gwrq8eQ= github.com/pkg/browser v0.0.0-20240102092130-5ac0b6a4141c/go.mod h1:7rwL4CYBLnjLxUqIJNnCWiEdr3bn6IUYi15bNlnbCCU= +github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4= +github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 h1:Jamvg5psRIccs7FGNTlIRMkT8wgtp5eCXdBlqhYGL6U= github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= @@ -132,8 +143,9 @@ github.com/russross/blackfriday/v2 v2.1.0 h1:JIOH55/0cWyOuilr9/qlrm0BSXldqnqwMsf github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM= github.com/santhosh-tekuri/jsonschema/v6 v6.0.2 h1:KRzFb2m7YtdldCEkzs6KqmJw4nqEVZGK7IN2kJkjTuQ= github.com/santhosh-tekuri/jsonschema/v6 v6.0.2/go.mod h1:JXeL+ps8p7/KNMjDQk3TCwPpBy0wYklyWTfbkIzdIFU= -github.com/sergi/go-diff v1.1.0 h1:we8PVUC3FE2uYfodKH/nBHMSetSfHDR6scGdBi+erh0= github.com/sergi/go-diff v1.1.0/go.mod h1:STckp+ISIX8hZLjrqAeVduY0gWCT9IjLuqbuNXdaHfM= +github.com/sergi/go-diff v1.3.2-0.20230802210424-5b0b94c5c0d3 h1:n661drycOFuPLCN3Uc8sB6B/s6Z4t2xvBgU1htSHuq8= +github.com/sergi/go-diff v1.3.2-0.20230802210424-5b0b94c5c0d3/go.mod h1:A0bzQcvG0E7Rwjx0REVgAGH58e96+X0MeOfepqsbeW4= github.com/speakeasy-api/jsonpath v0.6.3 h1:c+QPwzAOdrWvzycuc9HFsIZcxKIaWcNpC+xhOW9rJxU= github.com/speakeasy-api/jsonpath v0.6.3/go.mod h1:2cXloNuQ+RSXi5HTRaeBh7JEmjRXTiaKpFTdZiL7URI= github.com/speakeasy-api/openapi v1.24.0 h1:opoD27rupX7zBVPq1HkIGLeMOzNNA7JalhYP8q34i04= @@ -160,8 +172,8 @@ go.yaml.in/yaml/v3 v3.0.4/go.mod h1:DhzuOOF2ATzADvBadXxruRBLzYTpT36CKvDb3+aBEFg= golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w= golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI= golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto= -golang.org/x/exp v0.0.0-20240404231335-c0f41cb1a7a0 h1:985EYyeCOxTpcgOTJpflJUwOeEz0CQOdPt73OzpE9F8= -golang.org/x/exp v0.0.0-20240404231335-c0f41cb1a7a0/go.mod h1:/lliqkxwWAhPjf5oSOIJup2XcqJaw8RGS6k3TGEc7GI= +golang.org/x/exp v0.0.0-20240719175910-8a7402abbf56 h1:2dVuKD2vS7b0QIHQbpyTISPd0LeHDbnYEryqj5Q1ug8= +golang.org/x/exp v0.0.0-20240719175910-8a7402abbf56/go.mod h1:M4RDyNAINzryxdtnbRXRL/OHtkFuWGRjvuhBJpk2IlY= golang.org/x/mod v0.3.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/mod v0.38.0 h1:MECBjubtXD7yj4HrhIUcywNaGeNVUdfVnxmPajOk4yk= golang.org/x/mod v0.38.0/go.mod h1:V6Xz0pq8TQ3dGqVQ1FVHuelZpAL0uNhSkk9ogYP3c40= @@ -199,8 +211,8 @@ golang.org/x/sys v0.47.0 h1:o7XGOvZQCADBQQ4Y7VNq2dRWQR7JmOUW8Kxx4ZsNgWs= golang.org/x/sys v0.47.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo= golang.org/x/term v0.0.0-20210927222741-03fcf44c2211/go.mod h1:jbD1KX2456YbFQfuXm/mYQcufACuNUgVhRMnK/tPxf8= -golang.org/x/term v0.41.0 h1:QCgPso/Q3RTJx2Th4bDLqML4W6iJiaXFq2/ftQF13YU= -golang.org/x/term v0.41.0/go.mod h1:3pfBgksrReYfZ5lvYM0kSO0LIkAl4Yl2bXOkKP7Ec2A= +golang.org/x/term v0.45.0 h1:NwWyBmoJCbfTHpxrWoZ9C6/VxOf7ic219I8xZZFdrf0= +golang.org/x/term v0.45.0/go.mod h1:9aqxs0blBcrm/n0L9QW0aRVD+ktan8ssZromtqJC43w= golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= golang.org/x/text v0.3.6/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= @@ -231,11 +243,12 @@ gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c/go.mod h1:JHkPIbrfpd72SG/EV gopkg.in/fsnotify.v1 v1.4.7/go.mod h1:Tz8NjZHkW78fSQdbUxIjBTcgA1z1m8ZHf0WmKUhAMys= gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7 h1:uRGJdciOHaEIrze2W8Q3AKkepLTh2hOroT7a+7czfdQ= gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7/go.mod h1:dt/ZhP58zS4L8KSrWDmTeBkI65Dw0HsyUHuEVlX15mw= +gopkg.in/warnings.v0 v0.1.2 h1:wFXVbFY8DY5/xOe1ECiWdKCzZlxgshcYVNkBHstARME= +gopkg.in/warnings.v0 v0.1.2/go.mod h1:jksf8JmL6Qr/oQM2OXTHunEvvTAsrWBLb6OOjuVWRNI= gopkg.in/yaml.v2 v2.2.1/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= gopkg.in/yaml.v2 v2.2.2/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= gopkg.in/yaml.v2 v2.2.4/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= gopkg.in/yaml.v2 v2.3.0/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= -gopkg.in/yaml.v2 v2.4.0 h1:D8xgwECY7CYvx+Y2n4sBz93Jn9JRvxdiyyo8CTfuKaY= gopkg.in/yaml.v2 v2.4.0/go.mod h1:RDklbk79AGWmwhnvt/jBztapEOGDOx6ZbXqjP6csGnQ= gopkg.in/yaml.v3 v3.0.0-20191026110619-0b21df46bc1d/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/internal/cmd/serverless/deploy.go b/internal/cmd/serverless/deploy.go index 3f5acdd..b10d72b 100644 --- a/internal/cmd/serverless/deploy.go +++ b/internal/cmd/serverless/deploy.go @@ -24,6 +24,7 @@ func newDeployCmd(logger *log.Logger) *cobra.Command { requirements []string minWorkers int32 gpusPerWorker int32 + srcDir string ) cmd := &cobra.Command{ @@ -31,12 +32,29 @@ func newDeployCmd(logger *log.Logger) *cobra.Command { Short: "Deploy a new serverless application", Long: `Create a new serverless application from a Python entry file. -The file is zipped and submitted as the application source. Worker settings -are supplied via flags (a local project config via 'runware serverless init' -is planned). Endpoints are derived server-side from the SDK.`, - Example: ` # deploy a Python entry file +The whole source directory is zipped and submitted as the application source, so +the entry file can import its own modules and read its own data files. That +directory is the working directory unless --src-dir says otherwise. + +The entry file must live inside the source directory. A relative path is resolved +inside it; an absolute path is taken as given. + +Exclude what the app does not need with a .runwareignore file at the root of the +source directory; it takes gitignore syntax. Without one, a .gitignore is used +instead. Either way .env files are never uploaded, and neither are .git, +__pycache__, .venv or node_modules. + +Worker settings are supplied via flags (a local project config via 'runware +serverless init' is planned). Endpoints are derived server-side from the SDK.`, + Example: ` # deploy the current directory, with app.py as the entry point runware serverless deploy ./app.py --id my-app --gpu-type h100 + # deploy a project that lives elsewhere; app.py is resolved inside --src-dir + runware serverless deploy app.py --src-dir ~/projects/my-app --id my-app --gpu-type h100 + + # an entry file in a subdirectory of the project + runware serverless deploy src/app.py --src-dir ~/projects/my-app --id my-app --gpu-type h100 + # override worker settings and base image runware serverless deploy ./app.py --id my-app --name "My App" \ --max-workers 2 --idle-ttl 120 --gpu-type h100 \ @@ -48,7 +66,7 @@ is planned). Endpoints are derived server-side from the SDK.`, name = id } - zipBase64, modelFile, err := packPythonFile(entryFile) + zipBase64, modelFile, err := packDirectory(srcDir, entryFile) if err != nil { return err } @@ -94,6 +112,7 @@ is planned). Endpoints are derived server-side from the SDK.`, }, } + cmd.Flags().StringVar(&srcDir, "src-dir", "", "Directory to package as the application source (default: the working directory)") cmd.Flags().StringVar(&id, "id", "", "Application ID (immutable, lowercase slug)") cmd.Flags().StringVar(&name, "name", "", "Display name (defaults to --id)") cmd.Flags().Int32Var(&maxWorkers, "max-workers", 1, "Maximum number of workers") diff --git a/internal/cmd/serverless/pack.go b/internal/cmd/serverless/pack.go index 16e2e35..12841c6 100644 --- a/internal/cmd/serverless/pack.go +++ b/internal/cmd/serverless/pack.go @@ -6,68 +6,410 @@ import ( "encoding/base64" "fmt" "io" + "io/fs" "os" "path/filepath" + "sort" + "strings" + + "github.com/go-git/go-git/v5/plumbing/format/gitignore" ) -// maxPackEntryBytes is the maximum size of a single entry file we will zip and -// base64-encode for createApp. Keeps accidental large inputs from -// blowing memory (base64 expands the payload by ~4/3). +// maxPackEntryBytes is the maximum size of a single file we will put in the +// archive. Keeps one accidental artefact — a checkpoint, a dataset someone left +// in the project — from blowing memory, since the whole archive is held in +// memory and base64 expands it by ~4/3. const maxPackEntryBytes int64 = 10 << 20 // 10 MiB -// packPythonFile zips a single Python entry file and returns the base64-encoded -// archive plus the modelFile path expected inside the zip (the file's basename). -func packPythonFile(path string) (zipBase64, modelFile string, err error) { - info, err := os.Stat(path) +// maxPackTotalBytes bounds the archive as a whole. The per-file cap alone does +// not: a virtualenv is thousands of small files and would sail past it. +const maxPackTotalBytes int64 = 25 << 20 // 25 MiB + +// runwareIgnoreFile is the project's own exclude list. When it is absent the +// packer falls back to gitIgnoreFile, because a project that already tells git +// what not to track has usually said the same thing this needs to know. +const ( + runwareIgnoreFile = ".runwareignore" + gitIgnoreFile = ".gitignore" +) + +// defaultIgnorePatterns are excluded when no rule says otherwise. They are +// ordinary gitignore patterns evaluated before the project's own, so a later +// `!*.pyc` re-includes what one of the file patterns here excluded. The +// directory patterns are not negatable the same way -- git does not allow +// re-including a file whose parent directory is excluded, and collectFiles +// prunes those directories for exactly that reason. +var defaultIgnorePatterns = []string{ + // Python build and cache output. None of it is source, and a virtualenv is + // the difference between an 80KB upload and a 400MB one. + "__pycache__/", + "*.pyc", + "*.pyo", + ".venv/", + "venv/", + "*.egg-info/", + // Tool caches, which appear in any project that has been tested or linted. + ".pytest_cache/", + ".ruff_cache/", + ".mypy_cache/", + ".tox/", + ".ipynb_checkpoints/", + // Everything else. + "node_modules/", + ".DS_Store", + runwareIgnoreFile, +} + +// alwaysExcluded reports paths that no rule may re-include. +// +// `.env` and its variants hold credentials, and the build unpacks this archive +// into a pod where its contents are readable — the builder keeps top-level +// dotfiles out of the *image*, but that is a later step and not a promise about +// the build. Matching on every path segment rather than the root alone, because +// `config/.env` is the same secret one directory down. `.git` goes with it: +// nothing in the build reads it, and it carries the whole history of everything +// the other rules exclude. +func alwaysExcluded(segments []string) bool { + for _, s := range segments { + if s == ".git" || s == ".env" || strings.HasPrefix(s, ".env.") { + return true + } + } + return false +} + +// packDirectory zips srcDir and returns the base64-encoded archive plus the path +// of modelFile inside it. +// +// srcDir is the archive root: every path in the zip is relative to it, which is +// what the builder puts on PYTHONPATH and hands to MLflow code_paths, so the +// project's own imports resolve at build and serve time exactly as they do +// locally. An empty srcDir means the working directory. +func packDirectory(srcDir, modelFile string) (zipBase64, modelFileRel string, err error) { + root, err := resolveSrcDir(srcDir) + if err != nil { + return "", "", err + } + + modelFileRel, err = relativeModelFile(root, modelFile) + if err != nil { + return "", "", err + } + + matcher, err := loadIgnoreMatcher(root) + if err != nil { + return "", "", err + } + + files, err := collectFiles(root, modelFileRel, matcher) + if err != nil { + return "", "", err + } + if len(files) == 0 { + // Unreachable while the model file is forced in, but a packer that can + // return an empty archive should say so rather than let the builder + // answer with a 422 about a missing model file. + return "", "", fmt.Errorf("no files to pack under %q", root) + } + + raw, err := writeArchive(root, files) if err != nil { - return "", "", fmt.Errorf("read entry file: %w", err) + return "", "", err + } + return base64.StdEncoding.EncodeToString(raw), modelFileRel, nil +} + +// resolveSrcDir defaults an empty srcDir to the working directory and checks it +// is a directory. Symlinks are resolved so the relative path of the model file +// is computed against the same tree WalkDir will produce. +func resolveSrcDir(srcDir string) (string, error) { + if srcDir == "" { + wd, err := os.Getwd() + if err != nil { + return "", fmt.Errorf("resolve working directory: %w", err) + } + srcDir = wd + } + abs, err := filepath.Abs(srcDir) + if err != nil { + return "", fmt.Errorf("resolve source directory: %w", err) + } + resolved, err := filepath.EvalSymlinks(abs) + if err != nil { + return "", fmt.Errorf("read source directory %q: %w", srcDir, err) + } + info, err := os.Stat(resolved) + if err != nil { + return "", fmt.Errorf("read source directory %q: %w", srcDir, err) + } + if !info.IsDir() { + return "", fmt.Errorf("source directory %q is not a directory", srcDir) + } + return resolved, nil +} + +// relativeModelFile locates the model file inside the archive root. +// +// The builder rejects a modelFile that is absolute or escapes the codebase, and +// answers 422 for a modelFile it cannot find in the zip. Both are worth catching +// here: the failure is the caller's to fix and the archive may be megabytes. +func relativeModelFile(root, modelFile string) (string, error) { + if modelFile == "" { + return "", fmt.Errorf("model file is required") + } + abs := locateModelFile(root, modelFile) + info, err := os.Stat(abs) + if err != nil { + return "", fmt.Errorf( + "read model file %q: %w (relative paths are resolved inside the source directory %s)", + modelFile, err, root, + ) } if info.IsDir() { - return "", "", fmt.Errorf("entry file %q is a directory", path) - } - if info.Size() > maxPackEntryBytes { - return "", "", fmt.Errorf( - "entry file %q is %d bytes; maximum supported size is %d bytes (%d MiB)", - path, - info.Size(), - maxPackEntryBytes, - maxPackEntryBytes>>20, + return "", fmt.Errorf("model file %q is a directory", modelFile) + } + // EvalSymlinks on the parent only: a model file reached through a symlinked + // directory still belongs to the tree, and resolving the file itself would + // reject the common case of an editor's saved-through link. + parent, err := filepath.EvalSymlinks(filepath.Dir(abs)) + if err != nil { + return "", fmt.Errorf("read model file %q: %w", modelFile, err) + } + abs = filepath.Join(parent, filepath.Base(abs)) + + rel, err := filepath.Rel(root, abs) + if err != nil { + return "", fmt.Errorf("locate model file inside %q: %w", root, err) + } + if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + return "", fmt.Errorf( + "model file %q is outside the source directory %q; pass --src-dir pointing at the project root", + modelFile, root, ) } + return filepath.ToSlash(rel), nil +} + +// locateModelFile resolves a model file argument to an absolute path. +// +// One rule: absolute is taken as given, relative is relative to the source +// directory. modelFile is a path *inside* the codebase -- that is what the field +// means on the wire and what the builder looks up in the zip -- so the source +// directory is the only sensible thing for it to be relative to. With no +// --src-dir the source directory is the working directory, so `deploy ./app.py` +// from the project means what it always did. +func locateModelFile(root, modelFile string) string { + if filepath.IsAbs(modelFile) { + return modelFile + } + return filepath.Join(root, modelFile) +} + +// loadIgnoreMatcher builds the exclusion matcher: the built-in defaults first, +// then the project's own rules, so a project rule can override a default. +// .runwareignore wins outright when present — a project that writes one is +// saying what to ship, and silently unioning .gitignore into it would make the +// result impossible to reason about. +func loadIgnoreMatcher(root string) (gitignore.Matcher, error) { + patterns := make([]gitignore.Pattern, 0, len(defaultIgnorePatterns)) + for _, p := range defaultIgnorePatterns { + patterns = append(patterns, gitignore.ParsePattern(p, nil)) + } + + for _, name := range []string{runwareIgnoreFile, gitIgnoreFile} { + lines, err := readIgnoreFile(filepath.Join(root, name)) + if err != nil { + return nil, err + } + if lines == nil { + continue + } + for _, line := range lines { + patterns = append(patterns, gitignore.ParsePattern(line, nil)) + } + break + } + + return gitignore.NewMatcher(patterns), nil +} + +// readIgnoreFile returns the file's meaningful lines, or nil when it is absent. +// A present-but-empty file returns a non-nil empty slice, so it still counts as +// "the project chose .runwareignore" and suppresses the .gitignore fallback. +func readIgnoreFile(path string) ([]string, error) { + raw, err := os.ReadFile(path) + if os.IsNotExist(err) { + return nil, nil + } + if err != nil { + return nil, fmt.Errorf("read %s: %w", filepath.Base(path), err) + } - modelFile = filepath.Base(path) - if modelFile == "." || modelFile == string(filepath.Separator) { - return "", "", fmt.Errorf("invalid entry file path %q", path) + lines := []string{} + for _, line := range strings.Split(string(raw), "\n") { + trimmed := strings.TrimSpace(line) + if trimmed == "" || strings.HasPrefix(trimmed, "#") { + continue + } + lines = append(lines, trimmed) } + return lines, nil +} + +// packedFile is one archive entry: its path relative to the root, and its size. +type packedFile struct { + rel string + size int64 +} - f, err := os.Open(path) +// collectFiles walks the tree and returns what belongs in the archive, in the +// lexical order WalkDir yields — so the same tree always packs to the same bytes. +func collectFiles(root, modelFileRel string, matcher gitignore.Matcher) ([]packedFile, error) { + var files []packedFile + var total int64 + + err := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if path == root { + return nil + } + + rel, err := filepath.Rel(root, path) + if err != nil { + return err + } + rel = filepath.ToSlash(rel) + segments := strings.Split(rel, "/") + + if alwaysExcluded(segments) { + if d.IsDir() { + return fs.SkipDir + } + return nil + } + // The model file is packed whatever the rules say: the deploy cannot + // succeed without it, and an ignore rule that happens to cover it is a + // worse failure than a file the customer did not mean to ship. + if rel != modelFileRel && matcher.Match(segments, d.IsDir()) { + // Pruning the directory rather than descending is what keeps a + // .venv from costing a stat per file. It also reproduces git's own + // rule -- "it is not possible to re-include a file if a parent + // directory of that file is excluded" -- so a `!node_modules/keep.js` + // does not apply, exactly as it would not for git. + if d.IsDir() { + return fs.SkipDir + } + return nil + } + if d.IsDir() { + return nil + } + // Regular files only. A symlink may point outside the tree, and the + // archive is a copy rather than a checkout; sockets and devices have + // nothing to copy. + if !d.Type().IsRegular() { + return nil + } + + info, err := d.Info() + if err != nil { + return err + } + if info.Size() > maxPackEntryBytes { + return fmt.Errorf( + "%s is %s; the maximum for a single file is %s. Exclude it with a %s rule", + rel, humanBytes(info.Size()), humanBytes(maxPackEntryBytes), runwareIgnoreFile, + ) + } + total += info.Size() + files = append(files, packedFile{rel: rel, size: info.Size()}) + return nil + }) if err != nil { - return "", "", fmt.Errorf("open entry file: %w", err) + return nil, err + } + + if total > maxPackTotalBytes { + return nil, fmt.Errorf( + "the source directory holds %s of files to pack; the maximum is %s.\n%s\nExclude what the app does not need with a %s file", + humanBytes(total), humanBytes(maxPackTotalBytes), largestFilesSummary(files), runwareIgnoreFile, + ) } - defer f.Close() //nolint:errcheck + return files, nil +} +// largestFilesSummary names what filled the archive. "Too big" on its own leaves +// the caller to find the offender by hand, which for a deep tree is the whole +// problem rather than a detail of it. +func largestFilesSummary(files []packedFile) string { + sorted := make([]packedFile, len(files)) + copy(sorted, files) + sort.Slice(sorted, func(i, j int) bool { return sorted[i].size > sorted[j].size }) + + var b strings.Builder + b.WriteString("Largest files:") + for i, f := range sorted { + if i == 5 { + break + } + fmt.Fprintf(&b, "\n %8s %s", humanBytes(f.size), f.rel) + } + return b.String() +} + +func humanBytes(n int64) string { + switch { + case n >= 1<<20: + return fmt.Sprintf("%.1f MiB", float64(n)/(1<<20)) + case n >= 1<<10: + return fmt.Sprintf("%.1f KiB", float64(n)/(1<<10)) + default: + return fmt.Sprintf("%d B", n) + } +} + +// writeArchive builds the zip. Entry names are the already-slashed relative +// paths: zip names are always "/"-separated, which is what makes an archive +// packed on Windows unpack correctly in the Linux build pod. +func writeArchive(root string, files []packedFile) ([]byte, error) { var buf bytes.Buffer zw := zip.NewWriter(&buf) - w, err := zw.Create(modelFile) + + for _, f := range files { + if err := writeArchiveEntry(zw, root, f); err != nil { + return nil, err + } + } + if err := zw.Close(); err != nil { + return nil, fmt.Errorf("close zip: %w", err) + } + return buf.Bytes(), nil +} + +func writeArchiveEntry(zw *zip.Writer, root string, f packedFile) error { + src, err := os.Open(filepath.Join(root, filepath.FromSlash(f.rel))) if err != nil { - return "", "", fmt.Errorf("create zip entry: %w", err) + return fmt.Errorf("open %s: %w", f.rel, err) } - // Cap the copy in case the file grows after Stat. - written, err := io.Copy(w, io.LimitReader(f, maxPackEntryBytes+1)) + defer src.Close() //nolint:errcheck + + w, err := zw.Create(f.rel) if err != nil { - return "", "", fmt.Errorf("write zip entry: %w", err) + return fmt.Errorf("create zip entry %s: %w", f.rel, err) + } + // Capped in case the file grew between the walk and here, so a file that + // changes underneath us cannot defeat the limit checked above. + written, err := io.Copy(w, io.LimitReader(src, maxPackEntryBytes+1)) + if err != nil { + return fmt.Errorf("write zip entry %s: %w", f.rel, err) } if written > maxPackEntryBytes { - return "", "", fmt.Errorf( - "entry file %q exceeds maximum supported size of %d bytes (%d MiB)", - path, - maxPackEntryBytes, - maxPackEntryBytes>>20, + return fmt.Errorf( + "%s grew past the maximum for a single file (%s) while packing", + f.rel, humanBytes(maxPackEntryBytes), ) } - if err := zw.Close(); err != nil { - return "", "", fmt.Errorf("close zip: %w", err) - } - - return base64.StdEncoding.EncodeToString(buf.Bytes()), modelFile, nil + return nil } diff --git a/internal/cmd/serverless/pack_test.go b/internal/cmd/serverless/pack_test.go index 6201b46..2be9ec0 100644 --- a/internal/cmd/serverless/pack_test.go +++ b/internal/cmd/serverless/pack_test.go @@ -4,75 +4,508 @@ import ( "archive/zip" "bytes" "encoding/base64" + "io" "os" "path/filepath" + "runtime" + "sort" "strings" "testing" ) -func TestPackPythonFile(t *testing.T) { +// writeTree materialises files under dir. Keys are slash-separated relative +// paths; parent directories are created as needed. +func writeTree(t *testing.T, dir string, files map[string]string) { + t.Helper() + for rel, content := range files { + path := filepath.Join(dir, filepath.FromSlash(rel)) + if err := os.MkdirAll(filepath.Dir(path), 0o750); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatal(err) + } + } +} + +// unpack decodes an archive into path -> contents. +func unpack(t *testing.T, encoded string) map[string]string { + t.Helper() + raw, err := base64.StdEncoding.DecodeString(encoded) + if err != nil { + t.Fatalf("decode: %v", err) + } + zr, err := zip.NewReader(bytes.NewReader(raw), int64(len(raw))) + if err != nil { + t.Fatalf("zip.NewReader: %v", err) + } + + out := make(map[string]string, len(zr.File)) + for _, f := range zr.File { + rc, err := f.Open() + if err != nil { + t.Fatal(err) + } + content, err := io.ReadAll(rc) + _ = rc.Close() + if err != nil { + t.Fatal(err) + } + out[f.Name] = string(content) + } + return out +} + +// Repeated across the table-free tests below; goconst wants them named. +const ( + testModelFile = "app.py" + testPySource = "x = 1\n" +) + +func names(packed map[string]string) []string { + out := make([]string, 0, len(packed)) + for name := range packed { + out = append(out, name) + } + sort.Strings(out) + return out +} + +// TestPackDirectory proves the whole project ships, not just the entry file: +// nested modules and data files keep the relative paths the app imports them by, +// which is what the builder puts on PYTHONPATH. +func TestPackDirectory(t *testing.T) { dir := t.TempDir() - path := filepath.Join(dir, "app.py") - content := []byte("def predict():\n return 1\n") - if err := os.WriteFile(path, content, 0o600); err != nil { - t.Fatal(err) + writeTree(t, dir, map[string]string{ + testModelFile: "import lib\n", + "lib/__init__.py": "", + "lib/helpers.py": "def go(): pass\n", + "data/prompt.txt": "hello", + "nested/deep/x.py": testPySource, + }) + + encoded, modelFile, err := packDirectory(dir, filepath.Join(dir, testModelFile)) + if err != nil { + t.Fatalf("packDirectory: %v", err) + } + if modelFile != testModelFile { + t.Errorf("modelFile = %q, want app.py", modelFile) + } + + packed := unpack(t, encoded) + for _, want := range []string{testModelFile, "lib/helpers.py", "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"]) } +} + +// TestPackDirectory_DefaultsToWorkingDirectory covers `deploy ./app.py` with no +// --src-dir, which is the common invocation. +func TestPackDirectory_DefaultsToWorkingDirectory(t *testing.T) { + dir := t.TempDir() + writeTree(t, dir, map[string]string{ + testModelFile: testPySource, + "lib.py": "y = 2\n", + }) + t.Chdir(dir) - encoded, modelFile, err := packPythonFile(path) + encoded, modelFile, err := packDirectory("", testModelFile) if err != nil { - t.Fatalf("packPythonFile: %v", err) + t.Fatalf("packDirectory: %v", err) } - if modelFile != "app.py" { + if modelFile != testModelFile { t.Errorf("modelFile = %q, want app.py", modelFile) } + if got := names(unpack(t, encoded)); len(got) != 2 { + t.Errorf("archive = %v, want app.py and lib.py", got) + } +} - raw, err := base64.StdEncoding.DecodeString(encoded) +// TestPackDirectory_ModelFileInSubdirectory proves modelFile is reported as the +// path inside the archive, which is what the builder looks up. +func TestPackDirectory_ModelFileInSubdirectory(t *testing.T) { + dir := t.TempDir() + writeTree(t, dir, map[string]string{"src/app.py": testPySource}) + + _, modelFile, err := packDirectory(dir, filepath.Join(dir, "src", testModelFile)) if err != nil { - t.Fatalf("decode: %v", err) + t.Fatalf("packDirectory: %v", err) } - zr, err := zip.NewReader(bytes.NewReader(raw), int64(len(raw))) + if modelFile != "src/app.py" { + t.Errorf("modelFile = %q, want src/app.py", modelFile) + } +} + +// TestPackDirectory_ModelFileOutsideRoot fails locally rather than uploading an +// archive the builder will reject with a 422. +func TestPackDirectory_ModelFileOutsideRoot(t *testing.T) { + base := t.TempDir() + root := filepath.Join(base, "project") + writeTree(t, root, map[string]string{"keep.py": ""}) + writeTree(t, base, map[string]string{"outside.py": ""}) + + _, _, err := packDirectory(root, filepath.Join(base, "outside.py")) + if err == nil { + t.Fatal("expected an error for a model file outside the source directory") + } + if !strings.Contains(err.Error(), "--src-dir") { + t.Errorf("error %q does not say how to fix it", err) + } +} + +// TestPackDirectory_DefaultExcludes proves the built-in list keeps the usual +// noise out without any ignore file present. +func TestPackDirectory_DefaultExcludes(t *testing.T) { + dir := t.TempDir() + writeTree(t, dir, map[string]string{ + testModelFile: "", + "__pycache__/app.pyc": "", + "lib/__pycache__/mod.pyc": "", + "lib/mod.py": "", + ".venv/lib/python/x.py": "", + "node_modules/pkg/i.js": "", + ".git/config": "", + ".DS_Store": "", + }) + + encoded, _, err := packDirectory(dir, filepath.Join(dir, testModelFile)) if err != nil { - t.Fatalf("zip.NewReader: %v", err) + t.Fatalf("packDirectory: %v", err) + } + + got := names(unpack(t, encoded)) + want := []string{testModelFile, "lib/mod.py"} + if strings.Join(got, ",") != strings.Join(want, ",") { + t.Errorf("archive = %v, want %v", got, want) + } +} + +// TestPackDirectory_RunwareIgnore proves the project's own rules apply, that a +// negation re-includes a file an earlier rule excluded, and that comments and +// blank lines are ignored. +func TestPackDirectory_RunwareIgnore(t *testing.T) { + dir := t.TempDir() + writeTree(t, dir, map[string]string{ + testModelFile: "", + "outputs/result.png": "", + "notes.md": "", + "README.md": "", + "cached/x.pyc": "", + runwareIgnoreFile: "outputs/\n*.md\n\n# a comment\n!README.md\n", + }) + + encoded, _, err := packDirectory(dir, filepath.Join(dir, testModelFile)) + if err != nil { + t.Fatalf("packDirectory: %v", err) + } + + packed := unpack(t, encoded) + for _, absent := range []string{"outputs/result.png", "notes.md", runwareIgnoreFile, "cached/x.pyc"} { + if _, ok := packed[absent]; ok { + t.Errorf("%q should have been excluded; archive = %v", absent, names(packed)) + } + } + // A later ! rule wins over the earlier *.md that excluded it. + if _, ok := packed["README.md"]; !ok { + t.Errorf("a ! rule did not re-include README.md; archive = %v", names(packed)) + } +} + +// TestPackDirectory_NegationCannotReachIntoExcludedDirectory pins git's own rule: +// "It is not possible to re-include a file if a parent directory of that file is +// excluded." The packer prunes an excluded directory rather than walking it, so a +// negation inside one never applies -- the same result git gives, and the reason +// a .venv costs nothing to skip. +func TestPackDirectory_NegationCannotReachIntoExcludedDirectory(t *testing.T) { + dir := t.TempDir() + writeTree(t, dir, map[string]string{ + testModelFile: "", + "node_modules/keep.js": "", + runwareIgnoreFile: "!node_modules/keep.js\n", + }) + + encoded, _, err := packDirectory(dir, filepath.Join(dir, testModelFile)) + if err != nil { + t.Fatalf("packDirectory: %v", err) + } + if _, ok := unpack(t, encoded)["node_modules/keep.js"]; ok { + t.Error("a negation reached inside an excluded directory; git does not allow that") + } +} + +// TestPackDirectory_GitignoreFallback proves .gitignore is honoured when there is +// no .runwareignore, and ignored when there is one — the two must not union, or +// the result is impossible to reason about. +func TestPackDirectory_GitignoreFallback(t *testing.T) { + tree := map[string]string{ + testModelFile: "", + "secret.txt": "", + "build/o.so": "", + ".gitignore": "secret.txt\nbuild/\n", + } + + t.Run("used when no runwareignore", func(t *testing.T) { + dir := t.TempDir() + writeTree(t, dir, tree) + + encoded, _, err := packDirectory(dir, filepath.Join(dir, testModelFile)) + if err != nil { + t.Fatalf("packDirectory: %v", err) + } + packed := unpack(t, encoded) + if _, ok := packed["secret.txt"]; ok { + t.Errorf(".gitignore was not honoured; archive = %v", names(packed)) + } + if _, ok := packed["build/o.so"]; ok { + t.Errorf(".gitignore directory rule was not honoured; archive = %v", names(packed)) + } + }) + + t.Run("ignored when runwareignore exists", func(t *testing.T) { + dir := t.TempDir() + writeTree(t, dir, tree) + writeTree(t, dir, map[string]string{runwareIgnoreFile: "build/\n"}) + + encoded, _, err := packDirectory(dir, filepath.Join(dir, testModelFile)) + if err != nil { + t.Fatalf("packDirectory: %v", err) + } + packed := unpack(t, encoded) + // .runwareignore says nothing about secret.txt, so it ships even though + // .gitignore excludes it. + if _, ok := packed["secret.txt"]; !ok { + t.Errorf(".gitignore was still applied alongside .runwareignore; archive = %v", names(packed)) + } + if _, ok := packed["build/o.so"]; ok { + t.Errorf(".runwareignore rule was not honoured; archive = %v", names(packed)) + } + }) +} + +// TestPackDirectory_NeverPacksEnvFiles is the rule that must not be overridable: +// .env files hold credentials and the build pod unpacks whatever it is sent, so +// no ignore rule — not even an explicit negation — may re-include one. +func TestPackDirectory_NeverPacksEnvFiles(t *testing.T) { + dir := t.TempDir() + writeTree(t, dir, map[string]string{ + testModelFile: "", + ".env": "SECRET=1", + ".env.local": "SECRET=2", + ".env.production": "SECRET=3", + "config/.env": "SECRET=4", + "deep/nest/.env.ci": "SECRET=5", + "envoy.py": "not an env file", + ".runwareignore": "!.env\n!.env.*\n!config/.env\n", + }) + + encoded, _, err := packDirectory(dir, filepath.Join(dir, testModelFile)) + if err != nil { + t.Fatalf("packDirectory: %v", err) + } + + packed := unpack(t, encoded) + for name, content := range packed { + if strings.Contains(content, "SECRET=") { + t.Errorf("%q carries an env secret into the archive", name) + } } - if len(zr.File) != 1 { - t.Fatalf("expected 1 zip entry, got %d", len(zr.File)) + for _, absent := range []string{".env", ".env.local", ".env.production", "config/.env", "deep/nest/.env.ci"} { + if _, ok := packed[absent]; ok { + t.Errorf("%q was packed despite the absolute exclusion; archive = %v", absent, names(packed)) + } } - if zr.File[0].Name != "app.py" { - t.Errorf("zip entry name = %q, want app.py", zr.File[0].Name) + // The rule matches env files, not every name that starts with "env". + if _, ok := packed["envoy.py"]; !ok { + t.Errorf("envoy.py was wrongly treated as an env file; archive = %v", names(packed)) } +} + +// TestPackDirectory_NeverPacksGitDirectory is the second half of the absolute +// exclusions: .git holds the whole history of everything the other rules exclude +// -- including any .env ever committed -- so no rule may re-include it, and a +// nested repo one directory down is the same problem. +func TestPackDirectory_NeverPacksGitDirectory(t *testing.T) { + dir := t.TempDir() + writeTree(t, dir, map[string]string{ + testModelFile: "", + ".git/config": "[core]", + ".git/objects/ab/cdef": "history", + "vendor/dep/.git/config": "[core]", + "gitignore-notes.md": "not a git directory", + runwareIgnoreFile: "!.git\n!.git/**\n!vendor/dep/.git/**\n", + }) - rc, err := zr.File[0].Open() + encoded, _, err := packDirectory(dir, testModelFile) if err != nil { + t.Fatalf("packDirectory: %v", err) + } + + packed := unpack(t, encoded) + for name := range packed { + if strings.Contains(name, ".git/") { + t.Errorf("%q was packed despite the absolute exclusion; archive = %v", name, names(packed)) + } + } + // The rule matches the directory, not every name containing "git". + if _, ok := packed["gitignore-notes.md"]; !ok { + t.Errorf("gitignore-notes.md was wrongly treated as a git directory; archive = %v", names(packed)) + } +} + +// TestPackDirectory_ModelFileAlwaysPacked proves an ignore rule covering the +// entry file cannot produce an archive the build is guaranteed to reject. +func TestPackDirectory_ModelFileAlwaysPacked(t *testing.T) { + dir := t.TempDir() + writeTree(t, dir, map[string]string{ + testModelFile: "x = 1\n", + runwareIgnoreFile: "*.py\n", + }) + + encoded, modelFile, err := packDirectory(dir, filepath.Join(dir, testModelFile)) + if err != nil { + t.Fatalf("packDirectory: %v", err) + } + packed := unpack(t, encoded) + if _, ok := packed[modelFile]; !ok { + t.Errorf("the model file was excluded by an ignore rule; archive = %v", names(packed)) + } +} + +// 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" { + t.Skip("symlink creation needs privileges on Windows") + } + dir := t.TempDir() + writeTree(t, dir, map[string]string{testModelFile: ""}) + outside := filepath.Join(t.TempDir(), "outside.txt") + if err := os.WriteFile(outside, []byte("elsewhere"), 0o600); err != nil { t.Fatal(err) } - defer rc.Close() //nolint:errcheck - got := new(bytes.Buffer) - if _, err := got.ReadFrom(rc); err != nil { + if err := os.Symlink(outside, filepath.Join(dir, "link.txt")); err != nil { t.Fatal(err) } - if !bytes.Equal(got.Bytes(), content) { - t.Errorf("zip contents = %q, want %q", got.Bytes(), content) + + encoded, _, err := packDirectory(dir, filepath.Join(dir, testModelFile)) + if err != nil { + t.Fatalf("packDirectory: %v", err) + } + if _, ok := unpack(t, encoded)["link.txt"]; ok { + t.Error("a symlink was followed into the archive") } } -func TestPackPythonFile_Missing(t *testing.T) { - _, _, err := packPythonFile(filepath.Join(t.TempDir(), "missing.py")) +// TestPackDirectory_Deterministic: the same tree must pack to the same bytes, so +// a redeploy of unchanged source is visibly unchanged. +func TestPackDirectory_Deterministic(t *testing.T) { + dir := t.TempDir() + writeTree(t, dir, map[string]string{ + testModelFile: "", + "b/two.py": "", + "a/one.py": "", + }) + + first, _, err := packDirectory(dir, filepath.Join(dir, testModelFile)) + if err != nil { + t.Fatalf("packDirectory: %v", err) + } + second, _, err := packDirectory(dir, filepath.Join(dir, testModelFile)) + if err != nil { + t.Fatalf("packDirectory: %v", err) + } + if first != second { + t.Error("packing the same tree twice produced different archives") + } +} + +// TestPackDirectory_ModelFileRelativeToSrcDir is the invocation the --src-dir +// flag exists for: name the entry file as it sits inside the project, from +// wherever you happen to be standing. +func TestPackDirectory_ModelFileRelativeToSrcDir(t *testing.T) { + project := t.TempDir() + writeTree(t, project, map[string]string{testModelFile: testPySource}) + t.Chdir(t.TempDir()) // stand somewhere else entirely + + _, modelFile, err := packDirectory(project, testModelFile) + if err != nil { + t.Fatalf("packDirectory: %v", err) + } + if modelFile != testModelFile { + t.Errorf("modelFile = %q, want %q", modelFile, testModelFile) + } +} + +// TestPackDirectory_ModelFileAbsolute: an absolute path is taken as given, which +// is the other half of the rule and what shell completion produces when the +// project is somewhere else. +func TestPackDirectory_ModelFileAbsolute(t *testing.T) { + project := t.TempDir() + writeTree(t, project, map[string]string{"src/" + testModelFile: testPySource}) + t.Chdir(t.TempDir()) + + _, modelFile, err := packDirectory(project, filepath.Join(project, "src", testModelFile)) + if err != nil { + t.Fatalf("packDirectory: %v", err) + } + if modelFile != "src/"+testModelFile { + t.Errorf("modelFile = %q, want src/%s", modelFile, testModelFile) + } +} + +// TestPackDirectory_RelativeModelFileIsNotWorkingDirRelative pins the rule that +// makes the two above unambiguous: a relative path is resolved inside the source +// directory and nowhere else, so a file that exists in the working directory but +// not in the project is an error rather than a silently different app. +func TestPackDirectory_RelativeModelFileIsNotWorkingDirRelative(t *testing.T) { + project := t.TempDir() + writeTree(t, project, map[string]string{"real.py": testPySource}) + + elsewhere := t.TempDir() + writeTree(t, elsewhere, map[string]string{testModelFile: "the wrong file\n"}) + t.Chdir(elsewhere) + + _, _, err := packDirectory(project, testModelFile) + if err == nil { + t.Fatal("expected an error: the model file exists in the working directory, not the source directory") + } + if !strings.Contains(err.Error(), "source directory") { + t.Errorf("error %q does not explain where relative paths resolve", err) + } +} + +func TestPackDirectory_MissingModelFile(t *testing.T) { + dir := t.TempDir() + _, _, err := packDirectory(dir, filepath.Join(dir, "missing.py")) + if err == nil { + t.Fatal("expected an error for a missing model file") + } +} + +func TestPackDirectory_MissingSourceDirectory(t *testing.T) { + _, _, err := packDirectory(filepath.Join(t.TempDir(), "nope"), testModelFile) if err == nil { - t.Fatal("expected error for missing file") + t.Fatal("expected an error for a missing source directory") } } -func TestPackPythonFile_Directory(t *testing.T) { - _, _, err := packPythonFile(t.TempDir()) +func TestPackDirectory_ModelFileIsDirectory(t *testing.T) { + dir := t.TempDir() + writeTree(t, dir, map[string]string{"pkg/app.py": ""}) + _, _, err := packDirectory(dir, filepath.Join(dir, "pkg")) if err == nil { - t.Fatal("expected error for directory") + t.Fatal("expected an error when the model file is a directory") } } -func TestPackPythonFile_TooLarge(t *testing.T) { +func TestPackDirectory_FileTooLarge(t *testing.T) { dir := t.TempDir() - path := filepath.Join(dir, "huge.py") - f, err := os.Create(path) + writeTree(t, dir, map[string]string{testModelFile: ""}) + f, err := os.Create(filepath.Join(dir, "huge.bin")) if err != nil { t.Fatal(err) } @@ -84,11 +517,40 @@ func TestPackPythonFile_TooLarge(t *testing.T) { t.Fatal(err) } - _, _, err = packPythonFile(path) + _, _, err = packDirectory(dir, filepath.Join(dir, testModelFile)) + if err == nil { + t.Fatal("expected an error for an oversized file") + } + // The message has to name the file and the remedy, or the caller is left + // hunting through a deep tree for whichever file it meant. + if !strings.Contains(err.Error(), "huge.bin") || !strings.Contains(err.Error(), runwareIgnoreFile) { + t.Errorf("error %q does not name the file and the remedy", err) + } +} + +func TestPackDirectory_TotalTooLarge(t *testing.T) { + dir := t.TempDir() + writeTree(t, dir, map[string]string{testModelFile: ""}) + // Several files, each under the per-file cap, over the total. + for _, name := range []string{"a.bin", "b.bin", "c.bin"} { + f, err := os.Create(filepath.Join(dir, name)) + if err != nil { + t.Fatal(err) + } + if err := f.Truncate(maxPackEntryBytes); err != nil { + _ = f.Close() + t.Fatal(err) + } + if err := f.Close(); err != nil { + t.Fatal(err) + } + } + + _, _, err := packDirectory(dir, filepath.Join(dir, testModelFile)) if err == nil { - t.Fatal("expected error for oversized entry file") + t.Fatal("expected an error for an oversized archive") } - if !strings.Contains(err.Error(), "maximum supported size") { - t.Errorf("error %q does not mention size limit", err) + if !strings.Contains(err.Error(), "Largest files") { + t.Errorf("error %q does not say what filled the archive", err) } } From 3938576b66be951e8b95ce2b1bde7cd95b285f76 Mon Sep 17 00:00:00 2001 From: D1-3105 Date: Thu, 20 Aug 2026 23:19:39 +0000 Subject: [PATCH 2/6] feat(serverless): add --volume for persistent node-local storage The public API has taken a `volumes` array on app create for a while and the deployer renders each entry as a hostPath mount, but the CLI had no way to say so -- which made it unusable for any app that downloads weights. Anything fetched at runtime belongs on a volume: the app runs in a sandbox whose filesystem is part of the checkpointed state, so an unmounted download is copied into every checkpoint AND re-fetched on every cold start. --volume takes an absolute path and repeats. The path is the volume's whole identity -- there is no name, because the path is what the app opens and what the node-local directory is keyed by -- so two entries resolving to the same place are a mistake rather than a merge. buildVolumes mirrors the server's checks locally: absolute, not root, within the length limits, allowed characters, no duplicates, no overlaps, at most 30. The duplication is deliberate and policy.go gives the reason for its own copy: a code deployment can build for up to ninety minutes, and learning only then that two mounts overlap wastes all of it. A shared prefix is not an overlap -- /data/weights-old sits beside /data/weights, not inside it. --- docs/runware_serverless_deploy.md | 10 ++ internal/api/serverless/client.go | 3 + internal/cmd/serverless/deploy.go | 17 ++++ internal/cmd/serverless/volume.go | 94 ++++++++++++++++++ internal/cmd/serverless/volume_test.go | 128 +++++++++++++++++++++++++ 5 files changed, 252 insertions(+) create mode 100644 internal/cmd/serverless/volume.go create mode 100644 internal/cmd/serverless/volume_test.go diff --git a/docs/runware_serverless_deploy.md b/docs/runware_serverless_deploy.md index 2a8cbb1..56d98aa 100644 --- a/docs/runware_serverless_deploy.md +++ b/docs/runware_serverless_deploy.md @@ -18,6 +18,11 @@ source directory; it takes gitignore syntax. Without one, a .gitignore is used instead. Either way .env files are never uploaded, and neither are .git, __pycache__, .venv or node_modules. +Anything the app downloads at runtime belongs on a --volume. The app runs in a +sandbox whose filesystem is part of the checkpointed state, so an unmounted +download is copied into every checkpoint and fetched again on every cold start. +A volume keeps it out of both. + Worker settings are supplied via flags (a local project config via 'runware serverless init' is planned). Endpoints are derived server-side from the SDK. @@ -37,6 +42,10 @@ runware serverless deploy [flags] # an entry file in a subdirectory of the project runware serverless deploy src/app.py --src-dir ~/projects/my-app --id my-app --gpu-type h100 + # keep downloaded model weights on persistent node-local storage + runware serverless deploy model.py --id my-app --gpu-type l40s \ + --volume /root/.cache/huggingface + # override worker settings and base image runware serverless deploy ./app.py --id my-app --name "My App" \ --max-workers 2 --idle-ttl 120 --gpu-type h100 \ @@ -58,6 +67,7 @@ runware serverless deploy [flags] --requirement stringArray Additional pip package to install (repeatable) --scaling-delay int32 Scaling delay in seconds (default 10) --src-dir string Directory to package as the application source (default: the working directory) + --volume stringArray Absolute path inside the app backed by persistent node-local storage (repeatable) ``` ### Options inherited from parent commands diff --git a/internal/api/serverless/client.go b/internal/api/serverless/client.go index 4638269..23eda55 100644 --- a/internal/api/serverless/client.go +++ b/internal/api/serverless/client.go @@ -46,6 +46,9 @@ type CodeSourceUpsert = gen.CodeSourceUpsert // CodebaseSource is the zipped customer code payload. type CodebaseSource = gen.CodebaseSource +// AppVolume is a persistent node-local directory mounted into the application. +type AppVolume = gen.AppVolume + // WorkerConfig is the live worker configuration on an app. type WorkerConfig = gen.WorkerConfig diff --git a/internal/cmd/serverless/deploy.go b/internal/cmd/serverless/deploy.go index b10d72b..9140234 100644 --- a/internal/cmd/serverless/deploy.go +++ b/internal/cmd/serverless/deploy.go @@ -25,6 +25,7 @@ func newDeployCmd(logger *log.Logger) *cobra.Command { minWorkers int32 gpusPerWorker int32 srcDir string + volumes []string ) cmd := &cobra.Command{ @@ -44,6 +45,11 @@ source directory; it takes gitignore syntax. Without one, a .gitignore is used instead. Either way .env files are never uploaded, and neither are .git, __pycache__, .venv or node_modules. +Anything the app downloads at runtime belongs on a --volume. The app runs in a +sandbox whose filesystem is part of the checkpointed state, so an unmounted +download is copied into every checkpoint and fetched again on every cold start. +A volume keeps it out of both. + Worker settings are supplied via flags (a local project config via 'runware serverless init' is planned). Endpoints are derived server-side from the SDK.`, Example: ` # deploy the current directory, with app.py as the entry point @@ -55,6 +61,10 @@ serverless init' is planned). Endpoints are derived server-side from the SDK.`, # an entry file in a subdirectory of the project runware serverless deploy src/app.py --src-dir ~/projects/my-app --id my-app --gpu-type h100 + # keep downloaded model weights on persistent node-local storage + runware serverless deploy model.py --id my-app --gpu-type l40s \ + --volume /root/.cache/huggingface + # override worker settings and base image runware serverless deploy ./app.py --id my-app --name "My App" \ --max-workers 2 --idle-ttl 120 --gpu-type h100 \ @@ -71,6 +81,11 @@ serverless init' is planned). Endpoints are derived server-side from the SDK.`, return err } + appVolumes, err := buildVolumes(volumes) + if err != nil { + return err + } + source, err := serverlessapi.NewCodeAppSource(serverlessapi.CodeSourceUpsert{ BaseImage: baseImage, Codebase: serverlessapi.CodebaseSource{ @@ -87,6 +102,7 @@ serverless init' is planned). Endpoints are derived server-side from the SDK.`, AppId: id, AppName: name, AppSource: source, + Volumes: appVolumes, Configuration: serverlessapi.WorkerConfigCreate{ MaxWorkers: maxWorkers, IdleTtlSecs: idleTTL, @@ -113,6 +129,7 @@ serverless init' is planned). Endpoints are derived server-side from the SDK.`, } cmd.Flags().StringVar(&srcDir, "src-dir", "", "Directory to package as the application source (default: the working directory)") + cmd.Flags().StringArrayVar(&volumes, "volume", nil, "Absolute path inside the app backed by persistent node-local storage (repeatable)") cmd.Flags().StringVar(&id, "id", "", "Application ID (immutable, lowercase slug)") cmd.Flags().StringVar(&name, "name", "", "Display name (defaults to --id)") cmd.Flags().Int32Var(&maxWorkers, "max-workers", 1, "Maximum number of workers") diff --git a/internal/cmd/serverless/volume.go b/internal/cmd/serverless/volume.go new file mode 100644 index 0000000..c6c9034 --- /dev/null +++ b/internal/cmd/serverless/volume.go @@ -0,0 +1,94 @@ +package serverless + +import ( + "fmt" + "path" + "strings" + + serverlessapi "github.com/runware/runware-cli/internal/api/serverless" +) + +// Volume limits, mirrored from the server. A local copy is worth its upkeep: a +// code deployment can build for up to ninety minutes, and learning only then +// that two mounts overlap is the whole of that wait wasted. The server validates +// every request independently regardless -- this is a courtesy, not a boundary. +const ( + maxVolumes = 30 + maxVolumeMountPathLen = 2048 + maxVolumePathComponent = 255 + // The characters the server's mountPath pattern allows besides letters and + // digits: ^/[A-Za-z0-9._/+@-]+$ + volumeMountPathExtra = "._-/+@" +) + +// buildVolumes turns --volume flags into the wire type, rejecting anything the +// server would reject. +// +// The mount path is the volume's whole identity: there is no name, because the +// path is what the app opens and what the node-local directory is keyed by. So +// nothing here needs a second field, and two entries that resolve to the same +// place are a mistake rather than a merge. +func buildVolumes(mountPaths []string) (*[]serverlessapi.AppVolume, error) { + if len(mountPaths) == 0 { + return nil, nil + } + if len(mountPaths) > maxVolumes { + return nil, fmt.Errorf("at most %d volumes (got %d)", maxVolumes, len(mountPaths)) + } + + volumes := make([]serverlessapi.AppVolume, 0, len(mountPaths)) + seen := make([]string, 0, len(mountPaths)) + for _, raw := range mountPaths { + mount, err := validateMountPath(raw, seen) + if err != nil { + return nil, err + } + seen = append(seen, mount) + volumes = append(volumes, serverlessapi.AppVolume{MountPath: mount}) + } + return &volumes, nil +} + +// validateMountPath returns the cleaned path, or an error naming what is wrong +// with it. `seen` holds the already-accepted paths, cleaned. +func validateMountPath(raw string, seen []string) (string, error) { + // path.Clean, not filepath.Clean: this is a path inside the Linux sandbox + // the app runs in, whatever the machine the CLI runs on. + mount := path.Clean(raw) + + switch { + case !path.IsAbs(mount): + return "", fmt.Errorf("volume %q: must be an absolute path", raw) + case mount == "/": + return "", fmt.Errorf("volume %q: must not be the root directory", raw) + case len(mount) > maxVolumeMountPathLen: + return "", fmt.Errorf("volume %q: exceeds the %d character limit", raw, maxVolumeMountPathLen) + } + + for _, r := range mount { + if (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || + (r >= '0' && r <= '9') || strings.ContainsRune(volumeMountPathExtra, r) { + continue + } + return "", fmt.Errorf("volume %q: contains unsupported character %q", raw, r) + } + + for _, component := range strings.Split(strings.TrimPrefix(mount, "/"), "/") { + if len(component) > maxVolumePathComponent { + return "", fmt.Errorf("volume %q: path component exceeds %d bytes", raw, maxVolumePathComponent) + } + } + + // Overlap, not just duplication: two volumes where one contains the other + // would bind-mount the same node directory into the sandbox twice, and there + // is no answer to which one owns the shared subtree. + for _, previous := range seen { + if previous == mount { + return "", fmt.Errorf("volume %q is listed twice", mount) + } + if strings.HasPrefix(mount, previous+"/") || strings.HasPrefix(previous, mount+"/") { + return "", fmt.Errorf("volume %q overlaps %q; declare only the outer path", mount, previous) + } + } + return mount, nil +} diff --git a/internal/cmd/serverless/volume_test.go b/internal/cmd/serverless/volume_test.go new file mode 100644 index 0000000..0752875 --- /dev/null +++ b/internal/cmd/serverless/volume_test.go @@ -0,0 +1,128 @@ +package serverless + +import ( + "strings" + "testing" +) + +// A representative mount path, repeated across the cases below. +const testMountPath = "/data/weights" + +func TestBuildVolumes(t *testing.T) { + got, err := buildVolumes([]string{"/root/.cache/huggingface", testMountPath}) + if err != nil { + t.Fatalf("buildVolumes: %v", err) + } + if got == nil || len(*got) != 2 { + t.Fatalf("expected 2 volumes, got %v", got) + } + if (*got)[0].MountPath != "/root/.cache/huggingface" { + t.Errorf("mountPath = %q", (*got)[0].MountPath) + } +} + +// No volumes must send nil rather than an empty array: the field is optional and +// an explicit [] is a different statement from saying nothing. +func TestBuildVolumes_NoneIsNil(t *testing.T) { + got, err := buildVolumes(nil) + if err != nil { + t.Fatalf("buildVolumes: %v", err) + } + if got != nil { + t.Errorf("expected nil for no volumes, got %v", *got) + } +} + +// The path is cleaned before it travels, so the server sees one spelling of a +// path however it was typed -- and so the overlap check below compares like +// with like. +func TestBuildVolumes_CleansPaths(t *testing.T) { + got, err := buildVolumes([]string{"/data//weights/"}) + if err != nil { + t.Fatalf("buildVolumes: %v", err) + } + if (*got)[0].MountPath != testMountPath { + t.Errorf("mountPath = %q, want /data/weights", (*got)[0].MountPath) + } +} + +func TestBuildVolumes_Rejects(t *testing.T) { + cases := []struct { + name string + paths []string + want string + }{ + { + name: "relative", + paths: []string{"cache/weights"}, + want: "absolute", + }, + { + name: "root", + paths: []string{"/"}, + want: "root directory", + }, + { + name: "unsupported character", + paths: []string{"/data/we ights"}, + want: "unsupported character", + }, + { + // The same path twice is a mistake, not a request for one volume. + name: "duplicate", + paths: []string{testMountPath, testMountPath}, + want: "listed twice", + }, + { + // The pair that motivates the whole check: one contains the other, + // so the same node directory would be mounted into the sandbox twice. + name: "overlapping", + paths: []string{"/data", testMountPath}, + want: "overlaps", + }, + { + // ...in either order. + name: "overlapping reversed", + paths: []string{testMountPath, "/data"}, + want: "overlaps", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + _, err := buildVolumes(tc.paths) + if err == nil { + t.Fatalf("expected an error for %v", tc.paths) + } + if !strings.Contains(err.Error(), tc.want) { + t.Errorf("error %q does not mention %q", err, tc.want) + } + }) + } +} + +// A shared prefix is not containment: /data/weights-old sits beside +// /data/weights rather than inside it, so both are legitimate. +func TestBuildVolumes_SharedPrefixIsNotOverlap(t *testing.T) { + got, err := buildVolumes([]string{testMountPath, "/data/weights-old"}) + if err != nil { + t.Fatalf("buildVolumes rejected sibling paths: %v", err) + } + if len(*got) != 2 { + t.Errorf("expected both volumes, got %v", *got) + } +} + +func TestBuildVolumes_TooMany(t *testing.T) { + paths := make([]string, maxVolumes+1) + for i := range paths { + paths[i] = "/data/vol" + string(rune('a'+i%26)) + string(rune('a'+i/26)) + } + _, err := buildVolumes(paths) + if err == nil { + t.Fatal("expected an error past the volume limit") + } + if !strings.Contains(err.Error(), "at most") { + t.Errorf("error %q does not mention the limit", err) + } +} From 443aaf77e07c4e4af0a3303f35adca029dad57d5 Mon Sep 17 00:00:00 2001 From: D1-3105 Date: Fri, 21 Aug 2026 00:49:34 +0000 Subject: [PATCH 3/6] feat(serverless): add --env and --env-file to deploy An app's environment is frozen into the version snapshot that create mints, and that snapshot is what the deployer renders the worker from. Nothing produces a second version -- /versions and /builds are GET-only, and POST /deploy says outright that it creates no version and re-applies the existing image -- so a variable set through the /environment-variables endpoints after the app exists is stored, listed back by `apps env set`/`env list`, and never reaches a pod. Verified against a live app: version 1's snapshot held environmentVariables {} one second before HF_TOKEN was set, a redeploy of that version logged "redeploy completed", and the workload still had no HF_TOKEN on it. So the create request is the only route into a worker, and the CLI had no way to populate it. --env takes KEY=VALUE and repeats; only the first separator splits, because a value may legitimately contain '=' (base64 padding, a DSN). --env-file exists because the argv form cannot hold a secret: a value passed as --env is visible in the process list to every other user on the machine for as long as the call runs, and the shell records it in history. It reads KEY=VALUE lines, skips blanks and comments, and absorbs a leading `export ` so a shell-sourced file can be pasted in as-is. An inline --env wins over a file entry of the same name, being the more specific statement. Names are validated against the server's EnvironmentVariableName rule so a name this accepts is one the API can store, rather than a 422 after the archive has already been uploaded. --- docs/runware_serverless_deploy.md | 12 ++ internal/cmd/serverless/deploy.go | 28 ++++- internal/cmd/serverless/envvar.go | 114 +++++++++++++++++ internal/cmd/serverless/envvar_test.go | 165 +++++++++++++++++++++++++ 4 files changed, 315 insertions(+), 4 deletions(-) create mode 100644 internal/cmd/serverless/envvar.go create mode 100644 internal/cmd/serverless/envvar_test.go diff --git a/docs/runware_serverless_deploy.md b/docs/runware_serverless_deploy.md index 56d98aa..26aa273 100644 --- a/docs/runware_serverless_deploy.md +++ b/docs/runware_serverless_deploy.md @@ -18,6 +18,12 @@ source directory; it takes gitignore syntax. Without one, a .gitignore is used instead. Either way .env files are never uploaded, and neither are .git, __pycache__, .venv or node_modules. +Environment variables must be supplied here with --env or --env-file. An app's +environment is frozen into the version this command creates, which is what the +worker is rendered from, so setting one afterwards with 'apps env set' stores it +without it ever reaching a pod. Prefer --env-file for anything secret: a value +passed as --env is visible in the process list and recorded in shell history. + Anything the app downloads at runtime belongs on a --volume. The app runs in a sandbox whose filesystem is part of the checkpointed state, so an unmounted download is copied into every checkpoint and fetched again on every cold start. @@ -42,6 +48,10 @@ runware serverless deploy [flags] # an entry file in a subdirectory of the project runware serverless deploy src/app.py --src-dir ~/projects/my-app --id my-app --gpu-type h100 + # pass a token to the worker without putting it in the process list + printf 'HF_TOKEN=%s' "$token" > .env.deploy + runware serverless deploy model.py --id my-app --gpu-type l40s --env-file .env.deploy + # keep downloaded model weights on persistent node-local storage runware serverless deploy model.py --id my-app --gpu-type l40s \ --volume /root/.cache/huggingface @@ -56,6 +66,8 @@ runware serverless deploy [flags] ``` --base-image string Builder base image (default "python:3.11-slim") + --env stringArray Environment variable as KEY=VALUE (repeatable) + --env-file stringArray File of KEY=VALUE lines to read environment variables from (repeatable) --gpu-type string GPU type ID (see 'serverless gpus') --gpus-per-worker int32 GPUs allocated per worker (default 1) -h, --help help for deploy diff --git a/internal/cmd/serverless/deploy.go b/internal/cmd/serverless/deploy.go index 9140234..bb55b26 100644 --- a/internal/cmd/serverless/deploy.go +++ b/internal/cmd/serverless/deploy.go @@ -26,6 +26,8 @@ func newDeployCmd(logger *log.Logger) *cobra.Command { gpusPerWorker int32 srcDir string volumes []string + envVars []string + envFiles []string ) cmd := &cobra.Command{ @@ -45,6 +47,12 @@ source directory; it takes gitignore syntax. Without one, a .gitignore is used instead. Either way .env files are never uploaded, and neither are .git, __pycache__, .venv or node_modules. +Environment variables must be supplied here with --env or --env-file. An app's +environment is frozen into the version this command creates, which is what the +worker is rendered from, so setting one afterwards with 'apps env set' stores it +without it ever reaching a pod. Prefer --env-file for anything secret: a value +passed as --env is visible in the process list and recorded in shell history. + Anything the app downloads at runtime belongs on a --volume. The app runs in a sandbox whose filesystem is part of the checkpointed state, so an unmounted download is copied into every checkpoint and fetched again on every cold start. @@ -61,6 +69,10 @@ serverless init' is planned). Endpoints are derived server-side from the SDK.`, # an entry file in a subdirectory of the project runware serverless deploy src/app.py --src-dir ~/projects/my-app --id my-app --gpu-type h100 + # pass a token to the worker without putting it in the process list + printf 'HF_TOKEN=%s' "$token" > .env.deploy + runware serverless deploy model.py --id my-app --gpu-type l40s --env-file .env.deploy + # keep downloaded model weights on persistent node-local storage runware serverless deploy model.py --id my-app --gpu-type l40s \ --volume /root/.cache/huggingface @@ -86,6 +98,11 @@ serverless init' is planned). Endpoints are derived server-side from the SDK.`, return err } + appEnv, err := buildEnvironmentVariables(envFiles, envVars) + if err != nil { + return err + } + source, err := serverlessapi.NewCodeAppSource(serverlessapi.CodeSourceUpsert{ BaseImage: baseImage, Codebase: serverlessapi.CodebaseSource{ @@ -99,10 +116,11 @@ serverless init' is planned). Endpoints are derived server-side from the SDK.`, } body := serverlessapi.AppCreate{ - AppId: id, - AppName: name, - AppSource: source, - Volumes: appVolumes, + AppId: id, + AppName: name, + AppSource: source, + Volumes: appVolumes, + EnvironmentVariables: appEnv, Configuration: serverlessapi.WorkerConfigCreate{ MaxWorkers: maxWorkers, IdleTtlSecs: idleTTL, @@ -130,6 +148,8 @@ serverless init' is planned). Endpoints are derived server-side from the SDK.`, cmd.Flags().StringVar(&srcDir, "src-dir", "", "Directory to package as the application source (default: the working directory)") cmd.Flags().StringArrayVar(&volumes, "volume", nil, "Absolute path inside the app backed by persistent node-local storage (repeatable)") + cmd.Flags().StringArrayVar(&envVars, "env", nil, "Environment variable as KEY=VALUE (repeatable)") + cmd.Flags().StringArrayVar(&envFiles, "env-file", nil, "File of KEY=VALUE lines to read environment variables from (repeatable)") cmd.Flags().StringVar(&id, "id", "", "Application ID (immutable, lowercase slug)") cmd.Flags().StringVar(&name, "name", "", "Display name (defaults to --id)") cmd.Flags().Int32Var(&maxWorkers, "max-workers", 1, "Maximum number of workers") diff --git a/internal/cmd/serverless/envvar.go b/internal/cmd/serverless/envvar.go new file mode 100644 index 0000000..525d807 --- /dev/null +++ b/internal/cmd/serverless/envvar.go @@ -0,0 +1,114 @@ +package serverless + +import ( + "fmt" + "os" + "regexp" + "strings" +) + +// Environment variable limits, mirrored from the server's EnvironmentVariableName +// and its deployment_configs column CHECK. +const ( + maxEnvVars = 100 + maxEnvNameLen = 128 + maxEnvValueLen = 4096 + envAssignSuffix = "=VALUE" +) + +// envNamePattern is the server's EnvironmentVariableName rule: POSIX-style, so a +// name this accepts is a name the API can store rather than one it rejects after +// the archive has already been uploaded. +var envNamePattern = regexp.MustCompile(`^[A-Za-z_][A-Za-z0-9_]{0,127}$`) + +// buildEnvironmentVariables turns --env KEY=VALUE pairs and --env-file paths into +// the create request's map. +// +// These belong on the CREATE request and nowhere else: an app's environment is +// frozen into its version snapshot, which is what the deployer renders from, and +// no endpoint creates a further version -- `deploy` re-applies an existing one by +// number and says so. So a variable set through the /environment-variables +// endpoints after the app exists is stored, listed back, and never reaches a +// worker. Passing it here is the only route that ends up in a pod. +// +// Files are read before the inline pairs are applied, so an explicit --env 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 { + return nil, nil + } + + env := make(map[string]string) + for _, path := range files { + if err := readEnvFile(path, env); err != nil { + return nil, err + } + } + for _, pair := range pairs { + name, value, err := splitEnvAssignment(pair) + if err != nil { + return nil, err + } + env[name] = value + } + + if len(env) > maxEnvVars { + return nil, fmt.Errorf("at most %d environment variables (got %d)", maxEnvVars, len(env)) + } + return &env, nil +} + +// readEnvFile reads KEY=VALUE lines into env, skipping blanks and # comments. +// +// The point of a file is that a secret never reaches an argv: a value passed as +// --env is visible in the process list to every other user on the machine for as +// long as the command runs, and the shell records it in history. +func readEnvFile(path string, env map[string]string) error { + raw, err := os.ReadFile(path) + if err != nil { + return fmt.Errorf("read env file: %w", err) + } + for i, line := range strings.Split(string(raw), "\n") { + trimmed := strings.TrimSpace(line) + if trimmed == "" || strings.HasPrefix(trimmed, "#") { + continue + } + // `export FOO=bar` is what a shell-sourced file looks like, and pasting one + // in is the obvious mistake to absorb rather than reject. + trimmed = strings.TrimPrefix(trimmed, "export ") + name, value, err := splitEnvAssignment(trimmed) + if err != nil { + return fmt.Errorf("%s line %d: %w", path, i+1, err) + } + env[name] = value + } + return nil +} + +// splitEnvAssignment parses one KEY=VALUE, validating the name and value against +// the limits the server enforces. +func splitEnvAssignment(assignment string) (name, value string, err error) { + name, value, found := strings.Cut(assignment, "=") + if !found { + return "", "", fmt.Errorf("%q is not KEY%s", assignment, envAssignSuffix) + } + // The name is trimmed but the value is not: trailing whitespace in a value can + // be deliberate, and a token with a stray newline is a 401 the app cannot + // explain -- so callers pass values through a file rather than have this guess. + name = strings.TrimSpace(name) + + switch { + case name == "": + return "", "", fmt.Errorf("%q has an empty name", assignment) + case len(name) > maxEnvNameLen: + return "", "", fmt.Errorf("environment variable name %q exceeds %d characters", name, maxEnvNameLen) + case !envNamePattern.MatchString(name): + return "", "", fmt.Errorf( + "environment variable name %q must be POSIX-style: letters, digits and underscore, not starting with a digit", + name, + ) + case len(value) > maxEnvValueLen: + return "", "", fmt.Errorf("value for %q exceeds %d characters", name, maxEnvValueLen) + } + return name, value, nil +} diff --git a/internal/cmd/serverless/envvar_test.go b/internal/cmd/serverless/envvar_test.go new file mode 100644 index 0000000..a160c27 --- /dev/null +++ b/internal/cmd/serverless/envvar_test.go @@ -0,0 +1,165 @@ +package serverless + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +func TestBuildEnvironmentVariables(t *testing.T) { + got, err := buildEnvironmentVariables(nil, []string{"HF_TOKEN=abc", "LOG_LEVEL=debug"}) + if err != nil { + t.Fatalf("buildEnvironmentVariables: %v", err) + } + if got == nil || len(*got) != 2 || (*got)["HF_TOKEN"] != "abc" { + t.Errorf("got %v", got) + } +} + +// 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) + if err != nil { + t.Fatalf("buildEnvironmentVariables: %v", err) + } + if got != nil { + t.Errorf("expected nil, got %v", *got) + } +} + +// 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="}) + if err != nil { + t.Fatalf("buildEnvironmentVariables: %v", err) + } + if (*got)["TOKEN"] != "abc==def=" { + t.Errorf("value = %q", (*got)["TOKEN"]) + } +} + +// 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="}) + if err != nil { + t.Fatalf("buildEnvironmentVariables: %v", err) + } + if v, ok := (*got)["QUIET"]; !ok || v != "" { + t.Errorf("got %v", *got) + } +} + +func TestBuildEnvironmentVariables_Rejects(t *testing.T) { + cases := []struct { + name string + pair string + want string + }{ + {name: "no separator", pair: "HF_TOKEN", want: "not KEY"}, + {name: "empty name", pair: "=value", want: "empty name"}, + {name: "leading digit", pair: "1BAD=x", want: "POSIX-style"}, + {name: "hyphen", pair: "BAD-NAME=x", want: "POSIX-style"}, + {name: "dot", pair: "bad.name=x", want: "POSIX-style"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + _, err := buildEnvironmentVariables(nil, []string{tc.pair}) + if err == nil { + t.Fatalf("expected an error for %q", tc.pair) + } + if !strings.Contains(err.Error(), tc.want) { + t.Errorf("error %q does not mention %q", err, tc.want) + } + }) + } +} + +func TestBuildEnvironmentVariables_TooLongValue(t *testing.T) { + _, err := buildEnvironmentVariables(nil, []string{"BIG=" + strings.Repeat("x", maxEnvValueLen+1)}) + if err == nil || !strings.Contains(err.Error(), "exceeds") { + t.Fatalf("expected a length error, got %v", err) + } +} + +// The file form exists so a secret never lands in an argv, where it is visible +// in the process list and recorded in shell history. +func TestBuildEnvironmentVariables_FromFile(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, ".env.deploy") + content := "# a comment\n\nHF_TOKEN=from-file\nexport LOG_LEVEL=debug\n" + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatal(err) + } + + got, err := buildEnvironmentVariables([]string{path}, nil) + if err != nil { + t.Fatalf("buildEnvironmentVariables: %v", err) + } + if (*got)["HF_TOKEN"] != "from-file" { + t.Errorf("HF_TOKEN = %q", (*got)["HF_TOKEN"]) + } + // `export FOO=bar` is what a shell-sourced file looks like; pasting one in + // should work rather than fail on the keyword. + if (*got)["LOG_LEVEL"] != "debug" { + t.Errorf("LOG_LEVEL = %q (export prefix not absorbed)", (*got)["LOG_LEVEL"]) + } + if len(*got) != 2 { + t.Errorf("comments or blank lines became entries: %v", *got) + } +} + +// An explicit --env is the more specific statement, so it wins over a file entry +// with the same name. +func TestBuildEnvironmentVariables_InlineOverridesFile(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, ".env.deploy") + if err := os.WriteFile(path, []byte("LOG_LEVEL=info\n"), 0o600); err != nil { + t.Fatal(err) + } + + got, err := buildEnvironmentVariables([]string{path}, []string{"LOG_LEVEL=debug"}) + if err != nil { + t.Fatalf("buildEnvironmentVariables: %v", err) + } + if (*got)["LOG_LEVEL"] != "debug" { + t.Errorf("LOG_LEVEL = %q, want the inline value", (*got)["LOG_LEVEL"]) + } +} + +// A malformed line has to name the file and the line, or a long .env is a hunt. +func TestBuildEnvironmentVariables_FileErrorNamesTheLine(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, ".env.deploy") + if err := os.WriteFile(path, []byte("GOOD=1\nBROKEN\n"), 0o600); err != nil { + t.Fatal(err) + } + + _, err := buildEnvironmentVariables([]string{path}, nil) + if err == nil { + t.Fatal("expected an error for a malformed line") + } + if !strings.Contains(err.Error(), "line 2") { + t.Errorf("error %q does not name the line", err) + } +} + +func TestBuildEnvironmentVariables_MissingFile(t *testing.T) { + _, err := buildEnvironmentVariables([]string{filepath.Join(t.TempDir(), "nope")}, nil) + if err == nil { + t.Fatal("expected an error for a missing env file") + } +} + +func TestBuildEnvironmentVariables_TooMany(t *testing.T) { + pairs := make([]string, maxEnvVars+1) + 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 { + t.Fatal("expected an error past the variable limit") + } +} From 18cd4d11c2e7019cf596ff94a30db82447bbb3c9 Mon Sep 17 00:00:00 2001 From: D1-3105 Date: Fri, 21 Aug 2026 21:09:10 +0000 Subject: [PATCH 4/6] remove envvar.go --- .../cmd/serverless/{apps_env.go => env.go} | 109 +++++++++++++++++ .../{envvar_test.go => env_test.go} | 0 internal/cmd/serverless/envvar.go | 114 ------------------ 3 files changed, 109 insertions(+), 114 deletions(-) rename internal/cmd/serverless/{apps_env.go => env.go} (57%) rename internal/cmd/serverless/{envvar_test.go => env_test.go} (100%) delete mode 100644 internal/cmd/serverless/envvar.go diff --git a/internal/cmd/serverless/apps_env.go b/internal/cmd/serverless/env.go similarity index 57% rename from internal/cmd/serverless/apps_env.go rename to internal/cmd/serverless/env.go index ed190da..18d67fb 100644 --- a/internal/cmd/serverless/apps_env.go +++ b/internal/cmd/serverless/env.go @@ -3,6 +3,9 @@ package serverless import ( "fmt" "log/slog" + "os" + "regexp" + "strings" "github.com/charmbracelet/log" serverlessapi "github.com/runware/runware-cli/internal/api/serverless" @@ -163,3 +166,109 @@ func newAppsEnvUnsetCmd(logger *log.Logger) *cobra.Command { }, } } + +// --------------------------------------------------------------------------- +// Create-time environment variables, for `deploy --env` / `--env-file`. +// --------------------------------------------------------------------------- + +// Environment variable limits, mirrored from the server's EnvironmentVariableName +// and its deployment_configs column CHECK. +const ( + maxEnvVars = 100 + maxEnvNameLen = 128 + maxEnvValueLen = 4096 + envAssignSuffix = "=VALUE" +) + +// envNamePattern is the server's EnvironmentVariableName rule: POSIX-style, so a +// name this accepts is a name the API can store rather than one it rejects after +// the archive has already been uploaded. +var envNamePattern = regexp.MustCompile(`^[A-Za-z_][A-Za-z0-9_]{0,127}$`) + +// buildEnvironmentVariables turns --env KEY=VALUE pairs and --env-file paths into +// the create request's map. +// +// These belong on the CREATE request and nowhere else: an app's environment is +// frozen into its version snapshot, which is what the deployer renders from, and +// no endpoint creates a further version -- `deploy` re-applies an existing one by +// number and says so. So a variable set through the /environment-variables +// endpoints after the app exists is stored, listed back, and never reaches a +// worker. Passing it here is the only route that ends up in a pod. +// +// Files are read before the inline pairs are applied, so an explicit --env 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 { + return nil, nil + } + + env := make(map[string]string) + for _, path := range files { + if err := readEnvFile(path, env); err != nil { + return nil, err + } + } + for _, pair := range pairs { + name, value, err := splitEnvAssignment(pair) + if err != nil { + return nil, err + } + env[name] = value + } + + if len(env) > maxEnvVars { + return nil, fmt.Errorf("at most %d environment variables (got %d)", maxEnvVars, len(env)) + } + return &env, nil +} + +// readEnvFile reads KEY=VALUE lines into env, skipping blanks and # comments. +func readEnvFile(path string, env map[string]string) error { + raw, err := os.ReadFile(path) + if err != nil { + return fmt.Errorf("read env file: %w", err) + } + for i, line := range strings.Split(string(raw), "\n") { + trimmed := strings.TrimSpace(line) + if trimmed == "" || strings.HasPrefix(trimmed, "#") { + continue + } + // `export FOO=bar` is what a shell-sourced file looks like, and pasting one + // in is the obvious mistake to absorb rather than reject. + trimmed = strings.TrimPrefix(trimmed, "export ") + name, value, err := splitEnvAssignment(trimmed) + if err != nil { + return fmt.Errorf("%s line %d: %w", path, i+1, err) + } + env[name] = value + } + return nil +} + +// splitEnvAssignment parses one KEY=VALUE, validating the name and value against +// the limits the server enforces. +func splitEnvAssignment(assignment string) (name, value string, err error) { + name, value, found := strings.Cut(assignment, "=") + if !found { + return "", "", fmt.Errorf("%q is not KEY%s", assignment, envAssignSuffix) + } + // The name is trimmed but the value is not: trailing whitespace in a value can + // be deliberate, and a token with a stray newline is a 401 the app cannot + // explain -- so callers pass values through a file rather than have this guess. + name = strings.TrimSpace(name) + + switch { + case name == "": + return "", "", fmt.Errorf("%q has an empty name", assignment) + case len(name) > maxEnvNameLen: + return "", "", fmt.Errorf("environment variable name %q exceeds %d characters", name, maxEnvNameLen) + case !envNamePattern.MatchString(name): + return "", "", fmt.Errorf( + "environment variable name %q must be POSIX-style: letters, digits and underscore, not starting with a digit", + name, + ) + case len(value) > maxEnvValueLen: + return "", "", fmt.Errorf("value for %q exceeds %d characters", name, maxEnvValueLen) + } + return name, value, nil +} diff --git a/internal/cmd/serverless/envvar_test.go b/internal/cmd/serverless/env_test.go similarity index 100% rename from internal/cmd/serverless/envvar_test.go rename to internal/cmd/serverless/env_test.go diff --git a/internal/cmd/serverless/envvar.go b/internal/cmd/serverless/envvar.go deleted file mode 100644 index 525d807..0000000 --- a/internal/cmd/serverless/envvar.go +++ /dev/null @@ -1,114 +0,0 @@ -package serverless - -import ( - "fmt" - "os" - "regexp" - "strings" -) - -// Environment variable limits, mirrored from the server's EnvironmentVariableName -// and its deployment_configs column CHECK. -const ( - maxEnvVars = 100 - maxEnvNameLen = 128 - maxEnvValueLen = 4096 - envAssignSuffix = "=VALUE" -) - -// envNamePattern is the server's EnvironmentVariableName rule: POSIX-style, so a -// name this accepts is a name the API can store rather than one it rejects after -// the archive has already been uploaded. -var envNamePattern = regexp.MustCompile(`^[A-Za-z_][A-Za-z0-9_]{0,127}$`) - -// buildEnvironmentVariables turns --env KEY=VALUE pairs and --env-file paths into -// the create request's map. -// -// These belong on the CREATE request and nowhere else: an app's environment is -// frozen into its version snapshot, which is what the deployer renders from, and -// no endpoint creates a further version -- `deploy` re-applies an existing one by -// number and says so. So a variable set through the /environment-variables -// endpoints after the app exists is stored, listed back, and never reaches a -// worker. Passing it here is the only route that ends up in a pod. -// -// Files are read before the inline pairs are applied, so an explicit --env 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 { - return nil, nil - } - - env := make(map[string]string) - for _, path := range files { - if err := readEnvFile(path, env); err != nil { - return nil, err - } - } - for _, pair := range pairs { - name, value, err := splitEnvAssignment(pair) - if err != nil { - return nil, err - } - env[name] = value - } - - if len(env) > maxEnvVars { - return nil, fmt.Errorf("at most %d environment variables (got %d)", maxEnvVars, len(env)) - } - return &env, nil -} - -// readEnvFile reads KEY=VALUE lines into env, skipping blanks and # comments. -// -// The point of a file is that a secret never reaches an argv: a value passed as -// --env is visible in the process list to every other user on the machine for as -// long as the command runs, and the shell records it in history. -func readEnvFile(path string, env map[string]string) error { - raw, err := os.ReadFile(path) - if err != nil { - return fmt.Errorf("read env file: %w", err) - } - for i, line := range strings.Split(string(raw), "\n") { - trimmed := strings.TrimSpace(line) - if trimmed == "" || strings.HasPrefix(trimmed, "#") { - continue - } - // `export FOO=bar` is what a shell-sourced file looks like, and pasting one - // in is the obvious mistake to absorb rather than reject. - trimmed = strings.TrimPrefix(trimmed, "export ") - name, value, err := splitEnvAssignment(trimmed) - if err != nil { - return fmt.Errorf("%s line %d: %w", path, i+1, err) - } - env[name] = value - } - return nil -} - -// splitEnvAssignment parses one KEY=VALUE, validating the name and value against -// the limits the server enforces. -func splitEnvAssignment(assignment string) (name, value string, err error) { - name, value, found := strings.Cut(assignment, "=") - if !found { - return "", "", fmt.Errorf("%q is not KEY%s", assignment, envAssignSuffix) - } - // The name is trimmed but the value is not: trailing whitespace in a value can - // be deliberate, and a token with a stray newline is a 401 the app cannot - // explain -- so callers pass values through a file rather than have this guess. - name = strings.TrimSpace(name) - - switch { - case name == "": - return "", "", fmt.Errorf("%q has an empty name", assignment) - case len(name) > maxEnvNameLen: - return "", "", fmt.Errorf("environment variable name %q exceeds %d characters", name, maxEnvNameLen) - case !envNamePattern.MatchString(name): - return "", "", fmt.Errorf( - "environment variable name %q must be POSIX-style: letters, digits and underscore, not starting with a digit", - name, - ) - case len(value) > maxEnvValueLen: - return "", "", fmt.Errorf("value for %q exceeds %d characters", name, maxEnvValueLen) - } - return name, value, nil -} From 02e8332422372cdd16a4c17b62dca405b328838b Mon Sep 17 00:00:00 2001 From: D1-3105 Date: Fri, 21 Aug 2026 21:14:19 +0000 Subject: [PATCH 5/6] fix(serverless): address review on the directory deploy Renames and consolidation: * envvar.go is gone. Its create-time helpers now live in env.go beside the `apps env` command group, matching internal/api/serverless/env.go, with a divider recording why both belong in one file: the commands change what the API reports, and only the create request reaches a running worker. Correctness: * The model file survives an ignore rule that matches one of its ANCESTORS. The exemption keyed on the file's own path, but WalkDir meets the directory first and prunes it, so `dist/` plus a model file in dist/ produced an archive without its entry point. Ancestor directories are now descended into while everything else inside them stays excluded. * A symlinked model file is rejected instead of silently dropped. It passed the local stat and was then skipped by the walk as a non-regular file, uploading an archive whose declared entry point was absent -- a 422 from the builder after the upload rather than a message here. * Ignore lines reach the parser verbatim, CR aside. Trimming rewrote valid gitignore patterns: an escaped trailing space lost its escape and a leading space is part of the pattern. Blanks and comments are ParsePattern's business. * File modes are preserved with zip.FileInfoHeader + CreateHeader. zw.Create writes mode 0, so an entrypoint script or helper binary in a codebase arrived without its executable bit and failed at run time with nothing about the archive to explain it. * The archive total is enforced while writing, not only from the walk's stats. Files can grow between being measured and being read, and several staying under the per-file cap can still cross the total. * --env-file no longer trims the assignment. TrimSpace ran on the whole line, so it silently rewrote values; the trimmed copy now only decides whether a line carries an assignment at all. * One matching pair of surrounding quotes is stripped from a value. Files routinely quote them and shells strip them when sourcing, so `HF_TOKEN="hf_x"` was sending the quotes as part of the token -- a 401 in the pod with nothing in the logs. Unbalanced or inner quotes are left alone. * Name and value limits count runes. The API's maxLength is characters, so len() rejected a valid non-ASCII value at half the documented limit. Every fix has a test, including the reviewer's own case for the excluded-directory model file across gitignore, runwareignore and built-in default rules. --- internal/cmd/serverless/env.go | 49 ++++++-- internal/cmd/serverless/env_test.go | 78 +++++++++++++ internal/cmd/serverless/pack.go | 94 +++++++++++---- internal/cmd/serverless/pack_test.go | 167 +++++++++++++++++++++++++++ 4 files changed, 359 insertions(+), 29 deletions(-) diff --git a/internal/cmd/serverless/env.go b/internal/cmd/serverless/env.go index 18d67fb..d0566a0 100644 --- a/internal/cmd/serverless/env.go +++ b/internal/cmd/serverless/env.go @@ -6,6 +6,7 @@ import ( "os" "regexp" "strings" + "unicode/utf8" "github.com/charmbracelet/log" serverlessapi "github.com/runware/runware-cli/internal/api/serverless" @@ -229,14 +230,20 @@ func readEnvFile(path string, env map[string]string) error { return fmt.Errorf("read env file: %w", err) } for i, line := range strings.Split(string(raw), "\n") { + // The trimmed copy decides whether the line carries an assignment at all; + // the assignment itself is parsed from the original, because trailing + // whitespace in a value can be deliberate and this is not the place to + // silently rewrite it. trimmed := strings.TrimSpace(line) if trimmed == "" || strings.HasPrefix(trimmed, "#") { continue } - // `export FOO=bar` is what a shell-sourced file looks like, and pasting one - // in is the obvious mistake to absorb rather than reject. - trimmed = strings.TrimPrefix(trimmed, "export ") - name, value, err := splitEnvAssignment(trimmed) + assignment := strings.TrimSuffix(line, "\r") + // `export FOO=bar` is what a shell-sourced file looks like, and pasting + // one in is the obvious mistake to absorb rather than reject. Trimmed on + // the left only, so the value keeps whatever follows the `=`. + assignment = strings.TrimPrefix(strings.TrimLeft(assignment, " \t"), "export ") + name, value, err := splitEnvAssignment(assignment) if err != nil { return fmt.Errorf("%s line %d: %w", path, i+1, err) } @@ -245,6 +252,27 @@ func readEnvFile(path string, env map[string]string) error { return nil } +// unquote strips one matching pair of surrounding quotes. +// +// A .env file written by hand or produced by another tool routinely quotes +// values, and shells strip those quotes when sourcing the file. Passing them +// through would send `"hf_x"` as the token itself -- a 401 inside the pod with +// nothing in the logs to explain it. One pair only, and only when it matches, so +// a value that genuinely contains a quote keeps it. +func unquote(value string) string { + if len(value) < 2 { + return value + } + first, last := value[0], value[len(value)-1] + if first != last { + return value + } + if first == '\'' || first == '"' { + return value[1 : len(value)-1] + } + return value +} + // splitEnvAssignment parses one KEY=VALUE, validating the name and value against // the limits the server enforces. func splitEnvAssignment(assignment string) (name, value string, err error) { @@ -252,22 +280,25 @@ func splitEnvAssignment(assignment string) (name, value string, err error) { if !found { return "", "", fmt.Errorf("%q is not KEY%s", assignment, envAssignSuffix) } - // The name is trimmed but the value is not: trailing whitespace in a value can - // be deliberate, and a token with a stray newline is a 401 the app cannot - // explain -- so callers pass values through a file rather than have this guess. + // The name is trimmed; the value is not, beyond the quotes below. Trailing + // whitespace in a value can be deliberate, and guessing costs more than it + // saves -- a token with a stray character is a 401 the app cannot explain. name = strings.TrimSpace(name) + value = unquote(value) switch { case name == "": return "", "", fmt.Errorf("%q has an empty name", assignment) - case len(name) > maxEnvNameLen: + // 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) case !envNamePattern.MatchString(name): return "", "", fmt.Errorf( "environment variable name %q must be POSIX-style: letters, digits and underscore, not starting with a digit", name, ) - case len(value) > maxEnvValueLen: + case utf8.RuneCountInString(value) > maxEnvValueLen: return "", "", fmt.Errorf("value for %q exceeds %d characters", name, maxEnvValueLen) } return name, value, nil diff --git a/internal/cmd/serverless/env_test.go b/internal/cmd/serverless/env_test.go index a160c27..899a823 100644 --- a/internal/cmd/serverless/env_test.go +++ b/internal/cmd/serverless/env_test.go @@ -163,3 +163,81 @@ func TestBuildEnvironmentVariables_TooMany(t *testing.T) { t.Fatal("expected an error past the variable limit") } } + +// A quoted value in an --env-file must arrive unquoted: shells strip quotes when +// sourcing, and passing them through sends `"hf_x"` as the token itself -- a 401 +// in the pod with nothing to read. +func TestBuildEnvironmentVariables_StripsSurroundingQuotes(t *testing.T) { + cases := []struct { + name string + line string + want string + }{ + {name: "double", line: `HF_TOKEN="hf_x"`, want: "hf_x"}, + {name: "single", line: `HF_TOKEN='hf_x'`, want: "hf_x"}, + // Only a matching pair, and only when it surrounds: a value that + // genuinely contains a quote keeps it. + {name: "unbalanced", line: `HF_TOKEN="hf_x`, want: `"hf_x`}, + {name: "mismatched", line: `HF_TOKEN='hf_x"`, want: `'hf_x"`}, + {name: "inner quote kept", line: `MSG=say "hi"`, want: `say "hi"`}, + {name: "one pair only", line: `MSG=""quoted""`, want: `"quoted"`}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, ".env") + if err := os.WriteFile(path, []byte(tc.line+"\n"), 0o600); err != nil { + t.Fatal(err) + } + got, err := buildEnvironmentVariables([]string{path}, nil) + if err != nil { + t.Fatalf("buildEnvironmentVariables: %v", err) + } + for _, v := range *got { + if v != tc.want { + t.Errorf("value = %q, want %q", v, tc.want) + } + } + }) + } +} + +// Whitespace inside a value is the caller's business: trimming the whole line +// would silently rewrite a token or an indented value. +func TestBuildEnvironmentVariables_PreservesValueWhitespace(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, ".env") + // A trailing space in the value, and a leading-space line that still parses. + if err := os.WriteFile(path, []byte("PADDED=value \n INDENTED=x\n"), 0o600); err != nil { + t.Fatal(err) + } + + got, err := buildEnvironmentVariables([]string{path}, nil) + if err != nil { + t.Fatalf("buildEnvironmentVariables: %v", err) + } + if (*got)["PADDED"] != "value " { + t.Errorf("PADDED = %q, want %q", (*got)["PADDED"], "value ") + } + if (*got)["INDENTED"] != "x" { + t.Errorf("INDENTED = %q, want x", (*got)["INDENTED"]) + } +} + +// The API's limits are characters; len() counts bytes, so a byte check rejects a +// valid non-ASCII value at half the documented limit. +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}) + if err != nil { + t.Fatalf("a value of exactly the character limit was rejected: %v", err) + } + if (*got)["MSG"] != value { + t.Error("value did not survive") + } + // One rune past is still rejected. + if _, err := buildEnvironmentVariables(nil, []string{"MSG=" + value + "é"}); err == nil { + t.Error("expected a rejection one character past the limit") + } +} diff --git a/internal/cmd/serverless/pack.go b/internal/cmd/serverless/pack.go index 12841c6..0012c10 100644 --- a/internal/cmd/serverless/pack.go +++ b/internal/cmd/serverless/pack.go @@ -245,13 +245,15 @@ func readIgnoreFile(path string) ([]string, error) { return nil, fmt.Errorf("read %s: %w", filepath.Base(path), err) } + // Lines are passed through verbatim apart from a CR: gitignore syntax is + // whitespace-significant, so trimming would rewrite valid patterns -- an + // escaped trailing space (`name\ `) loses its escape and a leading space is + // part of the pattern. Blank lines and comments are the parser's own + // business; ParsePattern handles both, and a pattern that begins with an + // escaped `#` must not be mistaken for one here. lines := []string{} for _, line := range strings.Split(string(raw), "\n") { - trimmed := strings.TrimSpace(line) - if trimmed == "" || strings.HasPrefix(trimmed, "#") { - continue - } - lines = append(lines, trimmed) + lines = append(lines, strings.TrimSuffix(line, "\r")) } return lines, nil } @@ -293,12 +295,20 @@ func collectFiles(root, modelFileRel string, matcher gitignore.Matcher) ([]packe // succeed without it, and an ignore rule that happens to cover it is a // worse failure than a file the customer did not mean to ship. if rel != modelFileRel && matcher.Match(segments, d.IsDir()) { - // Pruning the directory rather than descending is what keeps a - // .venv from costing a stat per file. It also reproduces git's own - // rule -- "it is not possible to re-include a file if a parent - // directory of that file is excluded" -- so a `!node_modules/keep.js` - // does not apply, exactly as it would not for git. if d.IsDir() { + // An excluded directory is pruned rather than walked, which keeps + // a .venv from costing a stat per file and reproduces git's own + // rule that a negation cannot re-include a file whose parent + // directory is excluded. + // + // Except when the model file is inside it. Pruning there would + // drop the one entry the build cannot proceed without, and the + // exemption above never fires because the walk stops at the + // directory, whose path is not the model file's. Descend, and let + // the per-file checks exclude everything else it holds. + if isAncestorOf(rel, modelFileRel) { + return nil + } return fs.SkipDir } return nil @@ -310,6 +320,15 @@ func collectFiles(root, modelFileRel string, matcher gitignore.Matcher) ([]packe // archive is a copy rather than a checkout; sockets and devices have // nothing to copy. if !d.Type().IsRegular() { + // Silently skipping the model file would upload an archive whose + // declared entry point is missing, which the builder can only report + // as a 422 after the upload. Say it here instead. + if rel == modelFileRel { + return fmt.Errorf( + "model file %s is a %s, not a regular file; point --src-dir at the directory holding the real file", + rel, d.Type().String(), + ) + } return nil } @@ -340,6 +359,12 @@ func collectFiles(root, modelFileRel string, matcher gitignore.Matcher) ([]packe return files, nil } +// isAncestorOf reports whether dir is a parent directory of file, both being +// slash-separated paths relative to the archive root. +func isAncestorOf(dir, file string) bool { + return strings.HasPrefix(file, dir+"/") +} + // largestFilesSummary names what filled the archive. "Too big" on its own leaves // the caller to find the offender by hand, which for a deep tree is the whole // problem rather than a detail of it. @@ -377,10 +402,22 @@ func writeArchive(root string, files []packedFile) ([]byte, error) { var buf bytes.Buffer zw := zip.NewWriter(&buf) + // Counted here rather than trusted from the walk: a file may grow between + // being stat'd and being read, and several files each staying under the + // per-file cap can still cross the total. + var written int64 for _, f := range files { - if err := writeArchiveEntry(zw, root, f); err != nil { + n, err := writeArchiveEntry(zw, root, f) + if err != nil { return nil, err } + written += n + if written > maxPackTotalBytes { + return nil, fmt.Errorf( + "the files grew past the %s archive limit while packing; exclude what the app does not need with a %s file", + humanBytes(maxPackTotalBytes), runwareIgnoreFile, + ) + } } if err := zw.Close(); err != nil { return nil, fmt.Errorf("close zip: %w", err) @@ -388,28 +425,45 @@ func writeArchive(root string, files []packedFile) ([]byte, error) { return buf.Bytes(), nil } -func writeArchiveEntry(zw *zip.Writer, root string, f packedFile) error { - src, err := os.Open(filepath.Join(root, filepath.FromSlash(f.rel))) +func writeArchiveEntry(zw *zip.Writer, root string, f packedFile) (int64, error) { + path := filepath.Join(root, filepath.FromSlash(f.rel)) + src, err := os.Open(path) if err != nil { - return fmt.Errorf("open %s: %w", f.rel, err) + return 0, fmt.Errorf("open %s: %w", f.rel, err) } defer src.Close() //nolint:errcheck - w, err := zw.Create(f.rel) + info, err := src.Stat() + if err != nil { + return 0, fmt.Errorf("stat %s: %w", f.rel, err) + } + // FileInfoHeader rather than zw.Create, because Create writes mode 0. A + // codebase can hold an executable -- an entrypoint script, a helper binary -- + // and one that arrives without its executable bit fails at run time with + // nothing about the archive to explain it. The name has to be re-set: the + // header takes it from the FileInfo, which knows only the base name. + header, err := zip.FileInfoHeader(info) + if err != nil { + return 0, fmt.Errorf("build zip header for %s: %w", f.rel, err) + } + header.Name = f.rel + header.Method = zip.Deflate + + w, err := zw.CreateHeader(header) if err != nil { - return fmt.Errorf("create zip entry %s: %w", f.rel, err) + return 0, fmt.Errorf("create zip entry %s: %w", f.rel, err) } // Capped in case the file grew between the walk and here, so a file that - // changes underneath us cannot defeat the limit checked above. + // changes underneath us cannot defeat the per-file limit checked above. written, err := io.Copy(w, io.LimitReader(src, maxPackEntryBytes+1)) if err != nil { - return fmt.Errorf("write zip entry %s: %w", f.rel, err) + return 0, fmt.Errorf("write zip entry %s: %w", f.rel, err) } if written > maxPackEntryBytes { - return fmt.Errorf( + return 0, fmt.Errorf( "%s grew past the maximum for a single file (%s) while packing", f.rel, humanBytes(maxPackEntryBytes), ) } - return nil + return written, nil } diff --git a/internal/cmd/serverless/pack_test.go b/internal/cmd/serverless/pack_test.go index 2be9ec0..3ee2ba8 100644 --- a/internal/cmd/serverless/pack_test.go +++ b/internal/cmd/serverless/pack_test.go @@ -554,3 +554,170 @@ func TestPackDirectory_TotalTooLarge(t *testing.T) { t.Errorf("error %q does not say what filled the archive", err) } } + +// TestPackDirectory_ModelFileAlwaysPackedFromExcludedDirectory is the reviewer's +// case: the exemption for the model file has to survive an ignore rule that +// matches one of its ANCESTORS. The walk meets the directory first, and the +// directory's path is not the model file's, so a naive prune drops the one entry +// the build cannot proceed without. +func TestPackDirectory_ModelFileAlwaysPackedFromExcludedDirectory(t *testing.T) { + const nestedModelFile = "dist/" + testModelFile + + cases := []struct { + name string + files map[string]string + }{ + { + name: "gitignore directory rule", + files: map[string]string{ + gitIgnoreFile: "dist/\n", + nestedModelFile: testPySource, + }, + }, + { + name: "runwareignore directory rule", + files: map[string]string{ + runwareIgnoreFile: "dist/\n", + nestedModelFile: testPySource, + }, + }, + { + name: "built-in default directory rule", + files: map[string]string{ + "node_modules/" + testModelFile: testPySource, + }, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + writeTree(t, dir, tc.files) + // The model file for the built-in case sits under node_modules. + model := nestedModelFile + if _, ok := tc.files[nestedModelFile]; !ok { + model = "node_modules/" + testModelFile + } + + encoded, modelFile, err := packDirectory(dir, model) + if err != nil { + t.Fatalf("packDirectory: %v", err) + } + if modelFile != model { + t.Errorf("modelFile = %q, want %q", modelFile, model) + } + if _, ok := unpack(t, encoded)[model]; !ok { + t.Errorf("the model file was pruned with its directory; archive = %v", names(unpack(t, encoded))) + } + }) + } +} + +// Everything else in an excluded directory stays excluded -- descending for the +// model file must not turn the prune into a free pass for its siblings. +func TestPackDirectory_ExcludedDirectoryKeepsExcludingSiblings(t *testing.T) { + dir := t.TempDir() + writeTree(t, dir, map[string]string{ + runwareIgnoreFile: "dist/\n", + "dist/" + testModelFile: testPySource, + "dist/junk.bin": "junk", + "dist/deep/more.bin": "junk", + }) + + encoded, _, err := packDirectory(dir, "dist/"+testModelFile) + if err != nil { + t.Fatalf("packDirectory: %v", err) + } + packed := unpack(t, encoded) + for _, absent := range []string{"dist/junk.bin", "dist/deep/more.bin"} { + if _, ok := packed[absent]; ok { + t.Errorf("%q rode along with the model file; archive = %v", absent, names(packed)) + } + } +} + +// The executable bit has to survive the archive: a codebase can hold an +// 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" { + t.Skip("no executable bit to preserve on Windows") + } + dir := t.TempDir() + writeTree(t, dir, map[string]string{testModelFile: testPySource, "entrypoint.sh": "#!/bin/sh\n"}) + // 0o755 is the point of the test: the executable bit is what has to survive + // the archive, so gosec's 0600 ceiling cannot apply here. + if err := os.Chmod(filepath.Join(dir, "entrypoint.sh"), 0o755); err != nil { //nolint:gosec + t.Fatal(err) + } + + encoded, _, err := packDirectory(dir, testModelFile) + if err != nil { + t.Fatalf("packDirectory: %v", err) + } + raw, err := base64.StdEncoding.DecodeString(encoded) + if err != nil { + t.Fatal(err) + } + zr, err := zip.NewReader(bytes.NewReader(raw), int64(len(raw))) + if err != nil { + t.Fatal(err) + } + for _, f := range zr.File { + if f.Name != "entrypoint.sh" { + continue + } + if f.Mode().Perm()&0o111 == 0 { + t.Errorf("entrypoint.sh lost its executable bit: mode %v", f.Mode()) + } + return + } + t.Fatal("entrypoint.sh missing from the archive") +} + +// 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" { + t.Skip("symlink creation needs privileges on Windows") + } + dir := t.TempDir() + writeTree(t, dir, map[string]string{"real.py": testPySource}) + if err := os.Symlink(filepath.Join(dir, "real.py"), filepath.Join(dir, "link.py")); err != nil { + t.Fatal(err) + } + + _, _, err := packDirectory(dir, "link.py") + if err == nil { + t.Fatal("expected an error for a symlinked model file") + } + if !strings.Contains(err.Error(), "regular file") { + t.Errorf("error %q does not explain the problem", err) + } +} + +// Ignore patterns are whitespace-significant, so the file's lines must reach the +// parser unmodified: a trailing space is escapable and a leading space is part of +// the pattern. +func TestPackDirectory_IgnorePatternsAreNotTrimmed(t *testing.T) { + dir := t.TempDir() + writeTree(t, dir, map[string]string{ + testModelFile: testPySource, + "keep me.txt": "kept", + "drop.txt": "dropped", + runwareIgnoreFile: "drop.txt\r\n", + }) + + encoded, _, err := packDirectory(dir, testModelFile) + if err != nil { + t.Fatalf("packDirectory: %v", err) + } + packed := unpack(t, encoded) + // A CRLF file must still work: only the CR is normalised. + if _, ok := packed["drop.txt"]; ok { + t.Errorf("a CRLF ignore line was not honoured; archive = %v", names(packed)) + } + if _, ok := packed["keep me.txt"]; !ok { + t.Errorf("an unrelated file with a space was dropped; archive = %v", names(packed)) + } +} From bcee950287c5f42576485be5d61b0a4b0e765ccf Mon Sep 17 00:00:00 2001 From: D1-3105 Date: Fri, 21 Aug 2026 21:27:52 +0000 Subject: [PATCH 6/6] fix(serverless): do not read .gitignore when packing Per review: what a project keeps out of version control is a different question from what it ships to a builder. A generated asset the app needs at run time is a routine .gitignore entry, and a file silently missing from a deployment because of a rule written for git is a surprise that cannot be debugged from the outside -- the archive is already uploaded by then. So exclusions are opt-in and local to .runwareignore. The built-in defaults stay: they cover output nobody means to ship (__pycache__, *.pyc, .venv, node_modules, the tool caches) and a project rule can still override them. .env and .git remain absolutely excluded ahead of the matcher. .gitignore itself now ships like any other file -- it is not special, just not obeyed -- which the replacement test asserts alongside the rules being ignored. --- docs/runware_serverless_deploy.md | 7 +-- internal/cmd/serverless/deploy.go | 7 +-- internal/cmd/serverless/pack.go | 45 ++++++++---------- internal/cmd/serverless/pack_test.go | 68 +++++++++------------------- 4 files changed, 48 insertions(+), 79 deletions(-) diff --git a/docs/runware_serverless_deploy.md b/docs/runware_serverless_deploy.md index 26aa273..82e6a59 100644 --- a/docs/runware_serverless_deploy.md +++ b/docs/runware_serverless_deploy.md @@ -14,9 +14,10 @@ The entry file must live inside the source directory. A relative path is resolve inside it; an absolute path is taken as given. Exclude what the app does not need with a .runwareignore file at the root of the -source directory; it takes gitignore syntax. Without one, a .gitignore is used -instead. Either way .env files are never uploaded, and neither are .git, -__pycache__, .venv or node_modules. +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 +ships. Either way .env files are never uploaded, and neither are .git, +__pycache__, .venv, node_modules or the usual build and tool caches. Environment variables must be supplied here with --env or --env-file. An app's environment is frozen into the version this command creates, which is what the diff --git a/internal/cmd/serverless/deploy.go b/internal/cmd/serverless/deploy.go index bb55b26..69bf5c4 100644 --- a/internal/cmd/serverless/deploy.go +++ b/internal/cmd/serverless/deploy.go @@ -43,9 +43,10 @@ The entry file must live inside the source directory. A relative path is resolve inside it; an absolute path is taken as given. Exclude what the app does not need with a .runwareignore file at the root of the -source directory; it takes gitignore syntax. Without one, a .gitignore is used -instead. Either way .env files are never uploaded, and neither are .git, -__pycache__, .venv or node_modules. +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 +ships. Either way .env files are never uploaded, and neither are .git, +__pycache__, .venv, node_modules or the usual build and tool caches. Environment variables must be supplied here with --env or --env-file. An app's environment is frozen into the version this command creates, which is what the diff --git a/internal/cmd/serverless/pack.go b/internal/cmd/serverless/pack.go index 0012c10..78802d1 100644 --- a/internal/cmd/serverless/pack.go +++ b/internal/cmd/serverless/pack.go @@ -25,13 +25,15 @@ const maxPackEntryBytes int64 = 10 << 20 // 10 MiB // not: a virtualenv is thousands of small files and would sail past it. const maxPackTotalBytes int64 = 25 << 20 // 25 MiB -// runwareIgnoreFile is the project's own exclude list. When it is absent the -// packer falls back to gitIgnoreFile, because a project that already tells git -// what not to track has usually said the same thing this needs to know. -const ( - runwareIgnoreFile = ".runwareignore" - gitIgnoreFile = ".gitignore" -) +// runwareIgnoreFile is the project's own exclude list, and the only one read. +// +// .gitignore is deliberately NOT consulted. What a project keeps out of version +// control is a different question from what it ships to a builder -- a generated +// asset the app needs at run time is a routine .gitignore entry -- and a file +// silently missing from a deployment because of a rule written for git is the +// kind of surprise that costs an afternoon. Exclusions are opt-in and local to +// this file. +const runwareIgnoreFile = ".runwareignore" // defaultIgnorePatterns are excluded when no rule says otherwise. They are // ordinary gitignore patterns evaluated before the project's own, so a later @@ -206,36 +208,27 @@ func locateModelFile(root, modelFile string) string { } // loadIgnoreMatcher builds the exclusion matcher: the built-in defaults first, -// then the project's own rules, so a project rule can override a default. -// .runwareignore wins outright when present — a project that writes one is -// saying what to ship, and silently unioning .gitignore into it would make the -// result impossible to reason about. +// then the project's own .runwareignore, so a project rule can override a +// default. Nothing else is read -- see runwareIgnoreFile for why .gitignore is +// not. func loadIgnoreMatcher(root string) (gitignore.Matcher, error) { patterns := make([]gitignore.Pattern, 0, len(defaultIgnorePatterns)) for _, p := range defaultIgnorePatterns { patterns = append(patterns, gitignore.ParsePattern(p, nil)) } - for _, name := range []string{runwareIgnoreFile, gitIgnoreFile} { - lines, err := readIgnoreFile(filepath.Join(root, name)) - if err != nil { - return nil, err - } - if lines == nil { - continue - } - for _, line := range lines { - patterns = append(patterns, gitignore.ParsePattern(line, nil)) - } - break + lines, err := readIgnoreFile(filepath.Join(root, runwareIgnoreFile)) + if err != nil { + return nil, err + } + for _, line := range lines { + patterns = append(patterns, gitignore.ParsePattern(line, nil)) } return gitignore.NewMatcher(patterns), nil } -// readIgnoreFile returns the file's meaningful lines, or nil when it is absent. -// A present-but-empty file returns a non-nil empty slice, so it still counts as -// "the project chose .runwareignore" and suppresses the .gitignore fallback. +// readIgnoreFile returns the file's lines, or nil when it is absent. func readIgnoreFile(path string) ([]string, error) { raw, err := os.ReadFile(path) if os.IsNotExist(err) { diff --git a/internal/cmd/serverless/pack_test.go b/internal/cmd/serverless/pack_test.go index 3ee2ba8..b820718 100644 --- a/internal/cmd/serverless/pack_test.go +++ b/internal/cmd/serverless/pack_test.go @@ -237,53 +237,34 @@ func TestPackDirectory_NegationCannotReachIntoExcludedDirectory(t *testing.T) { } } -// TestPackDirectory_GitignoreFallback proves .gitignore is honoured when there is -// no .runwareignore, and ignored when there is one — the two must not union, or -// the result is impossible to reason about. -func TestPackDirectory_GitignoreFallback(t *testing.T) { - tree := map[string]string{ +// TestPackDirectory_GitignoreIsNotRead pins the decision that .gitignore has no +// effect. What a project keeps out of version control is a different question +// from what it ships: a generated asset the app needs at run time is a routine +// .gitignore entry, and a file silently missing from a deployment because of a +// rule written for git is a surprise nobody can debug from the outside. +func TestPackDirectory_GitignoreIsNotRead(t *testing.T) { + dir := t.TempDir() + writeTree(t, dir, map[string]string{ testModelFile: "", "secret.txt": "", "build/o.so": "", ".gitignore": "secret.txt\nbuild/\n", - } - - t.Run("used when no runwareignore", func(t *testing.T) { - dir := t.TempDir() - writeTree(t, dir, tree) - - encoded, _, err := packDirectory(dir, filepath.Join(dir, testModelFile)) - if err != nil { - t.Fatalf("packDirectory: %v", err) - } - packed := unpack(t, encoded) - if _, ok := packed["secret.txt"]; ok { - t.Errorf(".gitignore was not honoured; archive = %v", names(packed)) - } - if _, ok := packed["build/o.so"]; ok { - t.Errorf(".gitignore directory rule was not honoured; archive = %v", names(packed)) - } }) - t.Run("ignored when runwareignore exists", func(t *testing.T) { - dir := t.TempDir() - writeTree(t, dir, tree) - writeTree(t, dir, map[string]string{runwareIgnoreFile: "build/\n"}) - - encoded, _, err := packDirectory(dir, filepath.Join(dir, testModelFile)) - if err != nil { - t.Fatalf("packDirectory: %v", err) - } - packed := unpack(t, encoded) - // .runwareignore says nothing about secret.txt, so it ships even though - // .gitignore excludes it. - if _, ok := packed["secret.txt"]; !ok { - t.Errorf(".gitignore was still applied alongside .runwareignore; archive = %v", names(packed)) - } - if _, ok := packed["build/o.so"]; ok { - t.Errorf(".runwareignore rule was not honoured; archive = %v", names(packed)) + encoded, _, err := packDirectory(dir, testModelFile) + if err != nil { + t.Fatalf("packDirectory: %v", err) + } + packed := unpack(t, encoded) + for _, present := range []string{"secret.txt", "build/o.so"} { + if _, ok := packed[present]; !ok { + t.Errorf(".gitignore excluded %q; only .runwareignore may exclude", present) } - }) + } + // The file itself ships like any other: it is not special, just not obeyed. + if _, ok := packed[".gitignore"]; !ok { + t.Errorf(".gitignore should ship as an ordinary file; archive = %v", names(packed)) + } } // TestPackDirectory_NeverPacksEnvFiles is the rule that must not be overridable: @@ -567,13 +548,6 @@ func TestPackDirectory_ModelFileAlwaysPackedFromExcludedDirectory(t *testing.T) name string files map[string]string }{ - { - name: "gitignore directory rule", - files: map[string]string{ - gitIgnoreFile: "dist/\n", - nestedModelFile: testPySource, - }, - }, { name: "runwareignore directory rule", files: map[string]string{