Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

GetSupportedVersion returns the right win default #3079

Merged
merged 7 commits into from
May 29, 2018
Merged

GetSupportedVersion returns the right win default #3079

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d9a3b54
GetSupportedVersion returns the right win default
May 24, 2018
2a28c39
unit tests
May 24, 2018
916e621
fix unit test
May 24, 2018
7964f17
fixed validation with windows pools
May 25, 2018
6140f62
fix lint
May 25, 2018
3d8c866
Fix bug not validatinf first agent
May 26, 2018
1b0af6d
check for empty version
May 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/acsengine/defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,7 @@ func setOrchestratorDefaults(cs *api.ContainerService) {
o := a.OrchestratorProfile
o.OrchestratorVersion = common.GetValidPatchVersion(
o.OrchestratorType,
o.OrchestratorVersion)
o.OrchestratorVersion, a.HasWindows())

switch o.OrchestratorType {
case api.Kubernetes:
Expand Down
43 changes: 43 additions & 0 deletions pkg/acsengine/testdata/v20170131/kubernetes-win.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
{
"apiVersion": "2017-01-31",
"properties": {
"orchestratorProfile": {
"orchestratorType": "Kubernetes"
},
"masterProfile": {
"count": 1,
"dnsPrefix": "masterdns1"
},
"agentPoolProfiles": [
{
"name": "agentpool1",
"count": 3,
"vmSize": "Standard_D2_v2"
},
{
"name": "agentpool2",
"count": 3,
"vmSize": "Standard_D2_v2",
"osType": "Windows"
}
],
"windowsProfile": {
"adminUsername": "azureuser",
"adminPassword": "replacepassword1234$"
},
"linuxProfile": {
"adminUsername": "azureuser",
"ssh": {
"publicKeys": [
{
"keyData": "ssh-rsa PUBLICKEY azureuser@linuxvm"
}
]
}
},
"servicePrincipalProfile": {
"clientId": "ServicePrincipalClientID",
"secret": "myServicePrincipalClientSecret"
}
}
}
3 changes: 1 addition & 2 deletions pkg/acsengine/testdata/v20170131/kubernetes.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,7 @@
{
"name": "agentpool2",
"count": 3,
"vmSize": "Standard_D2_v2",
"osType": "Windows"
"vmSize": "Standard_D2_v2"
}
],
"windowsProfile": {
Expand Down
5 changes: 2 additions & 3 deletions pkg/acsengine/testdata/v20170701/kubernetes-default-version.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,7 @@
"name": "agentpool2",
"count": 1,
"vmSize": "Standard_D3_v2",
"availabilityProfile": "AvailabilitySet",
"osType": "Windows"
"availabilityProfile": "AvailabilitySet"
}
],
"windowsProfile": {
Expand All @@ -45,4 +44,4 @@
"secret": "myServicePrincipalClientSecret"
}
}
}
}
48 changes: 48 additions & 0 deletions pkg/acsengine/testdata/v20170701/kubernetes-win-default-version.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
{
"apiVersion": "2017-07-01",
"properties": {
"orchestratorProfile": {
"orchestratorType": "Kubernetes"
},
"masterProfile": {
"count": 1,
"dnsPrefix": "masterdns1",
"OSDiskSizeGB": 200,
"vmSize": "Standard_D2_v2"
},
"agentPoolProfiles": [
{
"name": "agentpool1",
"count": 1,
"vmSize": "Standard_D2_v2",
"OSDiskSizeGB": 200,
"availabilityProfile": "AvailabilitySet"
},
{
"name": "agentpool2",
"count": 1,
"vmSize": "Standard_D3_v2",
"availabilityProfile": "AvailabilitySet",
"osType": "Windows"
}
],
"windowsProfile": {
"adminUsername": "azureuser",
"adminPassword": "replacepassword1234$"
},
"linuxProfile": {
"adminUsername": "azureuser",
"ssh": {
"publicKeys": [
{
"keyData": "ssh-rsa PUBLICKEY azureuser@linuxvm"
}
]
}
},
"servicePrincipalProfile": {
"clientId": "ServicePrincipalClientID",
"secret": "myServicePrincipalClientSecret"
}
}
}
2 changes: 1 addition & 1 deletion pkg/acsengine/testdata/v20170701/kubernetes.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"properties": {
"orchestratorProfile": {
"orchestratorType": "Kubernetes",
"orchestratorVersion": "1.6.11"
"orchestratorVersion": "1.8.12"
},
"masterProfile": {
"count": 1,
Expand Down
30 changes: 23 additions & 7 deletions pkg/api/apiloader_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,47 +25,63 @@ func TestLoadContainerServiceFromFile(t *testing.T) {
if err != nil {
t.Error(err.Error())
}
if containerService.Properties.OrchestratorProfile.OrchestratorVersion != "1.6.11" {
t.Error("Failed to set orcherstator version when it is set in the json")
if containerService.Properties.OrchestratorProfile.OrchestratorVersion != "1.8.12" {
t.Errorf("Failed to set orcherstator version when it is set in the json, expected 1.8.12 but got %s", containerService.Properties.OrchestratorProfile.OrchestratorVersion)
}

containerService, _, err = apiloader.LoadContainerServiceFromFile("../acsengine/testdata/v20170701/kubernetes-default-version.json", true, false, existingContainerService)
if err != nil {
t.Error(err.Error())
}
if containerService.Properties.OrchestratorProfile.OrchestratorVersion != "1.6.9" {
t.Errorf("Failed set orcherstator version when it is not set in the json, got %s", containerService.Properties.OrchestratorProfile.OrchestratorVersion)
t.Errorf("Failed to set orcherstator version when it is not set in the json, got %s", containerService.Properties.OrchestratorProfile.OrchestratorVersion)
}

containerService, _, err = apiloader.LoadContainerServiceFromFile("../acsengine/testdata/v20170131/kubernetes.json", true, false, existingContainerService)
if err != nil {
t.Error(err.Error())
}
if containerService.Properties.OrchestratorProfile.OrchestratorVersion != "1.6.9" {
t.Errorf("Failed set orcherstator version when it is not set in the json, got %s", containerService.Properties.OrchestratorProfile.OrchestratorVersion)
t.Errorf("Failed to set orcherstator version when it is not set in the json, got %s", containerService.Properties.OrchestratorProfile.OrchestratorVersion)
}

containerService, _, err = apiloader.LoadContainerServiceFromFile("../acsengine/testdata/v20160930/kubernetes.json", true, false, existingContainerService)
if err != nil {
t.Error(err.Error())
}
if containerService.Properties.OrchestratorProfile.OrchestratorVersion != "1.6.9" {
t.Errorf("Failed set orcherstator version when it is not set in the json, got %s", containerService.Properties.OrchestratorProfile.OrchestratorVersion)
t.Errorf("Failed to set orcherstator version when it is not set in the json, got %s", containerService.Properties.OrchestratorProfile.OrchestratorVersion)
}

containerService, _, err = apiloader.LoadContainerServiceFromFile("../acsengine/testdata/v20170701/kubernetes-default-version.json", true, false, nil)
if err != nil {
t.Error(err.Error())
}
if containerService.Properties.OrchestratorProfile.OrchestratorVersion != common.GetDefaultKubernetesVersion() {
t.Errorf("Failed set orcherstator version when it is not set in the json, got %s", containerService.Properties.OrchestratorProfile.OrchestratorVersion)
t.Errorf("Failed to set orcherstator version when it is not set in the json API v20170701, got %s but expected %s", containerService.Properties.OrchestratorProfile.OrchestratorVersion, common.GetDefaultKubernetesVersion())
}

containerService, _, err = apiloader.LoadContainerServiceFromFile("../acsengine/testdata/v20170701/kubernetes-win-default-version.json", true, false, nil)
if err != nil {
t.Error(err.Error())
}
if containerService.Properties.OrchestratorProfile.OrchestratorVersion != common.GetDefaultKubernetesVersionWindows() {
t.Errorf("Failed to set orcherstator version to windows default when it is not set in the json API v20170701, got %s but expected %s", containerService.Properties.OrchestratorProfile.OrchestratorVersion, common.GetDefaultKubernetesVersionWindows())
}

containerService, _, err = apiloader.LoadContainerServiceFromFile("../acsengine/testdata/v20170131/kubernetes.json", true, false, nil)
if err != nil {
t.Error(err.Error())
}
if containerService.Properties.OrchestratorProfile.OrchestratorVersion != common.GetDefaultKubernetesVersion() {
t.Errorf("Failed set orcherstator version when it is not set in the json, got %s", containerService.Properties.OrchestratorProfile.OrchestratorVersion)
t.Errorf("Failed to set orcherstator version when it is not set in the json API v20170131, got %s but expected %s", containerService.Properties.OrchestratorProfile.OrchestratorVersion, common.GetDefaultKubernetesVersion())
}

containerService, _, err = apiloader.LoadContainerServiceFromFile("../acsengine/testdata/v20170131/kubernetes-win.json", true, false, nil)
if err != nil {
t.Error(err.Error())
}
if containerService.Properties.OrchestratorProfile.OrchestratorVersion != common.GetDefaultKubernetesVersionWindows() {
t.Errorf("Failed to set orcherstator version to windows default when it is not set in the json API v20170131, got %s but expected %s", containerService.Properties.OrchestratorProfile.OrchestratorVersion, common.GetDefaultKubernetesVersionWindows())
}
}
25 changes: 17 additions & 8 deletions pkg/api/common/versions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,11 +68,20 @@ func GetDefaultKubernetesVersionWindows() string {
}

// GetSupportedKubernetesVersion verifies that a passed-in version string is supported, or returns a default version string if not
func GetSupportedKubernetesVersion(version string) string {
if k8sVersion := version; AllKubernetesSupportedVersions[k8sVersion] {
return k8sVersion
func GetSupportedKubernetesVersion(version string, hasWindows bool) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

we shouldn't silently override. If a value is passed in we should throw an error if invalid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would tend to agree with you but this PR is intended to fix the bug, not change the behavior of default k8s version definition... The silent override was already in place for getting the k8s version for API version v20170701 so I don't think we want to change the behavior in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

does validation happen before this so we don't have to worry about it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do validate beforehand. So if I pass in something random as orchestratorVersion such as 0.5 I get FATA[0000] error loading API model in generateCmd: error parsing the api model: OrchestratorProfile has unknown orchestrator version: 0.5
The only case where this function overrides the version is if it is empty or if the cluster is a Windows cluster and the passed in version is valid but not supported for Windows, in which case the user gets default. I looked into fixing validation to also take into account "is this a Windows cluster? If so, is the passed in version valid for Windows?" but it's not straight forward since it requires having the agent pool profiles. I can make that an additional change in this PR if we see value in it.
TLDR: Is it okay to override the version if the value passed in for the user is not supported with windows but is a valid version or should we fail instead?

var k8sVersion string
if hasWindows {
k8sVersion = GetDefaultKubernetesVersionWindows()
if AllKubernetesWindowsSupportedVersions[version] {
k8sVersion = version
}
} else {
k8sVersion = GetDefaultKubernetesVersion()
if AllKubernetesSupportedVersions[version] {
k8sVersion = version
}
}
return GetDefaultKubernetesVersion()
return k8sVersion
}

// GetAllSupportedKubernetesVersions returns a slice of all supported Kubernetes versions
Expand Down Expand Up @@ -248,21 +257,21 @@ func GetSupportedVersions(orchType string, hasWindows bool) (versions []string,
}

//GetValidPatchVersion gets the current valid patch version for the minor version of the passed in version
func GetValidPatchVersion(orchType, orchVer string) string {
func GetValidPatchVersion(orchType, orchVer string, hasWindows bool) string {
if orchVer == "" {
return RationalizeReleaseAndVersion(
orchType,
"",
"",
false)
hasWindows)
}

// check if the current version is valid, this allows us to have multiple supported patch versions in the future if we need it
version := RationalizeReleaseAndVersion(
orchType,
"",
orchVer,
false)
hasWindows)

if version == "" {
sv, err := semver.NewVersion(orchVer)
Expand All @@ -275,7 +284,7 @@ func GetValidPatchVersion(orchType, orchVer string) string {
orchType,
sr,
"",
false)
hasWindows)
}
return version
}
Expand Down
76 changes: 72 additions & 4 deletions pkg/api/common/versions_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,29 @@ func Test_GetAllSupportedKubernetesVersions(t *testing.T) {
func Test_GetSupportedKubernetesVersion(t *testing.T) {
versions := GetAllSupportedKubernetesVersions()
for _, version := range versions {
supportedVersion := GetSupportedKubernetesVersion(version)
supportedVersion := GetSupportedKubernetesVersion(version, false)
if supportedVersion != version {
t.Errorf("GetSupportedKubernetesVersion(%s) should return the same passed-in string, instead returned %s", version, supportedVersion)
}
}

defaultVersion := GetSupportedKubernetesVersion("")
defaultVersion := GetSupportedKubernetesVersion("", false)
if defaultVersion != GetDefaultKubernetesVersion() {
t.Errorf("GetSupportedKubernetesVersion(\"\") should return the default version %s, instead returned %s", GetDefaultKubernetesVersion(), defaultVersion)
}

winVersions := GetAllSupportedKubernetesVersionsWindows()
for _, version := range winVersions {
supportedVersion := GetSupportedKubernetesVersion(version, true)
if supportedVersion != version {
t.Errorf("GetSupportedKubernetesVersion(%s) should return the same passed-in string, instead returned %s", version, supportedVersion)
}
}

defaultWinVersion := GetSupportedKubernetesVersion("", true)
if defaultWinVersion != GetDefaultKubernetesVersionWindows() {
t.Errorf("GetSupportedKubernetesVersion(\"\") should return the default version for windows %s, instead returned %s", GetDefaultKubernetesVersionWindows(), defaultWinVersion)
}
}

func TestGetVersionsGt(t *testing.T) {
Expand Down Expand Up @@ -265,14 +278,28 @@ func TestGetVersionsBetween(t *testing.T) {
}

func Test_GetValidPatchVersion(t *testing.T) {
v := GetValidPatchVersion(Kubernetes, "")
v := GetValidPatchVersion(Kubernetes, "", false)
if v != GetDefaultKubernetesVersion() {
t.Errorf("It is not the default Kubernetes version")
}

for version, enabled := range AllKubernetesSupportedVersions {
if enabled {
v = GetValidPatchVersion(Kubernetes, version)
v = GetValidPatchVersion(Kubernetes, version, false)
if v != version {
t.Errorf("Expected version %s, instead got version %s", version, v)
}
}
}

v = GetValidPatchVersion(Kubernetes, "", true)
if v != GetDefaultKubernetesVersionWindows() {
t.Errorf("It is not the default Kubernetes version")
}

for version, enabled := range AllKubernetesWindowsSupportedVersions {
if enabled {
v = GetValidPatchVersion(Kubernetes, version, true)
if v != version {
t.Errorf("Expected version %s, instead got version %s", version, v)
}
Expand Down Expand Up @@ -398,4 +425,45 @@ func Test_RationalizeReleaseAndVersion(t *testing.T) {
if version != "" {
t.Errorf("It is not empty string")
}

version = RationalizeReleaseAndVersion(Kubernetes, "", "", true)
if version != GetDefaultKubernetesVersionWindows() {
t.Errorf("It is not the default Windows Kubernetes version")
}

version = RationalizeReleaseAndVersion(Kubernetes, "1.6", "", true)
if version != "" {
t.Errorf("It is not empty string")
}

expectedVersion = "1.8.12"
version = RationalizeReleaseAndVersion(Kubernetes, "", expectedVersion, true)
if version != expectedVersion {
t.Errorf("It is not Kubernetes version %s", expectedVersion)
}

version = RationalizeReleaseAndVersion(Kubernetes, "1.8", expectedVersion, true)
if version != expectedVersion {
t.Errorf("It is not Kubernetes version %s", expectedVersion)
}

version = RationalizeReleaseAndVersion(Kubernetes, "", "v"+expectedVersion, true)
if version != expectedVersion {
t.Errorf("It is not Kubernetes version %s", expectedVersion)
}

version = RationalizeReleaseAndVersion(Kubernetes, "v1.8", "v"+expectedVersion, true)
if version != expectedVersion {
t.Errorf("It is not Kubernetes version %s", expectedVersion)
}

version = RationalizeReleaseAndVersion(Kubernetes, "1.1", "", true)
if version != "" {
t.Errorf("It is not empty string")
}

version = RationalizeReleaseAndVersion(Kubernetes, "1.1", "1.6.6", true)
if version != "" {
t.Errorf("It is not empty string")
}
}
Loading