Enhanced flexibility in Consul Catalog configuration #1565

aantono · 2017-05-08T17:51:49Z

Description

Enhanced Consul Catalog configuration by adding the following:

Ability to configure default FrontEndRule using Go Template
Ability to use all tags for constraint filtering
Ability to have empty/no prefix
Added getTag and hasTag functions to be used in advanced custom configuration template

ldez

Thanks for the contribution.

could you also edit traefik.sample.toml?

ldez · 2017-05-10T07:31:54Z

provider/consul/consul_catalog.go

@@ -202,22 +231,46 @@ func (p *CatalogProvider) getBackendName(node *api.ServiceEntry, index int) stri
 }

 func (p *CatalogProvider) getAttribute(name string, tags []string, defaultValue string) string {
+	//for _, tag := range tags {


could your remove the comment block?

ldez · 2017-05-10T07:32:02Z

provider/consul/consul_catalog.go

+	var buffer bytes.Buffer
+	t.Execute(&buffer, templateObjects)
+
+	//return "Host:" + service.ServiceName + "." + p.Domain


could your remove the comment?

ldez · 2017-05-10T07:32:33Z

provider/consul/consul_catalog.go

+	FrontEndRule               string `description:"Frontend rule used for Consul services"`
+	AllTagsConstraintFiltering bool   `description:"Should use all tags for constraint filtering"`
+	client                     *api.Client
+	FrontEndRuleTemplate       *template.Template


move up this line and add the description.

@ldez The FrontEndRuleTemplate is the actual Template object that is not provided via configuration, but initialized from FrontEndRule value. So I see it being similar to api.Client kind. Guess it should probably be made lower-cased name frontEndRuleTemplate instead to clearly indicate that, correct?

yes it's the way 😃

Awesome... I've made the change in the last update to this PR along with adding content to the traefik.sample.toml file, and also rebased it on top of the latest changes in master. Let me know if anything else needs changing or correcting.

emilevauge

Thanks @aantono for your contribution :)
Few comments but overall looks great!

emilevauge · 2017-05-19T10:08:18Z

provider/consul/consul_catalog.go

+}
+
+func (p *CatalogProvider) hasTag(name string, tags []string) bool {
+	tag := p.getTag(name, tags, "=!=") // Very-very unlikely that a Consul tag would ever start with '=!='


@aantono Could you explain why you need =!= here?

This is my internal dummy "default" value (because getTag function wants a default) which is unlikely to be seen in the real world (who would tag their service with =!=... :) )
The logic is this: if getTag(...) returns =!=, then there is no name tag in existence, otherwise we would get a different value.

emilevauge · 2017-05-19T10:10:10Z

provider/consul/consul_catalog.go

+	Domain                     string `description:"Default domain used"`
+	Prefix                     string `description:"Prefix used for Consul catalog tags"`
+	FrontEndRule               string `description:"Frontend rule used for Consul services"`
+	AllTagsConstraintFiltering bool   `description:"Should use all tags for constraint filtering"`


Not fan of the naming of AllTagsConstraintFiltering. Could you explain why you added this option in a use case?

The use-case here is to enable the ability to filter (use tag constraints) on the existing Consul tags, not only the ones that are defined in traefik.tags Many places with existing Consul infrastructure already have quite an elaborate tagging setup for their service registrations, so being able to leverage those for filtering is a big plus. For example, each Spring Boot application that uses sping-cloud-consul module, will automatically register the management endpoint using management tag. Because it might not be desired to expose LB routes to management endpoints, one might want to create a constraint rule like constraints = ["tag!=management"], and when coupled with AllTagsConstraintFiltering=true, it will use the "raw" management tag value, instead of only the values provided by traefik.tags tag. Does this make things clearer?

P.S. I'm not a big fan of the name either, just couldn't really come up with anything reasonably short and yet more descriptive, so any suggestions are welcomed! :)

emilevauge · 2017-05-19T10:11:34Z

provider/consul/consul_catalog.go

