Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ARO-13402 | Adding Identity field to the HCPOpenShiftCluster #1028

Closed
wants to merge 1 commit into from

Conversation

venkateshsredhat
Copy link
Collaborator

What this PR does

The frontend needs to parse and validate the top-level ARM-defined "identity" resource field to fully support managed identities.

Jira: https://issues.redhat.com/browse/ARO-13402
Link to demo recording:

Special notes for your reviewer

The ticket says Visibility tags are TBD so have left them blank .

@venkateshsredhat
Copy link
Collaborator Author

venkateshsredhat commented Jan 2, 2025

@microsoft-github-policy-service agree company="Red Hat"

@miguelsorianod
Copy link
Collaborator

I see that the MR are related to changes in the cluster's frontend payload side.

Is the logic to send that information to the CS payload already performed and we are only missing the parsing from the frontend side? or are we missing there something?

I am asking this because we intended to rollout CS side changes that will make the managed identities required at API level, and we need to have the frontend side changes related to sending that information to CS implemented. If I understand correctly without the changes in this MR that wouldn't be possible, so we need to wait until this MR is merged. Let me know if this understanding is not accurate.

@miguelsorianod
Copy link
Collaborator

If I understand correctly without the changes in this MR that wouldn't be possible, so we need to wait until this MR is merged. Let me know if this understanding is not accurate.

I think this part might not be accurate. Although the changes on this MR seem to be missing, they might not be strictly required for the CS side payload requirements.

@mbarnes
Copy link
Collaborator

mbarnes commented Jan 2, 2025

Is the logic to send that information to the CS payload already performed

Yes, that was added in #977 but with no validation on the frontend side. Just a blind pass-through.

This PR is fulfilling Azure requirements for how ARM passes managed identities to all resource providers. After this merges we'll need validation logic that pairs each MI here with the operator-specific MI fields in #977.

Copy link
Collaborator

@mbarnes mbarnes left a comment

Choose a reason for hiding this comment

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

Looking great so far.

FYI, the GET logic for this structure is going to be messy. On GET, each user-assigned identity has to include ClientId and PrincipalId properties. But that information comes from Cluster Service by way of its operator-specific managed identity properties. So we'll have to construct the identity.userAssignedIdentities section from data returned by Cluster Service (in /frontend/pkg/frontend/ocm.go). I'm cool with deferring that messiness to another PR if you'd rather just get the structs defined here.

Copy link
Collaborator

@SudoBrendan SudoBrendan left a comment

Choose a reason for hiding this comment

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

Looking good so far - LMK when you're ready to take this out of draft :)

@venkateshsredhat
Copy link
Collaborator Author

venkateshsredhat commented Jan 3, 2025

Looking great so far.

FYI, the GET logic for this structure is going to be messy. On GET, each user-assigned identity has to include ClientId and PrincipalId properties. But that information comes from Cluster Service by way of its operator-specific managed identity properties. So we'll have to construct the identity.userAssignedIdentities section from data returned by Cluster Service (in /frontend/pkg/frontend/ocm.go). I'm cool with deferring that messiness to another PR if you'd rather just get the structs defined here.

okay sure , since this PR was raised from my forked branch due to permission issue . I will raise a new PR for this and for ClientId and principalId properties will raise a separate one .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants