fix: do not use return in updating kubelet node labels (#5151)

Azure · Oct 24, 2024 · 1c75357 · 1c75357
1 parent 41567a0
commit 1c75357
Show file tree

Hide file tree

Showing 3 changed files with 52 additions and 36 deletions.
diff --git a/parts/windows/windowscsehelper.tests.ps1 b/parts/windows/windowscsehelper.tests.ps1
@@ -147,3 +147,21 @@ Describe 'Validate Exit Codes' {
     }
   }
 }
+
+# When using return to return values in a function with using Write-Log, the logs will be returned as well.
+Describe "Mock Write-Log" {
+  It 'should never exist in ut' {
+    # Path to the PowerShell script you want to test
+    $scriptPaths = @()
+    $cseScripts = Get-ChildItem -Path "$PSScriptRoot\..\..\staging\cse\windows\" -Filter "*tests.ps1"
+    foreach($script in $cseScripts) {
+      $scriptPaths += $script.FullName
+    }
+
+    foreach($scriptPath in $scriptPaths) {
+      Write-Host "Validating $scriptPath"
+      $scriptContent = Get-Content -Path $scriptPath
+      $scriptContent -join "`n" | Should -Not -Match "Mock Write-Log"
+    }
+  }
+}
diff --git a/staging/cse/windows/kubeletfunc.ps1 b/staging/cse/windows/kubeletfunc.ps1
@@ -211,35 +211,31 @@ function Get-KubePackage {
 
 function Add-KubeletNodeLabel {
     Param(
-        [Parameter(Mandatory=$true)][string]
-        $KubeletNodeLabels,
         [Parameter(Mandatory=$true)][string]
         $Label
     )
 
-    $labelList = $KubeletNodeLabels -split ","
+    $labelList = $global:KubeletNodeLabels -split ","
     foreach ($existingLabel in $labelList) {
         if ($existingLabel -eq $Label) {
             Write-Log "found existing kubelet node label $existingLabel, will continue without adding anything"
-            return $KubeletNodeLabels
+            return
         }
     }
     Write-Log "adding label $Label to kubelet node labels..."
     $labelList += $Label
-    return $labelList -join ","
+    $global:KubeletNodeLabels = $labelList -join ","
 }
 
 function Remove-KubeletNodeLabel {
     Param(
-        [Parameter(Mandatory=$true)][string]
-        $KubeletNodeLabels,
         [Parameter(Mandatory=$true)][string]
         $Label
     )
 
-    $labelList = $KubeletNodeLabels -split ","
+    $labelList = $global:KubeletNodeLabels -split ","
     $filtered = $labelList | Where-Object { $_ -ne $Label }
-    return $filtered -join ","
+    $global:KubeletNodeLabels = $filtered -join ","
 }
 
 function Get-TagValue {
@@ -285,12 +281,12 @@ function Configure-KubeletServingCertificateRotation {
     if ($disabled -eq "true") {
         Write-Log "Kubelet serving certificate rotation is disabled by nodepool tags, will reconfigure kubelet flags and node labels"
         $global:KubeletConfigArgs = $global:KubeletConfigArgs -replace "--rotate-server-certificates=true", "--rotate-server-certificates=false"
-        $global:KubeletNodeLabels = Remove-KubeletNodeLabel -KubeletNodeLabels $global:KubeletNodeLabels -Label $nodeLabel
+        Remove-KubeletNodeLabel -Label $nodeLabel
         return
     }
 
     Write-Log "Kubelet serving certificate rotation is enabled, will add node label if needed"
-    $global:KubeletNodeLabels = Add-KubeletNodeLabel -KubeletNodeLabels $global:KubeletNodeLabels -Label $nodeLabel
+    Add-KubeletNodeLabel -Label $nodeLabel
 }
 
 # DEPRECATED - TODO(cameissner): remove once k8s setup script has been updated
@@ -316,7 +312,7 @@ function Disable-KubeletServingCertificateRotationForTags {
     Write-Log "Kubelet serving certificate rotation is disabled by nodepool tags, will reconfigure kubelet flags and node labels"
 
     $global:KubeletConfigArgs = $global:KubeletConfigArgs -replace "--rotate-server-certificates=true", "--rotate-server-certificates=false"
-    $global:KubeletNodeLabels = Remove-KubeletNodeLabel -KubeletNodeLabels $global:KubeletNodeLabels -Label "kubernetes.azure.com/kubelet-serving-ca=cluster"
+    Remove-KubeletNodeLabel -Label "kubernetes.azure.com/kubelet-serving-ca=cluster"
 }
 
 # TODO: replace KubeletStartFile with a Kubelet config, remove NSSM, and use built-in service integration

diff --git a/staging/cse/windows/kubeletfunc.tests.ps1 b/staging/cse/windows/kubeletfunc.tests.ps1
@@ -67,7 +67,6 @@ Describe 'Get-KubePackage' {
 Describe 'Configure-KubeletServingCertificateRotation' {
     BeforeEach {
         Mock Logs-To-Event
-        Mock Write-Log
     }
 
     It "Should no-op when EnableKubeletServingCertificateRotation is false" {
@@ -233,64 +232,67 @@ Describe 'Get-TagValue' {
 
 Describe 'Add-KubeletNodeLabel' {
     BeforeEach {
-        Mock Write-Log
+        $global:KubeletNodeLabels = ""
     }
 
     It "Should perform a no-op when the specified label already exists within the label string" {
-        $labelString = "kubernetes.azure.com/nodepool-type=VirtualMachineScaleSets,kubernetes.azure.com/kubelet-serving-ca=cluster,kubernetes.azure.com/agentpool=wp0"
+        $global:KubeletNodeLabels = "kubernetes.azure.com/nodepool-type=VirtualMachineScaleSets,kubernetes.azure.com/kubelet-serving-ca=cluster,kubernetes.azure.com/agentpool=wp0"
         $label = "kubernetes.azure.com/kubelet-serving-ca=cluster"
         $expected = "kubernetes.azure.com/nodepool-type=VirtualMachineScaleSets,kubernetes.azure.com/kubelet-serving-ca=cluster,kubernetes.azure.com/agentpool=wp0"
-        $result = Add-KubeletNodeLabel -KubeletNodeLabels $labelString -Label $label
-        Compare-Object $result $expected | Should -Be $null
+        Add-KubeletNodeLabel -Label $label
+        Compare-Object $global:KubeletNodeLabels $expected | Should -Be $null
     }
 
     It "Should append the label when it does not already exist within the label string" {
-        $labelString = "kubernetes.azure.com/nodepool-type=VirtualMachineScaleSets,kubernetes.azure.com/agentpool=wp0"
+        $global:KubeletNodeLabels = "kubernetes.azure.com/nodepool-type=VirtualMachineScaleSets,kubernetes.azure.com/agentpool=wp0"
         $label = "kubernetes.azure.com/kubelet-serving-ca=cluster"
         $expected = "kubernetes.azure.com/nodepool-type=VirtualMachineScaleSets,kubernetes.azure.com/agentpool=wp0,kubernetes.azure.com/kubelet-serving-ca=cluster"
-        $result = Add-KubeletNodeLabel -KubeletNodeLabels $labelString -Label $label
-        Compare-Object $result $expected | Should -Be $null
+        Add-KubeletNodeLabel -Label $label
+        Compare-Object $global:KubeletNodeLabels $expected | Should -Be $null
     }
 }
 
 Describe 'Remove-KubeletNodeLabel' {
+    BeforeEach {
+        $global:KubeletNodeLabels = ""
+    }
     It "Should remove the specified label when it exists within the label string" {
-        $labelString = "kubernetes.azure.com/nodepool-type=VirtualMachineScaleSets,kubernetes.azure.com/kubelet-serving-ca=cluster,kubernetes.azure.com/agentpool=wp0"
+        $global:KubeletNodeLabels = "kubernetes.azure.com/nodepool-type=VirtualMachineScaleSets,kubernetes.azure.com/kubelet-serving-ca=cluster,kubernetes.azure.com/agentpool=wp0"
         $label = "kubernetes.azure.com/kubelet-serving-ca=cluster"
         $expected = "kubernetes.azure.com/nodepool-type=VirtualMachineScaleSets,kubernetes.azure.com/agentpool=wp0"
-        $result = Remove-KubeletNodeLabel -KubeletNodeLabels $labelString -Label $label
-        Compare-Object $result $expected | Should -Be $null
+        Remove-KubeletNodeLabel -Label $label
+        Compare-Object $global:KubeletNodeLabels $expected | Should -Be $null
     }
 
     It "Should remove the specified label when it is the first label within the label string" {
-        $labelString = "kubernetes.azure.com/kubelet-serving-ca=cluster,kubernetes.azure.com/nodepool-type=VirtualMachineScaleSets,kubernetes.azure.com/agentpool=wp0"
+        $global:KubeletNodeLabels = "kubernetes.azure.com/kubelet-serving-ca=cluster,kubernetes.azure.com/nodepool-type=VirtualMachineScaleSets,kubernetes.azure.com/agentpool=wp0"
         $label = "kubernetes.azure.com/kubelet-serving-ca=cluster"
         $expected = "kubernetes.azure.com/nodepool-type=VirtualMachineScaleSets,kubernetes.azure.com/agentpool=wp0"
-        $result = Remove-KubeletNodeLabel -KubeletNodeLabels $labelString -Label $label
-        Compare-Object $result $expected | Should -Be $null
+        Remove-KubeletNodeLabel -Label $label
+        Compare-Object $global:KubeletNodeLabels $expected | Should -Be $null
     }
 
     It "Should remove the specified label when it is the last label within the label string" {
-        $labelString = "kubernetes.azure.com/nodepool-type=VirtualMachineScaleSets,kubernetes.azure.com/agentpool=wp0,kubernetes.azure.com/kubelet-serving-ca=cluster"
+        $global:KubeletNodeLabels = "kubernetes.azure.com/nodepool-type=VirtualMachineScaleSets,kubernetes.azure.com/agentpool=wp0,kubernetes.azure.com/kubelet-serving-ca=cluster"
         $label = "kubernetes.azure.com/kubelet-serving-ca=cluster"
         $expected = "kubernetes.azure.com/nodepool-type=VirtualMachineScaleSets,kubernetes.azure.com/agentpool=wp0"
-        $result = Remove-KubeletNodeLabel -KubeletNodeLabels $labelString -Label $label
-        Compare-Object $result $expected | Should -Be $null
+        Remove-KubeletNodeLabel -Label $label
+        Compare-Object $global:KubeletNodeLabels $expected | Should -Be $null
     }
 
     It "Should not alter the specified label string if the target label does not exist" {
-        $labelString = "kubernetes.azure.com/nodepool-type=VirtualMachineScaleSets,kubernetes.azure.com/agentpool=wp0"
+        $global:KubeletNodeLabels = "kubernetes.azure.com/nodepool-type=VirtualMachineScaleSets,kubernetes.azure.com/agentpool=wp0"
         $label = "kubernetes.azure.com/kubelet-serving-ca=cluster"
-        $expected = $labelString
-        $result = Remove-KubeletNodeLabel -KubeletNodeLabels $labelString -Label $label
-        Compare-Object $result $expected | Should -Be $null
+        $expected = $global:KubeletNodeLabels
+        Remove-KubeletNodeLabel -Label $label
+        Compare-Object $global:KubeletNodeLabels $expected | Should -Be $null
     }
 
     It "Should return an empty string if the only label within the label string is the target" {
-        $labelString = "kubernetes.azure.com/kubelet-serving-ca=cluster"
+        $global:KubeletNodeLabels = "kubernetes.azure.com/kubelet-serving-ca=cluster"
         $label = "kubernetes.azure.com/kubelet-serving-ca=cluster"
         $expected = ""
-        $result = Remove-KubeletNodeLabel -KubeletNodeLabels $labelString -Label $label
-        Compare-Object $result $expected | Should -Be $null
+        Remove-KubeletNodeLabel -Label $label
+        Compare-Object $global:KubeletNodeLabels $expected | Should -Be $null
     }
 }