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.

Sign up for GitHub

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

Jump to bottom

[BUGFIX] Added Service Meta support in configuration files #4047

Merged
merged 10 commits into from
Apr 25, 2018
Merged

[BUGFIX] Added Service Meta support in configuration files #4047

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
d2ab3de
[BUGFIX] Added Service Meta support in configuration files
pierresouchay Apr 18, 2018
2f5e675
Added unit tests for bad meta values
pierresouchay Apr 18, 2018
9bb1573
Added unit test on key length
pierresouchay Apr 18, 2018
06a1819
Use safer stringVal()
pierresouchay Apr 18, 2018
c152cb7
Added Missing Service Meta synchronization and field
pierresouchay Apr 21, 2018
eccc223
Fixed Meta name for JSON + Added unit tests for HCL/JSON
pierresouchay Apr 24, 2018
4c1ddbd
Added documentation for meta
pierresouchay Apr 24, 2018
7e0d6f5
Improved documentation
pierresouchay Apr 24, 2018
208566d
Changed a bit doc
pierresouchay Apr 24, 2018
303997f
Improved unit test (example close to actual value)
pierresouchay Apr 24, 2018
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
13 changes: 11 additions & 2 deletions agent/agent_endpoint.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,9 +131,18 @@ func (s *HTTPServer) AgentServices(resp http.ResponseWriter, req *http.Request)

