search

ThreeMammals / Ocelot

.NET API Gateway
https://www.nuget.org/packages/Ocelot
MIT License
8.38k stars 1.64k forks source link

#1634 #1487 #1329 #1304 #1294 #793 Consul polling of services: enhancements and fix errors #1670

Closed ggnaegi closed 1 year ago

ggnaegi commented 1 year ago

Fixes #1634 #1487 #1329 #1304 #1294 #793

Proposing some improvements for Consul "polling"

Proposed Changes

raman-m commented 1 year ago

Nice PR, Guillaume! 😸

You forgot to request my review. 😉 And I will review in a couple of work days...

ggnaegi commented 1 year ago

@raman-m I can't add you as a reviewer...

ggnaegi commented 1 year ago

@raman-m Just checked the changes (with lock) in a docker container. It works

ggnaegi commented 1 year ago

If you don't mind, I will make some code readability improvements...

What do you mean? Maybe in the future an editorconfig file would help? https://learn.microsoft.com/en-us/visualstudio/ide/create-portable-custom-editor-options?view=vs-2022

raman-m commented 1 year ago

What do you mean? Maybe in the future an editorconfig file would help? Docs: Create portable, custom editor settings with EditorConfig

Interesting Visual Studio feature, but not now!

I meant the improvements in your code to read. Not in entire project! 🤣 You will see...

ggnaegi commented 1 year ago

I meant the improvements in your code to read. Not in entire project! 🤣 You will see...

Ok, but, in general, it would be a nice addition.

ggnaegi commented 1 year ago

raman-m left a comment

Maybe it makes sense to emphasize that Consul is default type provider even if type is not specified (in case of .AddConsul())...

Yeah, it makes sense

ggnaegi commented 1 year ago

@raman-m So, I thought we could do the same with the kubernetes provider factory, like Eureka and Consul...

raman-m commented 1 year ago

@raman-m So, I thought we could do the same with the kubernetes provider factory, like Eureka and Consul...

Yes, we could! It seems I have to review once again... 😫

ggnaegi commented 1 year ago

@raman-m commented on Jun 28

😅 Yeah, sorry about this. I just modified that bit, since it seems logical to me, that the "getter logic" should be applied everywhere.

ggnaegi commented 1 year ago

@raman-m Hello, it's done...

raman-m commented 1 year ago

@ggnaegi commented on June 27, 2023

Why not to update the Service Discovery docs? 😉

I guess we need to:

Any ideas are welcome!

ggnaegi commented 1 year ago

Why not to update the Service Discovery docs? 😉

Yeah, well. Let's say, we merge this PR and then I will create a new PR for the docs, OK?

raman-m commented 1 year ago

Not OK! All PRs in this repo come with docs updating too! Tom points to updating docs always while reviewing a PR. It is better to add docs before asking him to review and merge. See example: #1655 where Tom asked to update docs.

If you don't know how to update docs, no worries, I will help you. Maybe today, maybe during this weekend...

Happy Friday!

ggnaegi commented 1 year ago

Not OK! All PRs in this repo come with docs updating too!

It was a rhetorical question then 😅

Yeah so I will update the docs during the week-end.

raman-m commented 1 year ago

@raman-m commented on Jun 30

Ha-ha! I said "Happy Friday" 3 months ago... 😆 It is time, this autumn, to say once again: Happy Friday, Guillaume! 🤣

Sorry, I was sooo busy with open PRs reviewing, conflicts resolutions, and getting ready them for real code reviews. 😫 Now I am back to your PR... 😉

@ggnaegi has invited you to collaborate on this repository.

Wow! 😃 Thank you! But it is expired!

Is everything OK with your feature branch? Is it rebased onto develop?

raman-m commented 1 year ago

The feature branch has been rebased onto develop! Please, pull commits!

Fröhlichen Freitag! :bowtie:

ggnaegi commented 1 year ago

Hi @raman-m, Sorry, I forgot to write the documentation. I was a bit frustrated with the current situation, so I started my own PoC with YARP, OIDC and Consul here : https://github.com/ggnaegi/SwizlyPeasy.Gateway

raman-m commented 1 year ago

@ggnaegi commented on Sep 16

Sad hearing that you moved to Yarp instead of Ocelot. But I understand your decision. Ocelot is deprecated, and Tom as an owner, doesn't care about this project. I will return to you with some news in a couple of weeks...

Regarding documentation... Don't worry, I will update docs during my own code review... Updating docs requires focusing on source code to understand proposed changes and how it should be reflected to developer's documentation.

Welcome to code review!

raman-m commented 1 year ago

@ggnaegi added 2 commits on Sep 16

What have you merged? I cannot get it... Ghost merge-commits have no sense.

image

That's bad!

ggnaegi commented 1 year ago

@raman-m I just pulled the changes as you wrote and then this happened... That was a bit odd, since the same commits were shown twice. Should I have done git pull --rebase ?

raman-m commented 1 year ago

Please make linear history! How, it is up to you.

ggnaegi commented 1 year ago

@ggnaegi commented on Sep 16

Sad hearing that you moved to Yarp instead of Ocelot. But I understand this decision. Ocelot is deprecated, and Tom as an owner, doesn't care about this project. I will return to you with some news in a couple of weeks...

@raman-m Tom could at least write somewhere that the project is deprecated and that it's only maintained for security reasons. I read an article about ocelot on linkedin two weeks ago! You still have a strong developer base but no support and no improvements on some buggy features. It's a sad thing. I was happy with Ocelot but it's not an option anymore...

raman-m commented 1 year ago

@ggnaegi

Tom could at least write somewhere that the project is deprecated and that it's only maintained for security reasons.

Where? And how? Ocelot community tells that every day... ))

I read an article about ocelot on LinkedIn two weeks ago!

Could you share the link plz? Who is the author?

You still have a strong developer base but no support and no improvements on some buggy features. It's a sad thing. I was happy with Ocelot but it's not an option anymore...

You did the right thing because other gateway products just evolute having regular releases.

Anyway, hope that you will return to this project to contribute...

raman-m commented 1 year ago

@ggnaegi commented on Sep 18, 11:32

Are you going to fix the problem? Can I help you?

Should I have done git pull --rebase ?

Yes, please! If you have time...

ggnaegi commented 1 year ago

@raman-m

I read an article about ocelot on LinkedIn two weeks ago!

Could you share the link plz? Who is the author?

Yes sure: JOVANOVIC Milan: https://www.linkedin.com/posts/milan-jovanovic_softwareengineering-dotnet-microservices-activity-7015949109593309184-Ssxx DOKIC Stefan: https://lnkd.in/dCmEkCNP

ggnaegi commented 1 year ago

@ggnaegi commented on Sep 18, 11:32

Are you going to fix the problem? Can I help you?

Should I have done git pull --rebase ?

Yes, please! If you have time...

@raman-m should be fine now. Should I also commit the changes suggested by @RaynaldM?

raman-m commented 1 year ago

@ggnaegi commented on Sep 18, 13:30

Already Done! See https://github.com/ThreeMammals/Ocelot/pull/1670/commits/b3f1d551c150967f9fc2178fd5839fc77c841e05

ggnaegi commented 1 year ago

@ggnaegi commented on Sep 18, 13:30

Already Done! See b3f1d55

@raman-m, ok, that's another perspective... I just got your point. But still, we could have: public const string PollKube = nameof(PollKube);

raman-m commented 1 year ago

@ggnaegi We could, but it references to itself: image Technically we can change.

ggnaegi commented 1 year ago

@raman-m commented on Sep 18, 13:41

Let's keep it as it is in https://github.com/ThreeMammals/Ocelot/commit/b3f1d551c150967f9fc2178fd5839fc77c841e05, ok?

ggnaegi commented 1 year ago

@raman-m I just had a look at the Eureka and Kubernetes providers... IMHO, we could modify them, because PollKube is using the same Timer-Logic as PollConsul did...

raman-m commented 1 year ago

@ggnaegi Ocelot.AcceptanceTests.ConfigurationReloadTests.should_trigger_change_token_on_change Please, fix the failed test! Something was changed and this test must be reviewed. It fails locally too.

P.S. I've rebased feature branch. Don't pull, because pull can have auto-merge enabled. Just do:

In this case, I hope, there will be no auto-merge commits. Don't forget to Sync fork 😉 There is new commit for develop branch.

ggnaegi commented 1 year ago

@ggnaegi Ocelot.AcceptanceTests.ConfigurationReloadTests.should_trigger_change_token_on_change Please, fix the failed test! Something was changed and this test must be reviewed. It fails locally too.

@raman-m I should check, Xunit is running the tests in parallel, it might cause the problems.

ggnaegi commented 1 year ago

@raman-m Ok, tests are passing now (2161db0). But you might have a problem somewhere else. Look at the job on the main branch, it failed: https://app.circleci.com/pipelines/github/ThreeMammals/Ocelot/857/workflows/cf2a0f20-4d12-4515-a251-c00a418f5259/jobs/1592

ggnaegi commented 1 year ago

@raman-m I just had a look at the Eureka and Kubernetes providers... IMHO, we could modify them, because PollKube is using the same Timer-Logic as PollConsul did...

@raman-m Should I include the changes in this PR too, or later?

raman-m commented 1 year ago

@ggnaegi commented on Sep 19:

@raman-m Ok, tests are passing now (2161db0).

Thanks for fixing it! I saw this fix in another PR that got stuck and wasn't delivered to develop branch. To be fair, I wondered why parallelization was not disabled for hard acceptance tests. Or we have some hidden setting in the project to disable XUnit parallelization... Anyway, it is nice to include to your PR...

Look at the job on the main branch, it failed: https://app.circleci.com/pipelines/github/ThreeMammals/Ocelot/857/workflows/cf2a0f20-4d12-4515-a251-c00a418f5259/jobs/1592

This failed build doesn't belong to your feature branch!!!

raman-m commented 1 year ago

@ggnaegi commented on Sep 19:

Should I include the changes in this PR too, or later?

Did you mean fixing thread safety & starting errors for another types of providers? Well... This is beyond the scope of this feature, but... It will be very desirable to fix other service providers. In this case we must search for other open issues in backlog and link them to fix by this PR. But this is not trivial task... 😆

raman-m commented 1 year ago

@ggnaegi commented on Sep 19:

Ok, tests are passing now (2161db0). But you might have a problem somewhere else. Look at the job on the main branch, it failed: Build 1592

So, I've created issue:

Could you create new PR for #1700 please? You will be the author of this fix. 😉 PR creation will take 5 minutes, I guess... I would like to merge this fix ASAP on Monday...

ggnaegi commented 1 year ago

@raman-m What should we do with this one?

@raman-m commented on Sep 19: Did you mean fixing thread safety & starting errors for another types of providers? Well... This is beyond the scope of this feature, but... It will be very desirable to fix other service providers. In this case we must search for other open issues in backlog and link them to fix by this PR. But this is not trivial task... 😆

I would create an new PR per Provider later, ok?

ggnaegi commented 1 year ago

@raman-m @RaynaldM I might have a solution to generalize the "polling" for Consul, Kubernetes and Eureka... Should I push it, or would you like to merge first and then open a new PR?

using Ocelot.Logging;
using Ocelot.ServiceDiscovery.Providers;

namespace Ocelot.Polling;

public class PollingServicesManager<T, TU>
    where T : class, IServiceDiscoveryProvider
    where TU : ServicePollingHandler<T>
{
    private readonly object _lockObject = new();
    private readonly List<ServicePollingHandler<T>> _serviceDiscoveryProviders = new();

    public ServicePollingHandler<T> GetServicePollingHandler(T baseProvider, string serviceName, int pollingInterval,
        IOcelotLoggerFactory factory)
    {
        lock (_lockObject)
        {
            var discoveryProvider = _serviceDiscoveryProviders.FirstOrDefault(x => x.ServiceName == serviceName);
            if (discoveryProvider != null)
            {
                return discoveryProvider;
            }

            discoveryProvider =
                (TU)Activator.CreateInstance(typeof(TU), baseProvider, pollingInterval, serviceName, factory);
            _serviceDiscoveryProviders.Add(discoveryProvider);

            return (TU)discoveryProvider;
        }
    }
}
using Ocelot.Logging;
using Ocelot.ServiceDiscovery.Providers;
using Ocelot.Values;

namespace Ocelot.Polling;

public abstract class ServicePollingHandler<T> : IServiceDiscoveryProvider
    where T : class, IServiceDiscoveryProvider
{
    private readonly T _baseProvider;
    private readonly object _lockObject = new();
    private readonly IOcelotLogger _logger;
    private readonly int _pollingInterval;

    private DateTime _lastUpdateTime;
    private List<Service> _services;

    protected ServicePollingHandler(T baseProvider, int pollingInterval, string serviceName,
        IOcelotLoggerFactory factory)
    {
        _logger = factory.CreateLogger<ServicePollingHandler<T>>();
        _pollingInterval = pollingInterval;

        // Initialize by DateTime.MinValue as lowest value.
        // Polling will occur immediately during the first call
        _lastUpdateTime = DateTime.MinValue;
        _services = new List<Service>();
        ServiceName = serviceName;
        _baseProvider = baseProvider;
    }

    public string ServiceName { get; protected set; }

    public Task<List<Service>> Get()
    {
        lock (_lockObject)
        {
            var refreshTime = _lastUpdateTime.AddMilliseconds(_pollingInterval);

            // Check if any services available
            if (refreshTime >= DateTime.UtcNow && _services.Any())
            {
                return Task.FromResult(_services);
            }

            _logger.LogInformation($"Retrieving new client information for service: {ServiceName}.");
            _services = _baseProvider.Get().Result;
            _lastUpdateTime = DateTime.UtcNow;

            return Task.FromResult(_services);
        }
    }
}
using Ocelot.Logging;
using Ocelot.Polling;

