From 3c780fe2a21f2d3f178a85da36835491244d6175 Mon Sep 17 00:00:00 2001 From: Danil-Grigorev Date: Thu, 18 Jun 2020 11:15:27 +0200 Subject: [PATCH] Fix for image selection --- pkg/apis/machine/v1beta1/machine_webhook.go | 38 +++++- .../machine/v1beta1/machine_webhook_test.go | 111 +++++++++++++++++- 2 files changed, 142 insertions(+), 7 deletions(-) diff --git a/pkg/apis/machine/v1beta1/machine_webhook.go b/pkg/apis/machine/v1beta1/machine_webhook.go index 7edc742bb6..96531b67bf 100644 --- a/pkg/apis/machine/v1beta1/machine_webhook.go +++ b/pkg/apis/machine/v1beta1/machine_webhook.go @@ -433,7 +433,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) } @@ -516,16 +516,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 { @@ -562,6 +560,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 index f476279039..b7e4292561 100644 --- a/pkg/apis/machine/v1beta1/machine_webhook_test.go +++ b/pkg/apis/machine/v1beta1/machine_webhook_test.go @@ -301,7 +301,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", @@ -317,7 +388,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", @@ -480,6 +551,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{