Skip to content

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.

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
merged 1 commit into from
May 27, 2022
Merged

fix: Add command to reconnecting PTY #1860

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
6 changes: 3 additions & 3 deletions agent/agent.go
View file
Open in desktop
Original file line number Diff line number Diff 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 Down Expand 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 deletions agent/agent_test.go
View file
Open in desktop
Original file line number Diff line number Diff 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 Down Expand 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 deletions agent/conn.go
View file
Open in desktop
Original file line number Diff line number Diff 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 in handleReconnectingPTY?

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
Member Author

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 adding some 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.

Copy link
Member Author

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 deletion coderd/workspaceagents.go
View file
Open in desktop
Original file line number Diff line number Diff 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