ready? method for remote_resource.as_json is always true

[kamaradclimber]

To reproduce, build a simple template to an endpoint that returns a 401 with json content type (such as apache marathon api with authentication). The template will be considered ready immediately.

The cause is https://github.com/criteo/consul-templaterb/blob/master/lib/consul/async/json_endpoint.rb#L205 where

enforce_json_200 && !(200..299).cover?(http.response_header.status) && http.response_header['Content-Type'] != 'application/json'

should probably be replaced by:

enforce_json_200 && !(200..299).cover?(http.response_header.status) || http.response_header['Content-Type'] != 'application/json'

The logic should be: if the response is not a 200 OK or is not advertize as json, then it's an error.

This bug leads to consider template as immediately ready.

[kamaradclimber]

The reason I'm not submitting a PR is that I wonder how this could break some template.

[pierresouchay]

@kamaradclimber I think you are correct regarding your patch from some POV.

Still, I would also add a parenthesis to be sure (I hate operator priorities):

if (enforce_json_200 && !(200..299).cover?(http.response_header.status)) || http.response_header['Content-Type'] != 'application/json'

The fun part about that is that this test was added exactly to solve the marathon issue (IIRC): I wanted it to converge when the reverse proxy was not serving that page and being able to still display some stuff by handling the error myself, because I when retry is set, the template never converges. And was thinking that anyway having an HTTP 401 would never converge, so, let's fail fast. So, basically, you want to change the behavior for the same use case, but you expect a different behavior :-) (which might be legit)

The question here is what is the semantic of enforce_json_200, with the current implementation:

• enforce_json_200 => Fails if HTTP Code != 2XX OR Content-Type is not JSON

This means that if enforce_json_200 is not set, it will return the result if the content-type is "text/plain".

With your patch, you are changing the semantics, so in the real world, this patch would break the templates of people having JSON data served with "application/javascript" or "text/javascript" for instance.

This is very likely to break some people's code, so most likely I would keep it that way.

However, it might be possible to add additional options:

enforce_json_mime_type => ensures that Content-Type is "application/json" and fails whenever enforce_json_200 has been set or not.

Globally, I taking the RETRY stuff of consul templaterb should be avoided for errors that are not transient (I mean, in such case, the response will always be HTTP 401, so IMHO, triggering RETRY is not a good idea).

So, to sum up:

• IMHO, I would avoid this patch as it breaks the mime-type stuff
• I would add another opt-in option to validate mime-type validation whenever the response is 2XX or 4XX
• maybe for your use case (I mean if you want ready? to be false in case of HTTP 4XX), then I would add the possibility to add a lambda to evaluate some code:

def should_retry_default(json_endpoint, http):
   false
end

def should_retry_enforce_json200(json_endpoint, http):
  json_endpoint.enforce_json_200 && !(200..299).cover?(http.response_header.status)) || http.response_header['Content-Type'] != 'application/json'
end

In the constructor of JSONEndpoint, I would add the following:

@retry_evaluator = should_retry_default
@retry_evaluator = should_retry_enforce_json200 if enforce_json_200
if retry_evaluator:
  @retry_evaluator = retry_evaluator
  raise "enforce_json_200 and retry_evaluator are exclusive" if enforce_json_200
end

and would change the retry stuff with:

         http.callback do
            if @retry_evaluator(self, http)
              _handle_error(http) do
                warn "[RETRY][#{url}] (#{@consecutive_errors} errors)" if (@consecutive_errors % 10) == 1
              end
            else

something like that (possibly adding other default evaluator methods like validation of mime-type only whatever the HTTP return code...

So, you would be free to implement your own logic when adding the endpoint while not breaking existing compatibility.