-			if kv := strings.SplitN(tag[len(p.Prefix+"."):], "=", 2); len(kv) == 2 && strings.ToLower(kv[0]) == strings.ToLower(name) {
-				return kv[1]
+		if strings.Index(strings.ToLower(tag), strings.ToLower(name)) == 0 {
+			if kv := strings.SplitN(tag, "=", 2); strings.ToLower(kv[0]) == strings.ToLower(name) {


Could you add some comments ? This block is a bit hard to read ;)

emilevauge · 2017-05-19T10:12:15Z

provider/consul/consul_catalog.go

-		if strings.Index(strings.ToLower(tag), p.Prefix+".tags=") == 0 {
-			splitedTags := strings.Split(tag[len(p.Prefix+".tags="):], ",")
+		if strings.Index(strings.ToLower(tag), p.getPrefixedName("tags=")) == 0 {
+			splitedTags := strings.Split(tag[len(p.getPrefixedName("tags=")):], ",")


Could you add some comments ? This block is a bit hard to read ;)

emilevauge · 2017-05-19T10:15:00Z

docs/toml.md

+#
+# Optional
+#
+frontEndRule = "{{.ServiceName}}.{{Domain}}"


Shouldn't it be Host:{{.ServiceName}}.{{Domain}} instead?

Yes, indeed it should. Great catch!

emilevauge · 2017-05-19T10:16:16Z

traefik.sample.toml

+#
+# Optional
+#
+#frontEndRule = "{{.ServiceName}}.{{Domain}}"


Same as in traefik.sample.toml

emilevauge · 2017-05-22T16:09:20Z

docs/toml.md

+# Default frontEnd Rule for Consul services
+#  - The format is a Go Template with ".ServiceName", ".Domain" and ".Attributes" available
+#  -- "getTag(name, tags, defaultValue)", "hasTag(name, tags)" and "getAttribute(name, tags, defaultValue)" functions are available
+#  --- "getAttribute(...)" function uses prefixed tag names based on "prefix" value


Could explain why you added in the comments ?

- -- ---

No reason, just to help with indentation... Removed.

emilevauge · 2017-05-23T20:34:09Z

script/crossbinary-default

+#    echo "Building binary for $OS/$ARCH..."
+#    GOARCH=$ARCH GOOS=$OS CGO_ENABLED=0 $GO_BUILD_CMD "$GO_BUILD_OPT" -o "dist/traefik_$OS-$ARCH" ./cmd/traefik/
+#  done
+#done


I don't think that's what we want ^^

Oops, my bad... accidentally committed something that should not have been... will revert ASAP

emilevauge

Thanks @aantono
LGTM

ldez

Thanks!

LGTM

aantono force-pushed the master branch 3 times, most recently from 1ec044d to badee03 Compare May 8, 2017 21:43

ldez added kind/enhancement a new or improved feature. area/provider/consul labels May 9, 2017

timoreimann added the status/1-needs-design-review label May 9, 2017

ldez suggested changes May 10, 2017

View reviewed changes

aantono force-pushed the master branch 3 times, most recently from 43a6b6e to d271e2a Compare May 10, 2017 20:43

emilevauge requested changes May 19, 2017

View reviewed changes

emilevauge added status/2-needs-review and removed status/1-needs-design-review labels May 19, 2017

aantono force-pushed the master branch from d271e2a to f67a0de Compare May 19, 2017 18:27

emilevauge reviewed May 22, 2017

View reviewed changes

aantono force-pushed the master branch 5 times, most recently from e9da784 to b0b4085 Compare May 23, 2017 18:10

emilevauge reviewed May 23, 2017

View reviewed changes

aantono force-pushed the master branch 2 times, most recently from 8ec2216 to 8f47846 Compare May 23, 2017 20:36

aantono force-pushed the master branch from 8f47846 to 510797a Compare May 31, 2017 21:38

emilevauge approved these changes Jun 7, 2017

View reviewed changes

aantono force-pushed the master branch 2 times, most recently from 8fe4b9a to d39e7d2 Compare June 12, 2017 18:37

Enhanced flexibility in Consul Catalog configuration

d866cb2

ldez force-pushed the master branch from d39e7d2 to d866cb2 Compare June 12, 2017 19:01

ldez added this to the 1.4 milestone Jun 12, 2017

ldez approved these changes Jun 12, 2017

View reviewed changes

ldez removed the status/2-needs-review label Jun 12, 2017

ldez merged commit 7d6c778 into traefik:master Jun 12, 2017

ldez added area/provider/consulcatalog and removed area/provider/consul labels Oct 1, 2017

aantono mentioned this pull request Jan 11, 2018

Allow to route to invididual containers (e.g. variable substitution in frontend.rule) #2578

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhanced flexibility in Consul Catalog configuration #1565

Enhanced flexibility in Consul Catalog configuration #1565

aantono commented May 8, 2017

ldez left a comment

ldez May 10, 2017

ldez May 10, 2017

ldez May 10, 2017

aantono May 10, 2017

ldez May 10, 2017

aantono May 10, 2017 •

edited

Loading

emilevauge left a comment

emilevauge May 19, 2017

aantono May 19, 2017 •

edited

Loading

emilevauge May 19, 2017

aantono May 19, 2017 •

edited

Loading

emilevauge May 19, 2017

emilevauge May 19, 2017

emilevauge May 19, 2017

aantono May 19, 2017

emilevauge May 19, 2017

emilevauge May 22, 2017

aantono May 22, 2017

emilevauge May 23, 2017

aantono May 23, 2017

emilevauge left a comment

ldez left a comment

Enhanced flexibility in Consul Catalog configuration #1565

Enhanced flexibility in Consul Catalog configuration #1565

Conversation

aantono commented May 8, 2017

Description

ldez 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

aantono May 10, 2017 • edited Loading

Choose a reason for hiding this comment

emilevauge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aantono May 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aantono May 19, 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

Choose a reason for hiding this comment

emilevauge left a comment

Choose a reason for hiding this comment

ldez left a comment

Choose a reason for hiding this comment

aantono May 10, 2017 •

edited

Loading

aantono May 19, 2017 •

edited

Loading

aantono May 19, 2017 •

edited

Loading