Service check definitions ignore check id and name.

[andremarianiello]

If you have a question, please direct it to the
consul mailing list if it hasn't been
addressed in either the FAQ or in one
of the Consul Guides.

When filing a bug, please include the following:

consul version for both Client and Server

Client: 0.8.3
Server: N/A

consul info for both Client and Server

Client:

agent:
        check_monitors = 2
        check_ttls = 0
        checks = 5
        services = 2
build:
        prerelease =
        revision = ea2a82b
        version = 0.8.3
consul:
        known_servers = 1
        server = false
runtime:
        arch = amd64
        cpu_count = 4
        goroutines = 45
        max_procs = 4
        os = linux
        version = go1.8.1
serf_lan:
        encrypted = false
        event_queue = 0
        event_time = 4
        failed = 0
        health_score = 0
        intent_queue = 0
        left = 0
        member_time = 110
        members = 7
        query_queue = 0
        query_time = 1

Server: N/A

Operating system and Environment details

Description of the Issue (and unexpected/desired result)

Reproduction steps

Create a service definition file with a check definition with a custom name and id. Start the consul agent client, and use the /v1/agent/checks endpoint to verify that the id is overridden with 'service:' and the name is overridden with "Service '' check".

Log Fragments or Link to gist

N/A

[magiconair]

I'll have a look at this

[magiconair]

This doesn't look like a regression. I have the same behavior with 0.8.1

# start consul 
consul agent -server -dev

# service definition with custom check name and ID
$ cat service.json
{
  "ID": "redis1",
  "Name": "redis",
  "Address": "127.0.0.1",
  "Port": 8000,
  "Check": {
    "CheckID": "foo",
    "Name": "bar",
    "Script": "/usr/bin/true",
    "Interval": "10s"
  }
}

# register service in consul
curl -i -d @service.json -XPUT localhost:8500/v1/agent/service/register

# fetch check definition
$ curl -i localhost:8500/v1/agent/checks
HTTP/1.1 200 OK
Content-Type: application/json
Date: Mon, 15 May 2017 19:41:44 GMT
Content-Length: 359

{
    "service:redis1": {
        "Node": "hashibook.lan",
        "CheckID": "service:redis1", <-- "foo" ?
        "Name": "Service 'redis' check", <-- "bar" ?
        "Status": "critical",
        "Notes": "",
        "Output": "",
        "ServiceID": "redis1",
        "ServiceName": "redis",
        "ServiceTags": [],
        "CreateIndex": 0,
        "ModifyIndex": 0
    }
}

[magiconair]

I'm too new to know for sure whether there is a reason for this. I have a patch which still needs a test but there is at least one pitfall in that you could then register a check with the same id for different services. However, that might already be possible through the /v1/agent/check/register API.

[magiconair]

@slackpad I would need some guidance here.

[andremarianiello]

@magiconair I believe the check name and id are being ignored in this loop

consul/command/agent/agent.go

Line 1159 in e2c37b4

for i, chkType := range chkTypes {

[magiconair]

yes, that's correct.

[magiconair]

Please ignore the patch since this isn't done yet. Deleting the branch apparently does not remove the reference to the commit.

[magiconair]

After banging my head against this for a while I think this approach should be OK. The CheckDefinition structure used to embed the CheckType which is used throughout the API for defining and returning health checks. Both structures share some fields (Notes and Status) and with the addition of CheckID and Name they share two more. However, CheckType and CheckDefinition use CheckID vs ID which is something we should probably clean up.

Following the Go Proverb "A little copying is better than a little dependency." I've copied the shared fields from CheckType to CheckDefinition to decouple the two.

The website files still need updating.