Fix bug in service-resolver redirects if the destination uses a default resolver. (#6122)

Also:
- add back an internal http endpoint to dump a compiled discovery chain for debugging purposes

Before the CompiledDiscoveryChain.IsDefault() method would test:

- is this chain just one resolver step?
- is that resolver step just the default?

But what I forgot to test:

- is that resolver step for the same service that the chain represents?

This last point is important because if you configured just one config
entry:

    kind = "service-resolver"
    name = "web"
    redirect {
      service = "other"
    }

and requested the chain for "web" you'd get back a **default** resolver
for "other".  In the xDS code the IsDefault() method is used to
determine if this chain is "empty". If it is then we use the
pre-discovery-chain logic that just uses data embedded in the Upstream
object (and still lets the escape hatches function).

In the example above that means certain parts of the xDS code were going
to try referencing a cluster named "web..." despite the other parts of
the xDS code maintaining clusters named "other...".

Author: rboyer

agent/consul/discoverychain/compile_test.go

@@ -23,10 +23,13 @@ func TestCompile_NoEntries_NoInferDefaults(t *testing.T) {
 }
 
 type compileTestCase struct {
-	entries        *structs.DiscoveryChainConfigEntries
-	expect         *structs.CompiledDiscoveryChain // the GroupResolverNodes map should have nil values
-	expectErr      string
-	expectGraphErr bool
+	entries *structs.DiscoveryChainConfigEntries
+	// expect: the GroupResolverNodes map should have nil values
+	expect *structs.CompiledDiscoveryChain
+	// expectIsDefault tests behavior of CompiledDiscoveryChain.IsDefault()
+	expectIsDefault bool
+	expectErr       string
+	expectGraphErr  bool
 }
 
 func TestCompile(t *testing.T) {
@@ -40,7 +43,7 @@ func TestCompile(t *testing.T) {
 		"router with defaults and noop split and resolver": testcase_RouterWithDefaults_WithNoopSplit_WithResolver(),
 		"route bypasses splitter":                          testcase_RouteBypassesSplit(),
 		"noop split":                                       testcase_NoopSplit_DefaultResolver(),
-		"noop split with protocol from proxy defaults":     testcase_NoopSplit_DefaultResolver_ProcotolFromProxyDefaults(),
+		"noop split with protocol from proxy defaults":     testcase_NoopSplit_DefaultResolver_ProtocolFromProxyDefaults(),
 		"noop split with resolver":                         testcase_NoopSplit_WithResolver(),
 		"subset split":                                     testcase_SubsetSplit(),
 		"service split":                                    testcase_ServiceSplit(),
@@ -55,6 +58,7 @@ func TestCompile(t *testing.T) {
 		"resolver with default subset":                     testcase_Resolve_WithDefaultSubset(),
 		"resolver with no entries and inferring defaults":  testcase_DefaultResolver(),
 		"default resolver with proxy defaults":             testcase_DefaultResolver_WithProxyDefaults(),
+		"service redirect to service with default resolver is not a default chain": testcase_RedirectToDefaultResolverIsNotDefaultChain(),
 
 		// TODO(rb): handle this case better: "circular split":                                   testcase_CircularSplit(),
 		"all the bells and whistles": testcase_AllBellsAndWhistles(),
@@ -125,6 +129,7 @@ func TestCompile(t *testing.T) {
 				}
 
 				require.Equal(t, tc.expect, res)
+				require.Equal(t, tc.expectIsDefault, res.IsDefault())
 			}
 		})
 	}
@@ -298,7 +303,7 @@ func testcase_RouterWithDefaults_WithNoopSplit_DefaultResolver() compileTestCase
 	return compileTestCase{entries: entries, expect: expect}
 }
 
-func testcase_NoopSplit_DefaultResolver_ProcotolFromProxyDefaults() compileTestCase {
+func testcase_NoopSplit_DefaultResolver_ProtocolFromProxyDefaults() compileTestCase {
 	entries := newEntries()
 	setGlobalProxyProtocol(entries, "http")
 
@@ -1230,7 +1235,7 @@ func testcase_DefaultResolver() compileTestCase {
 			newTarget("main", "", "default", "dc1"): nil,
 		},
 	}
-	return compileTestCase{entries: entries, expect: expect}
+	return compileTestCase{entries: entries, expect: expect, expectIsDefault: true}
 }
 
 func testcase_DefaultResolver_WithProxyDefaults() compileTestCase {
@@ -1273,7 +1278,47 @@ func testcase_DefaultResolver_WithProxyDefaults() compileTestCase {
 			newTarget("main", "", "default", "dc1"): nil,
 		},
 	}
