Merge pull request #519 from redhat-performance/development

feature and fixes sync from development branch

Author: grafuls

README.md

@@ -13,6 +13,4 @@
     else:
         raise RuntimeError("Unable to find version string in src/badfish/__init__.py")
 
-setuptools.setup(
-    version=current_version
-)
+setuptools.setup(version=current_version)

setup.py

@@ -10,8 +10,8 @@ def create_parser():
         allow_abbrev=False,
     )
     parser.add_argument("-H", "--host", help="iDRAC host address")
-    parser.add_argument("-u", help="iDRAC username", required=True)
-    parser.add_argument("-p", help="iDRAC password", required=True)
+    parser.add_argument("-u", help="iDRAC username")
+    parser.add_argument("-p", help="iDRAC password")
     parser.add_argument("-i", help="Path to iDRAC interfaces yaml", default=None)
     parser.add_argument("-t", help="Type of host as defined on iDRAC interfaces yaml")
     parser.add_argument("-l", "--log", help="Optional argument for logging results to a file")

src/badfish/helpers/parser.py

@@ -382,9 +382,15 @@ async def find_session_uri(self):
         if not response:
             raise BadfishException(f"Failed to communicate with {self.host}")
 
+        if response.status == 401:
+            raise BadfishException(f"Failed to authenticate. Verify your credentials for {self.host}")
+
         raw = await response.text("utf-8", "ignore")
         data = json.loads(raw.strip())
-        redfish_version = int(data["RedfishVersion"].replace(".", ""))
+        try:
+            redfish_version = int(data["RedfishVersion"].replace(".", ""))
+        except KeyError:
+            raise BadfishException("Was unable to get Redfish Version. Please verify credentials/host.")
         session_uri = None
         if redfish_version >= 160:
             session_uri = "/redfish/v1/SessionService/Sessions"
@@ -2405,7 +2411,7 @@ async def set_nic_attribute(self, fqdd, attribute, value):
             )
             self.logger.debug(uri)
         except (IndexError, ValueError):
-            self.logger.error("Invalid FQDD suplied.")
+            self.logger.error("Invalid FQDD supplied.")
             return False
 
         headers = {"content-type": "application/json"}
@@ -2450,8 +2456,19 @@ async def set_nic_attribute(self, fqdd, attribute, value):
 
 
 async def execute_badfish(_host, _args, logger, format_handler=None):
-    _username = _args["u"]
-    _password = _args["p"]
+    _username = _args.get("u") or os.environ.get("BADFISH_USERNAME")
+    _password = _args.get("p") or os.environ.get("BADFISH_PASSWORD")
+
+    if _args.get("p"):
+        logger.warning(
+            "Passing secrets via command line arguments can be unsafe. "
+            "Consider using environment variables (BADFISH_USERNAME, BADFISH_PASSWORD)."
+        )
+
+    if not _username or not _password:
+        logger.error("Missing credentials. Please provide credentials via CLI arguments or environment variables.")
+        return _host, False
+
     host_type = _args["t"]
     interfaces_path = _args["i"]
     force = _args["force"]

src/badfish/main.py

@@ -924,18 +924,15 @@ def render_device_dict(index, device):
 BIOS_PASS_SET_MISS_ARG = """\
 - ERROR    - Missing argument: `--new-password`
 """
-BIOS_PASS_RM_GOOD = (
-    """\
+BIOS_PASS_RM_GOOD = """\
 - INFO     - Command passed to set BIOS password.
 - WARNING  - Host will now be rebooted for changes to take place.
 - INFO     - Command passed to On server, code return is 200.
 - INFO     - JobID: %s
 - INFO     - Name: Task
 - INFO     - Message: Job completed successfully.
 - INFO     - PercentComplete: 100
-"""
-    % JOB_ID
-)
+""" % JOB_ID
 BIOS_PASS_RM_MISS_ARG = """\
 - ERROR    - Missing argument: `--old-password`
 """
@@ -1010,13 +1007,10 @@ def render_device_dict(index, device):
 - INFO     - Polling for host state: Not Down
 - INFO     - Command passed to On server, code return is 200.
 """
-BIOS_SET_BAD_VALUE = (
-    """\
+BIOS_SET_BAD_VALUE = """\
 - WARNING  - List of accepted values for '%s': ['Enabled', 'Disabled']
 - ERROR    - Value not accepted
-"""
-    % ATTRIBUTE_OK
-)
+""" % ATTRIBUTE_OK
 BIOS_SET_BAD_ATTR = """\
 - WARNING  - Could not retrieve Bios Attributes.
 - ERROR    - NotThere not found. Please check attribute name.
