Operator: Expose Cluster SP as Condition

[bennerv]

Work in progress for some early feedback

Which issue this PR addresses:

Fixes 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

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

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

$ 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

$ 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

# 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

[mjudeikis]

I think this is a good start, but we have some structural changes we need to address before we progress. kicked off POC PR how it could look like. Can you take a look and lets discuss this.

[github-actions]

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]

Please rebase pull request.

[nilsanderselde]

looks good, just some nitpicking

[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

[github-actions]

Please rebase pull request.

[github-actions]

Please rebase pull request.

[mjudeikis]

very close :)

[mjudeikis]

Now this call is not needed. Validator initiation with NewValidator is needed to instantiate clients. In this case we dont use clients as we do only ValidateServicePrincipals which gets token inside.

[mjudeikis]

All this could be like this:

// IMPORTANT: We provide authorizer = nil because operator service principal validation do not need any of the clients to be initiated inside validator. 
spDynamic, err := dynamic.NewValidator(r.log, &azEnv, resource.SubscriptionID, nil, dynamic.AuthorizerClusterServicePrincipal)
	if err != nil {
		return err
	}

[bennerv]

@mjudeikis these are two different endpoints (GraphEndpoint, ResourceManagerEndpoint) we're verifying against which is why there are two aad.GetToken calls: 1 within the validator, 1 within the sp checker.

Do we only need to validate against the graph endpoint now? I thought we needed both graph and resourcemanager endpoints as this is what we're doing in openshiftcluster_validatedynamic.go.

https://github.com/Azure/ARO-RP/blob/master/pkg/api/validate/openshiftcluster_validatedynamic.go#L73-L86

[github-actions]

Please rebase pull request.

[github-actions]

Please rebase pull request.

[mjudeikis]

Would be good to tide this up and merge :)

stale