Repository: Azure/ARO-RP
Pull request #1230 (2020-12-15 22:47:33)

Operator: Expose Cluster SP as Condition
Work in progress for some early feedback

### Which issue this PR addresses:
Fixes [8858759](https://msazure.visualstudio.com/AzureRedHatOpenShift/_workitems/edit/8858759/)

### What this PR does / why we need it:

Expose the cluster service principal validity as a condition in the ARO cluster CRD through use of the operator. 

### Test plan for issue:
Unit tests, e2e, manual

Check the Service Principal is Valid
```bash
oc get cluster cluster -o json | jq -r '.status.conditions[] | select(.type == "ServicePrincipalValid") | .'
{
  "lastTransitionTime": "2021-01-04T23:57:03Z",
  "message": "service principal is valid",
  "reason": "CheckDone",
  "status": "True",
  "type": "ServicePrincipalValid"
}
```

Get Service Principal Information
```bash
CLUSTER=bvesel
RESOURCEGROUP=bvesel
CLUSTER_CLIENT_ID=$(az aro show -g bvesel -n $CLUSTER | jq -r .servicePrincipalProfile.clientId)
CLUSTER_SERVICE_PRINCIPAL=$(az ad sp list --all --filter "appId eq '${CLUSTER_CLIENT_ID}'" | jq -r '.[].objectId')
```

Get and delete the VNET role assignment
```bash
$ az role assignment list --all -o json | jq -r --arg RESOURCEGROUP $RESOURCEGROUP --arg ARG $CLUSTER_SERVICE_PRINCIPAL '.[] | select( (.principalId == $ARG) and (.resourceGroup == $RESOURCEGROUP) and (.roleDefinitionName == "Network Contributor") ) | .id'
/subscriptions/SUB_ID/resourceGroups/bvesel/providers/Microsoft.Network/virtualNetworks/bvesel-vnet/providers/Microsoft.Authorization/roleAssignments/RES_ID

$ az role assignment delete --ids \
	/subscriptions/SUB_ID/resourceGroups/bvesel/providers/Microsoft.Network/virtualNetworks/bvesel-vnet/providers/Microsoft.Authorization/roleAssignments/RES_ID
```

Recheck the Service Principal
```bash
$ oc get cluster cluster -o json | jq -r '.status.conditions[] | select(.type == "ServicePrincipalValid") | .'
{
  "lastTransitionTime": "2021-01-05T00:59:04Z",
  "message": "400: InvalidServicePrincipalPermissions: : The provided service principal does not have Network Contributor permission on vnet '/subscriptions/SUB_ID/resourceGroups/bvesel/providers/Microsoft.Network/virtualNetworks/bvesel-vnet'.",
  "reason": "CheckDone",
  "status": "False",
  "type": "ServicePrincipalValid"
}
```

Add the Role Assignment Back
```bash

# This will add back the role assignment
az aro update -g $RESOURCEGROUP -n $CLUSTER

# Check it's back
$ az role assignment list --all -o json | jq -r --arg RESOURCEGROUP $RESOURCEGROUP --arg ARG $CLUSTER_SERVICE_PRINCIPAL '.[] | select( (.principalId == $ARG) and (.resourceGroup == $RESOURCEGROUP) and (.roleDefinitionName == "Network Contributor") ) | .id'
/subscriptions/SUB_ID/resourceGroups/bvesel/providers/Microsoft.Network/virtualNetworks/bvesel-vnet/providers/Microsoft.Authorization/roleAssignments/RES_ID

$ oc get cluster cluster -o json | jq -r '.status.conditions[] | select(.type == "ServicePrincipalValid") | .'
{
  "lastTransitionTime": "2021-01-04T23:57:03Z",
  "message": "service principal is valid",
  "reason": "CheckDone",
  "status": "True",
  "type": "ServicePrincipalValid"
}
```

### Is there any documentation that needs to be updated for this PR?
No

github-actions[bot]: Please rebase pull request.
bennerv: @mjudeikis  @olga-mir  @jim-minter  This is ready for review, but there's a few things that need to be changed before the e2e tests pass.  

I still need to test running the container on a cluster.  I tested locally using a kubeconfig, but when attempting to update the image to a custom quay image I am running into a problem with my glibc version of the compiled binary not matching the version in ubi8-min.  Additionally, we will need to push this to arointsvc.azurecr.io/aro at some point (and then prod) for the e2e test to pass as the new condition being exposed by the operator won't be present for the installation to succeed until the image is updated. 

Will work on reconciling the glibc version so I can test locally and then sync up on any additional review items.  
github-actions[bot]: Please rebase pull request.
troy0820: This PR is marked `next up` but doesn't #1244 need to be merged first and this to be refactored to use the changes from the implementation of the validator? @bennerv @mjudeikis 
bennerv: /azp run e2e
azure-pipelines[bot]: <samp>
Azure Pipelines successfully started running 1 pipeline(s).<br>

</samp>
github-actions[bot]: Please rebase pull request.
github-actions[bot]: Please rebase pull request.
github-actions[bot]: Please rebase pull request.
github-actions[bot]: Please rebase pull request.
github-actions[bot]: Please rebase pull request.
github-actions[bot]: Please rebase pull request.
github-actions[bot]: Please rebase pull request.

Patch:
diff --git a/cmd/aro/operator.go b/cmd/aro/operator.go
index 801288a1c..556c80918 100644
--- a/cmd/aro/operator.go
+++ b/cmd/aro/operator.go
@@ -154,7 +154,7 @@ func operator(ctx context.Context, log *logrus.Entry) error {
 
 	if err = (checker.NewReconciler(
 		log.WithField("controller", controllers.CheckerControllerName),
-		maocli, arocli, role, isDevelopmentMode)).SetupWithManager(mgr); err != nil {
+		maocli, arocli, kubernetescli, role, isDevelopmentMode)).SetupWithManager(mgr); err != nil {
 		return fmt.Errorf("unable to create controller InternetChecker: %v", err)
 	}
 
diff --git a/pkg/operator/apis/aro.openshift.io/v1alpha1/cluster_types.go b/pkg/operator/apis/aro.openshift.io/v1alpha1/cluster_types.go
index 4782d0039..31bd905f3 100644
--- a/pkg/operator/apis/aro.openshift.io/v1alpha1/cluster_types.go
+++ b/pkg/operator/apis/aro.openshift.io/v1alpha1/cluster_types.go
@@ -13,17 +13,18 @@ const (
 	InternetReachableFromMaster status.ConditionType = "InternetReachableFromMaster"
 	InternetReachableFromWorker status.ConditionType = "InternetReachableFromWorker"
 	MachineValid                status.ConditionType = "MachineValid"
+	ServicePrincipalValid       status.ConditionType = "ServicePrincipalValid"
 )
 
 // AllConditionTypes is a operator conditions currently in use, any condition not in this list is not
 // added to the operator.status.conditions list
 func AllConditionTypes() []status.ConditionType {
-	return []status.ConditionType{InternetReachableFromMaster, InternetReachableFromWorker, MachineValid}
+	return []status.ConditionType{InternetReachableFromMaster, InternetReachableFromWorker, MachineValid, ServicePrincipalValid}
 }
 
 // ClusterChecksTypes represents checks performed on the cluster to verify basic functionality
 func ClusterChecksTypes() []status.ConditionType {
-	return []status.ConditionType{InternetReachableFromMaster, InternetReachableFromWorker, MachineValid}
+	return []status.ConditionType{InternetReachableFromMaster, InternetReachableFromWorker, MachineValid, ServicePrincipalValid}
 }
 
 type GenevaLoggingSpec struct {
diff --git a/pkg/operator/controllers/checker/checker_controller.go b/pkg/operator/controllers/checker/checker_controller.go
index c94e81bb8..c3840f5b8 100644
--- a/pkg/operator/controllers/checker/checker_controller.go
+++ b/pkg/operator/controllers/checker/checker_controller.go
@@ -12,6 +12,7 @@ import (
 	"github.com/sirupsen/logrus"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/client-go/kubernetes"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/builder"
 	"sigs.k8s.io/controller-runtime/pkg/handler"
@@ -32,11 +33,14 @@ type CheckerController struct {
 	checkers []Checker
 }
 
-func NewReconciler(log *logrus.Entry, maocli maoclient.Interface, arocli aroclient.Interface, role string, isDevelopmentMode bool) *CheckerController {
+func NewReconciler(log *logrus.Entry, maocli maoclient.Interface, arocli aroclient.Interface, kubernetescli kubernetes.Interface, role string, isDevelopmentMode bool) *CheckerController {
 	checkers := []Checker{NewInternetChecker(log, arocli, role)}
 
 	if role == operator.RoleMaster {
-		checkers = append(checkers, NewMachineChecker(log, maocli, arocli, role, isDevelopmentMode))
+		checkers = append(checkers,
+			NewMachineChecker(log, maocli, arocli, role, isDevelopmentMode),
+			NewServicePrincipalChecker(log, maocli, arocli, kubernetescli, role),
+		)
 	}
 
 	return &CheckerController{
diff --git a/pkg/operator/controllers/checker/const.go b/pkg/operator/controllers/checker/const.go
new file mode 100644
index 000000000..a4f70c304
--- /dev/null
+++ b/pkg/operator/controllers/checker/const.go
@@ -0,0 +1,10 @@
+package checker
+
+// Copyright (c) Microsoft Corporation.
+// Licensed under the Apache License 2.0.
+
+const (
+	azureCredentialSecretNamespace = "kube-system"
+	azureCredentialSecretName      = "azure-credentials"
+	machineSetsNamespace           = "openshift-machine-api"
+)
diff --git a/pkg/operator/controllers/checker/machinechecker.go b/pkg/operator/controllers/checker/machinechecker.go
index a28ba904f..08c5dc499 100644
--- a/pkg/operator/controllers/checker/machinechecker.go
+++ b/pkg/operator/controllers/checker/machinechecker.go
@@ -25,10 +25,6 @@ import (
 	_ "github.com/Azure/ARO-RP/pkg/util/scheme"
 )
 
-const (
-	machineSetsNamespace = "openshift-machine-api"
-)
-
 // MachineChecker reconciles the alertmanager webhook
 type MachineChecker struct {
 	clustercli        maoclient.Interface
diff --git a/pkg/operator/controllers/checker/serviceprincipalchecker.go b/pkg/operator/controllers/checker/serviceprincipalchecker.go
new file mode 100644
index 000000000..3e2b1f680
--- /dev/null
+++ b/pkg/operator/controllers/checker/serviceprincipalchecker.go
@@ -0,0 +1,121 @@
+package checker
+
+// Copyright (c) Microsoft Corporation.
+// Licensed under the Apache License 2.0.
+
+import (
+	"context"
+
+	"github.com/Azure/go-autorest/autorest/azure"
+	maoclient "github.com/openshift/machine-api-operator/pkg/generated/clientset/versioned"
+	"github.com/operator-framework/operator-sdk/pkg/status"
+	"github.com/sirupsen/logrus"
+	corev1 "k8s.io/api/core/v1"
+	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/operator/controllers"
+	"github.com/Azure/ARO-RP/pkg/util/aad"
+)
+
+type ServicePrincipalChecker struct {
+	log           *logrus.Entry
+	clustercli    maoclient.Interface
+	arocli        aroclient.Interface
+	kubernetescli kubernetes.Interface
+	role          string
+}
+
+func NewServicePrincipalChecker(log *logrus.Entry, maocli maoclient.Interface, arocli aroclient.Interface, kubernetescli kubernetes.Interface, role string) *ServicePrincipalChecker {
+	return &ServicePrincipalChecker{
+		log:           log,
+		clustercli:    maocli,
+		arocli:        arocli,
+		kubernetescli: kubernetescli,
+		role:          role,
+	}
+}
+
+func (r *ServicePrincipalChecker) Name() string {
+	return "ServicePrincipalChecker"
+}
+
+func (r *ServicePrincipalChecker) Check(ctx context.Context) error {
+	cond := &status.Condition{
+		Type:    arov1alpha1.ServicePrincipalValid,
+		Status:  corev1.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
+	}
+
+	resource, err := azure.ParseResourceID(cluster.Spec.ResourceID)
+	if err != nil {
+		return err
+	}
+
+	azEnv, err := azure.EnvironmentFromName(cluster.Spec.AZEnvironment)
+	if err != nil {
+		return err
+	}
+
+	azCred, err := azCredentials(ctx, r.kubernetescli)
+	if err != nil {
+		return err
+	}
+
+	_, err = aad.GetToken(ctx, r.log, azCred.clientID, azCred.clientSecret, azCred.tenantID, azEnv.ActiveDirectoryEndpoint, azEnv.ResourceManagerEndpoint)
+	if err != nil {
+		updateFailedCondition(cond, err)
+	}
+
+	spDynamic, err := dynamic.NewValidator(r.log, &azEnv, resource.SubscriptionID, nil, dynamic.AuthorizerClusterServicePrincipal)
+	if err != nil {
+		return err
+	}
+
+	err = spDynamic.ValidateServicePrincipal(ctx, azCred.clientID, azCred.clientSecret, azCred.tenantID)
+	if err != nil {
+		updateFailedCondition(cond, err)
+	}
+
+	return controllers.SetCondition(ctx, r.arocli, cond, r.role)
+}
+
+type credentials struct {
+	clientID     string
+	clientSecret string
+	tenantID     string
+}
+
+func azCredentials(ctx context.Context, kubernetescli kubernetes.Interface) (*credentials, error) {
+	var creds credentials
+
+	mysec, err := kubernetescli.CoreV1().Secrets(azureCredentialSecretNamespace).Get(ctx, azureCredentialSecretName, metav1.GetOptions{})
+	if err != nil {
+		return nil, err
+	}
+
+	creds.clientID = string(mysec.Data["azure_client_id"])
+	creds.clientSecret = string(mysec.Data["azure_client_secret"])
+	creds.tenantID = string(mysec.Data["azure_tenant_id"])
+
+	return &creds, nil
+}
+
+func updateFailedCondition(cond *status.Condition, err error) {
+	cond.Status = corev1.ConditionFalse
+	if tErr, ok := err.(*api.CloudError); ok {
+		cond.Message = tErr.Message
+	} else {
+		cond.Message = err.Error()
+	}
+}


Test patch:
diff --git a/pkg/operator/controllers/checker/serviceprincipalchecker_test.go b/pkg/operator/controllers/checker/serviceprincipalchecker_test.go
new file mode 100644
index 000000000..7bd137702
--- /dev/null
+++ b/pkg/operator/controllers/checker/serviceprincipalchecker_test.go
@@ -0,0 +1,93 @@
+package checker
+
+// Copyright (c) Microsoft Corporation.
+// Licensed under the Apache License 2.0.
+
+import (
+	"context"
+	"fmt"
+	"testing"
+
+	azuretypes "github.com/openshift/installer/pkg/types/azure"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/client-go/kubernetes/fake"
+
+	arov1alpha1 "github.com/Azure/ARO-RP/pkg/operator/apis/aro.openshift.io/v1alpha1"
+	arofake "github.com/Azure/ARO-RP/pkg/operator/clientset/versioned/fake"
+)
+
+// TODO - once aad.GetToken is mockable add tests for other cases
+func TestServicePrincipalValid(t *testing.T) {
+	ctx := context.Background()
+
+	for _, tt := range []struct {
+		name       string
+		aroCluster *arov1alpha1.Cluster
+		wantErr    string
+	}{
+		{
+			name:    "fail: aro cluster resource doesn't exist",
+			wantErr: `clusters.aro.openshift.io "cluster" not found`,
+		},
+		{
+			name: "fail: invalid cluster resource",
+			aroCluster: &arov1alpha1.Cluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: arov1alpha1.SingletonClusterName,
+				},
+				Spec: arov1alpha1.ClusterSpec{
+					AZEnvironment: azuretypes.PublicCloud.Name(),
+					ResourceID:    "invalid_resource",
+				},
+			},
+			wantErr: `parsing failed for invalid_resource. Invalid resource Id format`,
+		},
+		{
+			name: "fail: invalid az environment",
+			aroCluster: &arov1alpha1.Cluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: arov1alpha1.SingletonClusterName,
+				},
+				Spec: arov1alpha1.ClusterSpec{
+					AZEnvironment: "NEVERLAND",
+					ResourceID:    "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/myresourcegroup/providers/Microsoft.RedHatOpenShift/openShiftClusters/mycluster",
+				},
+			},
+			wantErr: `autorest/azure: There is no cloud environment matching the name "NEVERLAND"`,
+		},
+		{
+			name: "fail: azure-credential secret doesn't exist",
+			aroCluster: &arov1alpha1.Cluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: arov1alpha1.SingletonClusterName,
+				},
+				Spec: arov1alpha1.ClusterSpec{
+					AZEnvironment: azuretypes.PublicCloud.Name(),
+					ResourceID:    "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/myresourcegroup/providers/Microsoft.RedHatOpenShift/openShiftClusters/mycluster",
+				},
+			},
+			wantErr: `secrets "azure-credentials" not found`,
+		},
+	} {
+		arocli := arofake.NewSimpleClientset()
+		kubernetescli := fake.NewSimpleClientset()
+
+		if tt.aroCluster != nil {
+			arocli = arofake.NewSimpleClientset(tt.aroCluster)
+		}
+
+		sp := &ServicePrincipalChecker{
+			arocli:        arocli,
+			kubernetescli: kubernetescli,
+		}
+
+		t.Run(tt.name, func(t *testing.T) {
+			err := sp.Check(ctx)
+
+			if err != nil && err.Error() != tt.wantErr ||
+				err == nil && tt.wantErr != "" {
+				t.Error(fmt.Errorf("\n%s\n !=\n%s", err.Error(), tt.wantErr))
+			}
+		})
+	}
+}