Add --address option to set registered service address

[davedittrich]

When ianitor is used on a host that is not running a Consul agent, and you use --consul-agent to connect to a remote agent or peer to register the service, the address that was registered was the remote Consul agent or peer, not the host where the service resides. This option allows you to specify the IP address to use, as well as the port, in this circumstance.

[swistakm]

Thanks for the contribution. I seems that something is wrong with current travis configuration so I will have to fix that and ask you to rebase your PR afterwards.

In the meantime you need to give me a meaningful use case for this feature. On initial design we purposefully missed the option to explicitly set the address of registered service in such "remote-agent" scenario. Here are the reasons for that:

• this is error prone
• this encourages to make bad design decisions
• this seems to be anti pattern in consul usage
• this makes things more complex

Before going deeper into detail I rush to say that I already failed in effort to not encourage to make bad decisions. I think that allowing to set remote address as --consul-agent argument was a bad design idea in the first place. It makes users think that setting consul agent address as a remote address is a reasonable option. It was only intended to use with local agent but allowing it to be bound on different addresses like:

ianitor --consul-agent 127.0.0.1 -- ...
ianitor --consul-agent 0.0.0.0 -- ...

See, the typical scenario for consul deployment is to setup agent on your every instance. Consul run in the 'client' mode is very lightweight and does not cost you much of the resources. People are successfully running consul clusters with thousands of nodes because it was simply designed for such scenario.

Including the service's address in its own invocation (so it becomes hosts configuration) is in my honest opinion an anti-pattern. If you require the service to have fixed address backed in its own configuration then you are undermining the whole idea of service discovery (especially using consul). Having consul agent on the same host is the right way of doing that.

Also, see what gives you the ianitor right now. Every host running the service can now have the same invocation regardless of it's location:

ianitor frontend --port 80 -- some_web_server --bind 80

No matter if it's 1, 10, or 10k hosts. This is because we assume that you should contact with the local consul agent that will handle the registration of service in the cluster. When you make 'ianitor' to contact with remote agent then you end up with requirement to provide unique configuration for every host. Today, it is easily achievable thanks to existing configuration management tools (puppet, chef, ansible, whatever) but still:

• requires extra effort
• makes things more complex
• creates more area for error and mistakes

And the most important issue: renders 'ianitor' useless. IMO it is better to use JSON in this such situation because 'ianitor' will not provide you any extra value in this kind of deployment.

I don't say that this feature is definitely bad but I need to have a realistic example of situation where it is really not reasonable to have consul agent installed on the same host as the service run by ianitor.
I know that this change seems to be very trivial. Also the Python version of consul client we use, makes such things trivial as well. But I'm afraid that by including it we will make easier to make bad architectural decisions for ianitor users.

[swistakm]

After long time I think that maybe it is worth reconsidering again. I have rebased it on the separate branch to make sure tests pass.

[inventionlabsSydney]

Hi,
I'd like to add that there's good use case for this,
Whilst the pattern for running consul agents across all instances was how consul originally orientated it's stack, there's cases like minimal docker images (I'm doing single alpine images running just the code I need executed) where running an agent and the script is just cumbersome and unnecessary.

As such, I think this should be merged as an option.

Thanks! :)

[swistakm]

Sorry, I was sick and forgot about this. I could merge it and release but don't remember why this build failed.

[swistakm]

closing in favor of #18

[swistakm]

Sorry folks it took so long. I could not find out what was the reason of CI failures, so I forgot about this issue. This feature is now merged (from #18) and released as 0.1.0