fix: networkplugin conversion only if networkpolicy is empty #1823

jackfrancis · 2019-08-20T21:11:21Z

Reason for Change:

This PR adds unit test coverage to generate + validate as vlabs (the new default flow with #1766), and fixes a faulty bit of logic in automatically setting the networkPlugin to the default value during "vlabs to unversioned" api model conversion.

This work reflects the fact that one of the examples api models we use to perform E2E tests was broken with the above commit, and we didn't know about it. In order to protect against that a full audit of the examples/ directory was performed, and all vlabs JSON files were given basic aks-engine generate unit test coverage. Where appropriate, api model specs were updated when these new unit tests revealed that in fact they were non-working. Also, where appropriate, api model files (e.g., no-longer aks-engine-supported Kubernetes versions) were deleted from the examples/ directory to improve the fidelity of stuff in there that actually works.

Additionally, *.json.env files which were used by an older, no longer used/maintained E2E script, were deleted.

Only api model examples under examples/ that are vlabs-versioned were included in additional unit tests.

Issue Fixed:

Fixes #1828

Requirements:

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

Notes:

codecov · 2019-08-20T21:45:44Z

Codecov Report

Merging #1823 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1823      +/-   ##
==========================================
- Coverage   76.42%   76.41%   -0.01%     
==========================================
  Files         130      130              
  Lines       19419    19413       -6     
==========================================
- Hits        14841    14835       -6     
  Misses       3780     3780              
  Partials      798      798

jackfrancis · 2019-08-20T21:58:07Z

pkg/api/convertertoapi.go

@@ -747,7 +747,7 @@ func setVlabsKubernetesDefaults(vp *vlabs.Properties, api *OrchestratorProfile)
 			api.KubernetesConfig.NetworkPolicy = vp.OrchestratorProfile.KubernetesConfig.NetworkPolicy
 		}
 	}
