Added FP Check for validation (#3335)

* Added FP Check for validation * Cloud Err to surface level backend for SP issue
Azure · Feb 8, 2024 · 78d366a · 78d366a
1 parent 3640bda
commit 78d366a
Show file tree

Hide file tree

Showing 2 changed files with 28 additions and 30 deletions.
diff --git a/pkg/backend/openshiftcluster.go b/pkg/backend/openshiftcluster.go
@@ -6,6 +6,7 @@ package backend
 import (
 	"context"
 	"fmt"
+	"net/http"
 	"strings"
 	"sync/atomic"
 	"time"
@@ -339,6 +340,11 @@ func (ocb *openShiftClusterBackend) asyncOperationResultLog(log *logrus.Entry, i
 		return
 	}
 
+	if strings.Contains(strings.ToLower(backendErr.Error()), "one of the claims 'puid' or 'altsecid' or 'oid' should be present") {
+		backendErr = api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidServicePrincipalClaims,
+			"properties.servicePrincipalProfile", "The Azure Red Hat Openshift resource provider service principal has been removed from your tenant. To restore, please unregister and then re-register the Azure Red Hat OpenShift resource provider.")
+	}
+
 	_, ok := backendErr.(*api.CloudError)
 	if ok {
 		log = log.WithField("resultType", utillog.UserErrorResultType)

diff --git a/pkg/validate/openshiftcluster_validatedynamic.go b/pkg/validate/openshiftcluster_validatedynamic.go
@@ -93,8 +93,8 @@ func ensureAccessTokenClaims(ctx context.Context, spTokenCredential azcore.Token
 	return api.NewCloudError(
 		http.StatusBadRequest,
 		api.CloudErrorCodeInvalidServicePrincipalClaims,
-		"properties.servicePrincipleProfile",
-		"The provided service principal does not give an access token with at least one of the claims 'altsecid', 'oid' or 'puid'.")
+		"properties.servicePrincipalProfile",
+		"The Azure Red Hat Openshift resource provider service principal has been removed from your tenant. To restore, please unregister and then re-register the Azure Red Hat OpenShift resource provider.")
 }
 
 // Dynamic validates an OpenShift cluster
@@ -113,11 +113,21 @@ func (dv *openShiftClusterDynamicValidator) Dynamic(ctx context.Context) error {
 		})
 	}
 
-	var fpClientCred azcore.TokenCredential
-	var spClientCred azcore.TokenCredential
 	var pdpClient remotepdp.RemotePDPClient
 	spp := dv.oc.Properties.ServicePrincipalProfile
 
+	tenantID := dv.subscriptionDoc.Subscription.Properties.TenantID
+	options := dv.env.Environment().ClientSecretCredentialOptions()
+	spClientCred, err := azidentity.NewClientSecretCredential(
+		tenantID, spp.ClientID, string(spp.ClientSecret), options)
+	if err != nil {
+		return err
+	}
+	fpClientCred, err := dv.env.FPNewClientCertificateCredential(tenantID)
+	if err != nil {
+		return err
+	}
+
 	useCheckAccess, err := dv.env.LiveConfig().UseCheckAccess(ctx)
 	dv.log.Info("USE_CHECKACCESS: ", useCheckAccess)
 	if err != nil {
@@ -130,21 +140,6 @@ func (dv *openShiftClusterDynamicValidator) Dynamic(ctx context.Context) error {
 	) {
 		// TODO remove after successfully migrating to CheckAccess
 		dv.log.Info("Using CheckAccess instead of ListPermissions")
-		var err error
-		fpClientCred, err = dv.env.FPNewClientCertificateCredential(dv.subscriptionDoc.Subscription.Properties.TenantID)
-		if err != nil {
-			return err
-		}
-
-		spClientCred, err = azidentity.NewClientSecretCredential(
-			dv.subscriptionDoc.Subscription.Properties.TenantID,
-			spp.ClientID,
-			string(spp.ClientSecret),
-			nil,
-		)
-		if err != nil {
-			return err
-		}
 
 		aroEnv := dv.env.Environment()
 		pdpClient = remotepdp.NewRemotePDPClient(
@@ -154,20 +149,12 @@ func (dv *openShiftClusterDynamicValidator) Dynamic(ctx context.Context) error {
 		)
 	}
 
-	tenantID := dv.subscriptionDoc.Subscription.Properties.TenantID
-	options := dv.env.Environment().ClientSecretCredentialOptions()
-	spTokenCredential, err := azidentity.NewClientSecretCredential(
-		tenantID, spp.ClientID, string(spp.ClientSecret), options)
-	if err != nil {
-		return err
-	}
-
 	scopes := []string{dv.env.Environment().ResourceManagerScope}
-	err = ensureAccessTokenClaims(ctx, spTokenCredential, scopes)
+	err = ensureAccessTokenClaims(ctx, spClientCred, scopes)
 	if err != nil {
 		return err
 	}
-	spAuthorizer := azidext.NewTokenCredentialAdapter(spTokenCredential, scopes)
+	spAuthorizer := azidext.NewTokenCredentialAdapter(spClientCred, scopes)
 
 	spDynamic := dynamic.NewValidator(
 		dv.log,
@@ -182,7 +169,7 @@ func (dv *openShiftClusterDynamicValidator) Dynamic(ctx context.Context) error {
 	)
 
 	// SP validation
-	err = spDynamic.ValidateServicePrincipal(ctx, spTokenCredential)
+	err = spDynamic.ValidateServicePrincipal(ctx, spClientCred)
 	if err != nil {
 		return err
 	}
@@ -223,6 +210,11 @@ func (dv *openShiftClusterDynamicValidator) Dynamic(ctx context.Context) error {
 		return err
 	}
 
+	err = ensureAccessTokenClaims(ctx, fpClientCred, scopes)
+	if err != nil {
+		return err
+	}
+
 	// FP validation
 	fpDynamic := dynamic.NewValidator(
 		dv.log,