Conversation
aichanda-png
requested review from
bennerv,
cadenmarchese,
fahlmant,
hawkowl,
hlipsig,
jharrington22,
kimorris27,
mociarain,
mrWinston,
pepedocs,
rogbas,
sankur-codes,
tiguelu,
tsatam,
wanghaoran1988 and
yjst2012
as code owners
mociarain
left a comment
mociarain
left a comment
There was a problem hiding this comment.
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?
aichanda-png
force-pushed
the
ARO-22665-ACR_token_minting
branch
from
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
mrWinston
left a comment
There was a problem hiding this comment.
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.
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" |
tuxerrante
requested review from
tuxerrante
and removed request for
fahlmant and
pepedocs
tuxerrante
left a comment
tuxerrante
left a comment
There was a problem hiding this comment.
The file pkg/cluster/delete.go:612 still uses m.localFpAuthorizer for ACR token deletion:
acrManager, err := acrtoken.NewManager(m.env, m.localFpAuthorizer)
This should likely also be changed to m.rpMIAuthorizer for consistency and to ensure deletion works with the new credential.
Is that right?
There was a problem hiding this comment.
Could you please add unit tests?
| ] | ||
| }, | ||
| { | ||
| "name": "[concat(substring(parameters('acrResourceId'), add(lastIndexOf(parameters('acrResourceId'), '/'), 1)), '/', '/Microsoft.Authorization/', guid(concat(parameters('acrResourceId'), 'RP / ARO v4 ContainerRegistry Token Contributor')))]", |
There was a problem hiding this comment.
Is that right to have consequent '/' in the names?
... , '/', '/Microsoft ...
|
This PR has been superseded by #4582 |
6 participants
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.