fix: kube-dns and coredns addons UT, back-compat #2433
Conversation
| @@ -3255,6 +3255,10 @@ func TestSetAddonsConfig(t *testing.T) { | |||
| Name: common.CloudNodeManagerAddonName, | |||
| Enabled: to.BoolPtr(false), | |||
| }, | |||
| { | |||
| Name: common.KubeDNSAddonName, | |||
There was a problem hiding this comment.
This was an error that wasn't caught because kube-dns wasn't actually being validated in this UT.
| @@ -4252,7 +4256,7 @@ func TestSetAddonsConfig(t *testing.T) { | |||
| Containers: []KubernetesContainerSpec{ | |||
| { | |||
| Name: common.CoreDNSAddonName, | |||
| Image: specConfig.KubernetesImageBase + K8sComponentsByVersionMap["1.15.4"][common.CoreDNSAddonName], | |||
| Image: specConfig.KubernetesImageBase + K8sComponentsByVersionMap["1.16.0"][common.CoreDNSAddonName], | |||
There was a problem hiding this comment.
These fixes are actually functionally no-ops, as we ship the same coredns for many versions, but it's still something we want to address.
| NetworkPlugin: NetworkPluginAzure, | ||
| Addons: []KubernetesAddon{ |
There was a problem hiding this comment.
The proper back-compat/upgrade flow would not have the kube-dns or coredns addons present
| @@ -7249,25 +7246,7 @@ func TestSetAddonsConfig(t *testing.T) { | |||
| expectedAddons: []KubernetesAddon{ | |||
| { | |||
| Name: common.HeapsterAddonName, | |||
| Enabled: to.BoolPtr(true), | |||
There was a problem hiding this comment.
This change is due to increasing the version to 1.13 (which makes sense because we no longer support 1.12)
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| { | ||
| name: "kube-dns enabled", | ||
| name: "upgrade w/ manual kube-dns enabled", |
There was a problem hiding this comment.
This is the "user manually modifies his/her api model to explicitly continue using kube-dns, which is now an option since making kube-dns a user-configurable addon"
| @@ -7608,15 +7589,600 @@ func TestSetAddonsConfig(t *testing.T) { | |||
| }, | |||
| Containers: []KubernetesContainerSpec{ | |||
| { | |||
| Name: common.KubeDNSAddonName, | |||
| Name: "kubedns", | |||
There was a problem hiding this comment.
Again, more errors slipping through because we weren't actually validating kube-dns in this UT.
| @@ -7649,6 +8215,8 @@ func TestSetAddonsConfig(t *testing.T) { | |||
| common.AzureFileCSIDriverAddonName, | |||
| common.AzureDiskCSIDriverAddonName, | |||
| common.CloudNodeManagerAddonName, | |||
| common.CoreDNSAddonName, | |||
There was a problem hiding this comment.
This is us actually testing kube-dns and coredns addons
Codecov Report
@@ Coverage Diff @@
## master #2433 +/- ##
==========================================
+ Coverage 72.29% 72.29% +<.01%
==========================================
Files 130 130
Lines 23603 23606 +3
==========================================
+ Hits 17063 17066 +3
Misses 5516 5516
Partials 1024 1024 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jackfrancis, mboersma 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:
Approvers can indicate their approval by writing |
|
Related to #2251 |
Reason for Change:
Follow up from #2393 and #2416. UT was not fully engaged, and thus the back-compat was not implemented exactly right. This PR fixes UT, and implements back-compat according to the following recipe:
This back-compat implementation includes some permissiveness for intrepid users who manually edit the addons array in their pre-existing api model prior to running
aks-engine upgrade. Specifically, we only do the above default enforcement if thecorednsaddon is not present in the api model (which is the api model signature a legacy cluster will exhibit). What this means in practice is that users who want to continue to usekube-dnson a >= 1.12 cluster may manually modify the api model accordingly. UT have been added to prove this out.Also, ensure we account for scenarios where coredns and kube-dns could be (incorrectly) enabled together.
Issue Fixed:
Requirements:
Notes: