-
Notifications
You must be signed in to change notification settings - Fork 29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ready?
method for remote_resource.as_json is always true
#83
Comments
The reason I'm not submitting a PR is that I wonder how this could break some template. |
@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
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:
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 @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. |
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
should probably be replaced by:
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.
The text was updated successfully, but these errors were encountered: