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

#692 Simplify downstream service config #1353

Conversation

cezarypiatek
Copy link

@cezarypiatek cezarypiatek commented Oct 13, 2020

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.

{
  "Routes": [{
      "DownstreamPathTemplate": "/api/products/{productId}",
      "DownstreamScheme": "http",
      "DownstreamHostAndPorts": [
        {
          "GlobalHostKey": "JsonPlaceholderService"
        }
      ],
      "UpstreamPathTemplate": "/products/{productId}",
      "UpstreamHttpMethod": [ "Put" ],
      "QoSOptions": {
        "ExceptionsAllowedBeforeBreaking": 3,
        "DurationOfBreak": 10,
        "TimeoutValue": 5000
      },
      "FileCacheOptions": { "TtlSeconds": 15 }
    },
    {
      "DownstreamPathTemplate": "/posts",
      "DownstreamScheme": "http",
      "DownstreamHostAndPorts": [
        {
          "GlobalHostKey": "JsonPlaceholderService"
        }
      ],
      "UpstreamPathTemplate": "/posts/",
      "UpstreamHttpMethod": [ "Get" ],
      "QoSOptions": {
        "ExceptionsAllowedBeforeBreaking": 3,
        "DurationOfBreak": 10,
        "TimeoutValue": 5000
      },
      "FileCacheOptions": { "TtlSeconds": 15 }
    },
    {
      "DownstreamPathTemplate": "/",
      "DownstreamScheme": "http",
      "DownstreamHostAndPorts": [
        {
          "Host": "www.bbc.co.uk",
          "Port": 80
        }
      ],
      "UpstreamPathTemplate": "/bbc/",
      "UpstreamHttpMethod": [ "Get" ]
    }
  ],

  "GlobalConfiguration": {
    "RequestIdKey": "ot-traceid",
    "DownstreamHosts": {
      "JsonPlaceholderService": {
        "Host": "jsonplaceholder.typicode.com",
        "Port": 80
      }
    } 
  }
}

This also allows to very easily override downstream address per environment.

Important: I've changed a lot of files because I need to rename FileHostAndPortFileDownstreamHostConfig.
All things related to rename are in a separated commit.

@Bonsai12345
Copy link

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?

@cezarypiatek
Copy link
Author

I didn't find anything that could allow for that. The Global Settings seems to be quite limited

@Bonsai12345
Copy link

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.

@gao-artur
Copy link

#692 (comment)

@raman-m raman-m changed the base branch from master to develop July 15, 2023 22:12
@raman-m raman-m added the conflicts Feature branch has merge conflicts label Jul 15, 2023
@raman-m
Copy link
Member

raman-m commented Jul 15, 2023

@cezarypiatek Hi Cezary!
Thanks for your interest in Ocelot!
Your PR is just great!

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!
After that, create new feature branch from develop one, and cherry pick required commits from current branch.

Syncing fork will ask you to resolve merge conflicts of this branch.
Give a try!

@raman-m raman-m added invalid Not actually an issue needs feedback Issue is waiting on feedback before acceptance waiting Waiting for answer to question or feedback from issue raiser and removed invalid Not actually an issue labels Jul 15, 2023
@raman-m raman-m force-pushed the feature/692_simplify_downstream_service_config branch from 7d40231 to f4d9633 Compare August 10, 2023 17:07
@raman-m raman-m added feature A new feature and removed waiting Waiting for answer to question or feedback from issue raiser conflicts Feature branch has merge conflicts labels Aug 10, 2023
@raman-m
Copy link
Member

raman-m commented Aug 10, 2023

@cezarypiatek Hi Cezary!
I thought you might need help resolving conflicts.
The feature branch has been rebased onto ThreeMammals:develop successfully!
The build is green.
So, we can start the code review...


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.

@raman-m
Copy link
Member

raman-m commented Aug 10, 2023

