Ocelot.Provider.Consul: The GetDownstreamHost method inefficiently checks the node object

[pkutsch]

Expected Behavior:
Ocelot.Provider.Consul works once properly configured as described at:
https://ocelot.readthedocs.io/en/latest/features/servicediscovery.html

Actual Behavior:
Posting to API Gateway with Ocelot configured throws Bad Gateway 502 error. Note that without Ocelot.Provider.Consul configured, the API Gateway works without any issue i.e. if 'DownstreamHostAndPorts' is used and no 'ServiceDiscoveryProvider' is configured in ocelot.json.

Performed VS debugging and found that the cause was at line 97 of DefaultConsulServiceBuilder.cs in Ocelot.Provider.Consul project:

=> node != null ? node.Name : entry.Service.Address;

Was able to fix the exception by replacing node.Name with node.Address.

Thought of editing the code and sending a pull request, but since I do not understand why there is a null reference check on the node object (being new to both Ocelot and Consul), I decided better to submit an issue instead, so someone with more knowledge regarding this code logic can apply a proper fix.

Versions:
.Net Core 8
Ocelot 23.3.6 (nuget package)
Ocelot.Provider.Consul 23.3.6 (nuget package)
Consul 1.20.0
Windows 10 (os of development environment)

[raman-m]

Hello, sir! What's your full name? What are your social networks?
Thanks for your curiosity about Ocelot!

=> node != null ? node.Name : entry.Service.Address;

Was able to fix the exception by replacing node.Name with node.Address.

Wow! I love C# developers who are able to debug! Really. 😉

---

[raman-m]

Expected Behavior:
Ocelot.Provider.Consul works once properly configured as described at:
https://ocelot.readthedocs.io/en/latest/features/servicediscovery.html

I have a concern, guessing you didn't read the whole document where you can find the ready solution for curious developers and users. So please read carefully the following feature documentation:

• Consul Service Builder
• and especially our quick fix as a recommended solution: AddConsul<T> method

Finally, this overridden method will help you:

    protected override string GetDownstreamHost(ServiceEntry entry, Node node) => entry.Service.Address;

P.S. I recommend reading our official Release Notes for v23.3, where we announced a new feature:

• 23.3.0 → What's new? → Service Discovery → Consul Service Builder

@pkutsch Anything else?

[raman-m]

Thought of editing the code and sending a pull request, but since I do not understand why there is a null reference check on the node object (being new to both Ocelot and Consul)

Well... Please note that since version 13.5.2 and #909, when the node name became the host name as Ocelot's best practice, now since v23.3, we configure Consul service building by the proposed #2067 feature aka DefaultConsulServiceBuilder class. So, the subject of discussion:

Ocelot/src/Ocelot.Provider.Consul/DefaultConsulServiceBuilder.cs

Lines 96 to 97 in da9d6fa

	protected virtual string GetDownstreamHost(ServiceEntry entry, Node node)
	=> node != null ? node.Name : entry.Service.Address;

I know this check is not ideal and looks strange. But what do you propose? Have an idea in mind? What are the best practices for organizing Consul services into a node group? I have the following idea, which makes real sense:

-     => node != null ? node.Name : entry.Service.Address; 
+     => !string.IsNullOrEmpty(node?.Name) && !node.Name.Equals("default") ? node.Name : entry.Service.Address;

So, my idea is to identify more practical use cases when DevOps engineers organize services into a Consul node and let this be a best practice in Ocelot integration with Consul.

I decided better to submit an issue instead, so someone with more knowledge regarding this code logic can apply a proper fix.

Yes, please! But I guess this PR will be a micro one: we are limited to the re-implementation of the virtual GetDownstreamHost method only. Let's discuss my idea while honoring yours...

[raman-m]

@pkutsch Where are you?

[raman-m]

@pkutsch @ggnaegi What is the default name of the single node group in Consul after installation? I mean, the default settings. Is this name empty, or does it have some predefined value? I agree that users (not C# developers) probably want a JSON option in ocelot.json. But for C# developers, there are no problems at all: DefaultConsulServiceBuilder can be customized in any way.

P.S.

I am not interested in engaging in any discussions here. I believe we cannot enhance further in this context. The DNS probe check should be delivered in #2208. This issue will be closed because the author has received the desired answer with the actual C# recipe