T keithyao/batch purge Adds ABAC support for purge command with batched token requests#577
T keithyao/batch purge Adds ABAC support for purge command with batched token requests#577keithy1012 wants to merge 12 commits intoAzure:mainfrom
Conversation
|
@keithy1012 please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
|
|
||
| // Request a token that covers all repositories in this batch | ||
| // The token scope will include: repository:repo1:pull repository:repo1:delete ... for each repo | ||
| if err := acrClient.RefreshTokenForAbac(ctx, batch); err != nil { |
There was a problem hiding this comment.
The token is only refreshed once at the start of the requests. This seems like it could be problematic as I presume these tokens time out. You should have some strategy to rotate this as time goes on.
There was a problem hiding this comment.
Now the token is refreshed per batch. In practice we want to generally avoid doing the refreshes upfront and just refresh as needed on the token acquisition portion. So for example if you look at the getManifest calls you will see we call refreshAcrCLIClientToken first on those. What you could do is modify that function to refresh the token you have if its necessary dynamically at that stage.
lizMSFT
left a comment
There was a problem hiding this comment.
You could follow https://github.com/Azure/acr-cli/tree/main?tab=readme-ov-file#installation to build the binary or check https://github.com/Azure/acr-cli/blob/main/scripts/experimental/README-purge-testing.md#1-test-purge-allsh-consolidated-test-suite and test the changes locally first.
estebanreyl
left a comment
There was a problem hiding this comment.
Looking good overall. I would recommend adding some test to the cli experimental tests as well. Just some minor things left to address
|
|
||
| // Request a token that covers all repositories in this batch | ||
| // The token scope will include: repository:repo1:pull repository:repo1:delete ... for each repo | ||
| if err := acrClient.RefreshTokenForAbac(ctx, batch); err != nil { |
There was a problem hiding this comment.
Now the token is refreshed per batch. In practice we want to generally avoid doing the refreshes upfront and just refresh as needed on the token acquisition portion. So for example if you look at the getManifest calls you will see we call refreshAcrCLIClientToken first on those. What you could do is modify that function to refresh the token you have if its necessary dynamically at that stage.
| // Process all repositories in this batch | ||
| for j, repoName := range batch { | ||
| // For ABAC registries, check if token expired and refresh for remaining repos in batch | ||
| if acrClient.IsAbac() && acrClient.IsTokenExpired() { |
There was a problem hiding this comment.
The token can fail at any moment, for example let's say there are a lot of images inside of a particular repository that need to be scanned. We could have the token expire mid iteration and thus that repo listing and exploration would fail. My suggestion would be to give the acquirer the ability to refresh itself, so for example I see that you are not storing the token anwyhere in this part of the code, instead its maintained inside of acrClient,
|
|
||
| // GetAcrTags list the tags of a repository with their attributes. | ||
| func (c *AcrCLIClient) GetAcrTags(ctx context.Context, repoName string, orderBy string, last string) (*acrapi.RepositoryTagsType, error) { | ||
| if c.isExpired() { |
There was a problem hiding this comment.
This block also needs to be updated. Otherwise, when GetAcrTags detects that the token has expired, it will obtain an RBAC access token instead. This also applies to DeleteAcrTag
Purpose of the PR
Fixes #