Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature Request: Support key-value attributes for services #3881

Merged
merged 6 commits into from
Mar 27, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions agent/agent_endpoint.go
Original file line number Diff line number Diff line change
Expand Up @@ -579,6 +579,11 @@ func (s *HTTPServer) AgentRegisterService(resp http.ResponseWriter, req *http.Re

// Get the node service.
ns := args.NodeService()
if err := structs.ValidateMetadata(ns.ServiceMeta, false); err != nil {
resp.WriteHeader(http.StatusBadRequest)
fmt.Fprint(resp, fmt.Errorf("Invalid Service Meta: %v", err))
return nil, nil
}

// Verify the check type.
chkTypes, err := args.CheckTypes()
Expand Down
1 change: 1 addition & 0 deletions agent/config/runtime_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4075,6 +4075,7 @@ func TestSanitize(t *testing.T) {
"ID": "",
"Name": "foo",
"Port": 0,
"ServiceMeta": {},
"Tags": [],
"Token": "hidden"
}
Expand Down
3 changes: 3 additions & 0 deletions agent/consul/state/catalog.go
Original file line number Diff line number Diff line change
Expand Up @@ -610,6 +610,9 @@ func (s *Store) ensureServiceTxn(tx *memdb.Txn, idx uint64, node string, svc *st
return fmt.Errorf("failed service lookup: %s", err)
}

if err = structs.ValidateMetadata(svc.ServiceMeta, false); err != nil {
return fmt.Errorf("Invalid Service Meta for node %s and serviceID %s: %v", node, svc.ID, err)
}
// Create the service node entry and populate the indexes. Note that
// conversion doesn't populate any of the node-specific information.
// That's always populated when we read from the state store.
Expand Down
13 changes: 13 additions & 0 deletions agent/consul/state/catalog_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,19 @@ func TestStateStore_EnsureRegistration(t *testing.T) {
}
verifyNode()

// Add in a invalid service definition with too long Key value for ServiceMeta
req.Service = &structs.NodeService{
ID: "redis1",
Service: "redis",
Address: "1.1.1.1",
Port: 8080,
ServiceMeta: map[string]string{strings.Repeat("a", 129): "somevalue"},
Tags: []string{"master"},
}
if err := s.EnsureRegistration(9, req); err == nil {
t.Fatalf("Service should not have been registered since ServiceMeta is invalid")
}

// Add in a service definition.
req.Service = &structs.NodeService{
ID: "redis1",
Expand Down
2 changes: 2 additions & 0 deletions agent/structs/service_definition.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ type ServiceDefinition struct {
Name string
Tags []string
Address string
ServiceMeta map[string]string
Port int
Check CheckType
Checks CheckTypes
Expand All @@ -19,6 +20,7 @@ func (s *ServiceDefinition) NodeService() *NodeService {
Service: s.Name,
Tags: s.Tags,
Address: s.Address,
ServiceMeta: s.ServiceMeta,
Port: s.Port,
EnableTagOverride: s.EnableTagOverride,
}
Expand Down
10 changes: 10 additions & 0 deletions agent/structs/structs.go
Original file line number Diff line number Diff line change
Expand Up @@ -368,6 +368,7 @@ type ServiceNode struct {
ServiceName string
ServiceTags []string
ServiceAddress string
ServiceMeta map[string]string
ServicePort int
ServiceEnableTagOverride bool

Expand All @@ -379,6 +380,10 @@ type ServiceNode struct {
func (s *ServiceNode) PartialClone() *ServiceNode {
tags := make([]string, len(s.ServiceTags))
copy(tags, s.ServiceTags)
nsmeta := make(map[string]string)
for k, v := range s.ServiceMeta {
nsmeta[k] = v
}

return &ServiceNode{
// Skip ID, see above.
Expand All @@ -390,6 +395,7 @@ func (s *ServiceNode) PartialClone() *ServiceNode {
ServiceTags: tags,
ServiceAddress: s.ServiceAddress,
ServicePort: s.ServicePort,
ServiceMeta: nsmeta,
ServiceEnableTagOverride: s.ServiceEnableTagOverride,
RaftIndex: RaftIndex{
CreateIndex: s.CreateIndex,
Expand All @@ -406,6 +412,7 @@ func (s *ServiceNode) ToNodeService() *NodeService {
Tags: s.ServiceTags,
Address: s.ServiceAddress,
Port: s.ServicePort,
ServiceMeta: s.ServiceMeta,
EnableTagOverride: s.ServiceEnableTagOverride,
RaftIndex: RaftIndex{
CreateIndex: s.CreateIndex,
Expand All @@ -422,6 +429,7 @@ type NodeService struct {
Service string
Tags []string
Address string
ServiceMeta map[string]string
Port int
EnableTagOverride bool

Expand All @@ -438,6 +446,7 @@ func (s *NodeService) IsSame(other *NodeService) bool {
!reflect.DeepEqual(s.Tags, other.Tags) ||
s.Address != other.Address ||
s.Port != other.Port ||
!reflect.DeepEqual(s.ServiceMeta, other.ServiceMeta) ||
s.EnableTagOverride != other.EnableTagOverride {
return false
}
Expand All @@ -457,6 +466,7 @@ func (s *NodeService) ToServiceNode(node string) *ServiceNode {
ServiceTags: s.Tags,
ServiceAddress: s.Address,
ServicePort: s.Port,
ServiceMeta: s.ServiceMeta,
ServiceEnableTagOverride: s.EnableTagOverride,
RaftIndex: RaftIndex{
CreateIndex: s.CreateIndex,
Expand Down
42 changes: 33 additions & 9 deletions agent/structs/structs_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -133,11 +133,14 @@ func testServiceNode() *ServiceNode {
NodeMeta: map[string]string{
"tag": "value",
},
ServiceID: "service1",
ServiceName: "dogs",
ServiceTags: []string{"prod", "v1"},
ServiceAddress: "127.0.0.2",
ServicePort: 8080,
ServiceID: "service1",
ServiceName: "dogs",
ServiceTags: []string{"prod", "v1"},
ServiceAddress: "127.0.0.2",
ServicePort: 8080,
ServiceMeta: map[string]string{
"service": "metadata",
},
ServiceEnableTagOverride: true,
RaftIndex: RaftIndex{
CreateIndex: 1,
Expand Down Expand Up @@ -175,6 +178,17 @@ func TestStructs_ServiceNode_PartialClone(t *testing.T) {
if reflect.DeepEqual(sn, clone) {
t.Fatalf("clone wasn't independent of the original")
}

revert := make([]string, len(sn.ServiceTags)-1)
copy(revert, sn.ServiceTags[0:len(sn.ServiceTags)-1])
sn.ServiceTags = revert
if !reflect.DeepEqual(sn, clone) {
t.Fatalf("bad: %v VS %v", clone, sn)
}
sn.ServiceMeta["new_meta"] = "new_value"
if reflect.DeepEqual(sn, clone) {
t.Fatalf("clone wasn't independent of the original for ServiceMeta")
}
}

func TestStructs_ServiceNode_Conversions(t *testing.T) {
Expand All @@ -196,10 +210,14 @@ func TestStructs_ServiceNode_Conversions(t *testing.T) {

func TestStructs_NodeService_IsSame(t *testing.T) {
ns := &NodeService{
ID: "node1",
Service: "theservice",
Tags: []string{"foo", "bar"},
Address: "127.0.0.1",
ID: "node1",
Service: "theservice",
Tags: []string{"foo", "bar"},
Address: "127.0.0.1",
ServiceMeta: map[string]string{
"meta1": "value1",
"meta2": "value2",
},
Port: 1234,
EnableTagOverride: true,
}
Expand All @@ -214,6 +232,11 @@ func TestStructs_NodeService_IsSame(t *testing.T) {
Address: "127.0.0.1",
Port: 1234,
EnableTagOverride: true,
ServiceMeta: map[string]string{
// We don't care about order
"meta2": "value2",
"meta1": "value1",
},
RaftIndex: RaftIndex{
CreateIndex: 1,
ModifyIndex: 2,
Expand Down Expand Up @@ -245,6 +268,7 @@ func TestStructs_NodeService_IsSame(t *testing.T) {
check(func() { other.Tags = []string{"foo"} }, func() { other.Tags = []string{"foo", "bar"} })
check(func() { other.Address = "XXX" }, func() { other.Address = "127.0.0.1" })
check(func() { other.Port = 9999 }, func() { other.Port = 1234 })
check(func() { other.ServiceMeta["meta2"] = "wrongValue" }, func() { other.ServiceMeta["meta2"] = "value2" })
check(func() { other.EnableTagOverride = false }, func() { other.EnableTagOverride = true })
}

Expand Down
13 changes: 7 additions & 6 deletions api/agent.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,12 +60,13 @@ type MembersOpts struct {

// AgentServiceRegistration is used to register a new service
type AgentServiceRegistration struct {
ID string `json:",omitempty"`
Name string `json:",omitempty"`
Tags []string `json:",omitempty"`
Port int `json:",omitempty"`
Address string `json:",omitempty"`
EnableTagOverride bool `json:",omitempty"`
ID string `json:",omitempty"`
Name string `json:",omitempty"`
Tags []string `json:",omitempty"`
Port int `json:",omitempty"`
Address string `json:",omitempty"`
EnableTagOverride bool `json:",omitempty"`
ServiceMeta map[string]string `json:",omitempty"`
Check *AgentServiceCheck
Checks AgentServiceChecks
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to prevent abusing this feature to store arbitarily long metadata values, add some validation.

There is existing logic for node metadata here https://github.com/hashicorp/consul/blob/master/agent/structs/structs.go#L306. To keep things consistent we can use the same rules for service metadata as well. You should validate it in the agent and catalog registration endpoints.

Its fine to allow the reserved consul prefix for service metadata since we don't rely on this for any internal uses.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually used this exact function in agent/agent_endpoint.go:582 but I am adding another check in the catalog as well in next patch and a unit test for Catalog insertion.

}
Expand Down
1 change: 1 addition & 0 deletions api/catalog.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ type CatalogService struct {
ServiceName string
ServiceAddress string
ServiceTags []string
ServiceMeta map[string]string
ServicePort int
ServiceEnableTagOverride bool
CreateIndex uint64
Expand Down
11 changes: 10 additions & 1 deletion website/source/api/agent/service.html.md
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,9 @@ $ curl \
"Service": "redis",
"Tags": [],
"Address": "",
"ServiceMeta": {
"redis_version": "4.0"
},
"Port": 8000
}
}
Expand Down Expand Up @@ -96,14 +99,17 @@ The table below shows this endpoint's support for
provided, the agent's address is used as the address for the service during
DNS queries.

- `ServiceMeta` `(map<string|string>: nil)` - Specifies arbitrary KV metadata
linked to the service instance.

- `Port` `(int: 0)` - Specifies the port of the service.

- `Check` `(Check: nil)` - Specifies a check. Please see the
[check documentation](/api/agent/check.html) for more information about the
accepted fields. If you don't provide a name or id for the check then they
will be generated. To provide a custom id and/or name set the `CheckID`
and/or `Name` field.

- `Checks` `(array<Check>: nil`) - Specifies a list of checks. Please see the
[check documentation](/api/agent/check.html) for more information about the
accepted fields. If you don't provide a name or id for the check then they
Expand Down Expand Up @@ -147,6 +153,9 @@ The table below shows this endpoint's support for
],
"Address": "127.0.0.1",
"Port": 8000,
"ServiceMeta": {
"redis_version": "4.0"
},
"EnableTagOverride": false,
"Check": {
"DeregisterCriticalServiceAfter": "90m",
Expand Down
14 changes: 13 additions & 1 deletion website/source/api/catalog.html.md
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ The table below shows this endpoint's support for
- `Service` `(Service: nil)` - Specifies to register a service. If `ID` is not
provided, it will be defaulted to the value of the `Service.Service` property.
Only one service with a given `ID` may be present per node. The service
`Tags`, `Address`, and `Port` fields are all optional.
`Tags`, `Address`, `ServiceMeta` and `Port` fields are all optional.

- `Check` `(Check: nil)` - Specifies to register a check. The register API
manipulates the health check entry in the Catalog, but it does not setup the
Expand Down Expand Up @@ -105,6 +105,9 @@ and vice versa. A catalog entry can have either, neither, or both.
"v1"
],
"Address": "127.0.0.1",
"ServiceMeta": {
"redis_version": "4.0"
},
"Port": 8000
},
"Check": {
Expand Down Expand Up @@ -432,6 +435,9 @@ $ curl \
"ServiceID": "32a2a47f7992:nodea:5000",
"ServiceName": "foobar",
"ServicePort": 5000,
"ServiceMeta": {
"foobar_meta_value": "baz"
},
"ServiceTags": [
"tacos"
]
Expand Down Expand Up @@ -467,6 +473,8 @@ $ curl \

- `ServiceName` is the name of the service

- `ServiceMeta` is a list of user-defined metadata key/value pairs for the service

- `ServicePort` is the port number of the service

- `ServiceTags` is a list of tags for the service
Expand Down Expand Up @@ -529,6 +537,7 @@ $ curl \
"ID": "consul",
"Service": "consul",
"Tags": null,
"ServiceMeta": {},
"Port": 8300
},
"redis": {
Expand All @@ -537,6 +546,9 @@ $ curl \
"Tags": [
"v1"
],
"ServiceMeta": {
"redis_version": "4.0"
},
"Port": 8000
}
}
Expand Down
3 changes: 3 additions & 0 deletions website/source/api/health.html.md
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,9 @@ $ curl \
"Service": "redis",
"Tags": ["primary"],
"Address": "10.1.10.12",
"ServiceMeta": {
"redis_version": "4.0"
},
"Port": 8000
},
"Checks": [
Expand Down
4 changes: 4 additions & 0 deletions website/source/api/query.html.md
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,7 @@ The table below shows this endpoint's support for
"Near": "node1",
"OnlyPassing": false,
"Tags": ["primary", "!experimental"],
"ServiceMeta": {"redis_version": "4.0"},
"NodeMeta": {"instance_type": "m3.large"}
},
"DNS": {
Expand Down Expand Up @@ -313,6 +314,7 @@ $ curl \
},
"OnlyPassing": false,
"Tags": ["primary", "!experimental"],
"ServiceMeta": {"redis_version": "4.0"},
"NodeMeta": {"instance_type": "m3.large"}
},
"DNS": {
Expand Down Expand Up @@ -510,6 +512,7 @@ $ curl \
"ID": "redis",
"Service": "redis",
"Tags": null,
"ServiceMeta": {"redis_version": "4.0"},
"Port": 8000
},
"Checks": [
Expand Down Expand Up @@ -616,6 +619,7 @@ $ curl \
},
"OnlyPassing": true,
"Tags": ["primary"],
"ServiceMeta": { "mysql_version": "5.7.20" },
"NodeMeta": {"instance_type": "m3.large"}
}
}
Expand Down