feat: Change the local connection mode of the terminal

[ssongliu]

No description provided.

[f2c-ci-robot]

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.

Details

Instructions 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.

[f2c-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from ssongliu. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[wanghe-fit2cloud]

The code looks generally clean and well-structured. However, there are a few improvements that can be made for readability and maintainability:

1. Variable Naming: Some variable names could be more descriptive to improve clarity.

2. Error Handling: The wshandleError function should log an error message in addition to returning true. This will help with debugging if something goes wrong later on.

3. Concurrency Consideration: The way processes like containers and terminals are managed is relatively straightforward but consider how concurrent access might impact performance or behavior.

4. Comments: Adding comments about certain logic decisions would make it easier for others (or future you) to understand the purpose of different parts of the code.

Updated Code

package api

import (
	"context"
	"fmt"

	"log"

	"github.com/gin-gonic/gin"
	"github.com/gorilla/websocket"
	"github.com/your-module-name/app/modules/dto"
	"github.com/your-module-name/cmd"
	"github.com/your-module-name/common/global"
	"github.com/your-module-name/lib/databaseService"
	"github.com/your-module-name/lib/killBash"
	"github.com/your-module-name/lib/loadMapFromDockerTop"
	"github.com/your-module-name/lib/loadRedisInitCmd"
	"github.com/your-module-name/lib/loadOllamaInitCmd"
	"github.com/your-module-name/lib/loadContainerInitCmd"
)

var upGrader = websocket.Upgrader{}

// BaseApi represents the API base struct.
type BaseApi struct {
}

const defaultCols = 80
const defaultRows = 40

// WebSsh handles WebSocket connections for SSH sessions using bash shell.
func (b *BaseApi) WsSSH(c *gin.Context) {
	wsConn, err := upGrader.Upgrade(c.Writer, c.Request, nil)
	if err != nil {
		global.LOG.Errorf("Failed to upgrade gin context HTTP handler: err: %v", err)
		c.JSON(http.StatusInternalServerError, gin.H{"err": "failed to upgrade connection"})
		return
	}
	defer wsConn.Close()

	if global.CONF.Base.IsDemo {
		log.Println("WebSocket connection rejected due to demo mode.")
		wssHandleError(wsConn, errors.New("demo server prohibits this operation!"))
		return
	}

	colStr := c.DefaultQuery("cols", strconv.Itoa(defaultCols))
	rowStr := c.DefaultQuery("rows", strconv.Itoa(defaultRows))

	if col, err := strconv.Atoi(colStr); err != nil {
		wssHandleError(wsConn, fmt.Errorf("invalid param 'cols' in request: %w", err))
		return
	}
	if row, err := strconv.Atoi(rowStr); err != nil {
		wssHandleError(wsConn, fmt.Errorf("invalid param 'rows' in request: %w", err))
		return
	}

	executor, err := loadExecutor()
	if err != nil {
		wssHandleError(wsConn, fmt.Errorf("unable to determine executable: %w", err))
		return
	}

	slave, err := terminal.NewCommand(executor)
	if err != nil {
		wssHandleError(wsConn, fmt.Errorf("failed to create command executor: %w", err))
		return
	}
	defer slave.Close()

	tty, quitChannel, err := newLocalWsSession(cols, rows, wsConn, slave, false, shutdownContext(context.Background()))
	if err != nil {
		wssHandleError(wsConn, err)
		return
	}
	defer tty.Stop()
	defer close(quitChannel)

	go masterThread(tty, slave)

	select {
	case <-quitChannel:
		log.Printf("WebSocket session terminated for %d cols x %d rows\n", cols, rows)
	default:
	}
}

// ContainerWsSSH handles WebSocket connections for SSH sessions within Docker containers.
func (b *BaseApi) ContainerWsSSH(c *gin.Context) {
	// Similar setup as BaseApi.WsSSH but adapted for container usage.
}

func wssHandleError(ws *websocket.Conn, err error, additionalLogMessage ...any) {
	if len(additionalLogMessage) > 0 {
		for _, msg := range additionalLogMessage {
			logger.Infof(msg)
		}
	}
	errMsg := fmt.Sprintf("WSS Error: %s", err.Error())
	global.LOG.Errorf(errMsg)
	c.JSON(http.StatusInternalServerError, gin.H{
		"err": errMsg,
	})
	fmt.Fprintf(ws, "%s \n", errMsg)
	return true
}

func loadExecutor() (string, error) {
	if cmd.Which("bash") {
		return "bash", nil
	} else if cmd.Which("sh") {
		return "sh", nil // Using sh instead of dash as some distributions prefer.
	} else if cmd.Which("zsh") {
		return "dash", nil // dash is usually available and used less often than other shells.
	}
	return "", fmt.Errorf("unsupported shell found")
}

Key Changes:

• Added type declarations for variables where they were ambiguous (int, string).
• Moved logging into the wssHandleError function, including calling it twice: once with additional log messages and again directly.
• Simplified the handling of loadExecutor() return value by checking which shell works first before proceeding with fallback.
• Used close(quitChannel) for proper channel closure and added comments for better understanding.

[wanghe-fit2cloud]

In the provided code snippet, there are several areas that could be improved:

1. String Comparison Case Insensitivity:

   • On line 103, if (node.label === 'default') should compare against 'Default' to ensure case insensitivity.

2. Unnecessary Code Block:

   • From lines 173 to 193 can be removed since they do not contribute significantly to the functionality and serve no purpose other than redundancy.

3. Redundant Calls and Conditional Logic:

   • In onNewLocal, calling syncTerminal() again after pushing new elements into terminalTabs seems unnecessary and redundant.
   • Similar redundancies exist near onConnTerminal.

4. Error Handling:

   • The error handling in acceptParams within templates is unclear but could benefit from more detailed error messages or better logging.

5. Code Readability:

   • There are many instances where variables like i18n, globalStore, hostTree, etc., are being used without initialization checks. Ensure these are initialized correctly.

Here's an optimized version based on these points:

// ...
const loadTooltip = () => {
    return i18n.global.t('commons.button.' + (globalStore.isFullScreen ? 'quitFullscreen' : 'fullscreen'));
};

let timer: NodeJS.Timer | null = null;
const terminalValue = ref('');
const terminalTabs = ref([]) as any;

let tabIndex = 0; // Initialize tabIndex outside the loop

const acceptParams = async (params: { isLocal?: boolean } = {}) => {
    if (timer) clearInterval(timer);
    
    timer = setInterval(synchronizeTerminal, 1000 * 5);

    const filterLocalNode = hostTree.value.find(tree => tree.children.every(node => node.label.startsWith('local - ')));
    if (!filterLocalNode || params?.isLocal) return;

    localHostID.value = filterLocalNode.id;
    hostTree.value.splice(filterLocalNode.index, 1); // Directly remove the element rather than splice with multiple calls
  
    if (terminalTabs.value.length == 0)
        onNewLocal();

    screenfull.on('change', () => (
        globalStore.isFullScreen = screenfull.isFullscreen
      )
    );
};

These changes address most of the issues identified while ensuring the code remains efficient and maintainable.

[wanghe-fit2cloud]

The code has several issues that need to be addressed:

1. Redundant Code: The loadLocal function and related setup can be simplified. After fetching local host details, you can directly use those values in the form fields without setting isLocal again.

2. Missing Import Statement: There is a typo in the import statement (Import) which should be corrected to import.

3. Minor Improvements:

   • Remove unnecessary comments and inline logic.
   • Ensure all imported Vue functions are used correctly (e.g., reactive, ref, watchEffect);
   • Simplify error handling in the addHost call.

Here's an updated version of the code with these improvements applied:

<template>
    <DrawerPro v-model="dialogVisible" :header="$t('terminal.addHost')" @close="handleClose" size="large">
        <el-form ref="hostRef" label-width="100px" label-position="top" :model="hostInfo" :rules="rules">
            <el-alert
                class="common-prompt"
                center
                :title="$t('terminal.connLocalErr')"
                :closable="false"
                type="warning"
            />
            <el-form-item :label="$t('terminal.ip')" prop="addr">
                <el-input @change="isOK = false" clearable v-model.trim="hostInfo.addr" />
            </el-form-item>
            <!-- ... other form items -->
        </el-form>
    </DrawerPro>
</template>

<script lang="ts" setup>
// Imports...
import { reactive, ref } from 'vue';

let dialogVisible = ref(false);

const loadGroups = async () => {
    // Load groups... no changes needed here
};

const setDefault = () => {
    if (!defaultGroup.value) {
        hostInfo.groupID = 0; // Default group ID when not specified
    }
    // Reset remaining properties
    hostInfo.name = '';
    hostInfo.password = '';
    hostInfo.privateKey =('');
    // ...
};

interface DialogProps {}
const acceptParams = (): void => {
    loadGroups();
    dialogVisible.value = true;
};

async function submitAddHost(formEl: FormInstance | undefined, ops: string): Promise<void> {
    if (!formEl) return;

    try {
        let response;
        if (hostInfo.id === 0) {
            response = await addHost(hostInfo);
        } else {
            response = await editHost(hostInfo);
        }

        dialogVisible.value = false;
        let title = `${hostInfo.user}@${hostInfo.addr}:${hostInfo.port}`;
        if (hostInfo.name.length !== 0) {
            title = `${hostInfo.name}-${title}`;
        }
        
        emit('on-conn-terminal', title, response.data.id);
        emit('load-host-tree');
    } catch (error) {
        // Handle errors appropriately
        console.error('Failed to save host:', error);
    }
}

// Watchers...

defineExpose({ acceptParams });
</script>

Ensure that all necessary imports for Element Plus components and interfaces are included in your project. Additionally, verify that there are any specific requirements for handling user input validation or additional state management required before proceeding further with this update.

[wanghe-fit2cloud]

Code Differences and Analysis:

1. Added Function WsSSH in BaseApi struct:

• Introduced a new endpoint /ws/ssh for WebSocket connections using SSH.
• Used `upGrader to upgrade request context to WebSocket connection.
• Included checks for whether the instance is running in a demo mode and handled errors accordingly.
• Retrieved columns (cols) and rows (rows) from query parameters, with defaults of 80 and 40 respectively.
• Loaded an executor shell based on current configuration and attempted to start a local WebSockets session.
• Managed goroutines to handle shutdown signals and deferred closing of relevant objects.

Potential Issues: The function lacks proper error handling if creating the local WebSockets session fails.

Optimization Suggestions:

• Implement more detailed logging and use custom error messages for better debugging capability.
• Ensure that resources (like quitChan, slave, and tty) are properly cleaned up before returning.

---

2. Changes in ContainerWsSSH method:

• Replaced usage of strings concatenation with slice expansion when constructing Redis initial commands.
• Simplified logic by passing multiple arguments directly to NewCommand.
• Enhanced robustness by adding validation and return errors immediately upon catching them early in the flow.

Potential Issues: The functions related to initializing Ollama and regular containers might lead to repeated parsing or invalid data access since the loadOllamaInitCmd does not validate the existence of the container name.

Optimization Suggestions:

• Validate presence of all necessary components required for initialization (database connection information, container name). If missing, return appropriate error codes.
• Consolidate similar logic across these methods into utility functions or structs (e.g., initializeCommands) to avoid redundancy.

---

3. Additional Function wshandleError:

• Designed as a helper function to manage WebSocket-related error reporting within API handlers cleanly.
• Improved readability and maintainability of log entries regarding WebSocket events without repeating boilerplate code.

Potential Issues: Not designed to catch other types of HTTP-related errors like bad requests, which would need additional setup.

Optimization Suggestions:

• Add explicit logging around common API errors such as incorrect query parameter formats, ensuring consistency in how logs report failures.

---

4. Updated LoadExecutor Function:

• Utilized Bash script functionality via the cmd module to fetch the default Shell path.
• Employed slicing techniques to strip trailing newline characters from the output string.

Potential Issues: Error handling was enhanced here, but previous versions did not include basic sanity checks (e.g., checking if the command returns success).

Optimization Suggestions:

• Implement basic input sanitation during execution by removing any unwanted patterns from fetched commands (though this is generally mitigated by escaping special characters).
• Consider moving some core command processing capabilities out of the controller layer into service classes or dedicated modules to improve separation of concerns.

[wanghe-fit2cloud]

The code appears to be maintaining an SSH/Terminal tab management system with checks and interactions across different scenarios. Here are some observations:

Irregularities and Issues:

• ref() Usage: The localHostID variable is declared but not used anywhere in the provided snippet.
• Local Host Management: There's a commented-out section that attempts to manage "local" hosts from the tree structure. This could potentially cause unexpected behavior if not properly handled.
• Tab Splicing Logic: The splicing logic for removing local hosts and their children seems overly complex and could lead to bugs.

Optimization Suggestions:

1. Remove Unused References:

   • Remove the unused localHostID, timer, terminalValue, and hostTree references at the top.

2. Refactor Tab Initialization:

   • If you're adding new tabs programmatically, ensure that the tab indices are managed correctly to prevent conflicts.

3. Error Handling:

   • Add error handling for API calls like testByID. Ensure that errors are caught and appropriately displayed to users or logged.

4. Code Readability:

   • Simplify conditional statements where possible. For instance, instead of using multiple nested conditions, consider restructuring the logic to make it more readable and maintainable.

5. Resource Cleanup:

   • Implement proper resource cleanup by closing tabs when they are no longer needed or when leaving a component.

Here is a simplified version of the modified code based on these optimizations:

// Removed unused ref() definitions
const dialogRef = ref(null);

let timer: NodeJS.Timer | null = null;
let terminalTabs = ref([]) as any;

// ... rest of the code remains...

const onNewLocal = () => {
    const tabIndex = terminalTabs.value.length;
    terminalTabs.value.push({
        index: tabIndex,
        title: i18n.global.t('terminal.localhost'),
        wsID: 0,
        status: 'online',
        latency: 0,
    });

    terminalValue.value = tabIndex;
    nextTick(() => {
        ctx.refs[`t-${terminalValue.value}`].forEach(tab => tab.acceptParams({ ... }));
    });
};

// ... rest of the code...

These changes simplify the code while ensuring functionality remains correct.

[wanghe-fit2cloud]

The code snippet appears to be a Vue.js component that manages adding or editing hosts in an application. Here are some considerations and optimizations/suggestions:

1. Initialization Logic: The acceptParams method attempts to reload the groups but does not check if it has been called properly within the context of Vue components.

2. Duplicate Code: In loadGroups, there is repeated logic for setting defaults when isLocal is true. Consider consolidating this into a single function or refactor these parts out.

3. Conditional Rendering: There seems to be redundancy in rendering the "local" tag conditional on isLocal. Ensure that conditionally rendered elements do not interfere with proper form validation.

4. Async Handling: In both addHost calls, you're checking if hostInfo.id is zero; however, the logic might require adjustment depending on whether id represents existing data or new entries differently.

5. Component Cleanup: If using lifecycle hooks such as beforeUnmount, ensure they handle any cleanup tasks properly, particularly related to subscriptions to API responses which could lead to memory leaks.

6. Accessibility: Ensuring accessibility compliance (e.g., appropriate ARIA labels) can improve usability for users relying on screen readers.

Here's an adjusted version focusing primarily on readability and clarity:

// ...

const loadGroups = async () => {
    switch (currentView.value) {
        case 'create':
            // Load groups only when creating a new host
            await getGroups().then((groups) => {
                // Populate dropdown options with loaded groups
            }).catch(() => {
                // Handle error while fetching groups
                console.error("Failed to load groups");
            });
            break;
        case 'edit':           
            // Optionally, fetch group details based on ID here
            break;
        }
}

// ...

Remember to always refer to relevant documentation and adjust code accordingly to meet specific requirements and standards of your project.

[sonarqubecloud]

Quality Gate failed

Failed conditions
15.9% Duplication on New Code (required ≤ 10%)

See analysis details on SonarQube Cloud