From c11f5f9b72daae98c7f8df035fc1de40193cbf0c Mon Sep 17 00:00:00 2001 From: trotttrotttrott Date: Tue, 13 Feb 2018 22:05:47 +0200 Subject: [PATCH 1/5] Consul service address is blank Setting an explicit service address eliminates the ability for Consul to dynamically decide what it should be based on its translate_wan_addrs setting. translate_wan_addrs configures Consul to return its lan address to nodes in its same datacenter but return its wan address to nodes in foreign datacenters. --- physical/consul/consul.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/physical/consul/consul.go b/physical/consul/consul.go index 4609d9eff4bc..e8db6b359e5b 100644 --- a/physical/consul/consul.go +++ b/physical/consul/consul.go @@ -731,7 +731,7 @@ func (c *ConsulBackend) reconcileConsul(registeredServiceID string, activeFunc p Name: c.serviceName, Tags: tags, Port: int(c.redirectPort), - Address: c.redirectHost, + Address: "", EnableTagOverride: false, } From d26f3327e70f57377a36dbe1cc9f1f0e95a990d1 Mon Sep 17 00:00:00 2001 From: trotttrotttrott Date: Thu, 22 Feb 2018 12:00:09 +0100 Subject: [PATCH 2/5] service_address parameter for Consul storage backend This parameter allows users to override the use of what Vault knows to be its HA redirect address. This option is particularly commpelling because if set to a blank string, Consul will leverage the node configuration where the service is registered which includes the `translate_wan_addrs` option. This option conditionally associates nodes' lan or wan address based on where requests originate. --- physical/consul/consul.go | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/physical/consul/consul.go b/physical/consul/consul.go index e8db6b359e5b..ec78427fd3d8 100644 --- a/physical/consul/consul.go +++ b/physical/consul/consul.go @@ -92,6 +92,7 @@ type ConsulBackend struct { redirectPort int64 serviceName string serviceTags []string + serviceAddress *string disableRegistration bool checkTimeout time.Duration consistencyMode string @@ -150,11 +151,22 @@ func NewConsulBackend(conf map[string]string, logger log.Logger) (physical.Backe // Get the additional tags to attach to the registered service name tags := conf["service_tags"] - if logger.IsDebug() { logger.Debug("physical/consul: config service_tags set", "service_tags", tags) } + // Get the service-specific address to override the use of the HA redirect address + var serviceAddr *string + serviceAddrStr, ok := conf["service_address"] + if ok { + serviceAddr = &serviceAddrStr + } else { + serviceAddr = nil + } + if logger.IsDebug() { + logger.Debug("physical/consul: config service_address set", "service_address", serviceAddr) + } + checkTimeout := defaultCheckTimeout checkTimeoutStr, ok := conf["check_timeout"] if ok { @@ -247,6 +259,7 @@ func NewConsulBackend(conf map[string]string, logger log.Logger) (physical.Backe permitPool: physical.NewPermitPool(maxParInt), serviceName: service, serviceTags: strutil.ParseDedupLowercaseAndSortStrings(tags, ","), + serviceAddress: serviceAddr, checkTimeout: checkTimeout, disableRegistration: disableRegistration, consistencyMode: consistencyMode, @@ -726,12 +739,21 @@ func (c *ConsulBackend) reconcileConsul(registeredServiceID string, activeFunc p return serviceID, nil } + // If service address was set explicitly in configuration, use that + // as the service-specific address instead of the HA redirect address. + var serviceAddress string + if c.serviceAddress == nil { + serviceAddress = c.redirectHost + } else { + serviceAddress = *c.serviceAddress + } + service := &api.AgentServiceRegistration{ ID: serviceID, Name: c.serviceName, Tags: tags, Port: int(c.redirectPort), - Address: "", + Address: serviceAddress, EnableTagOverride: false, } From 16e61d753ae0d8ca2af95ca1d54a40d5a8283903 Mon Sep 17 00:00:00 2001 From: trotttrotttrott Date: Thu, 22 Feb 2018 12:50:13 +0100 Subject: [PATCH 3/5] Add TestConsul_ServiceAddress Ensures that the service_address configuration parameter is setting the serviceAddress field of ConsulBackend instances properly. If the "service_address" parameter is not set, the ConsulBackend serviceAddress field must instantiate as nil to indicate that it can be ignored. --- physical/consul/consul_test.go | 45 ++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/physical/consul/consul_test.go b/physical/consul/consul_test.go index 8be1fe956d21..7b1d1f4dc84c 100644 --- a/physical/consul/consul_test.go +++ b/physical/consul/consul_test.go @@ -117,6 +117,51 @@ func TestConsul_ServiceTags(t *testing.T) { } } +func TestConsul_ServiceAddress(t *testing.T) { + tests := []struct { + consulConfig map[string]string + serviceAddrNil bool + }{ + { + consulConfig: map[string]string{ + "service_address": "", + }, + }, + { + consulConfig: map[string]string{ + "service_address": "vault.example.com", + }, + }, + { + serviceAddrNil: true, + }, + } + + for _, test := range tests { + logger := logformat.NewVaultLogger(log.LevelTrace) + + be, err := NewConsulBackend(test.consulConfig, logger) + if err != nil { + t.Fatalf("expected Consul to initialize: %v", err) + } + + c, ok := be.(*ConsulBackend) + if !ok { + t.Fatalf("Expected ConsulBackend") + } + + if test.serviceAddrNil { + if c.serviceAddress != nil { + t.Fatalf("expected service address to be nil") + } + } else { + if c.serviceAddress == nil { + t.Fatalf("did not expect service address to be nil") + } + } + } +} + func TestConsul_newConsulBackend(t *testing.T) { tests := []struct { name string From 905dd864c2f9acc8c250545256bdf4fa9ae793e1 Mon Sep 17 00:00:00 2001 From: trotttrotttrott Date: Thu, 22 Feb 2018 13:13:42 +0100 Subject: [PATCH 4/5] Add service_address to Consul storage backend docs --- website/source/docs/configuration/storage/consul.html.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/website/source/docs/configuration/storage/consul.html.md b/website/source/docs/configuration/storage/consul.html.md index b8f0ac3510e4..e327fe3696fe 100644 --- a/website/source/docs/configuration/storage/consul.html.md +++ b/website/source/docs/configuration/storage/consul.html.md @@ -86,6 +86,14 @@ at Consul's service discovery layer. - `service_tags` `(string: "")` – Specifies a comma-separated list of tags to attach to the service registration in Consul. +- `service_address` `(string: nil)` – Specifies a service-specific address to + set on the service registration in Consul. If unset, Vault will use what it + knows to be the HA redirect address - which is usually desirable. Setting + this parameter to `""` will tell Consul to leverage the configuration of the + node the service is registered on dynamically. This could be beneficial if + you intend to leverage Consul's + [`translate_wan_addrs`](consul-translate-wan-addrs) parameter. + - `token` `(string: "")` – Specifies the [Consul ACL token][consul-acl] with permission to read and write from the `path` in Consul's key-value store. This is **not** a Vault token. See the ACL section below for help. @@ -216,3 +224,4 @@ storage "consul" { [consul-acl]: https://www.consul.io/docs/guides/acl.html "Consul ACLs" [consul-consistency]: https://www.consul.io/api/index.html#consistency-modes "Consul Consistency Modes" [consul-encryption]: https://www.consul.io/docs/agent/encryption.html "Consul Encryption" +[consul-translate-wan-addrs]: https://www.consul.io/docs/agent/options.html#translate_wan_addrs "Consul Configuration" From 255557fae635642ce2a3acd86cb45d5e31624da0 Mon Sep 17 00:00:00 2001 From: trotttrotttrott Date: Thu, 22 Feb 2018 18:12:40 +0100 Subject: [PATCH 5/5] Remove else block that uncessarily sets nil value *string variables are inherently nil when instantiated so there is no need to explicitly set serviceAddr to nil. --- physical/consul/consul.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/physical/consul/consul.go b/physical/consul/consul.go index ec78427fd3d8..f479782baf28 100644 --- a/physical/consul/consul.go +++ b/physical/consul/consul.go @@ -160,8 +160,6 @@ func NewConsulBackend(conf map[string]string, logger log.Logger) (physical.Backe serviceAddrStr, ok := conf["service_address"] if ok { serviceAddr = &serviceAddrStr - } else { - serviceAddr = nil } if logger.IsDebug() { logger.Debug("physical/consul: config service_address set", "service_address", serviceAddr)