@mariotacke Hi Mario!
As an author of the #692 issue,
Could you review the current solution please?


@NicoJuicy @philproctor @gao-artur @golavr
After your discussions of the issue #692 , could you join to code review sessions too?

Copy link

@NicoJuicy NicoJuicy left a 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.

@raman-m
Copy link
Member

raman-m commented Aug 18, 2023

@NicoJuicy reviewed on Aug 11:

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.

Nico,
Thanks for your review!
Your idea is good! We have some problems with configuration JSON.
There are a lot of feature requests which would like to change Configuration feature: 👇

So, your idea to use appsettings.json for custom properties will allow to make the pipeline extendable and more stable.
Current ocelot.json design is a source of problems. This Configuration feature was designed by Tom. 😄
I believe migration to appsettings.json will solve a long list of issues, but such kind of refactoring requires a large efforts.

@NicoJuicy
Copy link

NicoJuicy commented Aug 18, 2023

@raman-m I actually like the configuration how it is.
I have separate files per feature ( products, clients,
.. ) the only thing that is annoying is that I have to repeat my endpoints ( I use it as a gateway for a pwa on top of a modular monolith)

The ocelot implementation in the same app avoids cors issues.

Here, backendgateway is a setting in app.settings

Screenshot_20230818_143710_Chrome

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;
                    }
                }
            }
        }
    }
}

Copy link
Member

@raman-m raman-m left a 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.

Copy link
Member

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; }
Copy link
Member

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!

Comment on lines +12 to +13
public string Host { get; set; }
public int Port { get; set; }
Copy link
Member

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.

Comment on lines +3 to +11
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; }
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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; }
}

Copy link
Member

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.

Copy link
Member

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>
Copy link
Member

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!

Suggested change
DownstreamHosts = new Dictionary<string, FileGlobalDownstreamHostConfig>
DownstreamHosts = new Dictionary<string, FileHostAndPort>

_logger = loggerFactory.CreateLogger<DownstreamAddressesCreator>();
}

public List<DownstreamHostAndPort> Create(FileRoute route, FileGlobalConfiguration globalConfiguration)
Copy link
Member

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)
Copy link
Member

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);
Copy link
Member

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!

@raman-m
Copy link
Member

raman-m commented Aug 18, 2023

@NicoJuicy commented on Aug 18

Bad formatting of the description! It is hard to read!
I guess we must discuss the PR but not your custom solutions. You could open any issues in backlog to other custom features.

@raman-m
Copy link
Member

raman-m commented Aug 18, 2023

@cezarypiatek commented on Oct 13, 2020:

Important: I've changed a lot of files because I need to rename FileHostAndPortFileDownstreamHostConfig.
All things related to rename are in a separated commit.

Yeah, it is bad idea! See code review plz!
We should not rename classes, and should not provide any breaking changes at all!


This also allows to very easily override downstream address per environment.

What Ocelot feature do we need to use to achieve this? 😉


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.

{
  "Routes": [{
      "DownstreamPathTemplate": "/api/products/{productId}",
      "DownstreamScheme": "http",
      "DownstreamHostAndPorts": [
        {
          "GlobalHostKey": "JsonPlaceholderService"
        }
      ],
      "UpstreamPathTemplate": "/products/{productId}",
      "UpstreamHttpMethod": [ "Put" ],
    },
    {
      "DownstreamPathTemplate": "/posts",
      "DownstreamScheme": "http",
      "DownstreamHostAndPorts": [
        {
          "GlobalHostKey": "JsonPlaceholderService"
        }
      ],
      "UpstreamPathTemplate": "/posts/",
      "UpstreamHttpMethod": [ "Get" ],
    },
    {
      "DownstreamPathTemplate": "/",
      "DownstreamScheme": "http",
      "DownstreamHostAndPorts": [
        {
          "Host": "www.bbc.co.uk",
          "Port": 80
        }
      ],
      "UpstreamPathTemplate": "/bbc/",
      "UpstreamHttpMethod": [ "Get" ]
    }
  ],

  "GlobalConfiguration": {
    "RequestIdKey": "ot-traceid",
    "DownstreamHosts": {
      "JsonPlaceholderService": {
        "Host": "jsonplaceholder.typicode.com",
        "Port": 80
      }
    } 
  }
}

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...
I understand simplification like that:

