Add marathon label to configure basic auth #1799

nikore · 2017-06-28T00:25:52Z

Description

Adding in support for configuring basic auth via a marathon label; there are similar functions in docker and the rancher providers.

This addresses the marathon part of #1465

timoreimann

Some comments left.

timoreimann · 2017-07-02T23:23:52Z

provider/marathon/marathon_test.go

+func TestMarathonGetBasicAuth(t *testing.T) {
+	provider := &Provider{}
+
+	applications := []struct {


Could you rename applications to cases?

(I realize you likely just copied this from existing tests; which unfortunately did not pick a better name either.)

I would argue to keep the the style the same an in a different PR correct them all at once. Would be confusing to any one else giving a contribution why there are two styles

Given that we run two or three different test styles in marathon_test.go, things must already be pretty confusing. At this point, I'm only interested in reducing the amount of alignment effort new code brings along until I get my cleanup PR through.

We'll most likely move to cases everywhere in the not-too-remote future, so it's okay to adjust accordingly in your PR.

timoreimann · 2017-07-02T23:25:18Z

provider/marathon/marathon_test.go

+		},
+	}
+
+	for _, a := range applications {


Could you please turn this into a group of parallelized sub-tests given proper names? See some of the other Marathon tests where we already employ those.

Give me a direct example and I can try; this is literally the first goland code (besides basic hello world stuff) I have written; so this is mostly gibberish to me.

Sure thing: Here you go (from an in-flight PR of mine).

Key points:

Capture the loop variable like c := c.

Pass an expressive description/test name as first parameter to t.Run().

Call t.Parallel() first thing inside the body of t.Run().

If you're interested in a bit of background, I suggest this blog post.

Let me know if you have further questions!

timoreimann · 2017-07-02T23:26:49Z

provider/marathon/marathon_test.go

+	for _, a := range applications {
+		actual := provider.getBasicAuth(a.application)
+		if !reflect.DeepEqual(actual, a.expected) {
+			t.Fatalf("expected %q, got %q", a.expected, actual)


Please revert the expected, actual order to match the check order one line above, and also to be in line with the preferred order in Go.

Then it should also be fixed here https://github.com/containous/traefik/blob/master/provider/marathon/marathon_test.go#L1371

Absolutely! Would you mind taking care of it?

timoreimann · 2017-07-02T23:27:27Z

provider/marathon/marathon_test.go

+	for _, a := range applications {
+		actual := provider.getBasicAuth(a.application)
+		if !reflect.DeepEqual(actual, a.expected) {
+			t.Fatalf("expected %q, got %q", a.expected, actual)


Once you've converted the tests into sub-tests, please don't forget to use t.Errorf instead of t.Fatalf.

timoreimann · 2017-07-02T23:29:45Z

docs/toml.md

@@ -1022,6 +1022,7 @@ Labels can be used on containers to override default behaviour:
 - `traefik.frontend.passHostHeader=true`: forward client `Host` header to the backend.
 - `traefik.frontend.priority=10`: override default frontend priority
 - `traefik.frontend.entryPoints=http,https`: assign this frontend to entry points `http` and `https`. Overrides `defaultEntryPoints`.
+- `traefik.frontend.auth.basic=test:$apr1$H6uskkkW$IgXLP6ewTrSuBkTrqE8wj/,test2:$apr1$d9hr9HBB$4HxwgUir3HP4EsggP/QNo0`: Sets a Basic Auth for that frontend with the users test:test and test2:test2


grammar nit-picks:

"Sets a Basic Auth" -> "Sets basic authentication"

"the users test:test and test2:test2" -> "the usernames and passwords test:test and test2:test2, respectively"

If we want to grammar nit-pick then this line should be changed as well since I directly copy pasted it to here https://github.com/containous/traefik/blame/master/docs/toml.md#L915

I agree -- please do.

timoreimann · 2017-07-02T23:30:25Z

provider/marathon/marathon_test.go

@@ -99,6 +99,7 @@ func TestMarathonLoadConfig(t *testing.T) {
 				`frontend-test`: {
 					Backend:        "backend-test",
 					PassHostHeader: true,
+					BasicAuth:      []string{},


Do the unaffected tests fail if we do not add empty BasicAuth slices?

We should not extend them unless it's required.

Yes its required; all of the tests failed given how they just blind compare the structs.

Gotcha.

We could still extract the empty BasicAuth (and EntryPoints, for that matter) into the test body. I'll take care of that in my cleanup PR.

timoreimann

Comment responses. :-)

timoreimann · 2017-07-02T23:57:04Z

docs/toml.md

@@ -1022,6 +1022,7 @@ Labels can be used on containers to override default behaviour:
 - `traefik.frontend.passHostHeader=true`: forward client `Host` header to the backend.
 - `traefik.frontend.priority=10`: override default frontend priority
 - `traefik.frontend.entryPoints=http,https`: assign this frontend to entry points `http` and `https`. Overrides `defaultEntryPoints`.
+- `traefik.frontend.auth.basic=test:$apr1$H6uskkkW$IgXLP6ewTrSuBkTrqE8wj/,test2:$apr1$d9hr9HBB$4HxwgUir3HP4EsggP/QNo0`: Sets a Basic Auth for that frontend with the users test:test and test2:test2


I agree -- please do.

timoreimann · 2017-07-02T23:58:52Z

provider/marathon/marathon_test.go

@@ -99,6 +99,7 @@ func TestMarathonLoadConfig(t *testing.T) {
 				`frontend-test`: {
 					Backend:        "backend-test",
 					PassHostHeader: true,
+					BasicAuth:      []string{},


Gotcha.

We could still extract the empty BasicAuth (and EntryPoints, for that matter) into the test body. I'll take care of that in my cleanup PR.

timoreimann · 2017-07-03T00:04:19Z

provider/marathon/marathon_test.go

+func TestMarathonGetBasicAuth(t *testing.T) {
+	provider := &Provider{}
+
+	applications := []struct {


Given that we run two or three different test styles in marathon_test.go, things must already be pretty confusing. At this point, I'm only interested in reducing the amount of alignment effort new code brings along until I get my cleanup PR through.

We'll most likely move to cases everywhere in the not-too-remote future, so it's okay to adjust accordingly in your PR.

timoreimann · 2017-07-03T00:11:54Z

provider/marathon/marathon_test.go

+		},
+	}
+
+	for _, a := range applications {


Sure thing: Here you go (from an in-flight PR of mine).

Key points:

Capture the loop variable like c := c.

Pass an expressive description/test name as first parameter to t.Run().

Call t.Parallel() first thing inside the body of t.Run().

If you're interested in a bit of background, I suggest this blog post.

Let me know if you have further questions!

timoreimann · 2017-07-03T00:12:57Z

provider/marathon/marathon_test.go

+	for _, a := range applications {
+		actual := provider.getBasicAuth(a.application)
+		if !reflect.DeepEqual(actual, a.expected) {
+			t.Fatalf("expected %q, got %q", a.expected, actual)


Absolutely! Would you mind taking care of it?

timoreimann · 2017-07-03T00:15:46Z

Note to myself: Answering to review comments by another review apparently produces duplicate comments; at least, that's what it looks like on the UI . :/

timoreimann

LGTM!

Thanks a lot for your contribution. 🎉

juliens

LGTM

ldez

Could you change the base of the PR from v1.3 branch to master branch.

ldez

I did the job.

LGTM

…cher providers

ldez added area/authentication area/provider/marathon kind/enhancement a new or improved feature. status/2-needs-review labels Jun 28, 2017

timoreimann suggested changes Jul 2, 2017

View reviewed changes

timoreimann suggested changes Jul 3, 2017

View reviewed changes

ldez requested a review from juliens July 6, 2017 23:09

timoreimann approved these changes Jul 7, 2017

View reviewed changes

juliens approved these changes Jul 7, 2017

View reviewed changes

ldez suggested changes Jul 7, 2017

View reviewed changes

ldez added the contributor/waiting-for-corrections label Jul 7, 2017

ldez changed the base branch from v1.3 to master July 7, 2017 20:33

ldez removed contributor/waiting-for-corrections status/2-needs-review labels Jul 7, 2017

ldez approved these changes Jul 7, 2017

View reviewed changes

ldez added the status/3-needs-merge label Jul 7, 2017

ldez added this to the 1.4 milestone Jul 7, 2017

Add marathon label to configure basic auth, similar to docker and ran…

8ff6347

…cher providers

ldez merged commit 4dfbb6d into traefik:master Jul 7, 2017

ldez removed the status/3-needs-merge label Jul 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add marathon label to configure basic auth #1799

Add marathon label to configure basic auth #1799

nikore commented Jun 28, 2017 •

edited

Loading

timoreimann left a comment

timoreimann Jul 2, 2017

nikore Jul 2, 2017

timoreimann Jul 3, 2017

timoreimann Jul 2, 2017

nikore Jul 2, 2017

timoreimann Jul 3, 2017

timoreimann Jul 2, 2017

nikore Jul 2, 2017

timoreimann Jul 3, 2017

timoreimann Jul 2, 2017

timoreimann Jul 2, 2017

nikore Jul 2, 2017

timoreimann Jul 2, 2017

timoreimann Jul 2, 2017

nikore Jul 2, 2017

timoreimann Jul 2, 2017

timoreimann left a comment

timoreimann Jul 2, 2017

timoreimann Jul 2, 2017

timoreimann Jul 3, 2017

timoreimann Jul 3, 2017

timoreimann Jul 3, 2017

timoreimann commented Jul 3, 2017 •

edited

Loading

timoreimann left a comment

juliens left a comment

ldez left a comment

ldez left a comment

Add marathon label to configure basic auth #1799

Add marathon label to configure basic auth #1799

Conversation

nikore commented Jun 28, 2017 • edited Loading

Description

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

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 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

timoreimann commented Jul 3, 2017 • edited Loading

timoreimann left a comment

Choose a reason for hiding this comment

juliens left a comment

Choose a reason for hiding this comment

ldez left a comment

Choose a reason for hiding this comment

ldez left a comment

Choose a reason for hiding this comment

nikore commented Jun 28, 2017 •

edited

Loading

timoreimann commented Jul 3, 2017 •

edited

Loading