Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 2 commits into from
Oct 24, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
002bb1d
fix: do not use return in updating kubelet node labels
AbelHu Oct 24, 2024
2ab3a7a
test: add a ut
AbelHu Oct 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions parts/windows/windowscsehelper.tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
}
}
20 changes: 8 additions & 12 deletions staging/cse/windows/kubeletfunc.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down
50 changes: 26 additions & 24 deletions staging/cse/windows/kubeletfunc.tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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" {
Expand Down Expand Up @@ -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
}
}
Loading