Merge pull request #1345 from diegooliveira/IP-Per-Task-Fix-Hostname

[Marathon] Detect proper hostname automatically.

docs/toml.md

@@ -969,6 +969,16 @@ domain = "marathon.localhost"
 # Default: "10s"
 #
 # keepAlive = "10s"
+
+# By default, a task's IP address (as returned by the Marathon API) is used as 
+# backend server if an IP-per-task configuration can be found; otherwise, the
+# name of the host running the task is used.
+# The latter behavior can be enforced by enabling this switch.
+# 
+# Optional
+# Default: false
+#
+# forceTaskHostname: false 
 ```
 
 Labels can be used on containers to override default behaviour:

provider/marathon/marathon.go

@@ -42,6 +42,7 @@ type Provider struct {
 	TLS                     *provider.ClientTLS `description:"Enable Docker TLS support"`
 	DialerTimeout           flaeg.Duration      `description:"Set a non-default connection timeout for Marathon"`
 	KeepAlive               flaeg.Duration      `description:"Set a non-default TCP Keep Alive time in seconds"`
+	ForceTaskHostname       bool                `description:"Force to use the task's hostname."`
 	Basic                   *Basic
 	marathonClient          marathon.Marathon
 }
@@ -481,12 +482,9 @@ func processPorts(application marathon.Application, task marathon.Task) (int, er
 	portIndexLabel, ok := (*application.Labels)[labelPortIndex]
 	if ok {
 		var err error
-		portIndex, err = strconv.Atoi(portIndexLabel)
+		portIndex, err = parseIndex(portIndexLabel, len(ports))
-		switch {
+		if err != nil {
-		case err != nil:
+			return 0, fmt.Errorf("cannot use port index to select from %d ports: %s", len(ports), err)
-			return 0, fmt.Errorf("failed to parse port index label: %s", err)
-		case portIndex < 0, portIndex > len(ports)-1:
-			return 0, fmt.Errorf("port index %d must be within port range (0, %d)", portIndex, len(ports)-1)
 		}
 	}
 	return ports[portIndex], nil
@@ -526,21 +524,41 @@ func (p *Provider) getBackendServer(task marathon.Task, applications []marathon.
 		log.Errorf("Unable to get marathon application from task %s", task.AppID)
 		return ""
 	}
-	if len(task.IPAddresses) == 0 {
+
+	numTaskIPAddresses := len(task.IPAddresses)
+	switch {
+	case application.IPAddressPerTask == nil || p.ForceTaskHostname:
+		return task.Host
+	case numTaskIPAddresses == 0:
+		log.Errorf("Missing IP address for Marathon application %s on task %s", application.ID, task.ID)
 		return ""
-	} else if len(task.IPAddresses) == 1 {
+	case numTaskIPAddresses == 1:
 		return task.IPAddresses[0].IPAddress
-	} else {
+	default:
 		ipAddressIdxStr, ok := p.getLabel(application, "traefik.ipAddressIdx")
 		if !ok {
-			log.Errorf("Unable to get marathon IPAddress from task %s", task.AppID)
+			log.Errorf("Found %d task IP addresses but missing IP address index for Marathon application %s on task %s", numTaskIPAddresses, application.ID, task.ID)
 			return ""
 		}
-		ipAddressIdx, err := strconv.Atoi(ipAddressIdxStr)
+
+		ipAddressIdx, err := parseIndex(ipAddressIdxStr, numTaskIPAddresses)
 		if err != nil {
-			log.Errorf("Invalid marathon IPAddress from task %s", task.AppID)
+			log.Errorf("Cannot use IP address index to select from %d task IP addresses for Marathon application %s on task %s: %s", numTaskIPAddresses, application.ID, task.ID, err)
 			return ""
 		}
+
 		return task.IPAddresses[ipAddressIdx].IPAddress
 	}
 }
+
+func parseIndex(index string, length int) (int, error) {
+	parsed, err := strconv.Atoi(index)
+	switch {
+	case err != nil:
+		return 0, fmt.Errorf("failed to parse index '%s': %s", index, err)
+	case parsed < 0, parsed > length-1:
+		return 0, fmt.Errorf("index %d must be within range (0, %d)", parsed, length-1)
+	}
+
+	return parsed, nil
+}

provider/marathon/marathon_test.go

@@ -5,9 +5,12 @@ import (
 	"reflect"
 	"testing"
 
+	"fmt"
+
 	"github.com/containous/traefik/mocks"
 	"github.com/containous/traefik/testhelpers"
 	"github.com/containous/traefik/types"
+	"github.com/davecgh/go-spew/spew"
 	"github.com/gambol99/go-marathon"
 	"github.com/stretchr/testify/mock"
 )
@@ -108,7 +111,7 @@ func TestMarathonLoadConfig(t *testing.T) {
 				"backend-test": {
 					Servers: map[string]types.Server{
 						"server-test": {
-							URL:    "http://127.0.0.1:80",
+							URL:    "http://localhost:80",
 							Weight: 0,
 						},
 					},
@@ -161,7 +164,7 @@ func TestMarathonLoadConfig(t *testing.T) {
 				"backend-testLoadBalancerAndCircuitBreaker.dot": {
 					Servers: map[string]types.Server{
 						"server-testLoadBalancerAndCircuitBreaker-dot": {
-							URL:    "http://127.0.0.1:80",
+							URL:    "http://localhost:80",
 							Weight: 0,
 						},
 					},
@@ -219,7 +222,7 @@ func TestMarathonLoadConfig(t *testing.T) {
 				"backend-testMaxConn": {
 					Servers: map[string]types.Server{
 						"server-testMaxConn": {
-							URL:    "http://127.0.0.1:80",
+							URL:    "http://localhost:80",
 							Weight: 0,
 						},
 					},
@@ -274,7 +277,7 @@ func TestMarathonLoadConfig(t *testing.T) {
 				"backend-testMaxConnOnlySpecifyAmount": {
 					Servers: map[string]types.Server{
 						"server-testMaxConnOnlySpecifyAmount": {
-							URL:    "http://127.0.0.1:80",
+							URL:    "http://localhost:80",
 							Weight: 0,
 						},
 					},
@@ -326,7 +329,7 @@ func TestMarathonLoadConfig(t *testing.T) {
 				"backend-testMaxConnOnlyExtractorFunc": {
 					Servers: map[string]types.Server{
 						"server-testMaxConnOnlyExtractorFunc": {
-							URL:    "http://127.0.0.1:80",
+							URL:    "http://localhost:80",
 							Weight: 0,
 						},
 					},
@@ -337,6 +340,12 @@ func TestMarathonLoadConfig(t *testing.T) {
 	}
 
 	for _, c := range cases {
+		appID := ""
+		if len(c.applications.Apps) > 0 {
+			appID = c.applications.Apps[0].ID
+		}
+		t.Run(fmt.Sprintf("app ID: %s", appID), func(t *testing.T) {
+			t.Parallel()
 			fakeClient := newFakeClient(c.applicationsError, c.applications, c.tasksError, c.tasks)
 			provider := &Provider{
 				Domain:           "docker.localhost",
@@ -347,17 +356,18 @@ func TestMarathonLoadConfig(t *testing.T) {
 			fakeClient.AssertExpectations(t)
 			if c.expectedNil {
 				if actualConfig != nil {
-				t.Fatalf("Should have been nil, got %v", actualConfig)
+					t.Fatalf("configuration should have been nil, got %v", actualConfig)
 				}
 			} else {
 				// Compare backends
 				if !reflect.DeepEqual(actualConfig.Backends, c.expectedBackends) {
-				t.Fatalf("expected %#v, got %#v", c.expectedBackends, actualConfig.Backends)
+					t.Errorf("got backend %v, want %v", spew.Sdump(actualConfig.Backends), spew.Sdump(c.expectedBackends))
 				}
 				if !reflect.DeepEqual(actualConfig.Frontends, c.expectedFrontends) {
-				t.Fatalf("expected %#v, got %#v", c.expectedFrontends, actualConfig.Frontends)
+					t.Errorf("got frontend %v, want %v", spew.Sdump(actualConfig.Frontends), spew.Sdump(c.expectedFrontends))
 				}
 			}
+		})
 	}
 }
 
@@ -1057,38 +1067,6 @@ func TestMarathonGetPort(t *testing.T) {
 			},
 			expected: "",
 		},
-		{
-			desc: "sub-zero port index specified",
-			applications: []marathon.Application{
-				{
-					ID: "app",
-					Labels: &map[string]string{
-						"traefik.portIndex": "-1",
-					},
-				},
-			},
-			task: marathon.Task{
-				AppID: "app",
-				Ports: []int{80},
-			},
-			expected: "",
-		},
-		{
-			desc: "too high port index specified",
-			applications: []marathon.Application{
-				{
-					ID: "app",
-					Labels: &map[string]string{
-						"traefik.portIndex": "42",
-					},
-				},
-			},
-			task: marathon.Task{
-				AppID: "app",
-				Ports: []int{80, 443},
-			},
-			expected: "",
-		},
 		{
 			desc: "task port preferred over application port",
 			applications: []marathon.Application{
@@ -1451,3 +1429,200 @@ func TestMarathonGetSubDomain(t *testing.T) {
 		}
 	}
 }
+
+func TestGetBackendServer(t *testing.T) {
+	appID := "appId"
+	host := "host"
+	tests := []struct {
+		desc              string
+		application       marathon.Application
+		addIPAddrPerTask  bool
+		task              marathon.Task
+		forceTaskHostname bool
+		wantServer        string
+	}{
+		{
+			desc:        "application missing",
+			application: marathon.Application{ID: "other"},
+			wantServer:  "",
+		},
+		{
+			desc:       "application without IP-per-task",
+			wantServer: host,
+		},
+		{
+			desc:              "task hostname override",
+			addIPAddrPerTask:  true,
+			forceTaskHostname: true,
+			wantServer:        host,
+		},
+		{
+			desc: "task IP address missing",
+			task: marathon.Task{
+				IPAddresses: []*marathon.IPAddress{},
+			},
+			addIPAddrPerTask: true,
+			wantServer:       "",
+		},
+		{
+			desc: "single task IP address",
+			task: marathon.Task{
+				IPAddresses: []*marathon.IPAddress{
+					{
+						IPAddress: "1.1.1.1",
+					},
+				},
+			},
+			addIPAddrPerTask: true,
+			wantServer:       "1.1.1.1",
+		},
+		{
+			desc: "multiple task IP addresses without index label",
+			task: marathon.Task{
+				IPAddresses: []*marathon.IPAddress{
+					{
+						IPAddress: "1.1.1.1",
+					},
+					{
+						IPAddress: "2.2.2.2",
+					},
+				},
+			},
+			addIPAddrPerTask: true,
+			wantServer:       "",
+		},
+		{
+			desc: "multiple task IP addresses with invalid index label",
+			application: marathon.Application{
+				Labels: &map[string]string{"traefik.ipAddressIdx": "invalid"},
+			},
+			task: marathon.Task{
+				IPAddresses: []*marathon.IPAddress{
+					{
+						IPAddress: "1.1.1.1",
+					},
+					{
+						IPAddress: "2.2.2.2",
+					},
+				},
+			},
+			addIPAddrPerTask: true,
+			wantServer:       "",
+		},
+		{
+			desc: "multiple task IP addresses with valid index label",
+			application: marathon.Application{
+				Labels: &map[string]string{"traefik.ipAddressIdx": "1"},
+			},
+			task: marathon.Task{
+				IPAddresses: []*marathon.IPAddress{
+					{
+						IPAddress: "1.1.1.1",
+					},
+					{
+						IPAddress: "2.2.2.2",
+					},
+				},
+			},
+			addIPAddrPerTask: true,
+			wantServer:       "2.2.2.2",
+		},
+	}
+
+	for _, test := range tests {
+		test := test
+		t.Run(test.desc, func(t *testing.T) {
+			t.Parallel()
+			provider := &Provider{ForceTaskHostname: test.forceTaskHostname}
+
+			// Populate application.
+			if test.application.ID == "" {
+				test.application.ID = appID
+			}
+			if test.application.Labels == nil {
+				test.application.Labels = &map[string]string{}
+			}
+			if test.addIPAddrPerTask {
+				test.application.IPAddressPerTask = &marathon.IPAddressPerTask{
+					Discovery: &marathon.Discovery{
+						Ports: &[]marathon.Port{
+							{
+								Number: 8000,
+								Name:   "port",
+							},
+						},
+					},
+				}
+			}
+			applications := []marathon.Application{test.application}
+
+			// Populate task.
+			test.task.AppID = appID
+			test.task.Host = "host"
+
+			gotServer := provider.getBackendServer(test.task, applications)
+
+			if gotServer != test.wantServer {
+				t.Errorf("got server '%s', want '%s'", gotServer, test.wantServer)
+			}
+		})
+	}
+}
+
+func TestParseIndex(t *testing.T) {
+	tests := []struct {
+		idxStr        string
+		length        int
+		shouldSucceed bool
+		parsed        int
+	}{
+		{
+			idxStr:        "illegal",
+			length:        42,
+			shouldSucceed: false,
+		},
+		{
+			idxStr:        "-1",
+			length:        42,
+			shouldSucceed: false,
+		},
+		{
+			idxStr:        "10",
+			length:        1,
+			shouldSucceed: false,
+		},
+		{
+			idxStr:        "10",
+			length:        10,
+			shouldSucceed: false,
+		},
+		{
+			idxStr:        "0",
+			length:        1,
+			shouldSucceed: true,
+			parsed:        0,
+		},
+		{
+			idxStr:        "10",
+			length:        11,
+			shouldSucceed: true,
+			parsed:        10,
+		},
+	}
+
+	for _, test := range tests {
+		test := test
+		t.Run(fmt.Sprintf("parseIndex(%s, %d)", test.idxStr, test.length), func(t *testing.T) {
+			t.Parallel()
+			parsed, err := parseIndex(test.idxStr, test.length)
+
+			if test.shouldSucceed != (err == nil) {
+				t.Fatalf("got error '%s', want error: %t", err, !test.shouldSucceed)
+			}
+
+			if test.shouldSucceed && parsed != test.parsed {
+				t.Errorf("got parsed index %d, want %d", parsed, test.parsed)
+			}
+		})
+	}
+}

traefik.sample.toml

@@ -599,6 +599,16 @@
 #
 # keepAlive = "10s"
 
+# By default, a task's IP address (as returned by the Marathon API) is used as 
+# backend server if an IP-per-task configuration can be found; otherwise, the
+# name of the host running the task is used.
+# The latter behavior can be enforced by enabling this switch.
+# 
+# Optional
+# Default: false
+#
+# forceTaskHostname: false 
+
 ################################################################
 # Mesos configuration backend
 ################################################################