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

Refactoring & extensibility #63

Merged
merged 13 commits into from
Feb 21, 2019
Merged

Refactoring & extensibility #63

merged 13 commits into from
Feb 21, 2019

Conversation

cristipufu
Copy link
Collaborator

@cristipufu cristipufu commented Feb 17, 2019

This pull request refactors all the duplicate code and contains multiple fixes and extensibility improvements based on the community issues.


Quick summary:

The AsyncLock code is also used here and I used Ben.BlockingDetector to detect any blocking calls, so it should be just fine.

using (await AsyncLock.WriterLockAsync(counterId).ConfigureAwait(false))
{
        var entry = await _counterStore.GetAsync(counterId, cancellationToken);

        if (entry.HasValue)
        {
                // entry has not expired
                if (entry.Value.Timestamp + rule.PeriodTimespan.Value >= DateTime.UtcNow)
                {
                    // increment request count
                    var totalRequests = entry.Value.TotalRequests + 1;

                    // deep copy
                    counter = new RateLimitCounter
                    {
                        Timestamp = entry.Value.Timestamp,
                        TotalRequests = totalRequests
                    };
                }
        }

        // stores: id (string) - timestamp (datetime) - total_requests (long)
        await _counterStore.SetAsync(counterId, counter, rule.PeriodTimespan.Value, cancellationToken);
}
  • All the stores are now async (performance improvement for Redis-backed stores) - but now you have to manually seed the app settings client / IP policies.

https://andrewlock.net/running-async-tasks-on-app-startup-in-asp-net-core-part-2/

public static async Task Main(string[] args)
{
        IWebHost webHost = CreateWebHostBuilder(args).Build();

        using (var scope = webHost.Services.CreateScope())
        {
            // get the ClientPolicyStore instance
            var clientPolicyStore = scope.ServiceProvider.GetRequiredService<IClientPolicyStore>();

            // seed Client data from appsettings
            await clientPolicyStore.SeedAsync();

            // get the IpPolicyStore instance
            var ipPolicyStore = scope.ServiceProvider.GetRequiredService<IIpPolicyStore>();

            // seed IP data from appsettings
            await ipPolicyStore.SeedAsync();
        }

        await webHost.RunAsync();
}
public class CustomRateLimitConfiguration : RateLimitConfiguration
{
    protected override void RegisterResolvers()
    {
    	base.RegisterResolvers();

    	ClientResolvers.Add(new ClientQueryStringResolveContributor(HttpContextAccessor, ClientRateLimitOptions.ClientIdHeader));
    }
}
public class CustomRateLimitConfiguration : RateLimitConfiguration
{
    public override ICounterKeyBuilder EndpointCounterKeyBuilder { get; } = new EndpointCounterKeyBuilder();
}

If you want to upgrade and make it all work, you have to configure two new services in your Startup.cs:

// https://github.com/aspnet/Hosting/issues/793
// the IHttpContextAccessor service is not registered by default.
// the ClientId / IP resolvers use it.
services.AddSingleton<IHttpContextAccessor, HttpContextAccessor>();

// configure the resolvers (default config)
services.AddSingleton<IRateLimitConfiguration, RateLimitConfiguration>();

Summing up the breaking-chages:

  • You have to manually seed the appsettings.json policies.
  • You have to register two new services.

PS: I will wait a week to discuss/talk about the changes/features before merging this.

Suggestions and feedback are most welcome.

Thanks

@cristipufu cristipufu changed the title Refactor / Resolvers / Counter Key Builders / Async Stores / Wildcard Match Fixes Refactoring & extensibility Feb 17, 2019
@cristipufu cristipufu merged commit 729e852 into master Feb 21, 2019
@cristipufu cristipufu deleted the feature/refactor_duplicate branch February 21, 2019 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant