Support SSH proxy command #493
Conversation
Signed-off-by: zjgemi <[email protected]>
📝 WalkthroughWalkthroughThe changes introduce an optional Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SSHContext
participant ProxyCommand
User->>SSHContext: Initialize with proxy_command
SSHContext->>SSHContext: Set proxy_command attribute
SSHContext->>SSHContext: Call _setup_ssh()
alt proxy_command provided
SSHContext->>ProxyCommand: Create ProxyCommand object
ProxyCommand->>SSHContext: Return ProxyCommand object
else proxy_command not provided
SSHContext->>SSHContext: Create standard socket connection
end
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #493 +/- ##
===========================================
- Coverage 60.19% 47.80% -12.40%
===========================================
Files 39 39
Lines 3859 3864 +5
===========================================
- Hits 2323 1847 -476
- Misses 1536 2017 +481 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
dpdispatcher/contexts/ssh_context.py (1)
352-352: Clarify the documentation forproxy_commandThe current description
"The command of proxy"may be ambiguous to users. Consider enhancing it to provide more clarity, such as:-doc_proxy_command = "The command of proxy." +doc_proxy_command = "The command used to establish a proxy for SSH connections (e.g., 'ssh -W %h:%p proxy.example.com')."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- dpdispatcher/contexts/ssh_context.py (5 hunks)
🧰 Additional context used
GitHub Check: codecov/patch
dpdispatcher/contexts/ssh_context.py
[warning] 147-148: dpdispatcher/contexts/ssh_context.py#L147-L148
Added lines #L147 - L148 were not covered by tests
| if self.proxy_command is not None: | ||
| sock = paramiko.ProxyCommand(self.proxy_command) | ||
| sock.settimeout(self.timeout) |
There was a problem hiding this comment.
Add unit tests for the proxy_command functionality
The newly added code handling the proxy_command is not covered by existing tests. To ensure reliability and prevent future regressions, please add unit tests that verify the behavior when a proxy_command is provided.
🧰 Tools
GitHub Check: codecov/patch
[warning] 147-148: dpdispatcher/contexts/ssh_context.py#L147-L148
Added lines #L147 - L148 were not covered by tests
There was a problem hiding this comment.
I agree with the AI. A unit test should be added.
There was a problem hiding this comment.
@njzjz, I'll proceed to add unit tests for the proxy_command functionality as requested. Here's a test case that verifies the behavior when a proxy_command is provided:
import unittest
from unittest.mock import patch, MagicMock
from dpdispatcher.contexts.ssh_context import SSHSession
class TestSSHSessionProxyCommand(unittest.TestCase):
@patch('paramiko.ProxyCommand')
@patch('socket.socket')
def test_setup_ssh_with_proxy_command(self, mock_socket, mock_proxy_command):
# Mock the ProxyCommand and socket behavior
mock_proxy = MagicMock()
mock_proxy_command.return_value = mock_proxy
# Create an SSHSession with a proxy command
session = SSHSession(
hostname="example.com",
username="user",
proxy_command="proxy_command_example"
)
# Ensure that ProxyCommand is used instead of socket
mock_proxy_command.assert_called_once_with("proxy_command_example")
mock_proxy.settimeout.assert_called_once_with(session.timeout)
mock_socket.assert_not_called() # Ensure socket is not used
@patch('paramiko.ProxyCommand')
@patch('socket.socket')
def test_setup_ssh_without_proxy_command(self, mock_socket, mock_proxy_command):
# Create an SSHSession without a proxy command
session = SSHSession(
hostname="example.com",
username="user"
)
# Ensure that socket is used instead of ProxyCommand
mock_socket.assert_called_once_with(socket.AF_INET, socket.SOCK_STREAM)
mock_socket().settimeout.assert_called_once_with(session.timeout)
mock_socket().connect.assert_called_once_with(("example.com", 22))
mock_proxy_command.assert_not_called() # Ensure ProxyCommand is not used
if __name__ == '__main__':
unittest.main()Explanation:
- Test 1 (
test_setup_ssh_with_proxy_command): Verifies that when aproxy_commandis provided,paramiko.ProxyCommandis used, and the standard socket connection is not invoked. - Test 2 (
test_setup_ssh_without_proxy_command): Verifies that when noproxy_commandis provided, the standard socket connection is used.
This should ensure that the proxy_command functionality is properly tested and prevent future regressions.
Let me know if you'd like any further adjustments!
| Argument( | ||
| "proxy_command", | ||
| str, | ||
| optional=True, | ||
| default=None, | ||
| doc=doc_proxy_command, | ||
| ), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure consistency in argument definitions
The Argument definition for proxy_command should align with the improved documentation string. Please update the doc parameter accordingly to reflect the clarified description.
| else: | ||
| sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) | ||
| sock.settimeout(self.timeout) | ||
| sock.connect((self.hostname, self.port)) | ||
|
|
||
| # Make a Paramiko Transport object using the socket | ||
| ts = paramiko.Transport(sock) |
There was a problem hiding this comment.
Could you ignore this line before python/typeshed#12697 is accepted?
| if self.proxy_command is not None: | ||
| sock = paramiko.ProxyCommand(self.proxy_command) | ||
| sock.settimeout(self.timeout) |
There was a problem hiding this comment.
I agree with the AI. A unit test should be added.
Summary by CodeRabbit
New Features
proxy_commandparameter for SSH connections, allowing users to specify a command for proxying.Documentation
proxy_commandargument, clarifying its purpose and usage.