diff --git a/agent/agent_endpoint.go b/agent/agent_endpoint.go index 8fd1d022f..f9358477c 100644 --- a/agent/agent_endpoint.go +++ b/agent/agent_endpoint.go @@ -8,6 +8,7 @@ import ( "strings" "github.com/hashicorp/consul/acl" + "github.com/hashicorp/consul/agent/config" "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/api" "github.com/hashicorp/consul/ipaddr" @@ -524,6 +525,12 @@ func (s *HTTPServer) AgentRegisterService(resp http.ResponseWriter, req *http.Re return nil } + // see https://github.com/hashicorp/consul/pull/3557 why we need this + // and why we should get rid of it. + config.TranslateKeys(rawMap, map[string]string{ + "enable_tag_override": "EnableTagOverride", + }) + for k, v := range rawMap { switch strings.ToLower(k) { case "check": diff --git a/agent/agent_endpoint_test.go b/agent/agent_endpoint_test.go index 5484cca82..0ebfdc204 100644 --- a/agent/agent_endpoint_test.go +++ b/agent/agent_endpoint_test.go @@ -21,6 +21,7 @@ import ( "github.com/hashicorp/consul/testutil/retry" "github.com/hashicorp/consul/types" "github.com/hashicorp/serf/serf" + "github.com/pascaldekloe/goe/verify" ) func makeReadOnlyAgentACL(t *testing.T, srv *HTTPServer) string { @@ -1157,6 +1158,34 @@ func TestAgent_RegisterService(t *testing.T) { } } +func TestAgent_RegisterService_TranslateKeys(t *testing.T) { + t.Parallel() + a := NewTestAgent(t.Name(), "") + defer a.Shutdown() + + json := `{"name":"test", "port":8000, "enable_tag_override": true}` + req, _ := http.NewRequest("PUT", "/v1/agent/service/register", strings.NewReader(json)) + + obj, err := a.srv.AgentRegisterService(nil, req) + if err != nil { + t.Fatalf("err: %v", err) + } + if obj != nil { + t.Fatalf("bad: %v", obj) + } + + svc := &structs.NodeService{ + ID: "test", + Service: "test", + Port: 8000, + EnableTagOverride: true, + } + + if got, want := a.state.Services()["test"], svc; !verify.Values(t, "", got, want) { + t.Fail() + } +} + func TestAgent_RegisterService_ACLDeny(t *testing.T) { t.Parallel() a := NewTestAgent(t.Name(), TestACLConfig()) diff --git a/agent/config.go b/agent/config.go index 3ba1437fe..84052986b 100644 --- a/agent/config.go +++ b/agent/config.go @@ -5,6 +5,8 @@ import ( "fmt" "strings" "time" + + "github.com/hashicorp/consul/agent/config" ) var errInvalidHeaderFormat = errors.New("agent: invalid format of 'header' field") @@ -15,6 +17,15 @@ func FixupCheckType(raw interface{}) error { return nil } + // see https://github.com/hashicorp/consul/pull/3557 why we need this + // and why we should get rid of it. + config.TranslateKeys(rawMap, map[string]string{ + "deregister_critical_service_after": "DeregisterCriticalServiceAfter", + "docker_container_id": "DockerContainerID", + "tls_skip_verify": "TLSSkipVerify", + "service_id": "ServiceID", + }) + parseDuration := func(v interface{}) (time.Duration, error) { if v == nil { return 0, nil @@ -56,13 +67,6 @@ func FixupCheckType(raw interface{}) error { return m, nil } - replace := func(oldKey, newKey string, val interface{}) { - rawMap[newKey] = val - if oldKey != newKey { - delete(rawMap, oldKey) - } - } - for k, v := range rawMap { switch strings.ToLower(k) { case "header": @@ -72,28 +76,12 @@ func FixupCheckType(raw interface{}) error { } rawMap[k] = h - case "ttl", "interval", "timeout": + case "ttl", "interval", "timeout", "deregistercriticalserviceafter": d, err := parseDuration(v) if err != nil { return fmt.Errorf("invalid %q: %v", k, err) } rawMap[k] = d - - case "deregister_critical_service_after", "deregistercriticalserviceafter": - d, err := parseDuration(v) - if err != nil { - return fmt.Errorf("invalid %q: %v", k, err) - } - replace(k, "DeregisterCriticalServiceAfter", d) - - case "docker_container_id": - replace(k, "DockerContainerID", v) - - case "service_id": - replace(k, "ServiceID", v) - - case "tls_skip_verify": - replace(k, "TLSSkipVerify", v) } } return nil diff --git a/agent/config/builder.go b/agent/config/builder.go index e665b53b5..414fe778f 100644 --- a/agent/config/builder.go +++ b/agent/config/builder.go @@ -869,9 +869,6 @@ func (b *Builder) checkVal(v *CheckDefinition) *structs.CheckDefinition { } id := types.CheckID(b.stringVal(v.ID)) - if v.CheckID != nil { - id = types.CheckID(b.stringVal(v.CheckID)) - } return &structs.CheckDefinition{ ID: id, diff --git a/agent/config/config.go b/agent/config/config.go index 1f18e0931..ab98118ec 100644 --- a/agent/config/config.go +++ b/agent/config/config.go @@ -86,15 +86,29 @@ func Parse(data string, format string) (c Config, err error) { "watches", }) - // toJSON := func(v interface{}) string { - // b, err := json.MarshalIndent(v, "", " ") - // if err != nil { - // panic(err) - // } - // return string(b) - // } - // fmt.Println("raw:", toJSON(raw)) - // fmt.Println("patched:", toJSON(m)) + // There is a difference of representation of some fields depending on + // where they are used. The HTTP API uses CamelCase whereas the config + // files use snake_case and between the two there is no automatic mapping. + // While the JSON and HCL parsers match keys without case (both `id` and + // `ID` are mapped to an ID field) the same thing does not happen between + // CamelCase and snake_case. Since changing either format would break + // existing setups we have to support both and slowly transition to one of + // the formats. Also, there is at least one case where we use the "wrong" + // key and want to map that to the new key to support deprecation + // (`check.id` vs `service.check.CheckID`) See [GH-3179]. TranslateKeys + // maps potentially CamelCased values to the snake_case that is used in the + // config file parser. If both the CamelCase and snake_case values are set, + // the snake_case value is used and the other value is discarded. + TranslateKeys(m, map[string]string{ + "check_id": "id", + "checkid": "id", + "deregistercriticalserviceafter": "deregister_critical_service_after", + "dockercontainerid": "docker_container_id", + "enabletagoverride": "enable_tag_override", + "scriptargs": "args", + "serviceid": "service_id", + "tlsskipverify": "tls_skip_verify", + }) var md mapstructure.Metadata d, err := mapstructure.NewDecoder(&mapstructure.DecoderConfig{ @@ -305,7 +319,6 @@ type ServiceDefinition struct { type CheckDefinition struct { ID *string `json:"id,omitempty" hcl:"id" mapstructure:"id"` - CheckID *string `json:"check_id,omitempty" hcl:"check_id" mapstructure:"check_id"` Name *string `json:"name,omitempty" hcl:"name" mapstructure:"name"` Notes *string `json:"notes,omitempty" hcl:"notes" mapstructure:"notes"` ServiceID *string `json:"service_id,omitempty" hcl:"service_id" mapstructure:"service_id"` diff --git a/agent/config/runtime_test.go b/agent/config/runtime_test.go index c5699f052..77f50ddf7 100644 --- a/agent/config/runtime_test.go +++ b/agent/config/runtime_test.go @@ -1773,6 +1773,61 @@ func TestConfigFlagsAndEdgecases(t *testing.T) { rt.DataDir = dataDir }, }, + { + desc: "translated keys", + flags: []string{ + `-data-dir=` + dataDir, + }, + json: []string{ + `{ + "service": { + "name": "a", + "port": 80, + "EnableTagOverride": true, + "check": { + "CheckID": "x", + "name": "y", + "DockerContainerID": "z", + "DeregisterCriticalServiceAfter": "10s", + "ScriptArgs": ["a", "b"] + } + } + }`, + }, + hcl: []string{ + `service = { + name = "a" + port = 80 + EnableTagOverride = true + check = { + CheckID = "x" + name = "y" + DockerContainerID = "z" + DeregisterCriticalServiceAfter = "10s" + ScriptArgs = ["a", "b"] + } + }`, + }, + patch: func(rt *RuntimeConfig) { + rt.Services = []*structs.ServiceDefinition{ + &structs.ServiceDefinition{ + Name: "a", + Port: 80, + EnableTagOverride: true, + Checks: []*structs.CheckType{ + &structs.CheckType{ + CheckID: types.CheckID("x"), + Name: "y", + DockerContainerID: "z", + DeregisterCriticalServiceAfter: 10 * time.Second, + ScriptArgs: []string{"a", "b"}, + }, + }, + }, + } + rt.DataDir = dataDir + }, + }, } testConfig(t, tests, dataDir) diff --git a/agent/config/translate.go b/agent/config/translate.go new file mode 100644 index 000000000..201df9d21 --- /dev/null +++ b/agent/config/translate.go @@ -0,0 +1,54 @@ +package config + +import "strings" + +// TranslateKeys recursively translates all keys from m in-place to their +// canonical form as defined in dict which maps an alias name to the canonical +// name. If m already has a value for the canonical name then that one is used +// and the value for the alias name is discarded. Alias names are matched +// case-insensitive. +// +// Example: +// +// m = TranslateKeys(m, map[string]string{"CamelCase": "snake_case"}) +// +func TranslateKeys(v map[string]interface{}, dict map[string]string) { + ck(v, dict) +} + +func ck(v interface{}, dict map[string]string) interface{} { + switch x := v.(type) { + case map[string]interface{}: + for k, v := range x { + canonKey := dict[strings.ToLower(k)] + + // no canonical key? -> use this key + if canonKey == "" { + x[k] = ck(v, dict) + continue + } + + // delete the alias + delete(x, k) + + // if there is a value for the canonical key then keep it + if _, ok := x[canonKey]; ok { + continue + } + + // otherwise translate to the canonical key + x[canonKey] = ck(v, dict) + } + return x + + case []interface{}: + var a []interface{} + for _, xv := range x { + a = append(a, ck(xv, dict)) + } + return a + + default: + return v + } +} diff --git a/agent/config/translate_test.go b/agent/config/translate_test.go new file mode 100644 index 000000000..aaa5dad19 --- /dev/null +++ b/agent/config/translate_test.go @@ -0,0 +1,83 @@ +package config + +import ( + "encoding/json" + "testing" + + "github.com/pascaldekloe/goe/verify" +) + +func TestTranslateKeys(t *testing.T) { + fromJSON := func(s string) map[string]interface{} { + var m map[string]interface{} + if err := json.Unmarshal([]byte(s), &m); err != nil { + t.Fatal(err) + } + return m + } + + tests := []struct { + desc string + in map[string]interface{} + out map[string]interface{} + dict map[string]string + }{ + { + desc: "x->y", + in: map[string]interface{}{"a": "aa", "x": "xx"}, + out: map[string]interface{}{"a": "aa", "y": "xx"}, + dict: map[string]string{"x": "y"}, + }, + { + desc: "discard x", + in: map[string]interface{}{"a": "aa", "x": "xx", "y": "yy"}, + out: map[string]interface{}{"a": "aa", "y": "yy"}, + dict: map[string]string{"x": "y"}, + }, + { + desc: "b.x->b.y", + in: map[string]interface{}{"a": "aa", "b": map[string]interface{}{"x": "xx"}}, + out: map[string]interface{}{"a": "aa", "b": map[string]interface{}{"y": "xx"}}, + dict: map[string]string{"x": "y"}, + }, + { + desc: "json: x->y", + in: fromJSON(`{"a":"aa","x":"xx"}`), + out: fromJSON(`{"a":"aa","y":"xx"}`), + dict: map[string]string{"x": "y"}, + }, + { + desc: "json: X->y", + in: fromJSON(`{"a":"aa","X":"xx"}`), + out: fromJSON(`{"a":"aa","y":"xx"}`), + dict: map[string]string{"x": "y"}, + }, + { + desc: "json: discard x", + in: fromJSON(`{"a":"aa","x":"xx","y":"yy"}`), + out: fromJSON(`{"a":"aa","y":"yy"}`), + dict: map[string]string{"x": "y"}, + }, + { + desc: "json: b.x->b.y", + in: fromJSON(`{"a":"aa","b":{"x":"xx"}}`), + out: fromJSON(`{"a":"aa","b":{"y":"xx"}}`), + dict: map[string]string{"x": "y"}, + }, + { + desc: "json: b[0].x->b[0].y", + in: fromJSON(`{"a":"aa","b":[{"x":"xx"}]}`), + out: fromJSON(`{"a":"aa","b":[{"y":"xx"}]}`), + dict: map[string]string{"x": "y"}, + }, + } + + for _, tt := range tests { + t.Run(tt.desc, func(t *testing.T) { + TranslateKeys(tt.in, tt.dict) + if got, want := tt.in, tt.out; !verify.Values(t, "", got, want) { + t.Fail() + } + }) + } +} diff --git a/website/source/docs/agent/checks.html.md b/website/source/docs/agent/checks.html.md index 886af999e..b66445f4a 100644 --- a/website/source/docs/agent/checks.html.md +++ b/website/source/docs/agent/checks.html.md @@ -163,10 +163,17 @@ A Docker check: } ``` -Each type of definition must include a `name` and may optionally -provide an `id` and `notes` field. The `id` is set to the `name` if not -provided. It is required that all checks have a unique ID per node: if names -might conflict, unique IDs should be provided. +Each type of definition must include a `name` and may optionally provide an +`id` and `notes` field. The `id` must be unique per _agent_ otherwise only the +last defined check with that `id` will be registered. If the `id` is not set +and the check is embedded within a service definition a unique check id is +generated. Otherwise, `id` will be set to `name`. If names might conflict, +unique IDs should be provided. + +-> **Note:** Consul 0.9.3 and before require the optional check ID for a check + that is embedded in a service definition to be configured via the `CheckID` + field. Consul 1.0 accepts both `id` and `CheckID` but the latter is + deprecated and will be removed in Consul 1.1. The `notes` field is opaque to Consul but can be used to provide a human-readable description of the current state of the check. With a script check, the field is diff --git a/website/source/docs/agent/services.html.md b/website/source/docs/agent/services.html.md index 8b14f3561..18d23b339 100644 --- a/website/source/docs/agent/services.html.md +++ b/website/source/docs/agent/services.html.md @@ -26,7 +26,7 @@ A service definition that is a script looks like: "tags": ["primary"], "address": "", "port": 8000, - "enableTagOverride": false, + "enable_tag_override": false, "checks": [ { "script": "/usr/local/bin/check_redis.py", @@ -38,11 +38,15 @@ A service definition that is a script looks like: ``` A service definition must include a `name` and may optionally provide an -`id`, `tags`, `address`, `port`, `check`, and `enableTagOverride`. The +`id`, `tags`, `address`, `port`, `check`, and `enable_tag_override`. The `id` is set to the `name` if not provided. It is required that all services have a unique ID per node, so if names might conflict then unique IDs should be provided. +For Consul 0.9.3 and earlier you need to use `enableTagOverride`. Consul 1.0 +supports both `enable_tag_override` and `enableTagOverride` but the latter is +deprecated and will be removed in Consul 1.1. + The `tags` property is a list of values that are opaque to Consul but can be used to distinguish between `primary` or `secondary` nodes, different versions, or any other service level labels. @@ -75,32 +79,41 @@ from `1`. -> **Note:** There is more information about [checks here](/docs/agent/checks.html). -The `enableTagOverride` can optionally be specified to disable the -anti-entropy feature for this service. If `enableTagOverride` is set to +-> **Note:** Consul 0.9.3 and before require the optional check ID for a check + that is embedded in a service definition to be configured via the `CheckID` + field. Consul 1.0 accepts both `id` and `CheckID` but the latter is + deprecated and will be removed in Consul 1.1. + +The `enable_tag_override` can optionally be specified to disable the +anti-entropy feature for this service. If `enable_tag_override` is set to `TRUE` then external agents can update this service in the [catalog](/api/catalog.html) and modify the tags. Subsequent local sync operations by this agent will ignore the updated tags. For example, if an external agent modified both the tags and the port for -this service and `enableTagOverride` was set to `TRUE` then after the next +this service and `enable_tag_override` was set to `TRUE` then after the next sync cycle the service's port would revert to the original value but the tags would maintain the updated value. As a counter example: If an external agent modified both the tags and port for this service and -`enableTagOverride` was set to `FALSE` then after the next sync cycle the +`enable_tag_override` was set to `FALSE` then after the next sync cycle the service's port *and* the tags would revert to the original value and all modifications would be lost. It's important to note that this applies only to the locally registered service. If you have multiple nodes all registering the same service -their `enableTagOverride` configuration and all other service +their `enable_tag_override` configuration and all other service configuration items are independent of one another. Updating the tags for the service registered on one node is independent of the same -service (by name) registered on another node. If `enableTagOverride` is +service (by name) registered on another node. If `enable_tag_override` is not specified the default value is false. See [anti-entropy syncs](/docs/internals/anti-entropy.html) for more info. +For Consul 0.9.3 and earlier you need to use `enableTagOverride`. Consul 1.0 +supports both `enable_tag_override` and `enableTagOverride` but the latter is +deprecated and will be removed in Consul 1.1. + To configure a service, either provide it as a `-config-file` option to the agent or place it inside the `-config-dir` of the agent. The file -must end in the `.json` extension to be loaded by Consul. Check +must end in the `.json` or `.hcl` extension to be loaded by Consul. Check definitions can be updated by sending a `SIGHUP` to the agent. Alternatively, the service can be registered dynamically using the [HTTP API](/api/index.html).