sonic: add --refresh-host-key option to refresh SSH host keys after r…

[berendt]

…edeployment

Adds an optional --refresh-host-key flag to all SSH-using sonic commands (load, backup, ztp, reload, reboot, reset, show). When set, existing known_hosts entries for the target host are removed before connecting, allowing the new host key to be accepted after a switch has been redeployed and the host key changed.

AI-assisted: Claude Code

[sourcery-ai]

Hey - I've found 2 issues

Prompt for AI Agents

Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="osism/commands/sonic.py" line_range="129-133" />
<code_context>
                 f"Could not initialize {KNOWN_HOSTS_PATH}, continuing with AutoAddPolicy"
             )

+        if refresh_host_key:
+            logger.info(
+                f"Refreshing SSH host key: removing known_hosts entries for {ssh_host}"
+            )
+            remove_known_hosts_entries(ssh_host, KNOWN_HOSTS_PATH)
+
         ssh = paramiko.SSHClient()
</code_context>
<issue_to_address>
**issue:** Guard host-key refresh with error handling to avoid hard failures on filesystem/permission issues.

If `remove_known_hosts_entries` raises (e.g., permissions, malformed file, concurrent changes to `KNOWN_HOSTS_PATH`), the command fails before connecting. Consider wrapping this call in `try/except`, logging a warning on failure, and proceeding with the connection so `--refresh-host-key` fails gracefully instead of aborting the workflow.
</issue_to_address>

### Comment 2
<location path="tests/unit/commands/test_sonic_ssh.py" line_range="11-19" />
<code_context>
+
+from osism.commands import sonic
+
+SSH_COMMAND_CLASSES = [
+    sonic.Load,
+    sonic.Backup,
+    sonic.Ztp,
+    sonic.Reload,
+    sonic.Reboot,
+    sonic.Reset,
+    sonic.Show,
+]
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests that verify each command’s `take_action` forwards `parsed_args.refresh_host_key` into `_create_ssh_connection`

The existing tests cover argument parsing and `SonicCommandBase._create_ssh_connection`, but they don’t verify that each concrete command actually passes `parsed_args.refresh_host_key` to `_create_ssh_connection` within `take_action`. This means one command could silently stop honoring the flag while others still work.

Please add a parametrized test over `SSH_COMMAND_CLASSES` that:
- instantiates each command,
- builds `parsed_args` with and without `--refresh-host-key`,
- patches `_create_ssh_connection` on the instance to a `MagicMock`,
- calls `take_action(parsed_args)`, and
- asserts `_create_ssh_connection` receives `refresh_host_key=True` when the flag is set and `False` otherwise.

That will ensure each SONiC command actually honors the new CLI option end-to-end.

Suggested implementation:

```python
# SPDX-License-Identifier: Apache-2.0

"""Tests for the --refresh-host-key option on SONiC SSH-using commands."""

from unittest.mock import MagicMock

import pytest

from osism.commands import sonic

SSH_COMMAND_CLASSES = [
    sonic.Load,
    sonic.Backup,
    sonic.Ztp,
    sonic.Reload,
    sonic.Reboot,
    sonic.Reset,
    sonic.Show,
]


@pytest.mark.parametrize("command_class", SSH_COMMAND_CLASSES)
@pytest.mark.parametrize("refresh_host_key", [True, False])
def test_sonic_commands_forward_refresh_host_key_to_ssh(command_class, refresh_host_key):
    """Each SONiC command must forward parsed_args.refresh_host_key to _create_ssh_connection."""
    # NOTE: cliff commands are usually instantiated with (app, app_args).
    # In tests we can safely pass None for both unless the command relies on them.
    command = command_class(app=None, app_args=None)

    # Patch _create_ssh_connection on the instance
    command._create_ssh_connection = MagicMock(name="_create_ssh_connection")

    # Build parsed_args with the desired refresh_host_key value
    parsed_args = MagicMock()
    parsed_args.refresh_host_key = refresh_host_key

    # Exercise
    command.take_action(parsed_args)

    # Verify that _create_ssh_connection was called with the correct flag
    assert command._create_ssh_connection.called, (
        f"_create_ssh_connection was not called in {command_class.__name__}.take_action"
    )
    _, _, kwargs = command._create_ssh_connection.mock_calls[0]
    assert kwargs.get("refresh_host_key") is refresh_host_key, (
        f"{command_class.__name__}.take_action did not forward "
        "parsed_args.refresh_host_key to _create_ssh_connection"
    )

```

This implementation assumes:
1. All SONiC command classes (`Load`, `Backup`, `Ztp`, `Reload`, `Reboot`, `Reset`, `Show`) can be instantiated with `app=None, app_args=None`. If any require specific `app`/`app_args`, adjust the instantiation accordingly in the test.
2. `take_action` on each command only requires `parsed_args.refresh_host_key`. If some commands require additional attributes on `parsed_args` (e.g. `host`, `username`, etc.), you should set those on the `parsed_args` `MagicMock` before calling `take_action`.
3. `_create_ssh_connection` is called with `refresh_host_key` as a keyword argument. If the real implementation passes it positionally, update the assertion to inspect `args` instead of `kwargs`.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[osfrickler]

the code looks reasonable to me. I assume the unit tests are fine, but it would take a lot more time to verify that they really cover all reasonable scenarios, so I've skipped that part