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

azcontainerregistry: delegate token caching #23272

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

Conversation

stevekuznetsov
Copy link
Contributor

azcontainerregistry: decouple token exchange from request specifics

I would like to propose a generally-useful token cache in a future
commit. Managing the refresh tokens and exchanging them for access
tokens does not need to rely specifically on the authentication policy
or the specific client request being made, so this commit decouples the
two concerns, thereby enabling the future refactor to be simpler.

Signed-off-by: Steve Kuznetsov [email protected]


azcontainerregistry: delegate token caching

Mechanically refactor token caching, retrieval and exchange behavior
from the authentication policy into a dedicated object, allowing us to
separate token management concerns from Azure client policy concerns.

Signed-off-by: Steve Kuznetsov [email protected]


azcontainerregistry: move code

This commit simply moves code from one location to another, broken out
from other commits for ease of review.

Signed-off-by: Steve Kuznetsov [email protected]


Builds on top of #23270

Proving out a separation of concerns that could solve #20571

@github-actions github-actions bot added Community Contribution Community members are working on the issue customer-reported Issues that are reported by GitHub users external to the Azure organization. labels Aug 2, 2024
Copy link

github-actions bot commented Aug 2, 2024

Thank you for your contribution @stevekuznetsov! We will review the pull request and get back to you soon.

@chlowell
Copy link
Member

chlowell commented Aug 2, 2024

I don't see anything wrong with this, however I also don't see a reason to make these changes, because they don't affect client behavior or the module's public API. Can you please describe the scenario motivating this? I take it you want to replace the ACR token cache of a BlobClient or Client instance with a custom implementation. Why is the current internal caching function not sufficient?

@stevekuznetsov
Copy link
Contributor Author

stevekuznetsov commented Aug 5, 2024

@chlowell were you aware of the conversation happening on #20571? Sorry if I was not clear - I 100% agree with you that the changes here as they are will not be user-visible, but their value comes from the context of that issue, specifically what I laid out in this comment.

I would like to propose a generally-useful token cache in a future
commit. Managing the refresh tokens and exchanging them for access
tokens does not need to rely specifically on the authentication policy
or the specific client request being made, so this commit decouples the
two concerns, thereby enabling the future refactor to be simpler.

Signed-off-by: Steve Kuznetsov <[email protected]>
Mechanically refactor token caching, retrieval and exchange behavior
from the authentication policy into a dedicated object, allowing us to
separate token management concerns from Azure client policy concerns.

Signed-off-by: Steve Kuznetsov <[email protected]>
This commit simply moves code from one location to another, broken out
from other commits for ease of review.

Signed-off-by: Steve Kuznetsov <[email protected]>
@stevekuznetsov stevekuznetsov force-pushed the skuznets/authentication-token-cache branch from 86a8f8d to 308795f Compare August 6, 2024 13:18
@stevekuznetsov
Copy link
Contributor Author

Rebased after pre-requisite PR merged.

Copy link

Hi @stevekuznetsov. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days.

@github-actions github-actions bot added the no-recent-activity There has been no recent activity on this issue. label Oct 11, 2024
@stevekuznetsov
Copy link
Contributor Author

Would be great to do it but not sure if maintainers want this feature.

@github-actions github-actions bot removed the no-recent-activity There has been no recent activity on this issue. label Oct 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community Contribution Community members are working on the issue customer-reported Issues that are reported by GitHub users external to the Azure organization.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants