-
-
Notifications
You must be signed in to change notification settings - Fork 66
feat: enforce proxy dial timeouts and improve timeout handling #98
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ package easyssh | |
|
||
import ( | ||
"context" | ||
"errors" | ||
"os" | ||
"os/user" | ||
"path" | ||
|
@@ -512,3 +513,117 @@ func TestCommandTimeout(t *testing.T) { | |
assert.NotNil(t, err) | ||
assert.Equal(t, "Run Command Timeout: "+context.DeadlineExceeded.Error(), err.Error()) | ||
} | ||
|
||
// TestProxyTimeoutHandling tests that timeout is properly respected when using proxy connections | ||
// This test uses a non-existent proxy server to force a timeout during proxy connection | ||
func TestProxyTimeoutHandling(t *testing.T) { | ||
ssh := &MakeConfig{ | ||
Server: "example.com", | ||
User: "testuser", | ||
Port: "22", | ||
KeyPath: "./tests/.ssh/id_rsa", | ||
Timeout: 1 * time.Second, // Short timeout for testing | ||
Proxy: DefaultConfig{ | ||
User: "testuser", | ||
Server: "10.255.255.1", // Non-routable IP that should timeout | ||
Port: "22", | ||
KeyPath: "./tests/.ssh/id_rsa", | ||
Timeout: 1 * time.Second, | ||
}, | ||
} | ||
|
||
// Test Connect() method directly to test proxy connection timeout | ||
start := time.Now() | ||
session, client, err := ssh.Connect() | ||
elapsed := time.Since(start) | ||
|
||
// Should timeout within reasonable bounds | ||
assert.True(t, elapsed < 3*time.Second, "Connection should timeout within 3 seconds, took %v", elapsed) | ||
assert.True(t, elapsed >= 1*time.Second, "Connection should take at least 1 second (timeout value), took %v", elapsed) | ||
|
||
// Should return nil session and client | ||
assert.Nil(t, session) | ||
assert.Nil(t, client) | ||
|
||
// Should have error | ||
assert.NotNil(t, err) | ||
} | ||
Comment on lines
+520
to
+551
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a good test for proxy connection timeouts. To make it more specific, you could also assert on the content of the error message to ensure it's a timeout error as expected. For example: assert.ErrorContains(t, err, "i/o timeout") This would make the test more robust by verifying not just that an error occurred, but that the correct error occurred. |
||
|
||
// TestProxyDialTimeout tests the specific scenario described in issue #93 | ||
// where proxy dial timeout should be respected and properly detected | ||
func TestProxyDialTimeout(t *testing.T) { | ||
ssh := &MakeConfig{ | ||
Server: "10.255.255.1", // Non-routable IP that should timeout | ||
User: "testuser", | ||
Port: "22", | ||
KeyPath: "./tests/.ssh/id_rsa", | ||
Timeout: 2 * time.Second, // Short timeout for testing | ||
Proxy: DefaultConfig{ | ||
User: "testuser", | ||
Server: "10.255.255.2", // Another non-routable IP for proxy | ||
Port: "22", | ||
KeyPath: "./tests/.ssh/id_rsa", | ||
Timeout: 2 * time.Second, | ||
}, | ||
} | ||
|
||
// Test Connect() method directly to avoid SSH server dependency | ||
start := time.Now() | ||
session, client, err := ssh.Connect() | ||
elapsed := time.Since(start) | ||
|
||
// Should timeout within reasonable bounds | ||
assert.True(t, elapsed < 5*time.Second, "Connection should timeout within 5 seconds, took %v", elapsed) | ||
assert.True(t, elapsed >= 2*time.Second, "Connection should take at least 2 seconds (timeout value), took %v", elapsed) | ||
|
||
// Should return nil session and client | ||
assert.Nil(t, session) | ||
assert.Nil(t, client) | ||
|
||
// Should have error | ||
assert.NotNil(t, err) | ||
// Note: This will timeout at the proxy connection level, not at proxy dial level | ||
// so it won't be ErrProxyDialTimeout, but we can still verify the timeout behavior | ||
} | ||
Comment on lines
+553
to
+588
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test does not verify what its name suggests. The test name Consider one of the following:
Apply this diff for option 1 (renaming): -// TestProxyDialTimeout tests the specific scenario described in issue #93
-// where proxy dial timeout should be respected and properly detected
-func TestProxyDialTimeout(t *testing.T) {
+// TestProxyConnectionTimeout tests timeout behavior when connecting to proxy
+// This tests proxy connection timeout, not proxy dial timeout
+func TestProxyConnectionTimeout(t *testing.T) { Or for option 2, modify the test to use a working proxy but non-routable target: ssh := &MakeConfig{
Server: "10.255.255.1", // Non-routable IP that should timeout
User: "testuser",
Port: "22",
KeyPath: "./tests/.ssh/id_rsa",
Timeout: 2 * time.Second, // Short timeout for testing
Proxy: DefaultConfig{
User: "testuser",
- Server: "10.255.255.2", // Another non-routable IP for proxy
+ Server: "localhost", // Use working proxy
Port: "22",
KeyPath: "./tests/.ssh/id_rsa",
Timeout: 2 * time.Second,
},
} Then update the comment and add an assertion for - // Should have error
assert.NotNil(t, err)
- // Note: This will timeout at the proxy connection level, not at proxy dial level
- // so it won't be ErrProxyDialTimeout, but we can still verify the timeout behavior
+ // Should specifically be a proxy dial timeout
+ assert.ErrorIs(t, err, ErrProxyDialTimeout) |
||
|
||
// TestProxyDialTimeoutInRun tests timeout detection in Run method | ||
func TestProxyDialTimeoutInRun(t *testing.T) { | ||
ssh := &MakeConfig{ | ||
Server: "example.com", | ||
User: "testuser", | ||
Port: "22", | ||
KeyPath: "./tests/.ssh/id_rsa", | ||
Timeout: 2 * time.Second, | ||
Proxy: DefaultConfig{ | ||
User: "testuser", | ||
Server: "127.0.0.1", // Assume localhost SSH exists | ||
Port: "22", | ||
KeyPath: "./tests/.ssh/id_rsa", | ||
Timeout: 2 * time.Second, | ||
}, | ||
} | ||
|
||
// Mock a scenario where Connect() returns ErrProxyDialTimeout | ||
// by temporarily changing the target to a non-routable address | ||
ssh.Server = "10.255.255.1" | ||
|
||
start := time.Now() | ||
outStr, errStr, isTimeout, err := ssh.Run("whoami") | ||
elapsed := time.Since(start) | ||
|
||
// Should timeout within reasonable bounds | ||
assert.True(t, elapsed < 5*time.Second, "Should timeout within 5 seconds, took %v", elapsed) | ||
|
||
// Should return empty output | ||
assert.Equal(t, "", outStr) | ||
assert.Equal(t, "", errStr) | ||
|
||
// Should have error | ||
assert.NotNil(t, err) | ||
|
||
// If it's specifically a proxy dial timeout, isTimeout should be true | ||
if errors.Is(err, ErrProxyDialTimeout) { | ||
assert.True(t, isTimeout, "isTimeout should be true for proxy dial timeout") | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This goroutine may leak if the
proxyClient.Dial
call blocks indefinitely. When theselect
statement in the parent goroutine times out viactx.Done()
, this goroutine is not terminated and will continue running untilproxyClient.Dial
returns. IfproxyClient.Dial
never returns, the goroutine and its associated resources will be leaked.While the underlying
ssh
library doesn't seem to provide a context-awareDial
method on the client, it's important to be aware of this potential resource leak.