-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
#692 Simplify downstream service config #1353
#692 Simplify downstream service config #1353
Conversation
I can't believe this is not possible already. The docu says "The Global configuration is a bit hacky and allows overrides of Route specific settings." That sounds like it's exactly for this purpose. Is there a workaround for this prior to this change? |
I didn't find anything that could allow for that. The Global Settings seems to be quite limited |
Me neither sadly, this would be very useful. |
@cezarypiatek Hi Cezary! Unfortunately a lot of thing have changed during 3 years, and your feature branch contains merge conflicts which are impossible to resolve. Could you Sync fork please? So, new develop branch will occur with all top commits! Syncing fork will ask you to resolve merge conflicts of this branch. |
7d40231
to
f4d9633
Compare
@cezarypiatek Hi Cezary! I don't see develop branch in your fork! Your fork is too old. Could you Sync fork please? So, new develop branch will occur with all top commits! Could you add me as collaborator to your forked repo please? I will create develop branch and make it default. |
@mariotacke Hi Mario! @NicoJuicy @philproctor @gao-artur @golavr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First PR review on Ocelot, but PR looks good.
Just an addition, the current assumption is that the key is defined within the same GlobalConfiguration file.
I think functionality-wise, it would simplify a lot if you could reference a setting in app.settings. Instead of referencing a named host in the same ocelot config.
Eg. Current: JsonPlaceholderService
Alternative: '/ProxySettings/FeatureFlagManagerEndpoint'
This setting would be looked up in:
appsettings.json
{
"ProxySettings":{ "FeatureFlagManagerEndpoint":"https://myfeatureflagmanager.domain.com"}
}
This however would expect a URL, instead of the combined host + port that seems to be standard within Ocelot.
@NicoJuicy reviewed on Aug 11:
Nico,
So, your idea to use appsettings.json for custom properties will allow to make the pipeline extendable and more stable. |
@raman-m I actually like the configuration how it is. The ocelot implementation in the same app avoids cors issues. Here, backendgateway is a setting in app.settings I'm using an extension for that using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
namespace Shop.Backend.Ocelot
{
using global::Ocelot.Configuration.File;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.DependencyInjection;
public class GlobalHosts : Dictionary<string, Uri> { }
public static class FileConfigurationExtensions
{
public static IServiceCollection ConfigureDownstreamHostAndPortsPlaceholders(
this IServiceCollection services,
IConfiguration configuration)
{
services.PostConfigure<FileConfiguration>(fileConfiguration =>
{
var globalHosts = configuration
.GetSection($"Ocelot:{nameof(FileConfiguration.GlobalConfiguration)}:Hosts")
.Get<GlobalHosts>();
foreach (var route in fileConfiguration.Routes)
{
ConfigureRoute(route, globalHosts);
}
});
return services;
}
private static void ConfigureRoute(FileRoute route, GlobalHosts globalHosts)
{
foreach (var hostAndPort in route.DownstreamHostAndPorts)
{
var host = hostAndPort.Host;
if (host.StartsWith("{") && host.EndsWith("}"))
{
var placeHolder = host.TrimStart('{').TrimEnd('}');
if (globalHosts.TryGetValue(placeHolder, out var uri))
{
route.DownstreamScheme = uri.Scheme;
hostAndPort.Host = uri.Host;
hostAndPort.Port = uri.Port;
}
}
}
}
}
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cezarypiatek
Hi Cezary!
We should keep in mind that class renaming is quite aggressive refactoring, and we must avoid such refactoring in favor of using composition and inheritance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renaming from FileHostAndPort
to FileGlobalDownstreamHostConfig
is just crazy idea!
@@ -48,7 +48,7 @@ public FileRoute() | |||
public FileRateLimitRule RateLimitOptions { get; set; } | |||
public FileAuthenticationOptions AuthenticationOptions { get; set; } | |||
public FileHttpHandlerOptions HttpHandlerOptions { get; set; } | |||
public List<FileHostAndPort> DownstreamHostAndPorts { get; set; } | |||
public List<FileDownstreamHostConfig> DownstreamHostAndPorts { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no need to rename the property to indicate that it contains configuration for downstream service route. It is totally clear thing!
public string Host { get; set; } | ||
public int Port { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These properties can be inherited from old existing FileHostAndPort
class.
public class FileDownstreamHostConfig | ||
{ | ||
/// <summary> | ||
/// Key to reference downstream host config from global configuration. | ||
/// </summary> | ||
/// <value> | ||
/// Key reference from <see cref="FileGlobalConfiguration.DownstreamHosts"/>. | ||
/// </value> | ||
public string GlobalHostKey { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public class FileDownstreamHostConfig | |
{ | |
/// <summary> | |
/// Key to reference downstream host config from global configuration. | |
/// </summary> | |
/// <value> | |
/// Key reference from <see cref="FileGlobalConfiguration.DownstreamHosts"/>. | |
/// </value> | |
public string GlobalHostKey { get; set; } | |
public class FileDownstreamHostConfig : FileHostAndPort | |
{ | |
public string GlobalHostKey { get; set; } | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just zero benefits in defining of this new model!
You can reuse the old FileHostAndPort
model for global config logic.
test/Ocelot.ManualTest/ocelot.json
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it is better to make new copied ocelot2.json from this file and write new test(s) which will test exactly this new feature.
|
||
var globalConfig = new FileGlobalConfiguration | ||
{ | ||
DownstreamHosts = new Dictionary<string, FileGlobalDownstreamHostConfig> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Values should have the FileHostAndPort
type actually!
DownstreamHosts = new Dictionary<string, FileGlobalDownstreamHostConfig> | |
DownstreamHosts = new Dictionary<string, FileHostAndPort> |
_logger = loggerFactory.CreateLogger<DownstreamAddressesCreator>(); | ||
} | ||
|
||
public List<DownstreamHostAndPort> Create(FileRoute route, FileGlobalConfiguration globalConfiguration) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have to keep the version with one argument, and we have to add new overloaded version to the IDownstreamAddressesCreator
interface!
return route.DownstreamHostAndPorts.Select(hostAndPort => new DownstreamHostAndPort(hostAndPort.Host, hostAndPort.Port)).ToList(); | ||
foreach (var downstreamHost in route.DownstreamHostAndPorts) | ||
{ | ||
if (string.IsNullOrWhiteSpace(downstreamHost.GlobalHostKey) == false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't read nullable type, it is safe to remove this equality operator.
@@ -6,6 +6,6 @@ namespace Ocelot.Configuration.Creator | |||
{ | |||
public interface IDownstreamAddressesCreator | |||
{ | |||
List<DownstreamHostAndPort> Create(FileRoute route); | |||
List<DownstreamHostAndPort> Create(FileRoute route, FileGlobalConfiguration globalConfiguration); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have to keep the version with one argument, and we have to add new overloaded version with 2 arguments!
Bad formatting of the description! It is hard to read! |
@cezarypiatek commented on Oct 13, 2020:
Yeah, it is bad idea! See code review plz!
What Ocelot feature do we need to use to achieve this? 😉
More short version: {
"Routes": [{
"DownstreamHostAndPorts": [
{
"GlobalHostKey": "JsonPlaceholderService"
}
],
},
],
"GlobalConfiguration": {
"DownstreamHosts": {
"JsonPlaceholderService": {
"Host": "jsonplaceholder.typicode.com",
"Port": 80
}
}
}
} What is simplified here? You just removed one line from the "DownstreamHostAndPorts" property... {
"Routes": [{
"DownstreamHostAndPorts": ["JsonPlaceholderService"],
},
],
"GlobalConfiguration": {
"DownstreamHosts": {
"JsonPlaceholderService": {
"Host": "jsonplaceholder.typicode.com",
"Port": 80
}
}
}
} ...which looks more simplified. But, because "DownstreamHostAndPorts" property has described in a lot of docs, it is totally crazy idea to refactor this property by introducing breaking changes and confusion in docs! {
"Routes": [{
"DownstreamHosts": ["JsonPlaceholderService"],
},
],
"GlobalConfiguration": {
"DownstreamHosts": {
"JsonPlaceholderService": {
"Host": "jsonplaceholder.typicode.com",
"Port": 80
}}}
} Moreover, I suggest you to introduce Scheme property in alias-object: {
"Routes": [
// old definition
{
"DownstreamPathTemplate": "/posts",
"DownstreamScheme": "http",
"DownstreamHostAndPorts": [
{
"Host": "jsonplaceholder.typicode.com",
"Port": 80
}
],
},
// new definition 1
{
"DownstreamPathTemplate": "/posts",
"DownstreamScheme": "http",
"DownstreamHosts": ["JsonPlaceholderService"],
},
// new definition 2
{
"DownstreamPathTemplate": "/posts",
"DownstreamAlias": "JsonPlaceholderService", // not array!!!
},
],
"GlobalConfiguration": {
"DownstreamHosts": {
"JsonPlaceholderService": {
"Host": "jsonplaceholder.typicode.com",
"Port": 80
}},
"DownstreamAliases": {
"JsonPlaceholderService": {
"Scheme": "http",
"Host": "jsonplaceholder.typicode.com",
"Port": 80
}}
}
} Pay attention "DownstreamAlias" of a route can be just single string, not an array object. Finally, because "DownstreamAliases" contains all required props of an URL, we can define alias like that: {
"Routes": [
// old definition
{
"DownstreamPathTemplate": "/posts",
"DownstreamScheme": "http",
"DownstreamHostAndPorts": [
{
"Host": "jsonplaceholder.typicode.com",
"Port": 80
}
],
},
// new definition
{
"DownstreamPathTemplate": "/posts",
"DownstreamUrl": "JsonPlaceholderService", // not array!!!
},
],
"GlobalConfiguration": {
"DownstreamUrls": {
"JsonPlaceholderService": "http://jsonplaceholder.typicode.com",
}
}
} But it seems such simplification by aliases is overhead design, because route can have simplest URL definition: {
"Routes": [
// old definition
{
"DownstreamPathTemplate": "/posts",
"DownstreamScheme": "http",
"DownstreamHostAndPorts": [
{
"Host": "jsonplaceholder.typicode.com",
"Port": 80
}
],
},
// new definition
{
"DownstreamPathTemplate": "/posts",
"DownstreamUrl": "http://jsonplaceholder.typicode.com",
// and/or
"Downstream": "http://jsonplaceholder.typicode.com",
},
],
"GlobalConfiguration": {
// empty
}
} No global aliases at all. And global config is empty. And we use default port |
{
"DownstreamPathTemplate": "/posts",
"DownstreamScheme": "http",
"DownstreamHosts": ["JsonPlaceholderService"],
}, This is an interesting PR, separating Routes and Hosts is a wise choice... We could imagine splitting the config file and with the help of an IHostedService retrieve the clusters dynamically. Then we would achieve a real polling 😄 |
Gui, I don't know... This PR is about placeholders in global section, and this PR is not about dynamic routing, or service discovery... |
Correct, I don't care. I needed that 3 years ago. |
@raman-m It's still a very interesting PR though. A "neat" but breaking solution would be to use the |
Strange answer... Good luck! FYI, I have been leading this project since May 2023, started 6 months ago... |
Each new release seems to bring more conflicts our way! 😂 |
This PR cannot be accepted for the following reasons:
The PR will be closed. |
The concept is commendable; however, as an OSS project, Ocelot project consistently faces challenges with professional contributions. While many developers propose ideas, only a handful are capable of implementing the feature. |
Hello Nico, |
Closes #692
This allows to simplify downstream configuration by defining a given downstream service once in the global configuration and reuse it later by referencing with
GlobalHostKey
.This also allows to very easily override downstream address per environment.
Important: I've changed a lot of files because I need to rename
FileHostAndPort
→FileDownstreamHostConfig
.All things related to rename are in a separated commit.