Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add HTTP headers to healthcheck. #3047

Merged
merged 10 commits into from
Apr 16, 2018
Merged

Conversation

zetaab
Copy link
Contributor

@zetaab zetaab commented Mar 19, 2018

What does this PR do?

Tries to fix issue #3043

Motivation

We need this feature, describe in issue

More

  • Added/updated tests
  • Added/updated documentation

Additional Notes

Fixes #3043

@zetaab
Copy link
Contributor Author

zetaab commented Mar 19, 2018

This is still WIP

I am adding healthchecks

        "healthcheck": {
            "path": "/",
            "interval": "5s",
            "headers": {
            	"Host": "foobar.com",
            	"header1": "test",
            	"headeR2": "test2"
            }
        },

I can see in logs DEBU[2018-03-19T14:32:53+02:00] Setting up backend health check [Headers: map[Host:foobar.com header1:test headeR2:test2] Path: / Port: 0 Interval: 5s]

However, still I am not able to see Host header in another server.

ERROR:root:Host: 192.168.1.43:8000
User-Agent: Go-http-client/1.1
Header1: test
Header2: test2

I am thinking might this cause this issue still golang/go#7682

@zetaab
Copy link
Contributor Author

zetaab commented Mar 19, 2018

package main

import (
	"fmt"
	"net/http"
	"os"
)

func main() {
	client := &http.Client{}
	req, err := http.NewRequest("GET", "http://localhost:8000", nil)
	if err != nil {
		os.Exit(1)
	}
	req.Header.Add("User-Agent", "myClient")
	req.Header.Add("Host", "leet.com")
	resp, err := client.Do(req)
	defer resp.Body.Close()
	fmt.Println(resp)
}

Does not change host header to leet.com. Cool :). So is this golang bug?

@zetaab
Copy link
Contributor Author

zetaab commented Mar 19, 2018

Got it working, its little bit tricky in Host header. Is these tests and docs enough or do we need more?

I have no idea how to fix these tests:

WARNING: Ignoring http://dl-cdn.alpinelinux.org/alpine/v3.7/community/x86_64/APKINDEX.tar.gz: BAD signature

@juliens
Copy link
Member

juliens commented Mar 27, 2018

Thanks for your contribution!

I understand your use case with the hostname, but I'm not sure about changing headers.

Do you think you can just add "hostname option" and remove other headers option?

@zetaab
Copy link
Contributor Author

zetaab commented Mar 28, 2018

@juliens IMO headers are useful for instance if the healthcheck needs authentication. So I do not see why we could not add those in same PR

@juliens
Copy link
Member

juliens commented Apr 10, 2018

@zetaab After discussion with the team, we agree to add both hostname and headers on healthcheck.

But we think we need to separate hostname from the headers.
If I take your proposal, it becomes like this:

        "healthcheck": {
            "path": "/",
            "interval": "5s",
            "hostname": "foobar.com",
            "headers": {
            	"header1": "test",
            	"headeR2": "test2"
            }
        },

Moreover, we need the functionnality to be added on every providers.

If you need help, you can share with us on our Slack workspace

Copy link
Contributor

@ldez ldez left a comment

Choose a reason for hiding this comment

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

@zetaab Could rebase your PR on the master branch.

Also, you have to take this feature available by the labels and the KV:

@zetaab
Copy link
Contributor Author

zetaab commented Apr 12, 2018

could someone review these test, I cannot now find the problem(s). To get tests working

edit: now tests are working locally, but ECS provider tests are failing to some another reasons

@ldez
Copy link
Contributor

ldez commented Apr 12, 2018

@zetaab Could you also update the doc for each provider, thanks.

@ldez ldez added this to the 1.7 milestone Apr 12, 2018
Copy link
Contributor

@ldez ldez left a comment

Choose a reason for hiding this comment

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

Thank you for your patience and your responsiveness. We really enjoyed that.

LGTM 🎉

Copy link
Member

@mmatur mmatur left a comment

Choose a reason for hiding this comment

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

Nice work 👏

Copy link
Member

@juliens juliens left a comment

Choose a reason for hiding this comment

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

LGTM

@traefiker traefiker merged commit 1954a49 into traefik:master Apr 16, 2018
@ldez ldez changed the title add http headers to healthcheck Add HTTP headers to healthcheck. Apr 18, 2018
@ldez ldez mentioned this pull request Apr 18, 2018
1 task
@ldez ldez mentioned this pull request May 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support to additional httpheaders in healthchecks
5 participants