From a1466cb3fc1322a06f4be6d5c80d34cbc44db15d Mon Sep 17 00:00:00 2001 From: Paul Bellamy Date: Tue, 20 Oct 2015 14:26:27 +0100 Subject: [PATCH] Review Feedback --- .gitignore | 1 + Makefile | 9 ++++++--- common/sanitize/sanitize.go | 22 +++++++++++----------- common/sanitize/sanitize_test.go | 9 ++++++--- docker/Dockerfile | 1 + experimental/demoprobe/main.go | 2 +- experimental/fixprobe/main.go | 2 +- xfer/http_publisher.go | 13 ++----------- xfer/http_publisher_test.go | 2 +- 9 files changed, 30 insertions(+), 31 deletions(-) diff --git a/.gitignore b/.gitignore index c1ca51366..344ce9365 100644 --- a/.gitignore +++ b/.gitignore @@ -43,6 +43,7 @@ docker/scope-probe docker/docker* docker/weave docker/runsvinit +docker/ca-certificates.crt experimental/bridge/bridge experimental/demoprobe/demoprobe experimental/fixprobe/fixprobe diff --git a/Makefile b/Makefile index e196b4cd9..4d6e2eef9 100644 --- a/Makefile +++ b/Makefile @@ -29,19 +29,22 @@ docker/weave: curl -L git.io/weave -o docker/weave chmod u+x docker/weave -$(SCOPE_EXPORT): $(APP_EXE) $(PROBE_EXE) $(DOCKER_DISTRIB) docker/weave $(RUNSVINIT) docker/Dockerfile docker/run-app docker/run-probe docker/entrypoint.sh +$(SCOPE_EXPORT): $(APP_EXE) $(PROBE_EXE) $(DOCKER_DISTRIB) docker/weave $(RUNSVINIT) docker/Dockerfile docker/run-app docker/run-probe docker/entrypoint.sh docker/ca-certificates.crt @if [ -z '$(DOCKER_SQUASH)' ] ; then echo "Please install docker-squash by running 'make deps' (and make sure GOPATH/bin is in your PATH)." && exit 1 ; fi cp $(APP_EXE) $(PROBE_EXE) docker/ cp $(DOCKER_DISTRIB) docker/docker.tgz $(SUDO) docker build -t $(SCOPE_IMAGE) docker/ $(SUDO) docker save $(SCOPE_IMAGE):latest | sudo $(DOCKER_SQUASH) -t $(SCOPE_IMAGE) | tee $@ | $(SUDO) docker load +docker/ca-certificates.crt: /etc/ssl/certs/ca-certificates.crt + cp $? $@ + $(RUNSVINIT): vendor/runsvinit/*.go go build -o $@ github.com/weaveworks/scope/vendor/runsvinit -$(APP_EXE): app/*.go render/*.go report/*.go xfer/*.go +$(APP_EXE): app/*.go render/*.go report/*.go xfer/*.go common/sanitize/*.go -$(PROBE_EXE): probe/*.go probe/docker/*.go probe/kubernetes/*.go probe/endpoint/*.go probe/host/*.go probe/process/*.go probe/overlay/*.go report/*.go xfer/*.go +$(PROBE_EXE): probe/*.go probe/docker/*.go probe/kubernetes/*.go probe/endpoint/*.go probe/host/*.go probe/process/*.go probe/overlay/*.go report/*.go xfer/*.go common/sanitize/*.go common/exec/*.go $(APP_EXE) $(PROBE_EXE): go get -d -tags netgo ./$(@D) diff --git a/common/sanitize/sanitize.go b/common/sanitize/sanitize.go index bd12bf8c9..fa35b897e 100644 --- a/common/sanitize/sanitize.go +++ b/common/sanitize/sanitize.go @@ -10,29 +10,29 @@ import ( // URL returns a function that sanitizes a URL string. It lets underspecified // strings to be converted to usable URLs via some default arguments. -func URL(scheme string, port int, path string) func(string) string { - if scheme == "" { - scheme = "http://" +func URL(defaultScheme string, defaultPort int, defaultPath string) func(string) string { + if defaultScheme == "" { + defaultScheme = "http://" } return func(s string) string { if s == "" { return s // can't do much here } - if !strings.HasPrefix(s, "http") { - s = scheme + s + if !strings.Contains(s, "://") { + s = defaultScheme + s } u, err := url.Parse(s) if err != nil { log.Printf("%q: %v", s, err) return s // oh well } - if port > 0 { - if _, _, err = net.SplitHostPort(u.Host); err != nil { - u.Host += fmt.Sprintf(":%d", port) - } + if _, port, err := net.SplitHostPort(u.Host); err != nil && defaultPort > 0 { + u.Host += fmt.Sprintf(":%d", defaultPort) + } else if port == "443" { + u.Scheme = "https" } - if path != "" && u.Path != path { - u.Path = path + if defaultPath != "" && u.Path != defaultPath { + u.Path = defaultPath } return u.String() } diff --git a/common/sanitize/sanitize_test.go b/common/sanitize/sanitize_test.go index 1d6502488..b329d52ee 100644 --- a/common/sanitize/sanitize_test.go +++ b/common/sanitize/sanitize_test.go @@ -22,9 +22,12 @@ func TestSanitizeURL(t *testing.T) { {"https://", 0, "", "foo", "https://foo"}, {"https://", 80, "", "foo", "https://foo:80"}, {"https://", 0, "some/path", "foo", "https://foo/some/path"}, - {"https://", 0, "", "http://foo", "http://foo"}, // specified scheme beats default... - {"", 9999, "", "foo:80", "http://foo:80"}, // specified port beats default... - {"", 0, "/bar", "foo/baz", "http://foo/bar"}, // ...but default path beats specified! + {"https://", 0, "", "http://foo", "http://foo"}, // specified scheme beats default... + {"", 0, "", "https://foo", "https://foo"}, // https can be a specified scheme without default... + {"http://", 0, "", "https://foo", "https://foo"}, // https can be a specified scheme with default... + {"", 9999, "", "foo:80", "http://foo:80"}, // specified port beats default... + {"", 0, "/bar", "foo/baz", "http://foo/bar"}, // ...but default path beats specified! + {"", 0, "", "foo:443", "https://foo:443"}, // port 443 addrs default to https scheme } { if want, have := input.want, sanitize.URL(input.scheme, input.port, input.path)(input.input); want != have { t.Errorf("sanitize.URL(%q, %d, %q)(%q): want %q, have %q", input.scheme, input.port, input.path, input.input, want, have) diff --git a/docker/Dockerfile b/docker/Dockerfile index c794b56ce..aa94c7732 100644 --- a/docker/Dockerfile +++ b/docker/Dockerfile @@ -10,5 +10,6 @@ ADD ./weave /usr/bin/ COPY ./scope-app ./scope-probe ./runsvinit ./entrypoint.sh /home/weave/ COPY ./run-app /etc/service/app/run COPY ./run-probe /etc/service/probe/run +COPY ./ca-certificates.crt /etc/ssl/certs/ EXPOSE 4040 ENTRYPOINT ["/home/weave/entrypoint.sh"] diff --git a/experimental/demoprobe/main.go b/experimental/demoprobe/main.go index 51c3ac5bf..c53b4686e 100644 --- a/experimental/demoprobe/main.go +++ b/experimental/demoprobe/main.go @@ -23,7 +23,7 @@ func main() { ) flag.Parse() - _, publisher, err := xfer.NewHTTPPublisher(*publish, "demoprobe", "demoprobe") + _, publisher, err := xfer.NewHTTPPublisher(*publish, "demoprobe", "demoprobe", false) if err != nil { log.Fatal(err) } diff --git a/experimental/fixprobe/main.go b/experimental/fixprobe/main.go index bcec4c05a..105572996 100644 --- a/experimental/fixprobe/main.go +++ b/experimental/fixprobe/main.go @@ -34,7 +34,7 @@ func main() { } f.Close() - _, publisher, err := xfer.NewHTTPPublisher(*publish, "fixprobe", "fixprobe") + _, publisher, err := xfer.NewHTTPPublisher(*publish, "fixprobe", "fixprobe", false) if err != nil { log.Fatal(err) } diff --git a/xfer/http_publisher.go b/xfer/http_publisher.go index 32c787290..bbd6aea54 100644 --- a/xfer/http_publisher.go +++ b/xfer/http_publisher.go @@ -5,7 +5,6 @@ import ( "encoding/json" "fmt" "io" - "net" "net/http" "time" @@ -26,16 +25,8 @@ var fastClient = &http.Client{ // NewHTTPPublisher returns an HTTPPublisher ready for use. func NewHTTPPublisher(target, token, probeID string, insecure bool) (string, *HTTPPublisher, error) { - _, port, err := net.SplitHostPort(target) - if err != nil { - return "", nil, err - } - scheme := "http" - if port == "443" { - scheme = "https" - } p := &HTTPPublisher{ - url: sanitize.URL(scheme+"://", 0, "/api/report")(target), + url: sanitize.URL("", 0, "/api/report")(target), token: token, probeID: probeID, client: http.DefaultClient, @@ -45,7 +36,7 @@ func NewHTTPPublisher(target, token, probeID string, insecure bool) (string, *HT allowInsecure(fastClient) allowInsecure(p.client) } - req, err := p.authorizedRequest("GET", sanitize.URL(scheme+"://", 0, "/api")(target), nil) + req, err := p.authorizedRequest("GET", sanitize.URL("", 0, "/api")(target), nil) if err != nil { return "", nil, err } diff --git a/xfer/http_publisher_test.go b/xfer/http_publisher_test.go index f80b75f49..007642521 100644 --- a/xfer/http_publisher_test.go +++ b/xfer/http_publisher_test.go @@ -67,7 +67,7 @@ func TestHTTPPublisher(t *testing.T) { s := httptest.NewServer(handlers.CompressHandler(handler)) defer s.Close() - _, p, err := xfer.NewHTTPPublisher(s.URL, token, id) + _, p, err := xfer.NewHTTPPublisher(s.URL, token, id, false) if err != nil { t.Fatal(err) }