From c3f1a6ef9ebe24c8d698eb3fca019a8a58ead31a Mon Sep 17 00:00:00 2001 From: Evan Baker Date: Wed, 13 Nov 2024 15:22:03 -0600 Subject: [PATCH] fix: remove PowerShell from Windows registry interactions (#2993) 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 | 106 ++++++++++++++++++++++-------------- platform/os_windows_test.go | 36 ------------ test/validate/validate.go | 2 +- 5 files changed, 67 insertions(+), 82 deletions(-) diff --git a/cns/service/main.go b/cns/service/main.go index 5438115eb..83d301249 100644 --- a/cns/service/main.go +++ b/cns/service/main.go @@ -788,8 +788,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 9e659b06a..129a7f196 100644 --- a/platform/os_linux.go +++ b/platform/os_linux.go @@ -179,7 +179,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 63900c6e5..c76b8a704 100644 --- a/platform/os_windows.go +++ b/platform/os_windows.go @@ -20,6 +20,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 ( @@ -61,24 +64,10 @@ 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 fetch netadapter and pnp id + // TODO: can we replace this (and things in endpoint_windows) with other utils from "golang.org/x/sys/windows"? GetMacAddressVFPPnpIDMapping = "Get-NetAdapter | Select-Object MacAddress, PnpDeviceID| Format-Table -HideTableHeaders" - // Command to restart HNS service - RestartHnsServiceCommand = "Restart-Service -Name hns" - // Interval between successive checks for mellanox adapter's PriorityVLANTag value defaultMellanoxMonitorInterval = 30 * time.Second @@ -257,40 +246,73 @@ func (p *execClient) ExecutePowershellCommandWithContext(ctx context.Context, co } // 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 5cb5dacc1..71541868d 100644 --- a/platform/os_windows_test.go +++ b/platform/os_windows_test.go @@ -4,7 +4,6 @@ import ( "context" "errors" "os/exec" - "strings" "testing" "time" @@ -115,41 +114,6 @@ func TestExecuteCommandError(t *testing.T) { require.ErrorIs(t, err, exec.ErrNotFound) } -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 -} - func TestFetchPnpIDMapping(t *testing.T) { mockExecClient := NewMockExecClient(false) // happy path diff --git a/test/validate/validate.go b/test/validate/validate.go index f6020f325..66b8822af 100644 --- a/test/validate/validate.go +++ b/test/validate/validate.go @@ -124,7 +124,7 @@ func (v *Validator) ValidateStateFile(ctx context.Context) error { } func (v *Validator) validateIPs(ctx context.Context, stateFileIps stateFileIpsFunc, cmd []string, checkType, namespace, labelSelector, containerName string) error { - log.Printf("Validating %s state file", checkType) + log.Printf("Validating %s state file for %s on %s", checkType, v.cni, v.os) nodes, err := acnk8s.GetNodeListByLabelSelector(ctx, v.clientset, nodeSelectorMap[v.os]) if err != nil { return errors.Wrapf(err, "failed to get node list")