Allow service discovery providers to specify a path prefix #2289
Conversation
|
Hello, @dauthleikr
Which service discovery provider are you referring to? Consul, Kubernetes, Eureka, or Service Fabric? As far as I am aware, these providers do not require any custom service prefixes, with the possible exception of Service Fabric. Therefore, path prefixes can be conveniently specified when defining a route path template. For instance: {
"ServiceName": "someService",
"UpstreamPathTemplate": "/someService/endpoints.someService",
"DownstreamPathTemplate": "/endpoints",
"LoadBalancerOptions": {
"Type": "LeastConnection"
}
}To clarify, I am unable to comprehend your template |
10000
There was a problem hiding this comment.
PR doesn't look professional!
Perhaps I don't fully understand the purpose of this pull request or the specific issue it aims to address for the discovery provider.
And, what's your discovery provider?
Have you reviewed the documentation detailing how to develop a custom service discovery provider?
P.S. According to our development process, each pull request must be linked to a corresponding issue or discussion. However, this pull request lacks any related issues and has not been discussed.
| @@ -16,7 +16,7 @@ public DownstreamRequest(HttpRequestMessage request) | |||
| Scheme = _request.RequestUri.Scheme; | |||
| Host = _request.RequestUri.Host; | |||
| Port = _request.RequestUri.Port; | |||
| AbsolutePath = _request.RequestUri.AbsolutePath; | |||
| Path = _request.RequestUri.AbsolutePath; | |||
There was a problem hiding this comment.
You rename the property, yet the underlying logic remains. What is the purpose of such a renaming?
There was a problem hiding this comment.
Because AbsolutePath is now a relative path, if the prefix exists (the naming would be misleading).
There was a problem hiding this comment.
Misleading for whom? I don't think the 'Absolute' prefix is misleading to anyone, as it is an internal class of Ocelot Core. 😉
| public string AbsolutePath { get; set; } | ||
| public string Path { get; set; } | ||
|
|
||
| public string ServicePathPrefix { get; set; } = string.Empty; |
There was a problem hiding this comment.
To address the issue effectively, you have to share your solution by uploading it to GitHub. Introducing new property should be justified by a compelling reason, particularly when the existing implementation fails to resolve t 10000 he specific use case.
There was a problem hiding this comment.
I cannot upload our custom service discovery implementation to github. Basically our service discovery returns an Uri, which is passed to ocelot as follows:
private static List<Service> AsServiceList(ImmutableArray<ServiceInstance> serviceInstances)
{
return serviceInstances.Select(item =>
{
var uri = new Uri(item.Uri, UriKind.Absolute);
return new Service(item.ServiceName,
new ServiceHostAndPort(uri.Host, uri.Port, uri.Scheme, /* This is the new "prefix" */ uri.AbsolutePath),
item.Id,
"v1",
[]);
}).ToList();
}| { | ||
| var uriBuilder = new UriBuilder | ||
| { | ||
| Port = Port, | ||
| Host = Host, | ||
| Path = AbsolutePath, | ||
| Path = string.IsNullOrEmpty(ServicePathPrefix) ? Path : $"{ServicePathPrefix.TrimEnd('/')}/{Path.TrimStart('/')}", |
There was a problem hiding this comment.
Trimming slashes is not advisable for Ocelot's current functionality for routing features and bugfixes which are related to slash in the path: last slash, omitting slash, Catch All paths.
We recommend verifying this by creating acceptance with and enabling a real service discovery provider.
There was a problem hiding this comment.
The TrimStart merely fixes the case where the Path already starts with a slash - without the TrimStart, we could have two slashes (a hardcoded slash is part of the interpolated string).
| public string DownstreamHost { get; } | ||
| public int DownstreamPort { get; } | ||
| public string Scheme { get; } | ||
| public string PathPrefix { get; } = string.Empty; |
There was a problem hiding this comment.
Have you reviewed the name of the class? 😆
This class should strictly contain information relevant to host construction: scheme, address, and port. Nothing else!
There was a problem hiding this comment.
I agree the naming of the class no longer fits. Do you have a recommendation on how to proceed? Shall I rename the class?
| .Build(); | ||
| var serviceProviderConfig = new ServiceProviderConfigurationBuilder() | ||
| .Build(); | ||
| GivenTheDownStreamUrlIs("http://my.url/prefix/abc?q=123"); |
There was a problem hiding this comment.
Good unit test, but write more acceptance tests please
|
Hi @raman-m, thanks for responding. This is only useful when using a custom service discovery implementation (our case). {
"UpstreamPathTemplate": "/cashpoint/v1/{everything}",
"DownstreamPathTemplate": "/api/v1/Cashpoint/{everything}",
"DownstreamScheme": "https",
"ServiceName": "PaymentApi",
"UpstreamHttpMethod": [],
"SwaggerKey": "payment.api"
},Our software is deployed on customer infrastructure, so "PaymentApi" may run on a dedicated host/container, in which case this change is not needed (simply return host and port through our service discovery). However in some cases, customers may choose to expose "PaymentApi" through a reverse proxy, so the base path of the API is, for example, Now, you might say that we should simply adjust the route in ocelot.json to the following: {
"UpstreamPathTemplate": "/cashpoint/v1/{everything}",
"DownstreamPathTemplate": "/PaymentApi/api/v1/Cashpoint/{everything}",
"DownstreamScheme": "https",
"ServiceName": "PaymentApi",
"UpstreamHttpMethod": [],
"SwaggerKey": "payment.api"
},(notice the change in DownstreamPathTemplate). However that is not suitable for us, because
|
|
Wow! The path prefix has magically disappeared, and now you're telling me about host variations. If the customer's hosts vary, then you must use a service discovery tool to maintain a registry of those hosts.
Please proceed to develop a custom provider, your own one. In this custom provider you can do whatever you want with hosts and paths including prefixes. We are ready to assist you and consult you. |
|
I've decided to close this pull request because it is entirely unnecessary for the Ocelot team. It adds no new functionality, and the user scenario can be handled by any service discovery software. P.S. Because it was a pull request, I can't move it to discussions: only issues can be moved to discussions and vice versa. |
|
@raman-m I am confused on how to connect our service discovery (which resolved to Uri's) to ocelot, as we cannot use the path. I opened this PR hoping you would see it as a minor improvement that helps others which also implement some Uri-based custom service discovery. I was, of course, ready to polish/rework this prototye-PR myself. Though if your insist it is useless to the ocelot team, I can accept that. I guess we have to maintain a private fork :/ |
Once again! Develop your own custom provider.
Good luck! Ocelot is open-source software, so you can copy the code into a private repository. However, in that case, don't expect any consulting or support from us.
If you want to help other, open a discussion first! Seems in appropriate Ideas category please |
New Feature
This allows service discovery provider implementations to specify a path prefix.
Proposed changes
We need this for cases where services run behind a reverse proxy and are exposed like
https://reverse.proxy/someService/endpoints.someServicewould be the "path prefix" in this case.