Configurable service name separator

README.md

@@ -110,6 +110,7 @@ config-file                 |                 | Path to a JSON file to read conf
 consul-auth                 | `false`         | Use Consul with authentication
 consul-auth-password        |                 | The basic authentication password
 consul-auth-username        |                 | The basic authentication username
+consul-name-separator       | `.`             | Separator used to create default service name for Consul
 consul-get-services-retry   | `3`             | Number of retries on failure when performing requests to Consul. Each retry uses different cached agent
 consul-max-agent-failures   | `3`             | Max number of consecutive request failures for agent before removal from cache
 consul-port                 | `8500`          | Consul port

apps/app.go

@@ -2,7 +2,6 @@ package apps
 
 import (
 	"encoding/json"
-	"strings"
 )
 
 // Only Marathon apps with this label will be registered in Consul
@@ -45,23 +44,20 @@ func (id AppId) String() string {
 	return string(id)
 }
 
-func (id AppId) ConsulServiceName() string {
-	return strings.Replace(strings.Trim(strings.TrimSpace(id.String()), "/"), "/", ".", -1)
-}
-
 func (app *App) IsConsulApp() bool {
 	_, ok := app.Labels[MARATHON_CONSUL_LABEL]
 	return ok
 }
 
-func (app *App) ConsulServiceName() string {
-	if value, ok := app.Labels[MARATHON_CONSUL_LABEL]; ok && value != "true" {
-		value = AppId(value).ConsulServiceName()
-		if value != "" {
-			return value
-		}
+func (app *App) ConsulName() string {
+	if value, ok := app.Labels[MARATHON_CONSUL_LABEL]; ok && !isSpecialConsulNameValue(value) {
+		return value
 	}
-	return app.ID.ConsulServiceName()
+	return app.ID.String()
+}
+
+func isSpecialConsulNameValue(name string) bool {
+	return name == "true" || name == ""
 }
 
 func ParseApps(jsonBlob []byte) ([]*App, error) {

apps/app_test.go

@@ -119,68 +119,3 @@ func TestAppId_String(t *testing.T) {
 	t.Parallel()
 	assert.Equal(t, "appId", AppId("appId").String())
 }
-
-func TestAppId_ConsulServiceName(t *testing.T) {
-	t.Parallel()
-
-	// given
-	id := AppId("/rootGroup/subGroup/subSubGroup/name")
-
-	// when
-	serviceName := id.ConsulServiceName()
-
-	// then
-	assert.Equal(t, "rootGroup.subGroup.subSubGroup.name", serviceName)
-}
-
-func TestConsulServiceName_BackwardCompatibilityForConsulTrue(t *testing.T) {
-	t.Parallel()
-
-	// when
-	app := &App{
-		ID:     "/someApp",
-		Labels: map[string]string{"consul": "true"},
-	}
-
-	// then
-	assert.Equal(t, "someApp", app.ConsulServiceName())
-}
-
-func TestConsulServiceName_EmptyConsulLabelValue(t *testing.T) {
-	t.Parallel()
-
-	// when
-	app := &App{
-		ID:     "/someApp/other",
-		Labels: map[string]string{"consul": ""},
-	}
-
-	// then
-	assert.Equal(t, "someApp.other", app.ConsulServiceName())
-}
-
-func TestConsulServiceName_CustomName(t *testing.T) {
-	t.Parallel()
-
-	// when
-	app := &App{
-		ID:     "/someApp/other",
-		Labels: map[string]string{"consul": "otherName"},
-	}
-
-	// then
-	assert.Equal(t, "otherName", app.ConsulServiceName())
-}
-
-func TestConsulServiceName_CustomNameEscapingNotAllowedChars(t *testing.T) {
-	t.Parallel()
-
-	// when
-	app := &App{
-		ID:     "/someApp/other",
-		Labels: map[string]string{"consul": "/otherName/something"},
-	}
-
-	// then
-	assert.Equal(t, "otherName.something", app.ConsulServiceName())
-}

config/config.go

@@ -71,6 +71,7 @@ func (config *Config) parseFlags() {
 	flag.DurationVar(&config.Consul.Timeout, "consul-timeout", 3*time.Second, "Time limit for requests made by the Consul HTTP client. A Timeout of zero means no timeout")
 	flag.Uint32Var(&config.Consul.AgentFailuresTolerance, "consul-max-agent-failures", 3, "Max number of consecutive request failures for agent before removal from cache")
 	flag.Uint32Var(&config.Consul.RequestRetries, "consul-get-services-retry", 3, "Number of retries on failure when performing requests to Consul. Each retry uses different cached agent")
+	flag.StringVar(&config.Consul.ConsulNameSeparator, "consul-name-separator", ".", "Separator used to create default service name for Consul")
 
 	// Web
 	flag.StringVar(&config.Web.Listen, "listen", ":4000", "Accept connections at this address")

config/config_test.go

@@ -99,6 +99,7 @@ func TestConfig_ShouldBeMergedWithFileDefaultsAndFlags(t *testing.T) {
 			Timeout:                3 * time.Second,
 			RequestRetries:         5,
 			AgentFailuresTolerance: 3,
+			ConsulNameSeparator:    ".",
 		},
 		Web: web.Config{
 			Listen:       ":4000",

consul/config.go

@@ -14,6 +14,7 @@ type ConsulConfig struct {
 	Timeout                time.Duration
 	RequestRetries         uint32
 	AgentFailuresTolerance uint32
+	ConsulNameSeparator    string
 }
 
 type Auth struct {

consul/consul.go

@@ -18,6 +18,7 @@ type ConsulServices interface {
 	Register(task *apps.Task, app *apps.App) error
 	Deregister(serviceId apps.TaskId, agentAddress string) error
 	GetAgent(agentAddress string) (*consulapi.Client, error)
+	ServiceName(app *apps.App) string
 }
 
 type Consul struct {
@@ -191,6 +192,21 @@ func (c *Consul) GetAgent(agentAddress string) (*consulapi.Client, error) {
 	return c.agents.GetAgent(agentAddress)
 }
 
+func (c *Consul) ServiceName(app *apps.App) string {
+	appConsulName := app.ConsulName()
+	serviceName := c.marathonAppNameToConsulServiceName(appConsulName)
+	if serviceName == "" {
+		log.WithField("AppId", app.ID.String()).WithField("ConsulServiceName", appConsulName).
+			Warn("Warning! Invalid Consul service name provided for app. Will use default app name instead.")
+		return c.marathonAppNameToConsulServiceName(app.ID.String())
+	}
+	return serviceName
+}
+
+func (c *Consul) marathonAppNameToConsulServiceName(name string) string {
+	return strings.Replace(strings.Trim(strings.TrimSpace(name), "/"), "/", c.config.ConsulNameSeparator, -1)
+}
+
 func (c *Consul) marathonTaskToConsulService(task *apps.Task, app *apps.App) (*consulapi.AgentServiceRegistration, error) {
 	IP, err := utils.HostToIPv4(task.Host)
 	if err != nil {
@@ -200,7 +216,7 @@ func (c *Consul) marathonTaskToConsulService(task *apps.Task, app *apps.App) (*c
 
 	return &consulapi.AgentServiceRegistration{
 		ID:      task.ID.String(),
-		Name:    app.ConsulServiceName(),
+		Name:    c.ServiceName(app),
 		Port:    task.Ports[0],
 		Address: serviceAddress,
 		Tags:    c.marathonLabelsToConsulTags(app.Labels),

consul/consul_stub.go

@@ -21,7 +21,7 @@ func NewConsulStubWithTag(tag string) *ConsulStub {
 		services:         make(map[apps.TaskId]*consulapi.AgentServiceRegistration),
 		ErrorServices:    make(map[apps.TaskId]error),
 		ErrorGetServices: make(map[string]error),
-		consul:           New(ConsulConfig{Tag: tag}),
+		consul:           New(ConsulConfig{Tag: tag, ConsulNameSeparator: "."}),
 	}
 }
 
@@ -73,6 +73,10 @@ func (c *ConsulStub) Register(task *apps.Task, app *apps.App) error {
 	}
 }
 
+func (c *ConsulStub) ServiceName(app *apps.App) string {
+	return c.consul.ServiceName(app)
+}
+
 func (c *ConsulStub) Deregister(serviceId apps.TaskId, agent string) error {
 	if err, ok := c.ErrorServices[serviceId]; ok {
 		return err

consul/consul_test.go

@@ -646,3 +646,87 @@ func TestMarathonTaskToConsulServiceMapping_NotResolvableTaskHost(t *testing.T)
 	// then
 	assert.Error(t, err)
 }
+
+func TestServiceName_WithSerarator(t *testing.T) {
+	t.Parallel()
+
+	// given
+	app := &apps.App{
+		ID: "/rootGroup/subGroup/subSubGroup/name",
+	}
+	consul := New(ConsulConfig{ConsulNameSeparator: "."})
+
+	// when
+	serviceName := consul.ServiceName(app)
+
+	// then
+	assert.Equal(t, "rootGroup.subGroup.subSubGroup.name", serviceName)
+}
+
+func TestServiceName_WithEmptyConsulLabel(t *testing.T) {
+	t.Parallel()
+
+	// given
+	app := &apps.App{
+		ID:     "/rootGroup/subGroup/subSubGroup/name",
+		Labels: map[string]string{"consul": ""},
+	}
+	consul := New(ConsulConfig{ConsulNameSeparator: "-"})
+
+	// when
+	serviceName := consul.ServiceName(app)
+
+	// then
+	assert.Equal(t, "rootGroup-subGroup-subSubGroup-name", serviceName)
+}
+
+func TestServiceName_WithConsulLabelSetToTrue(t *testing.T) {
+	t.Parallel()
+
+	// given
+	app := &apps.App{
+		ID:     "/rootGroup/subGroup/subSubGroup/name",
+		Labels: map[string]string{"consul": "true"},
+	}
+	consul := New(ConsulConfig{ConsulNameSeparator: "-"})
+
+	// when
+	serviceName := consul.ServiceName(app)
+
+	// then
+	assert.Equal(t, "rootGroup-subGroup-subSubGroup-name", serviceName)
+}
+
+func TestServiceName_WithCustomConsulLabelEscapingChars(t *testing.T) {
+	t.Parallel()
+
+	// given
+	app := &apps.App{
+		ID:     "/rootGroup/subGroup/subSubGroup/name",
+		Labels: map[string]string{"consul": "/some-other/name"},
+	}
+	consul := New(ConsulConfig{ConsulNameSeparator: "-"})
+
+	// when
+	serviceName := consul.ServiceName(app)
+
+	// then
+	assert.Equal(t, "some-other-name", serviceName)
+}
+
+func TestServiceName_WithInvalidLabelValue(t *testing.T) {
+	t.Parallel()
+
+	// given
+	app := &apps.App{
+		ID:     "/rootGroup/subGroup/subSubGroup/name",
+		Labels: map[string]string{"consul": "     ///"},
+	}
+	consul := New(ConsulConfig{ConsulNameSeparator: "-"})
+
+	// when
+	serviceName := consul.ServiceName(app)
+
+	// then
+	assert.Equal(t, "rootGroup-subGroup-subSubGroup-name", serviceName)
+}

consul/consul_test_server.go

@@ -20,8 +20,9 @@ func ConsulClientAtServer(server *testutil.TestServer) *Consul {
 
 func consulClientAtAddress(host string, port int) *Consul {
 	config := ConsulConfig{
-		Timeout: 10 * time.Millisecond,
-		Port:    fmt.Sprintf("%d", port),
+		Timeout:             10 * time.Millisecond,
+		Port:                fmt.Sprintf("%d", port),
+		ConsulNameSeparator: ".",
 	}
 	consul := New(config)
 	// initialize the agents cache with a single client pointing at provided location

debian/config.json

@@ -5,6 +5,7 @@
       "Username": "",
       "Password": ""
     },
+    "ConsulNameSeparator": ".",
     "Port": "8500",
     "SslEnabled": false,
     "SslVerify": true,

events/deployment_info.go

@@ -92,7 +92,7 @@ func (d *DeploymentEvent) RenamedConsulApps() []*apps.App {
 		targetMap := d.appsMap(target)
 		for id, originalApp := range originalMap {
 			targetApp, ok := targetMap[id]
-			if !ok || originalApp.ConsulServiceName() != targetApp.ConsulServiceName() {
+			if !ok || originalApp.ConsulName() != targetApp.ConsulName() {
 				renamedApps = append(renamedApps, originalApp)
 			}
 		}

sync/error_consul_stub_test.go

@@ -26,6 +26,10 @@ func (c errorConsul) Deregister(serviceId apps.TaskId, agent string) error {
 	return fmt.Errorf("Error occured")
 }
 
+func (c errorConsul) ServiceName(app *apps.App) string {
+	return ""
+}
+
 func (c errorConsul) GetAgent(agent string) (*consulapi.Client, error) {
 	return nil, fmt.Errorf("Error occured")
 }

sync/sync_test.go

@@ -160,6 +160,10 @@ func (c *ConsulServicesMock) RegistrationsCount(instanceId string) int {
 	return c.registrations[instanceId]
 }
 
+func (c *ConsulServicesMock) ServiceName(app *apps.App) string {
+	return ""
+}
+
 func (c *ConsulServicesMock) Deregister(serviceId apps.TaskId, agent string) error {
 	return nil
 }

web/event_handler.go

@@ -227,7 +227,7 @@ func (fh *eventHandler) handleDeploymentStepSuccess(body []byte) error {
 func (fh *eventHandler) deregisterAllAppServices(app *apps.App) []error {
 
 	errors := []error{}
-	serviceName := app.ConsulServiceName()
+	serviceName := fh.service.ServiceName(app)
 
 	log.WithField("AppId", app.ID).WithField("ServiceName", serviceName).Info("Deregistering all services")