Add nil check for managed cluster and its properties object (#3197)

* adding nil check for managed cluster and its properties object

* adding check for update scenario

* adding merge code and UTs

* correcting UTs

* correcting lint errors

pkg/api/agentPoolOnlyApi/v20170831/merge.go

@@ -0,0 +1,44 @@
+package v20170831
+
+import (
+	"fmt"
+)
+
+// Merge existing ManagedCluster attribute into mc
+func (mc *ManagedCluster) Merge(emc *ManagedCluster) error {
+
+	// Merge properties.dnsPrefix
+	if emc.Properties.DNSPrefix == "" {
+		return fmt.Errorf("existing ManagedCluster expect properties.dnsPrefix not to be empty")
+	}
+
+	if mc.Properties.DNSPrefix == "" {
+		mc.Properties.DNSPrefix = emc.Properties.DNSPrefix
+	} else if mc.Properties.DNSPrefix != emc.Properties.DNSPrefix {
+		return fmt.Errorf("change dnsPrefix from %s to %s is not supported",
+			emc.Properties.DNSPrefix,
+			mc.Properties.DNSPrefix)
+	}
+	// Merge Properties.AgentPoolProfiles
+	if mc.Properties.AgentPoolProfiles == nil {
+		mc.Properties.AgentPoolProfiles = emc.Properties.AgentPoolProfiles
+	}
+	// Merge Properties.LinuxProfile
+	if mc.Properties.LinuxProfile == nil {
+		mc.Properties.LinuxProfile = emc.Properties.LinuxProfile
+	}
+	// Merge Properties.WindowsProfile
+	if mc.Properties.WindowsProfile == nil {
+		mc.Properties.WindowsProfile = emc.Properties.WindowsProfile
+	}
+	// Merge Properties.ServicePrincipalProfile
+	if mc.Properties.ServicePrincipalProfile == nil {
+		mc.Properties.ServicePrincipalProfile = emc.Properties.ServicePrincipalProfile
+	}
+	// Merge Properties.KubernetesVersion
+	if mc.Properties.KubernetesVersion == "" {
+		mc.Properties.KubernetesVersion = emc.Properties.KubernetesVersion
+	}
+
+	return nil
+}

pkg/api/agentPoolOnlyApi/v20180331/apiloader_test.go

@@ -95,7 +95,7 @@ var _ = Describe("v20180331 test suite", func() {
 			// ssh key should not be re-generated
 			Expect(sshAutoGenerated).To(BeFalse())
 			Expect(cs2.Properties.MasterProfile).To(BeNil())
-			Expect(cs2.Properties.LinuxProfile).To(BeNil())
+			Expect(cs2.Properties.LinuxProfile).NotTo(BeNil())
 			Expect(cs2.Properties.OrchestratorProfile).NotTo(BeNil())
 			Expect(cs2.Properties.OrchestratorProfile.OrchestratorVersion).To(Equal(k8sVersions[0]))
 			Expect(cs2.Properties.OrchestratorProfile.KubernetesConfig.NetworkPolicy).To(Equal(""))
@@ -105,10 +105,10 @@ var _ = Describe("v20180331 test suite", func() {
 			Expect(cs2.Properties.OrchestratorProfile.KubernetesConfig.DockerBridgeSubnet).To(Equal(api.DefaultDockerBridgeSubnet))
 			Expect(*cs2.Properties.OrchestratorProfile.KubernetesConfig.EnableRbac).To(Equal(false))
 			Expect(*cs2.Properties.OrchestratorProfile.KubernetesConfig.EnableSecureKubelet).To(Equal(false))
-			Expect(cs2.Properties.ServicePrincipalProfile).To(BeNil())
+			Expect(cs2.Properties.ServicePrincipalProfile).NotTo(BeNil())
 			Expect(cs2.Properties.HostedMasterProfile).NotTo(BeNil())
 			Expect(cs2.Properties.HostedMasterProfile.DNSPrefix).To(Equal(model.Properties.DNSPrefix))
-			Expect(len(cs2.Properties.AgentPoolProfiles)).To(Equal(0))
+			Expect(len(cs2.Properties.AgentPoolProfiles)).To(Equal(1))
 		})
 	})
 
@@ -185,7 +185,7 @@ var _ = Describe("v20180331 test suite", func() {
 			// ssh key should not be re-generated
 			Expect(sshAutoGenerated).To(BeFalse())
 			Expect(cs2.Properties.MasterProfile).To(BeNil())
-			Expect(cs2.Properties.LinuxProfile).To(BeNil())
+			Expect(cs2.Properties.LinuxProfile).NotTo(BeNil())
 			Expect(cs2.Properties.OrchestratorProfile).NotTo(BeNil())
 			Expect(cs2.Properties.OrchestratorProfile.OrchestratorVersion).To(Equal(k8sVersions[0]))
 			Expect(cs2.Properties.OrchestratorProfile.KubernetesConfig.NetworkPlugin).To(Equal("azure"))
@@ -195,10 +195,10 @@ var _ = Describe("v20180331 test suite", func() {
 			Expect(cs2.Properties.OrchestratorProfile.KubernetesConfig.DockerBridgeSubnet).To(Equal(dockerBridgeCidr))
 			Expect(*cs2.Properties.OrchestratorProfile.KubernetesConfig.EnableRbac).To(Equal(false))
 			Expect(*cs2.Properties.OrchestratorProfile.KubernetesConfig.EnableSecureKubelet).To(Equal(false))
-			Expect(cs2.Properties.ServicePrincipalProfile).To(BeNil())
+			Expect(cs2.Properties.ServicePrincipalProfile).NotTo(BeNil())
 			Expect(cs2.Properties.HostedMasterProfile).NotTo(BeNil())
 			Expect(cs2.Properties.HostedMasterProfile.DNSPrefix).To(Equal(model.Properties.DNSPrefix))
-			Expect(len(cs2.Properties.AgentPoolProfiles)).To(Equal(0))
+			Expect(len(cs2.Properties.AgentPoolProfiles)).To(Equal(1))
 		})
 	})
 
@@ -279,7 +279,7 @@ var _ = Describe("v20180331 test suite", func() {
 			// ssh key should not be re-generated
 			Expect(sshAutoGenerated).To(BeFalse())
 			Expect(cs2.Properties.MasterProfile).To(BeNil())
-			Expect(cs2.Properties.LinuxProfile).To(BeNil())
+			Expect(cs2.Properties.LinuxProfile).NotTo(BeNil())
 			Expect(cs2.Properties.OrchestratorProfile).NotTo(BeNil())
 			Expect(cs2.Properties.OrchestratorProfile.OrchestratorVersion).To(Equal(k8sVersions[0]))
 			Expect(cs2.Properties.OrchestratorProfile.KubernetesConfig.NetworkPlugin).To(Equal("azure"))
@@ -365,7 +365,7 @@ var _ = Describe("v20180331 test suite", func() {
 			// ssh key should not be re-generated
 			Expect(sshAutoGenerated).To(BeFalse())
 			Expect(cs2.Properties.MasterProfile).To(BeNil())
-			Expect(cs2.Properties.LinuxProfile).To(BeNil())
+			Expect(cs2.Properties.LinuxProfile).NotTo(BeNil())
 			Expect(cs2.Properties.OrchestratorProfile).NotTo(BeNil())
 			Expect(cs2.Properties.OrchestratorProfile.OrchestratorVersion).To(Equal(k8sVersions[1]))
 			Expect(cs2.Properties.OrchestratorProfile.KubernetesConfig.NetworkPlugin).To(Equal("azure"))
@@ -377,7 +377,7 @@ var _ = Describe("v20180331 test suite", func() {
 			Expect(*cs2.Properties.OrchestratorProfile.KubernetesConfig.EnableSecureKubelet).To(Equal(false))
 			Expect(cs2.Properties.HostedMasterProfile).NotTo(BeNil())
 			Expect(cs2.Properties.HostedMasterProfile.DNSPrefix).To(Equal(model.Properties.DNSPrefix))
-			Expect(len(cs2.Properties.AgentPoolProfiles)).To(Equal(0))
+			Expect(len(cs2.Properties.AgentPoolProfiles)).To(Equal(1))
 		})
 	})
 })

pkg/api/agentPoolOnlyApi/v20180331/merge.go

@@ -22,6 +22,27 @@ func (mc *ManagedCluster) Merge(emc *ManagedCluster) error {
 			mc.Properties.DNSPrefix)
 	}
 
