Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

fix: Add command to reconnecting PTY#1860

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
kylecarbs merged 1 commit intomainfromreconpty
May 27, 2022
Merged
Show file tree
Hide file tree
Changes fromall commits
Commits
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
6 changes: 3 additions & 3 deletionsagent/agent.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -456,8 +456,8 @@ func (a *agent) handleReconnectingPTY(ctx context.Context, rawID string, conn ne

// The ID format is referenced in conn.go.
// <uuid>:<height>:<width>
idParts := strings.Split(rawID, ":")
if len(idParts) !=3 {
idParts := strings.SplitN(rawID, ":", 4)
if len(idParts) !=4 {
a.logger.Warn(ctx, "client sent invalid id format", slog.F("raw-id", rawID))
return
}
Expand DownExpand Up@@ -489,7 +489,7 @@ func (a *agent) handleReconnectingPTY(ctx context.Context, rawID string, conn ne
}
} else {
// Empty command will default to the users shell!
cmd, err := a.createCommand(ctx,"", nil)
cmd, err := a.createCommand(ctx,idParts[3], nil)
if err != nil {
a.logger.Warn(ctx, "create reconnecting pty command", slog.Error(err))
return
Expand Down
4 changes: 2 additions & 2 deletionsagent/agent_test.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -221,7 +221,7 @@ func TestAgent(t *testing.T) {

conn := setupAgent(t, agent.Metadata{}, 0)
id := uuid.NewString()
netConn, err := conn.ReconnectingPTY(id, 100, 100)
netConn, err := conn.ReconnectingPTY(id, 100, 100, "/bin/bash")
require.NoError(t, err)
bufRead := bufio.NewReader(netConn)

Expand DownExpand Up@@ -259,7 +259,7 @@ func TestAgent(t *testing.T) {
expectLine(matchEchoOutput)

_ = netConn.Close()
netConn, err = conn.ReconnectingPTY(id, 100, 100)
netConn, err = conn.ReconnectingPTY(id, 100, 100, "/bin/bash")
require.NoError(t, err)
bufRead = bufio.NewReader(netConn)

Expand Down
6 changes: 4 additions & 2 deletionsagent/conn.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -34,8 +34,10 @@ type Conn struct {

// ReconnectingPTY returns a connection serving a TTY that can
// be reconnected to via ID.
func (c *Conn) ReconnectingPTY(id string, height, width uint16) (net.Conn, error) {
channel, err := c.CreateChannel(context.Background(), fmt.Sprintf("%s:%d:%d", id, height, width), &peer.ChannelOptions{
//
// The command is optional and defaults to start a shell.
func (c *Conn) ReconnectingPTY(id string, height, width uint16, command string) (net.Conn, error) {
channel, err := c.CreateChannel(context.Background(), fmt.Sprintf("%s:%d:%d:%s", id, height, width, command), &peer.ChannelOptions{
Copy link
Contributor

Choose a reason for hiding this comment

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

How about taking this opportunity to add a version indicator:

Suggested change
channel,err:=c.CreateChannel(context.Background(),fmt.Sprintf("%s:%d:%d:%s",id,height,width,command),&peer.ChannelOptions{
channel,err:=c.CreateChannel(context.Background(),fmt.Sprintf("v1:%s:%d:%d:%s",id,height,width,command),&peer.ChannelOptions{

and a corresponding check inhandleReconnectingPTY?

Without it, this change makes it much harder to ever add a fifth field in the future, because we wouldn't be able to reliably distinguish a 5-field label from a 4-field one whose "command" field contains a colon. And since we're passing the command directly to the shell, it seems like a situation where an incorrect parse could have bad consequences.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Hmm seems like this surfaces the greater issue of incompatibility with agent<->coder versions. I'm nervous about versioning this because it really masks the greater problem.

Am I understanding that correctly? If so, I can create an issue for enforcing some level of compatibility between versions.

Copy link
Contributor

Choose a reason for hiding this comment

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

I definitely agree that this is just the tip of a bigger issue that we don't need to worry about solving right now. I just think that addingsome kind of version indicator now -- no matter how simple -- will be enough to avoid painting ourselves into a corner.

The current version can use "too many colons" as an indicator that there's a version mismatch, and at least crash rather than silently doing the wrong thing, but this patch gets rid of that possibility.

Copy link
Contributor

Choose a reason for hiding this comment

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

adding some kind of version indicator now

Or more accurately, adding it before people start seriously using v2 in production. Doesn't have to be literally now, so it's not really a blocker for this PR.

kylecarbs reacted with thumbs up emoji
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Fair enough

Protocol: ProtocolReconnectingPTY,
})
if err != nil {
Expand Down
2 changes: 1 addition & 1 deletioncoderd/workspaceagents.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -381,7 +381,7 @@ func (api *API) workspaceAgentPTY(rw http.ResponseWriter, r *http.Request) {
return
}
defer agentConn.Close()
ptNetConn, err := agentConn.ReconnectingPTY(reconnect.String(), uint16(height), uint16(width))
ptNetConn, err := agentConn.ReconnectingPTY(reconnect.String(), uint16(height), uint16(width), "")
if err != nil {
_ = conn.Close(websocket.StatusInternalError, httpapi.WebsocketCloseSprintf("dial: %s", err))
return
Expand Down
[8]ページ先頭
©2009-2025 Movatter.jp