rogeralsing / Microphone

Apache License 2.0
461 stars 71 forks source link

Added missing parameter to surface host URL option to ASP.NET. #37

Closed Mikular closed 8 years ago

Mikular commented 8 years ago

Completely forgot to add this, but this is what I actually need to use it in my code. Sorry about the delay! If at all possible, a quick merge and publish (if you're satisfied with the changes) would be amazing.

Mikular commented 8 years ago

@rogeralsing Hi Roger. It's actually Microphone.AspNet/Extensions.UseMicrophone that I need to update. However, before I do this, (1) apologies for the many pull requests, (2) I'm actually not sure that putting an optional boolean in there makes sense. Can you think of a better way to achieve what I'm trying to do?

The route of this problem is that due to my network setup, my service registers itself with the IP 10.0.0.5 - which it is not on. Consul can then never connect for health checks etc.

rogeralsing commented 8 years ago

can you show an example on how you would like it to look and I'll see what I can come up with?

rogeralsing commented 8 years ago

I the specific case of Asp.NET Core, I think allowing to plugin some sort of Uri provider would be the most idiomatic.

e.g.

 public void ConfigureServices(IServiceCollection services)
        {
            services.AddMvc();
            services.AddMicrophone<ConsulProvider>();
            services.AddUriResolver<MyUriResolver>();
        }

Something like that. and then let the consul provider consume a uri resolver so it can register the services on the correct uri. Does that sound about right?

Mikular commented 8 years ago

So if UseMicrophone were to be changed to the following:

    public static IApplicationBuilder UseMicrophone(this IApplicationBuilder self, string serviceName, string version, bool useUriHost = false)
    {
        var loggingFactory = self.ApplicationServices.GetRequiredService<ILoggerFactory>();
        var clusterProvider = self.ApplicationServices.GetRequiredService<IClusterProvider>();
        var logger = loggingFactory.CreateLogger("Microphone.AspNet");
        try
        {
            var features = self.Properties["server.Features"] as FeatureCollection;
            var addresses = features.Get<IServerAddressesFeature>();
            var address = addresses.Addresses.First().Replace("*", "localhost");
            var uri = new Uri(address);
            Cluster.RegisterService(uri, clusterProvider, serviceName, version, logger, useUriHost);
        }
        catch(Exception x)
        {
            logger.LogCritical(x.ToString());
        }
        return self;
    }

Which would be called like:

app.UseMvc().UseMicrophone("ServiceName", "1.0", true);

That would do what I need it to do - which is to use "localhost" (i.e. 127.0.0.1) as a hostname when registering the service, instead of what Dns.GetHostName() returns. Maybe an alternative is an optional parameter in UseMicrophone for an address name, to replace useUriHost and be used instead of the address local variable, but what do you think?