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

Add a possibility to change service name dynamically because of Consul tags #1188

Open
mantasaudickas opened this issue Apr 10, 2020 · 10 comments · May be fixed by #1198
Open

Add a possibility to change service name dynamically because of Consul tags #1188

mantasaudickas opened this issue Apr 10, 2020 · 10 comments · May be fixed by #1198
Assignees
Labels
accepted Bug or feature would be accepted as a PR or is being worked on Consul Service discovery by Consul Dependency Injection Ocelot feature: Dependency Injection Service Discovery Ocelot feature: Service Discovery

Comments

@mantasaudickas
Copy link

mantasaudickas commented Apr 10, 2020

Hello,

At the moment service name is chosen based on first segment in the path.
I would like to be able to map one (or multiple) path segments to service name in Consul.

Few examples:
authorization => would mapped to authorization.service.web
authorization/api => would be mapped to authorization.service.api

Reason for this, that my routes are defined in Consul tags and I would like to use them as a key when selecting service name in Consul.
At the moment to achieve this I have to replace whole DownstreamRouteCreator class, while I would be keen to keep custom part as small as possible.

Would you mind if I make a PR for this?

So something like:

    public interface IServiceNameProvider
    {
        string GetServiceName(string upstreamUrlPath);
    }

    public class ServiceNameProvider: IServiceNameProvider
    {
        public string GetServiceName(string upstreamUrlPath)
        {
            if (upstreamUrlPath.IndexOf('/', 1) == -1)
            {
                return upstreamUrlPath
                    .Substring(1);
            }

            return upstreamUrlPath
                .Substring(1, upstreamUrlPath.IndexOf('/', 1))
                .TrimEnd('/');
        }
    }

and use this such provider in CustomDownstreamRouteCreator class.

with best regards

mantasaudickas pushed a commit to mantasaudickas/Ocelot that referenced this issue Apr 14, 2020
mantasaudickas pushed a commit to mantasaudickas/Ocelot that referenced this issue Apr 14, 2020
mantasaudickas pushed a commit to mantasaudickas/Ocelot that referenced this issue Apr 16, 2020
mantasaudickas pushed a commit to mantasaudickas/Ocelot that referenced this issue Apr 16, 2020
raman-m pushed a commit to mantasaudickas/Ocelot that referenced this issue Jul 26, 2023
@raman-m raman-m added the feature A new feature label Jul 26, 2023
@raman-m
Copy link
Member

raman-m commented Jul 27, 2023

Hello, Mantas!
Thanks for your interest in Ocelot!

I see the problem...
Do you have saved Consul configs or maybe screenshots of Consul dashboard?

authorization/api

Do you mean, an extra slash char starts Consul tag?
Is /api a tag in Consul for the service name?

Finally, I see that complete Consul service name template is {servicename}/{tag}. Correct?

Could you share a link to Consul docs regarding tags please?

@raman-m raman-m added the accepted Bug or feature would be accepted as a PR or is being worked on label Jul 27, 2023
@raman-m
Copy link
Member

raman-m commented Jul 27, 2023

+ Accepted

... because of ready PR #1198

@raman-m raman-m changed the title I would like to add a possibility to change service name dynamically Add a possibility to change service name dynamically because of Consul tags Jul 27, 2023
@mantasaudickas
Copy link
Author

Finally, I see that complete Consul service name template is {servicename}/{tag}. Correct?

Not really.. we can have multiple services behind gateway, for example:
/authorization - points to angular project
/authorization/api - points to independently deployed dotnet project
/authorization/api/user - can point to a separate service which is providing user details...
/authorization/jobs - might be some background job service which is executing some background business logic (like read model updates) and also provides some technical API endpoints

So as you can see url starts with the same word.. but further on we can have more microservices running.
When routing - we simply sort these known urls by length and then choose most matching.

@raman-m
Copy link
Member

raman-m commented Jul 28, 2023

OK
I see your team uses path-style to define tags...
In the example above authorization is like a DNS name, right?
I found this Consul docs related to tags: tags | Service configuration reference | Consul | HashiCorp Developer
This doc has these requirements for tag definition:

