From d727406d10bcdca55717642131f5b3b08f883e8e Mon Sep 17 00:00:00 2001 From: msvik Date: Mon, 18 Apr 2022 15:36:24 -0700 Subject: [PATCH 1/6] CNI Linux: Add 10 seconds timeout for ExecuteCommand --- platform/os_linux.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/platform/os_linux.go b/platform/os_linux.go index 0403ea422b..e665669cb9 100644 --- a/platform/os_linux.go +++ b/platform/os_linux.go @@ -5,6 +5,7 @@ package platform import ( "bytes" + "context" "fmt" "os" "os/exec" @@ -81,7 +82,12 @@ func (p *execClient) ExecuteCommand(command string) (string, error) { var stderr bytes.Buffer var out bytes.Buffer - cmd := exec.Command("sh", "-c", command) + + // Create a new context and add a timeout to it + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() // The cancel should be deferred so resources are cleaned up + + cmd := exec.CommandContext(ctx, "sh", "-c", command) cmd.Stderr = &stderr cmd.Stdout = &out @@ -170,4 +176,4 @@ func PrintDependencyPackageDetails() { func ReplaceFile(source, destination string) error { return os.Rename(source, destination) -} +} \ No newline at end of file From 7ebed931ce01654492a21a17f6d24e3413fd30df Mon Sep 17 00:00:00 2001 From: VK Date: Tue, 19 Apr 2022 09:59:50 -0700 Subject: [PATCH 2/6] ran gofmt on changed file --- platform/os_linux.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/platform/os_linux.go b/platform/os_linux.go index e665669cb9..fd44a7904e 100644 --- a/platform/os_linux.go +++ b/platform/os_linux.go @@ -176,4 +176,4 @@ func PrintDependencyPackageDetails() { func ReplaceFile(source, destination string) error { return os.Rename(source, destination) -} \ No newline at end of file +} From d9211b7411b29176f433c63dc150eec283ccaea4 Mon Sep 17 00:00:00 2001 From: VK Date: Wed, 20 Apr 2022 11:50:56 -0700 Subject: [PATCH 3/6] Fix lint error --- platform/os_linux.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/platform/os_linux.go b/platform/os_linux.go index fd44a7904e..b556946ed0 100644 --- a/platform/os_linux.go +++ b/platform/os_linux.go @@ -84,7 +84,8 @@ func (p *execClient) ExecuteCommand(command string) (string, error) { var out bytes.Buffer // Create a new context and add a timeout to it - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + const timeoutSeconds = 10 + ctx, cancel := context.WithTimeout(context.Background(), timeoutSeconds*time.Second) defer cancel() // The cancel should be deferred so resources are cleaned up cmd := exec.CommandContext(ctx, "sh", "-c", command) From e83cb82b31d4b38e4e716678e55cce10179bdf7d Mon Sep 17 00:00:00 2001 From: VK Date: Wed, 20 Apr 2022 13:07:57 -0700 Subject: [PATCH 4/6] Added unit test for ExecuteCommand for linux --- platform/os_linux_test.go | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 platform/os_linux_test.go diff --git a/platform/os_linux_test.go b/platform/os_linux_test.go new file mode 100644 index 0000000000..a2269ebb53 --- /dev/null +++ b/platform/os_linux_test.go @@ -0,0 +1,24 @@ +package platform + +import ( + "testing" +) + +// Default timeout in ExecuteCommand is 10 seconds so test should fail with 12 seconds execution time +func TestExecuteCommandTimeout(t *testing.T) { + client := NewExecClient() + _, err := client.ExecuteCommand("sleep 12") + if err == nil { + t.Errorf("TestExecuteCommandTimeout should have returned timeout error") + } + t.Logf("%s", err.Error()) +} + +// Default timeout in ExecuteCommand is 10 seconds so test should pass with 8 seconds execution time +func TestExecuteCommandNoTimeout(t *testing.T) { + client := NewExecClient() + _, err := client.ExecuteCommand("sleep 8") + if err != nil { + t.Errorf("TestExecuteCommandNoTimeout failed with error %v", err) + } +} From 2ab8bf86b807a96a347ad86d0a211dbc456b21a4 Mon Sep 17 00:00:00 2001 From: VK Date: Wed, 20 Apr 2022 14:21:56 -0700 Subject: [PATCH 5/6] Moved timeout to execlient. It has default timeout and method to override it now --- platform/osInterface.go | 22 ++++++++++++++++++++-- platform/os_linux.go | 13 ++++++------- platform/os_linux_test.go | 9 +++++++-- 3 files changed, 33 insertions(+), 11 deletions(-) diff --git a/platform/osInterface.go b/platform/osInterface.go index c0f47ed145..116e0ee5cd 100644 --- a/platform/osInterface.go +++ b/platform/osInterface.go @@ -1,6 +1,16 @@ package platform -type execClient struct{} +import ( + "time" +) + +const ( + defaultExecTimeout = 10 +) + +type execClient struct { + Timeout time.Duration +} //nolint:revive // ExecClient make sense type ExecClient interface { @@ -8,5 +18,13 @@ type ExecClient interface { } func NewExecClient() ExecClient { - return &execClient{} + return &execClient{ + Timeout: defaultExecTimeout * time.Second, + } +} + +func NewExecClientTimeout(timeout time.Duration) ExecClient { + return &execClient{ + Timeout: timeout, + } } diff --git a/platform/os_linux.go b/platform/os_linux.go index b556946ed0..047c14dc12 100644 --- a/platform/os_linux.go +++ b/platform/os_linux.go @@ -51,7 +51,7 @@ func GetOSInfo() string { } func GetProcessSupport() error { - p := &execClient{} + p := NewExecClient() cmd := fmt.Sprintf("ps -p %v -o comm=", os.Getpid()) _, err := p.ExecuteCommand(cmd) return err @@ -84,8 +84,7 @@ func (p *execClient) ExecuteCommand(command string) (string, error) { var out bytes.Buffer // Create a new context and add a timeout to it - const timeoutSeconds = 10 - ctx, cancel := context.WithTimeout(context.Background(), timeoutSeconds*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), p.Timeout) defer cancel() // The cancel should be deferred so resources are cleaned up cmd := exec.CommandContext(ctx, "sh", "-c", command) @@ -101,7 +100,7 @@ func (p *execClient) ExecuteCommand(command string) (string, error) { } func SetOutboundSNAT(subnet string) error { - p := execClient{} + p := NewExecClient() cmd := fmt.Sprintf("iptables -t nat -A POSTROUTING -m iprange ! --dst-range 168.63.129.16 -m addrtype ! --dst-type local ! -d %v -j MASQUERADE", subnet) _, err := p.ExecuteCommand(cmd) @@ -119,7 +118,7 @@ func ClearNetworkConfiguration() (bool, error) { } func KillProcessByName(processName string) error { - p := &execClient{} + p := NewExecClient() cmd := fmt.Sprintf("pkill -f %v", processName) _, err := p.ExecuteCommand(cmd) return err @@ -150,7 +149,7 @@ func GetOSDetails() (map[string]string, error) { } func GetProcessNameByID(pidstr string) (string, error) { - p := &execClient{} + p := NewExecClient() pidstr = strings.Trim(pidstr, "\n") cmd := fmt.Sprintf("ps -p %s -o comm=", pidstr) out, err := p.ExecuteCommand(cmd) @@ -166,7 +165,7 @@ func GetProcessNameByID(pidstr string) (string, error) { } func PrintDependencyPackageDetails() { - p := &execClient{} + p := NewExecClient() out, err := p.ExecuteCommand("iptables --version") out = strings.TrimSuffix(out, "\n") log.Printf("[cni-net] iptable version:%s, err:%v", out, err) diff --git a/platform/os_linux_test.go b/platform/os_linux_test.go index a2269ebb53..c0f41fd656 100644 --- a/platform/os_linux_test.go +++ b/platform/os_linux_test.go @@ -2,11 +2,14 @@ package platform import ( "testing" + "time" ) // Default timeout in ExecuteCommand is 10 seconds so test should fail with 12 seconds execution time func TestExecuteCommandTimeout(t *testing.T) { - client := NewExecClient() + const timeout = 10 * time.Second + client := NewExecClientTimeout(timeout) + _, err := client.ExecuteCommand("sleep 12") if err == nil { t.Errorf("TestExecuteCommandTimeout should have returned timeout error") @@ -16,7 +19,9 @@ func TestExecuteCommandTimeout(t *testing.T) { // Default timeout in ExecuteCommand is 10 seconds so test should pass with 8 seconds execution time func TestExecuteCommandNoTimeout(t *testing.T) { - client := NewExecClient() + const timeout = 10 * time.Second + client := NewExecClientTimeout(timeout) + _, err := client.ExecuteCommand("sleep 8") if err != nil { t.Errorf("TestExecuteCommandNoTimeout failed with error %v", err) From 29bc306bcc35a52ebaa0754b2e15359aa485cfac Mon Sep 17 00:00:00 2001 From: VK Date: Wed, 20 Apr 2022 15:20:35 -0700 Subject: [PATCH 6/6] reduce timeout values in unit tests --- platform/os_linux_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/platform/os_linux_test.go b/platform/os_linux_test.go index c0f41fd656..1848f22d03 100644 --- a/platform/os_linux_test.go +++ b/platform/os_linux_test.go @@ -5,24 +5,24 @@ import ( "time" ) -// Default timeout in ExecuteCommand is 10 seconds so test should fail with 12 seconds execution time +// Command execution time is more than timeout, so ExecuteCommand should return error func TestExecuteCommandTimeout(t *testing.T) { - const timeout = 10 * time.Second + const timeout = 2 * time.Second client := NewExecClientTimeout(timeout) - _, err := client.ExecuteCommand("sleep 12") + _, err := client.ExecuteCommand("sleep 3") if err == nil { t.Errorf("TestExecuteCommandTimeout should have returned timeout error") } t.Logf("%s", err.Error()) } -// Default timeout in ExecuteCommand is 10 seconds so test should pass with 8 seconds execution time +// Command execution time is less than timeout, so ExecuteCommand should work without error func TestExecuteCommandNoTimeout(t *testing.T) { - const timeout = 10 * time.Second + const timeout = 2 * time.Second client := NewExecClientTimeout(timeout) - _, err := client.ExecuteCommand("sleep 8") + _, err := client.ExecuteCommand("sleep 1") if err != nil { t.Errorf("TestExecuteCommandNoTimeout failed with error %v", err) }