@@ -1039,13 +1033,10 @@ def render_device_dict(index, device):
 - INFO     - WarningText: None
 - INFO     - WriteOnly: False
 """
-BIOS_GET_ONE_BAD = (
-    """\
+BIOS_GET_ONE_BAD = """\
 - WARNING  - Could not retrieve Bios Attributes.
 - ERROR    - Unable to locate the Bios attribute: %s
-"""
-    % ATTRIBUTE_BAD
-)
+""" % ATTRIBUTE_BAD
 NEXT_BOOT_PXE_OK = '- INFO     - PATCH command passed to set next boot onetime boot device to: "Pxe".\n'
 NEXT_BOOT_PXE_BAD = (
     "- ERROR    - Command failed, error code is 400.\n" "- ERROR    - Error reading response from host.\n"
@@ -2347,3 +2338,5 @@ def render_device_dict(index, device):
 {RESPONSE_UNSOPPORTED_IDRAC_VERSION}
 {RESPONSE_NIC_ATTR_GET_ERROR}
 """
+MANAGER_INSTANCE_RESP = '{"Jobs":{"@odata.id":"/redfish/v1/Managers/iDRAC.Embedded.1/Jobs"}}'
+JOBS_RESP = '{"Members":[]}'

tests/config.py

@@ -4,25 +4,29 @@
 
 
 class TestAsyncioFix(unittest.TestCase):
-    @patch('badfish.main.execute_badfish')
-    @patch('badfish.main.BadfishLogger')
-    @patch('badfish.main.parse_arguments')
-    @patch('asyncio.set_event_loop')
-    @patch('asyncio.new_event_loop')
-    @patch('asyncio.get_event_loop')
-    def test_main_handles_no_event_loop(self, mock_get_loop, mock_new_loop,
-                                        mock_set_loop, mock_parse_args,
-                                        mock_logger, mock_execute):
+    @patch("badfish.main.execute_badfish")
+    @patch("badfish.main.BadfishLogger")
+    @patch("badfish.main.parse_arguments")
+    @patch("asyncio.set_event_loop")
+    @patch("asyncio.new_event_loop")
+    @patch("asyncio.get_event_loop")
+    def test_main_handles_no_event_loop(
+        self, mock_get_loop, mock_new_loop, mock_set_loop, mock_parse_args, mock_logger, mock_execute
+    ):
         mock_get_loop.side_effect = RuntimeError("No event loop")
 
         mock_loop_instance = MagicMock()
         mock_new_loop.return_value = mock_loop_instance
         mock_loop_instance.run_until_complete.return_value = ("localhost", True)
 
         mock_parse_args.return_value = {
-            "verbose": False, "host": "localhost", "delta": None,
-            "firmware_inventory": None, "host_list": None, "log": None,
-            "output": None
+            "verbose": False,
+            "host": "localhost",
+            "delta": None,
+            "firmware_inventory": None,
+            "host_list": None,
+            "log": None,
+            "output": None,
         }
 
         main()
