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

[core-rest-pipeline] expose tokenCycler #15294

Closed
wants to merge 1 commit into from

Conversation

jeremymeng
Copy link
Member

So that other libraries can re-use its auto-refreshing mechanism.

So that other libraries can re-use its auto-refreshing mechanism.
@jeremymeng
Copy link
Member Author

Opening PR for discussion. I don't like to expose these from core-rest-pipeline and prefer keeping a copy for ACR now until more libraries have similar need.

  • put them into core-util?
    • Not good either as we don't want core-rest-pipeline to have more dependencies.
  • put the generic version in core-util when there are more need?

@jeremymeng jeremymeng added Azure.Core Client This issue points to a problem in the data-plane of the library. labels May 14, 2021
@xirzec
Copy link
Member

xirzec commented May 17, 2021

So I definitely think cycler shouldn't be in core-rest-pipeline, especially since we already have customers (i.e. the Portal Team) that are telling us our bundle sizes are too big.

I wonder if bearerTokenChallengeAuthenticationPolicy shouldn't be in core-rest-pipeline either, which would solve the trouble of duplication. Maybe we need core-auth-policies?

@witemple-msft
Copy link
Member

Just as a word of caution, the reason the token cycler exists in the first place is that we had a lot of difficulty modifying the original OOP bearer token policy because it was public, and so I rewrote the policy implementation from scratch. If there's a need to refactor the token cycler into a more robust set of abstractions and move it into a separate package, then we can do that, but it will mean committing to the design.

CC @sadasant

@witemple-msft
Copy link
Member

So I definitely think cycler shouldn't be in core-rest-pipeline, especially since we already have customers (i.e. the Portal Team) that are telling us our bundle sizes are too big.

@xirzec If it's used by policies in core-rest-pipeline (like bearerTokenAuthenticationPolicy), then I wouldn't think we'll save anyone any bytes by moving it to a separate package. A separate policies module could work, but wouldn't that already be breaking?

I'll also add that there's an open PR with some proposed changes to the cycler: #15120

@jeremymeng
Copy link
Member Author

I wonder if bearerTokenChallengeAuthenticationPolicy shouldn't be in core-rest-pipeline either, which would solve the trouble of duplication. Maybe we need core-auth-policies?

The ACR specific refresh token caching also needs which is why I have a slightly different copy of cycler in ACR.

It is also planned to merge bearerTokenChallengeAuthenticationPolicy back to bearTokenAuthenticationPolicy (correct me if I am wrong @sadasant)

@ramya-rao-a
Copy link
Contributor

@jeremymeng Is this still applicable?

@jeremymeng
Copy link
Member Author

@jeremymeng Is this still applicable?

An issue is probably better place for this discussion. Logged #15678

@jeremymeng jeremymeng closed this Jun 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants