Merge pull request #618 from Danil-Grigorev/webhook-image-allow-offer…

…-selection Bug 1845610: Windows VM MachineSet error on Azure
openshift · Jul 1, 2020 · 2c3b0e0 · 2c3b0e0
2 parents 168f501 + 3c780fe
commit 2c3b0e0
Show file tree

Hide file tree

Showing 2 changed files with 142 additions and 7 deletions.
diff --git a/pkg/apis/machine/v1beta1/machine_webhook.go b/pkg/apis/machine/v1beta1/machine_webhook.go
@@ -444,7 +444,7 @@ func defaultAzure(h *defaulterHandler, m *Machine) (bool, utilerrors.Aggregate)
 		}
 	}
 
-	if providerSpec.Image.ResourceID == "" {
+	if providerSpec.Image == (azure.Image{}) {
 		providerSpec.Image.ResourceID = defaultAzureImageResourceID(h.clusterID)
 	}
 
@@ -527,16 +527,14 @@ func validateAzure(h *validatorHandler, m *Machine) (bool, utilerrors.Aggregate)
 		errs = append(errs, field.Required(field.NewPath("providerSpec", "networkResourceGroup"), "must provide a network resource group when a virtual network or subnet is specified"))
 	}
 
-	if providerSpec.Image.ResourceID == "" {
-		errs = append(errs, field.Required(field.NewPath("providerSpec", "image", "resourceID"), "resourceID must be provided"))
-	}
+	errs = append(errs, validateAzureImage(providerSpec.Image)...)
 
 	if providerSpec.ManagedIdentity == "" {
 		errs = append(errs, field.Required(field.NewPath("providerSpec", "managedIdentity"), "managedIdentity must be provided"))
 	}
 
 	if providerSpec.ResourceGroup == "" {
-		errs = append(errs, field.Required(field.NewPath("providerSpec", "resourceGropu"), "resourceGroup must be provided"))
+		errs = append(errs, field.Required(field.NewPath("providerSpec", "resourceGroup"), "resourceGroup must be provided"))
 	}
 
 	if providerSpec.UserDataSecret == nil {
@@ -573,6 +571,36 @@ func validateAzure(h *validatorHandler, m *Machine) (bool, utilerrors.Aggregate)
 	return true, nil
 }
 
+func validateAzureImage(image azure.Image) []error {
+	errors := []error{}
+	if image == (azure.Image{}) {
+		return append(errors, field.Required(field.NewPath("providerSpec", "image"), "an image reference must be provided"))
+	}
+
+	if image.ResourceID != "" {
+		if image != (azure.Image{ResourceID: image.ResourceID}) {
+			return append(errors, field.Required(field.NewPath("providerSpec", "image", "resourceID"), "resourceID is already specified, other fields such as [Offer, Publisher, SKU, Version] should not be set"))
+		}
+		return errors
+	}
+
+	// Resource ID not provided, so Offer, Publisher, SKU and Version are required
+	if image.Offer == "" {
+		errors = append(errors, field.Required(field.NewPath("providerSpec", "image", "Offer"), "Offer must be provided"))
+	}
+	if image.Publisher == "" {
+		errors = append(errors, field.Required(field.NewPath("providerSpec", "image", "Publisher"), "Publisher must be provided"))
+	}
+	if image.SKU == "" {
+		errors = append(errors, field.Required(field.NewPath("providerSpec", "image", "SKU"), "SKU must be provided"))
+	}
+	if image.Version == "" {
+		errors = append(errors, field.Required(field.NewPath("providerSpec", "image", "Version"), "Version must be provided"))
+	}
+
+	return errors
+}
+
 type gcpDefaulter struct {
 	projectID string
 }

diff --git a/pkg/apis/machine/v1beta1/machine_webhook_test.go b/pkg/apis/machine/v1beta1/machine_webhook_test.go
@@ -302,7 +302,78 @@ func TestValidateAzureProviderSpec(t *testing.T) {
 				p.Image = azure.Image{}
 			},
 			expectedOk:    false,
