Repository: Azure/ARO-RP
Pull request #2588 (2022-12-09 15:24:07)

Operator checker split - move service principal checker into a separate controller
### Which issue this PR addresses:

Work item [16394925](https://msazure.visualstudio.com/AzureRedHatOpenShift/_workitems/edit/16394925/).

### What this PR does / why we need it:

Apart from the split I did the following changes:

* Now we write the full error into the condition in case of a `CloudError`: previously it was only message from the error (which does not include status code, error code, etc).
* Now we watch for the changes in the service principal secret. This should give us feedback faster (no need to wait for 1 hour in the worst case scenario)
* Also fixes unit test coverage for this checker: currently in master we have a separate code branch just for unit tests and production code was in a different branch.

### Is there any documentation that needs to be updated for this PR?

No, refactoring/optimisation.


Patch:
diff --git a/cmd/aro/operator.go b/cmd/aro/operator.go
index 5a23d54d0..516a17537 100644
--- a/cmd/aro/operator.go
+++ b/cmd/aro/operator.go
@@ -28,6 +28,7 @@ import (
 	"github.com/Azure/ARO-RP/pkg/operator/controllers/banner"
 	"github.com/Azure/ARO-RP/pkg/operator/controllers/checker"
 	"github.com/Azure/ARO-RP/pkg/operator/controllers/checkers/internetchecker"
+	"github.com/Azure/ARO-RP/pkg/operator/controllers/checkers/serviceprincipalchecker"
 	"github.com/Azure/ARO-RP/pkg/operator/controllers/clusteroperatoraro"
 	"github.com/Azure/ARO-RP/pkg/operator/controllers/dnsmasq"
 	"github.com/Azure/ARO-RP/pkg/operator/controllers/genevalogging"
@@ -243,6 +244,11 @@ func operator(ctx context.Context, log *logrus.Entry) error {
 			arocli, kubernetescli, maocli, operatorcli, configcli, role)).SetupWithManager(mgr); err != nil {
 			return fmt.Errorf("unable to create controller %s: %v", checker.ControllerName, err)
 		}
+		if err = (serviceprincipalchecker.NewReconciler(
+			log.WithField("controller", serviceprincipalchecker.ControllerName),
+			arocli, kubernetescli, role)).SetupWithManager(mgr); err != nil {
+			return fmt.Errorf("unable to create controller %s: %v", serviceprincipalchecker.ControllerName, err)
+		}
 	}
 
 	if err = (internetchecker.NewReconciler(
diff --git a/pkg/operator/controllers/checker/checker_controller.go b/pkg/operator/controllers/checker/checker_controller.go
index 315d2337c..60a79597c 100644
--- a/pkg/operator/controllers/checker/checker_controller.go
+++ b/pkg/operator/controllers/checker/checker_controller.go
@@ -38,7 +38,6 @@ type Reconciler struct {
 
 func NewReconciler(log *logrus.Entry, arocli aroclient.Interface, kubernetescli kubernetes.Interface, machinecli machineclient.Interface, operatorcli operatorclient.Interface, configcli configclient.Interface, role string) *Reconciler {
 	checkers := []Checker{
-		NewServicePrincipalChecker(log, arocli, kubernetescli, machinecli, role),
 		NewIngressCertificateChecker(log, arocli, operatorcli, configcli, role),
 		NewClusterDNSChecker(log, arocli, operatorcli, role),
 	}
diff --git a/pkg/operator/controllers/checker/serviceprincipalchecker.go b/pkg/operator/controllers/checker/serviceprincipalchecker.go
deleted file mode 100644
index cfeec2cd5..000000000
--- a/pkg/operator/controllers/checker/serviceprincipalchecker.go
+++ /dev/null
@@ -1,105 +0,0 @@
-package checker
-
-// Copyright (c) Microsoft Corporation.
-// Licensed under the Apache License 2.0.
-
-import (
-	"context"
-
-	operatorv1 "github.com/openshift/api/operator/v1"
-	machineclient "github.com/openshift/client-go/machine/clientset/versioned"
-	"github.com/sirupsen/logrus"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/client-go/kubernetes"
-
-	"github.com/Azure/ARO-RP/pkg/api"
-	"github.com/Azure/ARO-RP/pkg/api/validate/dynamic"
-	arov1alpha1 "github.com/Azure/ARO-RP/pkg/operator/apis/aro.openshift.io/v1alpha1"
-	aroclient "github.com/Azure/ARO-RP/pkg/operator/clientset/versioned"
-	"github.com/Azure/ARO-RP/pkg/util/aad"
-	"github.com/Azure/ARO-RP/pkg/util/azureclient"
-	"github.com/Azure/ARO-RP/pkg/util/clusterauthorizer"
-	"github.com/Azure/ARO-RP/pkg/util/conditions"
-)
-
-type ServicePrincipalChecker struct {
-	log           *logrus.Entry
-	arocli        aroclient.Interface
-	kubernetescli kubernetes.Interface
-	maocli        machineclient.Interface
-
-	role string
-
-	tokenClient              aad.TokenClient
-	validateServicePrincipal dynamic.ServicePrincipalValidator
-}
-
-func NewServicePrincipalChecker(log *logrus.Entry, arocli aroclient.Interface, kubernetescli kubernetes.Interface, maocli machineclient.Interface, role string) *ServicePrincipalChecker {
-	return &ServicePrincipalChecker{
-		log:           log,
-		arocli:        arocli,
-		kubernetescli: kubernetescli,
-		maocli:        maocli,
-		role:          role,
-		tokenClient:   aad.NewTokenClient(),
-	}
-}
-
-func (r *ServicePrincipalChecker) Name() string {
-	return "ServicePrincipalChecker"
-}
-
-func (r *ServicePrincipalChecker) Check(ctx context.Context) error {
-	cond := &operatorv1.OperatorCondition{
-		Type:    arov1alpha1.ServicePrincipalValid,
-		Status:  operatorv1.ConditionTrue,
-		Message: "service principal is valid",
-		Reason:  "CheckDone",
-	}
-
-	cluster, err := r.arocli.AroV1alpha1().Clusters().Get(ctx, arov1alpha1.SingletonClusterName, metav1.GetOptions{})
-	if err != nil {
-		return err
-	}
-
-	azEnv, err := azureclient.EnvironmentFromName(cluster.Spec.AZEnvironment)
-	if err != nil {
-		return err
-	}
-
-	azCred, err := clusterauthorizer.AzCredentials(ctx, r.kubernetescli)
-	if err != nil {
-		return err
-	}
-
-	_, err = r.tokenClient.GetToken(ctx, r.log, string(azCred.ClientID), string(azCred.ClientSecret), string(azCred.TenantID), azEnv.ActiveDirectoryEndpoint, azEnv.ResourceManagerEndpoint)
-	if err != nil {
-		updateFailedCondition(cond, err)
-	}
-
-	spDynamic, err := dynamic.NewServicePrincipalValidator(r.log, &azEnv, dynamic.AuthorizerClusterServicePrincipal, aad.NewTokenClient())
-	if err != nil {
-		return err
-	}
-
-	if r.validateServicePrincipal != nil {
-		err = r.validateServicePrincipal.ValidateServicePrincipal(ctx, string(azCred.ClientID), string(azCred.ClientSecret), string(azCred.TenantID))
-	} else {
-		err = spDynamic.ValidateServicePrincipal(ctx, string(azCred.ClientID), string(azCred.ClientSecret), string(azCred.TenantID))
-	}
-
-	if err != nil {
-		updateFailedCondition(cond, err)
-	}
-
-	return conditions.SetCondition(ctx, r.arocli, cond, r.role)
-}
-
-func updateFailedCondition(cond *operatorv1.OperatorCondition, err error) {
-	cond.Status = operatorv1.ConditionFalse
-	if tErr, ok := err.(*api.CloudError); ok {
-		cond.Message = tErr.Message
-	} else {
-		cond.Message = err.Error()
-	}
-}
diff --git a/pkg/operator/controllers/checkers/serviceprincipalchecker/checker.go b/pkg/operator/controllers/checkers/serviceprincipalchecker/checker.go
new file mode 100644
index 000000000..7ea087d7b
--- /dev/null
+++ b/pkg/operator/controllers/checkers/serviceprincipalchecker/checker.go
@@ -0,0 +1,61 @@
+package serviceprincipalchecker
+
+// Copyright (c) Microsoft Corporation.
+// Licensed under the Apache License 2.0.
+
+import (
+	"context"
+
+	"github.com/sirupsen/logrus"
+	"k8s.io/client-go/kubernetes"
+
+	"github.com/Azure/ARO-RP/pkg/api/validate/dynamic"
+	"github.com/Azure/ARO-RP/pkg/util/aad"
+	"github.com/Azure/ARO-RP/pkg/util/azureclient"
+	"github.com/Azure/ARO-RP/pkg/util/clusterauthorizer"
+)
+
+type servicePrincipalChecker interface {
+	Check(ctx context.Context, AZEnvironment string) error
+}
+
+type checker struct {
+	log *logrus.Entry
+
+	credentialsGetter      func(ctx context.Context) (*clusterauthorizer.Credentials, error)
+	spValidatorConstructor func(azEnv *azureclient.AROEnvironment) (dynamic.ServicePrincipalValidator, error)
+}
+
+func newServicePrincipalChecker(log *logrus.Entry, kubernetescli kubernetes.Interface) servicePrincipalChecker {
+	tokenClient := aad.NewTokenClient()
+
+	return &checker{
+		log: log,
+
+		credentialsGetter: func(ctx context.Context) (*clusterauthorizer.Credentials, error) {
+			return clusterauthorizer.AzCredentials(ctx, kubernetescli)
+		},
+		spValidatorConstructor: func(azEnv *azureclient.AROEnvironment) (dynamic.ServicePrincipalValidator, error) {
+			return dynamic.NewServicePrincipalValidator(log, azEnv, dynamic.AuthorizerClusterServicePrincipal, tokenClient)
+		},
+	}
+}
+
+func (r *checker) Check(ctx context.Context, AZEnvironment string) error {
+	azEnv, err := azureclient.EnvironmentFromName(AZEnvironment)
+	if err != nil {
+		return err
+	}
+
+	azCred, err := r.credentialsGetter(ctx)
+	if err != nil {
+		return err
+	}
+
+	spDynamic, err := r.spValidatorConstructor(&azEnv)
+	if err != nil {
+		return err
+	}
+
+	return spDynamic.ValidateServicePrincipal(ctx, string(azCred.ClientID), string(azCred.ClientSecret), string(azCred.TenantID))
+}
diff --git a/pkg/operator/controllers/checkers/serviceprincipalchecker/controller.go b/pkg/operator/controllers/checkers/serviceprincipalchecker/controller.go
new file mode 100644
index 000000000..212d787d5
--- /dev/null
+++ b/pkg/operator/controllers/checkers/serviceprincipalchecker/controller.go
@@ -0,0 +1,133 @@
+package serviceprincipalchecker
+
+// Copyright (c) Microsoft Corporation.
+// Licensed under the Apache License 2.0.
+
+import (
+	"context"
+	"time"
+
+	operatorv1 "github.com/openshift/api/operator/v1"
+	"github.com/sirupsen/logrus"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/client-go/kubernetes"
+	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/builder"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/handler"
+	"sigs.k8s.io/controller-runtime/pkg/predicate"
+	"sigs.k8s.io/controller-runtime/pkg/reconcile"
+	"sigs.k8s.io/controller-runtime/pkg/source"
+
+	arov1alpha1 "github.com/Azure/ARO-RP/pkg/operator/apis/aro.openshift.io/v1alpha1"
+	aroclient "github.com/Azure/ARO-RP/pkg/operator/clientset/versioned"
+	checkercommon "github.com/Azure/ARO-RP/pkg/operator/controllers/checkers/common"
+	"github.com/Azure/ARO-RP/pkg/util/clusterauthorizer"
+	"github.com/Azure/ARO-RP/pkg/util/conditions"
+)
+
+// This is the permissions that this controller needs to work.
+// "make generate" will run kubebuilder and cause operator/deploy/staticresources/*/role.yaml to be updated
+// from the annotation below.
+// +kubebuilder:rbac:groups=aro.openshift.io,resources=clusters,verbs=get;list;watch
+// +kubebuilder:rbac:groups=aro.openshift.io,resources=clusters/status,verbs=get;update;patch
+
+const (
+	ControllerName = "ServicePrincipalChecker"
+)
+
+// Reconciler runs a number of checkers
+type Reconciler struct {
+	log  *logrus.Entry
+	role string
+
+	arocli  aroclient.Interface
+	checker servicePrincipalChecker
+}
+
+func NewReconciler(log *logrus.Entry, arocli aroclient.Interface, kubernetescli kubernetes.Interface, role string) *Reconciler {
+	return &Reconciler{
+		log:  log,
+		role: role,
+
+		arocli:  arocli,
+		checker: newServicePrincipalChecker(log, kubernetescli),
+	}
+}
+
+// Reconcile will keep checking that the has a valid cluster service principal.
+func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.Result, error) {
+	instance, err := r.arocli.AroV1alpha1().Clusters().Get(ctx, arov1alpha1.SingletonClusterName, metav1.GetOptions{})
+	if err != nil {
+		return reconcile.Result{}, err
+	}
+
+	if !instance.Spec.OperatorFlags.GetSimpleBoolean(checkercommon.ControllerEnabled) {
+		r.log.Debug("controller is disabled")
+		return r.reconcileDisabled(ctx)
+	}
+
+	r.log.Debug("running")
+	checkErr := r.checker.Check(ctx, instance.Spec.AZEnvironment)
+	condition := r.condition(checkErr)
+
+	err = conditions.SetCondition(ctx, r.arocli, condition, r.role)
+	if err != nil {
+		return reconcile.Result{}, err
+	}
+
+	// We always requeue here:
+	// * Either immediately (with rate limiting) based on the error
+	//   when checkErr != nil.
+	// * Or based on RequeueAfter when err == nil.
+	return reconcile.Result{RequeueAfter: time.Hour}, checkErr
+}
+
+func (r *Reconciler) reconcileDisabled(ctx context.Context) (ctrl.Result, error) {
+	condition := &operatorv1.OperatorCondition{
+		Type:   arov1alpha1.ServicePrincipalValid,
+		Status: operatorv1.ConditionUnknown,
+	}
+
+	return reconcile.Result{}, conditions.SetCondition(ctx, r.arocli, condition, r.role)
+}
+
+func (r *Reconciler) condition(checkErr error) *operatorv1.OperatorCondition {
+	if checkErr != nil {
+		return &operatorv1.OperatorCondition{
+			Type:    arov1alpha1.ServicePrincipalValid,
+			Status:  operatorv1.ConditionFalse,
+			Message: checkErr.Error(),
+			Reason:  "CheckFailed",
+		}
+	}
+
+	return &operatorv1.OperatorCondition{
+		Type:    arov1alpha1.ServicePrincipalValid,
+		Status:  operatorv1.ConditionTrue,
+		Message: "service principal is valid",
+		Reason:  "CheckDone",
+	}
+}
+
+// SetupWithManager setup our manager
+func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error {
+	aroClusterPredicate := predicate.NewPredicateFuncs(func(o client.Object) bool {
+		return o.GetName() == arov1alpha1.SingletonClusterName
+	})
+
+	clusterSPPredicate := predicate.NewPredicateFuncs(func(o client.Object) bool {
+		return o.GetName() == clusterauthorizer.AzureCredentialSecretName && o.GetNamespace() == clusterauthorizer.AzureCredentialSecretNameSpace
+	})
+
+	builder := ctrl.NewControllerManagedBy(mgr).
+		For(&arov1alpha1.Cluster{}, builder.WithPredicates(aroClusterPredicate)).
+		Watches(
+			&source.Kind{Type: &corev1.Secret{}},
+			&handler.EnqueueRequestForObject{},
+			builder.WithPredicates(clusterSPPredicate),
+		)
+
+	return builder.Named(ControllerName).Complete(r)
+}
diff --git a/pkg/util/clusterauthorizer/authorizer.go b/pkg/util/clusterauthorizer/authorizer.go
index eba8d62f3..ea903ea49 100644
--- a/pkg/util/clusterauthorizer/authorizer.go
+++ b/pkg/util/clusterauthorizer/authorizer.go
@@ -78,7 +78,7 @@ func (a *azRefreshableAuthorizer) NewRefreshableAuthorizerToken(ctx context.Cont
 
 // AzCredentials gets Cluster Service Principal credentials from the Kubernetes secrets
 func AzCredentials(ctx context.Context, kubernetescli kubernetes.Interface) (*Credentials, error) {
-	mysec, err := kubernetescli.CoreV1().Secrets(azureCredentialSecretNameSpace).Get(ctx, azureCredentialSecretName, metav1.GetOptions{})
+	mysec, err := kubernetescli.CoreV1().Secrets(AzureCredentialSecretNameSpace).Get(ctx, AzureCredentialSecretName, metav1.GetOptions{})
 	if err != nil {
 		return nil, err
 	}
diff --git a/pkg/util/clusterauthorizer/const.go b/pkg/util/clusterauthorizer/const.go
index dc38e4401..80371f2f9 100644
--- a/pkg/util/clusterauthorizer/const.go
+++ b/pkg/util/clusterauthorizer/const.go
@@ -4,6 +4,6 @@ package clusterauthorizer
 // Licensed under the Apache License 2.0.
 
 const (
-	azureCredentialSecretName      = "azure-credentials"
-	azureCredentialSecretNameSpace = "kube-system"
+	AzureCredentialSecretName      = "azure-credentials"
+	AzureCredentialSecretNameSpace = "kube-system"
 )


Test patch:
diff --git a/pkg/operator/controllers/checker/serviceprincipalchecker_test.go b/pkg/operator/controllers/checker/serviceprincipalchecker_test.go
deleted file mode 100644
index df3e58d77..000000000
--- a/pkg/operator/controllers/checker/serviceprincipalchecker_test.go
+++ /dev/null
@@ -1,216 +0,0 @@
-package checker
-
-// Copyright (c) Microsoft Corporation.
-// Licensed under the Apache License 2.0.
-
-import (
-	"context"
-	"fmt"
-	"testing"
-
-	"github.com/golang/mock/gomock"
-	operatorv1 "github.com/openshift/api/operator/v1"
-	azuretypes "github.com/openshift/installer/pkg/types/azure"
-	"github.com/sirupsen/logrus"
-	corev1 "k8s.io/api/core/v1"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/client-go/kubernetes/fake"
-
-	"github.com/Azure/ARO-RP/pkg/api"
-	arov1alpha1 "github.com/Azure/ARO-RP/pkg/operator/apis/aro.openshift.io/v1alpha1"
-	arofake "github.com/Azure/ARO-RP/pkg/operator/clientset/versioned/fake"
-	mock_aad "github.com/Azure/ARO-RP/pkg/util/mocks/aad"
-	mock_dynamic "github.com/Azure/ARO-RP/pkg/util/mocks/dynamic"
-)
-
-func TestServicePrincipalValid(t *testing.T) {
-	ctx := context.Background()
-
-	var (
-		name      = "azure-credentials"
-		nameSpace = "kube-system"
-		log       = logrus.NewEntry(logrus.StandardLogger())
-	)
-
-	for _, tt := range []struct {
-		name                 string
-		aroCluster           *arov1alpha1.Cluster
-		azureSecretName      string
-		azureSecretNameSpace string
-		azureSecret          string
-		secret               *corev1.Secret
-		wantErr              string
-	}{
-		{
-			name:    "fail: aro cluster resource doesn't exist",
-			wantErr: `clusters.aro.openshift.io "cluster" not found`,
-		},
-		{
-			name: "fail: azure-credential secret doesn't exist",
-			aroCluster: &arov1alpha1.Cluster{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: arov1alpha1.SingletonClusterName,
-				},
-				Spec: arov1alpha1.ClusterSpec{
-					AZEnvironment: azuretypes.PublicCloud.Name(),
-				},
-			},
-			wantErr: `secrets "azure-credentials" not found`,
-		},
-		{
-			name: "pass: token authentication",
-			aroCluster: &arov1alpha1.Cluster{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: arov1alpha1.SingletonClusterName,
-				},
-				Spec: arov1alpha1.ClusterSpec{
-					AZEnvironment: azuretypes.PublicCloud.Name(),
-				},
-			},
-			secret: &corev1.Secret{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      name,
-					Namespace: nameSpace,
-				},
-				Data: map[string][]byte{
-					"azure_client_id":     []byte("my-client-id"),
-					"azure_client_secret": []byte("my-client-secret"),
-					"azure_tenant_id":     []byte("my-tenant.example.com"),
-				},
-			},
-		},
-		{
-			name: "pass: dynamic token authentication",
-			aroCluster: &arov1alpha1.Cluster{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: arov1alpha1.SingletonClusterName,
-				},
-				Spec: arov1alpha1.ClusterSpec{
-					AZEnvironment: azuretypes.PublicCloud.Name(),
-				},
-			},
-			secret: &corev1.Secret{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      name,
-					Namespace: nameSpace,
-				},
-				Data: map[string][]byte{
-					"azure_client_id":     []byte("my-client-id"),
-					"azure_client_secret": []byte("my-client-secret"),
-					"azure_tenant_id":     []byte("my-tenant.example.com"),
-				},
-			},
-		},
-	} {
-		arocli := arofake.NewSimpleClientset()
-		kubernetescli := fake.NewSimpleClientset()
-
-		if tt.aroCluster != nil {
-			arocli = arofake.NewSimpleClientset(tt.aroCluster)
-		}
-		if tt.secret != nil {
-			kubernetescli = fake.NewSimpleClientset(tt.secret)
-		}
-
-		controller := gomock.NewController(t)
-		aad := mock_aad.NewMockTokenClient(controller)
-		dynamicController := gomock.NewController(t)
-		dynamic := mock_dynamic.NewMockDynamic(dynamicController)
-
-		if tt.secret != nil {
-			aadCall := aad.EXPECT().GetToken(ctx,
-				log,
-				string(tt.secret.Data["azure_client_id"]),
-				string(tt.secret.Data["azure_client_secret"]),
-				string(tt.secret.Data["azure_tenant_id"]),
-				"https://login.microsoftonline.com/",
-				"https://management.azure.com/").MaxTimes(1).Return(nil, nil)
-
-			dynamic.EXPECT().ValidateServicePrincipal(ctx,
-				string(tt.secret.Data["azure_client_id"]),
-				string(tt.secret.Data["azure_client_secret"]),
-				string(tt.secret.Data["azure_tenant_id"]),
-			).MaxTimes(1).After(aadCall).Return(nil)
-		}
-
-		sp := &ServicePrincipalChecker{
-			log:                      log,
-			arocli:                   arocli,
-			kubernetescli:            kubernetescli,
-			tokenClient:              aad,
-			validateServicePrincipal: dynamic,
-		}
-
-		t.Run(tt.name, func(t *testing.T) {
-			err := sp.Check(ctx)
-
-			if err != nil && err.Error() != tt.wantErr ||
-				err == nil && tt.wantErr != "" {
-				t.Errorf("\n%s\n !=\n%s", err, tt.wantErr)
-			}
-		})
-	}
-}
-
-func TestUpdateFailedCondition(t *testing.T) {
-	for _, tt := range []struct {
-		name       string
-		messageErr error
-		cloudErr   *api.CloudError
-		cond       *operatorv1.OperatorCondition
-		wantCond   *operatorv1.OperatorCondition
-	}{
-		{
-			name: "pass: successful cloud error condition update",
-			cond: &operatorv1.OperatorCondition{
-				Type:    arov1alpha1.ServicePrincipalValid,
-				Status:  operatorv1.ConditionTrue,
-				Message: "service principal is valid",
-				Reason:  "CheckDone",
-			},
-			wantCond: &operatorv1.OperatorCondition{
-				Type:    arov1alpha1.ServicePrincipalValid,
-				Status:  operatorv1.ConditionFalse,
-				Message: "service principal is invalid",
-				Reason:  "CheckDone",
-			},
-			cloudErr: &api.CloudError{
-				StatusCode: 400,
-				CloudErrorBody: &api.CloudErrorBody{
-					Code:    "1",
-					Message: "service principal is invalid",
-				},
-			},
-		},
-		{
-			name: "pass: successful string error condition update",
-			cond: &operatorv1.OperatorCondition{
-				Type:    arov1alpha1.ServicePrincipalValid,
-				Status:  operatorv1.ConditionTrue,
-				Message: "service principal is valid",
-				Reason:  "CheckDone",
-			},
-			wantCond: &operatorv1.OperatorCondition{
-				Type:    arov1alpha1.ServicePrincipalValid,
-				Status:  operatorv1.ConditionFalse,
-				Message: "service principal is invalid",
-				Reason:  "CheckDone",
-			},
-			messageErr: fmt.Errorf("service principal is invalid"),
-		},
-	} {
-		t.Run(tt.name, func(t *testing.T) {
-			if tt.cloudErr != nil {
-				updateFailedCondition(tt.cond, tt.cloudErr)
-			} else if tt.messageErr != nil {
-				updateFailedCondition(tt.cond, tt.messageErr)
-			}
-
-			if tt.cond.Type != tt.wantCond.Type {
-				t.Errorf("\n%s\n !=\n%s", tt.cond.Type, tt.wantCond.Type)
-			} else if tt.cond.Message != tt.wantCond.Message {
-				t.Errorf("\n%s\n !=\n%s", tt.cond.Message, tt.wantCond.Message)
-			}
-		})
-	}
-}
diff --git a/pkg/operator/controllers/checkers/serviceprincipalchecker/checker_test.go b/pkg/operator/controllers/checkers/serviceprincipalchecker/checker_test.go
new file mode 100644
index 000000000..533090877
--- /dev/null
+++ b/pkg/operator/controllers/checkers/serviceprincipalchecker/checker_test.go
@@ -0,0 +1,98 @@
+package serviceprincipalchecker
+
+// Copyright (c) Microsoft Corporation.
+// Licensed under the Apache License 2.0.
+
+import (
+	"context"
+	"errors"
+	"testing"
+
+	"github.com/golang/mock/gomock"
+	azuretypes "github.com/openshift/installer/pkg/types/azure"
+	"github.com/sirupsen/logrus"
+
+	"github.com/Azure/ARO-RP/pkg/api/validate/dynamic"
+	"github.com/Azure/ARO-RP/pkg/util/azureclient"
+	"github.com/Azure/ARO-RP/pkg/util/clusterauthorizer"
+	mock_dynamic "github.com/Azure/ARO-RP/pkg/util/mocks/dynamic"
+)
+
+func TestServicePrincipalValid(t *testing.T) {
+	ctx := context.Background()
+	log := logrus.NewEntry(logrus.StandardLogger())
+	mockCredentials := &clusterauthorizer.Credentials{
+		ClientID:     []byte("fake-client-id"),
+		ClientSecret: []byte("fake-client-secret"),
+		TenantID:     []byte("fake-tenant-id"),
+	}
+
+	for _, tt := range []struct {
+		name             string
+		credentialsExist bool
+		validator        func(controller *gomock.Controller) dynamic.ServicePrincipalValidator
+		wantErr          string
+	}{
+		{
+			name:             "valid service principal",
+			credentialsExist: true,
+			validator: func(controller *gomock.Controller) dynamic.ServicePrincipalValidator {
+				validator := mock_dynamic.NewMockDynamic(controller)
+				validator.EXPECT().ValidateServicePrincipal(ctx, string(mockCredentials.ClientID), string(mockCredentials.ClientSecret), string(mockCredentials.TenantID))
+				return validator
+			},
+		},
+		{
+			name:             "could not instantiate a validator",
+			credentialsExist: true,
+			validator: func(controller *gomock.Controller) dynamic.ServicePrincipalValidator {
+				validator := mock_dynamic.NewMockDynamic(controller)
+				validator.EXPECT().ValidateServicePrincipal(ctx, string(mockCredentials.ClientID), string(mockCredentials.ClientSecret), string(mockCredentials.TenantID)).
+					Return(errors.New("fake validation error"))
+				return validator
+			},
+			wantErr: "fake validation error",
+		},
+		{
+			name:             "could not instantiate a validator",
+			credentialsExist: true,
+			wantErr:          "fake validator constructor error",
+		},
+		{
+			name:    "could not get service principal credentials",
+			wantErr: "fake credentials get error",
+		},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			controller := gomock.NewController(t)
+			defer controller.Finish()
+
+			var validatorMock dynamic.ServicePrincipalValidator
+			if tt.validator != nil {
+				validatorMock = tt.validator(controller)
+			}
+
+			sp := &checker{
+				log: log,
+				credentialsGetter: func(ctx context.Context) (*clusterauthorizer.Credentials, error) {
+					if tt.credentialsExist {
+						return mockCredentials, nil
+					}
+					return nil, errors.New("fake credentials get error")
+				},
+				spValidatorConstructor: func(azEnv *azureclient.AROEnvironment) (dynamic.ServicePrincipalValidator, error) {
+					if validatorMock != nil {
+						return validatorMock, nil
+					}
+					return nil, errors.New("fake validator constructor error")
+				},
+			}
+
+			err := sp.Check(ctx, azuretypes.PublicCloud.Name())
+			if err != nil && err.Error() != tt.wantErr ||
+				err == nil && tt.wantErr != "" {
+				t.Errorf("\n%s\n !=\n%s", err, tt.wantErr)
+			}
+		})
+	}
+}
diff --git a/pkg/operator/controllers/checkers/serviceprincipalchecker/controller_test.go b/pkg/operator/controllers/checkers/serviceprincipalchecker/controller_test.go
new file mode 100644
index 000000000..1c27d314d
--- /dev/null
+++ b/pkg/operator/controllers/checkers/serviceprincipalchecker/controller_test.go
@@ -0,0 +1,132 @@
+package serviceprincipalchecker
+
+// Copyright (c) Microsoft Corporation.
+// Licensed under the Apache License 2.0.
+
+import (
+	"context"
+	"errors"
+	"reflect"
+	"testing"
+	"time"
+
+	operatorv1 "github.com/openshift/api/operator/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/reconcile"
+
+	arov1alpha1 "github.com/Azure/ARO-RP/pkg/operator/apis/aro.openshift.io/v1alpha1"
+	arofake "github.com/Azure/ARO-RP/pkg/operator/clientset/versioned/fake"
+	checkercommon "github.com/Azure/ARO-RP/pkg/operator/controllers/checkers/common"
+	"github.com/Azure/ARO-RP/pkg/util/azureclient"
+	"github.com/Azure/ARO-RP/pkg/util/cmp"
+	utillog "github.com/Azure/ARO-RP/pkg/util/log"
+)
+
+type fakeChecker func(ctx context.Context, AZEnvironment string) error
+
+func (fc fakeChecker) Check(ctx context.Context, AZEnvironment string) error {
+	return fc(ctx, AZEnvironment)
+}
+
+func TestReconcile(t *testing.T) {
+	ctx := context.Background()
+
+	tests := []struct {
+		name                 string
+		controllerDisabled   bool
+		checkerReturnErr     error
+		wantConditionStatus  operatorv1.ConditionStatus
+		wantConditionMessage string
+		wantErr              string
+		wantResult           reconcile.Result
+	}{
+		{
+			name:                 "no errors",
+			wantConditionStatus:  operatorv1.ConditionTrue,
+			wantConditionMessage: "service principal is valid",
+			wantResult:           reconcile.Result{RequeueAfter: time.Hour},
+		},
+		{
+			name:                 "check failed with an error",
+			wantConditionStatus:  operatorv1.ConditionFalse,
+			wantConditionMessage: "fake basic error",
+			checkerReturnErr:     errors.New("fake basic error"),
+			wantErr:              "fake basic error",
+			wantResult:           reconcile.Result{RequeueAfter: time.Hour},
+		},
+		{
+			name:                "controller disabled",
+			controllerDisabled:  true,
+			wantConditionStatus: operatorv1.ConditionUnknown,
+			wantResult:          reconcile.Result{},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			instance := &arov1alpha1.Cluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: arov1alpha1.SingletonClusterName,
+				},
+				Spec: arov1alpha1.ClusterSpec{
+					AZEnvironment: azureclient.PublicCloud.Environment.Name,
+					OperatorFlags: arov1alpha1.OperatorFlags{
+						checkercommon.ControllerEnabled: "true",
+					},
+				},
+			}
+			if tt.controllerDisabled {
+				instance.Spec.OperatorFlags[checkercommon.ControllerEnabled] = "false"
+			}
+
+			arocli := arofake.NewSimpleClientset(instance)
+
+			r := &Reconciler{
+				log:  utillog.GetLogger(),
+				role: "master",
+				checker: fakeChecker(func(ctx context.Context, AZEnvironment string) error {
+					if !reflect.DeepEqual(AZEnvironment, azureclient.PublicCloud.Environment.Name) {
+						t.Error(cmp.Diff(AZEnvironment, azureclient.PublicCloud.Environment.Name))
+					}
+
+					return tt.checkerReturnErr
+				}),
+				arocli: arocli,
+			}
+
+			result, err := r.Reconcile(ctx, ctrl.Request{})
+			if err != nil && err.Error() != tt.wantErr ||
+				err == nil && tt.wantErr != "" {
+				t.Error(err)
+			}
+
+			if !reflect.DeepEqual(tt.wantResult, result) {
+				t.Error(cmp.Diff(tt.wantResult, result))
+			}
+
+			instance, err = arocli.AroV1alpha1().Clusters().Get(ctx, arov1alpha1.SingletonClusterName, metav1.GetOptions{})
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			var condition *operatorv1.OperatorCondition
+			for i := range instance.Status.Conditions {
+				if instance.Status.Conditions[i].Type == arov1alpha1.ServicePrincipalValid {
+					condition = &instance.Status.Conditions[i]
+				}
+			}
+			if condition == nil {
+				t.Fatal("no condition found")
+			}
+
+			if condition.Status != tt.wantConditionStatus {
+				t.Errorf(string(condition.Status))
+			}
+
+			if condition.Message != tt.wantConditionMessage {
+				t.Errorf(condition.Message)
+			}
+		})
+	}
+}