zalando-zmon / zmon-worker

ZMON Python Worker
https://zmon.io/
Other
19 stars 41 forks source link

Add an option to disable redirects in "http" check command. #51

Closed dmitrykrivaltsevich closed 8 years ago

dmitrykrivaltsevich commented 8 years ago

For the check defined like

def check():
  status_code = http('https://service.dns.name/file/very-large-file.zip').code()
  return {"status_code": status_code}

I would like to have status_code = 302 when the server returns redirect. The reason is that I only need to check that the very-large-file.zip is accessible, but I don't want to download this file in ZMON.

Currently this check returns status_code = 200, which means that ZMON follow redirects. Would be good to somehow disable such behaviour.

hjacobs commented 8 years ago

@mohabusama maybe you can come up with a reasonable solution? Possibilities I see:

dmitrykrivaltsevich commented 8 years ago

The ideal solution would include both options. In our service we generate URLs and then we redirect users to these URLs. Ideally the check will look like

redirect_url = http('https://our.service/file/large-file').headers()['Location'] code = http(redirect_url, method='HEAD').code() ... check that code == 200

So that we would verify that our service generates correct URLs (so this part of user journey just doesn't fail) and then we can check that the file exists on destination server (which is AWS S3).

mohabusama commented 8 years ago

A quick test with requests lib, I can see that HEAD doesn't follow redirects by default (unlike GET). But that can be overridden in both cases.

So, I guess going with implementing both options would give more flexibility. The question is, should we keep the default behavior of HEAD, as in not to follow redirects? will it be confusing?

@hjacobs @dmitrykrivaltsevich

hjacobs commented 8 years ago

I guess introducing both method (only allow HEAD and GET!) and allow_redirects (defaults to True for GET and False for HEAD) makes sense.

mohabusama commented 8 years ago

I think this approach will lead to a missing use case, which is method=HEAD and allow_redirects=True, since http initialization will implicitly change allow_redirects to False in case of head.

I was thinking of making it more explicit, by introducing a method head, that way we can override default behavior in this method. So it will look like this

def head(allow_redirects=False)

http('https://some-url/file.zip').head().headers()['Location']
# or
http('https://some-url/file.zip').head(allow_redirects=True).code()
hjacobs commented 8 years ago

@mohabusama I would not introduce a new method head as it's not consistent with the existing code (all other methods are "response" methods and the actual http() call already "does" the request (not completely true as under the hood the request is made on-demand)).

I would rather add more params, like:

http('/foobar', method='HEAD', allow_redirects=True).code()

This makes http more look like the underlying requests.request function.

dmitrykrivaltsevich commented 8 years ago

@hjacobs, @mohabusama

I'd support the variant with method and allow_redirects parameter if following statement will be True when http://site/foobar returns redirect:

http('/foobar', method='GET', allow_redirects=False).code() == 302

Quick question, who will terminate (possibly) infinite loop when http('/foo', allow_redirects=True) gets a redirect to http://site/bar and http://site/bar returns a redirect to http://site/foo?

mohabusama commented 8 years ago

There is a max_redirects which is default to 30.