Skip to content

[WIP] feat: add controller that calculates Cluster Data Plane Identities extra information - #4193

Closed
Miguel Soriano (miguelsorianod) wants to merge 1 commit into
mainfrom
msoriano-fetchinfo-dataplaneoperators-mis
Closed

[WIP] feat: add controller that calculates Cluster Data Plane Identities extra information#4193
Miguel Soriano (miguelsorianod) wants to merge 1 commit into
mainfrom
msoriano-fetchinfo-dataplaneoperators-mis

Conversation

@miguelsorianod

@miguelsorianod Miguel Soriano (miguelsorianod) commented Feb 24, 2026

Copy link
Copy Markdown
Collaborator

We add a controller that retrieves the Client ID and Principal ID associated to the Data Plane operators identities associated to the ARO-HCP Cluster.

We leverage the Service Managed Identity associated to the ARO-HCP Cluster to retrieve the Data Plane operators identities information. We use Azure Go SDK's UserAssignedIdentities API to retrieve it. This is a different method than what's done for MSI based identities where the Managed Identities Data Plane service is used instead (#3820). This is because for the control plane identities. on the environments where the managed identities data plane service is not available we use the mi mock identity instead, which includes its clientid+principalid instead of the ones associated to the identities passed in the cluster payload. By using the mock managed identities data plane client we retrieve that transparently.

@openshift-ci

openshift-ci Bot commented Feb 24, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: miguelsorianod
Once this PR has been reviewed and has the lgtm label, please assign mmazur for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

continue
}

if len(currentMI.ClientID) == 0 || len(currentMI.PrincipalID) == 0 || len(currentMI.OperatorName) == 0 {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For when we allow updates of MI; we'll need to also sync the identities if currentMI info != newUpdatedInfo.

@miguelsorianod
Miguel Soriano (miguelsorianod) force-pushed the msoriano-fetchinfo-dataplaneoperators-mis branch from 6bb6ada to 5fab4cf Compare March 18, 2026 11:53
@miguelsorianod
Miguel Soriano (miguelsorianod) force-pushed the msoriano-fetchinfo-dataplaneoperators-mis branch from 5fab4cf to 5caac0b Compare March 18, 2026 12:23
@openshift-ci

openshift-ci Bot commented Mar 22, 2026

Copy link
Copy Markdown

PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

}

smiResourceID := existingCluster.CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.ServiceManagedIdentity
uaisClient, err := c.smiClientBuilder.UserAssignedIdentitiesClient(ctx, existingCluster.ServiceProviderProperties.ManagedIdentitiesDataPlaneIdentityURL, smiResourceID, existingCluster.ID.SubscriptionID)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
uaisClient, err := c.smiClientBuilder.UserAssignedIdentitiesClient(ctx, existingCluster.ServiceProviderProperties.ManagedIdentitiesDataPlaneIdentityURL, smiResourceID, existingCluster.ID.SubscriptionID)
userAssignedIdentitiesClient, err := c.smiClientBuilder.UserAssignedIdentitiesClient(ctx, existingCluster.ServiceProviderProperties.ManagedIdentitiesDataPlaneIdentityURL, smiResourceID, existingCluster.ID.SubscriptionID)

don't be stingy.

existingServiceProviderCluster.Status.DataPlaneOperatorsManagedIdentities = make(map[string]*api.ServiceProviderClusterDataPlaneOperatorManagedIdentity)
}

for _, desired := range desiredDataPlaneOperatorsManagedIdentities {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as written, entries created here can never be cleared. An "add only" map smells wrong. What clears unnecessary or extra entries over time.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like we need to ensure taht serviceprovidercluster should have its entries trimmed of any that don't exist in existingCluster.CustomerProperties.Platform.OperatorsAuthentication.UserAssignedIdentities.DataPlaneOperators

type ServiceProviderClusterDataPlaneOperatorManagedIdentity struct {
OperatorName string `json:"operatorName"`
ResourceID *azcorearm.ResourceID `json:"resourceID"`
ClientID string `json:"clientID"`

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be missing. pointer.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am using the empty string as the missing indicator. In that way there are no three different sets of values to have to consider: nil, empty string, non empty string

OperatorName string `json:"operatorName"`
ResourceID *azcorearm.ResourceID `json:"resourceID"`
ClientID string `json:"clientID"`
PrincipalID string `json:"principalID"`

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be missing. pointer.

@openshift-ci

openshift-ci Bot commented Apr 16, 2026

Copy link
Copy Markdown

Miguel Soriano (@miguelsorianod): The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/images-push 5caac0b link true /test images-push

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@raelga
Rael Garcia (raelga) marked this pull request as draft April 30, 2026 13:42
@miguelsorianod

Copy link
Copy Markdown
Collaborator Author

Superseded by #6300

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants