ThreeMammals / Ocelot

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

No LoadBalancer exists when using Consul ServiceDiscoveryProvider #142

Closed hampos closed 6 years ago

hampos commented 6 years ago

Hey, I've been having an issue when using Consul as ServiceDiscoveryProvider. If the OcelotConfiguration key does not exist in Consul Key/Value store, Ocelot runs the FileOcelotConfigurationCreator and sets up the LoadBalancers in this line: https://github.com/TomPallister/Ocelot/blob/1d1a68ff95bb1caf5c279c64e18b5d168e82f7cd/src/Ocelot/Configuration/Creator/FileOcelotConfigurationCreator.cs#L179 and everything works fine.

If I rerun Ocelot and OcelotConfiguration key exists in Consul store, it pulls the configuration from there and the FileOcelotConfigurationCreator never runs, because of that LoadBalancers are never set up and the LoadBalancingMiddleware always throws an error here: https://github.com/TomPallister/Ocelot/blob/1d1a68ff95bb1caf5c279c64e18b5d168e82f7cd/src/Ocelot/LoadBalancer/Middleware/LoadBalancingMiddleware.cs#L32 because there aren't any LoadBalancers registered in the LoadBalancerHouse.

Am I missing something? My configuration is:

"GlobalConfiguration": {
        "ServiceDiscoveryProvider": {
            "Provider": "Consul",
            "Host": "consul",
            "Port": 8500
        }
    }
"ReRoutes": [
        {
            "DownstreamPathTemplate": "/status",
            "DownstreamScheme": "http",
            "UpstreamPathTemplate": "/cs/status",
            "UpstreamHttpMethod": [ "Get" ],
            //"DownstreamPort": 80,
            //"DownstreamHost": "haproxy",
            "ServiceName": "haproxy-80",
            "LoadBalancer": "LeastConnection",
            "RequestIdKey": "OcRequestId"
        }
    ]

And what exists in Consul store is:

{
    "ReRoutes": [
        {
            "ReRouteKey": "/cs/status|Get",
            "DownstreamPathTemplate": {
                "Value": "/status"
            },
            "UpstreamPathTemplate": {
                "Value": "/cs/status"
            },
            "UpstreamTemplatePattern": "^(?i)/cs/status/$",
            "UpstreamHttpMethod": [
                {
                    "Method": "Get"
                }
            ],
            "IsAuthenticated": false,
            "IsAuthorised": false,
            "AuthenticationOptions": {
                "AllowedScopes": [],
                "AuthenticationProviderKey": null
            },
            "ClaimsToQueries": [],
            "ClaimsToHeaders": [],
            "ClaimsToClaims": [],
            "RouteClaimsRequirement": {},
            "RequestIdKey": "OcRequestId",
            "IsCached": false,
            "CacheOptions": {
                "TtlSeconds": 0,
                "Region": "Getcsstatus"
            },
            "DownstreamScheme": "http",
            "IsQos": false,
            "QosOptionsOptions": {
                "ExceptionsAllowedBeforeBreaking": 0,
                "DurationOfBreak": 0,
                "TimeoutValue": 0,
                "TimeoutStrategy": 1
            },
            "LoadBalancer": "LeastConnection",
            "DownstreamHost": null,
            "DownstreamPort": 0,
            "ServiceProviderConfiguraion": {
                "ServiceName": "haproxy-80",
                "DownstreamHost": null,
                "DownstreamPort": 0,
                "UseServiceDiscovery": true,
                "ServiceDiscoveryProvider": "Consul",
                "ServiceProviderHost": "consul",
                "ServiceProviderPort": 8500
            },
            "EnableEndpointEndpointRateLimiting": false,
            "RateLimitOptions": null,
            "HttpHandlerOptions": {
                "AllowAutoRedirect": true,
                "UseCookieContainer": true
            }
        }
    ],
    "AdministrationPath": null
}
TomPallister commented 6 years ago

@hampos this feels like a bug Ill look at it ASAP!

TomPallister commented 6 years ago

@hampos this is an interesting problem...

I need to think about a possible solution for your use case..I'm not sure it is possible to make it work in a nice way. I think I assumed that people would usually have a configuration.json which was stupid and have missed this obvious use case for load balancers ;(

TomPallister commented 6 years ago

@hampos if you use the Ocelot administration API to set your configuration then Ocelot will store it in Consul and create the load balancers.

You need to call AddOcelotStoreConfigurationInConsul after you call AddOcelot in order for this to work.

I'm still looking at ways this could work without setting the configuration in Consul with the Ocelot administration API.

hampos commented 6 years ago

@TomPallister Will I have to set the configuration through the administration API each time Ocelot starts? Since Consul stores my configuration it would make sense for it to be the central point of configuration truth. Even if I try to AddOcelotStoreConfigurationInConsul without service discovery and load balancers, just a simple DownstreamHost and DownstreamPort in my reroute, Ocelot still throws the same UnableToFindLoadBalancerError in the pipeline, because configuration has not been set through FileOcelotConfigurationCreator and the system has not initialized properly with NoLoadBalancer load balancers.

Is it possible to rewire the configuration coming from Consul to pass through the same initialization as file configuration? Why is configuration coming from Consul handled differently than File Configuration? Am I missing something obvious here?

We started researching Ocelot to build our API gateway and it will be an integral part of our work. Maybe me and my team could contribute on this?

TomPallister commented 6 years ago

@hampos Don't worry I will change Ocelot soon to handle the use case.

I'm thinking that I will change the way load balancers are set up. On the first request to a ReRoute the load balancer will be set up and cached. Each subsequent request will get the cached version. This means that we will suffer a very small performance hit the first time a ReRoute is requested but after that it will be the same as now. I think that this is probably an acceptance solution to the problem. There are a few other things that I might need to change to work like this also.

If the consul configuration went through the same setup as the file configuration this would have to happen per request to make sure it is up to date and this is a lot of work to do for each request. We store Ocelot's internal configuration in Consul so that we don't have to process the configuration on every request. I guess the configuration.json is meant to be initial set up configuration which can be changed while Ocelot is running.

TomPallister commented 6 years ago

I've made a bunch of changes that will enable users to manage the configuration in Consul check out the PR.

I think another step might be to store the FileConfiguration(external config, meant to be changed by users) in Consul rather than the OcelotConfiguration (internal config, not meant to be changed by users manually, should be changed with admin API). However I need to take a further look into how much overhead this would add.

Might be able to take the same approach as I have with the load balancers and qos providers so you create the configuration for a re route the first time it is created, maybe hash the configuration for the re route, then if the hash changes on a subsequent request you know that you need to rebuild the configuration. Something like that could work performance wise...I'll have a think about it..

hampos commented 6 years ago

@TomPallister Thanks and great job on this, saw some major changes in the PR. It seems to work fine on my original scenario, just a small comment here: https://github.com/TomPallister/Ocelot/blob/1d61e403ed7b8ba51449c2c667536ee35f0f913a/src/Ocelot/LoadBalancer/Middleware/LoadBalancingMiddleware.cs#L35 LoadBalancingMiddleware hits _configProvider.Get() , accessing the cache to get the Ocelot configuration for the second time after DownstreamRouteFinder. If in-memory cache is used this is not an problem, but when a distributed cache is used I'm not sure if it's desirable for this to happen a second time on the same Request's lifecycle. Since configuration is now needed in more than one places maybe it can be saved in IRequestScopedDataRepository ? Any thought on that or do you think it's a non-issue?

TomPallister commented 6 years ago

@hampos I agree will look into it, thanks for the help!

TomPallister commented 6 years ago

Made some more changes based on comment around the configuration!

hampos commented 6 years ago

@TomPallister works great, thanks!

TomPallister commented 6 years ago

@hampos I'll merge this when Ive got the documentation updates ready.

TomPallister commented 6 years ago

actually that doesn't matter I only need to do that when I do a release.

TomPallister commented 6 years ago

waiting for release

TomPallister commented 6 years ago

released a new package Install-Package Ocelot -Version 2.0.1 should work a bit better now.

I've opened another issue to think about storing the FileConfiguration in Consul rather than the internal OcelotConfiguration.