Ecwid / consul-api

Java client for Consul HTTP API
Apache License 2.0
416 stars 177 forks source link

Consul 1.0.0 compatibility #130

Closed lazystone closed 6 years ago

lazystone commented 6 years ago

Consul 1.0.0 has some breaking API changes: https://www.consul.io/docs/upgrade-specific.html#consul-1-0

One of them is that com.ecwid.consul.v1.agent.model.Self.Config must be broken down into two classes at least: Config and DebugConfig

alexmsu75 commented 6 years ago

This is related to advertise_addr, it looks like it is now array of strings

lazystone commented 6 years ago

I wonder though - is it possible to make it backwards compatible somehow? Hashicorp definitely did it wrong - now we'll need to upgrade Consul together with application stack. At once.

What they had to do, is to introduce new API version like '/v2' and support both 'v1' and 'v2' for some time, so people could migrate from old one to the new one. Otherwise - why to use API versioning at all?..

lazystone commented 6 years ago

Or might be IT IS backwards compatible:

Many endpoints in the HTTP API that previously took any HTTP verb now check for specific HTTP verbs and enforce them.

So new API version should work with old Consul.

lazystone commented 6 years ago

@vgv Could look into this one?

slackpad commented 6 years ago

Hi all sorry about this breakage - the /v1/agent/self endpoint had some bits that were pulled from internal data structures and were changing from release to release. We tried to audit for existing users and keep their fields in Config (and commit to just those fields as a stable API) but didn't see this use of them.

You probably don't want to start parsing DebugConfig as that's not going to be stable, only the Config portion will be. See https://www.consul.io/api/agent.html#read-configuration.

lazystone commented 6 years ago

@slackpad now I know whom to blame :) @vgv I still suggest to not change major API version to keep it aligned with Consul releases.

preetapan commented 6 years ago

Beyond the above noted issues with agent/self, looks like this library didn't use the right HTTP verbs for agent check and service deregistration (should be PUT not GET).

https://github.com/Ecwid/consul-api/blob/master/src/main/java/com/ecwid/consul/v1/agent/AgentConsulClient.java#L174

https://github.com/Ecwid/consul-api/blob/master/src/main/java/com/ecwid/consul/v1/agent/AgentConsulClient.java#L241

There are likely more - you will need to compare against the table with endpoints affected mentioned in our CHANGELOG

lazystone commented 6 years ago

@preetapan I fixed some verbs in #131 , would be nice if you could double-check it :)

preetapan commented 6 years ago

Checked AgentConsulClient.java and the changes look good there. Hopefully, you have tests you can run against Consul 1.0 for all client methods if there's anything else you missed.

lazystone commented 6 years ago

I guess it's up to @vgv to decide now.

slackpad commented 6 years ago

Hashicorp definitely did it wrong - now we'll need to upgrade Consul together with application stack. At once. What they had to do, is to introduce new API version like '/v2' and support both 'v1' and 'v2' for some time, so people could migrate from old one to the new one. Otherwise - why to use API versioning at all?..

We will definitely do that going forward. The /v1/agent/self endpoint had a huge internal structure exposed that we weren't going to be able to support in a forward way so we make the choice to break it for the 1.0 so we could get it into a supportable state. Sorry about this though!

vgv commented 6 years ago

Fix it, thank you @lazystone for fixes!

lazystone commented 6 years ago

@vgv тебе спасибо :)