Add HTTPRedirectHandler for pushing to gateway #131

Activity

1. brian-brazil commented on Jan 25, 2017

   

   brian-brazil

   on Jan 25, 2017

   Contributor

   More actions

   I don't see a problem there, I'd suggest sending a PR after #126 is in.
2. brian-brazil

   closed this as completedon Mar 17, 2017
3. espears1 commented on Jan 29, 2021

   

   espears1

   on Jan 29, 2021

   Last edited by espears1

   Contributor

   More actions

   @brian-brazil is it possible to re-open this one? Also, do you know any reason why direct use of HTTPRedirectHandler wouldn't work? I am creating a handler from this class, yet I still see some HTTP 301 not getting handled.

   You had a comment previously on the creation of the default handler here: https://github.com/prometheus/client_python/pull/126/files#r97828751 - my attempt was literally to copy/paste that entire handler function and just swap in HTTPRedirectHandler but it doesn't work. Assuming I can figure out what I am doing wrong, I am happy to PR that into the client lib as a built-in redirect option.
4. csmarchbanks commented on Feb 2, 2021

   

   csmarchbanks

   on Feb 2, 2021

   Member

   More actions

   Hi @espears1, I am happy to reopen this as it seems like redirects are not working.

   I believe that HTTPRedirectHandler is actually one of the default classes used already, see the docs for more information on the default handlers used. I think the issue is that the HTTPRedirectHandler does not redirect PUT requests, and will change POST requests to GET which will not work for push gateway.

   I think the best solution would be to add another handler (like basic_auth_handler) that handles PUT and POST redirects including sending data. The reason for a separate handler is that it is encouraged (technically a MUST in RFC 2616) to have user confirm a redirect POST/PUT, and I would say a separate handler classifies as the user giving confirmation that a redirect is expected. Plus having the default handler continue to use python defaults would be nice. Happy to hear opposing views though.
5. csmarchbanks

   reopened this on Feb 2, 2021
6. espears1 commented on Feb 2, 2021

   

   espears1

   on Feb 2, 2021

   Last edited by espears1

   Contributor

   More actions

   @csmarchbanks re: #131 (comment)

   First of all thank you so much for looking into this and providing that extra context. That definitely helps with my question and I will do more research using that info.

   I did want to clarify what you mean by saying

   HTTPRedirectHandler is actually one of the default classes used already.

   I don't believe that's true for the prometheus client. As in the link above, I think there is just default_handler which hard-codes the use of HTTPHandler and then a separate one for basic_auth_handler. My initial idea was just to copy/paste default_handler and adjust the base handler class to use HTTPRedirectHandler, like this:

   def redirect_handler(url, method, timeout, headers, data):
   
       def handle():
           request = Request(url, data=data)
           request.get_method = lambda: method
           for k, v in headers:
               request.add_header(k, v)
           resp = build_opener(HTTPRedirectHandler).open(request, timeout=timeout)
           if resp.code >= 400:
               raise IOError("error talking to pushgateway: {0} {1}".format(
                   resp.code, resp.msg))
   
       return handle
   

   but I ran into exactly the issue you suggested, that PUT and the POST/GET issue seem to be preventing the actual redirect handling from succeeding.

   I just wanted to check if my understanding above is correct, or if instead you meant there is already some "out of the box" support for a redirect handler in prometheus_client?
