-
-
Notifications
You must be signed in to change notification settings - Fork 63
Add functionality to upload log-file #488
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
base: master
Are you sure you want to change the base?
Changes from 5 commits
332b539
ee68d23
91b4f53
9239d14
9805d2a
48a8931
b0746a1
d37878c
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 |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ import ( | |
| "fmt" | ||
| "io" | ||
| "log" | ||
| "net/http" | ||
| "os" | ||
| "path/filepath" | ||
| "slices" | ||
|
|
@@ -50,18 +51,20 @@ func setupRemoteLogger(flags commandLineFlags, client *remote.Client) { | |
| clog.SetDefaultLogger(logger) | ||
| } | ||
|
|
||
| func setupTargetLogger(flags commandLineFlags, logTarget, commandOutput string) (io.Closer, error) { | ||
| func setupTargetLogger(flags commandLineFlags, logTarget, logUploadTarget, commandOutput string) (io.Closer, error) { | ||
| var ( | ||
| handler LogCloser | ||
| file io.Writer | ||
| err error | ||
| handler LogCloser | ||
| file io.Writer | ||
| filepath string | ||
| err error | ||
| ) | ||
| if scheme, hostPort, isURL := dial.GetAddr(logTarget); isURL { | ||
| handler, file, err = getSyslogHandler(scheme, hostPort) | ||
| } else if dial.IsURL(logTarget) { | ||
| err = fmt.Errorf("unsupported URL: %s", logTarget) | ||
| } else { | ||
| handler, file, err = getFileHandler(logTarget) | ||
| filepath = getLogfilePath(logTarget) | ||
| handler, file, err = getFileHandler(filepath) | ||
| } | ||
| if err != nil { | ||
| return nil, err | ||
|
|
@@ -79,11 +82,54 @@ func setupTargetLogger(flags commandLineFlags, logTarget, commandOutput string) | |
| } else if toLog { | ||
| term.SetAllOutput(file) | ||
| } | ||
| if logUploadTarget != "" && filepath != "" { | ||
| if !dial.IsURL(logUploadTarget) { | ||
| return nil, fmt.Errorf("log-upload: No valid URL %v", logUploadTarget) | ||
| } | ||
| handler = createLogUploadingLogHandler(handler, filepath, logUploadTarget) | ||
| } | ||
| } | ||
| // and return the handler (so we can close it at the end) | ||
| return handler, nil | ||
| } | ||
|
|
||
| type logUploadingLogCloser struct { | ||
| LogCloser | ||
| logfilePath string | ||
| logUploadTarget string | ||
| } | ||
|
|
||
| // Try to close the original handler | ||
| // Also upload the log to the configured log-upload-url | ||
| func (w logUploadingLogCloser) Close() error { | ||
| err := w.LogCloser.Close() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| // Open logfile for reading | ||
| logData, err := os.Open(w.logfilePath) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| // Upload logfile to server | ||
| client := &http.Client{Timeout: 30 * time.Second} | ||
| resp, err := client.Post(w.logUploadTarget, "application/octet-stream", logData) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer resp.Body.Close() | ||
| // HTTP-Status-Codes 200-299 signal success, return an error for everything else | ||
| if resp.StatusCode < 200 || resp.StatusCode > 299 { | ||
| respBody, _ := io.ReadAll(resp.Body) | ||
| return fmt.Errorf("log-upload: Got invalid http status %v: %v", resp.StatusCode, string(respBody)) | ||
| } | ||
| return nil | ||
| } | ||
|
Comment on lines
+105
to
+136
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. 💡 Verification agent 🧩 Analysis chainVerify potential race conditions with log file uploading. While the code correctly closes the original log handler before uploading, there could be a risk of uploading an incomplete log file if other processes are still writing to it. Consider checking if there are any safeguards in place to ensure the log file is fully written before upload. You may want to add a small delay or use file locking if available in your target environments. 🏁 Script executed: #!/bin/bash
# Check if there are any file locking or synchronization mechanisms elsewhere in the codebase
rg -A 3 -B 3 "file.*lock|flock|sync.*file" --ignore-caseLength of output: 56016 Action: Address potential race condition in log file upload After reviewing the code and repository, it appears that while the log file is closed via
Please review your logging and file upload strategy to mitigate any risk of uploading incomplete logs. |
||
|
|
||
| func createLogUploadingLogHandler(handler LogCloser, logfilePath string, logUploadTarget string) LogCloser { | ||
| return logUploadingLogCloser{LogCloser: handler, logfilePath: logfilePath, logUploadTarget: logUploadTarget} | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| func parseCommandOutput(commandOutput string) (all, log bool) { | ||
| if strings.TrimSpace(commandOutput) == "auto" { | ||
| if term.OsStdoutIsTerminal() { | ||
|
|
@@ -98,7 +144,7 @@ func parseCommandOutput(commandOutput string) (all, log bool) { | |
| return | ||
| } | ||
|
|
||
| func getFileHandler(logfile string) (*clog.StandardLogHandler, io.Writer, error) { | ||
| func getLogfilePath(logfile string) string { | ||
| if strings.HasPrefix(logfile, constants.TemporaryDirMarker) { | ||
| if tempDir, err := util.TempDir(); err == nil { | ||
| logfile = logfile[len(constants.TemporaryDirMarker):] | ||
|
|
@@ -109,7 +155,10 @@ func getFileHandler(logfile string) (*clog.StandardLogHandler, io.Writer, error) | |
| _ = os.MkdirAll(filepath.Dir(logfile), 0755) | ||
| } | ||
| } | ||
| return logfile | ||
| } | ||
|
|
||
| func getFileHandler(logfile string) (*clog.StandardLogHandler, io.Writer, error) { | ||
| // create a platform aware log file appender | ||
| keepOpen, appender := true, appendFunc(nil) | ||
| if platform.IsWindows() { | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2,9 +2,14 @@ package main | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "bufio" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "bytes" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "fmt" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "net/http" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "net/http/httptest" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "os" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "path/filepath" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "strconv" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "strings" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "testing" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "time" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -38,7 +43,8 @@ func TestFileHandlerWithTemporaryDirMarker(t *testing.T) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logFile := filepath.Join(util.MustGetTempDir(), "sub", "file.log") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assert.NoFileExists(t, logFile) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| handler, _, err := getFileHandler(filepath.Join(constants.TemporaryDirMarker, "sub", "file.log")) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| filepath := getLogfilePath(filepath.Join(constants.TemporaryDirMarker, "sub", "file.log")) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| handler, _, err := getFileHandler(filepath) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| require.NoError(t, err) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assert.FileExists(t, logFile) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -142,3 +148,34 @@ func TestCloseFileHandler(t *testing.T) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| handler.Close() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assert.Error(t, handler.LogEntry(clog.LogEntry{Level: clog.LevelInfo, Format: "log-line-2"})) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func TestLogUploadFailed(t *testing.T) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| r.Body.Close() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| w.WriteHeader(http.StatusInternalServerError) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| server := httptest.NewServer(handler) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| defer server.Close() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| closer, err := setupTargetLogger(commandLineFlags{}, filepath.Join(constants.TemporaryDirMarker, "file.log"), server.URL, "log") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assert.NoError(t, err) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assert.ErrorContains(t, closer.Close(), "log-upload: Got invalid http status "+strconv.Itoa(http.StatusInternalServerError)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func TestLogUpload(t *testing.T) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| buffer := bytes.Buffer{} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _, err := buffer.ReadFrom(r.Body) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assert.NoError(t, err) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| r.Body.Close() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| w.WriteHeader(http.StatusOK) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assert.Equal(t, strings.Trim(buffer.String(), "\r\n"), "TestLogLine") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| server := httptest.NewServer(handler) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| defer server.Close() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| closer, err := setupTargetLogger(commandLineFlags{}, filepath.Join(constants.TemporaryDirMarker, "file.log"), server.URL, "log") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assert.NoError(t, err) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _, err = term.Println("TestLogLine") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assert.NoError(t, err) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assert.NoError(t, closer.Close()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+163
to
+181
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. Fix ineffectual error assignment and use HTTP status constants. There are two issues with this test:
Apply these fixes: server := httptest.NewServer(handler)
defer server.Close()
closer, err := setupTargetLogger(commandLineFlags{}, filepath.Join(constants.TemporaryDirMarker, "file.log"), server.URL, "log")
+assert.NoError(t, err)
- _, err = term.Println("TestLogLine")
+ _, printErr := term.Println("TestLogLine")
- assert.NoError(t, err)
+ assert.NoError(t, printErr)
assert.NoError(t, closer.Close())- w.WriteHeader(200)
+ w.WriteHeader(http.StatusOK)📝 Committable suggestion
Suggested change
🧰 Tools🪛 golangci-lint (1.64.8)175-175: ineffectual assignment to err (ineffassign) 170-170: "200" can be replaced by http.StatusOK (usestdlibvars) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.