#954 #957 #1026 Customize Consul services creation in `Consul` service discovery provider

raman-m commented 4 months ago

Fixes #954 #957 #1026

954
957
1026

Proposed Changes

This PR introduces general customization options for the Consul provider, taking inspiration from the Kube provider customization in PR #2052. However, this PR offers a more flexible design as outlined below 👇

It allows overriding the behavior of the Consul provider.
It enables the injection of a new IConsulServiceBuilder service into the Consul provider.
It permits the overriding of the DefaultConsulServiceBuilder class's behavior.
The targeted virtual methods which solve the 954 problem are:
- protected virtual ServiceHostAndPort GetServiceHostAndPort(ServiceEntry entry, Node node)
- protected virtual string GetDownstreamHost(ServiceEntry entry, Node node)

In conclusion, customization hinges on the virtual methods of the DefaultConsulServiceBuilder class, allowing developers to override as needed for highly customized Consul setups in their environments.

Initially, I considered introducing JSON overrides for the ServiceDiscoveryProvider section in the global configuration of ocelot.json. However, I realized that approach was suboptimal. Writing C# code that inherits from the DefaultConsulServiceBuilder class is a superior solution.

Predecessors

909
2052

raman-m commented 4 months ago

@ignacy130 Hello Ignacy! I would value your code review and your thoughts on it as well.

@ggnaegi You are the primary reviewer for this PR. 😉 I believe the C# design is finalized, and I am currently working on the tests... You could begin the code review today...

ggnaegi commented 4 months ago

@raman-m Your idea is great, but I think we should rework it a bit first. We should think about polling and long polling and give the ability to the users to develop their own implementations.

raman-m commented 4 months ago

We should think about polling and long polling and give the ability to the users to develop their own implementations.

@ggnaegi, I understand you're expecting new method overrides for the next version of the provider with enhanced polling capabilities, correct? You'll have the opportunity to do so. 😉 Are you suggesting that this bug fix should not be included in the current monthly release and that we need to work on it further? Please note that we now have a common release strategy where we release the library, the provider, and even the documentation together. If we release this bug fix in v23.3, the corresponding Ocelot.Provider.Consul v23.3 will also be released. If we further refactor Ocelot.Provider.Consul as you propose, it will be released in subsequent versions 23.x or 24.x. Therefore, I don't see the benefit of waiting for an ideal solution by delaying the delivery of the bug fix.

Let's proceed with releasing this bug fix and the package update in version 23.3, shall we?

raman-m commented 4 months ago

The rest of the work

[x] Unit tests for DefaultConsulServiceBuilder and Consul with 100% coverage. Done → https://github.com/ThreeMammals/Ocelot/pull/2067/commits/c5de8a56a3872f21aa5a5d5b21ae6cf739ffdba3 (https://github.com/ThreeMammals/Ocelot/pull/2067/commits/972091f7625bff0d570c9e6aea29629eaf9a034c)
[x] Acceptance test for #954 user scenario. See "Steps to Reproduce the Problem". Done → https://github.com/ThreeMammals/Ocelot/pull/2067/commits/8dcaf7433a90d14d6eb16ef5404ed24e1e5d406d
[x] Review SD docs. Move "Store Configuration in Consul" to "Service Discovery". Done → https://github.com/ThreeMammals/Ocelot/pull/2067/commits/8073a9947c66eb32623b53d8e859c031ac1235b2

raman-m commented 4 months ago

@ggnaegi Dev Complete! Docs written! Waiting for your approval after final review... Could you review this weekend please?

ThreeMammals / Ocelot