From 002bb1d09f56a2eaa230605dc205070d8d607050 Mon Sep 17 00:00:00 2001 From: Abel Hu Date: Thu, 24 Oct 2024 07:07:46 +0000 Subject: [PATCH 1/2] fix: do not use return in updating kubelet node labels --- staging/cse/windows/kubeletfunc.ps1 | 20 ++++----- staging/cse/windows/kubeletfunc.tests.ps1 | 50 ++++++++++++----------- 2 files changed, 34 insertions(+), 36 deletions(-) diff --git a/staging/cse/windows/kubeletfunc.ps1 b/staging/cse/windows/kubeletfunc.ps1 index 2758a44897..1097a72804 100644 --- 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 index 1d4023f1b8..fc93f43e38 100644 --- 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 } } \ No newline at end of file From 2ab3a7a5f5211c015dc44225db62954c7ff17b68 Mon Sep 17 00:00:00 2001 From: Abel Hu Date: Thu, 24 Oct 2024 07:25:41 +0000 Subject: [PATCH 2/2] test: add a ut --- parts/windows/windowscsehelper.tests.ps1 | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/parts/windows/windowscsehelper.tests.ps1 b/parts/windows/windowscsehelper.tests.ps1 index e6e2d55a06..b3fbe9b7db 100644 --- 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" + } + } +}