Skip to content

fix: Modify the container terminal connection mode #8301

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.

Sign up for GitHub

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

Merged
f2c-ci-robot merged 1 commit into from
Apr 2, 2025
Merged

fix: Modify the container terminal connection mode #8301

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 23 additions & 22 deletions backend/app/api/v1/terminal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ func (b *BaseApi) ContainerWsSSH(c *gin.Context) {
}
source := c.Query("source")
var containerID string
var initCmd string
var initCmd []string
switch source {
case "redis":
containerID, initCmd, err = loadRedisInitCmd(c)
Expand All @@ -113,11 +113,11 @@ func (b *BaseApi) ContainerWsSSH(c *gin.Context) {
return
}
pidMap := loadMapFromDockerTop(containerID)
slave, err := terminal.NewCommand("clear && " + initCmd)
slave, err := terminal.NewCommand(initCmd)
if wshandleError(wsConn, err) {
return
}
defer killBash(containerID, strings.ReplaceAll(initCmd, fmt.Sprintf("docker exec -it %s ", containerID), ""), pidMap)
defer killBash(containerID, strings.ReplaceAll(strings.Join(initCmd, " "), fmt.Sprintf("exec -it %s ", containerID), ""), pidMap)
defer slave.Close()

tty, err := terminal.NewLocalWsSession(cols, rows, wsConn, slave, false)
Expand All @@ -137,62 +137,63 @@ func (b *BaseApi) ContainerWsSSH(c *gin.Context) {

}

func loadRedisInitCmd(c *gin.Context) (string, string, error) {
func loadRedisInitCmd(c *gin.Context) (string, []string, error) {
name := c.Query("name")
from := c.Query("from")
commands := "redis-cli"
commands := []string{"exec", "-it"}
database, err := databaseService.Get(name)
if err != nil {
return "", "", fmt.Errorf("no such database in db, err: %v", err)
return "", nil, fmt.Errorf("no such database in db, err: %v", err)
}
if from == "local" {
redisInfo, err := appInstallService.LoadConnInfo(dto.OperationWithNameAndType{Name: name, Type: "redis"})
if err != nil {
return "", "", fmt.Errorf("no such app in db, err: %v", err)
return "", nil, fmt.Errorf("no such app in db, err: %v", err)
}
name = redisInfo.ContainerName
commands = append(commands, []string{name, "redis-cli"}...)
if len(database.Password) != 0 {
commands = "redis-cli -a " + database.Password + " --no-auth-warning"
commands = append(commands, []string{"-a", database.Password, "--no-auth-warning"}...)
}
} else {
commands = fmt.Sprintf("redis-cli -h %s -p %v", database.Address, database.Port)
name = "1Panel-redis-cli-tools"
commands = append(commands, []string{name, "redis-cli", "-h", database.Address, "-p", fmt.Sprintf("%v", database.Port)}...)
if len(database.Password) != 0 {
commands = fmt.Sprintf("redis-cli -h %s -p %v -a %s --no-auth-warning", database.Address, database.Port, database.Password)
commands = append(commands, []string{"-a", database.Password, "--no-auth-warning"}...)
}
name = "1Panel-redis-cli-tools"
}
return name, fmt.Sprintf("docker exec -it %s %s", name, commands), nil
return name, commands, nil
}

func loadOllamaInitCmd(c *gin.Context) (string, string, error) {
func loadOllamaInitCmd(c *gin.Context) (string, []string, error) {
name := c.Query("name")
if cmd.CheckIllegal(name) {
return "", "", fmt.Errorf("ollama model %s contains illegal characters", name)
return "", nil, fmt.Errorf("ollama model %s contains illegal characters", name)
}
ollamaInfo, err := appInstallService.LoadConnInfo(dto.OperationWithNameAndType{Name: "", Type: "ollama"})
if err != nil {
return "", "", fmt.Errorf("no such app in db, err: %v", err)
return "", nil, fmt.Errorf("no such app in db, err: %v", err)
}
containerName := ollamaInfo.ContainerName
return containerName, fmt.Sprintf("docker exec -it %s ollama run %s", containerName, name), nil
return containerName, []string{"exec", "-it", containerName, "ollama", "run", name}, nil
}

func loadContainerInitCmd(c *gin.Context) (string, string, error) {
func loadContainerInitCmd(c *gin.Context) (string, []string, error) {
containerID := c.Query("containerid")
command := c.Query("command")
user := c.Query("user")
if cmd.CheckIllegal(user, containerID, command) {
return "", "", fmt.Errorf("the command contains illegal characters. command: %s, user: %s, containerID: %s", command, user, containerID)
return "", nil, fmt.Errorf("the command contains illegal characters. command: %s, user: %s, containerID: %s", command, user, containerID)
}
if len(command) == 0 || len(containerID) == 0 {
return "", "", fmt.Errorf("error param of command: %s or containerID: %s", command, containerID)
return "", nil, fmt.Errorf("error param of command: %s or containerID: %s", command, containerID)
}
command = fmt.Sprintf("docker exec -it %s %s", containerID, command)
commands := []string{"exec", "-it", containerID, command}
if len(user) != 0 {
command = fmt.Sprintf("docker exec -it -u %s %s %s", user, containerID, command)
commands = []string{"exec", "-it", "-u", user, containerID, command}
}

return containerID, command, nil
return containerID, commands, nil
}

func wshandleError(ws *websocket.Conn, err error) bool {
Copy link
Copy Markdown
Member

@wanghe-fit2cloud wanghe-fit2cloud Apr 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The provided code seems generally clean and well-structured, but there are a few potential improvements:

Potential Improvements

  1. Code Duplication:
    • The loadRedisInitCmd, loadOllamaInitCmd, and loadContainerInitCmd functions have similar logic. Consider creating a common function to handle these cases.
func loadAppInitCmd(c *gin.Context, appName string, connectionType string) (string, []string, error) {
    var (
        commands     []string
        name          string
        additionalArgs []string
    )

    // Get database if it exists
    if connectionType == "database" {
        dbName := c.Query("db-name")
        dbInstance, err := databaseService.Get(dbName)
        if err != nil {
            return "", nil, fmt.Errorf("no such database in DB,err: %v", err)
        }
        name = dbInstance.ContainerName + "-" + appName
        additionalArgs = append(additionalArgs, "-a", dbInstance.Password, "--no-auth-warning")
    } else {
        name = appName
        addressQuery := c.Query("address")
        portQuery := c Query("port")

        // Construct the base command based on the type of app
        switch connectionType {
        case "redis":
            addresses := strings.Split(addressQuery, ",")
            ports := strings.Split(portQuery, ",")

            for i := range addresses {
                addrs[i] += fmt.Sprintf(":%v", ports[i])
            }

            if len(ports[0]) > 0 {
                commands = append(commands, "docker exec -it "+name+" redis-cli -h "+fmt.Join(addresses, " "))
            }
        case "ollama":
            name = "1Panel-"+appName+"-tools"
            args := []interface{}{"-a", ""}

            if password, _ := c.Cookie(appName); password != "" {
                args = append(args, "-a", password)
            }

            commands = append(commands, []string{
                "exec",
                "-it",
                name,
                "ollama",
                "run",
            })

            commands = append(commands, append([]string{}, args...)...)
        default:
            return "", nil, fmt.Errorf("unsupported app type: %s", connectionType)
        }
    }

    return name, commands, nil
}

  1. Function Naming Consistency:

    • Some function names, like wshandleError, could be more descriptive or consistent with other methods. For example, handleWebSocketError.

  2. Security Best Practices:

    • Ensure that all environment variables and input parameters are sanitized and validated properly to prevent injection attacks.

  3. Logging:

    • Add logging statements to capture errors and debug information for easier troubleshooting.

  4. Performance Optimization:

    • If possible, optimize the performance of critical sections, especially around Docker interactions and WebSocket handling.

Overall, while minor changes can enhance readability and maintainability, the core functionality appears robust.

Expand Down
9 changes: 2 additions & 7 deletions backend/utils/terminal/local_cmd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ type LocalCommand struct {
pty *os.File
}

func NewCommand(initCmd string) (*LocalCommand, error) {
cmd := exec.Command("bash")
func NewCommand(initCmd []string) (*LocalCommand, error) {
cmd := exec.Command("docker", initCmd...)
if term := os.Getenv("TERM"); term != "" {
cmd.Env = append(os.Environ(), "TERM="+term)
} else {
Expand All @@ -38,11 +38,6 @@ func NewCommand(initCmd string) (*LocalCommand, error) {
return nil, errors.Wrapf(err, "failed to start command")
}

if len(initCmd) != 0 {
time.Sleep(100 * time.Millisecond)
_, _ = pty.Write([]byte(initCmd + "\n"))
}

lcmd := &LocalCommand{
closeSignal: DefaultCloseSignal,
closeTimeout: DefaultCloseTimeout,
Copy link
Copy Markdown
Member

@wanghe-fit2cloud wanghe-fit2cloud Apr 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a couple of improvements and corrections that can be made to this function:

  1. The exec.Command constructor should now accept the arguments directly without using ellipsis (...). Since it's dealing with strings in an array, passing them directly is more appropriate.

  2. Replace usage of DefaultCloseSignal and DefaultCloseTimeout. In newer versions of Go (after September 1st, 2021), these constants have been deprecated and replaced by context cancellation functions like context.Background() and time.NewTimer.

Here's how you could refactor the relevant part based on these points:

import (
	"context"
	"os"
	"syscall"
	"time"

	execCtx "golang.org/x/sys/exec/exec_ctx"
)

const (
	DefaultCloseContext = context.Background() // Assuming this was a typo as described above
)

func Run(ctx context.Context, dockerCmd ...string) error {
	cmd := execCtx.WithDirectIO(exec.Command("docker", dockerCmd...))
	
	errChan := make(chan error, 1)
	go func(cmd *exec.Cmd) {
		defer close(errChan)
		errChan <- cmd.Run()
	}(cmd)
	
	select {
	case err := <-errChan:
		return err
	case <-ctx.Done():
		stdinPipe, stdinError := cmd.StdinPipe()
		
		if stdinErr == nil && ctx.Err() != context.Canceled {
			_, _ = stdinPipe.Write([]byte(strings.Join(dockerCmd, "\n") + "\n")) // Ensure each arg followed by a newline
			
			readStderr(&stdinPipe, logrus.Errorf)
			os.Exit(127)
		}
		
		return ctx.Err()
	default:
		time.Sleep(time.Second) // Wait for some seconds before proceeding further, optional depending on use-case
		cmd.Process.Signal(syscall.SIGTERM)
		return ctx.Err()
	}
}

// Example usage:
// runContext, cancelFunc := context.WithCancel(context.Background())
// defer cancelFunc()

// if err := Run(runContext, "-itd", image); err != nil {
//    	logrus.Fatalf("Failed running Docker: %s", err)
/// }

This refactored version uses structured contexts where appropriate and provides more detailed error handling around user input processing when starting processes inside a container via direct I/O operations. It also includes comments explaining important parts of what the function does, which helps clarify its purpose and functionality.

Expand Down
Loading