cabot
cabot copied to clipboard
arachnys
Following redirects on HTTP Checks
Didn't expect http checks to follow redirects, but they do. I don't see any option to disable this. I think by default check should never follow http redirects, at least that's what i'm used to.
I don't have a particularly strong opinion on this but there's no particular problem with making it configurable. It's just an argument to the underlying call to requests.get. Feel free to send a PR. I'd suggest a BooleanField on the model that defaults to True (to make sure it doesn't break existing installs; I note this isn't your preferred option but doesn't seem unreasonable).
Because i'm checking for 200 but endpoint returns 3xx. This should fail because i don't care what's on the other end of the redirect. There obviously should be a switch option for this.
@dbuxton I have 0 python experience, sorry. I can read code just fine, but writing is another story. So the only way i can help is by opening issues. Sorry. I have other stuff, just don't want to spam issues... Having an ability to specify several return codes and wild cars would be nice also Logging for notifications. There are none. Had to modify code to print debugging info by hand coz i had problems. :/ The whole fab/ubuntu thing is not useful for me at all(i'm running in docker), but better than nothing for a reference. Twilio complains on format. Just a warning though... Thanks for the project.
Well @maxstepanov feel free to break those out as separate issues and I'm sure if someone else feels your pain they'll give you a hand! Some of those sound like things we'd want to implement (multiple return codes), others are obvious issues (making nice logging is often challenging).
What's the Twilio warning? Haven't seen that I don't think.
@maxstepanov I have exactly opposite use case - I check for 302, get 200 after redirect and receive an error.
@dbuxton Current Cabot behaviour means that HTTP checks expecting redirection status codes (302, 301) will NEVER work properly. I think that's not a lack of feature (configurability) but a bug.
@grzegorz-dubicki sounds like a bug. I'm a bit snowed at the moment so if you have time to have a look that would be appreciated but if not I'm aware and (on the face of it) I agree.
I think we can do 2 things here (and by that I also mean I can implement it and make a PR):
1. Fix check's not working for codes 301/302 without breaking compatibility with existing installations
For that let's assume that a HTTP check succeeds when its expected status code matches the status code of the final response OR the first response of a chain of requests (it there was a chain).
So for example if my check expects 302 and my response chain is: 302 :arrow_right: 301 :arrow_right: ... :arrow_right: 200 then my check succeeds because first response code matches. It will also succeed if my check expects 200.
We can easily use requests history feature to implement this.
Advantages of this change is that this will keep checks "just working" for most users and will not break compatibility with existing installations.
A possible disadvantage of this change is that is not a very obvious behavior and this may break Cabot's simplicity promise.. Also we will not fix @maxstepanov problem this way.
For that we need to
2. Make HTTP checks globally or locally configurable to follow/not follow the redirections.
Of cource this should be disabled by default, as @dbuxton has written above.
I that was my decision then I would be leaning towards global configurability here to KISS & make up for the additional complexity of the change described above. :)
Also, and more seriously, apparently this feature is not very much needed if it was not touched / requested by anyone for the last ~9 months so the simpler global approach will probably be enough.
@dbuxton What do you think? Should I implement it both?
I think I favour having local configuration on a per-check basis to make it really clear what's going on - otherwise the simplicity of programming will be offset by the complexity of debugging for users!
Option (1) seems like very tricky behaviour to understand, although I didn't realise that requests had history. Very cool.
It's not a huge change, just a new flag on the StatusCheck model which we then pass through to requests - I suggest we do that way.
Ok, I'll try to do it this way.
Grzegorz Dubicki http://about.me/grzegorzdubicki
Dnia 16 paź 2014 o godz. 19:37 David Buxton [email protected] napisał(a):
I think I favour having local configuration on a per-check basis to make it really clear what's going on - otherwise the simplicity of programming will be offset by the complexity of debugging for users!
Option (1) seems like very tricky behaviour to understand, although I didn't realise that requests had history. Very cool.
It's not a huge change, just a new flag on the StatusCheck model which we then pass through to requests - I suggest we do that way.
— Reply to this email directly or view it on GitHub.
Any updates on this one? I was looking at setting up checks to specifically test for a redirect being made (eg a http:// URL returning a 301 to https://) so knowing a 200 is the final result doesn't quite help with that.
