Refactor dynamic validator to be operator compatible

[mjudeikis]

Which issue this PR addresses:

Fixes our ability to consume dynamic validation in context of Cluster/Operator.
Suggestion for #1230

What this PR does / why we need it:

Introduced 2 flavors of dynamic validator:
Full - all validators. Intended to run in the context of RP.
Slim - Validators intended to be running in the context of Operator.

It would need more refactoring so could consume the data from the validator. But might be good start.

Test plan for issue:

unit tests

Is there any documentation that needs to be updated for this PR?

no

[mjudeikis]

Internal methods args now are big hairy but external consumption should be same or easier.

[ihcsim]

s/NewOpenShiftClusterlimDynamicValidator/NewOpenShiftClusterSlimDynamicValidator

[ihcsim]

AFAICT, these two fields aren't needed by the slim validator. They are only used by the full validator at line 178 and 198. If these fields can be moved to the full validator and the full validator can have its own logrus.Entry, then you might not need to embed the slim validator.

[mjudeikis]

Im not sure what you have in mind here.
TenatID is to avoid passing in all subscription documents. Same with subscriptionID.
And envName is to instantiate azureEnvironment.

Or I misunderstood the comment?

[ihcsim]

I was referring to the spProviders and spVirtualNetworks fields. My thought is to not embed the slim validator in the full validator since the slim validator has other unneeded fields.

Taking it a step further, we might not even need to declare them as fields in the struct. Will something like the following work?

diff --git a/pkg/api/validate/openshiftcluster_validatedynamic.go b/pkg/api/validate/openshiftcluster_validatedynamic.go
index e0ef97e3..1f290fc6 100644
--- a/pkg/api/validate/openshiftcluster_validatedynamic.go
+++ b/pkg/api/validate/openshiftcluster_validatedynamic.go
@@ -48,16 +48,13 @@ type OpenShiftClusterSlimDynamicValidator interface {
 // NewOpenShiftClusterFullDynamicValidator creates a new OpenShiftClusterFullDynamicValidator
 func NewOpenShiftClusterFullDynamicValidator(log *logrus.Entry, env env.Interface, oc *api.OpenShiftCluster, subscriptionDoc *api.SubscriptionDocument, fpAuthorizer refreshable.Authorizer) OpenShiftClusterFullDynamicValidator {
 	return &openShiftClusterFullDynamicValidator{
-		openShiftClusterSlimDynamicValidator: &openShiftClusterSlimDynamicValidator{
-			log: log,
-		},
 		env: env,
 
 		oc:              oc,
 		subscriptionDoc: subscriptionDoc,
 		fpAuthorizer:    fpAuthorizer,
 
-		fpPermissions: authorization.NewPermissionsClient(env.Environment(), subscriptionDoc.ID, fpAuthorizer),
+		log: log,
 	}
 }
 
@@ -80,14 +77,13 @@ func NewOpenShiftClusterSlimDynamicValidator(log *logrus.Entry, clientID string,
 // and can operate within cluster and firstParty service principals.
 // It includes openShiftClusterSlimDynamicValidator
 type openShiftClusterFullDynamicValidator struct {
-	*openShiftClusterSlimDynamicValidator
 	env env.Interface
 
 	oc              *api.OpenShiftCluster
 	subscriptionDoc *api.SubscriptionDocument
 	fpAuthorizer    refreshable.Authorizer
 
-	fpPermissions authorization.PermissionsClient
+	log *logrus.Entry
 }
 
 // openShiftClusterSlimDynamicValidator is dynamic validator used inside clusters
@@ -100,10 +96,6 @@ type openShiftClusterSlimDynamicValidator struct {
 	tenantID        string
 	subscriptionID  string
 	environmentName string
-
-	spPermissions     authorization.PermissionsClient
-	spProviders       features.ProvidersClient
-	spVirtualNetworks network.VirtualNetworksClient
 }
 
 // Dynamic validates an OpenShift cluster in the context of cluster
@@ -117,16 +109,14 @@ func (dv *openShiftClusterSlimDynamicValidator) Dynamic(ctx context.Context) err
 		return err
 	}
 
-	dv.spPermissions = authorization.NewPermissionsClient(&azureEnv, dv.subscriptionID, spAuthorizer)
-	dv.spProviders = features.NewProvidersClient(&azureEnv, dv.subscriptionID, spAuthorizer)
-	dv.spVirtualNetworks = network.NewVirtualNetworksClient(&azureEnv, dv.subscriptionID, spAuthorizer)
+	spPermissions := authorization.NewPermissionsClient(&azureEnv, dv.subscriptionID, spAuthorizer)
 
 	vnetr, err := azure.ParseResourceID(dv.clusterSpec.VNetID)
 	if err != nil {
 		return err
 	}
 
-	err = validateVnetPermissions(ctx, dv.log, spAuthorizer, dv.spPermissions, dv.clusterSpec.VNetID, &vnetr, api.CloudErrorCodeInvalidServicePrincipalPermissions, "provided service principal")
+	err = validateVnetPermissions(ctx, dv.log, spAuthorizer, spPermissions, dv.clusterSpec.VNetID, &vnetr, api.CloudErrorCodeInvalidServicePrincipalPermissions, "provided service principal")
 	if err != nil {
 		return err
 	}
@@ -145,14 +135,15 @@ func (dv *openShiftClusterFullDynamicValidator) Dynamic(ctx context.Context) err
 		return err
 	}
 
