Skip to content

Commit ac26e15

Browse files
djeebusdjeebus
authored
orchestrator: fix some http error codes for volume ops (#3371)
1 parent d7db2c2 commit ac26e15

8 files changed

Lines changed: 296 additions & 6 deletions

File tree

‎packages/orchestrator/pkg/volumes/dir_create.go‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"net/http"
1010
"os"
1111
"path/filepath"
12+
"syscall"
1213

1314
"go.opentelemetry.io/otel/attribute"
1415
"go.opentelemetry.io/otel/trace"
@@ -96,5 +97,13 @@ func processError(ctx context.Context, s string, err error) error {
9697
return newAPIError(ctx, codes.NotFound, http.StatusNotFound, orchestrator.UserErrorCode_PATH_NOT_FOUND, "%s: %s", s, err.Error()).Err()
9798
}
9899

100+
// ENOTDIR is returned when a path traverses a regular file as if it were a
101+
// directory (e.g. "file.txt/sub"). errors.Is(err, os.ErrNotExist) does not
102+
// match it, so map it explicitly to a client-facing bad request rather than
103+
// letting it fall through to a generic 500.
104+
if errors.Is(err, syscall.ENOTDIR) {
105+
return newAPIError(ctx, codes.InvalidArgument, http.StatusBadRequest, orchestrator.UserErrorCode_INVALID_REQUEST, "%s: a component of the path is a file, but must be a directory", s).Err()
106+
}
107+
99108
return fmt.Errorf("%s: %w", s, err)
100109
}

‎packages/orchestrator/pkg/volumes/dir_create_test.go‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,11 @@
33
package volumes
44

55
import (
6+
"errors"
7+
"fmt"
68
"os"
79
"path/filepath"
10+
"syscall"
811
"testing"
912

1013
"github.com/stretchr/testify/assert"
@@ -157,3 +160,40 @@ func TestDirCreate(t *testing.T) {
157160
require.Equal(t, originalMode, fi.Mode().Perm(), "Mode should not have been changed for an existing directory when CreateParents=true")
158161
})
159162
}
163+
164+
func TestProcessError(t *testing.T) {
165+
t.Parallel()
166+
167+
t.Run("ErrExist maps to AlreadyExists", func(t *testing.T) {
168+
t.Parallel()
169+
170+
err := processError(t.Context(), "op", fmt.Errorf("wrapped: %w", os.ErrExist))
171+
requireGRPCError(t, err, codes.AlreadyExists, orchestrator.UserErrorCode_PATH_ALREADY_EXISTS)
172+
})
173+
174+
t.Run("ErrNotExist maps to NotFound", func(t *testing.T) {
175+
t.Parallel()
176+
177+
err := processError(t.Context(), "op", fmt.Errorf("wrapped: %w", os.ErrNotExist))
178+
requireGRPCError(t, err, codes.NotFound, orchestrator.UserErrorCode_PATH_NOT_FOUND)
179+
})
180+
181+
t.Run("ENOTDIR maps to InvalidArgument", func(t *testing.T) {
182+
t.Parallel()
183+
184+
// Emulate a path traversing a regular file, e.g. "file.txt/sub".
185+
err := processError(t.Context(), "op", &os.PathError{Op: "open", Path: "file.txt/sub", Err: syscall.ENOTDIR})
186+
requireGRPCError(t, err, codes.InvalidArgument, orchestrator.UserErrorCode_INVALID_REQUEST)
187+
})
188+
189+
t.Run("generic error is passed through unmapped", func(t *testing.T) {
190+
t.Parallel()
191+
192+
sentinel := errors.New("boom")
193+
err := processError(t.Context(), "op", sentinel)
194+
195+
// A generic error is wrapped and passed through, not mapped to a
196+
// gRPC status (which would drop the original error from the chain).
197+
require.ErrorIs(t, err, sentinel)
198+
})
199+
}

‎packages/orchestrator/pkg/volumes/file_create.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ func (s *Service) CreateFile(server orchestrator.VolumeService_CreateFileServer)
7171

7272
file, err := fs.OpenFile(path, flags, os.FileMode(mode).Perm())
7373
if err != nil {
74-
return fmt.Errorf("failed to open file for create: %w", err)
74+
return processError(ctx, "failed to open file for create", err)
7575
}
7676

7777
defer func() {

‎packages/orchestrator/pkg/volumes/file_create_test.go‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import (
1212
"github.com/stretchr/testify/mock"
1313
"github.com/stretchr/testify/require"
1414
"google.golang.org/grpc"
15+
"google.golang.org/grpc/codes"
1516

1617
"github.com/e2b-dev/infra/packages/shared/pkg/grpc/orchestrator"
1718
)
@@ -124,6 +125,29 @@ func TestFileCreate(t *testing.T) {
124125
mockServer.AssertExpectations(t)
125126
})
126127

