[CHEF-2682] Redirected API requests can result in confusing error messages

[LoicGombeaud]

http://tickets.opscode.com/browse/CHEF-2682

Added a warning when following a redirect with a different method than that of the original request; this solution:

1. preserves conformity with RFC 2616, which states the following:
   "If the 301 status code is received in response to a request other than GET or HEAD, the user agent MUST NOT automatically redirect the request unless it can be confirmed by the user, since this might change the conditions under which the request was issued."
2. effectively fixes the bug by providing a non-confusing (or at least, less-confusing) log message.

[danielsdeleo]

I'm in favor of raising an error instead. I'd imagine something like:

# error class, goes in chef/exceptions
class InvalidRedirect < StandardError; end

# Error raise, in chef/rest
unless [:GET, :HEAD].include?(method)
  raise Exceptions::InvalidRedirect, "#{method} request was redirected from #{url} to #{location}. Only GET and HEAD support redirects."
end

Then we can catch this specific error in the error handling code for knife and chef and add a message to verify the URL in the config.

[LoicGombeaud]

You are thus implicitly stating that a change in the HTTP method, except for a change from HEAD to GET, will always result in buggy behaviour, right ?
I'm not familiar enough with the Chef API to confirm or disconfirm that statement myself, but since you work at Opscode I fully trust that it is correct, so I'll go ahead and follow your advice - new pull request coming up !
However, I don't know how to handle the error in the clients...

[danielsdeleo]

I think we have this redirect to GET behavior only because of a misunderstanding of the RFCs. If I remember correctly, the HTTP 1.0 RFC that preceded 2616 was a bit confusing on the point of non-GET redirects.

That said, I haven't seen anything good come of this behavior, and given that we use Chef::REST with chef-client, which may run unattended, the only way we can truly conform to the RFC's (2616) specification of requesting user confirmation is to have the user change his/her configuration.

As for the error handling bit, the relevant code is here for knife:

https://github.com/opscode/chef/blob/master/lib/chef/knife.rb#L424-474

And here for chef-client (with chef-client, this bug is most commonly seen when chef does a POST to BASE_URL/clients to generate the key for a new node):

https://github.com/opscode/chef/blob/master/lib/chef/formatters/error_inspectors/registration_error_inspector.rb#L22-104

Thanks in advance, this will be a much nicer experience for everyone.

[seth]

Is it worth considering a config option to allow unconditionally following redirects? Sort of a way for the user to pre-accept a warning for non-GET redirect following.

[LoicGombeaud]

Steven indeed referred to RFC 2616 here: http://lists.opscode.com/sympa/arc/chef/2012-12/msg00217.html
And there's a long dissertation on the subject here (haven't read it through yet): http://www.alanflavell.org.uk/www/post-redirect.html

Just to be clear, here's the behaviour my next pull request will (attempt to) achieve:

1. follow GET and HEAD redirects with the same method than the original request;
2. raise an error on non-GET, non-HEAD redirects;
3. handle the error gracefully in knife and chef-client.
   Does this sound good to you ?

As for a config option, I'll leave that to the pros for now: I don't want to be too ambitious for my first contribution to an open-source project.

[danielsdeleo]

I'll comment here since this is where the discussion is.

I'm not convinced we can make error vs. warn a configurable option in a way that's both helpful and simple. We know for sure that all requests to a Chef server that get redirected to a GET will result in a failure. It's possible that users of the http_request resource might be depending on this behavior, and the change will break them. For that reason I'm happy to only apply the change to Chef 11 and add a note in the breaking changes documentation.

The only scenario I can think of where this completely removes functionality is if a webservice is dynamically generating redirects, but only for PUT/POST/DELETE, and the result of the redirect is expected to be a GET. Otherwise, the user can simply change the HTTP method to GET in order to have the redirect followed.

[btm]

#536 got merged, so I think we're all set.