Marathon constraints filtering #2388

aantono · 2017-11-10T04:07:48Z

What does this PR do?

Add support for filtering based on Marathon-defined constraints, as described in https://mesosphere.github.io/marathon/docs/constraints.html

Motivation

Many Marathon clusters have applications deployed with various "constraints", configuring things like application service affinity, cluster binding, etc. It would be very helpful to not have to add additional/duplicate traefik.tags labels to the applications and to be able to filter using already defined Marathon constraints instead.

More

Added/updated tests
Added/updated documentation

ldez · 2017-11-16T10:22:35Z

@timoreimann WDYT ?

timoreimann

The feature definitely makes sense to me since Marathon constraints are a very canonical way to filter with this provider.

I left a few comments.

timoreimann · 2017-11-16T18:24:43Z

provider/marathon/marathon.go

@@ -59,6 +59,7 @@ type Provider struct {
 	GroupsAsSubDomains      bool             `description:"Convert Marathon groups to subdomains" export:"true"`
 	DCOSToken               string           `description:"DCOSToken for DCOS environment, This will override the Authorization header" export:"true"`
 	MarathonLBCompatibility bool             `description:"Add compatibility with marathon-lb labels" export:"true"`
+	MarathonConstraints     bool             `description:"Enable use of Marathon constraints in constraint filtering" export:"true"`


Since this is a boolean flag, can we have a verb-ish prefix? How about something like filterMarathonConstraints?

timoreimann · 2017-11-16T18:35:37Z

provider/marathon/marathon.go

@@ -247,6 +248,13 @@ func (p *Provider) applicationFilter(app marathon.Application) bool {
 			constraintTags = append(constraintTags, label)
 		}
 	}