+	fpPermissions := authorization.NewPermissionsClient(dv.env.Environment(), dv.subscriptionDoc.ID, dv.fpAuthorizer)
 	spAuthorizer, err := validateServicePrincipalProfile(ctx, dv.log, &dv.oc.Properties.ServicePrincipalProfile, dv.subscriptionDoc.Subscription.Properties.TenantID, dv.env.Environment().ResourceManagerEndpoint)
 	if err != nil {
 		return err
 	}
 
-	dv.spPermissions = authorization.NewPermissionsClient(dv.env.Environment(), r.SubscriptionID, spAuthorizer)
-	dv.spProviders = features.NewProvidersClient(dv.env.Environment(), r.SubscriptionID, spAuthorizer)
-	dv.spVirtualNetworks = network.NewVirtualNetworksClient(dv.env.Environment(), r.SubscriptionID, spAuthorizer)
+	spPermissions := authorization.NewPermissionsClient(dv.env.Environment(), r.SubscriptionID, spAuthorizer)
+	spProviders := features.NewProvidersClient(dv.env.Environment(), r.SubscriptionID, spAuthorizer)
+	spVirtualNetworks := network.NewVirtualNetworksClient(dv.env.Environment(), r.SubscriptionID, spAuthorizer)
 
 	vnetID, _, err := subnet.Split(dv.oc.Properties.MasterProfile.SubnetID)
 	if err != nil {
@@ -164,28 +155,28 @@ func (dv *openShiftClusterFullDynamicValidator) Dynamic(ctx context.Context) err
 		return err
 	}
 
-	err = validateVnetPermissions(ctx, dv.log, spAuthorizer, dv.spPermissions, vnetID, &vnetr, api.CloudErrorCodeInvalidServicePrincipalPermissions, "provided service principal")
+	err = validateVnetPermissions(ctx, dv.log, spAuthorizer, spPermissions, vnetID, &vnetr, api.CloudErrorCodeInvalidServicePrincipalPermissions, "provided service principal")
 	if err != nil {
 		return err
 	}
 
-	err = validateVnetPermissions(ctx, dv.log, dv.fpAuthorizer, dv.fpPermissions, vnetID, &vnetr, api.CloudErrorCodeInvalidResourceProviderPermissions, "resource provider")
+	err = validateVnetPermissions(ctx, dv.log, dv.fpAuthorizer, fpPermissions, vnetID, &vnetr, api.CloudErrorCodeInvalidResourceProviderPermissions, "resource provider")
 	if err != nil {
 		return err
 	}
 
 	// Get after validating permissions
-	vnet, err := dv.spVirtualNetworks.Get(ctx, vnetr.ResourceGroup, vnetr.ResourceName, "")
+	vnet, err := spVirtualNetworks.Get(ctx, vnetr.ResourceGroup, vnetr.ResourceName, "")
 	if err != nil {
 		return err
 	}
 
-	err = validateRouteTablePermissions(ctx, dv.log, dv.oc, spAuthorizer, dv.spPermissions, &vnet, api.CloudErrorCodeInvalidServicePrincipalPermissions, "provided service principal")
+	err = validateRouteTablePermissions(ctx, dv.log, dv.oc, spAuthorizer, spPermissions, &vnet, api.CloudErrorCodeInvalidServicePrincipalPermissions, "provided service principal")
 	if err != nil {
 		return err
 	}
 
-	err = validateRouteTablePermissions(ctx, dv.log, dv.oc, dv.fpAuthorizer, dv.fpPermissions, &vnet, api.CloudErrorCodeInvalidResourceProviderPermissions, "resource provider")
+	err = validateRouteTablePermissions(ctx, dv.log, dv.oc, dv.fpAuthorizer, fpPermissions, &vnet, api.CloudErrorCodeInvalidResourceProviderPermissions, "resource provider")
 	if err != nil {
 		return err
 	}
@@ -195,7 +186,7 @@ func (dv *openShiftClusterFullDynamicValidator) Dynamic(ctx context.Context) err
 		return err
 	}
 
-	err = validateProviders(ctx, dv.log, dv.spProviders)
+	err = validateProviders(ctx, dv.log, spProviders)
 	if err != nil {
 		return err
 	}

[mjudeikis]

@bennerv I tempted to merge this and leave this change for you to do in your PR? Sounds good?

[bennerv]

@mjudeikis Sounds good.

[mjudeikis]

/azp run e2e

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[olga-mir]

/lgtm

[github-actions]

Please rebase pull request.