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

Add identityURL to internal apis for CMSI usage #3514

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

cadenmarchese
Copy link
Collaborator

Which issue this PR addresses:

Fixes ARO-6086

What this PR does / why we need it:

The cluster doc needs to persist identityURL in order for Cluster MSI (CUAMSI) token refreshing to work. The identityURL is a header that is provided by ARM to the RP and is used for token refreshing purposes. For more info, see the design doc: https://docs.google.com/document/d/1dtgp6B-VYyXUmPsMX9f9MdlAE9sON4OuH1Cw9Ij0mmg/edit that @rajdeep wrote.

Test plan for issue:

API calls should succeed in persisting identityURL header when a put or patch call to RP is made.

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

How do you know this will function as expected in production?

@cadenmarchese cadenmarchese added hold Hold work-in-progress chainsaw Pull requests or issues owned by Team Chainsaw labels Apr 10, 2024
@rajdeepc2792
Copy link
Collaborator

Left a few comments, all other changes seems good for identityURL.

@cadenmarchese cadenmarchese changed the title Add identityURL to 2024-08-12-preview for CMSI usage Add identityURL to internal apis for CMSI usage Apr 11, 2024
@cadenmarchese cadenmarchese marked this pull request as ready for review April 11, 2024 15:22
@cadenmarchese
Copy link
Collaborator Author

I'm adding hold while we work on ARO-6623 which is required to resolve the open comments.

kimorris27
kimorris27 previously approved these changes May 9, 2024
@kimorris27
Copy link
Contributor

/azp run ci, e2e

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

s-amann
s-amann previously approved these changes May 16, 2024
Copy link
Contributor

@s-amann s-amann left a comment

Choose a reason for hiding this comment

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

minor nits, nothing critical. lgtm

pkg/frontend/openshiftcluster_putorpatch.go Outdated Show resolved Hide resolved
pkg/frontend/openshiftcluster_putorpatch.go Outdated Show resolved Hide resolved
pkg/frontend/openshiftcluster_putorpatch.go Outdated Show resolved Hide resolved
@cadenmarchese
Copy link
Collaborator Author

/azp run ci,e2e

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link
Collaborator

@rajdeepc2792 rajdeepc2792 left a comment

Choose a reason for hiding this comment

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

Commented the doubts, rather than any change request.
Other than the concerns, changes LGTM.

pkg/api/openshiftcluster.go Outdated Show resolved Hide resolved
pkg/api/admin/openshiftcluster.go Outdated Show resolved Hide resolved
Copy link
Contributor

@kimorris27 kimorris27 left a comment

Choose a reason for hiding this comment

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

Requesting one more small change for readability's sake.

pkg/frontend/openshiftcluster_putorpatch.go Outdated Show resolved Hide resolved
@kimorris27
Copy link
Contributor

/azp run ci, e2e

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chainsaw Pull requests or issues owned by Team Chainsaw ready-for-review size-small Size small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants