pszymczyk / embedded-consul

Embedded Consul provides easy way to run Consul in integration tests.
Apache License 2.0
95 stars 29 forks source link

Custom configuration ignores advertise and client #57

Open hughesadam87 opened 6 years ago

hughesadam87 commented 6 years ago

Hi,

This library is great and thank you! Just noticed some behavior that may constitute a bug, or at least a case that needs special handling. I'm configuring an agent from the following custom configuration json:

{
  "datacenter": "tpc",
  "server": true,
  "bind_addr": "127.0.0.2",
  "client_addr": "127.0.0.2",
  "ports": {
    "dns":8600,
    "http":8500,
    "serf_lan":8301,
    "serf_wan":8302,
    "server":8300
  }
}

Most of these properties are obeyed; however, the ConsulStarter's advertise and client fields are not set from the bind_addr and client_addr fields specified by the Consul Config docs. From my config, these should be 127.0.0.2, but in fact they are defaulting to 127.0.0.1.

consulStarter = ConsulStarterBuilder.consulStarter().withCustomConfig(consulConfiguration).build();

image

ruslansennov commented 6 years ago

hi @hugadams ConsulStarterBuilder have default advertise and client fields which are added to the consul command as parameters. It seems these parameters are preferable for the consul process and the corresponding fields in your custom config are ignored. Please try to use the withAdvertise and withClient methods in ConsulStarterBuilder. As an alternative solution we can remove the defaults from the consul command, but I'm afraid it will be uncomfortable for most users

hughesadam87 commented 6 years ago

Hi @ruslansennov ,

Yes, certainly don't want to remove defaults and break backwards compatibility. I can use the withAdvertise/client commands. Perhaps an error could be thrown if both withAdvertise and --bind/--advertise flags are set in customConfig to make this more noticeable?

ruslansennov commented 6 years ago

Perhaps an error could be thrown

I'm not sure. Perhaps we should show a warning