-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat(sdk/go): add user-scoped filesystem views #1348
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -114,12 +114,12 @@ func (s *Sandbox) startProcess(ctx context.Context, payload processStartRequest, | |
| return result, nil | ||
| } | ||
|
|
||
| func (s *Sandbox) readFile(ctx context.Context, path string) (string, error) { | ||
| func (s *Sandbox) readFile(ctx context.Context, path string, options ...fileRequestOption) (string, error) { | ||
| if err := s.ensureClient(); err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| query := url.Values{"path": []string{path}} | ||
| query := newEnvdFileQuery(path, options...) | ||
| req, err := s.newEnvdRequest(ctx, http.MethodGet, "/files", query, nil) | ||
| if err != nil { | ||
| return "", err | ||
|
|
@@ -149,11 +149,11 @@ func (s *Sandbox) readFile(ctx context.Context, path string) (string, error) { | |
| // writeFile uploads data through envd's POST /files API. It first tries a raw | ||
| // octet-stream body and, if the envd version rejects that, retries as a | ||
| // multipart upload — mirroring the Python SDK's fallback. | ||
| func (s *Sandbox) writeFile(ctx context.Context, path string, data []byte) error { | ||
| func (s *Sandbox) writeFile(ctx context.Context, path string, data []byte, options ...fileRequestOption) error { | ||
| if err := s.ensureClient(); err != nil { | ||
| return err | ||
| } | ||
| query := url.Values{"path": []string{path}} | ||
| query := newEnvdFileQuery(path, options...) | ||
|
|
||
| resp, err := s.doEnvdUpload(ctx, query, bytes.NewReader(data), "application/octet-stream") | ||
| if err != nil { | ||
|
|
@@ -183,6 +183,15 @@ func (s *Sandbox) writeFile(ctx context.Context, path string, data []byte) error | |
| return nil | ||
| } | ||
|
|
||
| func newEnvdFileQuery(path string, options ...fileRequestOption) url.Values { | ||
| query := url.Values{"path": []string{path}} | ||
| opts := resolveFileRequestOptions(options...) | ||
| if opts.user != "" { | ||
| query.Set("username", opts.user) | ||
| } | ||
| return query | ||
| } | ||
|
|
||
| func (s *Sandbox) doEnvdUpload(ctx context.Context, query url.Values, body io.Reader, contentType string) (*http.Response, error) { | ||
| req, err := s.newEnvdRequest(ctx, http.MethodPost, "/files", query, body) | ||
| if err != nil { | ||
|
|
@@ -243,6 +252,27 @@ func basicAuthUser(user string) string { | |
| return "Basic " + base64.StdEncoding.EncodeToString([]byte(user+":")) | ||
| } | ||
|
|
||
| func setFilesystemRPCHeaders(req *http.Request, options ...fileRequestOption) { | ||
| req.Header.Set("Content-Type", "application/json") | ||
| req.Header.Set("Connect-Protocol-Version", connectProtocolVersion) | ||
| setFilesystemRPCUser(req, options...) | ||
| } | ||
|
|
||
| func setFilesystemRPCStreamHeaders(req *http.Request, options ...fileRequestOption) { | ||
| req.Header.Set("Content-Type", connectContentType) | ||
| req.Header.Set("Connect-Protocol-Version", connectProtocolVersion) | ||
| setFilesystemRPCUser(req, options...) | ||
| } | ||
|
|
||
| func setFilesystemRPCUser(req *http.Request, options ...fileRequestOption) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. RPC user propagation is unverified in CI. The unit tests only assert that the |
||
| opts := resolveFileRequestOptions(options...) | ||
| if opts.user != "" { | ||
| // Preserve the legacy unscoped request shape. An explicit ForUser("root") | ||
| // is intentionally different and sends root through Basic authentication. | ||
| req.SetBasicAuth(opts.user, "") | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Also worth a comment: this guard is deliberate, but it makes the wire format asymmetric with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Two nits on the new envd Basic-auth construction:
|
||
| } | ||
| } | ||
|
|
||
| func parseProcessStartStream(r io.Reader) (*processStartResult, error) { | ||
| var result processStartResult | ||
| var stdout strings.Builder | ||
|
|
@@ -322,7 +352,7 @@ func decodeProcessBytes(value string) (string, error) { | |
| return string(raw), nil | ||
| } | ||
|
|
||
| func (s *Sandbox) filesystemRPC(ctx context.Context, method string, reqBody any) ([]byte, int, error) { | ||
| func (s *Sandbox) filesystemRPC(ctx context.Context, method string, reqBody any, options ...fileRequestOption) ([]byte, int, error) { | ||
| if err := s.ensureClient(); err != nil { | ||
| return nil, 0, err | ||
| } | ||
|
|
@@ -334,8 +364,7 @@ func (s *Sandbox) filesystemRPC(ctx context.Context, method string, reqBody any) | |
| if err != nil { | ||
| return nil, 0, err | ||
| } | ||
| req.Header.Set("Content-Type", "application/json") | ||
| req.Header.Set("Connect-Protocol-Version", connectProtocolVersion) | ||
| setFilesystemRPCHeaders(req, options...) | ||
|
|
||
| resp, err := s.client.dataHTTP.Do(req) | ||
| if err != nil { | ||
|
|
@@ -349,8 +378,8 @@ func (s *Sandbox) filesystemRPC(ctx context.Context, method string, reqBody any) | |
| return body, resp.StatusCode, nil | ||
| } | ||
|
|
||
| func (s *Sandbox) listDir(ctx context.Context, path string) ([]FileEntry, error) { | ||
| body, status, err := s.filesystemRPC(ctx, "ListDir", map[string]string{"path": path}) | ||
| func (s *Sandbox) listDir(ctx context.Context, path string, options ...fileRequestOption) ([]FileEntry, error) { | ||
| body, status, err := s.filesystemRPC(ctx, "ListDir", map[string]string{"path": path}, options...) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
@@ -369,8 +398,8 @@ func (s *Sandbox) listDir(ctx context.Context, path string) ([]FileEntry, error) | |
| return result.Entries, nil | ||
| } | ||
|
|
||
| func (s *Sandbox) statFile(ctx context.Context, path string) (*FileEntry, error) { | ||
| body, status, err := s.filesystemRPC(ctx, "Stat", map[string]string{"path": path}) | ||
| func (s *Sandbox) statFile(ctx context.Context, path string, options ...fileRequestOption) (*FileEntry, error) { | ||
| body, status, err := s.filesystemRPC(ctx, "Stat", map[string]string{"path": path}, options...) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
@@ -389,8 +418,8 @@ func (s *Sandbox) statFile(ctx context.Context, path string) (*FileEntry, error) | |
| return &result.Entry, nil | ||
| } | ||
|
|
||
| func (s *Sandbox) removeFile(ctx context.Context, path string) error { | ||
| body, status, err := s.filesystemRPC(ctx, "Remove", map[string]string{"path": path}) | ||
| func (s *Sandbox) removeFile(ctx context.Context, path string, options ...fileRequestOption) error { | ||
| body, status, err := s.filesystemRPC(ctx, "Remove", map[string]string{"path": path}, options...) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
@@ -400,8 +429,8 @@ func (s *Sandbox) removeFile(ctx context.Context, path string) error { | |
| return nil | ||
| } | ||
|
|
||
| func (s *Sandbox) moveFile(ctx context.Context, source, destination string) (*FileEntry, error) { | ||
| body, status, err := s.filesystemRPC(ctx, "Move", map[string]string{"source": source, "destination": destination}) | ||
| func (s *Sandbox) moveFile(ctx context.Context, source, destination string, options ...fileRequestOption) (*FileEntry, error) { | ||
| body, status, err := s.filesystemRPC(ctx, "Move", map[string]string{"source": source, "destination": destination}, options...) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
@@ -417,8 +446,8 @@ func (s *Sandbox) moveFile(ctx context.Context, source, destination string) (*Fi | |
| return &result.Entry, nil | ||
| } | ||
|
|
||
| func (s *Sandbox) makeDirFile(ctx context.Context, path string) (*FileEntry, error) { | ||
| body, status, err := s.filesystemRPC(ctx, "MakeDir", map[string]string{"path": path}) | ||
| func (s *Sandbox) makeDirFile(ctx context.Context, path string, options ...fileRequestOption) (*FileEntry, error) { | ||
| body, status, err := s.filesystemRPC(ctx, "MakeDir", map[string]string{"path": path}, options...) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
@@ -474,7 +503,7 @@ type watchDirFrame struct { | |
| Keepalive *struct{} `json:"keepalive,omitempty"` | ||
| } | ||
|
|
||
| func (s *Sandbox) watchDir(ctx context.Context, path string) (*Watcher, error) { | ||
| func (s *Sandbox) watchDir(ctx context.Context, path string, options ...fileRequestOption) (*Watcher, error) { | ||
| if err := s.ensureClient(); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
@@ -490,8 +519,7 @@ func (s *Sandbox) watchDir(ctx context.Context, path string) (*Watcher, error) { | |
| cancel() | ||
| return nil, err | ||
| } | ||
| req.Header.Set("Content-Type", connectContentType) | ||
| req.Header.Set("Connect-Protocol-Version", connectProtocolVersion) | ||
| setFilesystemRPCStreamHeaders(req, options...) | ||
|
|
||
| resp, err := s.client.dataHTTP.Do(req) | ||
| if err != nil { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,38 +13,73 @@ type Files struct { | |
| reader fileReader | ||
| writer fileWriter | ||
| filer fileFiler | ||
| user string | ||
| } | ||
|
|
||
| type fileRequestOptions struct { | ||
| user string | ||
| } | ||
|
|
||
| type fileRequestOption func(*fileRequestOptions) | ||
|
|
||
| func withUser(user string) fileRequestOption { | ||
| return func(options *fileRequestOptions) { | ||
| options.user = user | ||
| } | ||
| } | ||
|
|
||
| func resolveFileRequestOptions(options ...fileRequestOption) fileRequestOptions { | ||
| var resolved fileRequestOptions | ||
| for _, option := range options { | ||
| if option != nil { | ||
| option(&resolved) | ||
| } | ||
| } | ||
| return resolved | ||
| } | ||
|
|
||
| type fileReader interface { | ||
| readFile(context.Context, string) (string, error) | ||
| readFile(context.Context, string, ...fileRequestOption) (string, error) | ||
| } | ||
|
|
||
| type fileWriter interface { | ||
| writeFile(context.Context, string, []byte) error | ||
| writeFile(context.Context, string, []byte, ...fileRequestOption) error | ||
| } | ||
|
|
||
| type fileFiler interface { | ||
| listDir(context.Context, string) ([]FileEntry, error) | ||
| statFile(context.Context, string) (*FileEntry, error) | ||
| removeFile(context.Context, string) error | ||
| moveFile(context.Context, string, string) (*FileEntry, error) | ||
| makeDirFile(context.Context, string) (*FileEntry, error) | ||
| watchDir(context.Context, string) (*Watcher, error) | ||
| listDir(context.Context, string, ...fileRequestOption) ([]FileEntry, error) | ||
| statFile(context.Context, string, ...fileRequestOption) (*FileEntry, error) | ||
| removeFile(context.Context, string, ...fileRequestOption) error | ||
| moveFile(context.Context, string, string, ...fileRequestOption) (*FileEntry, error) | ||
| makeDirFile(context.Context, string, ...fileRequestOption) (*FileEntry, error) | ||
| watchDir(context.Context, string, ...fileRequestOption) (*Watcher, error) | ||
| } | ||
|
|
||
| // ForUser returns an immutable filesystem view that executes every operation | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider documenting how the user is actually propagated, since it differs by transport: HTTP file operations ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Documentation accuracy: "executes every operation as user" overstates the isolation this view provides. Per the integration test added in this PR ( Suggest a caveat such as: "runs operations in the filesystem context of |
||
| // as user. The returned view shares the underlying sandbox connection with f. | ||
| // An empty user restores the unscoped behavior used by Sandbox.Files. | ||
| func (f *Files) ForUser(user string) *Files { | ||
| if f == nil { | ||
| return nil | ||
| } | ||
| scoped := *f | ||
| scoped.user = user | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Docs overstate the isolation guarantee. The doc (and the README example) says |
||
| return &scoped | ||
| } | ||
|
|
||
| func (f *Files) Read(ctx context.Context, path string) (string, error) { | ||
| if f == nil || f.reader == nil { | ||
| return "", fmt.Errorf("files is not attached to a sandbox") | ||
| } | ||
| return f.reader.readFile(ctx, path) | ||
| return f.reader.readFile(ctx, path, withUser(f.user)) | ||
| } | ||
|
|
||
| // Write uploads data to path through envd's HTTP file API. | ||
| func (f *Files) Write(ctx context.Context, path string, data []byte) error { | ||
| if f == nil || f.writer == nil { | ||
| return fmt.Errorf("files is not attached to a sandbox") | ||
| } | ||
| return f.writer.writeFile(ctx, path, data) | ||
| return f.writer.writeFile(ctx, path, data, withUser(f.user)) | ||
| } | ||
|
|
||
| // WriteFiles uploads multiple files. It stops at the first error and returns | ||
|
|
@@ -66,15 +101,15 @@ func (f *Files) List(ctx context.Context, path string) ([]FileEntry, error) { | |
| if f == nil || f.filer == nil { | ||
| return nil, fmt.Errorf("files is not attached to a sandbox") | ||
| } | ||
| return f.filer.listDir(ctx, path) | ||
| return f.filer.listDir(ctx, path, withUser(f.user)) | ||
| } | ||
|
|
||
| // Stat returns metadata for a single file or directory. | ||
| func (f *Files) Stat(ctx context.Context, path string) (*FileEntry, error) { | ||
| if f == nil || f.filer == nil { | ||
| return nil, fmt.Errorf("files is not attached to a sandbox") | ||
| } | ||
| return f.filer.statFile(ctx, path) | ||
| return f.filer.statFile(ctx, path, withUser(f.user)) | ||
| } | ||
|
|
||
| // Exists returns true if the path exists inside the sandbox. | ||
|
|
@@ -95,23 +130,23 @@ func (f *Files) Remove(ctx context.Context, path string) error { | |
| if f == nil || f.filer == nil { | ||
| return fmt.Errorf("files is not attached to a sandbox") | ||
| } | ||
| return f.filer.removeFile(ctx, path) | ||
| return f.filer.removeFile(ctx, path, withUser(f.user)) | ||
| } | ||
|
|
||
| // Rename moves or renames a file or directory inside the sandbox. | ||
| func (f *Files) Rename(ctx context.Context, oldPath, newPath string) (*FileEntry, error) { | ||
| if f == nil || f.filer == nil { | ||
| return nil, fmt.Errorf("files is not attached to a sandbox") | ||
| } | ||
| return f.filer.moveFile(ctx, oldPath, newPath) | ||
| return f.filer.moveFile(ctx, oldPath, newPath, withUser(f.user)) | ||
| } | ||
|
|
||
| // MakeDir creates a directory inside the sandbox. | ||
| func (f *Files) MakeDir(ctx context.Context, path string) (*FileEntry, error) { | ||
| if f == nil || f.filer == nil { | ||
| return nil, fmt.Errorf("files is not attached to a sandbox") | ||
| } | ||
| return f.filer.makeDirFile(ctx, path) | ||
| return f.filer.makeDirFile(ctx, path, withUser(f.user)) | ||
| } | ||
|
|
||
| // WatchDir watches a directory for filesystem changes. The returned Watcher | ||
|
|
@@ -120,5 +155,5 @@ func (f *Files) WatchDir(ctx context.Context, path string) (*Watcher, error) { | |
| if f == nil || f.filer == nil { | ||
| return nil, fmt.Errorf("files is not attached to a sandbox") | ||
| } | ||
| return f.filer.watchDir(ctx, path) | ||
| return f.filer.watchDir(ctx, path, withUser(f.user)) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This switches the only octet-stream→multipart fallback test to the user-scoped path, so the unscoped
Files().Writefallback — and its request shape (nousernameparam) — is no longer exercised.TestFilesWithoutUserPreservesUnscopedRequestShapecovers onlyReadandList. Since the PR's stated goal is to preserve unscoped request behavior, it'd be worth keeping (or adding) an unscopedWriteassertion that the fallback still works with nousernamein the query on both attempts.