fix: do not override vnetCidr param when empty string

[CecileRobertMichon]

Reason for Change:

azuredeploy.json

"vnetCidr": {
  "defaultValue": "10.0.0.0/8",
  "metadata": {
    "description": "Cluster vnet cidr"
  },
  "type": "string"
},

azuredeploy.parameters.json overrides this with the empty string which results in an empty vnetCidr.

Issue Fixed:

#1550

Requirements:

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

Notes:

[CecileRobertMichon]

@PatrickLang @Alwandy @msbecker

[CecileRobertMichon]

I have another attempt at rationalizing the vnetCidr defaults and using apimodel defaults instead of ARM param defaults but it's more complex so I'd rather start with this PR as an easy, straightforward fix.

[codecov]

Codecov Report

Merging #1570 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master    #1570   +/-   ##
=======================================
  Coverage   76.06%   76.06%           
=======================================
  Files         129      129           
  Lines       18572    18572           
=======================================
  Hits        14127    14127           
  Misses       3655     3655           
  Partials      790      790

[Alwandy]

@CecileRobertMichon first fix looks good to me,

2nd fix looks OK, assuming that it will merge with the 1st fix? Or any reason why you won't be setting:
if properties.OrchestratorProfile.IsKubernetes() && properties.MasterProfile.VnetCidr != "" on 2nd fix?

[PatrickLang]

/lgtm

[acs-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon, PatrickLang

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]

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

[PatrickLang]

I think this fix makes the most sense - we shouldn't set an empty parameter that overrides the variables default.

For the second approach - I don't see why there's an if (windows) type decision tree. We should be passing the same networking parameters down to both Windows & Linux nodes and I would rather aim to get those consistent.

[PatrickLang]

this is included in https://github.com/Azure/aks-engine/releases/tag/v0.38.0 :)