-	if api.KubernetesConfig.NetworkPlugin == "" {
+	if api.KubernetesConfig.NetworkPlugin == "" && api.KubernetesConfig.NetworkPolicy == "" {


This is the fix to the prior logic during vlabs-->unversioned conversion. Because not every version of NetworkPolicy will work with the default NetworkPlugin values below, we don't want to brute force set the default unless NetworkPolicy is also empty.

so what happens now if NetworkPlugin is empty but not NetworkPolicy? Do we set the NetworkPlugin somewhere else?

examples/ipvs/kubernetes-msi.json

jackfrancis · 2019-08-21T16:37:27Z

examples/extensions/kubernetes.json

@@ -39,8 +39,8 @@
      }
    ],
    "servicePrincipalProfile": {
-      "servicePrincipalClientID": "",
-      "servicePrincipalClientSecret": ""
+      "clientId": "",


These properties were not valid :|

jackfrancis · 2019-08-21T16:37:35Z

examples/extensions/kubernetes.oms.json

@@ -40,8 +40,8 @@
      }
    ],
    "servicePrincipalProfile": {
-      "servicePrincipalClientID": "",
-      "servicePrincipalClientSecret": ""
+      "clientId": "",


jackfrancis · 2019-08-21T16:37:40Z

examples/extensions/kubernetes.preprovision.json

@@ -43,8 +43,8 @@
      }
    ],
    "servicePrincipalProfile": {
-      "servicePrincipalClientID": "",
-      "servicePrincipalClientSecret": ""
+      "clientId": "",


jackfrancis · 2019-08-21T16:38:18Z

examples/vnet/README.md

- 4. **swarmmodevnet.json** - deploying and using [Swarm Mode](../../docs/swarmmode.md)
+1. First, deploy a custom vnet.  An example of an arm template that does this is under directory vnetarmtemplate.
+2. Next configure the example templates, and deploy according to the examples:
+ 1. **kubernetesvnet.json** - deploying and using [Kubernetes](../../docs/topics/features.md#feat-custom-vnet)


Cleaned up broken references and did a super quick version of making what remains more or less correct.

jackfrancis · 2019-08-21T16:38:50Z

examples/e2e-tests/kubernetes/coreos/coreos.json

@@ -50,19 +50,11 @@
          },
          {
            "name": "keyvault-flexvolume",
-            "enabled": false,


These properties were not valid :|

jackfrancis · 2019-08-21T16:38:57Z

examples/e2e-tests/kubernetes/coreos/coreos.json

          },
          {
            "name": "blobfuse-flexvolume",
-            "enabled": false,


jackfrancis · 2019-08-21T16:57:12Z

pkg/api/defaults.go

@@ -99,7 +99,7 @@ func (cs *ContainerService) setOrchestratorDefaults(isUpgrade, isScale bool) {
 		// we translate deprecated NetworkPolicy usage to the NetworkConfig equivalent
 		// and set a default network policy enforcement configuration
 		switch o.KubernetesConfig.NetworkPolicy {
-		case NetworkPluginAzure:
+		case NetworkPolicyAzure:


@CecileRobertMichon I made this sort of superficial change because (1) it's more expressive to use the NetworkPolicy const here and (2) hopefully it answers your question below.

Sorry, your question above ;)

CecileRobertMichon

lgtm

mboersma

/lgtm

acs-bot · 2019-08-21T18:06:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon, 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:

~~OWNERS~~ [CecileRobertMichon,jackfrancis,mboersma]

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

fix: networkplugin conversion only if networkpolicy is empty

9f84892

acs-bot added the size/L label Aug 20, 2019

jackfrancis changed the title ~~fix: networkplugin conversion only if networkpolicy is empty~~ [WIP] fix: networkplugin conversion only if networkpolicy is empty Aug 20, 2019

acs-bot added do-not-merge/work-in-progress approved labels Aug 20, 2019

jackfrancis commented Aug 20, 2019

View reviewed changes

test: lots of examples and unit tests!

e2b4442

jackfrancis changed the title ~~[WIP] fix: networkplugin conversion only if networkpolicy is empty~~ fix: networkplugin conversion only if networkpolicy is empty Aug 21, 2019

acs-bot added size/XXL and removed do-not-merge/work-in-progress size/L labels Aug 21, 2019

jackfrancis commented Aug 21, 2019

View reviewed changes

examples/ipvs/kubernetes-msi.json Show resolved Hide resolved

jackfrancis commented Aug 21, 2019

View reviewed changes

examples/e2e-tests/kubernetes/coreos/coreos.json

},

{

"name": "blobfuse-flexvolume",

"enabled": false,

Copy link

Member Author

jackfrancis Aug 21, 2019

Choose a reason for hiding this comment

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

ditto

chore: use a more semantically correct const

d7f4021

jackfrancis commented Aug 21, 2019

View reviewed changes

CecileRobertMichon approved these changes Aug 21, 2019

View reviewed changes

mboersma approved these changes Aug 21, 2019

View reviewed changes

acs-bot assigned mboersma Aug 21, 2019

acs-bot added the lgtm label Aug 21, 2019

jackfrancis merged commit d879a29 into Azure:master Aug 21, 2019

jackfrancis deleted the vlabs-networkplugin-defaults branch August 21, 2019 19:02

jackfrancis mentioned this pull request Aug 24, 2019

fix: restore working calico networkPolicy vlabs conversion logic #1855

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: networkplugin conversion only if networkpolicy is empty #1823

fix: networkplugin conversion only if networkpolicy is empty #1823

jackfrancis commented Aug 20, 2019 •

edited

Loading

codecov bot commented Aug 20, 2019 •

edited

Loading

jackfrancis Aug 20, 2019

CecileRobertMichon Aug 21, 2019

jackfrancis Aug 21, 2019

jackfrancis Aug 21, 2019

jackfrancis Aug 21, 2019

jackfrancis Aug 21, 2019

jackfrancis Aug 21, 2019

jackfrancis Aug 21, 2019

jackfrancis Aug 21, 2019

jackfrancis Aug 21, 2019

CecileRobertMichon left a comment

mboersma left a comment

acs-bot commented Aug 21, 2019

fix: networkplugin conversion only if networkpolicy is empty #1823

fix: networkplugin conversion only if networkpolicy is empty #1823

Conversation

jackfrancis commented Aug 20, 2019 • edited Loading

codecov bot commented Aug 20, 2019 • edited Loading

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

CecileRobertMichon left a comment

Choose a reason for hiding this comment

mboersma left a comment

Choose a reason for hiding this comment

acs-bot commented Aug 21, 2019

jackfrancis commented Aug 20, 2019 •

edited

Loading

codecov bot commented Aug 20, 2019 •

edited

Loading