fix: Create a new Generic Service interface which provides an individual azure service easy for unittesting (#138)

Create Generic Service interface for all azureclients to be built on top of

Move groups to Service architecture, easier to write a Fake Service and simulate results

Fix accidental use of an older version of azure resources, use consistent versions across

Apply suggestions from code review

Co-Authored-By: awesomenix <[email protected]>

Address review comments, use the top level context defined at scope level

Rename services to a reconciler which hosts groups of services

Revert any changes made to network svc

Author: awesomenix

Gopkg.lock

@@ -27,6 +27,9 @@ import (
 
 // AzureClients contains all the Azure clients used by the scopes.
 type AzureClients struct {
+	SubscriptionID string
+
+	Authorizer autorest.Authorizer
 	// TODO: Remove legacy clients once interfaces are reimplemented
 	Compute   AzureComputeClient
 	Network   AzureNetworkClient
@@ -44,7 +47,6 @@ type AzureClients struct {
 	PublicIPAddresses network.PublicIPAddressesClient
 
 	// Resources
-	Groups      resources.GroupsClient
 	Deployments resources.DeploymentsClient
 	Tags        resources.TagsClient
 }
@@ -86,12 +88,6 @@ type AzureNetworkClient interface {
 
 // AzureResourcesClient defines the operations that will interact with the Azure Resources API
 type AzureResourcesClient interface {
-	// Resource Groups Operations
-	CreateOrUpdateGroup(resourceGroupName string, location string) (resources.Group, error)
-	DeleteGroup(resourceGroupName string) (resources.GroupsDeleteFuture, error)
-	CheckGroupExistence(rgName string) (autorest.Response, error)
-	WaitForGroupsDeleteFuture(future resources.GroupsDeleteFuture) error
-
 	// Deployment Operations
 	CreateOrUpdateDeployment(machine *clusterv1.Machine, clusterConfig *providerv1.AzureClusterProviderSpec, machineConfig *providerv1.AzureMachineProviderSpec, startupScript string) (*resources.DeploymentsCreateOrUpdateFuture, error)
 	GetDeploymentResult(future resources.DeploymentsCreateOrUpdateFuture) (de resources.DeploymentExtended, err error)

pkg/cloud/azure/actuators/clients.go

@@ -2,14 +2,18 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
 
 go_library(
     name = "go_default_library",
-    srcs = ["actuator.go"],
+    srcs = [
+        "actuator.go",
+        "reconciler.go",
+    ],
     importpath = "sigs.k8s.io/cluster-api-provider-azure/pkg/cloud/azure/actuators/cluster",
     visibility = ["//visibility:public"],
     deps = [
         "//pkg/cloud/azure/actuators:go_default_library",
+        "//pkg/cloud/azure/services:go_default_library",
         "//pkg/cloud/azure/services/certificates:go_default_library",
+        "//pkg/cloud/azure/services/groups:go_default_library",
         "//pkg/cloud/azure/services/network:go_default_library",
-        "//pkg/cloud/azure/services/resources:go_default_library",
         "//pkg/deployer:go_default_library",
         "//vendor/github.com/pkg/errors:go_default_library",
         "//vendor/k8s.io/klog:go_default_library",
@@ -23,5 +27,10 @@ go_test(
     name = "go_default_test",
     srcs = ["actuator_test.go"],
     embed = [":go_default_library"],
-    deps = ["//vendor/sigs.k8s.io/cluster-api/pkg/controller/cluster:go_default_library"],
+    deps = [
+        "//pkg/cloud/azure/actuators:go_default_library",
+        "//pkg/cloud/azure/services:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
+        "//vendor/sigs.k8s.io/cluster-api/pkg/apis/cluster/v1alpha1:go_default_library",
+    ],
 )

pkg/cloud/azure/actuators/cluster/BUILD.bazel

@@ -20,9 +20,7 @@ import (
 	"github.com/pkg/errors"
 	"k8s.io/klog"
 	"sigs.k8s.io/cluster-api-provider-azure/pkg/cloud/azure/actuators"
-	"sigs.k8s.io/cluster-api-provider-azure/pkg/cloud/azure/services/certificates"
 	"sigs.k8s.io/cluster-api-provider-azure/pkg/cloud/azure/services/network"
-	"sigs.k8s.io/cluster-api-provider-azure/pkg/cloud/azure/services/resources"
 	"sigs.k8s.io/cluster-api-provider-azure/pkg/deployer"
 	clusterv1 "sigs.k8s.io/cluster-api/pkg/apis/cluster/v1alpha1"
 	client "sigs.k8s.io/cluster-api/pkg/client/clientset_generated/clientset/typed/cluster/v1alpha1"
@@ -63,32 +61,27 @@ func (a *Actuator) Reconcile(cluster *clusterv1.Cluster) error {
 
 	defer scope.Close()
 
-	networkSvc := network.NewService(scope)
-	resourcesSvc := resources.NewService(scope)
-	certSvc := certificates.NewService(scope)
-
-	// Store cert material in spec.
-	if err := certSvc.ReconcileCertificates(); err != nil {
-		return errors.Wrapf(err, "failed to reconcile certificates for cluster %q", cluster.Name)
-	}
-
-	if err := resourcesSvc.ReconcileResourceGroup(); err != nil {
-		return errors.Wrapf(err, "failed to reconcile resource group for cluster %q", cluster.Name)
+	err = NewReconciler(scope).Reconcile()
+	if err != nil {
+		return errors.Wrap(err, "failed to reconcile cluster services")
 	}
 
-	if err := networkSvc.ReconcileNetwork(); err != nil {
-		return errors.Wrapf(err, "failed to reconcile network for cluster %q", cluster.Name)
+	networkSvc := network.NewService(scope)
+	err = networkSvc.ReconcileNetwork()
+	if err != nil {
+		return errors.Wrapf(err, "failed to reconcile network for cluster %s", scope.Cluster.Name)
 	}
 
 	// TODO: Add bastion method
 	/*
 		if err := resourcesSvc.ReconcileBastion(); err != nil {
-			return errors.Wrapf(err, "failed to reconcile bastion host for cluster %q", cluster.Name)
+			return errors.Wrapf(err, "failed to reconcile bastion host for cluster %q", scope.Cluster.Name)
 		}
 	*/
 
-	if err := networkSvc.ReconcileLoadBalancer("api"); err != nil {
-		return errors.Wrapf(err, "failed to reconcile load balancers for cluster %q", cluster.Name)
+	err = networkSvc.ReconcileLoadBalancer("api")
+	if err != nil {
+		return errors.Wrapf(err, "failed to reconcile load balancers for cluster %s", scope.Cluster.Name)
 	}
 
 	return nil
@@ -106,7 +99,6 @@ func (a *Actuator) Delete(cluster *clusterv1.Cluster) error {
 	defer scope.Close()
 
 	//networkSvc := network.NewService(scope)
-	resourcesSvc := resources.NewService(scope)
 
 	// TODO: Add load balancer method
 	/*
@@ -131,8 +123,7 @@ func (a *Actuator) Delete(cluster *clusterv1.Cluster) error {
 			}
 		}
 	*/
-
-	if err := resourcesSvc.DeleteResourceGroup(); err != nil {
+	if err := NewReconciler(scope).Delete(); err != nil {
 		klog.Errorf("Error deleting resource group: %v.", err)
 		return &controllerError.RequeueAfterError{
 			RequeueAfter: 5 * 1000 * 1000 * 1000,

pkg/cloud/azure/actuators/cluster/actuator.go

@@ -17,13 +17,80 @@ limitations under the License.
 package cluster
 
 import (
-	"sigs.k8s.io/cluster-api/pkg/controller/cluster"
-)
+	"context"
+	"testing"
 
-var (
-	_ cluster.Actuator = (*Actuator)(nil)
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"sigs.k8s.io/cluster-api-provider-azure/pkg/cloud/azure/actuators"
+	"sigs.k8s.io/cluster-api-provider-azure/pkg/cloud/azure/services"
+	clusterv1 "sigs.k8s.io/cluster-api/pkg/apis/cluster/v1alpha1"
 )
 
+func createFakeScope() *actuators.Scope {
+	return &actuators.Scope{
+		Context: context.Background(),
+		Cluster: &clusterv1.Cluster{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: "dummyClusterName",
+			},
+		},
+	}
+}
+
+func TestReconcileSuccess(t *testing.T) {
+	fakeSuccessSvc := &services.FakeSuccessService{}
+	fakeNotFoundSvc := &services.FakeNotFoundService{}
+
+	fakeReconciler := &Reconciler{
+		scope:           createFakeScope(),
+		groupsSvc:       fakeSuccessSvc,
+		certificatesSvc: fakeSuccessSvc,
+	}
+
+	if err := fakeReconciler.Reconcile(); err != nil {
+		t.Errorf("failed to reconcile cluster services: %+v", err)
+	}
+
+	if err := fakeReconciler.Delete(); err != nil {
+		t.Errorf("failed to delete cluster services: %+v", err)
+	}
+
+	fakeReconciler.groupsSvc = fakeNotFoundSvc
+
+	if err := fakeReconciler.Delete(); err != nil {
+		t.Errorf("failed to delete cluster services: %+v", err)
+	}
+}
+
+func TestReconcileFailure(t *testing.T) {
+	fakeSuccessSvc := &services.FakeSuccessService{}
+	fakeFailureSvc := &services.FakeFailureService{}
+
+	fakeReconciler := &Reconciler{
+		scope:           createFakeScope(),
+		groupsSvc:       fakeSuccessSvc,
+		certificatesSvc: fakeFailureSvc,
+	}
+
+	if err := fakeReconciler.Reconcile(); err == nil {
+		t.Errorf("expected reconcile to fail")
+	}
+
+	if err := fakeReconciler.Delete(); err != nil {
+		t.Errorf("expected delete to succeed, since we delete groupssvc only")
+	}
+
+	fakeReconciler.groupsSvc = fakeFailureSvc
+
+	if err := fakeReconciler.Reconcile(); err == nil {
+		t.Errorf("expected reconcile to fail")
+	}
+
+	if err := fakeReconciler.Delete(); err == nil {
+		t.Errorf("expected delete to fail")
+	}
+}
+
 // TODO: Reimplement tests
 /*
 import (

pkg/cloud/azure/actuators/cluster/actuator_test.go

@@ -0,0 +1,65 @@
+/*
+Copyright 2019 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package cluster
+
+import (
+	"github.com/pkg/errors"
+	"sigs.k8s.io/cluster-api-provider-azure/pkg/cloud/azure/actuators"
+	"sigs.k8s.io/cluster-api-provider-azure/pkg/cloud/azure/services"
+	"sigs.k8s.io/cluster-api-provider-azure/pkg/cloud/azure/services/certificates"
+	"sigs.k8s.io/cluster-api-provider-azure/pkg/cloud/azure/services/groups"
+)
+
+// Reconciler are list of services required by cluster actuator, easy to create a fake
+type Reconciler struct {
+	scope           *actuators.Scope
+	groupsSvc       services.Service
+	certificatesSvc services.Service
+}
+
+// NewReconciler populates all the services based on input scope
+func NewReconciler(scope *actuators.Scope) *Reconciler {
+	return &Reconciler{
+		scope:           scope,
+		groupsSvc:       groups.NewService(scope),
+		certificatesSvc: certificates.NewService(scope),
+	}
+}
+
+// Reconcile reconciles all the services in pre determined order
+func (s *Reconciler) Reconcile() error {
+	// Store cert material in spec.
+	if err := s.certificatesSvc.CreateOrUpdate(s.scope.Context); err != nil {
+		return errors.Wrapf(err, "failed to CreateOrUpdate certificates for cluster %s", s.scope.Cluster.Name)
+	}
+
+	if err := s.groupsSvc.CreateOrUpdate(s.scope.Context); err != nil {
+		return errors.Wrapf(err, "failed to CreateOrUpdate resource group for cluster %s", s.scope.Cluster.Name)
+	}
+	return nil
+}
+
+// Delete reconciles all the services in pre determined order
+func (s *Reconciler) Delete() error {
+	if err := s.groupsSvc.Delete(s.scope.Context); err != nil {
+		if services.ResourceNotFound(err) {
+			return nil
+		}
+		return errors.Wrapf(err, "failed to Delete resource group for cluster %s", s.scope.Cluster.Name)
+	}
+	return nil
+}