Add Custom header parsing to Docker Provider #2030

dtomcej · 2017-08-31T03:12:05Z

This PR provide a basis for updating the Kubernetes provider.

Fixes #2028

dtomcej · 2017-08-31T03:13:45Z

This PR will resolve #2028 and will provide a basis for updating the kubernetes provider

dtomcej · 2017-09-27T16:47:35Z

Still working on this.

dtomcej · 2017-10-01T01:45:59Z

Ok. Submitting this for review.

This PR allows the custom request and response headers to be set via docker labels. The labels get parsed, and invoke the headers middleware.

dtomcej · 2017-10-01T01:46:31Z

Ping @containous/traefik

dtomcej · 2017-10-01T01:47:22Z

If review is passed, will squash and rebase.

timoreimann

We should also extend the documentation.

timoreimann · 2017-10-01T06:14:32Z

provider/docker/docker.go

+	if requestHeaders, err := getLabel(container, types.LabelFrontendRequestHeader); err == nil {
+		for _, v := range strings.Split(requestHeaders, ",") {
+			k := strings.Split(v, ":")
+			h[k[0]] = k[1]


We will crash if the split in the previous line yields a single line only. Should we have some extra sanity check?

timoreimann · 2017-10-01T06:14:46Z

provider/docker/docker.go

+	if responseHeaders, err := getLabel(container, types.LabelFrontendResponseHeader); err == nil {
+		for _, v := range strings.Split(responseHeaders, ",") {
+			k := strings.Split(v, ":")
+			h[k[0]] = k[1]


timoreimann · 2017-10-01T06:16:18Z

provider/docker/docker.go

+			k := strings.Split(v, ":")
+			h[k[0]] = k[1]
+		}
+	}


The parse logic for the requests and responses seems identical. Might be worth extracting into a little helper function.

Seems legit.

timoreimann · 2017-10-01T06:20:26Z

templates/docker.tmpl

@@ -74,6 +74,18 @@
  basicAuth = [{{range getBasicAuth $container}}
    "{{.}}",
  {{end}}]
+  {{if getRequestHeaders $container}}


Using the parse method for checking if request headers exist means we'll emit the log outputs twice: once in the check and once in the actual extraction (line 79). This might be somewhat confusing.

It might be better do introduce a little hasRequestHeaders template function, similar to how we do it with other functions.

timoreimann · 2017-10-01T06:20:34Z

templates/docker.tmpl

+    {{$k}} = "{{$v}}"
+    {{end}}
+  {{end}}
+  {{if getResponseHeaders $container}}


dtomcej · 2017-10-13T02:14:26Z

@timoreimann Squashed, Rebased, Helper-ified, and Documented :)

dtomcej · 2017-10-13T15:04:00Z

ping @containous/traefik

timoreimann

Left two optional improvement suggestions. Approving anyway, I'm fine with the status quo too. 👍

timoreimann · 2017-10-13T15:13:24Z

provider/docker/docker.go

+	if _, err := getLabel(container, types.LabelFrontendRequestHeader); err != nil {
+		return false
+	}
+	return true


Could also be

_, err := getLabel(container, types.LabelFrontendRequestHeader) return err == nil

Similar with hasResponseHeaders.

ldez

Great job! A few comments.

ldez · 2017-10-19T20:45:30Z

provider/docker/docker.go

+
+	log.Debugf("Loaded %s: %v", containerType, h)
+	return h
+


could you remove this empty line?

ldez · 2017-10-19T20:46:32Z

provider/docker/docker.go

+
+func parseCustomHeaders(container dockerData, containerType string) map[string]string {
+	h := make(map[string]string)
+	if r, err := getLabel(container, containerType); err == nil {


maybe you could use explicit variable name instead of one letter.

By example:

h -> headers

r -> label

v -> pairs

k -> pair

ldez · 2017-10-19T20:50:08Z

provider/docker/docker.go

@@ -721,6 +725,45 @@ func (p *Provider) getBasicAuth(container dockerData) []string {
 	return []string{}
 }

+func (p *Provider) hasRequestHeaders(container dockerData) bool {
+	if _, err := getLabel(container, types.LabelFrontendRequestHeader); err != nil {


Maybe you could evaluate the length?

label, err := getLabel(container, types.LabelFrontendRequestHeader) return err == nil && len(label) > 0

same for hasResponseHeaders

ldez · 2017-10-19T20:51:50Z

provider/docker/docker.go

+			k := strings.Split(v, ":")
+			if len(k) != 2 {
+				log.Warnf("Could not load header %v", k)
+				return h


If you add a return you could add log.Errorf instead of log.Warnf

In the case of an invalid key/value pair, you return the current map with previous values (or an empty map if it's the first pair), this seem strange:

either you return a empty map (-> log.Errorf)

or you only skip the invalid key/value pair (-> log.Warnf)

update label parsing fix template ordering update to include response headers in templates abstract to helpers remove labels re-add labels move label for alphabet add doc on site better variable naming and error logic

dtomcej · 2017-10-20T01:12:21Z

@ldez corrections implemented!

ldez

LGTM 👍

juliens

LGTM

vito-c · 2017-11-30T23:40:42Z

provider/docker/docker.go

+func parseCustomHeaders(container dockerData, containerType string) map[string]string {
+	customHeaders := make(map[string]string)
+	if label, err := getLabel(container, containerType); err == nil {
+		for _, headers := range strings.Split(label, ",") {


Access-Control-Allow-Methods:POST,GET,OPTIONS wouldn't this header cause this split to fail? maybe we should use a ; similar to how the rules are separated?

time="2017-12-01T00:03:00Z" level=warning msg="Could not load header "traefik.frontend.headers.customrequestheaders": [POST], skipping..." time="2017-12-01T00:03:00Z" level=warning msg="Could not load header "traefik.frontend.headers.customrequestheaders": [OPTIONS], skipping..." time="2017-12-01T00:03:00Z" level=warning msg="Could not load header "traefik.frontend.headers.customrequestheaders": [DELETE], skipping..." time="2017-12-01T00:03:00Z" level=warning msg="Could not load header "traefik.frontend.headers.customrequestheaders": [PUT], skipping..."

@vito-c please open an issue instead of comment on an already merged PR.

dtomcej requested a review from a team as a code owner August 31, 2017 03:12

traefiker added area/provider/docker size/S status/0-needs-triage labels Aug 31, 2017

dtomcej added size/M WIP and removed size/S labels Aug 31, 2017

ldez changed the title ~~Add Custom header parsing to Docker Provider [Fixes #2028]~~ Add Custom header parsing to Docker Provider Aug 31, 2017

ldez removed the status/0-needs-triage label Aug 31, 2017

dtomcej removed the WIP label Oct 1, 2017

dtomcej added the status/2-needs-review label Oct 1, 2017

ldez added this to the 1.5 milestone Oct 1, 2017

timoreimann suggested changes Oct 1, 2017

View reviewed changes

traefiker added the contributor/waiting-for-corrections label Oct 1, 2017

ldez added the contributor/needs-resolve-conflicts label Oct 11, 2017

dtomcej force-pushed the tomcej-2028 branch 5 times, most recently from bb51dba to 255d9a1 Compare October 13, 2017 01:59

dtomcej removed the contributor/needs-resolve-conflicts label Oct 13, 2017

ldez removed the contributor/waiting-for-corrections label Oct 13, 2017

timoreimann approved these changes Oct 13, 2017

View reviewed changes

dtomcej mentioned this pull request Oct 15, 2017

SSLRedirect = true / Traefik Label for Docker Swarm #1903

Closed

ldez suggested changes Oct 19, 2017

View reviewed changes

traefiker added the contributor/waiting-for-corrections label Oct 19, 2017

dtomcej force-pushed the tomcej-2028 branch from 616c375 to f5a05b4 Compare October 20, 2017 00:28

ldez removed the contributor/waiting-for-corrections label Oct 20, 2017

initial commit

33a926c

update label parsing fix template ordering update to include response headers in templates abstract to helpers remove labels re-add labels move label for alphabet add doc on site better variable naming and error logic

dtomcej force-pushed the tomcej-2028 branch from f5a05b4 to 33a926c Compare October 20, 2017 00:56

ldez approved these changes Oct 20, 2017

View reviewed changes

juliens approved these changes Oct 20, 2017

View reviewed changes

juliens added status/3-needs-merge and removed status/2-needs-review status/3-needs-merge labels Oct 20, 2017

traefiker approved these changes Oct 20, 2017

View reviewed changes

juliens added the status/3-needs-merge label Oct 20, 2017

traefiker merged commit d973096 into traefik:master Oct 20, 2017

traefiker removed the status/3-needs-merge label Oct 20, 2017

ldez added the kind/enhancement a new or improved feature. label Oct 25, 2017

vito-c reviewed Nov 30, 2017

View reviewed changes

talolard mentioned this pull request Feb 9, 2018

Custom headers are not applied with docker #2832

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Custom header parsing to Docker Provider #2030

Add Custom header parsing to Docker Provider #2030

dtomcej commented Aug 31, 2017 •

edited by ldez

Loading

dtomcej commented Aug 31, 2017

dtomcej commented Sep 27, 2017

dtomcej commented Oct 1, 2017

dtomcej commented Oct 1, 2017

dtomcej commented Oct 1, 2017

timoreimann left a comment

timoreimann Oct 1, 2017

dtomcej Oct 1, 2017

timoreimann Oct 1, 2017

timoreimann Oct 1, 2017

dtomcej Oct 1, 2017

timoreimann Oct 1, 2017

dtomcej Oct 1, 2017

timoreimann Oct 1, 2017

dtomcej commented Oct 13, 2017

dtomcej commented Oct 13, 2017

timoreimann left a comment

timoreimann Oct 13, 2017

ldez left a comment

ldez Oct 19, 2017 •

edited

Loading

ldez Oct 19, 2017

ldez Oct 19, 2017 •

edited

Loading

dtomcej Oct 20, 2017

ldez Oct 19, 2017

dtomcej commented Oct 20, 2017

ldez left a comment

juliens left a comment

vito-c Nov 30, 2017

vito-c Dec 1, 2017

ldez Dec 1, 2017

Add Custom header parsing to Docker Provider #2030

Add Custom header parsing to Docker Provider #2030

Conversation

dtomcej commented Aug 31, 2017 • edited by ldez Loading

dtomcej commented Aug 31, 2017

dtomcej commented Sep 27, 2017

dtomcej commented Oct 1, 2017

dtomcej commented Oct 1, 2017

dtomcej commented Oct 1, 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

dtomcej commented Oct 13, 2017

dtomcej commented Oct 13, 2017

timoreimann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldez left a comment

Choose a reason for hiding this comment

ldez Oct 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

dtomcej commented Oct 20, 2017

ldez left a comment

Choose a reason for hiding this comment

juliens 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

dtomcej commented Aug 31, 2017 •

edited by ldez

Loading

ldez Oct 19, 2017 •

edited

Loading

ldez Oct 19, 2017 •

edited

Loading