namespace Ocelot.Provider.Consul;

public class PollConsul : ServicePollingHandler<Consul>
{
    public PollConsul(Consul baseProvider, int pollingInterval, string serviceName, IOcelotLoggerFactory factory) : base(baseProvider, pollingInterval, serviceName, factory)
    {
    }
}
using Microsoft.Extensions.DependencyInjection;
using Ocelot.Configuration;
using Ocelot.Logging;
using Ocelot.Polling;
using Ocelot.ServiceDiscovery.Providers;

namespace Ocelot.Provider.Consul;

public static class ConsulProviderFactory
{
    /// <summary>
    ///     String constant used for provider type definition.
    /// </summary>
    public const string PollConsul = nameof(Provider.Consul.PollConsul);
    private static readonly PollingServicesManager<Consul, PollConsul> ServicesManager = new();

    public static ServiceDiscoveryFinderDelegate Get { get; } = CreateProvider;

    private static IServiceDiscoveryProvider CreateProvider(IServiceProvider provider,
        ServiceProviderConfiguration config, DownstreamRoute route)
    {
        var factory = provider.GetService<IOcelotLoggerFactory>();
        var consulFactory = provider.GetService<IConsulClientFactory>();

        var consulRegistryConfiguration = new ConsulRegistryConfiguration(
            config.Scheme, config.Host, config.Port, route.ServiceName, config.Token);

        var consulProvider = new Consul(consulRegistryConfiguration, factory, consulFactory);

        if (PollConsul.Equals(config.Type, StringComparison.OrdinalIgnoreCase))
        {
            return ServicesManager.GetServicePollingHandler(consulProvider, route.ServiceName, config.PollingInterval, factory);
        }

        return consulProvider;
    }
}
raman-m commented 1 year ago

@ggnaegi commented on Sep 27, 1:38pm:

I might have a solution to generalize the "polling" for Consul, Kubernetes and Eureka... Should I push it, or would you like to merge first and then open a new PR?

Thank you for your intention to improve the code quality! But not in this PR please which is related to Consul services problems. Let's create a separate follow up PR, please... I am afraid this PR cannot be included into September's release if we have (will have) multiple review for a 3 month and more. Multiple code reviews makes delivery very slow. And, improvements for other service discovery providers can be included in October's release, not urgent. So, delivery of Consul improvements is very important in current upcoming release. The PR will close 6 issues! 😻

raman-m commented 1 year ago

@ggnaegi commented on Sep 26

I would create an new PR per Provider later, ok?

Totally 🆗 😉 See my opinion above plz! ☝️ But not per provider! I believe these improvements can be combined into one PR...

raman-m commented 1 year ago

@ggnaegi What are we waiting for? What is the rest of work?

ggnaegi commented 1 year ago

@ggnaegi What are we waiting for? What is the rest of work?

I don't know, you said docs?

raman-m commented 1 year ago

@ggnaegi commented on Sep 28

Yeah, I see:

@raman-m commented on Jun 29

Will update docs soon...

ggnaegi commented 1 year ago

@raman-m so, first merge to develop then update the docs?

raman-m commented 1 year ago

@ggnaegi commented: so, first merge to develop then update the docs?

No! First, Update feature branch by top commits from develop Second, Update docs by committing to feature branch Third, I will merge to develop

I believe you did everything for this PR if you cannot (don't know how) to review docs. I suggest you to focus on other PRs... Follow up PR for this one with refactoring other service providers (Eureka, etc.) Or, you can look into the bug #1706

I hope, this your PR will be updated & merged right today, because I've planned to work on it today.

P.S. And, don't forget to sync fork, update develop of your forked repo.

raman-m commented 1 year ago

If you don't mind, I will make some improvements to the code...

ggnaegi commented 1 year ago

@raman-m what do you mean by code improvements? We already had several reviews and changes... @raman-m You mean some missing braces? Yeah, ok, you could add them. I should modify my Resharper config.