search

hashicorp / consul

Consul is a distributed, highly available, and data center aware solution to connect and configure applications across dynamic, distributed infrastructure.
https://www.consul.io
Other
28.31k stars 4.42k forks source link

checkid vs id parameter issue #7566

Open hanshasselberg opened 4 years ago

hanshasselberg commented 4 years ago

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.

Maybe related:

Possible root causes:

Action items:

dnephin commented 4 years ago

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.

https://github.com/hashicorp/consul/blob/5c1858e70cfd0d586fd71c04f05847a27734ad47/agent/structs/check_definition.go#L12-L13 https://github.com/hashicorp/consul/blob/5c1858e70cfd0d586fd71c04f05847a27734ad47/agent/config/config.go#L415-L416

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

https://github.com/hashicorp/consul/blob/5c1858e70cfd0d586fd71c04f05847a27734ad47/api/agent.go#L179-L185

In this case the ID field is correct.

The part that may be broken is the service config, which uses CheckID https://github.com/hashicorp/consul/blob/5c1858e70cfd0d586fd71c04f05847a27734ad47/api/agent.go#L191

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 commented 4 years ago

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 commented 4 years ago

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 commented 1 year ago

In the mean time, even though https://github.com/G-Research/consuldotnet/pull/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 commented 1 year ago

@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?