@@ -32,24 +36,28 @@ def test_main_handles_no_event_loop(self, mock_get_loop, mock_new_loop,
         mock_set_loop.assert_called_once_with(mock_loop_instance)
         mock_loop_instance.run_until_complete.assert_called()
 
-    @patch('badfish.main.execute_badfish')
-    @patch('badfish.main.BadfishLogger')
-    @patch('badfish.main.parse_arguments')
-    @patch('asyncio.set_event_loop')
-    @patch('asyncio.new_event_loop')
-    @patch('asyncio.get_event_loop')
-    def test_main_uses_existing_loop(self, mock_get_loop, mock_new_loop,
-                                     mock_set_loop, mock_parse_args,
-                                     mock_logger, mock_execute):
+    @patch("badfish.main.execute_badfish")
+    @patch("badfish.main.BadfishLogger")
+    @patch("badfish.main.parse_arguments")
+    @patch("asyncio.set_event_loop")
+    @patch("asyncio.new_event_loop")
+    @patch("asyncio.get_event_loop")
+    def test_main_uses_existing_loop(
+        self, mock_get_loop, mock_new_loop, mock_set_loop, mock_parse_args, mock_logger, mock_execute
+    ):
         existing_loop = MagicMock()
         mock_get_loop.return_value = existing_loop
         mock_get_loop.side_effect = None
         existing_loop.run_until_complete.return_value = ("localhost", True)
 
         mock_parse_args.return_value = {
-            "verbose": False, "host": "localhost", "delta": None,
-            "firmware_inventory": None, "host_list": None, "log": None,
-            "output": None
+            "verbose": False,
+            "host": "localhost",
+            "delta": None,
+            "firmware_inventory": None,
+            "host_list": None,
+            "log": None,
+            "output": None,
         }
 
         main()

tests/test_async_loop.py

@@ -1,4 +1,5 @@
 import sys
+import os
 from unittest.mock import AsyncMock, MagicMock, PropertyMock, patch
 
 import pytest
@@ -49,25 +50,35 @@ def capture_wrap(self):
         yield
 
     def badfish_call(
-        self,
-        mock_host=config.MOCK_HOST,
-        mock_user=config.MOCK_USER,
-        mock_pass=config.MOCK_PASS,
+        self, mock_host=config.MOCK_HOST, mock_user=config.MOCK_USER, mock_pass=config.MOCK_PASS, use_cli_secrets=False
     ):
         argv = []
+        env_vars = os.environ.copy()
 
         if mock_host is not None:
             argv.extend(("-H", mock_host))
-        if mock_user is not None:
-            argv.extend(("-u", mock_user))
-        if mock_pass is not None:
-            argv.extend(("-p", mock_pass))
-
-        argv.extend(self.args)
-        try:
-            main(argv)
-        except BadfishException:
-            pass
+
+        if use_cli_secrets:
+            # Legacy behavior: Pass secrets via CLI args to test warning logic
+            if mock_user is not None:
+                argv.extend(("-u", mock_user))
+            if mock_pass is not None:
+                argv.extend(("-p", mock_pass))
+            argv.extend(self.args)
+        else:
+            # Default behavior for tests: Use Env Vars to suppress warnings
+            if mock_user is not None:
+                env_vars["BADFISH_USERNAME"] = mock_user
+            if mock_pass is not None:
+                env_vars["BADFISH_PASSWORD"] = mock_pass
+            argv.extend(self.args)
+
+        with patch.dict(os.environ, env_vars):
+            try:
+                main(argv)
+            except BadfishException:
+                pass
+
         out, err = self._capsys.readouterr()
         return out, err
 

tests/test_base.py

@@ -13,8 +13,11 @@
     RESPONSE_INIT_SYSTEMS_RESOURCE_NOT_FOUND,
     ROOT_RESP,
     SUCCESSFUL_HOST_LIST,
+    SYS_RESP,
     WRONG_BADFISH_EXECUTION,
     WRONG_BADFISH_EXECUTION_HOST_LIST,
+    MANAGER_INSTANCE_RESP,
+    JOBS_RESP,
 )
 from tests.test_base import TestBase
 
@@ -82,6 +85,21 @@ def test_host_list_extras(self, mock_get, mock_post, mock_delete):
 class TestInitialization(TestBase):
     args = ["--ls-jobs"]
 
+    @patch("aiohttp.ClientSession.delete")
+    @patch("aiohttp.ClientSession.post")
+    @patch("aiohttp.ClientSession.get")
+    def test_cli_secrets_warning(self, mock_get, mock_post, mock_delete):
+        """Test that passing credentials via CLI triggers a warning."""
+        responses = [ROOT_RESP] * 4 + [SYS_RESP, MAN_RESP, MANAGER_INSTANCE_RESP, JOBS_RESP]
+        self.set_mock_response(mock_get, 200, responses)
+        self.set_mock_response(mock_post, 200, "OK")
+        self.set_mock_response(mock_delete, 200, "OK")
+
+        # Explicitly use CLI secrets to trigger the warning
+        _, err = self.badfish_call(use_cli_secrets=True)
+
+        assert "Passing secrets via command line arguments can be unsafe" in err
+
     @patch("aiohttp.ClientSession.delete")
     @patch("aiohttp.ClientSession.post")
     @patch("aiohttp.ClientSession.get")

tests/test_execution.py

@@ -24,10 +24,7 @@ def test_hosts_good(self):
         for call in badfish_mock.await_args_list:
             _host, _args, _logger, _fh = call[0]
             assert _host == config.MOCK_HOST
-
             assert _args["host_list"] == self.mock_hosts_good_path
-            assert _args["u"] == config.MOCK_USER
-            assert _args["p"] == config.MOCK_PASS
 
     def test_hosts_non_existent(self):
         self.args = [self.option_arg, "non/existent/file"]
@@ -61,7 +58,4 @@ def test_hosts_bad(self):
         for call in badfish_mock.await_args_list:
             _host, _args, _logger, _fh = call[0]
             assert _host == config.MOCK_HOST
-
             assert _args["host_list"] == self.mock_hosts_garbled_path
-            assert _args["u"] == config.MOCK_USER
-            assert _args["p"] == config.MOCK_PASS