feat: add ARO-HCP Clusters Managed Identities existence cluster validation - #3820
Conversation
| return "azure-hcp-cluster-mis-existence-validation" | ||
| } | ||
|
|
||
| func (v *AzureHCPClusterMIsExistenceValidation) Validate(ctx context.Context, clusterSubscription *arm.Subscription, cluster *api.HCPOpenShiftCluster) error { |
There was a problem hiding this comment.
so far it is described as, these creds are used to
- do validation here: just don't the validation
- these creds are used pass to hypershift: write two controllers, one controller for SMI, one controller for not-service managed identity.
- skip all validation when SMI isn't available.
There was a problem hiding this comment.
do validation here: just don't the validation
I consider the validation to still be useful as it validates that the control plane and data plane operators MIs are there even in lower environments. The part that is not accurate of the validation is the authentication with the SMI but all the other parts are still exercised. It focuses it on validating as much as we can in the lower environments, even if not 100% accurate, to get faster feedback and ability to try changes faster.
| // TODO should we reuse FPAClientBuilder interface for this? It is a bit | ||
| // special because it is a service that is not part of azure go sdk and | ||
| // only available in some environments. | ||
| type FPAMIDataplaneClientBuilder interface { |
There was a problem hiding this comment.
It's not obvious to me why we stub here versus UserAssignedIdentitiesClient level. The small amount of code in func UserAssignedIdentitiesClient that is reused appears to add more complication than value. Is the func MIDataplane called somewhere else and used directly?
There was a problem hiding this comment.
Is the func MIDataplane called somewhere else and used directly?
Yes. During the cluster provisioning process the MIDataplane function will for example be called directly to get a Dataplane client to retrieve the initial set of short-lived credentials associated to the provided cluster's control plane operators identities and then store them in the "managed identities" key vault, which is then accessed from the management cluster side.
It will also be used to instantiate other clients like for example Subnets client with the SMI (or msi mock in the case of lower environments)
| // TODO should we reuse FPAClientBuilder interface for this? It is a bit | ||
| // special because it is a service that is not part of azure go sdk and | ||
| // only available in some environments. | ||
| type FPAMIDataplaneClientBuilder interface { |
There was a problem hiding this comment.
Adding some notes/thoughts here: This MR introduces the FPAMIDataplaneClientBuilder interface as the entrypoint to get the MIDataplane client. An alternative I considered before was to modify the FPAClientBuilder interface to have the MIDataplaneClient as a method. However, the implication of that would be that now the constructor of it had to receive:
- the serviceTenantID string
- the audience string
- And then it would either need to receive a FPAMIDataplaneClientBuilder anyway so it can leverage it internally, or instead receive some attribute which would be something like "useHardcodedIdentityDataplaneClient" boolean and then the logic would have branching to select the client to create.
Initially it seemed different enough (different attributes needed during construction, using a MS service that is not the real one always, ...) that I opted to have it as a different interface here.
affdf24 to
e6fb107
Compare
f753869 to
3dd70da
Compare
| // FPAMIDataplaneClientBuilder that allows to retrieve Managed Identities Data Plane clients | ||
| // based on the hardcoded identity implementation of the Managed Identities Data Plane client | ||
| // hardcodedIdentityManagedIdentitiesDataplaneClient. | ||
| func NewHardcodedIdentityFPAMIDataplaneClientBuilder(cloudConfiguration *cloud.Configuration, hardcodedIdentity *HardcodedIdentity) FPAMIDataplaneClientBuilder { |
There was a problem hiding this comment.
I tried to not abbreviate most of the type definitions but some like this ended up being way too large so I preferred to keep abbreviations in some places. For example, skipping abbreviation on this would end up being something like NewHardcodedIdentityFirstPartyApplicationManagedIdentitiesDataplaneClientBuilder which was 80 characters in itself. It seemed to be less readable that keeping the abbreviations at that point, as well as if we need to define attributes that reference the type of the name that also ended up with very long lines.
3dd70da to
ddfe2c8
Compare
|
I'm not fully convinced that we need a factory to produce a builder to produce a client versus simply requesting the client we want from one entity. Notice the usage smiClientBuilder := v.smiClientBuilderFactory.NewServiceManagedIdentityClientBuilder(clusterIdentityURL, smiResourceID)
// We check the existence of the Cluster's Service Managed Identity by
// attempting to retrieve the user assigned identities client using the
// service managed identity's identity credentials, which we obtain by
// requesting them via the Managed Identities Data Plane Service. If the
// service managed identity does not exist the request will fail.
uaisClient, err := smiClientBuilder.UserAssignedIdentitiesClient(ctx, cluster.ID.SubscriptionID)we could just do smiClientBuilder := v.smiClientBuilderFactory.UserAssignedIdentitiesClient(clusterIdentityURL, smiResourceID, cluster.ID.SubscriptionID)why have the two distinct structs? |
The original thinking of it was that multiple clients based on that identity will be retrieved within a specific code area. The information to specify what identity you want to use is {cluster identity url, smiresourceid}, which is only needed once when you have the identity information available. In that way, you first instantiate the first type providing {cluster identity url, smiresourceid} and then you retrieve the N clients from it using the other type, where the methods to retrieve the clients do not have {cluster identity url, smiresourceid} repeated for each one of them, as all of them need that information. If you want to pass around the ability to create the N clients then you pass the second type instead of having to pass the type + {cluster identity url, smiresourceid}. The consequence of removing the intermediate type is:
I added a new commit on top of the current one that removes the intermediate type just in case we want to go towards that approach after discussing it. |
0f2aca0 to
7a2f8f6
Compare
7a2f8f6 to
207b075
Compare
|
/lgtm |
Manyanda Chitimbo (machi1990)
left a comment
Manyanda Chitimbo (machi1990)
left a comment
There was a problem hiding this comment.
/lgtm
…ation This commit also introduces the ability to authenticate as the cluster's service managed identity, which is required to be able to instantiate a user-assigned identities client that uses it, which has enough permissions to check the existence of the cluster-scoped user-provided azure managed identities associated to the cluster's operators. The service managed identity (SMI) is a cluster-scoped azure user-assigned managed identity. This identity is used to interact with Azure resources that are created and provided by the end-user. To get be able to authenticate as the cluster's managed identity, credentials need to be retrieved for them. To achieve that, this commit also introduces the ability to interact with Microsoft's Managed Identities Data Plane service. This service is only available in environments where Microsoft's First Party Application integration is available. For the environments where the First Party Application integration is not available we cannot communicate with the Managed Identities Data Plane service, so instead we use a mock implementation of the ManagedIdentitiesDataplaneClient that always returns a single Azure Service Principal identity representing a Managed Identity. This commit also introduces this mock client implementation.
…only We now require passing the cluster identity url and the smi resource id in each call of the smiclientbuilder method.
207b075 to
1892906
Compare
|
The last remaining PR that blocked this PR has been recently been merged (#4244). As an important note, the actual full functionality won't be exercised until this is deployed to aro-hcp stage and higher. This is because it relies on having a real FPA identity, which is only available in ARO-HCP Stage and Production. |
|
I'll check the e2e run outcome before removing the hold |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, machi1990, miguelsorianod The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
|
I checked the e2e execution and I see occurrences of the validation containing the condition of the AzureClusterManagedIdentitiesExistenceValidation cluster validation set to Succeeded true: I also see the controller itself marked as non degraded: |
|
/hold cancel Cancelling hold. The first time we will be able to verify the actual communication with MI Dataplane will be in ARO-HCP stage. |
|
/retest |
4 participants
This MR builds on top of #3805 in a new commit.
This MR introduces the logic to validate that the cluster-scoped user-provided azure managed identities exist beforehand.
The MR is also used to introduce the ability to authenticate as the cluster's service managed identity, which is required to be able to instantiate a user-assigned identities client with enough permissions to check the existence of the cluster-scoped user-provided azure managed identities.
The "service managed identity" (SMI) is an ARO-HCP cluster-scoped azure user-assigned managed identity. This identity is used to interact with Azure resources that are created/provided by the end-user:
For example, the SMI associated to the cluster is used to validate that the control plane and data plane operators provided by the user by specifying their resource ids during creation exist in Azure. Another example is that it is used to validate that the Cluster's subnet provided by the user by specifying its resource id during creation exists in Azure, as well as its location being the same as the location of the cluster to be created. Another example is that it is used to obtain a token credential associated to it which is passed as parameters of API calls of Microsoft's checkaccessv2 service.
The mechanism that we use in ARO-HCP to retrieve short-lived credentials associated to a user-assigned managed identity is by leveraging Microsoft's Managed Identities Data plane (MI DP). The MI DP service is not available in all ARO-HCP environments.
Although this MR is focused on the service managed identity, there are other scenarios where e need to retrieve short-lived credentials associated to different user-assigned managed identities. Particularly, we retrieve credentials for all the control plane operators identities provided during aro-hcp cluster creation, which we end up storing in Azure Key Vault. Then, during the cluster provisioning process, on the management cluster side those credentials are retrieved from Key Vault and end up being leveraged by the corresponding aro-hcp cluster control plane operators to authenticate against the Azure API to perform their duties. As mentioned, the credentials are short-lived. Those are then periodically refreshed by a different component called "msi credentials refresher" which takes care of updating the contents in Key Vault.
Due to the MI Dataplane service is not available in all ARO-HCP environments, it means we cannot retrieve credentials associated to the service managed identity nor the control plane operators identities. At the same time, the control plane operators still need to be able to authenticate against Azure to perform their duties. In those limited environments what we do instead is that we use a single Azure Service Principal (not a Managed Identity, as we cannot get credentials associated to it) for all of them, and among all clusters, instead. We commonly refer to this identity as the "msi (or mi) mock identity" (not to be confused with the service managed identity (smi)). The same applies to the service managed identity, which is leveraged by CS/RP. This MR introduces the ability to instantiate a "stub" MI Dataplane client that always returns the credentials and information associated to this identity. This "stub" MI Dataplane client does not perform any API call and it returns the same msi mock identity credentials and information for each requested managed identity resource id.
The msi mock identity is created by Red Hat in the ARO-HCP environments where the Managed Identities Dataplane service is not avaialble
The flow for controlplane operators identities in the environments where the MI Dataplane service is available is:
In the environments where the MI Dataplane service does not exist, we do not interact with it and instead we simulate the responses it would return but returning a single service principal identity information as well as long-lived client certificate credentials associated to it. We then store that in key vault and we create the hostedcluster cr using the same client id, ignoring the user-provided managed identities information, as it cannot be leveraged. In those environments the msi credentials refresher is not executed.
Although in the limited environments the managed identities cannot be leveraged, we still share the validation logic codepaths and we try to validate as much as we can. For example, we still check that the permissions associated to them have been configured correctly, or that other user-provided resources like the cluster's subnet are also valid. The same occurs for the HostedCluster CR creation.