+	if p.MarathonConstraints {
+		if app.Constraints != nil && len(*app.Constraints) > 0 {


I believe we can drop the right-hand expression and merge the left-hand one with the previous line like this:

if p.MarathonConstraints && app.Constraints != nil { for _, list := range *app.Constraints {

timoreimann · 2017-11-16T18:40:34Z

provider/marathon/marathon_test.go

+			desc:                    "constraint valid",
+			application:             application(constraint("valid")),
+			marathonLBCompatibility: false,
+			marathonConstraints:     true,


~~Can we group by expected outcome, going with false first?~~ Nevermind actually, it doesn't make sense here.

timoreimann · 2017-11-16T18:42:41Z

docs/configuration/backends/marathon.md

@@ -68,6 +68,13 @@ domain = "marathon.localhost"
 #
 # marathonLBCompatibility = true

+# Enable marathon constraints.


I think we should elaborate just a bit what "enable" means (i.e., that we parse Marathon constraints and treat the compound constraint parts as a single tag to match literally).

Shall we phrase this as Enable filtering by Marathon constraints.?

timoreimann · 2017-11-16T21:31:45Z

provider/marathon/builder_test.go

@@ -44,6 +44,12 @@ func label(key, value string) func(*marathon.Application) {
 	}
 }

+func constraint(value string) func(*marathon.Application) {


Looking at the function signature, it might not be immediately clear to the caller that colons must be used as separators. I know it's only a few lines below, but how about making the function variadic at the second parameter (i.e., constraint(value1 string, values... string) func(*marathon.Application)) to increase clarity?

I was really trying to keep it as a single string, as in the Marathon config (not using the json API), it is defined as a single string with 3 components, separated with a :. Even the Marathon UI uses just a single text field to add the constrain labels, so I'd rather keep this aligned, to be consistent.

That's a reasonable argument. Let's keep a single string then.

timoreimann · 2017-11-16T21:34:26Z

provider/marathon/marathon.go

@@ -247,6 +248,13 @@ func (p *Provider) applicationFilter(app marathon.Application) bool {
 			constraintTags = append(constraintTags, label)
 		}
 	}
+	if p.MarathonConstraints {
+		if app.Constraints != nil && len(*app.Constraints) > 0 {
+			for _, list := range *app.Constraints {


Can we rename list to constraintParts? That's also how the official documentation calls them.

timoreimann · 2017-11-16T22:16:48Z

provider/marathon/marathon_test.go

 		expected                bool
 	}{
 		{
 			desc:                    "tags missing",
 			application:             application(),
 			marathonLBCompatibility: false,
+			marathonConstraints:     false,


Should we omit marathonConstraint (and possibly other parameters) when they are not needed / relevant for a test?

timoreimann

Some small bits left.

timoreimann · 2017-11-18T08:33:42Z

docs/configuration/backends/marathon.md

@@ -68,6 +68,13 @@ domain = "marathon.localhost"
 #
 # marathonLBCompatibility = true

+# Enable marathon constraints.


Shall we phrase this as Enable filtering by Marathon constraints.?

timoreimann · 2017-11-18T08:34:01Z

docs/configuration/backends/marathon.md

@@ -68,6 +68,16 @@ domain = "marathon.localhost"
 #
 # marathonLBCompatibility = true

+# Enable marathon constraints.
+# If enabled, Traefik will read Marathon constrains, as defined in https://mesosphere.github.io/marathon/docs/constraints.html


Typo: constraints

timoreimann · 2017-11-18T08:34:14Z

docs/configuration/backends/marathon.md

@@ -68,6 +68,16 @@ domain = "marathon.localhost"
 #
 # marathonLBCompatibility = true

+# Enable marathon constraints.
+# If enabled, Traefik will read Marathon constrains, as defined in https://mesosphere.github.io/marathon/docs/constraints.html
+# Each individual constraint will be treated as a verbatum compounded tag. 


Typo: verbatim

timoreimann · 2017-11-18T08:34:27Z

docs/configuration/backends/marathon.md

+# Optional
+# Default: false
+#
+# marathonConstraints = true


We need to update the name

timoreimann · 2017-11-18T08:34:53Z

provider/marathon/marathon.go

@@ -247,6 +248,11 @@ func (p *Provider) applicationFilter(app marathon.Application) bool {
 			constraintTags = append(constraintTags, label)
 		}
 	}
+	if p.FilterMarathonConstraints && app.Constraints != nil && len(*app.Constraints) > 0 {


We can omit the last expression on the constraint length.

timoreimann · 2017-11-18T08:35:22Z

provider/marathon/marathon_test.go

@@ -522,6 +522,7 @@ func TestMarathonApplicationFilterConstraints(t *testing.T) {
 		desc                    string
 		application             marathon.Application
 		marathonLBCompatibility bool
+		marathonConstraints     bool


Let's prepend this by filter too to match the real flag name.

timoreimann

LGTM -- thanks, Alex. 👏

juliens

LGTM

ldez

LGTM

aantono requested a review from a team as a code owner November 10, 2017 04:07

traefiker added area/provider/marathon size/M status/0-needs-triage labels Nov 10, 2017

aantono force-pushed the marathon-constraints branch 3 times, most recently from e2cfb14 to 2e29778 Compare November 10, 2017 04:36

ldez added kind/enhancement a new or improved feature. status/2-needs-review and removed status/0-needs-triage labels Nov 10, 2017

aantono force-pushed the marathon-constraints branch from 2e29778 to 2e632a0 Compare November 10, 2017 16:06

timoreimann suggested changes Nov 16, 2017

View reviewed changes

traefiker added the contributor/waiting-for-corrections label Nov 16, 2017

timoreimann suggested changes Nov 18, 2017

View reviewed changes

aantono force-pushed the marathon-constraints branch from 04539de to 4946c6f Compare November 18, 2017 21:39

ldez removed the contributor/waiting-for-corrections label Nov 18, 2017

timoreimann approved these changes Nov 19, 2017

View reviewed changes

juliens approved these changes Nov 21, 2017

View reviewed changes

ldez added this to the 1.5 milestone Nov 21, 2017

ldez added status/3-needs-merge and removed status/2-needs-review labels Nov 21, 2017

ldez approved these changes Nov 21, 2017

View reviewed changes

traefiker added the bot/need-human-merge label Nov 21, 2017

mmatur removed the bot/need-human-merge label Nov 21, 2017

traefiker added the status/4-merge-in-progress label Nov 21, 2017

aantono added 3 commits November 21, 2017 09:33

Added support for marathon constraint filtering

3966fa5

Added doc description and code cleanup

9163378

Renamed property to filterMarathonConstraints

bc4a411

traefiker force-pushed the marathon-constraints branch from 4946c6f to bc4a411 Compare November 21, 2017 09:33

traefiker removed the status/4-merge-in-progress label Nov 21, 2017

traefiker merged commit 4b91204 into traefik:master Nov 21, 2017

traefiker removed the status/3-needs-merge label Nov 21, 2017

deric mentioned this pull request Mar 1, 2018

Update Traefik to 1.5.3 d2iq-archive/universe#1720

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Marathon constraints filtering #2388

Marathon constraints filtering #2388

aantono commented Nov 10, 2017 •

edited by nmengin

Loading

ldez commented Nov 16, 2017

timoreimann left a comment

timoreimann Nov 16, 2017

timoreimann Nov 16, 2017

timoreimann Nov 16, 2017 •

edited

Loading

timoreimann Nov 16, 2017

aantono Nov 18, 2017

timoreimann Nov 18, 2017

timoreimann Nov 16, 2017

aantono Nov 18, 2017

timoreimann Nov 18, 2017

timoreimann Nov 16, 2017

timoreimann Nov 16, 2017

timoreimann left a comment

timoreimann Nov 18, 2017

timoreimann Nov 18, 2017

timoreimann Nov 18, 2017

timoreimann Nov 18, 2017

timoreimann Nov 18, 2017

timoreimann Nov 18, 2017

timoreimann left a comment

juliens left a comment

ldez left a comment

Marathon constraints filtering #2388

Marathon constraints filtering #2388

Conversation

aantono commented Nov 10, 2017 • edited by nmengin Loading

What does this PR do?

Motivation

More

ldez commented Nov 16, 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

timoreimann Nov 16, 2017 • edited Loading

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

Choose a reason for hiding this comment

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

aantono commented Nov 10, 2017 •

edited by nmengin

Loading

timoreimann Nov 16, 2017 •

edited

Loading