Validation logic backward compatibility support for custom VNET #2693
Conversation
…ted from prior v20180331 models.
ghost
assigned 
| // ErrorInvalidNetworkPlugin error | ||
| var ErrorInvalidNetworkPlugin = fmt.Errorf("Network plugin should be either \"azure\" or \"kubenet\"") | ||
| var ErrorInvalidNetworkPlugin = fmt.Errorf("Network plugin should be either Azure or Kubenet") |
There was a problem hiding this comment.
Kubenet or none? Also, is it case insensitive?
There was a problem hiding this comment.
The plugin name is Kubenet, but in the model, we represent it internally as "none". Since the error message is more for customer, I put the name to Kubenet. Case does not matter here.
| } | ||
| // validate network profile settings | ||
| if n != nil { | ||
| if string(n.NetworkPlugin) == string(Azure) || string(n.NetworkPlugin) == string(Kubenet) { |
There was a problem hiding this comment.
is it easier to read using switch and case? Also, can't it be compared using NetworkPlugin type? how about case sensitivity?
There was a problem hiding this comment.
In this case, Azure and Kubenet is handled through the same code path, so switch does not help.
NetworkPlugin can be instantiated by taking a string, I think it can not be compared directly to constant Azure or Kubenet, even though the string value are the same.
The model now requires case matching too.
There was a problem hiding this comment.
In pkg/acsengine/testdata/agentPoolOnly/v20180331/agentsWithFullNetworkProfile.json, it says azure, though..
There was a problem hiding this comment.
Actually tested (NetworkPlugin("azure") == Azure) is true, will change the code to make it simple.
"azure" is the serialized representation of object Azure, NetworkPlugin("Azure") will not equal to object Azure. So it is case sensitive.
| } | ||
| } else { | ||
| return ErrorNilAgentPoolProfile |
There was a problem hiding this comment.
In Update case, it may not be required.
There was a problem hiding this comment.
Yes, it is probably redundant and has been checked before, but I think it is no harm to be on the safe side to check this.
There was a problem hiding this comment.
if you are sure that update without AgentPoolProfile will be fine, then it's ok... but.. are you sure about that? :)
There was a problem hiding this comment.
There was a problem hiding this comment.
Since we allow partial API model in the update case, remove ErrorNilAgentPoolProfile.
I still want to leave the validateAgentPoolVNET() method in VNET validation since VNET validation will cross multiple profiles (for example, we will need to validate subnetCidr from agentPoolProfile against serviceCidr from networkProfile).
| } | ||
| } | ||
|
|
||
| return true | ||
| return false | ||
| } | ||
|
|
||
| // GetVNETSubnetIDComponents extract subscription, resourcegroup, vnetname, subnetname from the vnetSubnetID |
There was a problem hiding this comment.
The regex is static. Can be saved at package level. Also, most literals used in Azure are case insensitive. For instance, resourcegroups is as good as resourceGroups
There was a problem hiding this comment.
Yes, we will look into this in a separate task.
| @@ -59,7 +59,7 @@ const ( | |||
| // Azure represents Azure CNI network plugin | |||
| Azure NetworkPlugin = "azure" | |||
| // Kubenet represents Kubenet network plugin | |||
| Kubenet NetworkPlugin = "kubenet" | |||
| Kubenet NetworkPlugin = "none" | |||
There was a problem hiding this comment.
please use kubenet and translate to internal none
| t.Error("error in orchestrator profile networkPlugin conversion") | ||
| } | ||
|
|
||
| if api.KubernetesConfig.ServiceCIDR != "" { | ||
| if api.KubernetesConfig.ServiceCIDR != "10.0.0.0/16" { |
There was a problem hiding this comment.
can you define consts for 10.0.0.0/16 and 172.17.0.1/16
* Validation logic backward compatibility support for API models converted from prior v20180331 models. * Fix check-style. * Simplify network plugin check. * Change network plugin "none" to "kubenet", remove agent pool nil check. * Set default network values in un-versioned model. * Add defaults for network settings.
* Validation logic backward compatibility support for API models converted from prior v20180331 models. * Fix check-style. * Simplify network plugin check. * Change network plugin "none" to "kubenet", remove agent pool nil check. * Set default network values in un-versioned model. * Add defaults for network settings. (cherry picked from commit 89de6ab)
Validation logic backward compatibility support for API models converted from prior v20180331 models.
This PR adds validation support for API models generated by serialization of
Also adds various testings.