From cd5578e9d186c97cbda1b7278274a4619828377d Mon Sep 17 00:00:00 2001 From: Evan Baker Date: Wed, 27 Nov 2024 09:22:34 -0600 Subject: [PATCH] fix: remove PowerShell from Windows registry interactions (#2993) (#3165) remove powershell from windows registry txs Signed-off-by: Evan Baker --- cns/service/main.go | 3 +- platform/os_linux.go | 2 +- platform/os_windows.go | 105 +++++++++++++++++++++--------------- platform/os_windows_test.go | 36 ------------- 4 files changed, 65 insertions(+), 81 deletions(-) diff --git a/cns/service/main.go b/cns/service/main.go index d986964a12..3de9b51279 100644 --- a/cns/service/main.go +++ b/cns/service/main.go @@ -798,8 +798,7 @@ func main() { } // 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) + err = platform.SetSdnRemoteArpMacAddress(rootCtx) if err != nil { logger.Errorf("Failed to set remote ARP MAC address: %v", err) return diff --git a/platform/os_linux.go b/platform/os_linux.go index 0b29bb2868..7e8ec5e11b 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(_ ExecClient) error { +func SetSdnRemoteArpMacAddress(context.Context) error { return nil } diff --git a/platform/os_windows.go b/platform/os_windows.go index e4de7b8dcd..cf171343e6 100644 --- a/platform/os_windows.go +++ b/platform/os_windows.go @@ -19,6 +19,9 @@ import ( "github.com/pkg/errors" "go.uber.org/zap" "golang.org/x/sys/windows" + "golang.org/x/sys/windows/registry" + "golang.org/x/sys/windows/svc" + "golang.org/x/sys/windows/svc/mgr" ) const ( @@ -60,21 +63,6 @@ const ( // for vlan tagged arp requests SDNRemoteArpMacAddress = "12-34-56-78-9a-bc" - // Command to get SDNRemoteArpMacAddress registry key - GetSdnRemoteArpMacAddressCommand = "(Get-ItemProperty " + - "-Path HKLM:\\SYSTEM\\CurrentControlSet\\Services\\hns\\State -Name SDNRemoteArpMacAddress).SDNRemoteArpMacAddress" - - // Command to set SDNRemoteArpMacAddress registry key - SetSdnRemoteArpMacAddressCommand = "Set-ItemProperty " + - "-Path HKLM:\\SYSTEM\\CurrentControlSet\\Services\\hns\\State -Name SDNRemoteArpMacAddress -Value \"12-34-56-78-9a-bc\"" - - // 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 - RestartHnsServiceCommand = "Restart-Service -Name hns" - // Interval between successive checks for mellanox adapter's PriorityVLANTag value defaultMellanoxMonitorInterval = 30 * time.Second @@ -195,40 +183,73 @@ 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(CheckIfHNSStatePathExistsCommand) +func SetSdnRemoteArpMacAddress(ctx context.Context) error { + log.Printf("Setting SDNRemoteArpMacAddress regKey") + // open the registry key + k, err := registry.OpenKey(registry.LOCAL_MACHINE, `SYSTEM\CurrentControlSet\Services\hns\State`, registry.READ|registry.SET_VALUE) if err != nil { - 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 errors.Is(err, registry.ErrNotExist) { + return nil + } + return errors.Wrap(err, "could not open registry key") } - if strings.EqualFold(exists, "false") { - log.Printf("hns state path does not exist, skip setting SdnRemoteArpMacAddress") - return nil + defer k.Close() + // check the key value + if v, _, _ := k.GetStringValue("SDNRemoteArpMacAddress"); v == SDNRemoteArpMacAddress { + log.Printf("SDNRemoteArpMacAddress regKey already set") + return nil // already set } - if sdnRemoteArpMacAddressSet == false { - result, err := execClient.ExecutePowershellCommand(GetSdnRemoteArpMacAddressCommand) + if err = k.SetStringValue("SDNRemoteArpMacAddress", SDNRemoteArpMacAddress); err != nil { + return errors.Wrap(err, "could not set registry key") + } + log.Printf("SDNRemoteArpMacAddress regKey set successfully") + log.Printf("Restarting HNS service") + // connect to the service manager + m, err := mgr.Connect() + if err != nil { + return errors.Wrap(err, "could not connect to service manager") + } + defer m.Disconnect() //nolint:errcheck // ignore error + // open the HNS service + service, err := m.OpenService("hns") + if err != nil { + return errors.Wrap(err, "could not access service") + } + defer service.Close() + if err := restartService(ctx, service); err != nil { + return errors.Wrap(err, "could not restart service") + } + log.Printf("HNS service restarted successfully") + return nil +} + +func restartService(ctx context.Context, s *mgr.Service) error { + // Stop the service + _, err := s.Control(svc.Stop) + if err != nil { + return errors.Wrap(err, "could not stop service") + } + // Wait for the service to stop + ticker := time.NewTicker(500 * time.Millisecond) //nolint:gomnd // 500ms + defer ticker.Stop() + for { // hacky cancellable do-while + status, err := s.Query() if err != nil { - return err + return errors.Wrap(err, "could not query service status") } - - // Set the reg key if not already set or has incorrect value - if result != SDNRemoteArpMacAddress { - 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 := execClient.ExecutePowershellCommand(RestartHnsServiceCommand); err != nil { - log.Printf("Failed to Restart HNS Service due to error %s", err.Error()) - return err - } + if status.State == svc.Stopped { + break + } + select { + case <-ctx.Done(): + return errors.New("context cancelled") + case <-ticker.C: } - - sdnRemoteArpMacAddressSet = true } - + // Start the service again + if err := s.Start(); err != nil { + return errors.Wrap(err, "could not start service") + } return nil } diff --git a/platform/os_windows_test.go b/platform/os_windows_test.go index 424c30227f..a6e44d2fa5 100644 --- a/platform/os_windows_test.go +++ b/platform/os_windows_test.go @@ -3,7 +3,6 @@ package platform import ( "errors" "os/exec" - "strings" "testing" "github.com/Azure/azure-container-networking/platform/windows/adapter/mocks" @@ -99,38 +98,3 @@ 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 -}