Tag values are opaque to Consul. We recommend using valid DNS labels for service definition IDs for compatibility with external DNSs.

But I think it is not a stopper for PR logic, because you have not introduced any Consul hardcoded features. Only feature decoupling by interface.

I thought Consul has some requirements and you wish to introduce them in the PR.
But now I understand that you've introduced decoupling of service name getter logic, and then it will be possible to inject any custom service name getter including Consul one.

No questions anymore! 😸

@mantasaudickas
Copy link
Author

In the example above authorization is like a DNS name, right?

Not really. Path is just a key to configuration item. That item has all necessary information about downstream service.

To be honest - now I would do this PR a bit differently. Finder should take HttpRequest as parameter and then you can have various logics in routing where service instance can be identified by:

  • path prefix
  • query parameter
  • host name
  • request header
  • even port

In microservices world where services are created and registered kind of dynamically such kind of flexibility is necessary.
In our infrastructure there are hundreds of microservices running and routing rules might vary quite a lot thus in application gateway a lot of flexibility is required :)

@raman-m
Copy link
Member

raman-m commented Jul 31, 2023

@mantasaudickas commented on July 28:

To be honest - now I would do this PR a bit differently.

Any changes and improvements are welcome in current PR!
I'll be happy to review...


Finder should take HttpRequest as parameter and then you can have various logics in routing where service instance can be identified by:

  • path prefix
  • query parameter
  • host name
  • request header
  • even port

Some of these properties we have in current Get() method of the IDownstreamRouteProvider interface 👇

public Response<DownstreamRouteHolder> Get(
    string upstreamUrlPath, 
    string upstreamQueryString, 
    string upstreamHttpMethod, 
    IInternalConfiguration configuration, 
    string upstreamHost)
{ }

So, any additional objects vs data will require to change the IDownstreamRouteProvider interface.
Think twice before making such kind of code changes.

Will you push some commit soon?

@mantasaudickas
Copy link
Author

Will you push some commit soon?

Sorry, no chance..

But you can do it on your own.. just IDownstreamServiceFinder should have this signature:

string GetServiceName(HttpRequest request)

raman-m pushed a commit to mantasaudickas/Ocelot that referenced this issue Oct 12, 2023
raman-m pushed a commit to mantasaudickas/Ocelot that referenced this issue Feb 18, 2024
@raman-m
Copy link
Member

raman-m commented Mar 23, 2024

@ggnaegi Your opinion, Gui?
Will this feature be useful for you and the community?

raman-m pushed a commit to mantasaudickas/Ocelot that referenced this issue Apr 5, 2024
@raman-m
Copy link
Member

raman-m commented Jun 15, 2024

@ggnaegi Seems it is not useful anymore after #2067 delivery.

raman-m pushed a commit to mantasaudickas/Ocelot that referenced this issue Nov 11, 2024
@raman-m
Copy link
Member

raman-m commented Nov 11, 2024

Dear @mantasaudickas !

Reason for this, that my routes are defined in Consul tags and I would like to use them as a key when selecting service name in Consul.

Since the new behavior is necessary for service discovery, you will need to consider a new design approach and develop a Custom Provider tailored to your requirements.

At the moment to achieve this I have to replace whole DownstreamRouteCreator class, while I would be keen to keep custom part as small as possible.

There's no need to replace the entire DownstreamRouteCreator class to achieve this. Instead, you can simply create a Custom Provider that builds upon the Consul class. I also suggest utilizing the Consul Service Builder to customize Consul's behavior by overriding its virtual methods. In conclusion, your solution should incorporate elements of both the Custom Provider and the Consul Service Builder.

@ggnaegi FYI

@raman-m raman-m added Service Discovery Ocelot feature: Service Discovery Consul Service discovery by Consul Dependency Injection Ocelot feature: Dependency Injection and removed feature A new feature labels Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Bug or feature would be accepted as a PR or is being worked on Consul Service discovery by Consul Dependency Injection Ocelot feature: Dependency Injection Service Discovery Ocelot feature: Service Discovery
Projects
None yet
2 participants