From 228da48f5d1075900d79501789e0375b4344a933 Mon Sep 17 00:00:00 2001 From: Matt Keeler Date: Wed, 5 Feb 2020 10:06:11 -0500 Subject: [PATCH] Minor Non-Functional Updates (#7215) * Cleanup the discovery chain compilation route handling Nothing functionally should be different here. The real difference is that when creating new targets or handling route destinations we use the router config entries name and namespace instead of that of the top level request. Today they SHOULD always be the same but that may not always be the case. This hopefully also makes it easier to understand how the router entries are handled. * Refactor a small bit of the service manager tests in oss We used to use the stringHash function to compute part of the filename where things would get persisted to. This has been changed in the core code to calling the StringHash method on the ServiceID type. It just so happens that the new method will output the same value for anything in the default namespace (by design actually). However, logically this filename computation in the test should do the same thing as the core code itself so I updated it here. Also of note is that newer enterprise-only tests for the service manager cannot use the old stringHash function at all because it will produce incorrect results for non-default namespaces. --- agent/consul/discoverychain/compile.go | 6 +++--- agent/service_manager_test.go | 6 ++++-- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/agent/consul/discoverychain/compile.go b/agent/consul/discoverychain/compile.go index 9bb5d2905d..f091f91b9f 100644 --- a/agent/consul/discoverychain/compile.go +++ b/agent/consul/discoverychain/compile.go @@ -554,7 +554,7 @@ func (c *compiler) assembleChain() error { dest := route.Destination svc := defaultIfEmpty(dest.Service, c.serviceName) - destNamespace := defaultIfEmpty(dest.Namespace, c.evaluateInNamespace) + destNamespace := defaultIfEmpty(dest.Namespace, router.NamespaceOrDefault()) // Check to see if the destination is eligible for splitting. var ( @@ -579,13 +579,13 @@ func (c *compiler) assembleChain() error { // If we have a router, we'll add a catch-all route at the end to send // unmatched traffic to the next hop in the chain. - defaultDestinationNode, err := c.getSplitterOrResolverNode(c.newTarget(c.serviceName, "", "", "")) + defaultDestinationNode, err := c.getSplitterOrResolverNode(c.newTarget(router.Name, "", router.NamespaceOrDefault(), "")) if err != nil { return err } defaultRoute := &structs.DiscoveryRoute{ - Definition: newDefaultServiceRoute(c.serviceName, c.evaluateInNamespace), + Definition: newDefaultServiceRoute(router.Name, router.NamespaceOrDefault()), NextNode: defaultDestinationNode.MapKey(), } routeNode.Routes = append(routeNode.Routes, defaultRoute) diff --git a/agent/service_manager_test.go b/agent/service_manager_test.go index cb38266018..7b211884e6 100644 --- a/agent/service_manager_test.go +++ b/agent/service_manager_test.go @@ -310,8 +310,10 @@ func TestServiceManager_PersistService_API(t *testing.T) { EnterpriseMeta: *structs.DefaultEnterpriseMeta(), } - svcFile := filepath.Join(a.Config.DataDir, servicesDir, stringHash(svc.ID)) - configFile := filepath.Join(a.Config.DataDir, serviceConfigDir, stringHash(svc.ID)) + svcID := svc.CompoundServiceID() + + svcFile := filepath.Join(a.Config.DataDir, servicesDir, svcID.StringHash()) + configFile := filepath.Join(a.Config.DataDir, serviceConfigDir, svcID.StringHash()) // Service is not persisted unless requested, but we always persist service configs. require.NoError(a.AddService(svc, nil, false, "", ConfigSourceRemote))