+	// Merge Properties.AgentPoolProfiles
+	if mc.Properties.AgentPoolProfiles == nil {
+		mc.Properties.AgentPoolProfiles = emc.Properties.AgentPoolProfiles
+	}
+	// Merge Properties.LinuxProfile
+	if mc.Properties.LinuxProfile == nil {
+		mc.Properties.LinuxProfile = emc.Properties.LinuxProfile
+	}
+	// Merge Properties.WindowsProfile
+	if mc.Properties.WindowsProfile == nil {
+		mc.Properties.WindowsProfile = emc.Properties.WindowsProfile
+	}
+	// Merge Properties.ServicePrincipalProfile
+	if mc.Properties.ServicePrincipalProfile == nil {
+		mc.Properties.ServicePrincipalProfile = emc.Properties.ServicePrincipalProfile
+	}
+	// Merge Properties.KubernetesVersion
+	if mc.Properties.KubernetesVersion == "" {
+		mc.Properties.KubernetesVersion = emc.Properties.KubernetesVersion
+	}
+
 	// Merge properties.enableRBAC
 	if emc.Properties.EnableRBAC == nil {
 		return fmt.Errorf("existing ManagedCluster expect properties.enableRBAC not to be nil")

pkg/api/apiloader.go

@@ -200,30 +200,59 @@ func (a *Apiloader) LoadContainerServiceForAgentPoolOnlyCluster(
 		if e := json.Unmarshal(contents, &managedCluster); e != nil {
 			return nil, IsSSHAutoGenerated, e
 		}
-		// verify orchestrator version
-		if len(managedCluster.Properties.KubernetesVersion) > 0 && !common.AllKubernetesSupportedVersions[managedCluster.Properties.KubernetesVersion] {
-			return nil, IsSSHAutoGenerated, a.Translator.Errorf("The selected orchestrator version '%s' is not supported", managedCluster.Properties.KubernetesVersion)
+		// verify managedCluster.Properties is not nil for creating case
+		if managedCluster.Properties == nil {
+			if !isUpdate {
+				return nil, false, a.Translator.Errorf("properties object in managed cluster should not be nil")
+			}
+			managedCluster.Properties = &v20170831.Properties{}
+		}
+
+		if hasExistingCS {
+			vemc := ConvertContainerServiceToV20170831AgentPoolOnly(existingContainerService)
+			if e := managedCluster.Merge(vemc); e != nil {
+				return nil, IsSSHAutoGenerated, e
+			}
 		}
+
 		// use defaultKubernetesVersion arg if no version was supplied in the request contents
 		if managedCluster.Properties.KubernetesVersion == "" && defaultKubernetesVersion != "" {
 			if !common.AllKubernetesSupportedVersions[defaultKubernetesVersion] {
 				return nil, IsSSHAutoGenerated, a.Translator.Errorf("The selected orchestrator version '%s' is not supported", defaultKubernetesVersion)
 			}
 			managedCluster.Properties.KubernetesVersion = defaultKubernetesVersion
 		}
+
+		// verify orchestrator version
+		if len(managedCluster.Properties.KubernetesVersion) > 0 && !common.AllKubernetesSupportedVersions[managedCluster.Properties.KubernetesVersion] {
+			return nil, IsSSHAutoGenerated, a.Translator.Errorf("The selected orchestrator version '%s' is not supported", managedCluster.Properties.KubernetesVersion)
+		}
+
 		if e := managedCluster.Properties.Validate(); validate && e != nil {
 			return nil, IsSSHAutoGenerated, e
 		}
+
 		return ConvertV20170831AgentPoolOnly(managedCluster), false, nil
 	case v20180331.APIVersion:
 		managedCluster := &v20180331.ManagedCluster{}
 		if e := json.Unmarshal(contents, &managedCluster); e != nil {
 			return nil, IsSSHAutoGenerated, e
 		}
-		// verify orchestrator version
-		if len(managedCluster.Properties.KubernetesVersion) > 0 && !common.AllKubernetesSupportedVersions[managedCluster.Properties.KubernetesVersion] {
-			return nil, IsSSHAutoGenerated, a.Translator.Errorf("The selected orchestrator version '%s' is not supported", managedCluster.Properties.KubernetesVersion)
+		// verify managedCluster.Properties is not nil for creating case
+		if managedCluster.Properties == nil {
+			if !isUpdate {
+				return nil, false, a.Translator.Errorf("properties object in managed cluster should not be nil")
+			}
+			managedCluster.Properties = &v20180331.Properties{}
+		}
+
+		if hasExistingCS {
+			vemc := ConvertContainerServiceToV20180331AgentPoolOnly(existingContainerService)
+			if e := managedCluster.Merge(vemc); e != nil {
+				return nil, IsSSHAutoGenerated, e
+			}
 		}
+
 		// use defaultKubernetesVersion arg if no version was supplied in the request contents
 		if managedCluster.Properties.KubernetesVersion == "" && defaultKubernetesVersion != "" {
 			if !common.AllKubernetesSupportedVersions[defaultKubernetesVersion] {
@@ -236,12 +265,11 @@ func (a *Apiloader) LoadContainerServiceForAgentPoolOnlyCluster(
 			}
 		}
 
-		if hasExistingCS {
-			vemc := ConvertContainerServiceToV20180331AgentPoolOnly(existingContainerService)
-			if e := managedCluster.Merge(vemc); e != nil {
-				return nil, IsSSHAutoGenerated, e
-			}
+		// verify orchestrator version
+		if len(managedCluster.Properties.KubernetesVersion) > 0 && !common.AllKubernetesSupportedVersions[managedCluster.Properties.KubernetesVersion] {
+			return nil, IsSSHAutoGenerated, a.Translator.Errorf("The selected orchestrator version '%s' is not supported", managedCluster.Properties.KubernetesVersion)
 		}
+
 		if e := managedCluster.Properties.Validate(); validate && e != nil {
 			return nil, IsSSHAutoGenerated, e
 		}

pkg/api/apiloader_test.go

@@ -1,9 +1,15 @@
 package api
 
 import (
+	"encoding/json"
+
+	"github.com/Azure/acs-engine/pkg/api/agentPoolOnlyApi/v20170831"
+	"github.com/Azure/acs-engine/pkg/api/agentPoolOnlyApi/v20180331"
 	"github.com/Azure/acs-engine/pkg/api/common"
 	"github.com/Azure/acs-engine/pkg/i18n"
 	"github.com/leonelquinteros/gotext"
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
 
 	"path"
 	"testing"
@@ -84,4 +90,124 @@ func TestLoadContainerServiceFromFile(t *testing.T) {
 	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())
 	}
+
+}
+
+func TestLoadContainerServiceForAgentPoolOnlyCluster(t *testing.T) {
+	var _ = Describe("create/update cluster operations", func() {
+		locale := gotext.NewLocale(path.Join("../../..", "../../..", "translations"), "en_US")
+		i18n.Initialize(locale)
+		apiloader := &Apiloader{
+			Translator: &i18n.Translator{
+				Locale: locale,
+			},
+		}
+		k8sVersions := common.GetAllSupportedKubernetesVersions()
+		defaultK8sVersion := common.GetDefaultKubernetesVersion()
+
+		Context("v20180331", func() {
+			It("it should return error if managed cluster body is empty", func() {
+
+				model := v20180331.ManagedCluster{}
+
+				modelString, _ := json.Marshal(model)
+				_, _, err := apiloader.LoadContainerServiceForAgentPoolOnlyCluster([]byte(modelString), "2018-03-31", false, false, defaultK8sVersion, nil)
+				Expect(err).NotTo(BeNil())
+			})
+
+			It("it should merge if managed cluster body is empty and trying to update", func() {
+				model := v20180331.ManagedCluster{
+					Name: "myaks",
+					Properties: &v20180331.Properties{
+						DNSPrefix:         "myaks",
+						KubernetesVersion: k8sVersions[0],
+						AgentPoolProfiles: []*v20180331.AgentPoolProfile{
+							{
+								Name:           "agentpool1",
+								Count:          3,
+								VMSize:         "Standard_DS2_v2",
+								OSDiskSizeGB:   0,
+								StorageProfile: "ManagedDisk",
+							},
+						},
+						ServicePrincipalProfile: &v20180331.ServicePrincipalProfile{
+							ClientID: "clientID",
+							Secret:   "clientSecret",
+						},
+					},
+				}
+				modelString, _ := json.Marshal(model)
+				cs, sshAutoGenerated, err := apiloader.LoadContainerServiceForAgentPoolOnlyCluster([]byte(modelString), "2018-03-31", false, false, defaultK8sVersion, nil)
+				Expect(err).To(BeNil())
+				Expect(sshAutoGenerated).To(BeFalse())
+
+				model2 := v20180331.ManagedCluster{}
+				modelString2, _ := json.Marshal(model2)
+				cs2, sshAutoGenerated, err := apiloader.LoadContainerServiceForAgentPoolOnlyCluster([]byte(modelString2), "2018-03-31", false, true, defaultK8sVersion, cs)
+
+				Expect(err).To(BeNil())
+				// ssh key should not be re-generated
+				Expect(sshAutoGenerated).To(BeFalse())
+				Expect(cs2.Properties.AgentPoolProfiles).NotTo(BeNil())
+				Expect(cs2.Properties.LinuxProfile).NotTo(BeNil())
+				Expect(cs2.Properties.WindowsProfile).NotTo(BeNil())
+				Expect(cs2.Properties.ServicePrincipalProfile).NotTo(BeNil())
+				Expect(cs2.Properties.HostedMasterProfile).NotTo(BeNil())
+				Expect(cs2.Properties.HostedMasterProfile.DNSPrefix).To(Equal(model.Properties.DNSPrefix))
+				Expect(cs2.Properties.OrchestratorProfile.OrchestratorVersion).To(Equal(k8sVersions[0]))
+			})
+		})
+
+		Context("20170831", func() {
+			It("it should return error if managed cluster body is empty", func() {
+
+				model := v20170831.ManagedCluster{}
+
+				modelString, _ := json.Marshal(model)
+				_, _, err := apiloader.LoadContainerServiceForAgentPoolOnlyCluster([]byte(modelString), "2018-03-31", false, false, defaultK8sVersion, nil)
+				Expect(err).NotTo(BeNil())
+			})
+
+			It("it should merge if managed cluster body is empty and trying to update", func() {
+				model := v20170831.ManagedCluster{
+					Name: "myaks",
+					Properties: &v20170831.Properties{
+						DNSPrefix:         "myaks",
+						KubernetesVersion: k8sVersions[0],
+						AgentPoolProfiles: []*v20170831.AgentPoolProfile{
+							{
+								Name:           "agentpool1",
+								Count:          3,
+								VMSize:         "Standard_DS2_v2",
+								OSDiskSizeGB:   0,
+								StorageProfile: "ManagedDisk",
+							},
+						},
+						ServicePrincipalProfile: &v20170831.ServicePrincipalProfile{
+							ClientID: "clientID",
+							Secret:   "clientSecret",
+						},
+					},
+				}
+				modelString, _ := json.Marshal(model)
+				cs, sshAutoGenerated, err := apiloader.LoadContainerServiceForAgentPoolOnlyCluster([]byte(modelString), "2018-03-31", false, false, defaultK8sVersion, nil)
+				Expect(err).To(BeNil())
+				Expect(sshAutoGenerated).To(BeFalse())
+
+				model2 := v20170831.ManagedCluster{}
+				modelString2, _ := json.Marshal(model2)
+				cs2, sshAutoGenerated, err := apiloader.LoadContainerServiceForAgentPoolOnlyCluster([]byte(modelString2), "2018-03-31", false, true, defaultK8sVersion, cs)
+
+				Expect(err).To(BeNil())
+				// ssh key should not be re-generated
+				Expect(sshAutoGenerated).To(BeFalse())
+				Expect(cs2.Properties.AgentPoolProfiles).NotTo(BeNil())
+				Expect(cs2.Properties.LinuxProfile).NotTo(BeNil())
+				Expect(cs2.Properties.WindowsProfile).NotTo(BeNil())
+				Expect(cs2.Properties.ServicePrincipalProfile).NotTo(BeNil())
+				Expect(cs2.Properties.HostedMasterProfile).NotTo(BeNil())
+				Expect(cs2.Properties.HostedMasterProfile.DNSPrefix).To(Equal(model.Properties.DNSPrefix))
+			})
+		})
+	})
 }