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

chore: support Windows CSE and Customdata #396

Merged
merged 20 commits into from
Dec 11, 2020
Merged

chore: support Windows CSE and Customdata #396

merged 20 commits into from
Dec 11, 2020

Conversation

mainred
Copy link
Member

@mainred mainred commented Oct 19, 2020 •
edited
Loading

Changes included in this PR:

  • Windows custom scripts
    • copied from the latest aks-release-v0.47.0-1 of aks-engine to parts/windows directory
    • changed WrapAsVariable resolved by ARM template to the GetParameter by go-template in agentbaker, this change is on only parts/windows/kuberneteswindowsfunctions.ps1 and parts/windows/csecmd.ps1
    • give up the use of parts/windows/csecmd.ps1 for now
    • for the other windows custom scripts, they are mainly a copy-and-paste work, as they expose as function and the dynamic input variables is defined in parts/windows/kuberneteswindowsfunctions.ps1
  • pkg
    • support fetching Windows CSE command
    • add windows agentpool required functions in functionmap for later go-template parsing.
  • test
    • added UT tests in aks-engine master but lost in aks-engine aks branch
    • Added tests to generate testdata for Windows scenarios, these test data are generated and the compared with the result from the ones in the real cluster. some of the variables like tenenatid is not real, for sharing across the tests and comparision.
      • AKSWindows2019+K8S116
      • AKSWindows2019+K8S117
      • AKSWindows2019+K8S118
      • AKSWindows2019+K8S119
      • AKSWindows2019+K8S119+CSI
      • AKSWindows2019+ManagedIdentity
      • AKSWindows2004+K8S119+hyperv
      • AKSWindows2019+CustomCloud
      • AKSWindows2019+CustomVnet
      • AKSWindows2019+EnablePrivateClusterHostsConfigAgent

@mainred mainred force-pushed the qinhao/sync-windows-cse branch from 0db575b to 391d3e6 Compare October 26, 2020 14:45
@mainred mainred changed the title WIP: support Windows CSE chore: support Windows CSE Oct 26, 2020
@mainred mainred requested review from yizhang4321 and AbelHu October 26, 2020 15:04
mainred
mainred commented Oct 26, 2020
View reviewed changes
parts/windows/csecmd.ps1
echo %DATE%,%TIME%,%COMPUTERNAME% && powershell.exe -ExecutionPolicy Unrestricted -command \"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is referenced from
https://github.com/Azure/aks-engine/blob/aks-release-v0.47.0-1/pkg/engine/virtualmachinescalesets.go#L794.
The changed code is more readable

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This script is not used, but kept for further reference.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume you will file a second PR to use this template?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you plan to use agentbaker cse in ARM template and then convert ARM template to vmss-client for windows?
or do you plan to use agentbaker cse directly in vmss-client?
My imagination is we go with the latter one and if so, this file should be used

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We plan to first use agentbaker cse in ARM template and then convert ARM template to vmss-client for windows?
Just to be careful about the change. Also, I hit some issue when passing certificate in cse command, needing more time to fix.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file will definitely be used in our further PR.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got you. Thanks!

parts/windows/csecmd.ps1 Show resolved Hide resolved
AbelHu
AbelHu reviewed Nov 2, 2020
View reviewed changes
pkg/agent/variables.go Outdated Show resolved Hide resolved
AbelHu
AbelHu reviewed Nov 2, 2020
View reviewed changes
pkg/agent/params.go Outdated Show resolved Hide resolved
AbelHu
AbelHu reviewed Nov 2, 2020
View reviewed changes
pkg/agent/params.go Outdated Show resolved Hide resolved
@mainred mainred force-pushed the qinhao/sync-windows-cse branch from 391d3e6 to 3876a04 Compare November 25, 2020 13:14
mainred
mainred commented Dec 3, 2020
View reviewed changes
parts/windows/csecmd.ps1
echo %DATE%,%TIME%,%COMPUTERNAME% && powershell.exe -ExecutionPolicy Unrestricted -command \"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This script is not used, but kept for further reference.

parts/windows/csecmd.ps1 Show resolved Hide resolved
parts/windows/windowsazurecnifunc.ps1 Show resolved Hide resolved
pkg/agent/baker.go
if isUserAssignedIdentity {
userAssignedIdentityClientIDParams = "' -UserAssignedClientID ',reference(variables('userAssignedIDReference'), variables('apiVersionManagedIdentity')).clientId,"
}
commandExecStr := fmt.Sprintf("[concat('echo %s && powershell.exe -ExecutionPolicy Unrestricted -command \"', '$arguments = ', variables('singleQuote'),'-MasterIP ',variables('kubernetesAPIServerIP'),' -KubeDnsServiceIp ',parameters('kubeDnsServiceIp'),%s' -MasterFQDNPrefix ',variables('masterFqdnPrefix'),' -Location ',variables('location'),' -TargetEnvironment ',parameters('targetEnvironment'),' -AgentKey ',parameters('clientPrivateKey'),' -AADClientId ',variables('servicePrincipalClientId'),' -AADClientSecret ',variables('singleQuote'),variables('singleQuote'),base64(variables('servicePrincipalClientSecret')),variables('singleQuote'),variables('singleQuote'),' -NetworkAPIVersion ',variables('apiVersionNetwork'),' ',variables('singleQuote'), ' ; ', variables('windowsCustomScriptSuffix'), '\" > %s 2>&1 ; exit $LASTEXITCODE')]", "%DATE%,%TIME%,%COMPUTERNAME%", userAssignedIdentityClientIDParams, "%SYSTEMDRIVE%\\AzureData\\CustomDataSetupScript.log")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is from https://github.com/Azure/aks-engine/blob/master/pkg/engine/virtualmachinescalesets.go#L725

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this won't work outside of ARM template. The concat is ARM template func.

