Validate metadata config earlier and handle multiple filters

command/agent/agent.go

@@ -1695,25 +1695,33 @@ func (a *Agent) restoreCheckState(snap map[types.CheckID]*structs.HealthCheck) {
 	}
 }
 
-// loadMetadata loads and validates node metadata fields from the config and
+// loadMetadata loads node metadata fields from the agent config and
 // updates them on the local agent.
 func (a *Agent) loadMetadata(conf *Config) error {
 	a.state.Lock()
 	defer a.state.Unlock()
 
-	if len(conf.Meta) > metaMaxKeyPairs {
+	for key, value := range conf.Meta {
+		a.state.metadata[key] = value
+	}
+
+	a.state.changeMade()
+
+	return nil
+}
+
+// validateMeta validates a set of key/value pairs from the agent config
+func validateMetadata(meta map[string]string) error {
+	if len(meta) > metaMaxKeyPairs {
 		return fmt.Errorf("Node metadata cannot contain more than %d key/value pairs", metaMaxKeyPairs)
 	}
 
-	for key, value := range conf.Meta {
+	for key, value := range meta {
 		if err := validateMetaPair(key, value); err != nil {
 			return fmt.Errorf("Couldn't load metadata pair ('%s', '%s'): %s", key, value, err)
 		}
-		a.state.metadata[key] = value
 	}
 
-	a.state.changeMade()
-
 	return nil
 }
 
@@ -1726,25 +1734,23 @@ func validateMetaPair(key, value string) error {
 		return fmt.Errorf("Key contains invalid characters")
 	}
 	if len(key) > metaKeyMaxLength {
-		return fmt.Errorf("Key is longer than %d chars", metaKeyMaxLength)
+		return fmt.Errorf("Key is too long (limit: %d characters)", metaKeyMaxLength)
 	}
 	if strings.HasPrefix(key, metaKeyReservedPrefix) {
 		return fmt.Errorf("Key prefix '%s' is reserved for internal use", metaKeyReservedPrefix)
 	}
 	if len(value) > metaValueMaxLength {
-		return fmt.Errorf("Value is longer than %d characters", metaValueMaxLength)
+		return fmt.Errorf("Value is too long (limit: %d characters)", metaValueMaxLength)
 	}
 	return nil
 }
 
 // unloadMetadata resets the local metadata state
-func (a *Agent) unloadMetadata() error {
+func (a *Agent) unloadMetadata() {
 	a.state.Lock()
 	defer a.state.Unlock()
 
 	a.state.metadata = make(map[string]string)
-
-	return nil
 }
 
 // serviceMaintCheckID returns the ID of a given service's maintenance check

command/agent/agent_test.go

@@ -19,6 +19,7 @@ import (
 	"github.com/hashicorp/consul/logger"
 	"github.com/hashicorp/consul/testutil"
 	"github.com/hashicorp/raft"
+	"strings"
 )
 
 const (
@@ -1852,69 +1853,64 @@ func TestAgent_purgeCheckState(t *testing.T) {
 }
 
 func TestAgent_metadata(t *testing.T) {
-	config := nextConfig()
-	dir, agent := makeAgent(t, config)
-	defer os.RemoveAll(dir)
-	defer agent.Shutdown()
-
 	// Load a valid set of key/value pairs
-	config.Meta = map[string]string{
+	meta := map[string]string{
 		"key1": "value1",
 		"key2": "value2",
 	}
 	// Should succeed
-	if err := agent.loadMetadata(config); err != nil {
+	if err := validateMetadata(meta); err != nil {
 		t.Fatalf("err: %s", err)
 	}
-	agent.unloadMetadata()
 
 	// Should get error
-	config.Meta = map[string]string{
+	meta = map[string]string{
 		"": "value1",
 	}
-	if err := agent.loadMetadata(config); err == nil {
+	if err := validateMetadata(meta); !strings.Contains(err.Error(), "Couldn't load metadata pair") {
 		t.Fatalf("should have failed")
 	}
-	agent.unloadMetadata()
 
 	// Should get error
-	tooManyKeys := make(map[string]string)
+	meta = make(map[string]string)
 	for i := 0; i < metaMaxKeyPairs+1; i++ {
-		tooManyKeys[string(i)] = "value"
+		meta[string(i)] = "value"
 	}
-	if err := agent.loadMetadata(config); err == nil {
+	if err := validateMetadata(meta); !strings.Contains(err.Error(), "cannot contain more than") {
 		t.Fatalf("should have failed")
 	}
 }
 
 func TestAgent_validateMetaPair(t *testing.T) {
-	longKey := fmt.Sprintf(fmt.Sprintf("%%%ds", metaKeyMaxLength+1), "")
-	longValue := fmt.Sprintf(fmt.Sprintf("%%%ds", metaValueMaxLength+1), "")
+	longKey := strings.Repeat("a", metaKeyMaxLength+1)
+	longValue := strings.Repeat("b", metaValueMaxLength+1)
 	pairs := []struct {
-		Key     string
-		Value   string
-		Success bool
+		Key   string
+		Value string
+		Error string
 	}{
 		// valid pair
-		{"key", "value", true},
+		{"key", "value", ""},
 		// invalid, blank key
-		{"", "value", false},
+		{"", "value", "cannot be blank"},
 		// allowed special chars in key name
-		{"k_e-y", "value", true},
+		{"k_e-y", "value", ""},
 		// disallowed special chars in key name
-		{"(%key&)", "value", false},
+		{"(%key&)", "value", "invalid characters"},
 		// key too long
-		{longKey, "value", false},
+		{longKey, "value", "Key is too long"},
 		// reserved prefix
-		{metaKeyReservedPrefix + "key", "value", false},
+		{metaKeyReservedPrefix + "key", "value", "reserved for internal use"},
 		// value too long
-		{"key", longValue, false},
+		{"key", longValue, "Value is too long"},
 	}
 
 	for _, pair := range pairs {
 		err := validateMetaPair(pair.Key, pair.Value)
-		if pair.Success != (err == nil) {
-			t.Fatalf("bad: %v, %v", pair, err)
+		if pair.Error == "" && err != nil {
+			t.Fatalf("should have succeeded: %v, %v", pair, err)
+		} else if pair.Error != "" && !strings.Contains(err.Error(), pair.Error) {
+			t.Fatalf("should have failed: %v, %v", pair, err)
 		}
 	}
 }

command/agent/catalog_endpoint.go

@@ -64,7 +64,7 @@ func (s *HTTPServer) CatalogNodes(resp http.ResponseWriter, req *http.Request) (
 	// Setup the request
 	args := structs.DCSpecificRequest{}
 	s.parseSource(req, &args.Source)
-	s.parseMetaFilter(req, &args.NodeMetaKey, &args.NodeMetaValue)
+	args.NodeMetaFilters = s.parseMetaFilter(req)
 	if done := s.parse(resp, req, &args.Datacenter, &args.QueryOptions); done {
 		return nil, nil
 	}
@@ -86,7 +86,7 @@ func (s *HTTPServer) CatalogNodes(resp http.ResponseWriter, req *http.Request) (
 func (s *HTTPServer) CatalogServices(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
 	// Set default DC
 	args := structs.DCSpecificRequest{}
-	s.parseMetaFilter(req, &args.NodeMetaKey, &args.NodeMetaValue)
+	args.NodeMetaFilters = s.parseMetaFilter(req)
 	if done := s.parse(resp, req, &args.Datacenter, &args.QueryOptions); done {
 		return nil, nil
 	}

command/agent/command.go

@@ -364,6 +364,12 @@ func (c *Command) readConfig() *Config {
 		}
 	}
 
+	// Verify the node metadata entries are valid
+	if err := validateMetadata(config.Meta); err != nil {
+		c.Ui.Error(fmt.Sprintf("Failed to parse node metadata: %v", err))
+		return nil
+	}
+
 	// Set the version info
 	config.Revision = c.Revision
 	config.Version = c.Version
@@ -1081,10 +1087,7 @@ func (c *Command) handleReload(config *Config) (*Config, error) {
 		errs = multierror.Append(errs, fmt.Errorf("Failed unloading checks: %s", err))
 		return nil, errs
 	}
-	if err := c.agent.unloadMetadata(); err != nil {
-		errs = multierror.Append(errs, fmt.Errorf("Failed unloading metadata: %s", err))
-		return nil, errs
-	}
+	c.agent.unloadMetadata()
 
 	// Reload service/check definitions and metadata.
 	if err := c.agent.loadServices(newConf); err != nil {

command/agent/config.go

@@ -342,7 +342,9 @@ type Config struct {
 	// they are configured with TranslateWanAddrs set to true.
 	TaggedAddresses map[string]string
 
-	// Node metadata
+	// Node metadata key/value pairs. These are excluded from JSON output
+	// because they can be reloaded and might be stale when shown from the
+	// config instead of the local state.
 	Meta map[string]string `mapstructure:"node_meta" json:"-"`
 
 	// LeaveOnTerm controls if Serf does a graceful leave when receiving

command/agent/http.go

@@ -586,14 +586,20 @@ func (s *HTTPServer) parseSource(req *http.Request, source *structs.QuerySource)
 
 // parseMetaFilter is used to parse the ?node-meta=key:value query parameter, used for
 // filtering results to nodes with the given metadata key/value
-func (s *HTTPServer) parseMetaFilter(req *http.Request, key *string, value *string) {
-	if filter, ok := req.URL.Query()["node-meta"]; ok && len(filter) > 0 {
-		pair := strings.SplitN(filter[0], ":", 2)
-		*key = pair[0]
-		if len(pair) == 2 {
-			*value = pair[1]
+func (s *HTTPServer) parseMetaFilter(req *http.Request) map[string]string {
+	if filterList, ok := req.URL.Query()["node-meta"]; ok {
+		filters := make(map[string]string)
+		for _, filter := range filterList {
+			pair := strings.SplitN(filter, ":", 2)
+			if len(pair) == 2 {
+				filters[pair[0]] = pair[1]
+			} else {
+				filters[pair[0]] = ""
+			}
 		}
+		return filters
 	}
+	return nil
 }
 
 // parse is a convenience method for endpoints that need

consul/catalog_endpoint.go

@@ -166,8 +166,8 @@ func (c *Catalog) ListNodes(args *structs.DCSpecificRequest, reply *structs.Inde
 			var index uint64
 			var nodes structs.Nodes
 			var err error
-			if args.NodeMetaKey != "" {
-				index, nodes, err = state.NodesByMeta(args.NodeMetaKey, args.NodeMetaValue)
+			if len(args.NodeMetaFilters) > 0 {
+				index, nodes, err = state.NodesByMeta(args.NodeMetaFilters)
 			} else {
 				index, nodes, err = state.Nodes()
 			}
@@ -199,8 +199,8 @@ func (c *Catalog) ListServices(args *structs.DCSpecificRequest, reply *structs.I
 			var index uint64
 			var services structs.Services
 			var err error
-			if args.NodeMetaKey != "" {
-				index, services, err = state.ServicesByNodeMeta(args.NodeMetaKey, args.NodeMetaValue)
+			if len(args.NodeMetaFilters) > 0 {
+				index, services, err = state.ServicesByNodeMeta(args.NodeMetaFilters)
 			} else {
 				index, services, err = state.Services()
 			}

consul/catalog_endpoint_test.go

@@ -599,18 +599,6 @@ func TestCatalog_ListNodes_MetaFilter(t *testing.T) {
 	codec := rpcClient(t, s1)
 	defer codec.Close()
 
-	// Filter by a specific meta k/v pair
-	args := structs.DCSpecificRequest{
-		Datacenter:    "dc1",
-		NodeMetaKey:   "somekey",
-		NodeMetaValue: "somevalue",
-	}
-	var out structs.IndexedNodes
-	err := msgpackrpc.CallWithCodec(codec, "Catalog.ListNodes", &args, &out)
-	if err != nil {
-		t.Fatalf("err: %v", err)
-	}
-
 	testutil.WaitForLeader(t, s1.RPC, "dc1")
 
 	// Add a new node with the right meta k/v pair
@@ -619,6 +607,15 @@ func TestCatalog_ListNodes_MetaFilter(t *testing.T) {
 		t.Fatalf("err: %v", err)
 	}
 
+	// Filter by a specific meta k/v pair
+	args := structs.DCSpecificRequest{
+		Datacenter: "dc1",
+		NodeMetaFilters: map[string]string{
+			"somekey": "somevalue",
+		},
+	}
+	var out structs.IndexedNodes
+
 	testutil.WaitForResult(func() (bool, error) {
 		msgpackrpc.CallWithCodec(codec, "Catalog.ListNodes", &args, &out)
 		return len(out.Nodes) == 1, nil
@@ -639,12 +636,13 @@ func TestCatalog_ListNodes_MetaFilter(t *testing.T) {
 
 	// Now filter on a nonexistent meta k/v pair
 	args = structs.DCSpecificRequest{
-		Datacenter:    "dc1",
-		NodeMetaKey:   "somekey",
-		NodeMetaValue: "invalid",
+		Datacenter: "dc1",
+		NodeMetaFilters: map[string]string{
+			"somekey": "invalid",
+		},
 	}
 	out = structs.IndexedNodes{}
-	err = msgpackrpc.CallWithCodec(codec, "Catalog.ListNodes", &args, &out)
+	err := msgpackrpc.CallWithCodec(codec, "Catalog.ListNodes", &args, &out)
 	if err != nil {
 		t.Fatalf("err: %v", err)
 	}
@@ -1069,18 +1067,6 @@ func TestCatalog_ListServices_MetaFilter(t *testing.T) {
 	codec := rpcClient(t, s1)
 	defer codec.Close()
 
-	// Filter by a specific meta k/v pair
-	args := structs.DCSpecificRequest{
-		Datacenter:    "dc1",
-		NodeMetaKey:   "somekey",
-		NodeMetaValue: "somevalue",
-	}
-	var out structs.IndexedServices
-	err := msgpackrpc.CallWithCodec(codec, "Catalog.ListServices", &args, &out)
-	if err != nil {
-		t.Fatalf("err: %v", err)
-	}
-
 	testutil.WaitForLeader(t, s1.RPC, "dc1")
 
 	// Add a new node with the right meta k/v pair
@@ -1093,6 +1079,14 @@ func TestCatalog_ListServices_MetaFilter(t *testing.T) {
 		t.Fatalf("err: %v", err)
 	}
 
+	// Filter by a specific meta k/v pair
+	args := structs.DCSpecificRequest{
+		Datacenter: "dc1",
+		NodeMetaFilters: map[string]string{
+			"somekey": "somevalue",
+		},
+	}
+	var out structs.IndexedServices
 	if err := msgpackrpc.CallWithCodec(codec, "Catalog.ListServices", &args, &out); err != nil {
 		t.Fatalf("err: %v", err)
 	}
@@ -1112,12 +1106,13 @@ func TestCatalog_ListServices_MetaFilter(t *testing.T) {
 
 	// Now filter on a nonexistent meta k/v pair
 	args = structs.DCSpecificRequest{
-		Datacenter:    "dc1",
-		NodeMetaKey:   "somekey",
-		NodeMetaValue: "invalid",
+		Datacenter: "dc1",
+		NodeMetaFilters: map[string]string{
+			"somekey": "invalid",
+		},
 	}
 	out = structs.IndexedServices{}
-	err = msgpackrpc.CallWithCodec(codec, "Catalog.ListServices", &args, &out)
+	err := msgpackrpc.CallWithCodec(codec, "Catalog.ListServices", &args, &out)
 	if err != nil {
 		t.Fatalf("err: %v", err)
 	}