From af2901130db3a55775b4622cdf8fb3d4acac8130 Mon Sep 17 00:00:00 2001 From: Paul Banks Date: Fri, 13 Jul 2018 22:38:35 +0100 Subject: [PATCH 1/3] Implement missing HTTP host to ConsulResolver func for Connect SDK. --- connect/resolver.go | 93 ++++++++++++++++++++++++++++++++++ connect/resolver_test.go | 107 +++++++++++++++++++++++++++++++++++++++ connect/service.go | 9 ++-- connect/service_test.go | 25 +++++++++ 4 files changed, 230 insertions(+), 4 deletions(-) diff --git a/connect/resolver.go b/connect/resolver.go index 2923b81f33d4..67f876fb80e0 100644 --- a/connect/resolver.go +++ b/connect/resolver.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "math/rand" + "strings" "sync" "github.com/hashicorp/consul/agent/connect" @@ -202,3 +203,95 @@ func (cr *ConsulResolver) queryOptions(ctx context.Context) *api.QueryOptions { } return q.WithContext(ctx) } + +// ConsulResolverFromAddrFunc returns a function for constructing ConsulResolver +// from a consul DNS formatted hostname (e.g. foo.service.consul or +// foo.query.consul). +// +// Note, the returned ConsulResolver resolves the query via regular agent HTTP +// discovery API. DNS is not needed or used for discovery, only the hostname +// format re-used for consistency. +func ConsulResolverFromAddrFunc(client *api.Client) func(addr string) (Resolver, error) { + // Capture client dependency + return func(addr string) (Resolver, error) { + // Http clients might provide hostname and port + host := strings.ToLower(stripPort(addr)) + + // For now we force use of `.consul` TLD regardless of the configured domain + // on the cluster. That's because we don't know that domain here and it + // would be really complicated to discover it inline here. We do however + // need to be able to distingush a hostname with the optional datacenter + // segment which we can't do unambiguously if we allow arbitrary trailing + // domains. + domain := ".consul" + if !strings.HasSuffix(host, domain) { + return nil, fmt.Errorf("invalid Consul DNS domain: note Connect SDK " + + "currently requires use of .consul domain even if cluster is " + + "configured with a different domain.") + } + + // Remove the domain suffix + host = host[0 : len(host)-len(domain)] + + parts := strings.Split(host, ".") + numParts := len(parts) + + r := &ConsulResolver{ + Client: client, + Namespace: "default", + } + + // Note that 3 segments may be a valid DNS name like + // ..service.consul but not one we support, it might also be + // .service..consul which we do want to support so we + // have to figure out if the last segment is supported keyword and if not + // check if the supported keyword is further up... + + // To simplify logic for now, we must match one of the following (not domain + // is stripped): + // .[service|query] + // .[service|query]. + if numParts < 2 || numParts > 3 || !supportedTypeLabel(parts[1]) { + return nil, fmt.Errorf("unsupported Consul DNS domain: must be either " + + ".service[.].consul or " + + ".query[.].consul") + } + + if numParts == 3 { + // Must be datacenter case + r.Datacenter = parts[2] + } + + // By know we must have a supported query type which means at least 2 + // elements with first 2 being name, and type respectively. + r.Name = parts[0] + switch parts[1] { + case "service": + r.Type = ConsulResolverTypeService + case "query": + r.Type = ConsulResolverTypePreparedQuery + default: + // This should never happen (tm) unless the supportedTypeLabel + // implementation is changed and this switch isn't. + return nil, fmt.Errorf("invalid discovery type") + } + + return r, nil + } +} + +func supportedTypeLabel(label string) bool { + return label == "service" || label == "query" +} + +// stripPort copied from net/url/url.go +func stripPort(hostport string) string { + colon := strings.IndexByte(hostport, ':') + if colon == -1 { + return hostport + } + if i := strings.IndexByte(hostport, ']'); i != -1 { + return strings.TrimPrefix(hostport[:i], "[") + } + return hostport[:colon] +} diff --git a/connect/resolver_test.go b/connect/resolver_test.go index 6a1ca983be3c..5f319eb77023 100644 --- a/connect/resolver_test.go +++ b/connect/resolver_test.go @@ -216,3 +216,110 @@ func TestConsulResolver_Resolve(t *testing.T) { }) } } + +func TestConsulResolverFromAddrFunc(t *testing.T) { + // Don't need an actual instance since we don't do the service discovery but + // we do want to assert the client is pass through correctly. + client, err := api.NewClient(api.DefaultConfig()) + require.NoError(t, err) + + tests := []struct { + name string + addr string + want Resolver + wantErr string + }{ + { + name: "service", + addr: "foo.service.consul", + want: &ConsulResolver{ + Client: client, + Namespace: "default", + Name: "foo", + Type: ConsulResolverTypeService, + }, + }, + { + name: "query", + addr: "foo.query.consul", + want: &ConsulResolver{ + Client: client, + Namespace: "default", + Name: "foo", + Type: ConsulResolverTypePreparedQuery, + }, + }, + { + name: "service with dc", + addr: "foo.service.dc2.consul", + want: &ConsulResolver{ + Client: client, + Datacenter: "dc2", + Namespace: "default", + Name: "foo", + Type: ConsulResolverTypeService, + }, + }, + { + name: "query with dc", + addr: "foo.query.dc2.consul", + want: &ConsulResolver{ + Client: client, + Datacenter: "dc2", + Namespace: "default", + Name: "foo", + Type: ConsulResolverTypePreparedQuery, + }, + }, + { + name: "invalid host:port", + addr: "%%%", + wantErr: "invalid Consul DNS domain", + }, + { + name: "custom domain", + addr: "foo.service.my-consul.com", + wantErr: "invalid Consul DNS domain", + }, + { + name: "unsupported query type", + addr: "foo.connect.consul", + wantErr: "unsupported Consul DNS domain", + }, + { + name: "unsupported query type and datacenter", + addr: "foo.connect.dc1.consul", + wantErr: "unsupported Consul DNS domain", + }, + { + name: "unsupported query type and datacenter", + addr: "foo.connect.dc1.consul", + wantErr: "unsupported Consul DNS domain", + }, + { + name: "unsupported tag filter", + addr: "tag1.foo.service.consul", + wantErr: "unsupported Consul DNS domain", + }, + { + name: "unsupported tag filter with DC", + addr: "tag1.foo.service.dc1.consul", + wantErr: "unsupported Consul DNS domain", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + require := require.New(t) + + fn := ConsulResolverFromAddrFunc(client) + got, gotErr := fn(tt.addr) + if tt.wantErr != "" { + require.Error(gotErr) + require.Contains(gotErr.Error(), tt.wantErr) + } else { + require.NoError(gotErr) + require.Equal(tt.want, got) + } + }) + } +} diff --git a/connect/service.go b/connect/service.go index 462f1fb472c6..7bdaf3c60ffc 100644 --- a/connect/service.go +++ b/connect/service.go @@ -69,10 +69,11 @@ func NewService(serviceName string, client *api.Client) (*Service, error) { func NewServiceWithLogger(serviceName string, client *api.Client, logger *log.Logger) (*Service, error) { s := &Service{ - service: serviceName, - client: client, - logger: logger, - tlsCfg: newDynamicTLSConfig(defaultTLSConfig()), + service: serviceName, + client: client, + logger: logger, + tlsCfg: newDynamicTLSConfig(defaultTLSConfig()), + httpResolverFromAddr: ConsulResolverFromAddrFunc(client), } // Set up root and leaf watches diff --git a/connect/service_test.go b/connect/service_test.go index b18bf4bb414f..ce3b7c7a7213 100644 --- a/connect/service_test.go +++ b/connect/service_test.go @@ -252,3 +252,28 @@ func TestService_HTTPClient(t *testing.T) { } }) } + +func TestService_HasDefaultHTTPResolverFromAddr(t *testing.T) { + + client, err := api.NewClient(api.DefaultConfig()) + require.NoError(t, err) + + s, err := NewService("foo", client) + require.NoError(t, err) + + // Sanity check this is actually set in constructor since we always override + // it in tests. Full tests of the resolver func are in resolver_test.go + require.NotNil(t, s.httpResolverFromAddr) + + fn := s.httpResolverFromAddr + + expected := &ConsulResolver{ + Client: client, + Namespace: "default", + Name: "foo", + Type: ConsulResolverTypeService, + } + got, err := fn("foo.service.consul") + require.NoError(t, err) + require.Equal(t, expected, got) +} From dcd157ec7cd18810af3e512567e5fb2250f64b72 Mon Sep 17 00:00:00 2001 From: Paul Banks Date: Fri, 13 Jul 2018 23:08:26 +0100 Subject: [PATCH 2/3] Add notes about hostname gotchas to Connect HTTPClient docs --- website/source/docs/connect/native/go.html.md | 27 ++++++++++++++++--- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/website/source/docs/connect/native/go.html.md b/website/source/docs/connect/native/go.html.md index 467390699d58..e2e45358353f 100644 --- a/website/source/docs/connect/native/go.html.md +++ b/website/source/docs/connect/native/go.html.md @@ -157,14 +157,33 @@ The HTTP client configuration automatically sends the correct client certificate, verifies the server certificate, and manages background goroutines for updating our certificates as necessary. --> **Important:** The HTTP client _requires_ the hostname is a Consul -DNS name. Static IP addresses and external DNS cannot be used with the -HTTP client. For these values, please use `svc.Dial` directly. - If the application already uses a manually constructed `*http.Client`, the `svc.HTTPDialTLS` function can be used to configure the `http.Transport.DialTLS` field to achieve equivalent behavior. +### Hostname Requirements + +The hostname used in the request URL is used to identify the logical service +discovery mechanism for the target. **It's not actually resolved via DNS** but +used as a logical identifier for a Consul service discovery mechanism. It has +the following specific limitations: + + * The sheme must be `https://`. + * It must be a Consul DNS name in one of the following forms: + * `.service[.].consul` to discover a healthy service + instance for a given service. + * `.query[.].consul` to discover an instance via + [Prepared Query](/api/query.html). + * The top-level domain _must_ be `.consul` even if your cluster has a custom + `domain` configured for it's DNS interface. This might be relaxed in the + future. + * Tag filters for services are not currently supported (i.e. + `tag1.web.service.consul`) however the same behaviour can be acheived using a + prepared query. + * External DNS names, raw IP addresses and so on will cause an error and should + be fetched using a separate `HTTPClient`. + + ## Raw TLS Connection For a raw `net.Conn` TLS connection, the `svc.Dial` function can be used. From fa29fee4b6a1353c565fdc5067e5ba08e2c0229f Mon Sep 17 00:00:00 2001 From: Paul Banks Date: Fri, 13 Jul 2018 23:09:34 +0100 Subject: [PATCH 3/3] Typos --- website/source/docs/connect/native/go.html.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/website/source/docs/connect/native/go.html.md b/website/source/docs/connect/native/go.html.md index e2e45358353f..ad59caca3cd1 100644 --- a/website/source/docs/connect/native/go.html.md +++ b/website/source/docs/connect/native/go.html.md @@ -168,17 +168,17 @@ discovery mechanism for the target. **It's not actually resolved via DNS** but used as a logical identifier for a Consul service discovery mechanism. It has the following specific limitations: - * The sheme must be `https://`. + * The scheme must be `https://`. * It must be a Consul DNS name in one of the following forms: * `.service[.].consul` to discover a healthy service instance for a given service. - * `.query[.].consul` to discover an instance via + * `.query[.].consul` to discover an instance via [Prepared Query](/api/query.html). * The top-level domain _must_ be `.consul` even if your cluster has a custom `domain` configured for it's DNS interface. This might be relaxed in the future. * Tag filters for services are not currently supported (i.e. - `tag1.web.service.consul`) however the same behaviour can be acheived using a + `tag1.web.service.consul`) however the same behaviour can be achieved using a prepared query. * External DNS names, raw IP addresses and so on will cause an error and should be fetched using a separate `HTTPClient`.