From ce2b82d43d6076143885f77f744c74d37547a5a0 Mon Sep 17 00:00:00 2001 From: Jan-Eric Hellenberg Date: Tue, 8 Jun 2021 11:52:11 +0200 Subject: [PATCH] Copy files with relative path to active workspace directory of container --- api/dto/dto.go | 16 +++++-------- config/config.go | 9 -------- configuration.example.yaml | 5 ---- runner/runner.go | 16 ++++++------- runner/runner_test.go | 7 +++--- tests/e2e/runners_test.go | 47 ++++++++++++++++++++++++++++++-------- 6 files changed, 53 insertions(+), 47 deletions(-) diff --git a/api/dto/dto.go b/api/dto/dto.go index 7e0f6ed..6d64284 100644 --- a/api/dto/dto.go +++ b/api/dto/dto.go @@ -66,18 +66,14 @@ type File struct { Content []byte `json:"content"` } -// ToAbsolute returns the absolute path of the FilePath with respect to the given basePath. If the FilePath already is absolute, basePath will be ignored. -func (f FilePath) ToAbsolute(basePath string) string { - filePathString := string(f) - if path.IsAbs(filePathString) { - return path.Clean(filePathString) - } - return path.Clean(path.Join(basePath, filePathString)) +// Cleaned returns the cleaned path of the FilePath. +func (f FilePath) Cleaned() string { + return path.Clean(string(f)) } -// AbsolutePath returns the absolute path of the file. See FilePath.ToAbsolute for details. -func (f File) AbsolutePath(basePath string) string { - return f.Path.ToAbsolute(basePath) +// CleanedPath returns the cleaned path of the file. +func (f File) CleanedPath() string { + return f.Path.Cleaned() } // IsDirectory returns true iff the path of the File ends with a /. diff --git a/config/config.go b/config/config.go index 9d877e3..5ffc0f8 100644 --- a/config/config.go +++ b/config/config.go @@ -32,9 +32,6 @@ var ( TLS: false, Namespace: "default", }, - Runner: runner{ - WorkspacePath: "/home/python", - }, Logger: logger{ Level: "INFO", }, @@ -68,11 +65,6 @@ type nomad struct { Namespace string } -// runner configures the runners on the executor -type runner struct { - WorkspacePath string -} - // logger configures the used logger. type logger struct { Level string @@ -82,7 +74,6 @@ type logger struct { type configuration struct { Server server Nomad nomad - Runner runner Logger logger } diff --git a/configuration.example.yaml b/configuration.example.yaml index 9ce1e67..538ffeb 100644 --- a/configuration.example.yaml +++ b/configuration.example.yaml @@ -26,11 +26,6 @@ nomad: # Nomad namespace to use. If unset, 'default' is used namespace: poseidon -# Configuration of the runners -runner: - # Directory where all files with relative paths will be copied into. Must be writable by the default user in the container. - workspacepath: /home/python - # Configuration of the logger logger: # Log level that is used after reading the config (INFO until then) diff --git a/runner/runner.go b/runner/runner.go index e44b3f6..431efc5 100644 --- a/runner/runner.go +++ b/runner/runner.go @@ -8,7 +8,6 @@ import ( "errors" "fmt" "gitlab.hpi.de/codeocean/codemoon/poseidon/api/dto" - "gitlab.hpi.de/codeocean/codemoon/poseidon/config" "gitlab.hpi.de/codeocean/codemoon/poseidon/nomad" "io" "strings" @@ -28,7 +27,6 @@ const ( var ( ErrorFileCopyFailed = errors.New("file copy failed") - FileCopyBasePath = config.Config.Runner.WorkspacePath ) type Runner interface { @@ -111,7 +109,7 @@ func (r *NomadAllocation) UpdateFileSystem(copyRequest *dto.UpdateFileSystemRequ } fileDeletionCommand := fileDeletionCommand(copyRequest.Delete) - copyCommand := "tar --extract --absolute-names --verbose --directory=/ --file=/dev/stdin;" + copyCommand := "tar --extract --absolute-names --verbose --file=/dev/stdin;" updateFileCommand := (&dto.ExecutionRequest{Command: fileDeletionCommand + copyCommand}).FullCommand() stdOut := bytes.Buffer{} stdErr := bytes.Buffer{} @@ -155,15 +153,15 @@ func createTarArchiveForFiles(filesToCopy []dto.File, w io.Writer) error { return tarWriter.Close() } -func fileDeletionCommand(filesToDelete []dto.FilePath) string { - if len(filesToDelete) == 0 { +func fileDeletionCommand(pathsToDelete []dto.FilePath) string { + if len(pathsToDelete) == 0 { return "" } command := "rm --recursive --force " - for _, filePath := range filesToDelete { + for _, filePath := range pathsToDelete { // To avoid command injection, filenames need to be quoted. // See https://unix.stackexchange.com/questions/347332/what-characters-need-to-be-escaped-in-files-without-quotes for details. - singleQuoteEscapedFileName := strings.ReplaceAll(filePath.ToAbsolute(FileCopyBasePath), "'", "'\\''") + singleQuoteEscapedFileName := strings.ReplaceAll(filePath.Cleaned(), "'", "'\\''") command += fmt.Sprintf("'%s' ", singleQuoteEscapedFileName) } command += ";" @@ -174,13 +172,13 @@ func tarHeader(file dto.File) *tar.Header { if file.IsDirectory() { return &tar.Header{ Typeflag: tar.TypeDir, - Name: file.AbsolutePath(FileCopyBasePath), + Name: file.CleanedPath(), Mode: 0755, } } else { return &tar.Header{ Typeflag: tar.TypeReg, - Name: file.AbsolutePath(FileCopyBasePath), + Name: file.CleanedPath(), Mode: 0744, Size: int64(len(file.Content)), } diff --git a/runner/runner_test.go b/runner/runner_test.go index f629f22..409348d 100644 --- a/runner/runner_test.go +++ b/runner/runner_test.go @@ -181,16 +181,15 @@ func (s *UpdateFileSystemTestSuite) TestFilesToCopyAreIncludedInTarArchive() { s.Equal(tests.DefaultFileContent, tarFile.Content) } -func (s *UpdateFileSystemTestSuite) TestFilesWithRelativePathArePutInDefaultLocation() { +func (s *UpdateFileSystemTestSuite) TestTarFilesContainCorrectPathForRelativeFilePath() { s.mockedExecuteCommandCall.Return(0, nil) copyRequest := &dto.UpdateFileSystemRequest{Copy: []dto.File{{Path: tests.DefaultFileName, Content: []byte(tests.DefaultFileContent)}}} _ = s.runner.UpdateFileSystem(copyRequest) tarFiles := s.readFilesFromTarArchive(s.stdin) s.Len(tarFiles, 1) - tarFile := tarFiles[0] - s.True(strings.HasSuffix(tarFile.Name, tests.DefaultFileName)) - s.True(strings.HasPrefix(tarFile.Name, FileCopyBasePath)) + // tar is extracted in the active workdir of the container, file will be put relative to that + s.Equal(tests.DefaultFileName, tarFiles[0].Name) } func (s *UpdateFileSystemTestSuite) TestFilesWithAbsolutePathArePutInAbsoluteLocation() { diff --git a/tests/e2e/runners_test.go b/tests/e2e/runners_test.go index 0a027fe..e1094c2 100644 --- a/tests/e2e/runners_test.go +++ b/tests/e2e/runners_test.go @@ -7,7 +7,6 @@ import ( "github.com/gorilla/websocket" "gitlab.hpi.de/codeocean/codemoon/poseidon/api" "gitlab.hpi.de/codeocean/codemoon/poseidon/api/dto" - "gitlab.hpi.de/codeocean/codemoon/poseidon/runner" "gitlab.hpi.de/codeocean/codemoon/poseidon/tests" "gitlab.hpi.de/codeocean/codemoon/poseidon/tests/helpers" "io" @@ -74,13 +73,13 @@ func (s *E2ETestSuite) TestDeleteRunnerRoute() { } func (s *E2ETestSuite) TestCopyFilesRoute() { - runnerId, err := ProvideRunner(&dto.RunnerRequest{}) + runnerID, err := ProvideRunner(&dto.RunnerRequest{}) s.NoError(err) copyFilesRequestByteString, _ := json.Marshal(&dto.UpdateFileSystemRequest{ Copy: []dto.File{{Path: tests.DefaultFileName, Content: []byte(tests.DefaultFileContent)}}, }) sendCopyRequest := func(reader io.Reader) (*http.Response, error) { - return helpers.HttpPatch(helpers.BuildURL(api.BasePath, api.RunnersPath, runnerId, api.UpdateFileSystemPath), "application/json", reader) + return helpers.HttpPatch(helpers.BuildURL(api.BasePath, api.RunnersPath, runnerID, api.UpdateFileSystemPath), "application/json", reader) } s.Run("File copy with valid payload succeeds", func() { @@ -89,7 +88,33 @@ func (s *E2ETestSuite) TestCopyFilesRoute() { s.Equal(http.StatusNoContent, resp.StatusCode) s.Run("File content can be printed on runner", func() { - s.Equal(tests.DefaultFileContent, s.ContentOfFileOnRunner(runnerId, tests.DefaultFileName)) + s.Equal(tests.DefaultFileContent, s.PrintContentOfFileOnRunner(runnerID, tests.DefaultFileName)) + }) + }) + + s.Run("Files are put in correct location", func() { + relativeFilePath := "relative/file/path.txt" + relativeFileContent := "Relative file content" + absoluteFilePath := "/tmp/absolute/file/path.txt" + absoluteFileContent := "Absolute file content" + testFilePathsCopyRequestString, _ := json.Marshal(&dto.UpdateFileSystemRequest{ + Copy: []dto.File{ + {Path: dto.FilePath(relativeFilePath), Content: []byte(relativeFileContent)}, + {Path: dto.FilePath(absoluteFilePath), Content: []byte(absoluteFileContent)}, + }, + }) + + resp, err := sendCopyRequest(bytes.NewReader(testFilePathsCopyRequestString)) + s.NoError(err) + s.Equal(http.StatusNoContent, resp.StatusCode) + + s.Run("File content of file with relative path can be printed on runner", func() { + // the print command is executed in the context of the default working directory of the container + s.Equal(relativeFileContent, s.PrintContentOfFileOnRunner(runnerID, relativeFilePath)) + }) + + s.Run("File content of file with absolute path can be printed on runner", func() { + s.Equal(absoluteFileContent, s.PrintContentOfFileOnRunner(runnerID, absoluteFilePath)) }) }) @@ -103,7 +128,7 @@ func (s *E2ETestSuite) TestCopyFilesRoute() { s.Equal(http.StatusNoContent, resp.StatusCode) s.Run("File content can no longer be printed", func() { - s.Contains(s.ContentOfFileOnRunner(runnerId, tests.DefaultFileName), "No such file or directory") + s.Contains(s.PrintContentOfFileOnRunner(runnerID, tests.DefaultFileName), "No such file or directory") }) }) @@ -116,9 +141,10 @@ func (s *E2ETestSuite) TestCopyFilesRoute() { resp, err := sendCopyRequest(bytes.NewReader(copyFilesRequestByteString)) s.NoError(err) s.Equal(http.StatusNoContent, resp.StatusCode) + _ = resp.Body.Close() s.Run("File content can be printed on runner", func() { - s.Equal(tests.DefaultFileContent, s.ContentOfFileOnRunner(runnerId, tests.DefaultFileName)) + s.Equal(tests.DefaultFileContent, s.PrintContentOfFileOnRunner(runnerID, tests.DefaultFileName)) }) }) @@ -138,14 +164,15 @@ func (s *E2ETestSuite) TestCopyFilesRoute() { err = json.NewDecoder(resp.Body).Decode(internalServerError) s.NoError(err) s.Contains(internalServerError.Message, "Cannot open: Permission denied") + _ = resp.Body.Close() s.Run("File content can be printed on runner", func() { - s.Equal(string(newFileContent), s.ContentOfFileOnRunner(runnerId, tests.DefaultFileName)) + s.Equal(string(newFileContent), s.PrintContentOfFileOnRunner(runnerID, tests.DefaultFileName)) }) }) s.Run("File copy with invalid payload returns bad request", func() { - resp, err := helpers.HttpPatch(helpers.BuildURL(api.BasePath, api.RunnersPath, runnerId, api.UpdateFileSystemPath), "text/html", strings.NewReader("")) + resp, err := helpers.HttpPatch(helpers.BuildURL(api.BasePath, api.RunnersPath, runnerID, api.UpdateFileSystemPath), "text/html", strings.NewReader("")) s.NoError(err) s.Equal(http.StatusBadRequest, resp.StatusCode) }) @@ -157,8 +184,8 @@ func (s *E2ETestSuite) TestCopyFilesRoute() { }) } -func (s *E2ETestSuite) ContentOfFileOnRunner(runnerId string, filename string) string { - webSocketURL, _ := ProvideWebSocketURL(&s.Suite, runnerId, &dto.ExecutionRequest{Command: fmt.Sprintf("cat %s/%s", runner.FileCopyBasePath, filename)}) +func (s *E2ETestSuite) PrintContentOfFileOnRunner(runnerId string, filename string) string { + webSocketURL, _ := ProvideWebSocketURL(&s.Suite, runnerId, &dto.ExecutionRequest{Command: fmt.Sprintf("cat %s", filename)}) connection, _ := ConnectToWebSocket(webSocketURL) messages, err := helpers.ReceiveAllWebSocketMessages(connection)