From d77890a011e98e292de4625961750db3116d94e8 Mon Sep 17 00:00:00 2001 From: Kyle Havlovitz Date: Wed, 11 Jan 2017 14:41:12 -0500 Subject: [PATCH] Validate metadata config earlier and handle multiple filters --- command/agent/agent.go | 32 ++++++++++------- command/agent/agent_test.go | 52 +++++++++++++-------------- command/agent/catalog_endpoint.go | 4 +-- command/agent/command.go | 11 +++--- command/agent/config.go | 4 ++- command/agent/http.go | 18 ++++++---- consul/catalog_endpoint.go | 8 ++--- consul/catalog_endpoint_test.go | 59 ++++++++++++++----------------- consul/state/state_store.go | 24 ++++++++++--- consul/state/state_store_test.go | 12 +++---- consul/structs/structs.go | 13 ++++--- 11 files changed, 132 insertions(+), 105 deletions(-) diff --git a/command/agent/agent.go b/command/agent/agent.go index 2753d3159..582c62570 100644 --- a/command/agent/agent.go +++ b/command/agent/agent.go @@ -1695,20 +1695,13 @@ 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 { - return fmt.Errorf("Node metadata cannot contain more than %d key/value pairs", metaMaxKeyPairs) - } - for key, value := range conf.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 } @@ -1717,6 +1710,21 @@ func (a *Agent) loadMetadata(conf *Config) error { 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 meta { + if err := validateMetaPair(key, value); err != nil { + return fmt.Errorf("Couldn't load metadata pair ('%s', '%s'): %s", key, value, err) + } + } + + return nil +} + // validateMetaPair checks that the given key/value pair is in a valid format func validateMetaPair(key, value string) error { if key == "" { @@ -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 diff --git a/command/agent/agent_test.go b/command/agent/agent_test.go index 3306da285..4eedaba0b 100644 --- a/command/agent/agent_test.go +++ b/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) } } } diff --git a/command/agent/catalog_endpoint.go b/command/agent/catalog_endpoint.go index 05b431a92..913b3af1e 100644 --- a/command/agent/catalog_endpoint.go +++ b/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 } diff --git a/command/agent/command.go b/command/agent/command.go index 165839fdc..03f3b25e1 100644 --- a/command/agent/command.go +++ b/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 { diff --git a/command/agent/config.go b/command/agent/config.go index 9dfa083c6..266ad4eca 100644 --- a/command/agent/config.go +++ b/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 diff --git a/command/agent/http.go b/command/agent/http.go index 4c4a3ab2a..74f1f2531 100644 --- a/command/agent/http.go +++ b/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 diff --git a/consul/catalog_endpoint.go b/consul/catalog_endpoint.go index 2887559ac..c19082a8f 100644 --- a/consul/catalog_endpoint.go +++ b/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() } diff --git a/consul/catalog_endpoint_test.go b/consul/catalog_endpoint_test.go index 604b4ee25..b983eb59a 100644 --- a/consul/catalog_endpoint_test.go +++ b/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) } diff --git a/consul/state/state_store.go b/consul/state/state_store.go index eb2fcb8ce..b11ac91ee 100644 --- a/consul/state/state_store.go +++ b/consul/state/state_store.go @@ -550,7 +550,10 @@ func (s *StateStore) Nodes() (uint64, structs.Nodes, error) { } // NodesByMeta is used to return all nodes with the given meta key/value pair. -func (s *StateStore) NodesByMeta(key, value string) (uint64, structs.Nodes, error) { +func (s *StateStore) NodesByMeta(filters map[string]string) (uint64, structs.Nodes, error) { + if len(filters) > 1 { + return 0, nil, fmt.Errorf("multiple meta filters not supported") + } tx := s.db.Txn(false) defer tx.Abort() @@ -558,7 +561,11 @@ func (s *StateStore) NodesByMeta(key, value string) (uint64, structs.Nodes, erro idx := maxIndexTxn(tx, s.getWatchTables("Nodes")...) // Retrieve all of the nodes - nodes, err := tx.Get("nodes", "meta", key, value) + var args []interface{} + for key, value := range filters { + args = append(args, key, value) + } + nodes, err := tx.Get("nodes", "meta", args...) if err != nil { return 0, nil, fmt.Errorf("failed nodes lookup: %s", err) } @@ -781,8 +788,11 @@ func (s *StateStore) Services() (uint64, structs.Services, error) { return idx, results, nil } -// Services returns all services, filtered by given node metadata. -func (s *StateStore) ServicesByNodeMeta(key, value string) (uint64, structs.Services, error) { +// Services returns all services, filtered by the given node metadata. +func (s *StateStore) ServicesByNodeMeta(filters map[string]string) (uint64, structs.Services, error) { + if len(filters) > 1 { + return 0, nil, fmt.Errorf("multiple meta filters not supported") + } tx := s.db.Txn(false) defer tx.Abort() @@ -790,7 +800,11 @@ func (s *StateStore) ServicesByNodeMeta(key, value string) (uint64, structs.Serv idx := maxIndexTxn(tx, s.getWatchTables("ServiceNodes")...) // Retrieve all of the nodes with the meta k/v pair - nodes, err := tx.Get("nodes", "meta", key, value) + var args []interface{} + for key, value := range filters { + args = append(args, key, value) + } + nodes, err := tx.Get("nodes", "meta", args...) if err != nil { return 0, nil, fmt.Errorf("failed nodes lookup: %s", err) } diff --git a/consul/state/state_store_test.go b/consul/state/state_store_test.go index 3277cb391..855423067 100644 --- a/consul/state/state_store_test.go +++ b/consul/state/state_store_test.go @@ -759,7 +759,7 @@ func TestStateStore_GetNodesByMeta(t *testing.T) { s := testStateStore(t) // Listing with no results returns nil - idx, res, err := s.NodesByMeta("somekey", "somevalue") + idx, res, err := s.NodesByMeta(map[string]string{"somekey": "somevalue"}) if idx != 0 || res != nil || err != nil { t.Fatalf("expected (0, nil, nil), got: (%d, %#v, %#v)", idx, res, err) } @@ -775,7 +775,7 @@ func TestStateStore_GetNodesByMeta(t *testing.T) { } // Retrieve the node with role=client - idx, nodes, err := s.NodesByMeta("role", "client") + idx, nodes, err := s.NodesByMeta(map[string]string{"role": "client"}) if err != nil { t.Fatalf("err: %s", err) } @@ -800,7 +800,7 @@ func TestStateStore_GetNodesByMeta(t *testing.T) { } // Retrieve both nodes via their common meta field - idx, nodes, err = s.NodesByMeta("common", "1") + idx, nodes, err = s.NodesByMeta(map[string]string{"common": "1"}) if err != nil { t.Fatalf("err: %s", err) } @@ -1160,7 +1160,7 @@ func TestStateStore_ServicesByNodeMeta(t *testing.T) { s := testStateStore(t) // Listing with no results returns nil - idx, res, err := s.ServicesByNodeMeta("somekey", "somevalue") + idx, res, err := s.ServicesByNodeMeta(map[string]string{"somekey": "somevalue"}) if idx != 0 || len(res) != 0 || err != nil { t.Fatalf("expected (0, nil, nil), got: (%d, %#v, %#v)", idx, res, err) } @@ -1196,7 +1196,7 @@ func TestStateStore_ServicesByNodeMeta(t *testing.T) { } // Filter the services by the first node's meta value - idx, res, err = s.ServicesByNodeMeta("role", "client") + idx, res, err = s.ServicesByNodeMeta(map[string]string{"role": "client"}) if err != nil { t.Fatalf("err: %s", err) } @@ -1211,7 +1211,7 @@ func TestStateStore_ServicesByNodeMeta(t *testing.T) { } // Get all services using the common meta value - idx, res, err = s.ServicesByNodeMeta("common", "1") + idx, res, err = s.ServicesByNodeMeta(map[string]string{"common": "1"}) if err != nil { t.Fatalf("err: %s", err) } diff --git a/consul/structs/structs.go b/consul/structs/structs.go index bb4f6472d..e7dbbd068 100644 --- a/consul/structs/structs.go +++ b/consul/structs/structs.go @@ -229,10 +229,9 @@ type QuerySource struct { // DCSpecificRequest is used to query about a specific DC type DCSpecificRequest struct { - Datacenter string - NodeMetaKey string - NodeMetaValue string - Source QuerySource + Datacenter string + NodeMetaFilters map[string]string + Source QuerySource QueryOptions } @@ -288,6 +287,12 @@ type Node struct { } type Nodes []*Node +// Used for filtering nodes by metadata key/value pairs +type NodeMetaFilter struct { + Key string + Value string +} + // Used to return information about a provided services. // Maps service name to available tags type Services map[string][]string