-			expectedError: "providerSpec.image.resourceID: Required value: resourceID must be provided",
+			expectedError: "providerSpec.image: Required value: an image reference must be provided",
+		},
+		{
+			testCase: "with resourceId and other fields set it fails",
+			modifySpec: func(p *azure.AzureMachineProviderSpec) {
+				p.Image = azure.Image{
+					ResourceID: "rid",
+					SKU:        "sku-rand",
+					Offer:      "base-offer",
+					Version:    "1",
+					Publisher:  "test",
+				}
+			},
+			expectedOk:    false,
+			expectedError: "providerSpec.image.resourceID: Required value: resourceID is already specified, other fields such as [Offer, Publisher, SKU, Version] should not be set",
+		},
+		{
+			testCase: "with no offer it fails",
+			modifySpec: func(p *azure.AzureMachineProviderSpec) {
+				p.Image = azure.Image{
+					Version:   "1",
+					SKU:       "sku-rand",
+					Publisher: "test",
+				}
+			},
+			expectedOk:    false,
+			expectedError: "providerSpec.image.Offer: Required value: Offer must be provided",
+		},
+		{
+			testCase: "with no SKU it fails",
+			modifySpec: func(p *azure.AzureMachineProviderSpec) {
+				p.Image = azure.Image{
+					Offer:     "base-offer",
+					Version:   "1",
+					Publisher: "test",
+				}
+			},
+			expectedOk:    false,
+			expectedError: "providerSpec.image.SKU: Required value: SKU must be provided",
+		},
+		{
+			testCase: "with no Version it fails",
+			modifySpec: func(p *azure.AzureMachineProviderSpec) {
+				p.Image = azure.Image{
+					SKU:       "sku-rand",
+					Offer:     "base-offer",
+					Publisher: "test",
+				}
+			},
+			expectedOk:    false,
+			expectedError: "providerSpec.image.Version: Required value: Version must be provided",
+		},
+		{
+			testCase: "with no Publisher it fails",
+			modifySpec: func(p *azure.AzureMachineProviderSpec) {
+				p.Image = azure.Image{
+					SKU:     "sku-rand",
+					Offer:   "base-offer",
+					Version: "1",
+				}
+			},
+			expectedOk:    false,
+			expectedError: "providerSpec.image.Publisher: Required value: Publisher must be provided",
+		},
+		{
+			testCase: "with resourceID in image it succeeds",
+			modifySpec: func(p *azure.AzureMachineProviderSpec) {
+				p.Image = azure.Image{
+					ResourceID: "rid",
+				}
+			},
+			expectedOk: true,
 		},
 		{
 			testCase: "with no managed identity it fails",
@@ -318,7 +389,7 @@ func TestValidateAzureProviderSpec(t *testing.T) {
 				p.ResourceGroup = ""
 			},
 			expectedOk:    false,
-			expectedError: "providerSpec.resourceGropu: Required value: resourceGroup must be provided",
+			expectedError: "providerSpec.resourceGroup: Required value: resourceGroup must be provided",
 		},
 		{
 			testCase: "with no user data secret it fails",
@@ -481,6 +552,42 @@ func TestDefaultAzureProviderSpec(t *testing.T) {
 			expectedOk:    true,
 			expectedError: "",
 		},
+		{
+			testCase: "it does not override azure image spec",
+			providerSpec: &azure.AzureMachineProviderSpec{
+				Image: azure.Image{
+					Offer:     "test-offer",
+					SKU:       "test-sku",
+					Publisher: "base-publisher",
+					Version:   "1",
+				},
+			},
+			modifyDefault: func(p *azure.AzureMachineProviderSpec) {
+				p.Image = azure.Image{
+					Offer:     "test-offer",
+					SKU:       "test-sku",
+					Publisher: "base-publisher",
+					Version:   "1",
+				}
+			},
+			expectedOk:    true,
+			expectedError: "",
+		},
+		{
+			testCase: "it does not override azure image ResourceID",
+			providerSpec: &azure.AzureMachineProviderSpec{
+				Image: azure.Image{
+					ResourceID: "rid",
+				},
+			},
+			modifyDefault: func(p *azure.AzureMachineProviderSpec) {
+				p.Image = azure.Image{
+					ResourceID: "rid",
+				}
+			},
+			expectedOk:    true,
+			expectedError: "",
+		},
 		{
 			testCase: "does not overwrite the network resource group if it already exists",
 			providerSpec: &azure.AzureMachineProviderSpec{