-
Notifications
You must be signed in to change notification settings - Fork 191
ACR_token_changes #4506
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
base: master
Are you sure you want to change the base?
ACR_token_changes #4506
Conversation
mociarain
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The diff here seems to include a bunch of commits that are already merged. I think you have brought commits from master into this instead of rebasing. Is this possible?
17ec250 to
8b9f5de
Compare
I’ve now rebased ARO-22665-ACR_token_minting on top of the latest master and force-pushed, so the PR should now only show the ACR token minting changes |
mrWinston
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! just one small naming issue.
pkg/cluster/cluster.go
Outdated
| return nil, err | ||
| } | ||
|
|
||
| msiAuthorizer := azidext.NewTokenCredentialAdapter( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use env.NewMSIAuthorizer()
|
How are we removing the role assignments against the ACR to the 1p application? |
Thanks, @bennerv Once that validation is complete, we’ll handle the cleanup of the old role assignments as a separate step. Tagging @mrWinston here as well to confirm we're aligned. |
@microsoft-github-policy-service agree [company="Red Hat"] |
@microsoft-github-policy-service agree company="Red Hat" |
Which issue this PR addresses:
https://issues.redhat.com/browse/ARO-22665
What this PR does / why we need it:
We need to use the RP's managed identity credential instead of the 1p credential for ACR token minting in order to fix INT env
How do you know this will function as expected in production?
Its there to fix INT env.