7. csmarchbanks commented on Feb 3, 2021

   

   csmarchbanks

   on Feb 3, 2021

   Member

   More actions

   I was also a bit confused at first as I thought build_opener would only allow use HTTPHandler when called, but the docs actually say

   Instances of the following classes will be in front of the handlers,
   unless the handlers contain them, instances of them or subclasses of them: 
   ProxyHandler (if proxy settings are detected), UnknownHandler, HTTPHandler, 
   HTTPDefaultErrorHandler, HTTPRedirectHandler, FTPHandler, FileHandler, HTTPErrorProcessor.
   

   So I think all of those handlers are added in front of the HTTPHandler that we specify. Which also leads me to believe that specifying HTTPHandler doesn't actually do much.

   Here is the implementation of build_opener:

   def build_opener(*handlers):
       """Create an opener object from a list of handlers.
   
       The opener will use several default handlers, including support
       for HTTP, FTP and when applicable HTTPS.
   
       If any of the handlers passed as arguments are subclasses of the
       default handlers, the default handlers will not be used.
       """
       opener = OpenerDirector()
       default_classes = [ProxyHandler, UnknownHandler, HTTPHandler,
                          HTTPDefaultErrorHandler, HTTPRedirectHandler,
                          FTPHandler, FileHandler, HTTPErrorProcessor,
                          DataHandler]
       if hasattr(http.client, "HTTPSConnection"):
           default_classes.append(HTTPSHandler)
       skip = set()
       for klass in default_classes:
           for check in handlers:
               if isinstance(check, type):
                   if issubclass(check, klass):
                       skip.add(klass)
               elif isinstance(check, klass):
                   skip.add(klass)
       for klass in skip:
           default_classes.remove(klass)
   
       for klass in default_classes:
           opener.add_handler(klass())
   
       for h in handlers:
           if isinstance(h, type):
               h = h()
           opener.add_handler(h)
       return opener

   Where it looks to me like HTTPRedirectHandler will always be added unless you pass in a handler that is a subclass of HTTPRedirectHandler? Happy to be corrected if I am not tracing the code properly as it took me by surprise too :).
8. espears1 commented on Feb 3, 2021

   

   espears1

   on Feb 3, 2021

   Last edited by espears1

   Contributor

   More actions

   @csmarchbanks re: #131 (comment)

   Thank you - that clarifies my misconception and helps a lot. In that case, my current idea for a strategy is to create my own redirect handler class which inherits from BaseHandler, and basically just find a way to modify the method / error code logic here:

   • https://github.com/python/cpython/blob/master/Lib/urllib/request.py#L664

   Does that sound right according to your understanding of the problem, or is there an additional aspect of request method translation (e.g. put to post) that would have to be further handled in a different way?
9. csmarchbanks commented on Feb 3, 2021

   

   csmarchbanks

   on Feb 3, 2021

   Member

   More actions

   Sounds like the correct track to me. You could probably just inherit from HTTPRedirectHandler so that you keep the proper handlers for non-redirects.

   You will also need to make sure that https://github.com/python/cpython/blob/master/Lib/urllib/request.py#L682 passes the data and keeps the method the same as what it came in as.
10. espears1 commented on Feb 6, 2021

    

    espears1

    on Feb 6, 2021

    Contributor

    More actions

    @csmarchbanks I have checked out the repo and created a branch that I believe adds this functionality. Do I need special permissions or anything in order to push it and create a pull request? I get an access rights issue at the moment when I try to push. (I did not fork the repo, only cloned this one).

    Briefly the solution does this:

    • adds PrometheusRedirectHandler in utils.py, which subclasses HTTPRedirectHandler and makes the changes we mentioned above in order to pass through the method, data and headers.
    • adds a passthrough_redirect_handler helper function alongside basic_auth_handler in exposition.py (and a tiny refactor to make the code reuse a little better)
    • add test setup and a unit test in test_exposition.py to the TestPushGateway class, which tests the redirect functionality explicitly for a PUT and a simulated 301 response.

    Let me know how I can best submit it as a PR and then work to ensure it meets all the guidelines.
11. csmarchbanks commented on Feb 8, 2021

    

    csmarchbanks

    on Feb 8, 2021

    Member

    More actions

    Hello, the best way to submit a PR is to create a fork, push to a branch of your fork, and then create a PR from your fork to this one. The GitHub docs also have some information if needed. I look forward to your PR!
12. espears1 commented on Feb 8, 2021

    

    espears1

    on Feb 8, 2021

    Contributor

    More actions

    OK it is here: #622 - let me know what needs fixing.
13. csmarchbanks

    closed this as completedin #622on Feb 11, 2021