chore: deprecate support for creating new 1.16 clusters #4256

jackfrancis · 2021-02-11T00:09:41Z

Reason for Change:

This PR removes support for creating new v1.16.n clusters. In addition, lots of UT TLC to make the next version deprecation easier!

Issue Fixed:

Credit Where Due:

Does this change contain code from or inspired by another project?

No
Yes

If "Yes," did you notify that project's maintainers and provide attribution?

No
Yes

Requirements:

uses conventional commit messages
includes documentation
adds unit tests
tested upgrade from previous version

Notes:

codecov · 2021-02-11T00:25:41Z

Codecov Report

Merging #4256 (fc0eaf6) into master (91d1e11) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4256      +/-   ##
==========================================
- Coverage   72.08%   72.07%   -0.02%     
==========================================
  Files         141      141              
  Lines       21715    21676      -39     
==========================================
- Hits        15653    15622      -31     
+ Misses       5107     5105       -2     
+ Partials      955      949       -6

Impacted Files	Coverage Δ
pkg/api/common/versions.go	`96.37% <ø> (ø)`
pkg/api/vlabs/validate.go	`81.35% <ø> (+0.44%)`	⬆️
pkg/api/defaults-apiserver.go	`100.00% <100.00%> (ø)`
pkg/api/defaults-cloud-controller-manager.go	`92.85% <100.00%> (-0.25%)`	⬇️
pkg/api/mocks.go	`100.00% <100.00%> (ø)`
cmd/rotate_certs.go	`11.03% <0.00%> (-0.11%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 91d1e11...fc0eaf6. Read the comment docs.

jackfrancis · 2021-02-11T17:21:22Z

pkg/api/defaults-apiserver.go

@@ -97,11 +97,7 @@ func (cs *ContainerService) setAPIServerConfig() {

 	// RBAC configuration
 	if to.Bool(o.KubernetesConfig.EnableRbac) {
-		if common.IsKubernetesVersionGe(o.OrchestratorVersion, "1.7.0") {


Getting rid of these unnecessary version checks

jackfrancis · 2021-02-11T17:21:37Z

pkg/api/defaults-apiserver.go

@@ -113,12 +109,6 @@ func (cs *ContainerService) setAPIServerConfig() {
 	admissionControlKey, admissionControlValues := getDefaultAdmissionControls(cs)
 	defaultAPIServerConfig[admissionControlKey] = admissionControlValues

-	// Enable VolumeSnapshotDataSource feature gate for Azure Disk CSI Driver
-	// which is disabled from 1.13 to 1.16 by default
-	if !common.IsKubernetesVersionGe(o.OrchestratorVersion, "1.17.0") {


ditto (we're always >= 1.17.0 starting with this PR)

jackfrancis · 2021-02-11T17:21:50Z

pkg/api/defaults-apiserver_test.go

@@ -261,16 +261,6 @@ func TestAPIServerConfigEnableRbac(t *testing.T) {
 			a["--authorization-mode"])
 	}

-	// Test EnableRbac = true with 1.6 cluster


these tests are no longer relevant

jackfrancis · 2021-02-11T17:22:14Z

pkg/api/defaults-apiserver_test.go

@@ -495,27 +475,17 @@ func TestAPIServerCosmosEtcd(t *testing.T) {
 }

 func TestAPIServerFeatureGates(t *testing.T) {
-	// Test 1.13 <= k8s <= 1.16


these tests are no longer relevant

jackfrancis · 2021-02-11T17:22:43Z

pkg/api/defaults-cloud-controller-manager.go

@@ -32,11 +30,8 @@ func (cs *ContainerService) setCloudControllerManagerConfig() {
 		"--v":                           "2",
 	}

-	// Add new arguments for Azure cloud-controller-manager component.


no longer relevant

jackfrancis · 2021-02-11T17:23:02Z

pkg/api/defaults-cloud-controller-manager_test.go

 	cs := CreateMockContainerService("testcluster", k8sVersion, 3, 2, false)
 	cs.setCloudControllerManagerConfig()
 	cm := cs.Properties.OrchestratorProfile.KubernetesConfig.CloudControllerManagerConfig
 	if cm["--controllers"] != "*,-cloud-node" {
 		t.Fatalf("got unexpected '--controllers' Cloud Controller Manager config value for Kubernetes %s: %s",
 			k8sVersion, cm["--controllers"])
 	}
-
-	k8sVersion = "1.15.4"


no longer a relevant test

jackfrancis · 2021-02-11T17:23:27Z

pkg/api/defaults_test.go

@@ -3161,15 +3161,8 @@ func TestEnableAggregatedAPIs(t *testing.T) {
 }

 func TestCloudControllerManagerEnabled(t *testing.T) {
-	// test that 1.16 defaults to false


this test is no longer relevant

jackfrancis · 2021-02-11T17:25:06Z

pkg/api/mocks.go

@@ -53,7 +53,11 @@ func CreateMockContainerService(containerServiceName, orchestratorVersion string

 	cs.Properties.OrchestratorProfile = &OrchestratorProfile{}
 	cs.Properties.OrchestratorProfile.OrchestratorType = Kubernetes
-	cs.Properties.OrchestratorProfile.OrchestratorVersion = orchestratorVersion
+	if orchestratorVersion == "" {


This addition of giving significance to "" means that we can invoke this mock func without passing an explicit version, and interpret that as "just make me a mock cluster config with the current default version". This way as the set of supported versions evolves over time we don't have to keep manually updating a bunch of UT.

jackfrancis · 2021-02-11T17:25:40Z

pkg/api/vlabs/validate.go

@@ -229,10 +229,6 @@ func (a *Properties) ValidateOrchestratorProfile(isUpdate bool) error {
 			if err != nil {
 				return err
 			}
-			minVersion, err := semver.Make("1.7.0")


we don't need to validate against >= 1.7.0 anymore

jackfrancis · 2021-02-11T17:25:56Z

pkg/api/vlabs/validate.go

@@ -254,30 +246,11 @@ func (a *Properties) ValidateOrchestratorProfile(isUpdate bool) error {
 			}

 			if to.Bool(o.KubernetesConfig.EnableEncryptionWithExternalKms) {
-				minVersion, err := semver.Make("1.10.0")


we don't need to validate against >= 1.10.0 anymore

jackfrancis · 2021-02-11T17:26:10Z

pkg/api/vlabs/validate.go

 				if to.Bool(a.OrchestratorProfile.KubernetesConfig.UseManagedIdentity) && a.OrchestratorProfile.KubernetesConfig.UserAssignedID == "" {
 					log.Warnf("Clusters with enableEncryptionWithExternalKms=true and system-assigned identity are not upgradable! You will not be able to upgrade your cluster using `aks-engine upgrade`")
 				}
 			}

-			if o.KubernetesConfig.EnableRbac != nil && !o.KubernetesConfig.IsRBACEnabled() {


we don't need to validate against >= 1.15.0 anymore

jackfrancis · 2021-02-11T17:26:19Z

pkg/api/vlabs/validate.go

@@ -295,14 +268,6 @@ func (a *Properties) ValidateOrchestratorProfile(isUpdate bool) error {
 			}

 			if o.KubernetesConfig.LoadBalancerSku == StandardLoadBalancerSku {
-				minVersion, err := semver.Make("1.11.0")


we don't need to validate against >= 1.11.0 anymore

jackfrancis · 2021-02-11T17:26:35Z

pkg/api/vlabs/validate.go

@@ -727,14 +687,6 @@ func (a *Properties) validateAddons(isUpdate bool) error {
 							}
 						}
 					}
-				case "nvidia-device-plugin":


we don't need to validate against >= 1.10.0 anymore

jackfrancis · 2021-02-11T17:26:44Z

pkg/api/vlabs/validate.go

@@ -801,14 +753,6 @@ func (a *Properties) validateAddons(isUpdate bool) error {
 					} else {
 						return errors.Errorf("%s addon is deprecated for new clusters", common.FlannelAddonName)
 					}
-				case "azure-policy":


we don't need to validate against >= 1.14.0 anymore

jackfrancis · 2021-02-11T17:27:02Z

pkg/api/vlabs/validate_test.go

@@ -92,35 +92,11 @@ func Test_OrchestratorProfile_Validate(t *testing.T) {
 			},
 			expectedError: "enableAggregatedAPIs requires the enableRbac feature as a prerequisite",
 		},
-		"should error when KubernetesConfig has enableRBAC disabled for >= 1.15": {


got rid of a bunch of obsolete version-specific UT

jackfrancis · 2021-02-11T17:28:20Z

cmd/upgrade_test.go

 	setupValidVersions(map[string]bool{
-		"1.10.13": true,


strictly speaking this change wasn't necessary (it didn't traverse the versions allow list flow), but (IMO) it's good practice not to pin to a static version that eventually doesn't make sense (in this case 1.10.13 hasn't made sense for a long time)

jackfrancis · 2021-02-11T17:29:05Z

examples/dualstack/kubernetes-iptables.json

@@ -6,7 +6,6 @@
        },
        "orchestratorProfile": {
            "orchestratorType": "Kubernetes",
-            "orchestratorRelease": "1.20",


@aramase is it important to include this 1.20 config here (for example, because this dualstack example doesn't work on versions older than 1.20) ?

The dual-stack configurations should work for 1.17+. But we want to set to 1.20 because that's the recommended version for dual-stack.

added back, thanks for clarifying!

jackfrancis · 2021-02-11T17:29:39Z

examples/kubernetes-config/kubernetes-containerd-tmpdir.json

@@ -2,7 +2,6 @@
  "apiVersion": "vlabs",
  "properties": {
    "orchestratorProfile": {
-      "orchestratorRelease": "1.17",


got rid of unnecessary version declarations in these configs so we don't have to be annoyed by them in the future

jackfrancis · 2021-02-11T17:30:05Z

examples/kubernetes-zones/README.md

@@ -15,9 +15,6 @@ Here is an [example of a Kubernetes cluster with Availability Zones support](../
 {
    "apiVersion": "vlabs",
    "properties": {
-      "orchestratorProfile": {


if I observed documentation referencing an older version I got rid of that

jackfrancis · 2021-02-11T17:30:45Z

examples/v20160930/kubernetes.json

@@ -1,38 +0,0 @@
-{


these are no longer valid example api models at all, so cleaning them up while I'm here

jackfrancis · 2021-02-11T17:32:23Z

pkg/api/common/versions_test.go

@@ -668,53 +671,63 @@ func Test_IsSupportedKubernetesVersion(t *testing.T) {
 }

 func Test_IsValidMinVersion(t *testing.T) {
-	orchestratorRelease := "1.16"
+	defaultVersion := RationalizeReleaseAndVersion(Kubernetes, "", "", false, false, false)


these new foo allow us to UT the IsValidMinVersion helper func in a resilient way doesn't depend upon a particular version being supported or unsupported

jadarsie · 2021-02-11T17:40:32Z

pkg/api/common/versions.go

@@ -263,8 +263,8 @@ var AllKubernetesSupportedVersionsAzureStack = map[string]bool{
 	"1.16.10": false,
 	"1.16.11": false,
 	"1.16.13": false,
-	"1.16.14": true,
-	"1.16.15": true,
+	"1.16.14": false,


map AllKubernetesWindowsSupportedVersionsAzureStack (a few lines below) also needs to be updated.

jadarsie · 2021-02-11T18:02:27Z

/lgtm

acs-bot · 2021-02-11T18:02:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jackfrancis, jadarsie

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [jackfrancis,jadarsie]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

chore: deprecate support for creating new 1.16 clusters

f973613

jackfrancis commented Feb 11, 2021

View reviewed changes

remove 1.16 from VHD spec

7556081

jadarsie reviewed Feb 11, 2021

View reviewed changes

review feedback

fc0eaf6

jackfrancis merged commit 6a8a5af into Azure:master Feb 11, 2021

jackfrancis deleted the deprecate-1.16 branch February 11, 2021 18:30

gofish-bot mentioned this pull request Mar 15, 2021

aks-engine 0.61.0 fishworks/fish-food#1279

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: deprecate support for creating new 1.16 clusters #4256

chore: deprecate support for creating new 1.16 clusters #4256

jackfrancis commented Feb 11, 2021

codecov bot commented Feb 11, 2021 •

edited

jackfrancis Feb 11, 2021

jackfrancis Feb 11, 2021

jackfrancis Feb 11, 2021

jackfrancis Feb 11, 2021

jackfrancis Feb 11, 2021

jackfrancis Feb 11, 2021

jackfrancis Feb 11, 2021

jackfrancis Feb 11, 2021

jackfrancis Feb 11, 2021

jackfrancis Feb 11, 2021

jackfrancis Feb 11, 2021

jackfrancis Feb 11, 2021

jackfrancis Feb 11, 2021

jackfrancis Feb 11, 2021

jackfrancis Feb 11, 2021

jackfrancis Feb 11, 2021

jackfrancis Feb 11, 2021

aramase Feb 11, 2021

jackfrancis Feb 11, 2021

jackfrancis Feb 11, 2021

jackfrancis Feb 11, 2021

jackfrancis Feb 11, 2021

jackfrancis Feb 11, 2021

jadarsie Feb 11, 2021

jadarsie commented Feb 11, 2021

acs-bot commented Feb 11, 2021

chore: deprecate support for creating new 1.16 clusters #4256

chore: deprecate support for creating new 1.16 clusters #4256

Conversation

jackfrancis commented Feb 11, 2021

codecov bot commented Feb 11, 2021 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jadarsie commented Feb 11, 2021

acs-bot commented Feb 11, 2021

codecov bot commented Feb 11, 2021 •

edited