checkid vs id parameter issue

[hanshasselberg]

Apparently there is some confusion around registering a check. This issue collects the data, issues and PRs around that so that we can decide what to do.

• Fix documentation registering service checks #7393
• Make /register/check docs match actuality #7352
• Register check documentation is misleading #6635
• Update register check docs (closes #6635) #7364
• ID vs CheckID for Healthcheck in consul version 1.7.2 #7733

Maybe related:

• service script health checks not working #6923

Possible root causes:

• Add Namespace support to the API module and the CLI commands #6874

Action items:

• fix the bug in consul services register documented in the comment below
• consider making the upgrade from pre-1.7.x smoother by restoring support for both field names (CheckID and ID). This would also make the request/response pairs use the same name, where as currently they are different.

[dnephin]

From my reading #6874 does not appear to be the cause. I did a bit of digging and here is what I found.

Both the HTTP API endpoint and config file expect ID, which matches our docs.

consul/agent/structs/check_definition.go

Lines 12 to 13 in 5c1858e

	type CheckDefinition struct {
	ID types.CheckID

consul/agent/config/config.go

Lines 415 to 416 in 5c1858e

	type CheckDefinition struct {
	ID *string `json:"id,omitempty" hcl:"id" mapstructure:"id"`

The api client for "register check" uses a struct which has both ID and CheckID (CheckID comes from AgentServiceCheck)

consul/api/agent.go

Lines 179 to 185 in 5c1858e

	// AgentCheckRegistration is used to register a new check
	type AgentCheckRegistration struct {
	ID string `json:",omitempty"`
	Name string `json:",omitempty"`
	Notes string `json:",omitempty"`
	ServiceID string `json:",omitempty"`
	AgentServiceCheck

In this case the ID field is correct.

The part that may be broken is the service config, which uses CheckID

consul/api/agent.go

Line 191 in 5c1858e

CheckID string `json:",omitempty"`

This struct is translated into an api.AgentServiceRegistration using mapstructure, but from what I can see there is nothing telling it to map CheckID -> ID.

I suspect this is breaking adding checks from consul services register, but I haven't tested that yet.

I also haven't found the message which reports that ID is deprecated.

This seems to match the conclusion in #7395

So the Check API is correct, but registering checks a check with an ID as part of a service is broken.

[dnephin]

I suspect part of the confusion here is that the CheckDefinition (which uses ID) produces a CheckType (which uses CheckID). We should probably be consistent. We may need to copy the field for API responses to prevent breaking API integrations.

[dnephin]

#6923 is an issue with the same cause on a different field. Converting from one type to another when the fields do not match properly.

[dreusel]

In the mean time, even though G-Research/consuldotnet#185 states that ID is obsolete and CheckID should be used, the documentation here: https://developer.hashicorp.com/consul/api-docs/agent/check#register-check still tells us to use the ID field (and v1.16.1 returns an error if ID is actually used, which seems to be a regression).

[dreusel]

@jsosulska this issue doesn't seem to be getting much attention anymore but in the mean time the published documentation is pain wrong and at least that should be pretty easy to fix?