Also can you put the command template in a file?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We plan to first enable windows CSE in agentbaker first and remove arm variables and ARM template later.

pkg/agent/const.go Show resolved Hide resolved
pkg/agent/params.go Show resolved Hide resolved
parts/windows/csecmd.ps1 Show resolved Hide resolved
mainred
mainred commented Dec 3, 2020
View reviewed changes
parts/windows/kuberneteswindowssetup.ps1 Outdated
@@ -82,7 +102,7 @@ $global:SubscriptionId = "{{GetVariable "subscriptionId"}}"
$global:ResourceGroup = "{{GetVariable "resourceGroup"}}"
$global:VmType = "{{GetVariable "vmType"}}"
$global:SubnetName = "{{GetVariable "subnetName"}}"
$global:MasterSubnet = "{{GetWindowsMasterSubnetARMParam}}"
$global:MasterSubnet = "{{GetParameter "masterSubnet"}}"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

several lines after here, the assignment of global:KubeletNodeLabels is different from aks-engine as follows because we have labelResourceGroup related change implemented in

AgentBaker/pkg/agent/baker.go

Line 209 in 20d879d

func normalizeResourceGroupNameForLabel(resourceGroupName string) string { 

{{if IsKubernetesVersionGe "1.16.0"}}
$global:KubeletNodeLabels = "{{GetAgentKubernetesLabels . "',variables('labelResourceGroup'),'"}}"
{{else}}
$global:KubeletNodeLabels = "{{GetAgentKubernetesLabelsDeprecated . "',variables('labelResourceGroup'),'"}}"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"{{GetParameter "masterSubnet"}}" brings no harm as it provides subnet of VNetCIDR and it's being used by our current Windows nodepools, but as the first step to enable agentbaker windows, we here keep this for minimal change, and will delete it in the coming PR.

@mainred
Copy link
Member Author

mainred commented Dec 3, 2020

ping @AbelHu for another look.

mainred
mainred commented Dec 3, 2020
View reviewed changes
pkg/agent/testdata/AKSWindows2004+K8S119+hyperv/CSECommand
@@ -0,0 +1 @@
[concat('echo %DATE%,%TIME%,%COMPUTERNAME% && powershell.exe -ExecutionPolicy Unrestricted -command "', '$arguments = ', variables('singleQuote'),'-MasterIP ',variables('kubernetesAPIServerIP'),' -KubeDnsServiceIp ',parameters('kubeDnsServiceIp'),' -MasterFQDNPrefix ',variables('masterFqdnPrefix'),' -Location ',variables('location'),' -TargetEnvironment ',parameters('targetEnvironment'),' -AgentKey ',parameters('clientPrivateKey'),' -AADClientId ',variables('servicePrincipalClientId'),' -AADClientSecret ',variables('singleQuote'),variables('singleQuote'),base64(variables('servicePrincipalClientSecret')),variables('singleQuote'),variables('singleQuote'),' -NetworkAPIVersion ',variables('apiVersionNetwork'),' ',variables('singleQuote'), ' ; ', variables('windowsCustomScriptSuffix'), '" > %SYSTEMDRIVE%\AzureData\CustomDataSetupScript.log 2>&1 ; exit $LASTEXITCODE')]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty raw CSECommand, which is passed to ARM before being parsed.

AbelHu
AbelHu reviewed Dec 3, 2020
View reviewed changes
pkg/agent/params.go Outdated Show resolved Hide resolved
AbelHu
AbelHu reviewed Dec 3, 2020
View reviewed changes
pkg/agent/datamodel/types.go Outdated Show resolved Hide resolved
@mainred mainred force-pushed the qinhao/sync-windows-cse branch from 998d03b to 70eedf6 Compare December 4, 2020 00:52
@mainred
Copy link
Member Author

mainred commented Dec 7, 2020

ping @AbelHu and @xuto2 for another look, thanks.

xuto2
xuto2 reviewed Dec 7, 2020
View reviewed changes
pkg/agent/baker.go Outdated Show resolved Hide resolved
xuto2
xuto2 reviewed Dec 7, 2020
View reviewed changes
pkg/agent/datamodel/types.go Outdated Show resolved Hide resolved
xuto2
xuto2 reviewed Dec 10, 2020
View reviewed changes
pkg/agent/baker_test.go Outdated
Subnet: "10.240.0.0/16",
},
AgentPoolProfiles: []*datamodel.AgentPoolProfile{
{
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just to be clear we're not supposed to work with multiple agent pool. Please split them into different test cases.

xuto2
xuto2 reviewed Dec 10, 2020
View reviewed changes
pkg/agent/variables.go Outdated
Comment on lines 33 to 50
"customData": paramsMap{
"tenantID": config.TenantID,
"subscriptionId": config.SubscriptionID,
"resourceGroup": config.ResourceGroupName,
"location": cs.Location,
"vmType": cs.Properties.GetVMType(),
"subnetName": cs.Properties.GetSubnetName(),
"nsgName": cs.Properties.GetNSGName(),
"virtualNetworkName": cs.Properties.GetVirtualNetworkName(),
"routeTableName": cs.Properties.GetRouteTableName(),
"primaryAvailabilitySetName": cs.Properties.GetPrimaryAvailabilitySetName(),
"primaryScaleSetName": cs.Properties.GetPrimaryScaleSetName(),
"useManagedIdentityExtension": useManagedIdentity(cs),
"useInstanceMetadata": useInstanceMetadata(cs),
"loadBalancerSku": cs.Properties.OrchestratorProfile.KubernetesConfig.LoadBalancerSku,
"excludeMasterFromStandardLB": true,
"enableTelemetry": cs.Properties.FeatureFlags.IsFeatureEnabled("EnableTelemetry"),
},
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, please get these from NodeBootstrappingConfiguration instead of keep relying on cs.properties.
Also for linux, these are not needed for custom data generation, it's only for cse command. I'd be interested in why we need them for windows scenario

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we are still on the way to follow Linux's work. We just first copied the case from aks-engnie

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment for the usage of windows

xuto2
xuto2 reviewed Dec 10, 2020
View reviewed changes
pkg/agent/variables.go Outdated
@@ -28,9 +29,28 @@ func getCustomDataVariables(config *datamodel.NodeBootstrappingConfiguration) pa
"reconcilePrivateHostsService": getBase64EncodedGzippedCustomScript(reconcilePrivateHostsService, config),
"configureAzure0Script": getBase64EncodedGzippedCustomScript(kubernetesConfigAzure0Script, config),
},
// customData defined here is mainly used for Windows
"customData": paramsMap{
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes more sense to have a separate getWindowsCustomDataVariables - there is no much shared things in this method between windows and linux.

xuto2
xuto2 reviewed Dec 10, 2020
View reviewed changes
pkg/agent/variables.go Outdated
}

func getCSECommandVariables(config *datamodel.NodeBootstrappingConfiguration) paramsMap {
cs := config.ContainerService
profile := config.AgentPoolProfile
return map[string]interface{}{
cseCommandVariables := map[string]interface{}{
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you don't need the change since you reverted the change

@mainred mainred force-pushed the qinhao/sync-windows-cse branch from 2d690c1 to 8a9d1c5 Compare December 10, 2020 11:53
xuto2
xuto2 approved these changes Dec 11, 2020
View reviewed changes
Copy link

@xuto2 xuto2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

talked offline. please create todo task for mastersubnet change.

@mainred mainred merged commit 62f0d75 into master Dec 11, 2020
mainred pushed a commit that referenced this pull request Dec 11, 2020
chore: support Windows CSE and Customdata (#396)
0e2b601 
* support replacing windows CSE

* Add hyper-v and containerd support

* add customcloud back and fix comments

* Removed unused testdata

* Add TODO comment for current less code change

* More UTs and tetdata

* Add more test cases and copy CSECommand from aks-engine

* Update generated code

* fix comments

* Rebase onto the master

* Update CustomVnet testdata

* fix comments

* nil check kubernetesconfig

* Enable ip-masq-agent by default

* Remove used variables

* Split the function returning custom data

* Split test for Linux and Windows

* fix comments

* Remove mastersubnet and modify name of container runtimes

* Update testdata
xuto2 pushed a commit that referenced this pull request Dec 11, 2020
chore: support Windows CSE and Customdata (#396) (#479)
92e585e 
* support replacing windows CSE

* Add hyper-v and containerd support

* add customcloud back and fix comments

* Removed unused testdata

* Add TODO comment for current less code change

* More UTs and tetdata

* Add more test cases and copy CSECommand from aks-engine

* Update generated code

* fix comments

* Rebase onto the master

* Update CustomVnet testdata

* fix comments

* nil check kubernetesconfig

* Enable ip-masq-agent by default

* Remove used variables

* Split the function returning custom data

* Split test for Linux and Windows

* fix comments

* Remove mastersubnet and modify name of container runtimes

* Update testdata
@mainred mainred deleted the qinhao/sync-windows-cse branch January 29, 2021 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AbelHu AbelHu AbelHu left review comments

@xuto2 xuto2 xuto2 approved these changes

@yizhang4321 yizhang4321 Awaiting requested review from yizhang4321

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mainred @xuto2 @AbelHu