Skip to content

Cache effective policy in endpoint metadata #43124

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 25 commits into from
Aug 15, 2022
Merged

Cache effective policy in endpoint metadata #43124

merged 25 commits into from
Aug 15, 2022

Conversation

@HaoK
Copy link
Member

@HaoK HaoK commented Aug 5, 2022 •
edited by davidfowl
Loading

Perf followup to #39840

This adds a metadata cache so we cache the computed policies per endpoint

Fixes #43227

@HaoK HaoK requested review from a team, DamianEdwards, Tratcher and davidfowl August 5, 2022 23:25
@ghost ghost added the area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer label Aug 5, 2022
halter73
halter73 reviewed Aug 5, 2022
View reviewed changes
Kahbazi
Kahbazi reviewed Aug 6, 2022
View reviewed changes
@HaoK HaoK added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Aug 8, 2022
davidfowl
davidfowl reviewed Aug 10, 2022
View reviewed changes
@HaoK HaoK requested a review from captainsafia August 11, 2022 18:18
@HaoK
Copy link
Member Author

HaoK commented Aug 11, 2022

I'll send out an email API review for this now

@HaoK HaoK added api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Aug 11, 2022
@ghost
Copy link

ghost commented Aug 11, 2022

Thank you for your API proposal. I'm removing the api-ready-for-review label. API Proposals should be submitted for review through Issues based on this template.

@ghost ghost removed the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Aug 11, 2022
@HaoK HaoK added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Aug 11, 2022
@halter73 halter73 mentioned this pull request Aug 12, 2022
Closed
@HaoK
Copy link
Member Author

HaoK commented Aug 12, 2022

Updated to WithAuthorizationPolicyCache and IVT for the metadata per #43227

Kahbazi
Kahbazi reviewed Aug 12, 2022
View reviewed changes
src/Security/Authorization/Policy/src/AuthorizationEndpointConventionBuilderExtensions.cs Outdated
Copy link
Member

@Kahbazi Kahbazi Aug 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about to only add AuthorizationPolicyCache if there are IAuthorizeData or AuthorizationPolicy is available in the metadata. Also the new Finally method could be used instead of Add.

davidfowl
davidfowl reviewed Aug 15, 2022
View reviewed changes
davidfowl
davidfowl reviewed Aug 15, 2022
View reviewed changes
davidfowl
davidfowl reviewed Aug 15, 2022
View reviewed changes
src/Security/Authorization/Policy/src/AuthorizationMiddleware.cs Outdated
// Cache the computed policy
if (policy != null && canCachePolicy)
{
_policyCache!.Value![endpoint!] = policy;
Copy link
Member

@davidfowl davidfowl Aug 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the symbols here are funny 😄

Copy link
Member Author

@HaoK HaoK Aug 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah the cache code is very excited!!!

davidfowl
davidfowl reviewed Aug 15, 2022
View reviewed changes
davidfowl
davidfowl reviewed Aug 15, 2022
View reviewed changes
davidfowl
davidfowl reviewed Aug 15, 2022
View reviewed changes
davidfowl
davidfowl reviewed Aug 15, 2022
View reviewed changes
davidfowl
davidfowl reviewed Aug 15, 2022
View reviewed changes
src/Security/Authorization/Policy/src/AuthorizationPolicyCache.cs Outdated

public void Store(Endpoint endpoint, AuthorizationPolicy policy)
{
_policyCache!.Value![endpoint!] = policy;
Copy link
Member

@davidfowl davidfowl Aug 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No more null checks!

Suggested change
_policyCache!.Value![endpoint!] = policy;
_policyCache.Value[endpoint] = policy;

Copy link
Member Author

@HaoK HaoK Aug 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Value can still be null I think or at least vs says so

davidfowl
davidfowl reviewed Aug 15, 2022
View reviewed changes
JamesNK
JamesNK reviewed Aug 15, 2022
View reviewed changes
JamesNK
JamesNK reviewed Aug 15, 2022
View reviewed changes
JamesNK
JamesNK reviewed Aug 15, 2022
View reviewed changes
@HaoK HaoK enabled auto-merge (squash) August 15, 2022 08:48
@HaoK HaoK merged commit 525705f into main Aug 15, 2022
@HaoK HaoK deleted the haok/authzPerf branch August 15, 2022 10:29
@ghost ghost added this to the 7.0-rc1 milestone Aug 15, 2022
@HaoK HaoK mentioned this pull request Aug 15, 2022
Closed
@davidfowl davidfowl added the Perf label Aug 26, 2022
@HaoK HaoK mentioned this pull request Oct 17, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@halter73 halter73 halter73 left review comments

@davidfowl davidfowl davidfowl left review comments

@JamesNK JamesNK JamesNK approved these changes

@DamianEdwards DamianEdwards Awaiting requested review from DamianEdwards

@Tratcher Tratcher Awaiting requested review from Tratcher

@captainsafia captainsafia Awaiting requested review from captainsafia

+1 more reviewer

@Kahbazi Kahbazi Kahbazi left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

api-suggestion Early API idea and discussion, it is NOT ready for implementation area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer Perf

Projects

None yet

Milestone

7.0-rc1

Development

Successfully merging this pull request may close these issues.

Add AuthorizationPolicyCache

6 participants

@HaoK @halter73 @davidfowl @JamesNK @Kahbazi