Detect proper hostname automatically. #1345

diegooliveira · 2017-03-26T20:00:58Z

The IP-Per-Task feature changed the behavior for clients without this configuration (using the task IP instead of task hostname).
This patch make the new behavior available just for Mesos installation with IP-Per-Task enabled.
It also make it possible to force the use of task's hostname.

errm · 2017-03-28T10:53:37Z

Looks like the unit tests are failing on this change...

timoreimann · 2017-03-28T12:49:10Z

provider/marathon.go

@@ -526,21 +527,22 @@ func (provider *Marathon) getBackendServer(task marathon.Task, applications []ma
 		log.Errorf("Unable to get marathon application from task %s", task.AppID)
 		return ""
 	}
-	if len(task.IPAddresses) == 0 {
+	if application.IPAddressPerTask == nil || provider.ForceTaskHostname {


Can we turn the block into a switch-case statement? Would make things more readable IMHO.

timoreimann · 2017-03-28T12:53:05Z

provider/marathon_test.go

+	applications := []struct {
+		forceTaskHostname bool
+		applications      []marathon.Application
+		task              marathon.Task


The task seems to be the same for all tests. Please define it in the loop body.

timoreimann · 2017-03-28T12:53:52Z

provider/marathon_test.go

+			forceTaskHostname: true,
+			applications: []marathon.Application{
+				{
+					ID: "app-with-IP-per-task-config",


Can we call this something like app-with-hostname-enforced?

timoreimann · 2017-03-28T12:55:33Z

provider/marathon_test.go

+	}
+
+	for _, app := range applications {
+		provider := &Marathon{}


Please use sub-tests here. You can use the app IDs as descriptions (first parameter to the t.Run() method).

timoreimann · 2017-03-28T13:14:28Z

traefik.sample.toml

@@ -586,6 +586,14 @@
 #
 # keepAlive = 10

+# Force the use of the task's hostname in the backend server address even in a 


I think we should be a bit more elaborative here and describe the default behavior. I'm thinking of something like the following:

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.

Please also copy the description into the docs/toml.md file.

timoreimann · 2017-04-03T22:28:29Z

hey @diegooliveira, any chance to get an update on the PR? Thanks.

emilevauge · 2017-04-06T19:48:51Z

@diegooliveira tests are still failing.

jensendw · 2017-04-13T18:09:32Z

@emilevauge it looks like the tests are passing again. Do you think this will be merged in before the next release?

timoreimann

A few minor-ish details we need to address.

timoreimann · 2017-04-14T00:10:17Z

provider/marathon_test.go

+			} else {
+				// Compare backends
+				if !reflect.DeepEqual(actualConfig.Backends, c.expectedBackends) {
+					expected, _ := json.Marshal(c.expectedBackends)


Using JSON to display differences can be highly misleading: For instance, the serialization sometimes does not distinguish between a nil and an empty slice.

If pointers are involved, please use go-spew.Sdump.

timoreimann · 2017-04-14T06:35:03Z

provider/marathon_test.go

@@ -107,7 +110,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",


Was there a need to change this?

timoreimann · 2017-04-14T06:35:38Z

provider/marathon_test.go

 	"errors"
 	"reflect"
 	"testing"

+	"fmt"


Can you sort the imports alphabetically?

timoreimann · 2017-04-14T06:45:03Z

provider/marathon_test.go

-			// Compare backends
-			if !reflect.DeepEqual(actualConfig.Backends, c.expectedBackends) {
-				t.Fatalf("expected %#v, got %#v", c.expectedBackends, actualConfig.Backends)
+		t.Run(fmt.Sprintf("Running case: %s", appID), func(t *testing.T) {


I don't think you need the "Running case" prefix: it's clear we run a case when being logged.

Can we parallelize the tests? If so, please call t.Parallel() here.

timoreimann · 2017-04-14T06:45:58Z

provider/marathon_test.go

+			fakeClient.AssertExpectations(t)
+			if c.expectedNil {
+				if actualConfig != nil {
+					t.Fatalf("Should have been nil, got %v", actualConfig)


Please say "config should have been nil, got [...]".

timoreimann · 2017-04-14T06:47:14Z

provider/marathon_test.go

+				if !reflect.DeepEqual(actualConfig.Backends, c.expectedBackends) {
+					expected, _ := json.Marshal(c.expectedBackends)
+					actual, _ := json.Marshal(actualConfig.Backends)
+					t.Fatalf("expected\t %s\n, \tgot %s\n", expected, actual)


This should be an t.Errorf IMHO so that we can still see the frontend assertion even if the backend one fails.

timoreimann · 2017-04-14T06:48:34Z

provider/marathon_test.go

+	}
+
+	for _, app := range applications {
+		t.Run(fmt.Sprintf("running %s", app.application.ID), func(t *testing.T) {


No "running" here too.

Can we parallelize the tests? If so, please call t.Parallel() here and do app := app before the call to t.Run to capture the loop variable.

timoreimann · 2017-04-14T06:51:28Z

provider/marathon_test.go

+
+			actual := provider.getBackendServer(task, applications)
+			if actual != app.expected {
+				t.Errorf("App %s, expected %q, got %q", task.AppID, app.expected, actual)


No need to include the app ID in the message: we already get that context by the first parameter to t.Run.

Please swap the expected / got order: the actual value should go first (for consistency reasons and because that's Go-typical).

timoreimann · 2017-04-18T06:42:20Z

Hey @diegooliveira, do you have time for a final update? Would be great to get this feature merged before the code freeze.

timoreimann · 2017-04-22T21:07:09Z

Wanted to make sure this important PR makes it into the upcoming version 1.3 release, so I carried the PR "in-place". Here's what I did:

Addressed my own comments.
Completed the unit tests to cover all error paths.
A bit of refactoring.
Rebased the PR onto Check for explicitly defined Marathon port first. #1474 which had already refactored some of the production and test code this PR needs.

@diegooliveira I hope all of this is okay with you.

timoreimann · 2017-04-22T21:08:12Z

Fixes #1243.

timoreimann

Unsurprisingly, I approve my own changes. :-)

timoreimann · 2017-04-22T21:10:01Z

@containous/traefik please review.

diegooliveira · 2017-04-23T20:57:08Z

@timoreimann nice work, sorry for the delay, busy month. The addition of spew was a nice touch.

ldez

LGTM.
You must solve conflicts before 😉

The IP-Per-Task feature changed the behavior for clients without this configuration (using the task IP instead of task hostname). This patch make the new behavior available just for Mesos installation with IP-Per-Task enabled. It also make it possible to force the use of task's hostname.

Allows to move specific test cases to dedicated tests for new function.

- Cover error cases. - Use sub-tests.

diegooliveira mentioned this pull request Mar 26, 2017

IP address + exported port instead of endpoint from marathon #1243

Closed

diegooliveira force-pushed the IP-Per-Task-Fix-Hostname branch 2 times, most recently from 4a8390a to 9cee32c Compare March 26, 2017 20:53

timoreimann suggested changes Mar 28, 2017

View reviewed changes

timoreimann mentioned this pull request Mar 29, 2017

Incorrect backend in marathon provider #1362

Closed

emilevauge added the contributor/waiting-for-corrections label Apr 4, 2017

diegooliveira force-pushed the IP-Per-Task-Fix-Hostname branch from 9cee32c to 1f0d0ee Compare April 5, 2017 05:50

diegooliveira force-pushed the IP-Per-Task-Fix-Hostname branch from 1f0d0ee to c0c556b Compare April 6, 2017 21:55

timoreimann suggested changes Apr 14, 2017

View reviewed changes

timoreimann force-pushed the IP-Per-Task-Fix-Hostname branch 2 times, most recently from bc4173b to a8be80b Compare April 22, 2017 21:03

timoreimann changed the base branch from master to marathon-check-port-label-overwrite-earlier April 22, 2017 21:03

timoreimann added bug kind/enhancement a new or improved feature. area/provider/marathon status/2-needs-review and removed contributor/waiting-for-corrections labels Apr 22, 2017

timoreimann approved these changes Apr 22, 2017

View reviewed changes

timoreimann requested review from vdemeester and emilevauge April 22, 2017 21:09

timoreimann force-pushed the IP-Per-Task-Fix-Hostname branch from a8be80b to a3c8c99 Compare April 22, 2017 21:17

timoreimann added this to the 1.3 milestone Apr 22, 2017

timoreimann changed the title ~~Fix unsound behavior~~ [Marathon] Detect proper hostname automatically. Apr 22, 2017

timoreimann force-pushed the IP-Per-Task-Fix-Hostname branch from a3c8c99 to 6cd18dc Compare April 22, 2017 21:47

ldez added kind/bug/fix a bug fix and removed bug labels Apr 25, 2017

timoreimann force-pushed the marathon-check-port-label-overwrite-earlier branch from cb7f4bd to 099d605 Compare April 25, 2017 21:18

ldez approved these changes Apr 26, 2017

View reviewed changes

Diego de Oliveira and others added 5 commits April 26, 2017 11:35

Use go-spew to display diffs.

e615e83

Cosmetic changes.

16c8602

Extract index functionality into generic helper function.

a435556

Allows to move specific test cases to dedicated tests for new function.

Improve and extend TestGetBackendServer.

7eb3051

- Cover error cases. - Use sub-tests.

timoreimann force-pushed the IP-Per-Task-Fix-Hostname branch from 6cd18dc to 7eb3051 Compare April 26, 2017 09:36

timoreimann changed the base branch from marathon-check-port-label-overwrite-earlier to master April 26, 2017 09:36

timoreimann merged commit 5a8215a into traefik:master Apr 26, 2017

timoreimann added status/3-needs-merge and removed status/2-needs-review labels Apr 27, 2017

timoreimann mentioned this pull request Apr 30, 2017

[Rancher] No IP in backend in host networking mode #1498

Closed

ldez removed the status/3-needs-merge label May 19, 2017

ldez changed the title ~~[Marathon] Detect proper hostname automatically.~~ Detect proper hostname automatically. May 25, 2017

This comment has been minimized.

Sign in to view

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detect proper hostname automatically. #1345

Detect proper hostname automatically. #1345

diegooliveira commented Mar 26, 2017 •

edited by ldez

errm commented Mar 28, 2017

timoreimann Mar 28, 2017

timoreimann Mar 28, 2017

timoreimann Mar 28, 2017

timoreimann Mar 28, 2017

timoreimann Mar 28, 2017

timoreimann commented Apr 3, 2017

emilevauge commented Apr 6, 2017

jensendw commented Apr 13, 2017

timoreimann left a comment

timoreimann Apr 14, 2017

timoreimann Apr 14, 2017

timoreimann Apr 14, 2017

timoreimann Apr 14, 2017

timoreimann Apr 14, 2017

timoreimann Apr 14, 2017

timoreimann Apr 14, 2017

timoreimann Apr 14, 2017

timoreimann Apr 14, 2017

timoreimann Apr 14, 2017

timoreimann Apr 14, 2017

timoreimann commented Apr 18, 2017

timoreimann commented Apr 22, 2017 •

edited

timoreimann commented Apr 22, 2017

timoreimann left a comment

timoreimann commented Apr 22, 2017

diegooliveira commented Apr 23, 2017

ldez left a comment

This comment has been minimized.

Detect proper hostname automatically. #1345

Detect proper hostname automatically. #1345

Conversation

diegooliveira commented Mar 26, 2017 • edited by ldez

errm commented Mar 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timoreimann commented Apr 3, 2017

emilevauge commented Apr 6, 2017

jensendw commented Apr 13, 2017

timoreimann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timoreimann commented Apr 18, 2017

timoreimann commented Apr 22, 2017 • edited

timoreimann commented Apr 22, 2017

timoreimann left a comment

Choose a reason for hiding this comment

timoreimann commented Apr 22, 2017

diegooliveira commented Apr 23, 2017

ldez left a comment

Choose a reason for hiding this comment

This comment has been minimized.

diegooliveira commented Mar 26, 2017 •

edited by ldez

timoreimann commented Apr 22, 2017 •

edited