vert-x3 / vertx-consul-client

Vert.x Consul Client
Apache License 2.0
34 stars 23 forks source link

Check "ID" field is incorrect and fails validation with Consul 1.7 #74

Closed ncouse closed 4 years ago

ncouse commented 4 years ago

In Consul 1.7 they have introduced strict validation of JSON payloads.

Any requests containing invalid payload will return a 400 error. Previously, the invalid field would have been ignored.

Consul 1.7 came out this week, automated tests against the latest Docker image started failing. The issue is related to this validation.

I narrowed it down to registering a service using a Check.

ServiceOptions serviceOptions = new ServiceOptions()....
CheckOptions checkOptions = new CheckOptions().....

serviceOptions.setCheckOptions(checkOptions);
client.registerService(serviceOptions, result -> {...});

This fails consistently with Consul 1.7 whenever the CheckOptions are set, no matter what options I use.

Looking into it further, it looks like the code in ConsulClientImpl.checkOpts:

    JsonObject json = new JsonObject()
      .put("ID", checkOptions.getId())

ID is not valid, and it should be CheckID, if you consult with sample payload here: https://www.consul.io/api/catalog.html#sample-payload

I was testing this with 3.7.0, but code is still invalid on master.

I quickly recompiled with the change above (Change ID to CheckID), and my tests are now passing with Consul 1.7, so this appears to be a valid fix.

According to https://www.consul.io/api/catalog.html#check, the ID can be omitted and the name will be used as the ID. I guess this is what happened previously, as the wrong attribute was being used to set the CheckID field.

ncouse commented 4 years ago

Consul note on stricter JSON decoding: https://www.consul.io/docs/upgrade-specific.html#stricter-json-decoding

ruslansennov commented 4 years ago

@ncouse Thanks for the report. Could you check the current master version

ncouse commented 4 years ago

I have not tested on master version, but the code is still using the incorrect field name in master, so will fail in a similar manner.

I’ll see if I can run a small test against master later.

ruslansennov commented 4 years ago

Field ID is still used in /agent/check/register endpoint (see https://www.consul.io/api/agent/check.html#register-check)

ncouse commented 4 years ago

This bug concerns the case where the check is part of the registration of your service.

The links in description above show this must be CheckID. It is probably inconsistent on Consul’s API, but that is their API and will fail now in Consul 1.7.

ruslansennov commented 4 years ago

This bug concerns the case where the check is part of the registration of your service.

This bug has been fixed. Also, all tests passed with the consul 1.7.0 (which is released).

ncouse commented 4 years ago

I just tested with 3.8.5 (the latest release), and the bug is still there. I notice at least in 3.8.x that the error messages are better (than 3.7.0), as the body of the message was not reported, which meant it took me longer to track this down.

io.vertx.core.impl.NoStackTraceThrowable: Status message: 'Bad Request'. Body: 'Request decode failed: json: unknown field "ID"'
ncouse commented 4 years ago

I see there is an update on master (presumably for 4.x): https://github.com/vert-x3/vertx-consul-client/blob/master/src/main/java/io/vertx/ext/consul/impl/ConsulClientImpl.java#L450

I just tested with 4.0.0-milestone4 and get the same error. However a quick look at that tag doesn't show the change.

Is this fix going to be backported to the 3.x branches, as their currently is no release version that works now with Consul 1.7 (and we probably won't be immediately updating to 4.0 when released).

ruslansennov commented 4 years ago

Fixes will be available in 3.8.6, 3.9.0 etc

ruslansennov commented 4 years ago

You can try 3.8.6-SNAPSHOT version

ncouse commented 4 years ago

Yes, I managed to find your snapshots repo, and tested 4.0.0-SNAPSHOT, and 3.8.6-SNAPSHOT. Both work for my use case.

Thanks.