-	return compileTestCase{entries: entries, expect: expect}
+	return compileTestCase{entries: entries, expect: expect, expectIsDefault: true}
+}
+
+func testcase_RedirectToDefaultResolverIsNotDefaultChain() compileTestCase {
+	entries := newEntries()
+	entries.AddResolvers(
+		&structs.ServiceResolverConfigEntry{
+			Kind: structs.ServiceResolver,
+			Name: "main",
+			Redirect: &structs.ServiceResolverRedirect{
+				Service: "other",
+			},
+		},
+	)
+
+	resolver := newDefaultServiceResolver("other")
+
+	expect := &structs.CompiledDiscoveryChain{
+		Protocol: "tcp",
+		Node: &structs.DiscoveryGraphNode{
+			Type: structs.DiscoveryGraphNodeTypeGroupResolver,
+			Name: "other",
+			GroupResolver: &structs.DiscoveryGroupResolver{
+				Definition:     resolver,
+				Default:        true,
+				ConnectTimeout: 5 * time.Second,
+				Target:         newTarget("other", "", "default", "dc1"),
+			},
+		},
+		Resolvers: map[string]*structs.ServiceResolverConfigEntry{
+			"other": resolver,
+		},
+		Targets: []structs.DiscoveryTarget{
+			newTarget("other", "", "default", "dc1"),
+		},
+		GroupResolverNodes: map[structs.DiscoveryTarget]*structs.DiscoveryGraphNode{
+			newTarget("other", "", "default", "dc1"): nil,
+		},
+	}
+
+	return compileTestCase{entries: entries, expect: expect, expectIsDefault: false /*being explicit here because this is the whole point of this test*/}
 }
 
 func testcase_Resolve_WithDefaultSubset() compileTestCase {

agent/http_oss.go

@@ -88,6 +88,7 @@ func init() {
 	registerEndpoint("/v1/health/state/", []string{"GET"}, (*HTTPServer).HealthChecksInState)
 	registerEndpoint("/v1/health/service/", []string{"GET"}, (*HTTPServer).HealthServiceNodes)
 	registerEndpoint("/v1/health/connect/", []string{"GET"}, (*HTTPServer).HealthConnectServiceNodes)
+	registerEndpoint("/v1/internal/discovery-chain/", []string{"GET"}, (*HTTPServer).InternalDiscoveryChain)
 	registerEndpoint("/v1/internal/ui/nodes", []string{"GET"}, (*HTTPServer).UINodes)
 	registerEndpoint("/v1/internal/ui/node/", []string{"GET"}, (*HTTPServer).UINodeInfo)
 	registerEndpoint("/v1/internal/ui/services", []string{"GET"}, (*HTTPServer).UIServices)

agent/internal_endpoint.go

@@ -0,0 +1,40 @@
+package agent
+
+import (
+	"fmt"
+	"net/http"
+	"strings"
+
+	"github.com/hashicorp/consul/agent/structs"
+)
+
+// InternalDiscoveryChain is helpful for debugging. Eventually we should expose
+// this data officially somehow.
+func (s *HTTPServer) InternalDiscoveryChain(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
+	var args structs.DiscoveryChainRequest
+	if done := s.parse(resp, req, &args.Datacenter, &args.QueryOptions); done {
+		return nil, nil
+	}
+
+	args.Name = strings.TrimPrefix(req.URL.Path, "/v1/internal/discovery-chain/")
+	if args.Name == "" {
+		resp.WriteHeader(http.StatusBadRequest)
+		fmt.Fprint(resp, "Missing chain name")
+		return nil, nil
+	}
+
+	// Make the RPC request
+	var out structs.DiscoveryChainResponse
+	defer setMeta(resp, &out.QueryMeta)
+
+	if err := s.agent.RPC("ConfigEntry.ReadDiscoveryChain", &args, &out); err != nil {
+		return nil, err
+	}
+
+	if out.Chain == nil {
+		resp.WriteHeader(http.StatusNotFound)
+		return nil, nil
+	}
+
+	return out.Chain, nil
+}

agent/structs/discovery_chain.go

@@ -40,11 +40,18 @@ type CompiledDiscoveryChain struct {
 	Targets   []DiscoveryTarget                      `json:",omitempty"`
 }
 
+// IsDefault returns true if the compiled chain represents no routing, no
+// splitting, and only the default resolution.  We have to be careful here to
+// avoid returning "yep this is default" when the only resolver action being
+// applied is redirection to another resolver that is default, so we double
+// check the resolver matches the requested resolver.
 func (c *CompiledDiscoveryChain) IsDefault() bool {
 	if c.Node == nil {
 		return true
 	}
-	return c.Node.Type == DiscoveryGraphNodeTypeGroupResolver && c.Node.GroupResolver.Default
+	return c.Node.Name == c.ServiceName &&
+		c.Node.Type == DiscoveryGraphNodeTypeGroupResolver &&
+		c.Node.GroupResolver.Default
 }
 
 const (

test/integration/connect/envoy/case-cfg-resolver-svc-redirect/config_entries.hcl

@@ -0,0 +1,21 @@
+enable_central_service_config = true
+
+config_entries {
+  bootstrap {
+    kind = "proxy-defaults"
+    name = "global"
+
+    config {
+      protocol = "http"
+    }
+  }
+
+  bootstrap {
+    kind = "service-resolver"
+    name = "s2"
+
+    redirect {
+      service = "s3"
+    }
+  }
+}

test/integration/connect/envoy/case-cfg-resolver-svc-redirect/s3.hcl

@@ -0,0 +1,5 @@
+services {
+  name = "s3"
+  port = 8282
+  connect { sidecar_service {} }
+}

test/integration/connect/envoy/case-cfg-resolver-svc-redirect/setup.sh

@@ -0,0 +1,10 @@
+#!/bin/bash
+
+set -euo pipefail
+
+# retry because resolving the central config might race
+retry_default gen_envoy_bootstrap s1 19000
+retry_default gen_envoy_bootstrap s2 19001
+retry_default gen_envoy_bootstrap s3 19002
+
+export REQUIRED_SERVICES="s1 s1-sidecar-proxy s2 s2-sidecar-proxy s3 s3-sidecar-proxy"

test/integration/connect/envoy/case-cfg-resolver-svc-redirect/verify.bats

@@ -0,0 +1,46 @@
+#!/usr/bin/env bats
+
+load helpers
+
+@test "s1 proxy admin is up on :19000" {
+  retry_default curl -f -s localhost:19000/stats -o /dev/null
+}
+
+@test "s2 proxy admin is up on :19001" {
+  retry_default curl -f -s localhost:19001/stats -o /dev/null
+}
+
+@test "s3 proxy admin is up on :19002" {
+  retry_default curl -f -s localhost:19002/stats -o /dev/null
+}
+
+@test "s1 proxy listener should be up and have right cert" {
+  assert_proxy_presents_cert_uri localhost:21000 s1
+}
+
+@test "s2 proxy listener should be up and have right cert" {
+  assert_proxy_presents_cert_uri localhost:21001 s2
+}
+
+@test "s3 proxy listener should be up and have right cert" {
+  assert_proxy_presents_cert_uri localhost:21002 s3
+}
+
+@test "s3 proxy should be healthy" {
+  assert_service_has_healthy_instances s3 1
+}
+
+@test "s1 upstream should have healthy endpoints for s3" {
+  assert_upstream_has_healthy_endpoints 127.0.0.1:19000 s3 1
+}
+
+@test "s1 upstream should be able to connect to its upstream simply" {
+  run retry_default curl -s -f -d hello localhost:5000
+  [ "$status" -eq 0 ]
+  [ "$output" = "hello" ]
+}
+
+@test "s1 upstream should be able to connect to s3 via upstream s2" {
+  assert_expected_fortio_name s3
+}
+

test/integration/connect/envoy/docker-compose.yml

@@ -46,24 +46,48 @@ services:
     depends_on:
       - consul
     image: "fortio/fortio"
+    environment:
+      - "FORTIO_NAME=s1"
     command:
      - "server"
      - "-http-port"
      - ":8080"
      - "-grpc-port"
      - ":8079"
+     - "-redirect-port"
+     - "disabled"
     network_mode: service:consul
 
   s2:
     depends_on:
       - consul
     image: "fortio/fortio"
+    environment:
+      - "FORTIO_NAME=s2"
     command:
      - "server"
      - "-http-port"
      - ":8181"
      - "-grpc-port"
      - ":8179"
+     - "-redirect-port"
+     - "disabled"
+    network_mode: service:consul
+
+  s3:
+    depends_on:
+      - consul
+    image: "fortio/fortio"
+    environment:
+      - "FORTIO_NAME=s3"
+    command:
+     - "server"
+     - "-http-port"
+     - ":8282"
+     - "-grpc-port"
+     - ":8279"
+     - "-redirect-port"
+     - "disabled"
     network_mode: service:consul
 
   s1-sidecar-proxy:
@@ -108,6 +132,27 @@ services:
       - *workdir-volume
     network_mode: service:consul
 
+  s3-sidecar-proxy:
+    depends_on:
+      - consul
+    image: "envoyproxy/envoy:v${ENVOY_VERSION:-1.8.0}"
+    command:
+     - "envoy"
+     - "-c"
+     - "/workdir/envoy/s3-bootstrap.json"
+     - "-l"
+     - "debug"
+     # Hot restart breaks since both envoys seem to interact with each other
+     # despite separate containers that don't share IPC namespace. Not quite
+     # sure how this happens but may be due to unix socket being in some shared
+     # location?
+     - "--disable-hot-restart"
+     - "--drain-time-s"
+     - "1"
+    volumes:
+      - *workdir-volume
+    network_mode: service:consul
+
   verify:
     depends_on:
       - consul

test/integration/connect/envoy/helpers.bash

@@ -253,3 +253,18 @@ function gen_envoy_bootstrap {
     return $status
   fi
 }
+
+function get_upstream_fortio_name {
+  run retry_default curl -v -s -f localhost:5000/debug?env=dump
+  [ "$status" == 0 ]
+  echo "$output" | grep -E "^FORTIO_NAME="
+}
+
+function assert_expected_fortio_name {
+  local EXPECT_NAME=$1
+
+  GOT=$(get_upstream_fortio_name)
+  echo "GOT $GOT"
+
+  [ "$GOT" == "FORTIO_NAME=${EXPECT_NAME}" ]
+}