Skip to content
This repository has been archived by the owner on Oct 24, 2023. It is now read-only.

chore: force external cloud provider for Kubernetes v1.21+ on Azure Stack Hub #4849

Merged
merged 6 commits into from
Mar 3, 2022
Merged

chore: force external cloud provider for Kubernetes v1.21+ on Azure Stack Hub #4849

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
01171b0
enforce external cloud provider for Azure Stack cloud 1.21+
haofan-ms Feb 25, 2022
fe5439d
fix lint
haofan-ms Feb 28, 2022
c1529f3
address comments
haofan-ms Feb 28, 2022
75c8340
update unittest
haofan-ms Feb 28, 2022
37448ab
update logging
haofan-ms Feb 28, 2022
c1c18ac
remove kubelet flag cloud-provider on windows worker node
haofan-ms Mar 2, 2022
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
6 changes: 6 additions & 0 deletions cmd/upgrade.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,12 @@ func (uc *upgradeCmd) loadCluster() error {
}
}

// Enforce UseCloudControllerManager for Kubernetes 1.21+ on Azure Stack cloud
if uc.containerService.Properties.IsAzureStackCloud() && common.IsKubernetesVersionGe(uc.upgradeVersion, "1.21.0") {
log.Infoln("The in-tree cloud provider is not longer supported on Azure Stack Hub for v1.21+ clusters, overwriting UseCloudControllerManager to 'true'")
uc.containerService.Properties.OrchestratorProfile.KubernetesConfig.UseCloudControllerManager = to.BoolPtr(true)
}

// The cluster-init component is a cluster create-only feature, temporarily disable if enabled
if i := api.GetComponentsIndexByName(uc.containerService.Properties.OrchestratorProfile.KubernetesConfig.Components, common.ClusterInitComponentName); i > -1 {
if uc.containerService.Properties.OrchestratorProfile.KubernetesConfig.Components[i].IsEnabled() {
Expand Down
10 changes: 10 additions & 0 deletions pkg/api/addons.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -926,6 +926,16 @@ func (cs *ContainerService) setAddonsConfig(isUpgrade bool) {
}
}

// Ensure cloud-node-manager is enabled on appropriate upgrades for Azure Stack cloud
if isUpgrade &&
Copy link
Member

Choose a reason for hiding this comment

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

do we need to check for to.Bool(cs.Properties.OrchestratorProfile.KubernetesConfig.UseCloudControllerManager) && common.IsKubernetesVersionGe(o.OrchestratorVersion, "1.21.0"?

The UseCCM flag is overridden already and supported version list starts on 1.21

Copy link
Member

Choose a reason for hiding this comment

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

The UseCCM flag is overridden already

I am assuming that upgrade.go is executed first

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, upgrade.go executed first, I will remove the check for k8s version

cs.Properties.IsAzureStackCloud() &&
to.Bool(cs.Properties.OrchestratorProfile.KubernetesConfig.UseCloudControllerManager) {
// Force enabling cloud-node-manager addon
if i := getAddonsIndexByName(o.KubernetesConfig.Addons, common.CloudNodeManagerAddonName); i > -1 {
o.KubernetesConfig.Addons[i] = defaultCloudNodeManagerAddonsConfig
}
}

// Back-compat for older addon specs of cluster-autoscaler
if isUpgrade {
i := getAddonsIndexByName(o.KubernetesConfig.Addons, common.ClusterAutoscalerAddonName)
Expand Down
10 changes: 10 additions & 0 deletions pkg/api/components.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,16 @@ func (cs *ContainerService) setComponentsConfig(isUpgrade bool) {
}
}

// Ensure cloud-controller-manager is enabled on appropriate upgrades for Azure Stack cloud
Copy link
Member

Choose a reason for hiding this comment

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

Same comments as addons.go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

if isUpgrade &&
cs.Properties.IsAzureStackCloud() &&
to.Bool(cs.Properties.OrchestratorProfile.KubernetesConfig.UseCloudControllerManager) {
// Force enabling cloud-controller-manager
if i := GetComponentsIndexByName(kubernetesConfig.Components, common.CloudControllerManagerComponentName); i > -1 {
kubernetesConfig.Components[i] = defaultCloudControllerManagerComponentConfig
}
}

for _, component := range defaultComponents {
synthesizeComponentsConfig(kubernetesConfig.Components, component, isUpgrade)
}
Expand Down
4 changes: 4 additions & 0 deletions pkg/api/defaults-kubelet.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,10 @@ func (cs *ContainerService) setKubeletConfig(isUpgrade bool) {
// if upgrade, force default "--pod-infra-container-image" value
cs.Properties.MasterProfile.KubernetesConfig.KubeletConfig["--pod-infra-container-image"] = o.KubernetesConfig.KubeletConfig["--pod-infra-container-image"]
}
//Ensure cloud-provider setting
Copy link
Member

Choose a reason for hiding this comment

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

We don't need this, this is already being taken care of in L152-L154

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to ensure the KubeletConfig in MasterProfile get updated when we upgrade from a in-tree cloud-provider cluster to a external cloud-provider. Without this line, the previous value for this field will not get overwritten.

Copy link
Member

Choose a reason for hiding this comment

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

O.K., thanks for clarifying

if to.Bool(o.KubernetesConfig.UseCloudControllerManager) {
cs.Properties.MasterProfile.KubernetesConfig.KubeletConfig["--cloud-provider"] = "external"
}
setMissingKubeletValues(cs.Properties.MasterProfile.KubernetesConfig, o.KubernetesConfig.KubeletConfig)
addDefaultFeatureGates(cs.Properties.MasterProfile.KubernetesConfig.KubeletConfig, o.OrchestratorVersion, "", "")

Expand Down
6 changes: 5 additions & 1 deletion pkg/api/vlabs/validate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,10 @@ func (a *Properties) ValidateOrchestratorProfile(isUpdate bool) error {
}

if a.IsAzureStackCloud() {
if common.IsKubernetesVersionGe(a.OrchestratorProfile.OrchestratorVersion, "1.21.0") && !to.Bool(o.KubernetesConfig.UseCloudControllerManager) {
Copy link
Member

Choose a reason for hiding this comment

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

This validation happens before defaults enforcement, so this is going to throw an error every time a user tries to create a cluster with an empty UseCloudControllerManager configuration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's what we want. We want users to add this UseCloudControllerManager: true in the apimodel for v1.21+ clusters on Azure Stack Hub, and be aware of the existence of external cloud-provider in these clusters.

return errors.New("useCloudControllerManager should be set to true for Kubernetes v1.21+ clusters on Azure Stack Hub")
}

if to.Bool(o.KubernetesConfig.UseInstanceMetadata) {
return errors.New("useInstanceMetadata shouldn't be set to true as feature not yet supported on Azure Stack")
}
Expand Down Expand Up @@ -809,7 +813,7 @@ func (a *Properties) validateAddons(isUpdate bool) error {
// Validation for addons if they are disabled
switch addon.Name {
case "cloud-node-manager":
if a.ShouldEnableAzureCloudAddon(addon.Name) {
if a.ShouldEnableAzureCloudAddon(addon.Name) && !a.IsAzureStackCloud() {
minVersion := "1.16.0"
if a.HasWindows() {
minVersion = "1.18.0"
Expand Down
30 changes: 27 additions & 3 deletions pkg/api/vlabs/validate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4719,6 +4719,27 @@ func TestValidateLocation(t *testing.T) {
},
expectedErr: errors.New("missing ContainerService Location"),
},
{
name: "AzureStack UseCloudControllerManager is false",
location: "local",
propertiesnil: false,
cs: &ContainerService{
Location: "local",
Properties: &Properties{
CustomCloudProfile: &CustomCloudProfile{
PortalURL: "https://portal.local.cotoso.com",
},
OrchestratorProfile: &OrchestratorProfile{
OrchestratorType: Kubernetes,
OrchestratorVersion: common.RationalizeReleaseAndVersion(Kubernetes, "", "", false, false, true),
KubernetesConfig: &KubernetesConfig{
UseCloudControllerManager: to.BoolPtr(falseVal),
},
},
},
},
expectedErr: errors.New("useCloudControllerManager should be set to true for Kubernetes v1.21+ clusters on Azure Stack Hub"),
},
{
name: "AzureStack UseInstanceMetadata is true",
location: "local",
Expand All @@ -4733,7 +4754,8 @@ func TestValidateLocation(t *testing.T) {
OrchestratorType: Kubernetes,
OrchestratorVersion: common.RationalizeReleaseAndVersion(Kubernetes, "", "", false, false, true),
KubernetesConfig: &KubernetesConfig{
UseInstanceMetadata: to.BoolPtr(trueVal),
UseCloudControllerManager: to.BoolPtr(trueVal),
UseInstanceMetadata: to.BoolPtr(trueVal),
},
},
},
Expand All @@ -4754,7 +4776,8 @@ func TestValidateLocation(t *testing.T) {
OrchestratorType: Kubernetes,
OrchestratorVersion: common.RationalizeReleaseAndVersion(Kubernetes, "", "", false, false, true),
KubernetesConfig: &KubernetesConfig{
EtcdDiskSizeGB: "1024",
UseCloudControllerManager: to.BoolPtr(trueVal),
EtcdDiskSizeGB: "1024",
},
},
},
Expand All @@ -4775,7 +4798,8 @@ func TestValidateLocation(t *testing.T) {
OrchestratorType: Kubernetes,
OrchestratorVersion: common.RationalizeReleaseAndVersion(Kubernetes, "", "", false, false, true),
KubernetesConfig: &KubernetesConfig{
EtcdDiskSizeGB: "1024GB",
UseCloudControllerManager: to.BoolPtr(trueVal),
EtcdDiskSizeGB: "1024GB",
},
},
},
Expand Down
1 change: 1 addition & 0 deletions pkg/engine/testdata/azurestack/kubernetes-ubuntu16.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
"kubernetesConfig": {
"kubernetesImageBase": "k8s.gcr.io/",
"useInstanceMetadata": false,
"useCloudControllerManager": true,
"networkPolicy": "none"
}
},
Expand Down
1 change: 1 addition & 0 deletions pkg/engine/testdata/azurestack/kubernetes.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
"kubernetesConfig": {
"kubernetesImageBase": "k8s.gcr.io/",
"useInstanceMetadata": false,
"useCloudControllerManager": true,
"networkPolicy": "none"
}
},
Expand Down