Skip to content

Men 8382 mender artifact fails on calling mender update show provides #703

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 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
183 changes: 183 additions & 0 deletions cli/util/ssh.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,183 @@
// Copyright 2025 Northern.tech AS
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package util

import (
"bufio"
"context"
"io"
"os"
"os/exec"
"os/signal"
"strings"
"syscall"
"time"

"github.com/pkg/errors"
"github.com/urfave/cli"
)

type SSHCommand struct {
Cmd *exec.Cmd
ctx context.Context
Copy link
Contributor

Choose a reason for hiding this comment

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

The ctx is unused.

Suggested change
ctx context.Context

Side-note; storing Context in structs is in most cases considered an anti-pattern in Go

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the tip about anti-pattern.
I thought that by adding ctx here even though it's unused, I am making sure that the context lives long enough to be used in EchoSigHandler. Now I understand this is wrong.
Can you confirm that supplying _ctx to EchoSigHandler makes sure that the lifetime is extended so that EchoSigHandler can handle it every time, even if the app is being closed?

Stdout io.ReadCloser
cancel context.CancelFunc
sigChan chan os.Signal
errChan chan error
}

func StartSSHCommand(c *cli.Context,
_ctx context.Context,
cancel context.CancelFunc,
command string,
sshConnectedToken string,
) (*SSHCommand, error) {

var userAtHost string
var sigChan chan os.Signal
var errChan chan error
port := "22"
host := strings.TrimPrefix(c.String("file"), "ssh://")

if remotePort := strings.Split(host, ":"); len(remotePort) == 2 {
port = remotePort[1]
userAtHost = remotePort[0]
} else {
userAtHost = host
}

args := c.StringSlice("ssh-args")
// Check if port is specified explicitly with the --ssh-args flag
addPort := true
for _, arg := range args {
if strings.Contains(arg, "-p") {
addPort = false
break
}
}
if addPort {
args = append(args, "-p", port)
}
args = append(args, userAtHost)
args = append(
args,
"/bin/sh",
"-c",
command)

cmd := exec.Command("ssh", args...)

// Simply connect stdin/stderr
cmd.Stdin = os.Stdin
cmd.Stderr = os.Stderr
stdout, err := cmd.StdoutPipe()
if err != nil {
return nil, errors.New("Error redirecting stdout on exec")
}

// Disable tty echo before starting
term, err := DisableEcho(int(os.Stdin.Fd()))
if err == nil {
sigChan = make(chan os.Signal, 1)
errChan = make(chan error, 1)
// Make sure that echo is enabled if the process gets
// interrupted
signal.Notify(sigChan)
go EchoSigHandler(_ctx, sigChan, errChan, term)
} else if err != syscall.ENOTTY {
return nil, err
}

if err := cmd.Start(); err != nil {
return nil, err
}

// Wait for 60 seconds for ssh to establish connection
err = waitForBufferSignal(stdout, os.Stdout, sshConnectedToken, 2*time.Minute)
if err != nil {
_ = cmd.Process.Kill()
return nil, errors.Wrap(err,
"Error waiting for ssh session to be established.")
}
return &SSHCommand{
ctx: _ctx,
Cmd: cmd,
Stdout: stdout,
cancel: cancel,
sigChan: sigChan,
errChan: errChan,
}, nil
}

func (s *SSHCommand) EndSSHCommand() error {
if s.Cmd.ProcessState != nil && s.Cmd.ProcessState.Exited() {
return errors.New("SSH session closed unexpectedly")
}

if err := s.Cmd.Wait(); err != nil {
return errors.Wrap(err,
"SSH session closed with error")
}

if s.sigChan != nil {
signal.Stop(s.sigChan)
s.cancel()
if err := <-s.errChan; err != nil {
return err
}
} else {
s.cancel()
}

return nil
}

// Reads from src waiting for the string specified by signal, writing all other
// output appearing at src to sink. The function returns an error if occurs
// reading from the stream or the deadline exceeds.
func waitForBufferSignal(src io.Reader, sink io.Writer,
signal string, deadline time.Duration) error {

var err error
errChan := make(chan error)

go func() {
stdoutRdr := bufio.NewReader(src)
for {
line, err := stdoutRdr.ReadString('\n')
if err != nil {
errChan <- err
break
}
if strings.Contains(line, signal) {
errChan <- nil
break
}
_, err = sink.Write([]byte(line + "\n"))
if err != nil {
errChan <- err
break
}
}
}()

select {
case err = <-errChan:
// Error from goroutine
case <-time.After(deadline):
err = errors.New("Input deadline exceeded")
Comment on lines +179 to +180
Copy link
Contributor

Choose a reason for hiding this comment

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

We could also reuse the deadline from the context in the parent scope here instead of a fixed duration.

Suggested change
case <-time.After(deadline):
err = errors.New("Input deadline exceeded")
case <-ctx.Done():
err = ctx.Err()

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure I understand, sorry - new to Go and new to the code above (just moved, not written myself)
Do you mean to reuse context as in, "if the execution finished, context was marked as Done, we return an error in waitForBufferSignal"?
My understanding here is that we wait either for error, or for 120 seconds for user to provide a root password over ssh. If user does not provide a password within 120 seconds, we return an error. I don't understand how reusing context could be used here.

}
return err
}
Loading