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

Change ClientAddr to default to BindAddr when not present. #2786

Merged
merged 2 commits into from
Mar 6, 2017

Conversation

sean-
Copy link
Contributor

@sean- sean- commented Mar 6, 2017

With this change, it is now possible to only specify the -bind or
bind_addr attributes and get a functioning consul agent.

There are two possible paths from this PR:

  1. Set BindAddr's default to {{ GetPrivateIP }}, which will return an empty string and fail downstream if a private IP isn't available.
  2. Leave things as the status quo for the time being until we move to something that's 100% backwards compatible.

I researched option #2 but put a pin in that because it brought a pile of additional work. The rough sketch of what I had in mind to satisfy option #2 would be to:

  • introduce a -listen flag and config option which would contain one or more IP addresses that would be used for all services.
  • introduce similar -listen-http, -listen-serf that would be the more granular per-service overrides, but similar to -listen, also zero or more.
  • deprecate -advertise-addr and -advertise-addr-wan and tagged addresses
  • introduce a mapping scheme that would allow for NAT'ing Consul's various addresses before values are either queried or leaked into Consul/Serf. This was the biggest lift, but it simplifies the UX considerably.

The thought experiment went like this (JSON5 used in this example for comments-sake):

{
  // or simply "nat"
  "nat_rules": {
    "dc": {
      // Like _agent in prepared queries, _dc would be an alias for the current datacenter.  In this case, any query for the http or https services would be static NAT'ed to the following two endpoints.
      "_dc": {
        "http": "1.2.3.4:8600",
        "https": "6.7.8.9:8600"
      }
    }
    "agent": {
      "node1": { /* node1's override */ },
      "*._dc": { /* I contemplated a glob and removing the agent nesting... */ },
    },
  }
}

But again, that was a big lift potentially and it was easier to just set two defaults. It'd be nice to clean this all up in a unified manner, however, and a thrust in this direction seemed like a reasonable approach.

Regarding the PR, I considered a fallback to 127.0.0.1, but that never seemed like a good idea in a normal production environment. If someone wants 127.0.0.1, they should explicitly set it. Merge or discard as you see fit.

With this change, it is now possible to only specify the `-bind` or
`bind_addr` attributes and get a functioning consul agent.
@sean- sean- requested a review from slackpad March 6, 2017 18:53
Copy link
Contributor

@slackpad slackpad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a good way forward for now - we definitely don't have time to make a larger change for the 0.8 release. One note on the docs and otherwise LGTM.

@@ -115,8 +115,9 @@ will exit with an error at startup.
[`-bind` command-line flag](#_bind), and if this is not specified, the `-bind` option is used. This is available in Consul 0.7.1 and later.

* <a name="_client"></a><a href="#_client">`-client`</a> - The address to which
Consul will bind client interfaces, including the HTTP and DNS servers. By default,
this is "127.0.0.1", allowing only loopback connections.
Consul will bind client interfaces, including the HTTP and DNS servers. When
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you qualify these with "in Consul 0.8 or later this is X, and previously it was Y".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants