Skip to content
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

Following redirects on HTTP Checks #20

Open
maxstepanov opened this issue Jan 26, 2014 · 13 comments · May be fixed by #671
Open

Following redirects on HTTP Checks #20

maxstepanov opened this issue Jan 26, 2014 · 13 comments · May be fixed by #671
Labels

Comments

@maxstepanov
Copy link

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.

@jirutka
Copy link
Contributor

jirutka commented Jan 26, 2014

Why not?

@dbuxton
Copy link
Contributor

dbuxton commented Jan 26, 2014

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).

@maxstepanov
Copy link
Author

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.

@maxstepanov
Copy link
Author

@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.

@dbuxton
Copy link
Contributor

dbuxton commented Jan 26, 2014

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.

@gdubicki
Copy link
Contributor

@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.

@dbuxton
Copy link
Contributor

dbuxton commented Oct 13, 2014

@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.

@dbuxton dbuxton added bug and removed enhancement labels Oct 13, 2014
@gdubicki
Copy link
Contributor

@dbuxton Ok, I'll take a look at it.

@gdubicki
Copy link
Contributor

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 ➡️ 301 ➡️ ... ➡️ 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?

@dbuxton
Copy link
Contributor

dbuxton commented Oct 16, 2014

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.

@gdubicki
Copy link
Contributor

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.

@davidjb
Copy link
Contributor

davidjb commented Nov 9, 2017

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.

@venkataramanab
Copy link

Any updates on this thread??

thomasleveil pushed a commit to mediaveille/cabot that referenced this issue Jun 11, 2019
…redirects

fix arachnys#20

- The setting is per-check
- default is 'checked' which means Cabot does follow redirects (as before this change)

- [x] db migration
- [x] views
- [x] REST api
thomasleveil pushed a commit to mediaveille/cabot that referenced this issue Jun 11, 2019
…redirects

fix arachnys#20

- The setting is per-check
- default is 'checked' which means Cabot does follow redirects (as before this change)

- [x] db migration
- [x] views
- [x] REST api
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants