Merge ~jacekn/prometheus-registration/+git/prometheus-registration:master into prometheus-registration:master

[Jacek Nykis]

Add sanity checks when adding new targets. Fixes LP#1722774

Add sanity checks when adding new targets. Fixes LP1722774

[Joel Sing (jsing)]

review: Needs Fixing

[Jacek Nykis (jacekn)]

Addressed comments from Joel

review: Needs Resubmitting

[Joel Sing (jsing)]

LGTM, see comment.

review: Approve (+1)

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

Unable to determine commit message from repository - please click "Set commit message" and enter the commit message manually.

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

Change has no commit message, setting status to needs review.

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

Change successfully merged at revision 47e5f85b3fb83cc90c553c46d0412c77f9b50993

diff --git a/README.md b/README.md
index b994490..2f1a3f9 100644
--- a/README.md
+++ b/README.md
@@ -121,7 +121,9 @@ With the exception of the last dependency the rest straight forward and can be g
 The last one is special in that it is not a library and itself contains a vendor directory which causes the build to choke.
 This dependency is needed as it allows writing the targets.yaml with code from Prometheus to ensure it matches the same file format.
 To work around this problem I am using [govendor](https://github.com/kardianos/govendor), to pull in all the dependencies.
-After installing govendor just run `govendor sync` to retrieve dependencies and then build as usual.
+After installing govendor just run `govendor sync` to retrieve dependencies and then build:
+
+    go install launchpad.net/prometheus-registration/cmd/promreg/
 
 ## Misc
 While the standard metrics path for nearly any target is `/metrics`, this can be changed by specifying a `__metrics_path__` label when the target is registered. This should work, but can depend on your Prometheus configuration and is untested functionality.
@@ -131,3 +133,4 @@ While the standard metrics path for nearly any target is `/metrics`, this can be
 - Implement a mix of HTTP and HTTPS targets. Promreg can manage both but would write out two files and
   there would be different scrape_config jobs configured in Prometheus, one with "scheme: https".
 - Implement filtering of list output.
+- Improve testing so that it does not rely on specific result ordering
diff --git a/debian/changelog b/debian/changelog
index f0bdad8..43ffcba 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,9 @@
+promreg (0.0.9) xenial; urgency=medium
+
+  * Improve data validation when adding new targets. Fixes LP1722774
+
+ -- Jacek Nykis <[e-mail]>  Wed, 08 Nov 2017 17:28:01 +0000
+
 promreg (0.0.8) xenial; urgency=medium
 
   * README improvements and lots of other code cleanup following review.
diff --git a/promreg/http.go b/promreg/http.go
index f061d8c..68039e7 100644
--- a/promreg/http.go
+++ b/promreg/http.go
@@ -15,7 +15,9 @@ import (
 	"fmt"
 	"io"
 	"log"
+	"net"
 	"net/http"
+	"strconv"
 	"strings"
 
 	"github.com/prometheus/common/model"
@@ -101,6 +103,16 @@ func (th TargetsHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 			http.Error(w, "Target is undefined", http.StatusBadRequest)
 			return
 		}
+		// Ensure we add host:port target
+		_, port, err := net.SplitHostPort(t.Host)
+		if err != nil {
+			http.Error(w, fmt.Sprintf("Expected host:port, got: %v. E: %v" , t.Host, err), http.StatusBadRequest)
+			return
+		}
+		if p, err := strconv.ParseUint(port, 10, 16); err != nil || p < 1 || p > 65535 {
+			http.Error(w, fmt.Sprintf("Invalid port: %v", port), http.StatusBadRequest)
+			return
+		}
 		if err := t.ParseBody(r.Body); err != nil {
 			http.Error(w, fmt.Sprintf("Unable to parse request body %v", err), http.StatusBadRequest)
 			return
diff --git a/promreg/promreg_test.go b/promreg/promreg_test.go
index d54a5b1..b95bbea 100644
--- a/promreg/promreg_test.go
+++ b/promreg/promreg_test.go
@@ -64,10 +64,11 @@ func TestAPI(t *testing.T) {
 
 	//Add a new target
 	rdr := strings.NewReader(`{"comment": "New web server", "labels": {"labelname1": "labelvalue1"}}`)
-	req = authedRequest("POST", testURL+"/targets/myhost", rdr)
+	req = authedRequest("POST", testURL+"/targets/myhost:8080", rdr)
 	verifyResponse(t, req, http.StatusAccepted, "")
-	myhost := `{"Host":"myhost","labels":{"labelname1":"labelvalue1"}}`
-	verifyTargets(t, testURL, "["+selfTarget+","+myhost+"]")
+	myhost := `{"Host":"myhost:8080","labels":{"labelname1":"labelvalue1"}}`
+	targets := []string{selfTarget, myhost}
+	verifyTargets(t, testURL, fmt.Sprintf("[%v]", strings.Join(targets, ",")))
 
 	// Verify git commit
 	cmd := exec.Command("git", "-C", tempDir, "log", "-1", "--pretty=format:%s")
@@ -81,9 +82,24 @@ func TestAPI(t *testing.T) {
 
 	//Add again should error
 	rdr = strings.NewReader(`{"comment": "New web server", "labels": {"labelname1": "labelvalue1"}}`)
-	req = authedRequest("POST", testURL+"/targets/myhost", rdr)
+	req = authedRequest("POST", testURL+"/targets/myhost:8080", rdr)
 	verifyResponse(t, req, http.StatusInternalServerError, "Storage Error attempting to add an already existing Host, update or delete first\n")
 
+	//Add target without mandatory port
+	rdr = strings.NewReader(`{"comment": "New web server", "labels": {"labelname1": "labelvalue1"}}`)
+	req = authedRequest("POST", testURL+"/targets/host_no_port", rdr)
+	verifyResponse(t, req, http.StatusBadRequest, "Expected host:port, got: host_no_port. E: missing port in address host_no_port\n")
+
+	//Add target with out of range port
+	rdr = strings.NewReader(`{"comment": "New web server", "labels": {"labelname1": "labelvalue1"}}`)
+	req = authedRequest("POST", testURL+"/targets/host:99999", rdr)
+	verifyResponse(t, req, http.StatusBadRequest, "Invalid port: 99999\n")
+
+	//Add target with malformed port
+	rdr = strings.NewReader(`{"comment": "New web server", "labels": {"labelname1": "labelvalue1"}}`)
+	req = authedRequest("POST", testURL+"/targets/host:9103/metrics", rdr)
+	verifyResponse(t, req, http.StatusBadRequest, "Invalid port: 9103/metrics\n")
+
 	//Add unnamed target
 	rdr = strings.NewReader(`{"comment": "New blank server", "labels": {"labelname1": "labelvalue1"}}`)
 	req = authedRequest("POST", testURL+"/targets/", rdr)
@@ -91,32 +107,35 @@ func TestAPI(t *testing.T) {
 
 	//Add target with invalid JSON body
 	rdr = strings.NewReader("{comment: New web server, labels: {labelname1: labelvalue1}}")
-	req = authedRequest("POST", testURL+"/targets/myhost", rdr)
+	req = authedRequest("POST", testURL+"/targets/myhost:8080", rdr)
 	verifyResponse(t, req, http.StatusBadRequest, "Unable to parse request body invalid character 'c' looking for beginning of object key string\n")
 
 	//Add a 2nd target with the same labels
 	rdr = strings.NewReader(`{"comment": "New web server2", "labels": {"labelname1": "labelvalue2"}}`)
-	req = authedRequest("POST", testURL+"/targets/myhost2", rdr)
+	req = authedRequest("POST", testURL+"/targets/myhost2:8080", rdr)
 	verifyResponse(t, req, http.StatusAccepted, "")
-	myhost2 := `{"Host":"myhost2","labels":{"labelname1":"labelvalue2"}}`
-	verifyTargets(t, testURL, "["+selfTarget+","+myhost+","+myhost2+"]")
+	myhost2 := `{"Host":"myhost2:8080","labels":{"labelname1":"labelvalue2"}}`
+	targets = []string{selfTarget, myhost2, myhost}
+	verifyTargets(t, testURL, fmt.Sprintf("[%v]", strings.Join(targets, ",")))
 
 	//Add a 3rd target with different labels and a specific port
 	rdr = strings.NewReader(`{"comment": "New web server3", "labels": {"labelname1": "labelvalue1-3", "labelname3": "labelvalue3"}}`)
 	req = authedRequest("POST", testURL+"/targets/10.0.0.1:9127", rdr)
 	verifyResponse(t, req, http.StatusAccepted, "")
 	myhost3 := `{"Host":"10.0.0.1:9127","labels":{"labelname1":"labelvalue1-3","labelname3":"labelvalue3"}}`
-	verifyTargets(t, testURL, "["+myhost3+","+selfTarget+","+myhost+","+myhost2+"]")
+	targets = []string{myhost3, selfTarget, myhost2, myhost}
+	verifyTargets(t, testURL, fmt.Sprintf("[%v]", strings.Join(targets, ",")))
 
 	//Get a single host, multiple is being tested repeatedly in verifyTargets
-	req = authedRequest("GET", testURL+"/targets/myhost2", nil)
+	req = authedRequest("GET", testURL+"/targets/myhost2:8080", nil)
 	verifyResponse(t, req, http.StatusOK, "["+myhost2+"]")
 
 	//Delete target3
 	rdr = strings.NewReader(`{"comment": "Decommissioning web server3"}`)
 	req = authedRequest("DELETE", testURL+"/targets/10.0.0.1:9127", rdr)
 	verifyResponse(t, req, http.StatusAccepted, "")
-	verifyTargets(t, testURL, "["+selfTarget+","+myhost+","+myhost2+"]")
+	targets = []string{selfTarget, myhost2, myhost}
+	verifyTargets(t, testURL, fmt.Sprintf("[%v]", strings.Join(targets, ",")))
 
 	//Try to get host3 should be empty
 	req = authedRequest("GET", testURL+"/targets/10.0.0.1:9127", nil)
@@ -129,23 +148,26 @@ func TestAPI(t *testing.T) {
 
 	//Update Lables on target1
 	rdr = strings.NewReader(`{"comment": "Update web server lables", "labels": {"labelname1_updated": "labelvalue1_updated"}}`)
-	req = authedRequest("PUT", testURL+"/targets/myhost", rdr)
+	req = authedRequest("PUT", testURL+"/targets/myhost:8080", rdr)
 	verifyResponse(t, req, http.StatusAccepted, "")
-	myhostmod := `{"Host":"myhost","labels":{"labelname1_updated":"labelvalue1_updated"}}`
-	verifyTargets(t, testURL, "["+selfTarget+","+myhostmod+","+myhost2+"]")
+	myhostmod := `{"Host":"myhost:8080","labels":{"labelname1_updated":"labelvalue1_updated"}}`
+	targets = []string{selfTarget, myhost2, myhostmod}
+	verifyTargets(t, testURL, fmt.Sprintf("[%v]", strings.Join(targets, ",")))
 
 	//Add a 4th target with the same labels as target2
 	rdr = strings.NewReader(`{"comment": "New web server2", "labels": {"labelname1": "labelvalue2"}}`)
-	req = authedRequest("POST", testURL+"/targets/myhost4", rdr)
+	req = authedRequest("POST", testURL+"/targets/myhost4:8080", rdr)
 	verifyResponse(t, req, http.StatusAccepted, "")
-	myhost4 := `{"Host":"myhost4","labels":{"labelname1":"labelvalue2"}}`
-	verifyTargets(t, testURL, "["+selfTarget+","+myhostmod+","+myhost2+","+myhost4+"]")
+	myhost4 := `{"Host":"myhost4:8080","labels":{"labelname1":"labelvalue2"}}`
+	targets = []string{selfTarget, myhost2, myhost4, myhostmod}
+	verifyTargets(t, testURL, fmt.Sprintf("[%v]", strings.Join(targets, ",")))
 
 	//Delete 4th target it shares labels with target2 so exercises a different section of code.
 	rdr = strings.NewReader(`{"comment": "Decommissioning web server4"}`)
-	req = authedRequest("DELETE", testURL+"/targets/myhost4", rdr)
+	req = authedRequest("DELETE", testURL+"/targets/myhost4:8080", rdr)
 	verifyResponse(t, req, http.StatusAccepted, "")
-	verifyTargets(t, testURL, "["+selfTarget+","+myhostmod+","+myhost2+"]")
+	targets = []string{selfTarget, myhost2, myhostmod}
+	verifyTargets(t, testURL, fmt.Sprintf("[%v]", strings.Join(targets, ",")))
 }
 
 //authedRequest returns an http.Request with the proper auth headers.
diff --git a/snapcraft.yaml b/snapcraft.yaml
index 9d019a8..ea0e3b4 100644
--- a/snapcraft.yaml
+++ b/snapcraft.yaml
@@ -1,5 +1,5 @@
 name: promreg
-version: 0.0.8
+version: 0.0.9
 summary: A simple REST API for registering targets with Prometheus.
 description: >
   This service provides a simple API that can run alongside Prometheus allowing adding, updating and deleting of targets via a REST API and without reloading the Prometheus configuration. See https://launchpad.net/prometheus-registration for more information.