Merge pull request #1223 from CecileRobertMichon/backport-extension-fix

k8s-ci-robot · web-flow · commit ebc75ffb2bde · 2021-03-10T17:10:23.000-08:00
[backport-release-0.4] Ensure VM and VMSS extensions are applied once
diff --git a/cloud/services/scalesets/mock_scalesets/scalesets_mock.go b/cloud/services/scalesets/mock_scalesets/scalesets_mock.go
diff --git a/cloud/services/scalesets/scalesets.go b/cloud/services/scalesets/scalesets.go
@@ -42,6 +42,7 @@ type ScaleSetScope interface {
 	logr.Logger
 	azure.ClusterDescriber
 	ScaleSetSpec() azure.ScaleSetSpec
+	VMSSExtensionSpecs() []azure.VMSSExtensionSpec
 	GetBootstrapData(ctx context.Context) (string, error)
 	GetVMImage() (*infrav1.Image, error)
 	SetAnnotation(string, string)
@@ -329,6 +330,8 @@ func (s *Service) buildVMSSFromSpec(ctx context.Context, vmssSpec azure.ScaleSet
 		vmssSpec.AcceleratedNetworking = &accelNet
 	}
 
+	extensions := s.generateExtensions()
+
 	storageProfile, err := s.generateStorageProfile(vmssSpec, sku)
 	if err != nil {
 		return result, err
@@ -409,6 +412,9 @@ func (s *Service) buildVMSSFromSpec(ctx context.Context, vmssSpec azure.ScaleSet
 				Priority:       priority,
 				EvictionPolicy: evictionPolicy,
 				BillingProfile: billingProfile,
+				ExtensionProfile: &compute.VirtualMachineScaleSetExtensionProfile{
+					Extensions: &extensions,
+				},
 			},
 		},
 	}
@@ -504,6 +510,23 @@ func (s *Service) getVirtualMachineScaleSetIfDone(ctx context.Context, future *i
 	return converters.SDKToVMSS(vmss, vmssInstances), nil
 }
 
+func (s *Service) generateExtensions() []compute.VirtualMachineScaleSetExtension {
+	extensions := make([]compute.VirtualMachineScaleSetExtension, len(s.Scope.VMSSExtensionSpecs()))
+	for i, extensionSpec := range s.Scope.VMSSExtensionSpecs() {
+		extensions[i] = compute.VirtualMachineScaleSetExtension{
+			Name: &extensionSpec.Name,
+			VirtualMachineScaleSetExtensionProperties: &compute.VirtualMachineScaleSetExtensionProperties{
+				Publisher:          to.StringPtr(extensionSpec.Publisher),
+				Type:               to.StringPtr(extensionSpec.Name),
+				TypeHandlerVersion: to.StringPtr(extensionSpec.Version),
+				Settings:           nil,
+				ProtectedSettings:  nil,
+			},
+		}
+	}
+	return extensions
+}
+
 // generateStorageProfile generates a pointer to a compute.VirtualMachineScaleSetStorageProfile which can utilized for VM creation.
 func (s *Service) generateStorageProfile(vmssSpec azure.ScaleSetSpec, sku resourceskus.SKU) (*compute.VirtualMachineScaleSetStorageProfile, error) {
 	storageProfile := &compute.VirtualMachineScaleSetStorageProfile{
diff --git a/cloud/services/scalesets/scalesets_test.go b/cloud/services/scalesets/scalesets_test.go
@@ -896,6 +896,20 @@ func newDefaultVMSS() compute.VirtualMachineScaleSet {
 						},
 					},
 				},
+				ExtensionProfile: &compute.VirtualMachineScaleSetExtensionProfile{
+					Extensions: &[]compute.VirtualMachineScaleSetExtension{
+						{
+							Name: to.StringPtr("someExtension"),
+							VirtualMachineScaleSetExtensionProperties: &compute.VirtualMachineScaleSetExtensionProperties{
+								Publisher:          to.StringPtr("somePublisher"),
+								Type:               to.StringPtr("someExtension"),
+								TypeHandlerVersion: to.StringPtr("someVersion"),
+								Settings:           nil,
+								ProtectedSettings:  nil,
+							},
+						},
+					},
+				},
 				ScheduledEventsProfile: &compute.ScheduledEventsProfile{
 					TerminateNotificationProfile: &compute.TerminateNotificationProfile{
 						Enable:           to.BoolPtr(true),
@@ -984,6 +998,13 @@ func setupDefaultVMSSExpectations(s *mock_scalesets.MockScaleSetScopeMockRecorde
 			Version:   "1.0",
 		},
 	}, nil)
