feat(constraints): Implementation of constraint filtering (cmd + toml + matching functions), implementation proposal with consul #342

samber · 2016-04-29T14:36:10Z

emilevauge · 2016-05-04T14:36:03Z

Hi @samber thanks a lot for your great work!
What do you think of simplifying this a bit by moving constraints inside GlobalConfiguration, instead of being defined in each provider?
I think constraints should be defined for the whole instance, and be used by each provider.
WDYT?
/cc @keis, @vdemeester

keis · 2016-05-04T15:02:52Z

@emilevauge constraints defined for the instance sounds like the right way to go.

One potential problem with that is if there is a difference in what "constraint types" the different backends support. Right now the only supported type is matching on "tags" but I guess there could be others? like the overlay network for docker but that would not make any sense for consul. But I don't think it worth worrying to much about :)

keis · 2016-05-04T15:10:54Z

provider/provider.go

+func (p *BaseProvider) matchConstraintWithAtLeastOneTag(tags []string, constraint *types.Constraint) (bool, error) {
+	// At the moment, it only supports tags
+	if constraint.Key != "tag" {
+		return false, errors.New("Constraint must be tag-based. Syntax: tag==us-*")


I would verify this already in NewConstraint, I also think this whole function should actually be method of Constraint because that's the only instance data it operates on

emilevauge · 2016-05-07T10:49:54Z

@keis, you're right, maybe we could keep the providers constraints and add global constraints?
I think that global constraints are much simpler in the multiple traefik instances per environment use case :)

samber · 2016-05-09T15:17:00Z

Improvement in the last commit:

@emilevauge : global constraints + provider-specific constraints
@keis : refactored ;-)

Please note that MatchConstraints returns a new variable: failingConstraint (or nil), for logging purpose.

emilevauge · 2016-05-12T12:54:48Z

@samber validation error ;)

samber · 2016-05-12T14:45:50Z

Yes I didn't finish working on that.

if everybody agrees on implementation, it's time for me to write some tests ;-)

vdemeester · 2016-05-12T15:01:39Z

After reading #311 (comment) I like the idea, but as @emilevauge I like the idea of having global constaints to (no matter the provider used) and maybe merge them.

On the implementation side, for docker, the way to filter the contstaint would definitely be labels (but with a mix of container labels and node/host labels).

I agree with the design, and it's probably easier for now (and for the scope of this PR) to not implement global constraint right away 😉

emilevauge · 2016-05-12T15:08:26Z

OK thanks @vdemeester!
@samber I think everybody is OK on the implementation :)

keis · 2016-05-13T06:49:57Z

Concept ACK 👍

samber · 2016-05-25T16:13:28Z

It's alright on my side. Ready for review and merge ;-)

cc @pnegahdar

keis · 2016-05-26T09:08:42Z

provider/provider.go

+	}
+
+	for _, constraint := range p.Constraints {
+		if ok := constraint.MatchConstraintWithAtLeastOneTag(tags); xor(ok == true, constraint.MustMatch == true) {


bool == true is a bit redundant so this would be clearer to write as simply ok != constraint.MustMatch

emilevauge · 2016-05-26T15:17:19Z

cmd.go

 		Docker: &provider.Docker{
 			TLS: &provider.DockerTLS{},
+			// @todo: Docker constraints


sed 's/@todo/TODO/g'

emilevauge · 2016-05-26T15:30:02Z

Thanks a lot @samber ! Great job :)
Few minor comments, but other than that, looks good to me.
/cc @vdemeester

samber · 2016-05-31T12:11:33Z

I did a large refacto because of the new cli library.

Ready for review ;-)

vdemeester · 2016-05-31T12:16:55Z

types/types.go

+	Key string
+	// MustMatch is true if operator is "==" or false if operator is "!="
+	MustMatch bool
+	// TODO: support regex


Is this TODO still to do ? 😛

Yes, today, it's only supporting globing in a string. Not using RE2 syntax

emilevauge · 2016-05-31T20:08:53Z

server.go

@@ -248,7 +248,7 @@ func (server *Server) startProviders() {
 		log.Infof("Starting provider %v %s", reflect.TypeOf(provider), jsonConf)
 		currentProvider := provider
 		safe.Go(func() {
-			err := currentProvider.Provide(server.configurationChan, &server.routinesPool)
+			err := currentProvider.Provide(server.configurationChan, &server.routinesPool, server.globalConfiguration.Constraints.Get().([]types.Constraint))


Isn't server.globalConfiguration.Constraints sufficient instead of server.globalConfiguration.Constraints.Get().([]types.Constraint) ?

Arf, my bad ;-)

emilevauge · 2016-05-31T20:10:14Z

@samber impressive job 😻
Only one comment on the code.
Once rebased, LGTM!
/cc @vdemeester

… + matching functions), implementation proposal with consul

…ection

samber · 2016-06-01T08:32:53Z

Fixed and rebased 👍

vdemeester · 2016-06-01T14:00:54Z

Very good job indeed 👍
LGTM 🐮

samber force-pushed the TRAEFIK-311--adding-constraint-filtering branch 3 times, most recently from 6112281 to c9f11cc Compare April 30, 2016 20:33

keis reviewed May 4, 2016
View reviewed changes

samber force-pushed the TRAEFIK-311--adding-constraint-filtering branch from c9f11cc to 23c077f Compare May 9, 2016 15:12

vdemeester added the status/1-needs-design-review label May 12, 2016

samber force-pushed the TRAEFIK-311--adding-constraint-filtering branch 4 times, most recently from b4570ba to 1f9a4dd Compare May 23, 2016 08:29

emilevauge mentioned this pull request May 24, 2016

Add label matching for kubernetes ingests #363

Closed

samber force-pushed the TRAEFIK-311--adding-constraint-filtering branch 3 times, most recently from 5995467 to 7d14972 Compare May 25, 2016 15:54

keis reviewed May 26, 2016
View reviewed changes

emilevauge added the need-review label May 26, 2016

emilevauge reviewed May 26, 2016
View reviewed changes

cmd.go

Docker: &provider.Docker{

TLS: &provider.DockerTLS{},

// @todo: Docker constraints

Copy link

Member

emilevauge May 26, 2016 •

edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sed 's/@todo/TODO/g'

samber force-pushed the TRAEFIK-311--adding-constraint-filtering branch 6 times, most recently from c371c23 to 2cfa2b1 Compare May 31, 2016 10:11

vdemeester reviewed May 31, 2016
View reviewed changes

vdemeester added status/2-needs-review and removed status/1-needs-design-review labels May 31, 2016

emilevauge reviewed May 31, 2016
View reviewed changes

samber added 5 commits June 1, 2016 10:30

feat(constraints): Implementation of constraint filtering (cmd + toml…

ac08792

… + matching functions), implementation proposal with consul

doc(constraints): Added in ConsulCatalog backend + new 'Constraint' s…

cd2100e

…ection

test(constraint): unit tests + integration tests + make validate

f46accc

refacto(constraints): Migration to Flaeg cli library

1de5434

fix(constraints): Syntax

d297a22

samber force-pushed the TRAEFIK-311--adding-constraint-filtering branch from 2cfa2b1 to d297a22 Compare June 1, 2016 08:31

vdemeester merged commit d654746 into traefik:master Jun 1, 2016

This was referenced Jun 6, 2016

feat(constraints): Supports constraints for docker backend #438

Merged

Setup Traefik for selective services #311

Closed

ldez added status/3-needs-merge and removed need-review status/2-needs-review labels Apr 29, 2017

ldez removed the status/3-needs-merge label Nov 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(constraints): Implementation of constraint filtering (cmd + toml + matching functions), implementation proposal with consul #342

feat(constraints): Implementation of constraint filtering (cmd + toml + matching functions), implementation proposal with consul #342

samber commented Apr 29, 2016 •

edited

emilevauge commented May 4, 2016

keis commented May 4, 2016

keis May 4, 2016

emilevauge commented May 7, 2016

samber commented May 9, 2016 •

edited

emilevauge commented May 12, 2016

samber commented May 12, 2016

vdemeester commented May 12, 2016 •

edited

emilevauge commented May 12, 2016

keis commented May 13, 2016

samber commented May 25, 2016

keis May 26, 2016

emilevauge May 26, 2016 •

edited

emilevauge commented May 26, 2016 •

edited

samber commented May 31, 2016

vdemeester May 31, 2016

samber May 31, 2016

emilevauge May 31, 2016

samber Jun 1, 2016

emilevauge commented May 31, 2016

samber commented Jun 1, 2016

vdemeester commented Jun 1, 2016

feat(constraints): Implementation of constraint filtering (cmd + toml + matching functions), implementation proposal with consul #342

feat(constraints): Implementation of constraint filtering (cmd + toml + matching functions), implementation proposal with consul #342

Conversation

samber commented Apr 29, 2016 • edited

emilevauge commented May 4, 2016

keis commented May 4, 2016

keis May 4, 2016

Choose a reason for hiding this comment

emilevauge commented May 7, 2016

samber commented May 9, 2016 • edited

emilevauge commented May 12, 2016

samber commented May 12, 2016

vdemeester commented May 12, 2016 • edited

emilevauge commented May 12, 2016

keis commented May 13, 2016

samber commented May 25, 2016

keis May 26, 2016

Choose a reason for hiding this comment

emilevauge May 26, 2016 • edited

Choose a reason for hiding this comment

emilevauge commented May 26, 2016 • edited

samber commented May 31, 2016

vdemeester May 31, 2016

Choose a reason for hiding this comment

samber May 31, 2016

Choose a reason for hiding this comment

emilevauge May 31, 2016

Choose a reason for hiding this comment

samber Jun 1, 2016

Choose a reason for hiding this comment

emilevauge commented May 31, 2016

samber commented Jun 1, 2016

vdemeester commented Jun 1, 2016

samber commented Apr 29, 2016 •

edited

samber commented May 9, 2016 •

edited

vdemeester commented May 12, 2016 •

edited

emilevauge May 26, 2016 •

edited

emilevauge commented May 26, 2016 •

edited