treat external services differently

hashicorp · Aug 14, 2019 · 6596812 · 6596812
1 parent c69055c
commit 6596812
Show file tree

Hide file tree

Showing 4 changed files with 84 additions and 12 deletions.
diff --git a/agent/consul/discoverychain/compile.go b/agent/consul/discoverychain/compile.go
@@ -813,19 +813,38 @@ RESOLVE_AGAIN:
 
 	target.Subset = resolver.Subsets[target.ServiceSubset]
 
-	usingExternalSNI := false
 	if serviceDefault := c.entries.GetService(target.Service); serviceDefault != nil && serviceDefault.ExternalSNI != "" {
 		// Explicitly set the SNI value.
 		target.SNI = serviceDefault.ExternalSNI
-		usingExternalSNI = true
+		target.External = true
+	}
+
+	// If using external SNI the service is fundamentally external.
+	if target.External {
+		if len(resolver.Subsets) > 0 {
+			return nil, &structs.ConfigEntryGraphError{
+				Message: fmt.Sprintf(
+					"service %q has an external SNI set; cannot define subsets for external services",
+					target.Service,
+				),
+			}
+		}
+		if len(resolver.Failover) > 0 {
+			return nil, &structs.ConfigEntryGraphError{
+				Message: fmt.Sprintf(
+					"service %q has an external SNI set; cannot define failover for external services",
+					target.Service,
+				),
+			}
+		}
 	}
 
 	// TODO (mesh-gateway)- maybe allow using a gateway within a datacenter at some point
 	if target.Datacenter == c.useInDatacenter {
 		target.MeshGateway.Mode = structs.MeshGatewayModeDefault
 
-	} else if usingExternalSNI {
-		// Bypass mesh gateways if external SNI is configured.
+	} else if target.External {
+		// Bypass mesh gateways if it is an external service.
 		target.MeshGateway.Mode = structs.MeshGatewayModeDefault
 
 	} else {

diff --git a/agent/consul/discoverychain/compile_test.go b/agent/consul/discoverychain/compile_test.go
@@ -55,13 +55,15 @@ func TestCompile(t *testing.T) {
 		"multi dc canary":            testcase_MultiDatacenterCanary(),
 
 		// various errors
-		"splitter requires valid protocol": testcase_SplitterRequiresValidProtocol(),
-		"router requires valid protocol":   testcase_RouterRequiresValidProtocol(),
-		"split to unsplittable protocol":   testcase_SplitToUnsplittableProtocol(),
-		"route to unroutable protocol":     testcase_RouteToUnroutableProtocol(),
-		"failover crosses protocols":       testcase_FailoverCrossesProtocols(),
-		"redirect crosses protocols":       testcase_RedirectCrossesProtocols(),
-		"redirect to missing subset":       testcase_RedirectToMissingSubset(),
+		"splitter requires valid protocol":        testcase_SplitterRequiresValidProtocol(),
+		"router requires valid protocol":          testcase_RouterRequiresValidProtocol(),
+		"split to unsplittable protocol":          testcase_SplitToUnsplittableProtocol(),
+		"route to unroutable protocol":            testcase_RouteToUnroutableProtocol(),
+		"failover crosses protocols":              testcase_FailoverCrossesProtocols(),
+		"redirect crosses protocols":              testcase_RedirectCrossesProtocols(),
+		"redirect to missing subset":              testcase_RedirectToMissingSubset(),
+		"resolver with failover and external sni": testcase_Resolver_ExternalSNI_FailoverNotAllowed(),
+		"resolver with subsets and external sni":  testcase_Resolver_ExternalSNI_SubsetsNotAllowed(),
 
 		// overrides
 		"resolver with protocol from override":         testcase_ResolverProtocolOverride(),
@@ -1461,12 +1463,61 @@ func testcase_DefaultResolver_ExternalSNI() compileTestCase {
 		Targets: map[string]*structs.DiscoveryTarget{
 			"main.default.dc1": newTarget("main", "", "default", "dc1", func(t *structs.DiscoveryTarget) {
 				t.SNI = "main.some.other.service.mesh"
+				t.External = true
 			}),
 		},
 	}
 	return compileTestCase{entries: entries, expect: expect, expectIsDefault: true}
 }
 
+func testcase_Resolver_ExternalSNI_FailoverNotAllowed() compileTestCase {
+	entries := newEntries()
+	entries.AddServices(&structs.ServiceConfigEntry{
+		Kind:        structs.ServiceDefaults,
+		Name:        "main",
+		ExternalSNI: "main.some.other.service.mesh",
+	})
+	entries.AddResolvers(&structs.ServiceResolverConfigEntry{
+		Kind:           "service-resolver",
+		Name:           "main",
+		ConnectTimeout: 33 * time.Second,
+		Failover: map[string]structs.ServiceResolverFailover{
+			"*": {Service: "backup"},
+		},
+	})
+
+	return compileTestCase{
+		entries:        entries,
+		expectErr:      `service "main" has an external SNI set; cannot define failover for external services`,
+		expectGraphErr: true,
+	}
+}
+
+func testcase_Resolver_ExternalSNI_SubsetsNotAllowed() compileTestCase {
+	entries := newEntries()
+	entries.AddServices(&structs.ServiceConfigEntry{
+		Kind:        structs.ServiceDefaults,
+		Name:        "main",
+		ExternalSNI: "main.some.other.service.mesh",
+	})
+	entries.AddResolvers(&structs.ServiceResolverConfigEntry{
+		Kind:           "service-resolver",
+		Name:           "main",
+		ConnectTimeout: 33 * time.Second,
+		Subsets: map[string]structs.ServiceResolverSubset{
+			"canary": {
+				Filter: "Service.Meta.version == canary",
+			},
+		},
+	})
+
+	return compileTestCase{
+		entries:        entries,
+		expectErr:      `service "main" has an external SNI set; cannot define subsets for external services`,
+		expectGraphErr: true,
+	}
+}
+
 func testcase_MultiDatacenterCanary() compileTestCase {
 	entries := newEntries()
 	setServiceProtocol(entries, "main", "http")

diff --git a/agent/structs/discovery_chain.go b/agent/structs/discovery_chain.go
@@ -163,7 +163,8 @@ type DiscoveryTarget struct {
 
 	// SNI if set is the sni field to use when addressing this set of
 	// endpoints. If not configured then the default should be used.
-	SNI string `json:",omitempty"`
+	SNI      string `json:",omitempty"`
+	External bool   `json:",omitempty"`
 }
 
 func NewDiscoveryTarget(service, serviceSubset, namespace, datacenter string) *DiscoveryTarget {

diff --git a/api/discovery_chain.go b/api/discovery_chain.go
@@ -188,4 +188,5 @@ type DiscoveryTarget struct {
 	MeshGateway MeshGatewayConfig
 	Subset      ServiceResolverSubset
 	SNI         string
+	External    bool
 }