128+
t.Run("create existing file without force", func(t *testing.T) {
129+
t.Parallel()
130+
131+
filename := "already-exists.txt"
132+
err := os.WriteFile(filepath.Join(tmpdir, filename), []byte("existing"), 0o644)
133+
require.NoError(t, err)
134+
135+
mockServer := &mockCreateFileServer{}
136+
mockServer.On("Context").Return(t.Context())
137+
138+
mockServer.On("Recv").Return(&orchestrator.CreateFileRequest{
139+
Message: &orchestrator.CreateFileRequest_Start{
140+
Start: &orchestrator.VolumeFileCreateStart{
141+
Volume: volumeInfo,
142+
Path: filename,
143+
},
144+
},
145+
}, nil).Once()
146+
147+
err = s.CreateFile(mockServer)
148+
requireGRPCError(t, err, codes.AlreadyExists, orchestrator.UserErrorCode_PATH_ALREADY_EXISTS)
149+
})
150+
127151
t.Run("unexpected EOF", func(t *testing.T) {
128152
t.Parallel()
129153

‎packages/orchestrator/pkg/volumes/file_get.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,14 +34,14 @@ func (s *Service) GetFile(request *orchestrator.GetFileRequest, server orchestra
3434

3535
f, err := fs.Open(path)
3636
if err != nil {
37-
return fmt.Errorf("failed to open file: %w", err)
37+
return processError(ctx, "failed to open file", err)
3838
}
3939
defer f.Close()
4040

4141
span.AddEvent("getting file info")
4242
info, err := fs.Stat(path)
4343
if err != nil {
44-
return fmt.Errorf("failed to stat file: %w", err)
44+
return processError(ctx, "failed to stat file", err)
4545
}
4646

4747
span.AddEvent("sending file start", trace.WithAttributes(

‎packages/orchestrator/pkg/volumes/file_get_test.go‎

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111
"github.com/stretchr/testify/mock"
1212
"github.com/stretchr/testify/require"
1313
"google.golang.org/grpc"
14+
"google.golang.org/grpc/codes"
1415

1516
"github.com/e2b-dev/infra/packages/shared/pkg/grpc/orchestrator"
1617
)
@@ -92,6 +93,27 @@ func TestFileGet(t *testing.T) {
9293
Path: "non-existent",
9394
}, mockServer)
9495

95-
require.Error(t, err)
96+
requireGRPCError(t, err, codes.NotFound, orchestrator.UserErrorCode_PATH_NOT_FOUND)
97+
})
98+
99+
t.Run("get path traversing a regular file", func(t *testing.T) {
100+
t.Parallel()
101+
102+
// A path whose parent component is a regular file (not a directory)
103+
// yields ENOTDIR, which should map to a client-facing bad request
104+
// rather than a generic internal error.
105+
filename := "traverse-file.txt"
106+
err := os.WriteFile(filepath.Join(tmpdir, filename), []byte("test"), 0o644)
107+
require.NoError(t, err)
108+
109+
mockServer := &mockGetFileServer{}
110+
mockServer.On("Context").Return(t.Context())
111+
112+
err = s.GetFile(&orchestrator.GetFileRequest{
113+
Volume: volumeInfo,
114+
Path: filename + "/sub",
115+
}, mockServer)
116+
117+
requireGRPCError(t, err, codes.InvalidArgument, orchestrator.UserErrorCode_INVALID_REQUEST)
96118
})
97119
}

‎packages/orchestrator/pkg/volumes/path_update.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ func (s *Service) UpdatePath(ctx context.Context, request *orchestrator.UpdatePa
7171

7272
if err = fs.Chown(path, uid, gid); err != nil {
7373
if os.IsNotExist(err) {
74-
return nil, newAPIError(ctx, codes.NotFound, http.StatusBadRequest, orchestrator.UserErrorCode_PATH_NOT_FOUND, "failed to chown: %q not found.", request.GetPath()).Err()
74+
return nil, newAPIError(ctx, codes.NotFound, http.StatusNotFound, orchestrator.UserErrorCode_PATH_NOT_FOUND, "failed to chown: %q not found.", request.GetPath()).Err()
7575
}
7676

7777
return nil, fmt.Errorf("failed to update file ownership: %w", err)
@@ -81,7 +81,7 @@ func (s *Service) UpdatePath(ctx context.Context, request *orchestrator.UpdatePa
8181
fi, err := fs.Stat(path)
8282
if err != nil {
8383
if os.IsNotExist(err) {
84-
return nil, newAPIError(ctx, codes.NotFound, http.StatusBadRequest, orchestrator.UserErrorCode_PATH_NOT_FOUND, "failed to stat: %q not found.", request.GetPath()).Err()
84+
return nil, newAPIError(ctx, codes.NotFound, http.StatusNotFound, orchestrator.UserErrorCode_PATH_NOT_FOUND, "failed to stat: %q not found.", request.GetPath()).Err()
8585
}
8686

8787
return nil, fmt.Errorf("failed to stat file: %w", err)

‎packages/orchestrator/pkg/volumes/path_update_test.go‎

Lines changed: 195 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,195 @@
1+
//go:build linux
2+
3+
package volumes
4+
5+
import (
6+
"os"
7+
"path/filepath"
8+
"testing"
9+
10+
"github.com/stretchr/testify/require"
11+
"google.golang.org/grpc/codes"
12+
13+
"github.com/e2b-dev/infra/packages/shared/pkg/grpc/orchestrator"
14+
)
15+
16+
func TestUpdatePath(t *testing.T) {
17+
t.Parallel()
18+
19+
s, tmpdir, volumeInfo := setupTestService(t)
20+
21+
t.Run("update mode only", func(t *testing.T) {
22+
t.Parallel()
23+
24+
filename := "update-mode.txt"
25+
require.NoError(t, os.WriteFile(filepath.Join(tmpdir, filename), []byte("test"), 0o644))
26+
27+
resp, err := s.UpdatePath(t.Context(), &orchestrator.UpdatePathRequest{
28+
Volume: volumeInfo,
29+
Path: filename,
30+
Mode: new(uint32(0o600)),
31+
})
32+
require.NoError(t, err)
33+
require.Equal(t, uint32(0o600), resp.GetEntry().GetMode())
34+
35+
info, err := os.Stat(filepath.Join(tmpdir, filename))
36+
require.NoError(t, err)
37+
require.Equal(t, os.FileMode(0o600), info.Mode().Perm())
38+
})
39+
40+
t.Run("update uid and gid", func(t *testing.T) {
41+
t.Parallel()
42+
43+
filename := "update-owner.txt"
44+
require.NoError(t, os.WriteFile(filepath.Join(tmpdir, filename), []byte("test"), 0o644))
45+
46+
resp, err := s.UpdatePath(t.Context(), &orchestrator.UpdatePathRequest{
47+
Volume: volumeInfo,
48+
Path: filename,
49+
Uid: new(uint32(1234)),
50+
Gid: new(uint32(5678)),
51+
})
52+
require.NoError(t, err)
53+
require.Equal(t, uint32(1234), resp.GetEntry().GetUid())
54+
require.Equal(t, uint32(5678), resp.GetEntry().GetGid())
55+
56+
fs, _, errResponse := s.getFilesystemAndPath(t.Context(), &orchestrator.UpdatePathRequest{Volume: volumeInfo})
57+
require.Nil(t, errResponse)
58+
assertDir(t, fs, "/"+filename, 1234, 5678, 0o644)
59+
})
60+
61+
t.Run("update uid only leaves gid unchanged", func(t *testing.T) {
62+
t.Parallel()
63+
64+
filename := "update-uid-only.txt"
65+
require.NoError(t, os.WriteFile(filepath.Join(tmpdir, filename), []byte("test"), 0o644))
66+
require.NoError(t, os.Chown(filepath.Join(tmpdir, filename), 1000, 2000))
67+
68+
resp, err := s.UpdatePath(t.Context(), &orchestrator.UpdatePathRequest{
69+
Volume: volumeInfo,
70+
Path: filename,
71+
Uid: new(uint32(4321)),
72+
})
73+
require.NoError(t, err)
74+
require.Equal(t, uint32(4321), resp.GetEntry().GetUid())
75+
require.Equal(t, uint32(2000), resp.GetEntry().GetGid())
76+
})
77+
78+
t.Run("update gid only leaves uid unchanged", func(t *testing.T) {
79+
t.Parallel()
80+
81+
filename := "update-gid-only.txt"
82+
require.NoError(t, os.WriteFile(filepath.Join(tmpdir, filename), []byte("test"), 0o644))
83+
require.NoError(t, os.Chown(filepath.Join(tmpdir, filename), 1000, 2000))
84+
85+
resp, err := s.UpdatePath(t.Context(), &orchestrator.UpdatePathRequest{
86+
Volume: volumeInfo,
87+
Path: filename,
88+
Gid: new(uint32(8765)),
89+
})
90+
require.NoError(t, err)
91+
require.Equal(t, uint32(1000), resp.GetEntry().GetUid())
92+
require.Equal(t, uint32(8765), resp.GetEntry().GetGid())
93+
})
94+
95+
t.Run("update mode, uid, and gid together", func(t *testing.T) {
96+
t.Parallel()
97+
98+
filename := "update-all.txt"
99+
require.NoError(t, os.WriteFile(filepath.Join(tmpdir, filename), []byte("test"), 0o644))
100+
101+
resp, err := s.UpdatePath(t.Context(), &orchestrator.UpdatePathRequest{
102+
Volume: volumeInfo,
103+
Path: filename,
104+
Mode: new(uint32(0o640)),
105+
Uid: new(uint32(111)),
106+
Gid: new(uint32(222)),
107+
})
108+
require.NoError(t, err)
109+
require.Equal(t, uint32(0o640), resp.GetEntry().GetMode())
110+
require.Equal(t, uint32(111), resp.GetEntry().GetUid())
111+
require.Equal(t, uint32(222), resp.GetEntry().GetGid())
112+
113+
fs, _, errResponse := s.getFilesystemAndPath(t.Context(), &orchestrator.UpdatePathRequest{Volume: volumeInfo})
114+
require.Nil(t, errResponse)
115+
assertDir(t, fs, "/"+filename, 111, 222, 0o640)
116+
})
117+
118+
t.Run("update directory", func(t *testing.T) {
119+
t.Parallel()
120+
121+
dirname := "update-dir"
122+
require.NoError(t, os.Mkdir(filepath.Join(tmpdir, dirname), 0o755))
123+
124+
resp, err := s.UpdatePath(t.Context(), &orchestrator.UpdatePathRequest{
125+
Volume: volumeInfo,
126+
Path: dirname,
127+
Mode: new(uint32(0o700)),
128+
})
129+
require.NoError(t, err)
130+
require.Equal(t, orchestrator.FileType_FILE_TYPE_DIRECTORY, resp.GetEntry().GetType())
131+
require.Equal(t, uint32(0o700), resp.GetEntry().GetMode())
132+
})
133+
134+
t.Run("no fields set only stats", func(t *testing.T) {
135+
t.Parallel()
136+
137+
filename := "update-noop.txt"
138+
require.NoError(t, os.WriteFile(filepath.Join(tmpdir, filename), []byte("test"), 0o644))
139+
140+
resp, err := s.UpdatePath(t.Context(), &orchestrator.UpdatePathRequest{
141+
Volume: volumeInfo,
142+
Path: filename,
143+
})
144+
require.NoError(t, err)
145+
require.Equal(t, "/"+filename, resp.GetEntry().GetPath())
146+
require.Equal(t, uint32(0o644), resp.GetEntry().GetMode())
147+
})
148+
149+
t.Run("chmod non-existent path", func(t *testing.T) {
150+
t.Parallel()
151+
152+
_, err := s.UpdatePath(t.Context(), &orchestrator.UpdatePathRequest{
153+
Volume: volumeInfo,
154+
Path: "does-not-exist-chmod",
155+
Mode: new(uint32(0o600)),
156+
})
157+
requireGRPCError(t, err, codes.NotFound, orchestrator.UserErrorCode_PATH_NOT_FOUND)
158+
})
159+
160+
t.Run("chown non-existent path", func(t *testing.T) {
161+
t.Parallel()
162+
163+
_, err := s.UpdatePath(t.Context(), &orchestrator.UpdatePathRequest{
164+
Volume: volumeInfo,
165+
Path: "does-not-exist-chown",
166+
Uid: new(uint32(111)),
167+
})
168+
requireGRPCError(t, err, codes.NotFound, orchestrator.UserErrorCode_PATH_NOT_FOUND)
169+
})
170+
171+
t.Run("stat non-existent path", func(t *testing.T) {
172+
t.Parallel()
173+
174+
_, err := s.UpdatePath(t.Context(), &orchestrator.UpdatePathRequest{
175+
Volume: volumeInfo,
176+
Path: "does-not-exist-stat",
177+
})
178+
requireGRPCError(t, err, codes.NotFound, orchestrator.UserErrorCode_PATH_NOT_FOUND)
179+
})
180+
181+
t.Run("invalid team id", func(t *testing.T) {
182+
t.Parallel()
183+
184+
_, err := s.UpdatePath(t.Context(), &orchestrator.UpdatePathRequest{
185+
Volume: &orchestrator.VolumeInfo{
186+
VolumeType: volumeType,
187+
TeamId: "not-a-uuid",
188+
VolumeId: volumeInfo.GetVolumeId(),
189+
},
190+
Path: "whatever.txt",
191+
Mode: new(uint32(0o600)),
192+
})
193+
requireGRPCError(t, err, codes.InvalidArgument, orchestrator.UserErrorCode_INVALID_REQUEST)
194+
})
195+
}

0 commit comments

Comments
 (0)