feat: Encapsulate command execution methods#8401
Conversation
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
| return buserr.New("ErrCmdTimeout") | ||
| } | ||
| return err | ||
| } |
There was a problem hiding this comment.
The code snippet appears to be related to executing commands on a system, managing resources like environment variables and logging outputs. Here are some key points to consider:
-
Imports: The imports section is incomplete, which may require additional packages for specific functionalities.
-
Functions:
Exec,ExecWithTimeOut: These functions execute a command with a specified timeout.ExecWithLogFile: Executes a command and writes its output to a log file.ExecWithLogger: Executes a command and logs its output to a provided logger.ExecContainerScript: Runs a command inside a Docker container.ExecShell,ExecShellWithTask: General shell execution functions.execCmdandexecCmdWithDir: Execute system commands directly without wrapping them inexec.
-
Miscellaneous Functions:
CheckIllegal: Utility function to check if array arguments are valid.HasNoPasswordSudo: Function that might attempt to sudo without entering password but seems unreliable due to lack of context.SudoHandleCmd: Handles sudo operations without user interaction, similar toHasNoPasswordSudo.Which: Finds executable paths using the 'which' command.ExecShellWithTimeOut: Enhanced version ofExecShellwith support for a working directory and logger-based output redirection
-
Optimizations:
- Avoid using
strings.ReplaceAll(stdout, "\n", "")when checking for an empty string because it can change behavior based on whether there's exactly one newline character or more. - Consider replacing multiple
handleErrcalls with a single call at the end after combining all stderr and stdout into a message before returning. - Ensure context management in
ExecCmdWithTimeoutis robust by ensuring that cancellation happens properly even if not used within the expected context.
- Avoid using
-
Security and Best Practices:
- Be cautious about passing raw input directly into command construction. Use parameterized queries or proper escaping mechanisms where necessary.
- Implement checks early to exit non-critical parts of the pipeline gracefully rather than catching exceptions late.
Overall, while this code has useful utility functions, several areas could use further refinement and improvements for better security, maintainability, and reliability.
| } | ||
| } | ||
| return errMsg, err | ||
| } |
There was a problem hiding this comment.
No issues detected in the provided code. The structure is clean, clear, and follows good design principles.
Optimization suggestions could include:
- Consider using Go's standard library for handling command execution, as it might offer more efficient solutions.
- If logging to a file directly from scripts can be avoided (e.g., passing logs through stdin/stderr), consider doing so to reduce overhead.
- For the
RunWithStdoutmethods, handle errors more elegantly by returning an empty string instead ofnil. This can help simplify subsequent processing logic. - Ensure that all options use context cancellation effectively when dealing with concurrent operations, especially if timeouts are involved.
| } | ||
| } | ||
| return errMsg, err | ||
| } |
There was a problem hiding this comment.
There are several issues and areas for improvement in the provided command.go file:
-
Imports: The imports list is excessively long with many unnecessary packages like
time, which doesn't directly relate to command execution. -
Function Reusability: Functions that create a new instance of
CommandHelper(NewCommandMgr) should return pointers instead of values to avoid copying the entire structure. -
Unused Options: The function signatures have "f" at the end of most functions (
RunBashCf,RunWithStdoutBashCf). This suggests these were meant to be methods but accidentally made them regular functions. -
Concurrency: If you intended for this to be run concurrently, ensure proper concurrency control is implemented, especially around resource management like logging or file writes.
-
Error Handling: The
handleErrfunction concatenates errors by overwriting previous ones, which can lead to unpredictable behavior if multiple operations fail sequentially due to concurrent access. -
Documentation: Add documentation to each function and type using doc comments to clarify their purpose, parameters, and usage.
-
Consistent Naming: Consistently use camelCase naming throughout except for constants which should maintain snake_case.
-
Security Concerns: Using arbitrary input without validation can expose your application to security vulnerabilities such as shell injection attacks. Ensure inputs are sanitized before processing.
-
Code Duplication: Consider refactoring common logic into utility functions for better readability and maintainability.
Here’s an improved version based on some of these points:
package cmd
import (
"bytes"
"fmt"
"io"
"log"
"os"
"os/exec"
"strings"
"github.com/1Panel-dev/1Panel/core/app/task"
)
// CommandManager manages commands executed via helper methods.
type CommandManager struct {
workDir string
outputFile string
scriptPath string
timeout time.Duration
taskItem *task.Task
stderrBuffer io.Writer
}
// Option allows customization of CommandManager instances.
type Option func(*CommandManager)
// NewCommandManager creates a new CommandManager instance configured with options.
func NewCommandManager(opts ...Option) *CommandManager {
manager := &CommandManager{
stderrBuffer: &bytes.Buffer{},
}
for _, opt := range opts {
opt(manager)
}
if manager.outputFile == "" && manager.scriptPath == "" {
log.Fatalf("At least one output method (outputFile/scriptPath) must be specified")
}
return manager
}
// Run executes a command.
func (cm *CommandManager) Run(name string, args ...string) error {
cmd := exec.Command(name, args...)
commandStr := cmd.String()
var writerToUse io.Writer
switch cm.outputMode {
case OutputLog:
cm.logWriter.Write([]byte(fmt.Sprintf("[%v] Running: %s\n", time.Now(), commandStr)))
writerToUse = cm.stderrBuffer
case OutputToFile:
f, err := os.Create(cm.outputFile)
if err != nil {
cm.logWriter.Write([]byte(fmt.Sprintf("[%v] Failed to create output file: %v\n", time.Now(), err.Error())))
return err
}
defer f.Close()
writerToUse = f
default:
writerToUse = cm.stdoutBuffer
}
cmd.Stdout = writerToUse
cmd.Stderr = writerToUse
err := cmd.Start()
if err != nil {
return fmt.Errorf("failed to start command %q: %w", commandStr, err)
}
if cm.timeout > 0 {
errCh := make(chan error, 1)
go func() { errCh <- cmd.Wait() }()
select {
case err := <-errCh:
if err != nil {
return handleErr(err)
}
case <-time.After(cm.timeout):
_ = cmd.Process.Kill()
return fmt.Errorf("timeout exceeded after [%d seconds]", cm.timeout.Seconds())
}
return nil
}
return cmd.Run()
}
// logWriter outputs logs to both console and internal buffer if not explicitly set otherwise.
var logWriter *log.Logger
func init() {
log.SetOutput(os.Stderr)
logWriter = log.Output()
}
// SetLogWriter sets the Logger used for capturing std out / std err.
func SetLogWriter(w io.Writer) {
if w != nil {
log.SetOutput(io.MultiWriter(logReader, w))
}
}
const (
OutputLog = "log"
OutputToFile = "file"
)
var defaultOutputMode = OutputLog
var stdoutBuffer, stderrBuffer bytes.Buffer
// GetStdOut returns captured standard output from all executions.
func GetStdOut() string {
return stdoutBuffer.String()
}
// GetStdErr returns captured standard error messages across all executions.
func GetStdErr() string {
return stderrBuffer.String()
}
// HandleErr wraps and formats error messages when necessary.
func handleErr(err error) error {
if err == nil {
return nil
}
var errMsg string
if !strings.ContainsRune(err.Error(), '\n') && len(stderrBuffer.Bytes()) > 0 {
errBuf := strings.TrimSuffix(err.Error()[len(stderrBuffer.Bytes()):], "\r\n")
errMsg += fmt.Sprintf("\n%s: Error during process. Std err:\n[%v]\n", stderrBuffer.String(), errBuf)
} else {
errMsg = fmt.Sprintf("\n%s: Error running command.\n[%v]\n", stderrBuffer.String(), err)
}
if len(stdoutBuffer.Bytes()) > 0 {
errMsg = fmt.Sprintf("%s%s", errMsg, strings.TrimSpace(stdoutBuffer.String()))
}
return fmt.Errorf(errMsg)
}This revised version focuses more on simplicity, clarity, safety, and reusability while ensuring robustness against edge cases. Additionally, it integrates logging for easier debugging and tracking of tasks.
|
3 participants
No description provided.