+	s.VMSSExtensionSpecs().Return([]azure.VMSSExtensionSpec{
+		{
+			Name:      "someExtension",
+			Publisher: "somePublisher",
+			Version:   "someVersion",
+		},
+	}).AnyTimes()
 }
 
 func setupDefaultVMSSUpdateExpectations(s *mock_scalesets.MockScaleSetScopeMockRecorder) {
diff --git a/cloud/services/vmextensions/client.go b/cloud/services/vmextensions/client.go
@@ -27,6 +27,7 @@ import (
 
 // Client wraps go-sdk
 type client interface {
+	Get(ctx context.Context, resourceGroupName, vmName, name string) (compute.VirtualMachineExtension, error)
 	CreateOrUpdate(context.Context, string, string, string, compute.VirtualMachineExtension) error
 	Delete(context.Context, string, string, string) error
 }
@@ -51,6 +52,14 @@ func newVirtualMachineExtensionsClient(subscriptionID string, baseURI string, au
 	return vmextensionsClient
 }
 
+// Get the virtual machine extension
+func (ac *azureClient) Get(ctx context.Context, resourceGroupName, vmName, name string) (compute.VirtualMachineExtension, error) {
+	ctx, span := tele.Tracer().Start(ctx, "vmextensions.AzureClient.Get")
+	defer span.End()
+
+	return ac.vmextensions.Get(ctx, resourceGroupName, vmName, name, "")
+}
+
 // CreateOrUpdate creates or updates the virtual machine extension
 func (ac *azureClient) CreateOrUpdate(ctx context.Context, resourceGroupName, vmName, name string, parameters compute.VirtualMachineExtension) error {
 	ctx, span := tele.Tracer().Start(ctx, "vmextensions.AzureClient.CreateOrUpdate")
diff --git a/cloud/services/vmextensions/mock_vmextensions/client_mock.go b/cloud/services/vmextensions/mock_vmextensions/client_mock.go
diff --git a/cloud/services/vmextensions/vmextensions.go b/cloud/services/vmextensions/vmextensions.go
@@ -55,6 +55,11 @@ func (s *Service) Reconcile(ctx context.Context) error {
 	defer span.End()
 
 	for _, extensionSpec := range s.Scope.VMExtensionSpecs() {
+		if _, err := s.client.Get(ctx, s.Scope.ResourceGroup(), extensionSpec.VMName, extensionSpec.Name); err == nil {
+			// check for the extension and don't update if already exists
+			// TODO: set conditions based on extension status
+			continue
+		}
 		s.Scope.V(2).Info("creating VM extension", "vm extension", extensionSpec.Name)
 		err := s.client.CreateOrUpdate(
 			ctx,
@@ -81,6 +86,6 @@ func (s *Service) Reconcile(ctx context.Context) error {
 }
 
 // Delete is a no-op. Extensions will be deleted as part of VM deletion.
-func (s *Service) Delete(ctx context.Context) error {
+func (s *Service) Delete(_ context.Context) error {
 	return nil
 }
diff --git a/cloud/services/vmextensions/vmextensions_test.go b/cloud/services/vmextensions/vmextensions_test.go
@@ -38,6 +38,24 @@ func TestReconcileVMExtension(t *testing.T) {
 		expectedError string
 		expect        func(s *mock_vmextensions.MockVMExtensionScopeMockRecorder, m *mock_vmextensions.MockclientMockRecorder)
 	}{
+		{
+			name:          "extension already exists",
+			expectedError: "",
+			expect: func(s *mock_vmextensions.MockVMExtensionScopeMockRecorder, m *mock_vmextensions.MockclientMockRecorder) {
+				s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New())
+				s.VMExtensionSpecs().Return([]azure.VMExtensionSpec{
+					{
+						Name:      "my-extension-1",
+						VMName:    "my-vm",
+						Publisher: "some-publisher",
+						Version:   "1.0",
+					},
+				})
+				s.ResourceGroup().AnyTimes().Return("my-rg")
+				s.Location().AnyTimes().Return("test-location")
+				m.Get(gomockinternal.AContext(), "my-rg", "my-vm", "my-extension-1")
+			},
+		},
 		{
 			name:          "reconcile multiple extensions",
 			expectedError: "",
@@ -59,7 +77,11 @@ func TestReconcileVMExtension(t *testing.T) {
 				})
 				s.ResourceGroup().AnyTimes().Return("my-rg")
 				s.Location().AnyTimes().Return("test-location")
+				m.Get(gomockinternal.AContext(), "my-rg", "my-vm", "my-extension-1").
+					Return(compute.VirtualMachineExtension{}, autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 404}, "Not found"))
 				m.CreateOrUpdate(gomockinternal.AContext(), "my-rg", "my-vm", "my-extension-1", gomock.AssignableToTypeOf(compute.VirtualMachineExtension{}))
+				m.Get(gomockinternal.AContext(), "my-rg", "my-vm", "other-extension").
+					Return(compute.VirtualMachineExtension{}, autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 404}, "Not found"))
 				m.CreateOrUpdate(gomockinternal.AContext(), "my-rg", "my-vm", "other-extension", gomock.AssignableToTypeOf(compute.VirtualMachineExtension{}))
 			},
 		},
@@ -84,6 +106,8 @@ func TestReconcileVMExtension(t *testing.T) {
 				})
 				s.ResourceGroup().AnyTimes().Return("my-rg")
 				s.Location().AnyTimes().Return("test-location")
