connect: detect and prevent circular discovery chain references (#6246)

agent/consul/discoverychain/compile.go

@@ -239,10 +239,7 @@ func (c *compiler) compile() (*structs.CompiledDiscoveryChain, error) {
 		panic("impossible to return no results")
 	}
 
-	if err := c.detectCircularSplits(); err != nil {
-		return nil, err
-	}
-	if err := c.detectCircularResolves(); err != nil {
+	if err := c.detectCircularReferences(); err != nil {
 		return nil, err
 	}
 
@@ -304,13 +301,58 @@ func (c *compiler) compile() (*structs.CompiledDiscoveryChain, error) {
 	}, nil
 }
 
-func (c *compiler) detectCircularSplits() error {
-	// TODO(rb): detect when a tree of splitters backtracks
-	return nil
-}
+func (c *compiler) detectCircularReferences() error {
+	var (
+		todo       stringStack
+		visited    = make(map[string]struct{})
+		visitChain stringStack
+	)
+
+	todo.Push(c.startNode)
+	for {
+		current, ok := todo.Pop()
+		if !ok {
+			break
+		}
+		if current == "_popvisit" {
+			if v, ok := visitChain.Pop(); ok {
+				delete(visited, v)
+			}
+			continue
+		}
+		visitChain.Push(current)
+
+		if _, ok := visited[current]; ok {
+			return &structs.ConfigEntryGraphError{
+				Message: fmt.Sprintf(
+					"detected circular reference: [%s]",
+					strings.Join(visitChain.Items(), " -> "),
+				),
+			}
+		}
+		visited[current] = struct{}{}
+
+		todo.Push("_popvisit")
+
+		node := c.nodes[current]
+
+		switch node.Type {
+		case structs.DiscoveryGraphNodeTypeRouter:
+			for _, route := range node.Routes {
+				todo.Push(route.NextNode)
+			}
+		case structs.DiscoveryGraphNodeTypeSplitter:
+			for _, split := range node.Splits {
+				todo.Push(split.NextNode)
+			}
+		case structs.DiscoveryGraphNodeTypeResolver:
+			// Circular redirects are detected elsewhere and failover isn't
+			// recursive so there's nothing more to do here.
+		default:
+			return fmt.Errorf("unexpected graph node type: %s", node.Type)
+		}
+	}
 
-func (c *compiler) detectCircularResolves() error {
-	// TODO(rb): detect when a series of redirects and failovers cause a circular reference
 	return nil
 }
 
@@ -619,6 +661,13 @@ func (c *compiler) getSplitterNode(name string) (*structs.DiscoveryGraphNode, er
 // capabilities from a resolver config entry. It recurses into itself to
 // _generate_ targets used for failover out of convenience.
 func (c *compiler) getResolverNode(target structs.DiscoveryTarget, recursedForFailover bool) (*structs.DiscoveryGraphNode, error) {
+	var (
+		// State to help detect redirect cycles and print helpful error
+		// messages.
+		redirectHistory = make(map[structs.DiscoveryTarget]struct{})
+		redirectOrder   []structs.DiscoveryTarget
+	)
+
 RESOLVE_AGAIN:
 	// Do we already have the node?
 	if prev, ok := c.resolveNodes[target]; ok {
@@ -637,6 +686,24 @@ RESOLVE_AGAIN:
 		c.resolvers[target.Service] = resolver
 	}
 
+	if _, ok := redirectHistory[target]; ok {
+		redirectOrder = append(redirectOrder, target)
+
+		pretty := make([]string, len(redirectOrder))
+		for i, target := range redirectOrder {
+			pretty[i] = target.String()
+		}
+
+		return nil, &structs.ConfigEntryGraphError{
+			Message: fmt.Sprintf(
+				"detected circular resolver redirect: [%s]",
+				strings.Join(pretty, " -> "),
+			),
+		}
+	}
+	redirectHistory[target] = struct{}{}
+	redirectOrder = append(redirectOrder, target)
+
 	// Handle redirects right up front.
 	//
 	// TODO(rb): What about a redirected subset reference? (web/v2, but web redirects to alt/"")

agent/consul/discoverychain/compile_test.go

@@ -22,7 +22,6 @@ type compileTestCase struct {
 func TestCompile(t *testing.T) {
 	t.Parallel()
 
-	// TODO(rb): test circular dependency?
 	cases := map[string]compileTestCase{
 		"router with defaults":                             testcase_JustRouterWithDefaults(),
 		"router with defaults and resolver":                testcase_RouterWithDefaults_NoSplit_WithResolver(),
@@ -39,6 +38,8 @@ func TestCompile(t *testing.T) {
 		"service and subset redirect":                      testcase_ServiceAndSubsetRedirect(),
 		"datacenter redirect":                              testcase_DatacenterRedirect(),
 		"service failover":                                 testcase_ServiceFailover(),
+		"service failover through redirect":                testcase_ServiceFailoverThroughRedirect(),
+		"circular resolver failover":                       testcase_Resolver_CircularFailover(),
 		"service and subset failover":                      testcase_ServiceAndSubsetFailover(),
 		"datacenter failover":                              testcase_DatacenterFailover(),
 		"service failover with mesh gateways":              testcase_ServiceFailover_WithMeshGateways(),
@@ -48,7 +49,6 @@ func TestCompile(t *testing.T) {
 		"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(),
 		"multi dc canary":            testcase_MultiDatacenterCanary(),
 
@@ -65,6 +65,10 @@ func TestCompile(t *testing.T) {
 		"resolver with protocol from override":         testcase_ResolverProtocolOverride(),
 		"resolver with protocol from override ignored": testcase_ResolverProtocolOverrideIgnored(),
 		"router ignored due to protocol override":      testcase_RouterIgnored_ResolverProtocolOverride(),
+
+		// circular references
+		"circular resolver redirect": testcase_Resolver_CircularRedirect(),
+		"circular split":             testcase_CircularSplit(),
 	}
 
 	for name, tc := range cases {
@@ -1028,6 +1032,108 @@ func testcase_ServiceFailover() compileTestCase {
 	return compileTestCase{entries: entries, expect: expect}
 }
 
+func testcase_ServiceFailoverThroughRedirect() compileTestCase {
+	entries := newEntries()
+	entries.AddResolvers(
+		&structs.ServiceResolverConfigEntry{
+			Kind: "service-resolver",
+			Name: "backup",
+			Redirect: &structs.ServiceResolverRedirect{
+				Service: "actual",
+			},
+		},
+		&structs.ServiceResolverConfigEntry{
+			Kind: "service-resolver",
+			Name: "main",
+			Failover: map[string]structs.ServiceResolverFailover{
+				"*": {Service: "backup"},
+			},
+		},
+	)
+
+	resolverMain := entries.GetResolver("main")
+
+	wildFail := resolverMain.Failover["*"]
+
+	expect := &structs.CompiledDiscoveryChain{
+		Protocol:  "tcp",
+		StartNode: "resolver:main,,,dc1",
+		Nodes: map[string]*structs.DiscoveryGraphNode{
+			"resolver:main,,,dc1": &structs.DiscoveryGraphNode{
+				Type: structs.DiscoveryGraphNodeTypeResolver,
+				Name: "main,,,dc1",
+				Resolver: &structs.DiscoveryResolver{
+					Definition:     resolverMain,
+					ConnectTimeout: 5 * time.Second,
+					Target:         newTarget("main", "", "default", "dc1"),
+					Failover: &structs.DiscoveryFailover{
+						Definition: &wildFail,
+						Targets: []structs.DiscoveryTarget{
+							newTarget("actual", "", "default", "dc1"),
+						},
+					},
+				},
+			},
+		},
+		Targets: map[structs.DiscoveryTarget]structs.DiscoveryTargetConfig{
+			newTarget("actual", "", "default", "dc1"): structs.DiscoveryTargetConfig{},
+			newTarget("main", "", "default", "dc1"):   structs.DiscoveryTargetConfig{},
+		},
+	}
+	return compileTestCase{entries: entries, expect: expect}
+}
+
+func testcase_Resolver_CircularFailover() compileTestCase {
+	entries := newEntries()
+	entries.AddResolvers(
+		&structs.ServiceResolverConfigEntry{
+			Kind: "service-resolver",
+			Name: "backup",
+			Failover: map[string]structs.ServiceResolverFailover{
+				"*": {Service: "main"},
+			},
+		},
+		&structs.ServiceResolverConfigEntry{
+			Kind: "service-resolver",
+			Name: "main",
+			Failover: map[string]structs.ServiceResolverFailover{
+				"*": {Service: "backup"},
+			},
+		},
+	)
+
+	resolverMain := entries.GetResolver("main")
+
+	wildFail := resolverMain.Failover["*"]
+
+	expect := &structs.CompiledDiscoveryChain{
+		Protocol:  "tcp",
+		StartNode: "resolver:main,,,dc1",
+		Nodes: map[string]*structs.DiscoveryGraphNode{
+			"resolver:main,,,dc1": &structs.DiscoveryGraphNode{
+				Type: structs.DiscoveryGraphNodeTypeResolver,
+				Name: "main,,,dc1",
+				Resolver: &structs.DiscoveryResolver{
+					Definition:     resolverMain,
+					ConnectTimeout: 5 * time.Second,
+					Target:         newTarget("main", "", "default", "dc1"),
+					Failover: &structs.DiscoveryFailover{
+						Definition: &wildFail,
+						Targets: []structs.DiscoveryTarget{
+							newTarget("backup", "", "default", "dc1"),
+						},
+					},
+				},
+			},
+		},
+		Targets: map[structs.DiscoveryTarget]structs.DiscoveryTargetConfig{
+			newTarget("backup", "", "default", "dc1"): structs.DiscoveryTargetConfig{},
+			newTarget("main", "", "default", "dc1"):   structs.DiscoveryTargetConfig{},
+		},
+	}
+	return compileTestCase{entries: entries, expect: expect}
+}
+
 func testcase_ServiceAndSubsetFailover() compileTestCase {
 	entries := newEntries()
 	entries.AddResolvers(
@@ -1392,67 +1498,6 @@ func testcase_Resolve_WithDefaultSubset() compileTestCase {
 	return compileTestCase{entries: entries, expect: expect}
 }
 
-func testcase_CircularSplit() compileTestCase {
-	entries := newEntries()
-	setServiceProtocol(entries, "main", "http")
-	setServiceProtocol(entries, "other", "http")
-
-	entries.AddSplitters(
-		&structs.ServiceSplitterConfigEntry{
-			Kind: "service-splitter",
-			Name: "main",
-			Splits: []structs.ServiceSplit{
-				{Weight: 60, Service: "other"},
-				{Weight: 40, Service: "main"}, // goes straight to resolver
-			},
-		},
-		&structs.ServiceSplitterConfigEntry{
-			Kind: "service-splitter",
-			Name: "other",
-			Splits: []structs.ServiceSplit{
-				{Weight: 60, Service: "main"},
-				{Weight: 40, Service: "other"}, // goes straight to resolver
-			},
-		},
-	)
-
-	resolveMain := newDefaultServiceResolver("main")
-
-	expect := &structs.CompiledDiscoveryChain{
-		Protocol:  "http",
-		StartNode: "splitter:main",
-		Nodes: map[string]*structs.DiscoveryGraphNode{
-			"splitter:main": &structs.DiscoveryGraphNode{
-				Type: structs.DiscoveryGraphNodeTypeSplitter,
-				Name: "main",
-				Splits: []*structs.DiscoverySplit{
-					{
-						Weight:   60,
-						NextNode: "resolver:main,v2,,dc1",
-					},
-				},
-			},
-			"resolver:main,v2,,dc1": &structs.DiscoveryGraphNode{
-				Type: structs.DiscoveryGraphNodeTypeResolver,
-				Name: "main,v2,,dc1",
-				Resolver: &structs.DiscoveryResolver{
-					Definition:     resolveMain,
-					ConnectTimeout: 5 * time.Second,
-					Target:         newTarget("main", "v2", "default", "dc1"),
-				},
-			},
-		},
-		Targets: map[structs.DiscoveryTarget]structs.DiscoveryTargetConfig{
-			newTarget("main", "v2", "default", "dc1"): structs.DiscoveryTargetConfig{
-				Subset: structs.ServiceResolverSubset{
-					Filter: "TODO",
-				},
-			},
-		},
-	}
-	return compileTestCase{entries: entries, expect: expect}
-}
-
 func testcase_MultiDatacenterCanary() compileTestCase {
 	entries := newEntries()
 	setServiceProtocol(entries, "main", "http")
@@ -2031,6 +2076,57 @@ func testcase_RouterIgnored_ResolverProtocolOverride() compileTestCase {
 	}
 }
 
+func testcase_Resolver_CircularRedirect() compileTestCase {
+	entries := newEntries()
+	entries.AddResolvers(
+		&structs.ServiceResolverConfigEntry{
+			Kind: "service-resolver",
+			Name: "main",
+			Redirect: &structs.ServiceResolverRedirect{
+				Service: "other",
+			},
+		},
+		&structs.ServiceResolverConfigEntry{
+			Kind: "service-resolver",
+			Name: "other",
+			Redirect: &structs.ServiceResolverRedirect{
+				Service: "main",
+			},
+		},
+	)
+
+	return compileTestCase{entries: entries,
+		expectErr:      "detected circular resolver redirect",
+		expectGraphErr: true,
+	}
+}
+
+func testcase_CircularSplit() compileTestCase {
+	entries := newEntries()
+	setGlobalProxyProtocol(entries, "http")
+	entries.AddSplitters(
+		&structs.ServiceSplitterConfigEntry{
+			Kind: "service-splitter",
+			Name: "main",
+			Splits: []structs.ServiceSplit{
+				{Weight: 100, Service: "other"},
+			},
+		},
+		&structs.ServiceSplitterConfigEntry{
+			Kind: "service-splitter",
+			Name: "other",
+			Splits: []structs.ServiceSplit{
+				{Weight: 100, Service: "main"},
+			},
+		},
+	)
+
+	return compileTestCase{entries: entries,
+		expectErr:      "detected circular reference",
+		expectGraphErr: true,
+	}
+}
+
 func newSimpleRoute(name string, muts ...func(*structs.ServiceRoute)) structs.ServiceRoute {
 	r := structs.ServiceRoute{
 		Match: &structs.ServiceRouteMatch{

agent/consul/discoverychain/string_stack.go

@@ -0,0 +1,36 @@
+package discoverychain
+
+type stringStack []string
+
+func (s stringStack) Len() int {
+	return len(s)
+}
+
+func (s *stringStack) Push(v string) {
+	*s = append(*s, v)
+}
+
+func (s *stringStack) Pop() (string, bool) {
+	if len(*s) == 0 {
+		return "", false
+	}
+
+	size := len(*s)
+
+	v := (*s)[size-1]
+	*s = (*s)[0 : size-1]
+	return v, true
+}
+
+func (s stringStack) Peek() (string, bool) {
+	if len(s) == 0 {
+		return "", false
+	}
+	return s[len(s)-1], true
+}
+
+// Items returns the underlying slice. The first thing Pushed onto the stack is
+// in the 0th slice position.
+func (s stringStack) Items() []string {
+	return s
+}

agent/consul/discoverychain/string_stack_test.go

@@ -0,0 +1,58 @@
+package discoverychain
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestStringStack(t *testing.T) {
+	var (
+		v  string
+		ok bool
+	)
+
+	var ss stringStack
+	require.Equal(t, 0, ss.Len())
+	v, ok = ss.Peek()
+	require.Empty(t, v)
+	require.False(t, ok)
+
+	ss.Push("foo")
+	require.Equal(t, 1, ss.Len())
+
+	v, ok = ss.Peek()
+	require.Equal(t, "foo", v)
+	require.True(t, ok)
+	require.Equal(t, 1, ss.Len())
+
+	v, ok = ss.Pop()
+	require.Equal(t, "foo", v)
+	require.True(t, ok)
+	require.Equal(t, 0, ss.Len())
+
+	ss.Push("foo")
+	ss.Push("bar")
+	ss.Push("baz")
+	require.Equal(t, 3, ss.Len())
+
+	v, ok = ss.Peek()
+	require.Equal(t, "baz", v)
+	require.True(t, ok)
+	require.Equal(t, 3, ss.Len())
+
+	v, ok = ss.Pop()
+	require.Equal(t, "baz", v)
+	require.True(t, ok)
+	require.Equal(t, 2, ss.Len())
+
+	v, ok = ss.Pop()
+	require.Equal(t, "bar", v)
+	require.True(t, ok)
+	require.Equal(t, 1, ss.Len())
+
+	v, ok = ss.Pop()
+	require.Equal(t, "foo", v)
+	require.True(t, ok)
+	require.Equal(t, 0, ss.Len())
+}

agent/consul/state/config_entry.go

@@ -442,8 +442,6 @@ func (s *Store) testCompileDiscoveryChain(
 		return err
 	}
 
-	// TODO(rb): is this ok that we execute the compiler in the state store?
-
 	// Note we use an arbitrary namespace and datacenter as those would not
 	// currently affect the graph compilation in ways that matter here.
 	req := discoverychain.CompileRequest{

agent/consul/state/config_entry_test.go

@@ -685,6 +685,62 @@ func TestStore_ConfigEntry_GraphValidation(t *testing.T) {
 			expectErr:      `does not have a subset named "v1"`,
 			expectGraphErr: true,
 		},
+		/////////////////////////////////////////////////
+		{
+			name: "cannot introduce circular resolver redirect",
+			entries: []structs.ConfigEntry{
+				&structs.ServiceResolverConfigEntry{
+					Kind: structs.ServiceResolver,
+					Name: "other",
+					Redirect: &structs.ServiceResolverRedirect{
+						Service: "main",
+					},
+				},
+			},
+			op: func(t *testing.T, s *Store) error {
+				entry := &structs.ServiceResolverConfigEntry{
+					Kind: structs.ServiceResolver,
+					Name: "main",
+					Redirect: &structs.ServiceResolverRedirect{
+						Service: "other",
+					},
+				}
+				return s.EnsureConfigEntry(0, entry)
+			},
+			expectErr:      `detected circular resolver redirect`,
+			expectGraphErr: true,
+		},
+		{
+			name: "cannot introduce circular split",
+			entries: []structs.ConfigEntry{
+				&structs.ProxyConfigEntry{
+					Kind: structs.ProxyDefaults,
+					Name: structs.ProxyConfigGlobal,
+					Config: map[string]interface{}{
+						"protocol": "http",
+					},
+				},
+				&structs.ServiceSplitterConfigEntry{
+					Kind: "service-splitter",
+					Name: "other",
+					Splits: []structs.ServiceSplit{
+						{Weight: 100, Service: "main"},
+					},
+				},
+			},
+			op: func(t *testing.T, s *Store) error {
+				entry := &structs.ServiceSplitterConfigEntry{
+					Kind: "service-splitter",
+					Name: "main",
+					Splits: []structs.ServiceSplit{
+						{Weight: 100, Service: "other"},
+					},
+				}
+				return s.EnsureConfigEntry(0, entry)
+			},
+			expectErr:      `detected circular reference`,
+			expectGraphErr: true,
+		},
 	} {
 		tc := tc
 

agent/structs/discovery_chain.go

@@ -86,6 +86,18 @@ type DiscoveryGraphNode struct {
 	Resolver *DiscoveryResolver `json:",omitempty"`
 }
 
+func (s *DiscoveryGraphNode) IsRouter() bool {
+	return s.Type == DiscoveryGraphNodeTypeRouter
+}
+
+func (s *DiscoveryGraphNode) IsSplitter() bool {
+	return s.Type == DiscoveryGraphNodeTypeSplitter
+}
+
+func (s *DiscoveryGraphNode) IsResolver() bool {
+	return s.Type == DiscoveryGraphNodeTypeResolver
+}
+
 func (s *DiscoveryGraphNode) ServiceName() string {
 	if s.Type == DiscoveryGraphNodeTypeResolver {
 		return s.Resolver.Target.Service