From 8a3c26635757e5b3767c0e1246aef9048adcfc9d Mon Sep 17 00:00:00 2001 From: zetaozhuang Date: Fri, 20 Oct 2023 01:17:54 -0700 Subject: [PATCH 1/4] fix: skip setting SdnRemoteArpMacAddress when hns is not enabled --- cns/service/main.go | 5 +++-- platform/mockexec.go | 19 +++++++++++++++---- platform/os_windows.go | 24 ++++++++++++++++++------ platform/os_windows_test.go | 36 ++++++++++++++++++++++++++++++++++++ 4 files changed, 72 insertions(+), 12 deletions(-) diff --git a/cns/service/main.go b/cns/service/main.go index 6f53df9a4a..d6d2ff3c17 100644 --- a/cns/service/main.go +++ b/cns/service/main.go @@ -724,8 +724,9 @@ func main() { } } - // Setting the remote ARP MAC address to 12-34-56-78-9a-bc on windows for external traffic - err = platform.SetSdnRemoteArpMacAddress() + // Setting the remote ARP MAC address to 12-34-56-78-9a-bc on windows for external traffic if HNS is enabled + execClient := platform.NewExecClient(nil) + err = platform.SetSdnRemoteArpMacAddress(execClient) if err != nil { logger.Errorf("Failed to set remote ARP MAC address: %v", err) return diff --git a/platform/mockexec.go b/platform/mockexec.go index d4f8558d5a..d8b4a60bae 100644 --- a/platform/mockexec.go +++ b/platform/mockexec.go @@ -6,11 +6,15 @@ import ( ) type MockExecClient struct { - returnError bool - setExecCommand execCommandValidator + returnError bool + setExecCommand execCommandValidator + powershellCommandResponder powershellCommandResponder } -type execCommandValidator func(string) (string, error) +type ( + execCommandValidator func(string) (string, error) + powershellCommandResponder func(string) (string, error) +) // ErrMockExec - mock exec error var ErrMockExec = errors.New("mock exec error") @@ -37,11 +41,18 @@ func (e *MockExecClient) SetExecCommand(fn execCommandValidator) { e.setExecCommand = fn } +func (e *MockExecClient) SetPowershellCommandResponder(fn powershellCommandResponder) { + e.powershellCommandResponder = fn +} + func (e *MockExecClient) ClearNetworkConfiguration() (bool, error) { return true, nil } -func (e *MockExecClient) ExecutePowershellCommand(_ string) (string, error) { +func (e *MockExecClient) ExecutePowershellCommand(cmd string) (string, error) { + if e.powershellCommandResponder != nil { + return e.powershellCommandResponder(cmd) + } return "", nil } diff --git a/platform/os_windows.go b/platform/os_windows.go index 6fe2d2ab19..42c8617f4b 100644 --- a/platform/os_windows.go +++ b/platform/os_windows.go @@ -68,6 +68,10 @@ const ( SetSdnRemoteArpMacAddressCommand = "Set-ItemProperty " + "-Path HKLM:\\SYSTEM\\CurrentControlSet\\Services\\hns\\State -Name SDNRemoteArpMacAddress -Value \"12-34-56-78-9a-bc\"" + // Command to check if system have hns path or not + CheckIfHNSPathExistsCommand = "Test-Path " + + "-Path HKLM:\\SYSTEM\\CurrentControlSet\\Services\\hns\\State" + // Command to restart HNS service RestartHnsServiceCommand = "Restart-Service -Name hns" @@ -190,24 +194,32 @@ func (p *execClient) ExecutePowershellCommand(command string) (string, error) { return strings.TrimSpace(stdout.String()), nil } -// SetSdnRemoteArpMacAddress sets the regkey for SDNRemoteArpMacAddress needed for multitenancy -func SetSdnRemoteArpMacAddress() error { - p := NewExecClient(nil) +// SetSdnRemoteArpMacAddress sets the regkey for SDNRemoteArpMacAddress needed for multitenancy if hns is enabled +func SetSdnRemoteArpMacAddress(execClient ExecClient) error { + existed, err := execClient.ExecutePowershellCommand(CheckIfHNSPathExistsCommand) + if err != nil { + log.Printf("Failed to check the existent of hns path due to error %s", err.Error()) + return err + } + if existed == "false" { + log.Printf("hns path is not existed, skip setting SdnRemoteArpMacAddress") + return nil + } if sdnRemoteArpMacAddressSet == false { - result, err := p.ExecutePowershellCommand(GetSdnRemoteArpMacAddressCommand) + result, err := execClient.ExecutePowershellCommand(GetSdnRemoteArpMacAddressCommand) if err != nil { return err } // Set the reg key if not already set or has incorrect value if result != SDNRemoteArpMacAddress { - if _, err = p.ExecutePowershellCommand(SetSdnRemoteArpMacAddressCommand); err != nil { + if _, err = execClient.ExecutePowershellCommand(SetSdnRemoteArpMacAddressCommand); err != nil { log.Printf("Failed to set SDNRemoteArpMacAddress due to error %s", err.Error()) return err } log.Printf("[Azure CNS] SDNRemoteArpMacAddress regKey set successfully. Restarting hns service.") - if _, err := p.ExecutePowershellCommand(RestartHnsServiceCommand); err != nil { + if _, err := execClient.ExecutePowershellCommand(RestartHnsServiceCommand); err != nil { log.Printf("Failed to Restart HNS Service due to error %s", err.Error()) return err } diff --git a/platform/os_windows_test.go b/platform/os_windows_test.go index a6e44d2fa5..587ef465a3 100644 --- a/platform/os_windows_test.go +++ b/platform/os_windows_test.go @@ -3,6 +3,7 @@ package platform import ( "errors" "os/exec" + "strings" "testing" "github.com/Azure/azure-container-networking/platform/windows/adapter/mocks" @@ -98,3 +99,38 @@ func TestExecuteCommandError(t *testing.T) { assert.ErrorAs(t, err, &xErr) assert.Equal(t, 1, xErr.ExitCode()) } + +func TestSetSdnRemoteArpMacAddress_hnsNotEnabled(t *testing.T) { + mockExecClient := NewMockExecClient(false) + // testing skip setting SdnRemoteArpMacAddress when hns not enabled + mockExecClient.SetPowershellCommandResponder(func(_ string) (string, error) { + return "false", nil + }) + err := SetSdnRemoteArpMacAddress(mockExecClient) + assert.NoError(t, err) + assert.Equal(t, false, sdnRemoteArpMacAddressSet) + + // testing the scenario when there is an error in checking if hns is enabled or not + mockExecClient.SetPowershellCommandResponder(func(_ string) (string, error) { + return "", errTestFailure + }) + err = SetSdnRemoteArpMacAddress(mockExecClient) + assert.ErrorAs(t, err, &errTestFailure) + assert.Equal(t, false, sdnRemoteArpMacAddressSet) +} + +func TestSetSdnRemoteArpMacAddress_hnsEnabled(t *testing.T) { + mockExecClient := NewMockExecClient(false) + // happy path + mockExecClient.SetPowershellCommandResponder(func(cmd string) (string, error) { + if strings.Contains(cmd, "Test-Path") { + return "true", nil + } + return "", nil + }) + err := SetSdnRemoteArpMacAddress(mockExecClient) + assert.NoError(t, err) + assert.Equal(t, true, sdnRemoteArpMacAddressSet) + // reset sdnRemoteArpMacAddressSet + sdnRemoteArpMacAddressSet = false +} From d85d77d8231e2c094883c09924d8ed1d33899a1d Mon Sep 17 00:00:00 2001 From: zetaozhuang Date: Fri, 20 Oct 2023 11:15:59 -0700 Subject: [PATCH 2/4] fix lint and minor changes --- platform/os_windows.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/platform/os_windows.go b/platform/os_windows.go index 42c8617f4b..736aa19974 100644 --- a/platform/os_windows.go +++ b/platform/os_windows.go @@ -196,13 +196,14 @@ func (p *execClient) ExecutePowershellCommand(command string) (string, error) { // SetSdnRemoteArpMacAddress sets the regkey for SDNRemoteArpMacAddress needed for multitenancy if hns is enabled func SetSdnRemoteArpMacAddress(execClient ExecClient) error { - existed, err := execClient.ExecutePowershellCommand(CheckIfHNSPathExistsCommand) + exists, err := execClient.ExecutePowershellCommand(CheckIfHNSPathExistsCommand) if err != nil { - log.Printf("Failed to check the existent of hns path due to error %s", err.Error()) - return err + errMsg := fmt.Sprintf("Failed to check the existent of hns path due to error %s", err.Error()) + log.Printf(errMsg) + return errors.Errorf(errMsg) } - if existed == "false" { - log.Printf("hns path is not existed, skip setting SdnRemoteArpMacAddress") + if strings.ToLower(exists) == "false" { + log.Printf("hns path does not exist, skip setting SdnRemoteArpMacAddress") return nil } if sdnRemoteArpMacAddressSet == false { From af91acf454c92ba93c439af7e3b55738bb55c705 Mon Sep 17 00:00:00 2001 From: Zetao Zhuang Date: Fri, 20 Oct 2023 15:06:38 -0700 Subject: [PATCH 3/4] fix lint --- platform/os_linux.go | 2 +- platform/os_windows.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/platform/os_linux.go b/platform/os_linux.go index 444b32071e..0b29bb2868 100644 --- a/platform/os_linux.go +++ b/platform/os_linux.go @@ -144,7 +144,7 @@ func (p *execClient) KillProcessByName(processName string) error { // SetSdnRemoteArpMacAddress sets the regkey for SDNRemoteArpMacAddress needed for multitenancy // This operation is specific to windows OS -func SetSdnRemoteArpMacAddress() error { +func SetSdnRemoteArpMacAddress(_ ExecClient) error { return nil } diff --git a/platform/os_windows.go b/platform/os_windows.go index 736aa19974..94536d5762 100644 --- a/platform/os_windows.go +++ b/platform/os_windows.go @@ -202,7 +202,7 @@ func SetSdnRemoteArpMacAddress(execClient ExecClient) error { log.Printf(errMsg) return errors.Errorf(errMsg) } - if strings.ToLower(exists) == "false" { + if strings.EqualFold(exists, "false") { log.Printf("hns path does not exist, skip setting SdnRemoteArpMacAddress") return nil } From 048d89cc526f156ea459f491954b192cabceb878 Mon Sep 17 00:00:00 2001 From: zetaozhuang Date: Wed, 25 Oct 2023 12:31:06 -0700 Subject: [PATCH 4/4] address comment --- platform/os_windows.go | 10 +++++----- platform/os_windows_test.go | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/platform/os_windows.go b/platform/os_windows.go index 94536d5762..e4de7b8dcd 100644 --- a/platform/os_windows.go +++ b/platform/os_windows.go @@ -68,8 +68,8 @@ const ( SetSdnRemoteArpMacAddressCommand = "Set-ItemProperty " + "-Path HKLM:\\SYSTEM\\CurrentControlSet\\Services\\hns\\State -Name SDNRemoteArpMacAddress -Value \"12-34-56-78-9a-bc\"" - // Command to check if system have hns path or not - CheckIfHNSPathExistsCommand = "Test-Path " + + // Command to check if system has hns state path or not + CheckIfHNSStatePathExistsCommand = "Test-Path " + "-Path HKLM:\\SYSTEM\\CurrentControlSet\\Services\\hns\\State" // Command to restart HNS service @@ -196,14 +196,14 @@ func (p *execClient) ExecutePowershellCommand(command string) (string, error) { // SetSdnRemoteArpMacAddress sets the regkey for SDNRemoteArpMacAddress needed for multitenancy if hns is enabled func SetSdnRemoteArpMacAddress(execClient ExecClient) error { - exists, err := execClient.ExecutePowershellCommand(CheckIfHNSPathExistsCommand) + exists, err := execClient.ExecutePowershellCommand(CheckIfHNSStatePathExistsCommand) if err != nil { - errMsg := fmt.Sprintf("Failed to check the existent of hns path due to error %s", err.Error()) + errMsg := fmt.Sprintf("Failed to check the existent of hns state path due to error %s", err.Error()) log.Printf(errMsg) return errors.Errorf(errMsg) } if strings.EqualFold(exists, "false") { - log.Printf("hns path does not exist, skip setting SdnRemoteArpMacAddress") + log.Printf("hns state path does not exist, skip setting SdnRemoteArpMacAddress") return nil } if sdnRemoteArpMacAddressSet == false { diff --git a/platform/os_windows_test.go b/platform/os_windows_test.go index 587ef465a3..424c30227f 100644 --- a/platform/os_windows_test.go +++ b/platform/os_windows_test.go @@ -104,7 +104,7 @@ func TestSetSdnRemoteArpMacAddress_hnsNotEnabled(t *testing.T) { mockExecClient := NewMockExecClient(false) // testing skip setting SdnRemoteArpMacAddress when hns not enabled mockExecClient.SetPowershellCommandResponder(func(_ string) (string, error) { - return "false", nil + return "False", nil }) err := SetSdnRemoteArpMacAddress(mockExecClient) assert.NoError(t, err) @@ -124,7 +124,7 @@ func TestSetSdnRemoteArpMacAddress_hnsEnabled(t *testing.T) { // happy path mockExecClient.SetPowershellCommandResponder(func(cmd string) (string, error) { if strings.Contains(cmd, "Test-Path") { - return "true", nil + return "True", nil } return "", nil })