+				m.Get(gomockinternal.AContext(), "my-rg", "my-vm", "my-extension-1").
+					Return(compute.VirtualMachineExtension{}, autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 404}, "Not found"))
 				m.CreateOrUpdate(gomockinternal.AContext(), "my-rg", "my-vm", "my-extension-1", gomock.AssignableToTypeOf(compute.VirtualMachineExtension{})).Return(autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error"))
 
 			},
diff --git a/cloud/services/vmssextensions/client.go b/cloud/services/vmssextensions/client.go
@@ -27,6 +27,7 @@ import (
 
 // Client wraps go-sdk
 type client interface {
+	Get(context.Context, string, string, string) (compute.VirtualMachineScaleSetExtension, error)
 	CreateOrUpdate(context.Context, string, string, string, compute.VirtualMachineScaleSetExtension) error
 	Delete(context.Context, string, string, string) error
 }
@@ -51,6 +52,14 @@ func newVirtualMachineScaleSetExtensionsClient(subscriptionID string, baseURI st
 	return vmssextensionsClient
 }
 
+// Get creates or updates the virtual machine scale set extension
+func (ac *azureClient) Get(ctx context.Context, resourceGroupName, vmssName, name string) (compute.VirtualMachineScaleSetExtension, error) {
+	ctx, span := tele.Tracer().Start(ctx, "vmssextensions.AzureClient.Get")
+	defer span.End()
+
+	return ac.vmssextensions.Get(ctx, resourceGroupName, vmssName, name, "")
+}
+
 // CreateOrUpdate creates or updates the virtual machine scale set extension
 func (ac *azureClient) CreateOrUpdate(ctx context.Context, resourceGroupName, vmName, name string, parameters compute.VirtualMachineScaleSetExtension) error {
 	ctx, span := tele.Tracer().Start(ctx, "vmssextensions.AzureClient.CreateOrUpdate")
diff --git a/cloud/services/vmssextensions/mock_vmssextensions/client_mock.go b/cloud/services/vmssextensions/mock_vmssextensions/client_mock.go
diff --git a/cloud/services/vmssextensions/vmssextensions.go b/cloud/services/vmssextensions/vmssextensions.go
@@ -55,7 +55,13 @@ func (s *Service) Reconcile(ctx context.Context) error {
 	defer span.End()
 
 	for _, extensionSpec := range s.Scope.VMSSExtensionSpecs() {
-		s.Scope.V(2).Info("creating VM extension", "vm extension", extensionSpec.Name)
+		if _, err := s.client.Get(ctx, s.Scope.ResourceGroup(), extensionSpec.ScaleSetName, extensionSpec.Name); err == nil {
+			// check for the extension and don't update if already exists
+			// TODO: set conditions based on extension status
+			continue
+		}
+
+		s.Scope.V(2).Info("creating VMSS extension", "vssm extension", extensionSpec.Name)
 		err := s.client.CreateOrUpdate(
 			ctx,
 			s.Scope.ResourceGroup(),
@@ -72,14 +78,14 @@ func (s *Service) Reconcile(ctx context.Context) error {
 			},
 		)
 		if err != nil {
-			return errors.Wrapf(err, "failed to create VM extension %s on scale set %s in resource group %s", extensionSpec.Name, extensionSpec.ScaleSetName, s.Scope.ResourceGroup())
+			return errors.Wrapf(err, "failed to create VMSS extension %s on scale set %s in resource group %s", extensionSpec.Name, extensionSpec.ScaleSetName, s.Scope.ResourceGroup())
 		}
-		s.Scope.V(2).Info("successfully created VM extension", "vm extension", extensionSpec.Name)
+		s.Scope.V(2).Info("successfully created VMSS extension", "vm extension", extensionSpec.Name)
 	}
 	return nil
 }
 
 // Delete is a no-op. Extensions will be deleted as part of VMSS deletion.
-func (s *Service) Delete(ctx context.Context) error {
+func (s *Service) Delete(_ context.Context) error {
 	return nil
 }
diff --git a/cloud/services/vmssextensions/vmssextensions_test.go b/cloud/services/vmssextensions/vmssextensions_test.go
@@ -38,6 +38,24 @@ func TestReconcileVMSSExtension(t *testing.T) {
 		expectedError string
 		expect        func(s *mock_vmssextensions.MockVMSSExtensionScopeMockRecorder, m *mock_vmssextensions.MockclientMockRecorder)
 	}{
+		{
+			name:          "extension already exists",
+			expectedError: "",
+			expect: func(s *mock_vmssextensions.MockVMSSExtensionScopeMockRecorder, m *mock_vmssextensions.MockclientMockRecorder) {
+				s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New())
+				s.VMSSExtensionSpecs().Return([]azure.VMSSExtensionSpec{
+					{
+						Name:         "my-extension-1",
+						ScaleSetName: "my-vmss",
+						Publisher:    "some-publisher",
+						Version:      "1.0",
+					},
+				})
+				s.ResourceGroup().AnyTimes().Return("my-rg")
+				s.Location().AnyTimes().Return("test-location")
+				m.Get(gomockinternal.AContext(), "my-rg", "my-vmss", "my-extension-1")
+			},
+		},
 		{
 			name:          "reconcile multiple extensions",
 			expectedError: "",
@@ -59,13 +77,17 @@ func TestReconcileVMSSExtension(t *testing.T) {
 				})
 				s.ResourceGroup().AnyTimes().Return("my-rg")
 				s.Location().AnyTimes().Return("test-location")
+				m.Get(gomockinternal.AContext(), "my-rg", "my-vmss", "my-extension-1").
+					Return(compute.VirtualMachineScaleSetExtension{}, autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 404}, "Not found"))
 				m.CreateOrUpdate(gomockinternal.AContext(), "my-rg", "my-vmss", "my-extension-1", gomock.AssignableToTypeOf(compute.VirtualMachineScaleSetExtension{}))
+				m.Get(gomockinternal.AContext(), "my-rg", "my-vmss", "other-extension").
+					Return(compute.VirtualMachineScaleSetExtension{}, autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 404}, "Not found"))
 				m.CreateOrUpdate(gomockinternal.AContext(), "my-rg", "my-vmss", "other-extension", gomock.AssignableToTypeOf(compute.VirtualMachineScaleSetExtension{}))
 			},
 		},
 		{
 			name:          "error creating the extension",
-			expectedError: "failed to create VM extension my-extension-1 on scale set my-vmss in resource group my-rg: #: Internal Server Error: StatusCode=500",
+			expectedError: "failed to create VMSS extension my-extension-1 on scale set my-vmss in resource group my-rg: #: Internal Server Error: StatusCode=500",
 			expect: func(s *mock_vmssextensions.MockVMSSExtensionScopeMockRecorder, m *mock_vmssextensions.MockclientMockRecorder) {
 				s.V(gomock.AssignableToTypeOf(2)).AnyTimes().Return(klogr.New())
 				s.VMSSExtensionSpecs().Return([]azure.VMSSExtensionSpec{
@@ -84,6 +106,8 @@ func TestReconcileVMSSExtension(t *testing.T) {
 				})
 				s.ResourceGroup().AnyTimes().Return("my-rg")
 				s.Location().AnyTimes().Return("test-location")
+				m.Get(gomockinternal.AContext(), "my-rg", "my-vmss", "my-extension-1").
+					Return(compute.VirtualMachineScaleSetExtension{}, autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 404}, "Not found"))
 				m.CreateOrUpdate(gomockinternal.AContext(), "my-rg", "my-vmss", "my-extension-1", gomock.AssignableToTypeOf(compute.VirtualMachineScaleSetExtension{})).Return(autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error"))
 
 			},