{
  "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!
It seems we have to design a new property for route to achieve real simplification.

{
  "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.
@cezarypiatek Looks simplistic?
@mariotacke Looks good?


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 80 of the scheme!

@raman-m raman-m added the MVP Minimum Viable Product label Aug 18, 2023
@ggnaegi
Copy link
Member

ggnaegi commented Oct 4, 2023

@raman-m

{
      "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 😄

@raman-m
Copy link
Member

raman-m commented Oct 6, 2023

@ggnaegi commented on Oct 4

Gui, I don't know... This PR is about placeholders in global section, and this PR is not about dynamic routing, or service discovery...
To be fair, it will be hard to work on this PR...
It seems, as an author, @cezarypiatek doesn't care about his PR anymore. 😢 I've asked him a lot of questions, but he doesn't care... I cannot develop ideas of other developers when they don't respond, don't write code...

@cezarypiatek
Copy link
Author

@ggnaegi commented on Oct 4

Gui, I don't know... This PR is about placeholders in global section, and this PR is not about dynamic routing, or service discovery... To be fair, it will be hard to work on this PR... It seems, as an author, @cezarypiatek doesn't care about his PR anymore. 😢 I've asked him a lot of questions, but he doesn't care... I cannot develop ideas of other developers when they don't respond, don't write code...

Correct, I don't care. I needed that 3 years ago.

@ggnaegi
Copy link
Member

ggnaegi commented Oct 8, 2023

@raman-m It's still a very interesting PR though. A "neat" but breaking solution would be to use the ServiceName we would use for consul or eureka and then allow the host definition in GlobalConfiguration.

@raman-m
Copy link
Member

raman-m commented Oct 30, 2023

@cezarypiatek commented on Oct 7

Strange answer... Good luck!

FYI, I have been leading this project since May 2023, started 6 months ago...

@raman-m
Copy link
Member

raman-m commented Oct 30, 2023

@ggnaegi commented on Oct 7

Let's decide what to do with this PR after October's release...

@raman-m
Copy link
Member

raman-m commented Jun 11, 2024

@ggnaegi commented on Oct 8, 2023

Each new release seems to bring more conflicts our way! 😂

@raman-m raman-m added conflicts Feature branch has merge conflicts and removed MVP Minimum Viable Product labels Jun 11, 2024
@raman-m
Copy link
Member

raman-m commented Nov 7, 2024

This PR cannot be accepted for the following reasons:

  • There is an enormous number of merge conflicts.
  • The author has expressed no intention to deliver this feature.
  • The quality of the PR is poor.
  • The proposed changes are in draft status: a single class renaming has resulted in an enormous number of changed files.

The PR will be closed.

@raman-m raman-m closed this Nov 7, 2024
@raman-m
Copy link
Member

raman-m commented Nov 7, 2024

@ggnaegi commented on Oct 4, 2023

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.
I am open to someone else taking over this task. Are you ready?

@raman-m
Copy link
Member

raman-m commented Nov 7, 2024

@NicoJuicy commented on Aug 18, 2023

Hello Nico,
I'm quite impressed with your concept of using alias-placeholders in the Host property, as it aligns well with the original concept of host aliases in #692. 😍 You could open a new PR... FYI, this PR will not be pursued further.
I would like to invite you to take on #692 officially. Would you have the time and interest to contribute?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conflicts Feature branch has merge conflicts feature A new feature needs feedback Issue is waiting on feedback before acceptance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DownstreamHostAndPorts alias
7 participants