retrieval: fix IPv6 port default, add tests

This fixes port defaulting for IPv6 addresses and restructures
and test the construction of target label sets.

retrieval/targetmanager.go

@@ -15,6 +15,7 @@ package retrieval
 
 import (
 	"fmt"
+	"net"
 	"sort"
 	"strings"
 	"sync"
@@ -417,79 +418,103 @@ func providersFromConfig(cfg *config.ScrapeConfig) map[string]TargetProvider {
 	return providers
 }
 
-// targetsFromGroup builds targets based on the given TargetGroup and config.
+// populateLabels builds a label set from the given label set and scrape configuration.
-// Panics if target group is nil.
+// It returns a label set before relabeling was applied as the second return value.
-func targetsFromGroup(tg *config.TargetGroup, cfg *config.ScrapeConfig) ([]*Target, error) {
+// Returns a nil label set if the target is dropped during relabeling.
-	targets := make([]*Target, 0, len(tg.Targets))
+func populateLabels(lset model.LabelSet, cfg *config.ScrapeConfig) (res, orig model.LabelSet, err error) {
-
+	if _, ok := lset[model.AddressLabel]; !ok {
-	for i, labels := range tg.Targets {
+		return nil, nil, fmt.Errorf("no address")
-		for k, v := range cfg.Params {
-			if len(v) > 0 {
-				labels[model.LabelName(model.ParamLabelPrefix+k)] = model.LabelValue(v[0])
-			}
 	}
 	// Copy labels into the labelset for the target if they are not
 	// set already. Apply the labelsets in order of decreasing precedence.
-		labelsets := []model.LabelSet{
+	scrapeLabels := model.LabelSet{
-			tg.Labels,
-			{
 		model.SchemeLabel:      model.LabelValue(cfg.Scheme),
 		model.MetricsPathLabel: model.LabelValue(cfg.MetricsPath),
 		model.JobLabel:         model.LabelValue(cfg.JobName),
-			},
 	}
-		for _, lset := range labelsets {
+	for ln, lv := range scrapeLabels {
-			for ln, lv := range lset {
+		if _, ok := lset[ln]; !ok {
-				if _, ok := labels[ln]; !ok {
+			lset[ln] = lv
-					labels[ln] = lv
 		}
 	}
+	// Encode scrape query parameters as labels.
+	for k, v := range cfg.Params {
+		if len(v) > 0 {
+			lset[model.LabelName(model.ParamLabelPrefix+k)] = model.LabelValue(v[0])
+		}
 	}
 
-		if _, ok := labels[model.AddressLabel]; !ok {
+	preRelabelLabels := lset
-			return nil, fmt.Errorf("instance %d in target group %s has no address", i, tg)
+	lset = relabel.Process(lset, cfg.RelabelConfigs...)
-		}
-
-		preRelabelLabels := labels
-
-		labels := relabel.Process(labels, cfg.RelabelConfigs...)
 
 	// Check if the target was dropped.
-		if labels == nil {
+	if lset == nil {
-			continue
+		return nil, nil, nil
-		}
+	}
-		// If no port was provided, infer it based on the used scheme.
+
-		addr := string(labels[model.AddressLabel])
+	// addPort checks whether we should add a default port to the address.
-		if !strings.Contains(addr, ":") {
+	// If the address is not valid, we don't append a port either.
-			switch labels[model.SchemeLabel] {
+	addPort := func(s string) bool {
-			case "http", "":
+		// If we can split, a port exists and we don't have to add one.
-				addr = fmt.Sprintf("%s:80", addr)
+		if _, _, err := net.SplitHostPort(s); err == nil {
-			case "https":
+			return false
-				addr = fmt.Sprintf("%s:443", addr)
+		}
-			default:
+		// If adding a port makes it valid, the previous error
-				return nil, fmt.Errorf("invalid scheme: %q", cfg.Scheme)
+		// was not due to an invalid address and we can append a port.
-			}
+		_, _, err := net.SplitHostPort(s + ":1234")
-			labels[model.AddressLabel] = model.LabelValue(addr)
+		return err == nil
-		}
+	}
-		if err := config.CheckTargetAddress(labels[model.AddressLabel]); err != nil {
+	// If it's an address with no trailing port, infer it based on the used scheme.
-			return nil, err
+	if addr := string(lset[model.AddressLabel]); addPort(addr) {
+		// Addresses reaching this point are already wrapped in [] if necessary.
+		switch lset[model.SchemeLabel] {
+		case "http", "":
+			addr = addr + ":80"
+		case "https":
+			addr = addr + ":443"
+		default:
+			return nil, nil, fmt.Errorf("invalid scheme: %q", cfg.Scheme)
+		}
+		lset[model.AddressLabel] = model.LabelValue(addr)
+	}
+	if err := config.CheckTargetAddress(lset[model.AddressLabel]); err != nil {
+		return nil, nil, err
 	}
 
-		for ln := range labels {
 	// Meta labels are deleted after relabelling. Other internal labels propagate to
 	// the target which decides whether they will be part of their label set.
+	for ln := range lset {
 		if strings.HasPrefix(string(ln), model.MetaLabelPrefix) {
-				delete(labels, ln)
+			delete(lset, ln)
 		}
 	}
 
-		if _, ok := labels[model.InstanceLabel]; !ok {
+	// Default the instance label to the target address.
-			labels[model.InstanceLabel] = labels[model.AddressLabel]
+	if _, ok := lset[model.InstanceLabel]; !ok {
+		lset[model.InstanceLabel] = lset[model.AddressLabel]
+	}
+	return lset, preRelabelLabels, nil
 }
 
-		targets = append(targets, NewTarget(labels, preRelabelLabels, cfg.Params))
+// targetsFromGroup builds targets based on the given TargetGroup and config.
-	}
+func targetsFromGroup(tg *config.TargetGroup, cfg *config.ScrapeConfig) ([]*Target, error) {
+	targets := make([]*Target, 0, len(tg.Targets))
 
+	for i, lset := range tg.Targets {
+		// Combine target labels with target group labels.
+		for ln, lv := range tg.Labels {
+			if _, ok := lset[ln]; !ok {
+				lset[ln] = lv
+			}
+		}
+		labels, origLabels, err := populateLabels(lset, cfg)
+		if err != nil {
+			return nil, fmt.Errorf("instance %d in group %s: %s", i, tg, err)
+		}
+		if labels != nil {
+			targets = append(targets, NewTarget(labels, origLabels, cfg.Params))
+		}
+	}
 	return targets, nil
 }
 

retrieval/targetmanager_test.go

@@ -14,11 +14,13 @@
 package retrieval
 
 import (
+	"reflect"
 	"testing"
 
 	"golang.org/x/net/context"
 	"gopkg.in/yaml.v2"
 
+	"github.com/prometheus/common/model"
 	"github.com/prometheus/prometheus/config"
 	"github.com/prometheus/prometheus/storage/local"
 )
@@ -72,3 +74,135 @@ dns_sd_configs:
 	verifyPresence(ts.tgroups, "dns/0/srv.name.one.example.org", false)
 	verifyPresence(ts.tgroups, "dns/0/srv.name.two.example.org", true)
 }
+
+func mustNewRegexp(s string) config.Regexp {
+	re, err := config.NewRegexp(s)
+	if err != nil {
+		panic(err)
+	}
+	return re
+}
+
+func TestPopulateLabels(t *testing.T) {
+	cases := []struct {
+		in      model.LabelSet
+		cfg     *config.ScrapeConfig
+		res     model.LabelSet
+		resOrig model.LabelSet
+	}{
+		// Regular population of scrape config options.
+		{
+			in: model.LabelSet{
+				model.AddressLabel: "1.2.3.4:1000",
+				"custom":           "value",
+			},
+			cfg: &config.ScrapeConfig{
+				Scheme:      "https",
+				MetricsPath: "/metrics",
+				JobName:     "job",
+			},
+			res: model.LabelSet{
+				model.AddressLabel:     "1.2.3.4:1000",
+				model.InstanceLabel:    "1.2.3.4:1000",
+				model.SchemeLabel:      "https",
+				model.MetricsPathLabel: "/metrics",
+				model.JobLabel:         "job",
+				"custom":               "value",
+			},
+			resOrig: model.LabelSet{
+				model.AddressLabel:     "1.2.3.4:1000",
+				model.SchemeLabel:      "https",
+				model.MetricsPathLabel: "/metrics",
+				model.JobLabel:         "job",
+				"custom":               "value",
+			},
+		},
+		// Pre-define/overwrite scrape config labels.
+		// Leave out port and expect it to be defaulted to scheme.
+		{
+			in: model.LabelSet{
+				model.AddressLabel:     "1.2.3.4",
+				model.SchemeLabel:      "http",
+				model.MetricsPathLabel: "/custom",
+				model.JobLabel:         "custom-job",
+			},
+			cfg: &config.ScrapeConfig{
+				Scheme:      "https",
+				MetricsPath: "/metrics",
+				JobName:     "job",
+			},
+			res: model.LabelSet{
+				model.AddressLabel:     "1.2.3.4:80",
+				model.InstanceLabel:    "1.2.3.4:80",
+				model.SchemeLabel:      "http",
+				model.MetricsPathLabel: "/custom",
+				model.JobLabel:         "custom-job",
+			},
+			resOrig: model.LabelSet{
+				model.AddressLabel:     "1.2.3.4",
+				model.SchemeLabel:      "http",
+				model.MetricsPathLabel: "/custom",
+				model.JobLabel:         "custom-job",
+			},
+		},
+		// Provide instance label. HTTPS port default for IPv6.
+		{
+			in: model.LabelSet{
+				model.AddressLabel:  "[::1]",
+				model.InstanceLabel: "custom-instance",
+			},
+			cfg: &config.ScrapeConfig{
+				Scheme:      "https",
+				MetricsPath: "/metrics",
+				JobName:     "job",
+			},
+			res: model.LabelSet{
+				model.AddressLabel:     "[::1]:443",
+				model.InstanceLabel:    "custom-instance",
+				model.SchemeLabel:      "https",
+				model.MetricsPathLabel: "/metrics",
+				model.JobLabel:         "job",
+			},
+			resOrig: model.LabelSet{
+				model.AddressLabel:     "[::1]",
+				model.InstanceLabel:    "custom-instance",
+				model.SchemeLabel:      "https",
+				model.MetricsPathLabel: "/metrics",
+				model.JobLabel:         "job",
+			},
+		},
+		// Apply relabeling.
+		{
+			in: model.LabelSet{
+				model.AddressLabel: "1.2.3.4:1000",
+				"custom":           "value",
+			},
+			cfg: &config.ScrapeConfig{
+				Scheme:      "https",
+				MetricsPath: "/metrics",
+				JobName:     "job",
+				RelabelConfigs: []*config.RelabelConfig{
+					{
+						Action:       config.RelabelDrop,
+						Regex:        mustNewRegexp(".*"),
+						SourceLabels: model.LabelNames{"job"},
+					},
+				},
+			},
+			res:     nil,
+			resOrig: nil,
+		},
+	}
+	for i, c := range cases {
+		res, orig, err := populateLabels(c.in, c.cfg)
+		if err != nil {
+			t.Fatalf("case %d: %s", i, err)
+		}
+		if !reflect.DeepEqual(res, c.res) {
+			t.Errorf("case %d: expected res\n\t%+v\n got\n\t%+v", i, c.res, res)
+		}
+		if !reflect.DeepEqual(orig, c.resOrig) {
+			t.Errorf("case %d: expected resOrig\n\t%+v\n got\n\t%+v", i, c.resOrig, orig)
+		}
+	}
+}