Merge pull request #3826 from canonical/fix-console-creation-logic

Fix console creation

include/multipass/console.h

@@ -28,8 +28,6 @@
 
 namespace multipass
 {
-class Terminal;
-
 class Console : private DisabledCopyMove
 {
 public:
@@ -47,7 +45,6 @@ class Console : private DisabledCopyMove
     virtual void write_console() = 0;
     virtual void exit_console() = 0;
 
-    static UPtr make_console(ssh_channel channel, Terminal* term);
     static void setup_environment();
 
 protected:

include/multipass/terminal.h

@@ -19,12 +19,15 @@
 #define MULTIPASS_TERMINAL_H
 
 #include <istream>
+#include <libssh/libssh.h>
 #include <memory>
 #include <ostream>
 #include <string>
 
 namespace multipass
 {
+class Console;
+
 class Terminal
 {
 public:
@@ -44,6 +47,9 @@ class Terminal
 
     using UPtr = std::unique_ptr<Terminal>;
     static UPtr make_terminal();
+
+    using ConsolePtr = std::unique_ptr<Console>;
+    virtual ConsolePtr make_console(ssh_channel channel) = 0;
 };
 } // namespace multipass
 

src/client/cli/cmd/exec.cpp

@@ -160,7 +160,7 @@ mp::ReturnCode cmd::Exec::exec_success(const mp::SSHInfoReply& reply, const std:
 
     try
     {
-        auto console_creator = [&term](auto channel) { return Console::make_console(channel, term); };
+        auto console_creator = [&term](auto channel) { return term->make_console(channel); };
         mp::SSHClient ssh_client{host, port, username, priv_key_blob, console_creator};
 
         std::vector<std::vector<std::string>> all_args;

src/client/cli/cmd/shell.cpp

@@ -74,7 +74,7 @@ mp::ReturnCode cmd::Shell::run(mp::ArgParser* parser)
 
         try
         {
-            auto console_creator = [this](auto channel) { return Console::make_console(channel, term); };
+            auto console_creator = [this](auto channel) { return term->make_console(channel); };
             mp::SSHClient ssh_client{host, port, username, priv_key_blob, console_creator};
             ssh_client.connect();
         }

src/platform/console/console.cpp

@@ -23,11 +23,6 @@
 
 namespace mp = multipass;
 
-mp::Console::UPtr mp::Console::make_console(ssh_channel channel, Terminal* term)
-{
-    return std::make_unique<UnixConsole>(channel, static_cast<UnixTerminal*>(term));
-}
-
 void mp::Console::setup_environment()
 {
     UnixConsole::setup_environment();

src/platform/console/unix_terminal.cpp

@@ -22,6 +22,8 @@
 #include <termios.h>
 #include <unistd.h>
 
+#include "unix_console.h"
+
 namespace mp = multipass;
 
 int mp::UnixTerminal::cin_fd() const
@@ -56,3 +58,8 @@ void mp::UnixTerminal::set_cin_echo(const bool enable)
 
     tcsetattr(cin_fd(), TCSANOW, &tty);
 }
+
+mp::UnixTerminal::ConsolePtr mp::UnixTerminal::make_console(ssh_channel channel)
+{
+    return std::make_unique<UnixConsole>(channel, this);
+}

src/platform/console/unix_terminal.h

@@ -32,6 +32,8 @@ class UnixTerminal : public Terminal
     bool cout_is_live() const override;
 
     void set_cin_echo(const bool enable) override;
+
+    ConsolePtr make_console(ssh_channel channel) override;
 };
 } // namespace multipass
 

tests/mock_terminal.h

@@ -34,6 +34,7 @@ struct MockTerminal : public Terminal
     MOCK_METHOD(bool, cin_is_live, (), (const, override));
     MOCK_METHOD(bool, cout_is_live, (), (const, override));
     MOCK_METHOD(void, set_cin_echo, (const bool), (override));
+    MOCK_METHOD(ConsolePtr, make_console, (ssh_channel), (override));
 };
 } // namespace test
 } // namespace multipass

tests/stub_terminal.h

@@ -20,6 +20,8 @@
 
 #include <multipass/terminal.h>
 
+#include "stub_console.h"
+
 namespace multipass
 {
 namespace test
@@ -61,6 +63,11 @@ class StubTerminal : public multipass::Terminal
     {
     }
 
+    ConsolePtr make_console(ssh_channel channel) override
+    {
+        return std::make_unique<StubConsole>();
+    }
+
 private:
     std::ostream &cout_stream;
     std::ostream& cerr_stream;

tests/test_cli_client.cpp

@@ -445,6 +445,29 @@ auto make_info_function(const std::string& source_path = "", const std::string&
     return info_function;
 }
 
+mp::SSHInfo make_ssh_info(const std::string& host = "222.222.222.222",
+                          int port = 22,
+                          const std::string& priv_key = mpt::fake_key_data,
+                          const std::string& username = "user")
+{
+    mp::SSHInfo ssh_info;
+
+    ssh_info.set_host(host);
+    ssh_info.set_port(port);
+    ssh_info.set_priv_key_base64(priv_key);
+    ssh_info.set_username(username);
+
+    return ssh_info;
+}
+
+mp::SSHInfoReply make_fake_ssh_info_response(const std::string& instance_name)
+{
+    mp::SSHInfoReply response;
+    (*response.mutable_ssh_info())[instance_name] = make_ssh_info();
+
+    return response;
+}
+
 typedef std::vector<std::pair<std::string, mp::AliasDefinition>> AliasesVector;
 
 const std::string csv_header{"Alias,Instance,Command,Working directory,Context\n"};
@@ -743,6 +766,27 @@ TEST_F(Client, shell_cmd_no_args_targets_petenv)
     EXPECT_THAT(send_command({"shell"}), Eq(mp::ReturnCode::Ok));
 }
 
+TEST_F(Client, shell_cmd_creates_console)
+{
+    EXPECT_CALL(mock_daemon, ssh_info)
+        .WillOnce([](auto, grpc::ServerReaderWriter<mp::SSHInfoReply, mp::SSHInfoRequest>* server) {
+            server->Write(make_fake_ssh_info_response("fake-instance"));
+            return grpc::Status{};
+        });
+
+    std::string error_string = "attempted to create console";
+    std::stringstream cerr_stream;
+
+    mpt::MockTerminal term{};
+    EXPECT_CALL(term, cin()).WillRepeatedly(ReturnRef(trash_stream));
+    EXPECT_CALL(term, cout()).WillRepeatedly(ReturnRef(trash_stream));
+    EXPECT_CALL(term, cerr()).WillRepeatedly(ReturnRef(cerr_stream));
+    EXPECT_CALL(term, make_console(_)).WillOnce(Throw(std::runtime_error(error_string)));
+
+    EXPECT_EQ(setup_client_and_run({"shell"}, term), mp::ReturnCode::CommandFail);
+    EXPECT_THAT(cerr_stream.str(), HasSubstr(error_string));
+}
+
 TEST_F(Client, shell_cmd_considers_configured_petenv)
 {
     const auto custom_petenv = "jarjar binks";
@@ -1545,27 +1589,6 @@ TEST_F(Client, exec_cmd_fails_on_other_absent_instance)
                 Eq(mp::ReturnCode::CommandFail));
 }
 
-mp::SSHInfo make_ssh_info(const std::string& host = "222.222.222.222", int port = 22,
-                          const std::string& priv_key = mpt::fake_key_data, const std::string& username = "user")
-{
-    mp::SSHInfo ssh_info;
-
-    ssh_info.set_host(host);
-    ssh_info.set_port(port);
-    ssh_info.set_priv_key_base64(priv_key);
-    ssh_info.set_username(username);
-
-    return ssh_info;
-}
-
-mp::SSHInfoReply make_fake_ssh_info_response(const std::string& instance_name)
-{
-    mp::SSHInfoReply response;
-    (*response.mutable_ssh_info())[instance_name] = make_ssh_info();
-
-    return response;
-}
-
 struct SSHClientReturnTest : Client, WithParamInterface<int>
 {
 };

tests/unix/test_unix_terminal.cpp

@@ -19,6 +19,7 @@
 
 #include "mock_libc_functions.h"
 
+#include <src/platform/console/unix_console.h>
 #include <src/platform/console/unix_terminal.h>
 
 #include <tuple>
@@ -110,3 +111,16 @@ TEST_F(TestUnixTerminal, unsetsEchoOnTerminal)
 
     unix_terminal.set_cin_echo(false);
 }
+
+TEST_F(TestUnixTerminal, make_console_makes_unix_console)
+{
+    // force is_live() to return false so UnixConsole ctor doesn't break
+    REPLACE(fileno, [this](auto) { return fake_fd; });
+    REPLACE(isatty, [this](auto fd) {
+        EXPECT_EQ(fd, fake_fd);
+        return 0;
+    });
+
+    auto console = unix_terminal.make_console(nullptr);
+    EXPECT_NE(dynamic_cast<multipass::UnixConsole*>(console.get()), nullptr);
+}