// Use empty list instead of nil
for id, s := range services {
if s.Tags == nil {
if s.Tags == nil || s.Meta == nil {
clone := *s
clone.Tags = make([]string, 0)
if s.Tags == nil {
clone.Tags = make([]string, 0)
} else {
clone.Tags = s.Tags
}
if s.Meta == nil {
clone.Meta = make(map[string]string)
} else {
clone.Meta = s.Meta
}
services[id] = &clone
}
}
Expand Down
8 changes: 6 additions & 2 deletions agent/agent_endpoint_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1204,6 +1204,7 @@ func TestAgent_RegisterService(t *testing.T) {

args := &structs.ServiceDefinition{
Name: "test",
Meta: map[string]string{"hello": "world"},
Tags: []string{"master"},
Port: 8000,
Check: structs.CheckType{
Expand Down Expand Up @@ -1232,6 +1233,9 @@ func TestAgent_RegisterService(t *testing.T) {
if _, ok := a.State.Services()["test"]; !ok {
t.Fatalf("missing test service")
}
if val := a.State.Service("test").Meta["hello"]; val != "world" {
t.Fatalf("Missing meta: %v", a.State.Service("test").Meta)
}

// Ensure we have a check mapping
checks := a.State.Checks()
Expand All @@ -1254,7 +1258,7 @@ func TestAgent_RegisterService_TranslateKeys(t *testing.T) {
a := NewTestAgent(t.Name(), "")
defer a.Shutdown()

json := `{"name":"test", "port":8000, "enable_tag_override": true}`
json := `{"name":"test", "port":8000, "enable_tag_override": true, "meta": {"some": "meta"}}`
req, _ := http.NewRequest("PUT", "/v1/agent/service/register", strings.NewReader(json))

obj, err := a.srv.AgentRegisterService(nil, req)
Expand All @@ -1264,10 +1268,10 @@ func TestAgent_RegisterService_TranslateKeys(t *testing.T) {
if obj != nil {
t.Fatalf("bad: %v", obj)
}

svc := &structs.NodeService{
ID: "test",
Service: "test",
Meta: map[string]string{"some": "meta"},
Port: 8000,
EnableTagOverride: true,
}
Expand Down
7 changes: 7 additions & 0 deletions agent/config/builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -997,11 +997,18 @@ func (b *Builder) serviceVal(v *ServiceDefinition) *structs.ServiceDefinition {
checks = append(checks, b.checkVal(v.Check).CheckType())
}

meta := make(map[string]string)
if err := structs.ValidateMetadata(v.Meta, false); err != nil {
b.err = multierror.Append(fmt.Errorf("invalid meta for service %s: %v", b.stringVal(v.Name), err))
} else {
meta = v.Meta
}
return &structs.ServiceDefinition{
ID: b.stringVal(v.ID),
Name: b.stringVal(v.Name),
Tags: v.Tags,
Address: b.stringVal(v.Address),
Meta: meta,
Port: b.intVal(v.Port),
Token: b.stringVal(v.Token),
EnableTagOverride: b.boolVal(v.EnableTagOverride),
Expand Down
1 change: 1 addition & 0 deletions agent/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,7 @@ type ServiceDefinition struct {
Name *string `json:"name,omitempty" hcl:"name" mapstructure:"name"`
Tags []string `json:"tags,omitempty" hcl:"tags" mapstructure:"tags"`
Address *string `json:"address,omitempty" hcl:"address" mapstructure:"address"`
Meta map[string]string `json:"node_meta,omitempty" hcl:"meta" mapstructure:"meta"`
Copy link
Member

Choose a reason for hiding this comment

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

JSON tag node_meta seems like copy paste error here? should just be meta like the others.

Also interesting that this wasn't caught by a test case - on the surface it seems like your tests would exercise that but in fact even when config looks like JSON we parse it into an interface{} and then use mapstructure. So the JSON tags here may never actually be used in Consul, and I don't think we explicitly test native JSON parsing, but let's fix this so it doesn't bite someone one day!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@banks Yes, Added missing Tests case in next patch in agent/config/runtime_test.go for HCL/JSON parsing

Port *int `json:"port,omitempty" hcl:"port" mapstructure:"port"`
Check *CheckDefinition `json:"check,omitempty" hcl:"check" mapstructure:"check"`
Checks []CheckDefinition `json:"checks,omitempty" hcl:"checks" mapstructure:"checks"`
Expand Down
47 changes: 44 additions & 3 deletions agent/config/runtime_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ func TestConfigFlagsAndEdgecases(t *testing.T) {
dataDir := testutil.TempDir(t, "consul")
defer os.RemoveAll(dataDir)

metaVal := make(map[string]string)
metaVal["my"] = "value"
tests := []configTest{
// ------------------------------------------------------------
// cmd line flags
Expand Down Expand Up @@ -1923,20 +1925,59 @@ func TestConfigFlagsAndEdgecases(t *testing.T) {
},
json: []string{
`{ "service": { "name": "a", "port": 80 } }`,
`{ "service": { "name": "b", "port": 90 } }`,
`{ "service": { "name": "b", "port": 90, "meta": {"my": "value"} } }`,
},
hcl: []string{
`service = { name = "a" port = 80 }`,
`service = { name = "b" port = 90 }`,
`service = { name = "b" port = 90 meta={my="value"}}`,
},
patch: func(rt *RuntimeConfig) {
rt.Services = []*structs.ServiceDefinition{
&structs.ServiceDefinition{Name: "a", Port: 80},
&structs.ServiceDefinition{Name: "b", Port: 90},
&structs.ServiceDefinition{Name: "b", Port: 90, Meta: metaVal},
}
rt.DataDir = dataDir
},
},
{
desc: "service with wrong meta: too long key",
args: []string{
`-data-dir=` + dataDir,
},
json: []string{
`{ "service": { "name": "a", "port": 80, "meta": { "` + randomString(520) + `": "metaValue" } } }`,
},
hcl: []string{
`service = { name = "a" port = 80, meta={` + randomString(520) + `="metaValue"} }`,
},
err: `Key is too long`,
},
{
desc: "service with wrong meta: too long value",
args: []string{
`-data-dir=` + dataDir,
},
json: []string{
`{ "service": { "name": "a", "port": 80, "meta": { "a": "` + randomString(520) + `" } } }`,
},
hcl: []string{
`service = { name = "a" port = 80, meta={a="` + randomString(520) + `"} }`,
},
err: `Value is too long`,
},
{
desc: "service with wrong meta: too many meta",
args: []string{
`-data-dir=` + dataDir,
},
json: []string{
`{ "service": { "name": "a", "port": 80, "meta": { ` + metaPairs(70, "json") + `} } }`,
},
hcl: []string{
`service = { name = "a" port = 80 meta={` + metaPairs(70, "hcl") + `} }`,
},
err: `invalid meta for service a: Node metadata cannot contain more than 64 key`,
},
{
desc: "translated keys",
args: []string{
Expand Down
1 change: 1 addition & 0 deletions agent/consul/acl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -678,6 +678,7 @@ func vetRegisterWithACL(rule acl.ACL, subj *structs.RegisterRequest,
ID: subj.Service.ID,
Service: subj.Service.Service,
Tags: subj.Service.Tags,
Meta: subj.Service.Meta,
Address: subj.Service.Address,
Port: subj.Service.Port,
EnableTagOverride: subj.Service.EnableTagOverride,
Expand Down
1 change: 1 addition & 0 deletions api/agent.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ type AgentService struct {
ID string
Service string
Tags []string
Meta map[string]string
Port int
Address string
EnableTagOverride bool
Expand Down
5 changes: 4 additions & 1 deletion api/agent_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ func TestAPI_AgentReload(t *testing.T) {
agent := c.Agent()

// Update the config file with a service definition
config := `{"service":{"name":"redis", "port":1234}}`
config := `{"service":{"name":"redis", "port":1234, "Meta": {"some": "meta"}}}`
err = ioutil.WriteFile(configFile.Name(), []byte(config), 0644)
if err != nil {
t.Fatalf("err: %v", err)
Expand All @@ -95,6 +95,9 @@ func TestAPI_AgentReload(t *testing.T) {
if service.Port != 1234 {
t.Fatalf("bad: %v", service.Port)
}
if service.Meta["some"] != "meta" {
t.Fatalf("Missing metadata some:=meta in %v", service)
}
}

func TestAPI_AgentMembersOpts(t *testing.T) {
Expand Down