Surface plugin-not-found error for ssm start-session when the plugin is missing

awscli/customizations/sessionmanager.py

@@ -116,6 +116,23 @@ class StartSessionCaller(CLIOperationCaller):
     RECOMMENDED_MINIMUM_PLUGIN_VERSION = "1.2.764.0"
     DEFAULT_SSM_ENV_NAME = "AWS_SSM_START_SESSION_RESPONSE"
 
+    def _get_plugin_version(self):
+        """Return the session-manager-plugin version string.
+
+        Raises a ValueError with a helpful message when the plugin is not
+        installed instead of letting the raw OSError propagate.
+        """
+        try:
+            return check_output(
+                ["session-manager-plugin", "--version"], text=True
+            )
+        except OSError as ex:
+            if ex.errno == errno.ENOENT:
+                logger.debug('SessionManagerPlugin is not present',
+                             exc_info=True)
+                raise ValueError(''.join(ERROR_MESSAGE))
+            raise
+
     def _warn_if_plugin_version_is_outdated(self, plugin_version):
         """Warn when the plugin is older than the recommended minimum."""
         version_requirement = VersionRequirement(
@@ -139,6 +156,14 @@ def invoke(self, service_name, operation_name, parameters,
             service_name, region_name=parsed_globals.region,
             endpoint_url=parsed_globals.endpoint_url,
             verify=parsed_globals.verify_ssl)
+        # Check that the session-manager-plugin is installed before starting
+        # the session. Previously the plugin was only checked after the
+        # session had been started; when it was missing, the CLI called
+        # terminate_session to clean up, and if the caller lacked the
+        # ssm:TerminateSession permission that AccessDenied error masked the
+        # real "plugin not found" error. Failing fast also avoids creating a
+        # session that is immediately torn down.
+        plugin_version = self._get_plugin_version()
         response = client.start_session(**parameters)
         session_id = response['SessionId']
         region_name = client.meta.region_name
@@ -159,9 +184,6 @@ def invoke(self, service_name, operation_name, parameters,
             }
             start_session_response = json.dumps(session_parameters)
 
-            plugin_version = check_output(
-                ["session-manager-plugin", "--version"], text=True
-            )
             env = os.environ.copy()
 
             # Warn, but do not fail, when the plugin is older than the
@@ -207,6 +229,14 @@ def invoke(self, service_name, operation_name, parameters,
                 # session-manager-plugin. If plugin is not present, terminate
                 # is called so that service and ssm-agent terminates the
                 # session to avoid zombie session active on ssm-agent for
-                # default self terminate time
-                client.terminate_session(SessionId=session_id)
+                # default self terminate time. A failure of this cleanup
+                # call (e.g. the caller lacks ssm:TerminateSession
+                # permission) must not mask the plugin-not-found error
+                # below, which is the actionable message for the user.
+                try:
+                    client.terminate_session(SessionId=session_id)
+                except Exception:
+                    logger.debug('Failed to terminate session %s after the '
+                                 'session-manager-plugin was not found.',
+                                 session_id, exc_info=True)
                 raise ValueError(''.join(ERROR_MESSAGE))

tests/functional/ssm/test_start_session.py

@@ -331,24 +331,15 @@ def test_start_session_fails(self, mock_check_output, mock_check_call):
     def test_start_session_when_get_plugin_version_fails(
         self, mock_check_output, mock_check_call
     ):
+        # The plugin is checked before the session is started: when it is
+        # not installed the command fails fast with the plugin-not-found
+        # error and no API calls are made.
         cmdline = 'ssm start-session --target instance-id'
         mock_check_output.side_effect = OSError(errno.ENOENT, 'some error')
-        self.parsed_responses = [
-            {
-                "SessionId": "session-id",
-                "TokenValue": "token-value",
-                "StreamUrl": "stream-url",
-            }
-        ]
-        self.run_cmd(cmdline, expected_rc=255)
-        self.assertEqual(self.operations_called[0][0].name,
-                         'StartSession')
-        self.assertEqual(self.operations_called[0][1],
-                         {'Target': 'instance-id'})
-        self.assertEqual(self.operations_called[1][0].name,
-                         'TerminateSession')
-        self.assertEqual(self.operations_called[1][1],
-                         {'SessionId': 'session-id'})
+        stdout, stderr, rc = self.run_cmd(cmdline, expected_rc=255)
+        self.assertEqual(self.operations_called, [])
+        self.assertIn('SessionManagerPlugin is not found', stderr)
+        mock_check_call.assert_not_called()
 
 
 class TestHelpOutput(BaseAWSHelpOutputTest):

tests/unit/customizations/test_sessionmanager.py

@@ -38,7 +38,10 @@ def setUp(self):
         self.parsed_globals = mock.Mock()
         self.parsed_globals.profile = 'user_profile'
 
-    def test_start_session_when_non_custom_start_session_fails(self):
+    @mock.patch("awscli.customizations.sessionmanager.check_output")
+    def test_start_session_when_non_custom_start_session_fails(
+            self, mock_check_output):
+        mock_check_output.return_value = "1.2.500.0\n"
         self.client.start_session.side_effect = Exception('some exception')
         params = {}
         with self.assertRaisesRegex(Exception, 'some exception'):
@@ -226,23 +229,19 @@ def test_start_session_with_env_variable_success_scenario(
 
     @mock.patch("awscli.customizations.sessionmanager.check_call")
     @mock.patch("awscli.customizations.sessionmanager.check_output")
-    def test_start_session_when_check_output_fails(
+    def test_start_session_fails_fast_when_plugin_version_check_fails(
         self, mock_check_output, mock_check_call
     ):
+        # The plugin version is checked before the session is started, so a
+        # failure of that check must surface without any API calls.
         mock_check_output.side_effect = subprocess.CalledProcessError(
             returncode=1, cmd="session-manager-plugin", output="some error"
         )
 
         start_session_params = {
             "Target": "i-123456789"
         }
-        start_session_response = {
-            "SessionId": "session-id",
-            "TokenValue": "token-value",
-            "StreamUrl": "stream-url",
-        }
 
-        self.client.start_session.return_value = start_session_response
         with self.assertRaises(subprocess.CalledProcessError):
             self.caller.invoke(
                 "ssm",
@@ -251,13 +250,79 @@ def test_start_session_when_check_output_fails(
                 self.parsed_globals
             )
 
-        self.client.start_session.assert_called_with(**start_session_params)
+        self.client.start_session.assert_not_called()
         self.client.terminate_session.assert_not_called()
         mock_check_output.assert_called_with(
             ["session-manager-plugin", "--version"], text=True
         )
         mock_check_call.assert_not_called()
 
+    @mock.patch("awscli.customizations.sessionmanager.check_call")
+    @mock.patch("awscli.customizations.sessionmanager.check_output")
+    def test_start_session_when_plugin_not_installed(
+        self, mock_check_output, mock_check_call
+    ):
+        # When the plugin is not installed the CLI must fail fast with the
+        # actionable plugin-not-found error: no session may be started (which
+        # would then require a terminate_session cleanup call that can fail
+        # with a misleading AccessDenied error of its own).
+        mock_check_output.side_effect = OSError(
+            errno.ENOENT, 'session-manager-plugin not found'
+        )
+
+        start_session_params = {
+            "Target": "i-123456789"
+        }
+
+        with self.assertRaisesRegex(
+                ValueError, 'SessionManagerPlugin is not found'):
+            self.caller.invoke(
+                "ssm",
+                "StartSession",
+                start_session_params,
+                self.parsed_globals
+            )
+
+        self.client.start_session.assert_not_called()
+        self.client.terminate_session.assert_not_called()
+        mock_check_call.assert_not_called()
+
+    @mock.patch("awscli.customizations.sessionmanager.check_call")
+    @mock.patch("awscli.customizations.sessionmanager.check_output")
+    def test_start_session_plugin_not_found_error_not_masked_by_cleanup(
+        self, mock_check_output, mock_check_call
+    ):
+        # If the plugin disappears after the pre-flight check, the
+        # terminate_session cleanup of the started session must not mask
+        # the plugin-not-found error (e.g. when the caller lacks the
+        # ssm:TerminateSession permission).
+        mock_check_output.return_value = "1.2.500.0\n"
+        mock_check_call.side_effect = OSError(errno.ENOENT, 'some error')
+        self.client.terminate_session.side_effect = Exception(
+            'AccessDenied')
+
+        start_session_params = {
+            "Target": "i-123456789"
+        }
+        start_session_response = {
+            "SessionId": "session-id",
+            "TokenValue": "token-value",
+            "StreamUrl": "stream-url",
+        }
+        self.client.start_session.return_value = start_session_response
+
+        with self.assertRaisesRegex(
+                ValueError, 'SessionManagerPlugin is not found'):
+            self.caller.invoke(
+                "ssm",
+                "StartSession",
+                start_session_params,
+                self.parsed_globals
+            )
+
+        self.client.terminate_session.assert_called_with(
+            SessionId="session-id")
+
     @mock.patch("awscli.customizations.sessionmanager.check_call")
     @mock.patch("awscli.customizations.sessionmanager.check_output")
     def test_start_session_when_response_not_json(